diff --git a/CHANGES.txt b/CHANGES.txt index 09e1471b33..ff466d831d 100644 --- a/CHANGES.txt +++ b/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) diff --git a/NEWS.txt b/NEWS.txt index cd13c996d4..d5a05a77a4 100644 --- a/NEWS.txt +++ b/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. diff --git a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java index 646a1193da..19b7d2a70d 100644 --- a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java +++ b/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) diff --git a/src/java/org/apache/cassandra/config/Config.java b/src/java/org/apache/cassandra/config/Config.java index c39d9daaae..1dedb6b817 100644 --- a/src/java/org/apache/cassandra/config/Config.java +++ b/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 SENSITIVE_KEYS = new ArrayList() {{ + private static final Set SENSITIVE_KEYS = new HashSet() {{ add("client_encryption_options"); add("server_encryption_options"); }}; diff --git a/src/java/org/apache/cassandra/config/Replacement.java b/src/java/org/apache/cassandra/config/Replacement.java index b3fb330143..a201b63461 100644 --- a/src/java/org/apache/cassandra/config/Replacement.java +++ b/src/java/org/apache/cassandra/config/Replacement.java @@ -83,4 +83,9 @@ public final class Replacement } }; } + + public boolean isValueFormatReplacement() + { + return oldName.equals(newName); + } } diff --git a/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java b/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java index 1b4520754f..0e147aaff0 100644 --- a/src/java/org/apache/cassandra/config/YamlConfigurationLoader.java +++ b/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, Map> 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, Map> replacements, Map rawConfig) + { + List duplicates = new ArrayList<>(); + for (Map.Entry, Map> outerEntry : replacements.entrySet()) + { + for (Map.Entry 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, Map> replacements, byte[] configBytes) + { + LoaderOptions loaderOptions = new LoaderOptions(); + loaderOptions.setAllowDuplicateKeys(ALLOW_DUPLICATE_CONFIG_KEYS.getBoolean()); + Yaml rawYaml = new Yaml(loaderOptions); + + Map 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, Map> 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 updateFromMap(Map map, boolean shouldCheck, T obj) + public static T updateFromMap(Map map, boolean shouldCheck, T obj) { Class klass = (Class) obj.getClass(); Constructor constructor = new YamlConfigurationLoader.CustomConstructor(klass, klass.getClassLoader()) @@ -253,6 +295,7 @@ public class YamlConfigurationLoader implements ConfigurationLoader } }; Map, Map> 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); } } - - } diff --git a/test/unit/org/apache/cassandra/config/FailStartupDuplicateParamsTest.java b/test/unit/org/apache/cassandra/config/FailStartupDuplicateParamsTest.java new file mode 100644 index 0000000000..073ff8f9b1 --- /dev/null +++ b/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 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 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 exceptionMsgPredicate, boolean expectFailure, String message, String ... toAdd) throws IOException + { + Path p = Files.createTempFile("config_dupes",".yaml"); + try + { + List 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 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); + } +}