Fail starting when the same parameter exists more than once in cassandra.yaml

Patch by marcuse; reviewed by David Capwell and Ekaterina Dimitrova for CASSANDRA-17379

CHANGES.txt

@@ -1,4 +1,5 @@
 4.1
+ * Allow failing startup on duplicate config keys (CASSANDRA-17379)
  * Migrate threshold for minimum keyspace replication factor to guardrails (CASSANDRA-17212)
  * Add guardrail to disallow TRUNCATE and DROP TABLE commands (CASSANDRA-17558)
  * Add plugin support for CQLSH (CASSANDRA-16456)

NEWS.txt

@@ -175,6 +175,9 @@ Upgrading
       parameters. List of changed parameters and details to consider during configuration setup can be
       found at https://cassandra.apache.org/doc/latest/cassandra/new/configuration.html. (CASSANDRA-15234)
       Backward compatibility with the old cassandra.yaml file will be in place until at least the next major version.
+      By default we refuse starting Cassandra with a config containing both old and new config keys for the same parameter. Start
+      Cassandra with -Dcassandra.allow_new_old_config_keys=true to override. For historical reasons duplicate config keys
+      in cassandra.yaml are allowed by default, start Cassandra with -Dcassandra.allow_duplicate_config_keys=false to disallow this.
     - Many cassandra.yaml parameters' names have been changed. Full list and details to consider during configuration setup
       when installing/upgrading Cassandra can be found at https://cassandra.apache.org/doc/latest/cassandra/new/configuration.html (CASSANDRA-15234)
     - Negative values cannot be used for parameters of type data rate, duration and data storage with both old and new cassandra.yaml version.

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

@@ -238,6 +238,11 @@ public enum CassandraRelevantProperties
 
     PAXOS_REPAIR_RETRY_TIMEOUT_IN_MS("cassandra.paxos_repair_retry_timeout_millis", "60000"),
 
+    /** If we should allow having duplicate keys in the config file, default to true for legacy reasons */
+    ALLOW_DUPLICATE_CONFIG_KEYS("cassandra.allow_duplicate_config_keys", "true"),
+    /** If we should allow having both new (post CASSANDRA-15234) and old config keys for the same config item in the yaml */
+    ALLOW_NEW_OLD_CONFIG_KEYS("cassandra.allow_new_old_config_keys", "false"),
+
     // startup checks properties
     LIBJEMALLOC("cassandra.libjemalloc"),
     @Deprecated // should be removed in favor of enable flag of relevant startup check (checkDatacenter)

src/java/org/apache/cassandra/config/Config.java

@@ -19,10 +19,9 @@ package org.apache.cassandra.config;
 
 import java.lang.reflect.Field;
 import java.lang.reflect.Modifier;
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.List;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeMap;
@@ -1113,7 +1112,7 @@ public class Config
         exception
     }
 
-    private static final List<String> SENSITIVE_KEYS = new ArrayList<String>() {{
+    private static final Set<String> SENSITIVE_KEYS = new HashSet<String>() {{
         add("client_encryption_options");
         add("server_encryption_options");
     }};

src/java/org/apache/cassandra/config/Replacement.java

@@ -83,4 +83,9 @@ public final class Replacement
             }
         };
     }
+
+    public boolean isValueFormatReplacement()
+    {
+        return oldName.equals(newName);
+    }
 }

src/java/org/apache/cassandra/config/YamlConfigurationLoader.java

@@ -23,6 +23,7 @@ import java.io.InputStream;
 import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.Paths;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -43,6 +44,7 @@ import org.slf4j.LoggerFactory;
 
 import org.apache.cassandra.exceptions.ConfigurationException;
 import org.apache.cassandra.io.util.File;
+import org.yaml.snakeyaml.LoaderOptions;
 import org.yaml.snakeyaml.TypeDescription;
 import org.yaml.snakeyaml.Yaml;
 import org.yaml.snakeyaml.composer.Composer;
@@ -54,6 +56,8 @@ import org.yaml.snakeyaml.introspector.Property;
 import org.yaml.snakeyaml.introspector.PropertyUtils;
 import org.yaml.snakeyaml.nodes.Node;
 
+import static org.apache.cassandra.config.CassandraRelevantProperties.ALLOW_DUPLICATE_CONFIG_KEYS;
+import static org.apache.cassandra.config.CassandraRelevantProperties.ALLOW_NEW_OLD_CONFIG_KEYS;
 import static org.apache.cassandra.config.Replacements.getNameReplacements;
 
 public class YamlConfigurationLoader implements ConfigurationLoader
@@ -70,7 +74,6 @@ public class YamlConfigurationLoader implements ConfigurationLoader
     /**
      * Inspect the classpath to find storage configuration file
      */
