This is an automated email from the ASF dual-hosted git repository.
marcuse pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/cassandra.git
The following commit(s) were added to refs/heads/trunk by this push:
new 7ce140bd1d Fail starting when the same parameter exists more than once
in cassandra.yaml
7ce140bd1d is described below
commit 7ce140bd1dea311b9f98cdfbcd07dcff9fbd457c
Author: Marcus Eriksson <[email protected]>
AuthorDate: Thu Apr 7 14:43:40 2022 +0200
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 +
NEWS.txt | 3 +
.../config/CassandraRelevantProperties.java | 5 +
src/java/org/apache/cassandra/config/Config.java | 5 +-
.../org/apache/cassandra/config/Replacement.java | 5 +
.../cassandra/config/YamlConfigurationLoader.java | 49 +++++-
.../config/FailStartupDuplicateParamsTest.java | 167 +++++++++++++++++++++
7 files changed, 228 insertions(+), 7 deletions(-)
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<String> SENSITIVE_KEYS = new ArrayList<String>()
{{
+ private static final Set<String> SENSITIVE_KEYS = new HashSet<String>() {{
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<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);
}
}
-
-
}
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<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);
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