-    @VisibleForTesting
     private static URL getStorageConfigURL() throws ConfigurationException
     {
         String configUrl = System.getProperty("cassandra.config");
@@ -135,6 +138,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
 
             Constructor constructor = new CustomConstructor(Config.class, Yaml.class.getClassLoader());
             Map<Class<?>, Map<String, Replacement>> replacements = getNameReplacements(Config.class);
+            verifyReplacements(replacements, configBytes);
             PropertiesChecker propertiesChecker = new PropertiesChecker(replacements);
             constructor.setPropertyUtils(propertiesChecker);
             Yaml yaml = new Yaml(constructor);
@@ -169,6 +173,43 @@ public class YamlConfigurationLoader implements ConfigurationLoader
         }
     }
 
+    private static void verifyReplacements(Map<Class<?>, Map<String, Replacement>> replacements, Map<String, ?> rawConfig)
+    {
+        List<String> duplicates = new ArrayList<>();
+        for (Map.Entry<Class<?>, Map<String, Replacement>> outerEntry : replacements.entrySet())
+        {
+            for (Map.Entry<String, Replacement> entry : outerEntry.getValue().entrySet())
+            {
+                Replacement r = entry.getValue();
+                if (!r.isValueFormatReplacement() && rawConfig.containsKey(r.oldName) && rawConfig.containsKey(r.newName))
+                {
+                    String msg = String.format("[%s -> %s]", r.oldName, r.newName);
+                    duplicates.add(msg);
+                }
+            }
+        }
+
+        if (!duplicates.isEmpty())
+        {
+            String msg = String.format("Config contains both old and new keys for the same configuration parameters, migrate old -> new: %s", String.join(", ", duplicates));
+            if (!ALLOW_NEW_OLD_CONFIG_KEYS.getBoolean())
+                throw new ConfigurationException(msg);
+            else
+                logger.warn(msg);
+        }
+    }
+
+    private static void verifyReplacements(Map<Class<?>, Map<String, Replacement>> replacements, byte[] configBytes)
+    {
+        LoaderOptions loaderOptions = new LoaderOptions();
+        loaderOptions.setAllowDuplicateKeys(ALLOW_DUPLICATE_CONFIG_KEYS.getBoolean());
+        Yaml rawYaml = new Yaml(loaderOptions);
+
+        Map<String, Object> rawConfig = rawYaml.load(new ByteArrayInputStream(configBytes));
+        verifyReplacements(replacements, rawConfig);
+
+    }
+
     private static String readStorageConfig(URL url)
     {
         String content;
@@ -220,6 +261,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
     {
         Constructor constructor = new YamlConfigurationLoader.CustomConstructor(klass, klass.getClassLoader());
         Map<Class<?>, Map<String, Replacement>> replacements = getNameReplacements(Config.class);
+        verifyReplacements(replacements, map);
         YamlConfigurationLoader.PropertiesChecker propertiesChecker = new YamlConfigurationLoader.PropertiesChecker(replacements);
         constructor.setPropertyUtils(propertiesChecker);
         Yaml yaml = new Yaml(constructor);
@@ -239,7 +281,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
         return value;
     }
 
-    public static <T> T updateFromMap(Map<String, ? extends Object> map, boolean shouldCheck, T obj)
+    public static <T> T updateFromMap(Map<String, ?> map, boolean shouldCheck, T obj)
     {
         Class<T> klass = (Class<T>) obj.getClass();
         Constructor constructor = new YamlConfigurationLoader.CustomConstructor(klass, klass.getClassLoader())
@@ -253,6 +295,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader
             }
         };
         Map<Class<?>, Map<String, Replacement>> replacements = getNameReplacements(Config.class);
+        verifyReplacements(replacements, map);
         YamlConfigurationLoader.PropertiesChecker propertiesChecker = new YamlConfigurationLoader.PropertiesChecker(replacements);
         constructor.setPropertyUtils(propertiesChecker);
         Yaml yaml = new Yaml(constructor);
@@ -420,7 +463,5 @@ public class YamlConfigurationLoader implements ConfigurationLoader
                 logger.warn("{} parameters have been deprecated. They have new names and/or value format; For more information, please refer to NEWS.txt", deprecationWarnings);
         }
     }
-
-
 }
 

test/unit/org/apache/cassandra/config/FailStartupDuplicateParamsTest.java

@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.config;
+
+import java.io.IOException;
+import java.net.URISyntaxException;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.function.Predicate;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.cassandra.config.CassandraRelevantProperties.ALLOW_DUPLICATE_CONFIG_KEYS;
+import static org.apache.cassandra.config.CassandraRelevantProperties.ALLOW_NEW_OLD_CONFIG_KEYS;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+
+public class FailStartupDuplicateParamsTest
+{
+    private static final List<String> baseConfig = ImmutableList.of(
+        "cluster_name: Test Cluster",
+        "commitlog_sync: batch",
+        "commitlog_directory: build/test/cassandra/commitlog",
+        "hints_directory: build/test/cassandra/hints",
+        "partitioner: org.apache.cassandra.dht.ByteOrderedPartitioner",
+        "saved_caches_directory: build/test/cassandra/saved_caches",
+        "data_file_directories:",
+        "   - build/test/cassandra/data",
+        "seed_provider:" ,
+        "   - class_name: org.apache.cassandra.locator.SimpleSeedProvider",
+        "parameters:",
+        "   - seeds: \"127.0.0.1:7012\"",
+        "endpoint_snitch: org.apache.cassandra.locator.SimpleSnitch");
+
+    @Before
+    public void before()
+    {
+        ALLOW_DUPLICATE_CONFIG_KEYS.setBoolean(true);
+        ALLOW_NEW_OLD_CONFIG_KEYS.setBoolean(false);
+    }
+
+    @Test
+    public void testDuplicateParamThrows() throws IOException
+    {
+        ALLOW_DUPLICATE_CONFIG_KEYS.setBoolean(false);
+        testYaml("found duplicate key endpoint_snitch", true,
+                 "endpoint_snitch: org.apache.cassandra.locator.RackInferringSnitch");
+    }
+
+    @Test
+    public void testReplacementDupesOldFirst() throws IOException
+    {
+        testYaml("[enable_user_defined_functions -> user_defined_functions_enabled]", true,
+                 "enable_user_defined_functions: true",
+                 "user_defined_functions_enabled: false");
+
+        testYaml("[enable_user_defined_functions -> user_defined_functions_enabled]", true,
+                 "enable_user_defined_functions: true",
+                 "user_defined_functions_enabled: true");
+    }
+
+    @Test
+    public void testReplacementDupesNewFirst() throws IOException
+    {
+        testYaml("[enable_user_defined_functions -> user_defined_functions_enabled]", true,
+                 "user_defined_functions_enabled: false",
+                 "enable_user_defined_functions: true");
+
+    }
+
+    @Test
+    public void testReplacementDupesMultiReplace() throws IOException
+    {
+        /*
+        @Replaces(oldName = "internode_socket_send_buffer_size_in_bytes", converter = Converters.BYTES_DATASTORAGE, deprecated = true)
+        @Replaces(oldName = "internode_send_buff_size_in_bytes", converter = Converters.BYTES_DATASTORAGE, deprecated = true)
+        public DataStorageSpec internode_socket_send_buffer_size = new DataStorageSpec("0B");
+        */
+        Predicate<String> predicate = (s) -> s.contains("[internode_send_buff_size_in_bytes -> internode_socket_send_buffer_size]") &&
+                                             s.contains("[internode_socket_send_buffer_size_in_bytes -> internode_socket_send_buffer_size]");
+        String message = " does not contain both [internode_send_buff_size_in_bytes] and [internode_socket_send_buffer_size_in_bytes]";
+
+        testYaml(predicate, true,
+                 message,
+                 "internode_send_buff_size_in_bytes: 3",
+                 "internode_socket_send_buffer_size_in_bytes: 2",
+                 "internode_socket_send_buffer_size: 5B");
+
+        // and new first:
+        testYaml(predicate, true,
+                 message,
+                 "internode_socket_send_buffer_size: 5B",
+                 "internode_socket_send_buffer_size_in_bytes: 2",
+                 "internode_send_buff_size_in_bytes: 3");
+    }
+
+    private static void testYaml(String expected, boolean expectFailure, String ... toAdd) throws IOException
+    {
+        testYaml((s) -> s.contains(expected), expectFailure, "does not contain [" + expected + ']', toAdd);
+    }
+
+    private static void testYaml(Predicate<String> exceptionMsgPredicate, boolean expectFailure, String message, String ... toAdd) throws IOException
+    {
+        Path p = Files.createTempFile("config_dupes",".yaml");
+        try
+        {
+            List<String> lines = new ArrayList<>(baseConfig);
+            Collections.addAll(lines, toAdd);
+            Files.write(p, lines);
+            loadConfig(p.toUri().toURL(), message, exceptionMsgPredicate, expectFailure);
+        }
+        finally
+        {
+            Files.delete(p);
+        }
+    }
+
+    private static void loadConfig(URL config, String message, Predicate<String> exceptionMsgPredicate, boolean expectFailure)
+    {
+        try
+        {
+            new YamlConfigurationLoader().loadConfig(config);
+        }
+        catch (Exception e)
+        {
+            assertTrue(expectFailure);
+            e.printStackTrace(System.out);
+            Throwable t = e;
+            do
+            {
+                if (exceptionMsgPredicate.test(t.getMessage()))
+                    return;
+                t = t.getCause();
+            } while (t != null);
+
+            fail("Message\n["+e.getMessage()+ "]\n"+message);
+        }
+        assertFalse(expectFailure);
+    }
+}