From 61959e215c91d837c9dd67e65976456d74f93700 Mon Sep 17 00:00:00 2001 From: Aparna Naik Date: Fri, 26 Sep 2025 13:43:32 -0700 Subject: [PATCH] Add DDL Guardrail enabling administrators to disallow creation/modification of keyspaces with durable_writes = false patch by Aparna Naik; reviewed by Caleb Rackliffe and Stefan Miklosovic for CASSANDRA-20913 --- CHANGES.txt | 1 + NEWS.txt | 1 + conf/cassandra.yaml | 5 + .../org/apache/cassandra/config/Config.java | 3 + .../cassandra/config/GuardrailsOptions.java | 81 +++++- .../schema/AlterKeyspaceStatement.java | 8 + .../schema/CreateKeyspaceStatement.java | 2 + .../statements/schema/KeyspaceAttributes.java | 13 + .../cassandra/db/guardrails/Guardrails.java | 98 +++++++ .../db/guardrails/GuardrailsConfig.java | 15 ++ .../db/guardrails/GuardrailsMBean.java | 60 +++++ .../GuardrailKeyspacePropertiesTest.java | 247 ++++++++++++++++++ .../GuardrailTablePropertiesTest.java | 21 +- .../GuardrailsConfigCommandsTest.java | 3 + 14 files changed, 553 insertions(+), 5 deletions(-) create mode 100644 test/unit/org/apache/cassandra/db/guardrails/GuardrailKeyspacePropertiesTest.java diff --git a/CHANGES.txt b/CHANGES.txt index f8502538fb..f5c4a1fc74 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 5.1 + * Add DDL Guardrail enabling administrators to disallow creation/modification of keyspaces with durable_writes = false (CASSANDRA-20913) * Optimize Counter, Meter and Histogram metrics using thread local counters (CASSANDRA-20250) * Update snakeyaml to 2.4 (CASSANDRA-20928) * Update Netty to 4.1.125.Final (CASSANDRA-20925) diff --git a/NEWS.txt b/NEWS.txt index b3c525c213..f6e04f0223 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -96,6 +96,7 @@ New features More updates and documentation to follow. - New Guardrails added: - Whether bulk loading of SSTables is allowed. + - Allowed keyspace properties. - nodetool tpstats can display core pool size, max pool size and max tasks queued if --verbose / -v flag is specified. system_views.thread_pools adds core_pool_size, max_pool_size and max_tasks_queued columns. - Authentication mode is exposed in system_views.clients table, nodetool clientstats and ClientMetrics diff --git a/conf/cassandra.yaml b/conf/cassandra.yaml index cfa1b7ff9e..0147adc74b 100644 --- a/conf/cassandra.yaml +++ b/conf/cassandra.yaml @@ -2273,6 +2273,11 @@ drop_compact_storage_enabled: false # table_properties_ignored: [] # table_properties_disallowed: [] # +# Guardrail to warn about, ignore or reject properties when creating or modifying keyspaces. By default all properties are allowed. +# keyspace_properties_warned: [] +# keyspace_properties_ignored: [] +# keyspace_properties_disallowed: [] + # Guardrail to allow/disallow user-provided timestamps. Defaults to true. # user_timestamps_enabled: true # diff --git a/src/java/org/apache/cassandra/config/Config.java b/src/java/org/apache/cassandra/config/Config.java index e931b5a9d9..9783e086ad 100644 --- a/src/java/org/apache/cassandra/config/Config.java +++ b/src/java/org/apache/cassandra/config/Config.java @@ -908,6 +908,9 @@ public class Config public volatile Set table_properties_warned = Collections.emptySet(); public volatile Set table_properties_ignored = Collections.emptySet(); public volatile Set table_properties_disallowed = Collections.emptySet(); + public volatile Set keyspace_properties_warned = Collections.emptySet(); + public volatile Set keyspace_properties_ignored = Collections.emptySet(); + public volatile Set keyspace_properties_disallowed = Collections.emptySet(); public volatile Set read_consistency_levels_warned = Collections.emptySet(); public volatile Set read_consistency_levels_disallowed = Collections.emptySet(); public volatile Set write_consistency_levels_warned = Collections.emptySet(); diff --git a/src/java/org/apache/cassandra/config/GuardrailsOptions.java b/src/java/org/apache/cassandra/config/GuardrailsOptions.java index 1c2d5a513a..2d9aad8224 100644 --- a/src/java/org/apache/cassandra/config/GuardrailsOptions.java +++ b/src/java/org/apache/cassandra/config/GuardrailsOptions.java @@ -28,6 +28,7 @@ import com.google.common.collect.Sets; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.cassandra.cql3.statements.schema.KeyspaceAttributes; import org.apache.cassandra.cql3.statements.schema.TableAttributes; import org.apache.cassandra.db.ConsistencyLevel; import org.apache.cassandra.db.guardrails.CustomGuardrailConfig; @@ -37,6 +38,7 @@ import org.apache.cassandra.db.guardrails.ValueGenerator; import org.apache.cassandra.db.guardrails.ValueValidator; import org.apache.cassandra.io.util.FileUtils; import org.apache.cassandra.service.disk.usage.DiskUsageMonitor; +import org.apache.cassandra.utils.LocalizeString; import static java.lang.String.format; import static java.util.stream.Collectors.toSet; @@ -72,6 +74,9 @@ public class GuardrailsOptions implements GuardrailsConfig config.table_properties_warned = validateTableProperties(config.table_properties_warned, "table_properties_warned"); config.table_properties_ignored = validateTableProperties(config.table_properties_ignored, "table_properties_ignored"); config.table_properties_disallowed = validateTableProperties(config.table_properties_disallowed, "table_properties_disallowed"); + config.keyspace_properties_warned = validateKeyspaceProperties(config.keyspace_properties_warned, "keyspace_properties_warned"); + config.keyspace_properties_ignored = validateKeyspaceProperties(config.keyspace_properties_ignored, "keyspace_properties_ignored"); + config.keyspace_properties_disallowed = validateKeyspaceProperties(config.keyspace_properties_disallowed, "keyspace_properties_disallowed"); validateMaxIntThreshold(config.page_size_warn_threshold, config.page_size_fail_threshold, "page_size"); validateMaxIntThreshold(config.partition_keys_in_select_warn_threshold, config.partition_keys_in_select_fail_threshold, "partition_keys_in_select"); validateMaxIntThreshold(config.in_select_cartesian_product_warn_threshold, config.in_select_cartesian_product_fail_threshold, "in_select_cartesian_product"); @@ -181,6 +186,48 @@ public class GuardrailsOptions implements GuardrailsConfig x -> config.columns_per_table_fail_threshold = x); } + @Override + public Set getKeyspacePropertiesWarned() + { + return config.keyspace_properties_warned; + } + + public void setKeyspacePropertiesWarned(Set properties) + { + updatePropertyWithLogging("keyspace_properties_warned", + validateKeyspaceProperties(properties, "keyspace_properties_warned"), + () -> config.keyspace_properties_warned, + x -> config.keyspace_properties_warned = x); + } + + @Override + public Set getKeyspacePropertiesIgnored() + { + return config.keyspace_properties_ignored; + } + + public void setKeyspacePropertiesIgnored(Set properties) + { + updatePropertyWithLogging("keyspace_properties_ignored", + validateKeyspaceProperties(properties, "keyspace_properties_ignored"), + () -> config.keyspace_properties_ignored, + x -> config.keyspace_properties_ignored = x); + } + + @Override + public Set getKeyspacePropertiesDisallowed() + { + return config.keyspace_properties_disallowed; + } + + public void setKeyspacePropertiesDisallowed(Set properties) + { + updatePropertyWithLogging("keyspace_properties_disallowed", + validateKeyspaceProperties(properties, "keyspace_properties_disallowed"), + () -> config.keyspace_properties_disallowed, + x -> config.keyspace_properties_disallowed = x); + } + @Override public int getSecondaryIndexesPerTableWarnThreshold() { @@ -1424,12 +1471,42 @@ public class GuardrailsOptions implements GuardrailsConfig Set diff = Sets.difference(lowerCaseProperties, TableAttributes.allKeywords()); if (!diff.isEmpty()) - throw new IllegalArgumentException(format("Invalid value for %s: '%s' do not parse as valid table properties", - name, diff)); + throw new IllegalArgumentException(invalidValueMessage(name, diff, "table")); return lowerCaseProperties; } + private static Set validateKeyspaceProperties(Set properties, String name) + { + if (properties == null) + throw new IllegalArgumentException(format("Invalid value for %s: null is not allowed", name)); + + Set lowerCaseProperties = properties.stream().map(LocalizeString::toLowerCaseLocalized).collect(toSet()); + + for (String requiredKeyword : KeyspaceAttributes.requiredKeywords()) + { + if (lowerCaseProperties.contains(requiredKeyword)) + throw new IllegalArgumentException(format("Invalid value for %s: '%s' is a required keyspace property", name, requiredKeyword)); + } + + Set diff = Sets.difference(lowerCaseProperties, KeyspaceAttributes.allKeywords()); + + if (!diff.isEmpty()) + throw new IllegalArgumentException(invalidValueMessage(name, diff, "keyspace")); + + return lowerCaseProperties; + } + + public static String invalidValueMessage(String name, Set invalidProperties, String type) + { + if (invalidProperties.size() == 1) + return format("Invalid value for %s: '%s' does not parse as valid %s property.", + name, invalidProperties, type); + + return format("Invalid values for %s: '%s' do not parse as valid %s properties", + name, invalidProperties, type); + } + private static Set validateConsistencyLevels(Set consistencyLevels, String name) { if (consistencyLevels == null) diff --git a/src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java b/src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java index 14bdef0f68..94f2504653 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java @@ -122,6 +122,14 @@ public final class AlterKeyspaceStatement extends AlterSchemaStatement client.ensureKeyspacePermission(keyspaceName, Permission.ALTER); } + @Override + public void validate(ClientState state) + { + super.validate(state); + + Guardrails.keyspaceProperties.guard(attrs.updatedProperties(), attrs::removeProperty, state); + } + @Override Set clientWarnings(KeyspacesDiff diff) { diff --git a/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java b/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java index ed448f6842..1df860d123 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java @@ -137,6 +137,8 @@ public final class CreateKeyspaceStatement extends AlterSchemaStatement // Guardrail on number of keyspaces Guardrails.keyspaces.guard(Schema.instance.getUserKeyspaces().size() + 1, keyspaceName, false, state); + + Guardrails.keyspaceProperties.guard(attrs.updatedProperties(), attrs::removeProperty, state); } public static final class Raw extends CQLStatement.Raw diff --git a/src/java/org/apache/cassandra/cql3/statements/schema/KeyspaceAttributes.java b/src/java/org/apache/cassandra/cql3/statements/schema/KeyspaceAttributes.java index 0be8b882dd..6e77535311 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/KeyspaceAttributes.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/KeyspaceAttributes.java @@ -20,6 +20,7 @@ package org.apache.cassandra.cql3.statements.schema; import java.util.*; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; import org.apache.cassandra.cql3.statements.PropertyDefinitions; import org.apache.cassandra.exceptions.ConfigurationException; @@ -33,6 +34,7 @@ public final class KeyspaceAttributes extends PropertyDefinitions { private static final Set validKeywords; private static final Set obsoleteKeywords; + private static final Set requiredKeywords; static { @@ -41,6 +43,7 @@ public final class KeyspaceAttributes extends PropertyDefinitions validBuilder.add(option.toString()); validKeywords = validBuilder.build(); obsoleteKeywords = ImmutableSet.of(); + requiredKeywords = ImmutableSet.of(Option.REPLICATION.toString()); } public void validate() @@ -55,6 +58,16 @@ public final class KeyspaceAttributes extends PropertyDefinitions if (strategy != null && strategy.kind() == FastPathStrategy.Kind.INHERIT_KEYSPACE) throw new ConfigurationException("Cannot use keyspace inheriting fast path strategy with keyspaces"); } + + public static Set allKeywords() + { + return Sets.union(validKeywords, obsoleteKeywords); + } + + public static Set requiredKeywords() + { + return requiredKeywords; + } public String getReplicationStrategyClass() { diff --git a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java index 764dd27787..c2b5505ffd 100644 --- a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java +++ b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java @@ -143,6 +143,17 @@ public final class Guardrails implements GuardrailsMBean state -> CONFIG_PROVIDER.getOrCreate(state).getTablePropertiesDisallowed(), "Table Properties"); + /** + * Guardrail warning about, ignoring or rejecting the usage of certain keyspace properties. + */ + public static final Values keyspaceProperties = + new Values<>("keyspace_properties", + null, + state -> CONFIG_PROVIDER.getOrCreate(state).getKeyspacePropertiesWarned(), + state -> CONFIG_PROVIDER.getOrCreate(state).getKeyspacePropertiesIgnored(), + state -> CONFIG_PROVIDER.getOrCreate(state).getKeyspacePropertiesDisallowed(), + "Keyspace Properties"); + /** * Guardrail disabling user-provided timestamps. */ @@ -843,6 +854,93 @@ public final class Guardrails implements GuardrailsMBean setTablePropertiesIgnored(fromCSV(properties)); } + @Override + public Set getKeyspacePropertiesWarned() + { + return DEFAULT_CONFIG.getKeyspacePropertiesWarned(); + } + + @Override + public String getKeyspacePropertiesWarnedCSV() + { + return toCSV(DEFAULT_CONFIG.getKeyspacePropertiesWarned()); + } + + public void setKeyspacePropertiesWarned(String... properties) + { + setKeyspacePropertiesWarned(ImmutableSet.copyOf(properties)); + } + + @Override + public void setKeyspacePropertiesWarned(Set properties) + { + DEFAULT_CONFIG.setKeyspacePropertiesWarned(properties); + } + + @Override + public void setKeyspacePropertiesWarnedCSV(String properties) + { + setKeyspacePropertiesWarned(fromCSV(properties)); + } + + @Override + public Set getKeyspacePropertiesDisallowed() + { + return DEFAULT_CONFIG.getKeyspacePropertiesDisallowed(); + } + + @Override + public String getKeyspacePropertiesDisallowedCSV() + { + return toCSV(DEFAULT_CONFIG.getKeyspacePropertiesDisallowed()); + } + + public void setKeyspacePropertiesDisallowed(String... properties) + { + setKeyspacePropertiesDisallowed(ImmutableSet.copyOf(properties)); + } + + @Override + public void setKeyspacePropertiesDisallowed(Set properties) + { + DEFAULT_CONFIG.setKeyspacePropertiesDisallowed(properties); + } + + @Override + public void setKeyspacePropertiesDisallowedCSV(String properties) + { + setKeyspacePropertiesDisallowed(fromCSV(properties)); + } + + @Override + public Set getKeyspacePropertiesIgnored() + { + return DEFAULT_CONFIG.getKeyspacePropertiesIgnored(); + } + + @Override + public String getKeyspacePropertiesIgnoredCSV() + { + return toCSV(DEFAULT_CONFIG.getKeyspacePropertiesIgnored()); + } + + public void setKeyspacePropertiesIgnored(String... properties) + { + setKeyspacePropertiesIgnored(ImmutableSet.copyOf(properties)); + } + + @Override + public void setKeyspacePropertiesIgnored(Set properties) + { + DEFAULT_CONFIG.setKeyspacePropertiesIgnored(properties); + } + + @Override + public void setKeyspacePropertiesIgnoredCSV(String properties) + { + setKeyspacePropertiesIgnored(fromCSV(properties)); + } + @Override public boolean getUserTimestampsEnabled() { diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java index 5bfbe1b0f1..2d1fe9a424 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java @@ -126,6 +126,21 @@ public interface GuardrailsConfig */ Set getTablePropertiesDisallowed(); + /** + * @return The keyspace properties that are warned about when creating or altering a keyspace. + */ + Set getKeyspacePropertiesWarned(); + + /** + * @return The keyspace properties that are ignored when creating or altering a keyspace. + */ + Set getKeyspacePropertiesIgnored(); + + /** + * @return The keyspace properties that are disallowed when creating or altering a keyspace. + */ + Set getKeyspacePropertiesDisallowed(); + /** * Returns whether user-provided timestamps are allowed. * diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java index 421ef341bf..0df64bada6 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java @@ -195,6 +195,66 @@ public interface GuardrailsMBean */ void setTablePropertiesIgnoredCSV(String properties); + /** + * @return properties that are warned about when creating or altering a keyspace. + */ + Set getKeyspacePropertiesWarned(); + + /** + * @return Comma-separated list of properties that are warned about when creating or altering a keyspace. + */ + String getKeyspacePropertiesWarnedCSV(); + + /** + * @param properties properties that are warned about when creating or altering a keyspace. + */ + void setKeyspacePropertiesWarned(Set properties); + + /** + * @param properties Comma-separated list of properties that are warned about when creating or altering a keyspace. + */ + void setKeyspacePropertiesWarnedCSV(String properties); + + /** + * @return properties that are not allowed when creating or altering a keyspace. + */ + Set getKeyspacePropertiesDisallowed(); + + /** + * @return Comma-separated list of properties that are not allowed when creating or altering a keyspace. + */ + String getKeyspacePropertiesDisallowedCSV(); + + /** + * @param properties properties that are not allowed when creating or altering a keyspace. + */ + void setKeyspacePropertiesDisallowed(Set properties); + + /** + * @param properties Comma-separated list of properties that are not allowed when creating or altering a keyspace. + */ + void setKeyspacePropertiesDisallowedCSV(String properties); + + /** + * @return properties that are ignored when creating or altering a keyspace. + */ + Set getKeyspacePropertiesIgnored(); + + /** + * @return Comma-separated list of properties that are ignored when creating or altering a keyspace. + */ + String getKeyspacePropertiesIgnoredCSV(); + + /** + * @param properties properties that are ignored when creating or altering a keyspace. + */ + void setKeyspacePropertiesIgnored(Set properties); + + /** + * @param properties Comma-separated list of properties that are ignored when creating or altering a keyspace. + */ + void setKeyspacePropertiesIgnoredCSV(String properties); + /** * Returns whether user-provided timestamps are allowed. * diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailKeyspacePropertiesTest.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailKeyspacePropertiesTest.java new file mode 100644 index 0000000000..479dbd5121 --- /dev/null +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailKeyspacePropertiesTest.java @@ -0,0 +1,247 @@ +/* + * 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.db.guardrails; + +import static java.lang.String.format; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import org.apache.cassandra.config.GuardrailsOptions; +import org.apache.cassandra.cql3.statements.schema.KeyspaceAttributes; +import org.apache.cassandra.schema.KeyspaceParams; +import org.apache.cassandra.utils.LocalizeString; +import org.junit.Before; +import org.junit.Test; + +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; + +/** + * Tests the guardrail for keyspace properties, {@link Guardrails#keyspaceProperties}. + */ +public class GuardrailKeyspacePropertiesTest extends GuardrailTester +{ + private static final String CREATE_KEYSPACE = "CREATE KEYSPACE %s %s"; + private static final String ALTER_KEYSPACE = "ALTER KEYSPACE %s WITH %s"; + + private static final String WARNED_PROPERTY_NAME = "keyspace_properties_warned"; + private static final String IGNORED_PROPERTY_NAME = "keyspace_properties_ignored"; + private static final String DISALLOWED_PROPERTY_NAME = "keyspace_properties_disallowed"; + + private static final String DURABLE_WRITES = "durable_writes = false"; + private static final String REPLICATION = "WITH replication = {'class': 'NetworkTopologyStrategy', 'replication_factor': 1}"; + + private static final String WARN_MESSAGE = "Provided values [%s] are not recommended for Keyspace Properties"; + private static final String IGNORE_MESSAGE = "Ignoring provided values [%s] as they are not supported for Keyspace Properties"; + private static final String FAIL_MESSAGE = "Provided values [%s] are not allowed for Keyspace Properties"; + + public GuardrailKeyspacePropertiesTest() + { + super(Guardrails.keyspaceProperties); + } + + @Before + public void before() + { + // Set up basic test configuration - only allow "replication" by default + Set allowed = new HashSet<>(List.of("replication", "durable_writes")); + Set allKeyspaceProperties = Arrays.stream(KeyspaceParams.Option.values()) + .map(KeyspaceParams.Option::toString) + .collect(Collectors.toSet()); + guardrails().setKeyspacePropertiesDisallowed(allKeyspaceProperties + .stream() + .filter(p -> !allowed.contains(p)) + .map(LocalizeString::toUpperCaseLocalized) + .collect(Collectors.toSet())); + // Configure "durable_writes" to be warned about (tests will override as needed) + guardrails().setKeyspacePropertiesWarned("durable_writes"); + guardrails().setKeyspacePropertiesIgnored(Collections.emptySet()); + } + + @Test + public void testConfigValidation() + { + String message = "Invalid value for %s: null is not allowed"; + assertInvalidProperty(Guardrails::setKeyspacePropertiesWarned, (Set) null, message, WARNED_PROPERTY_NAME); + assertInvalidProperty(Guardrails::setKeyspacePropertiesIgnored, (Set) null, message, IGNORED_PROPERTY_NAME); + assertInvalidProperty(Guardrails::setKeyspacePropertiesDisallowed, (Set) null, message, DISALLOWED_PROPERTY_NAME); + + assertValidProperty(Collections.emptySet()); + assertValidProperty(getValidKeyspaceProperties()); + + assertValidPropertyCSV(""); + assertValidPropertyCSV(String.join(",", getValidKeyspaceProperties())); + + assertInvalidProperty(Collections.singleton("invalid"), Collections.singleton("invalid")); + assertInvalidProperty(ImmutableSet.of("invalid1", "invalid2"), ImmutableSet.of("invalid1", "invalid2")); + assertInvalidProperty(ImmutableSet.of("replication", "invalid2"), ImmutableSet.of("replication")); + assertInvalidProperty(ImmutableSet.of("durable_writes", "invalid2", "replication"), ImmutableSet.of("replication")); + assertInvalidProperty(ImmutableSet.of("durable_writes", "replication", "fast_path"), ImmutableSet.of("replication")); + + assertInvalidPropertyCSV("invalid", "[invalid]"); + assertInvalidPropertyCSV("invalid1,invalid2", "[invalid1, invalid2]"); + assertInvalidPropertyCSV("replication,invalid1", "[replication]"); + assertInvalidPropertyCSV("durable_writes,invalid2,replication", "[replication]"); + assertInvalidPropertyCSV("durable_writes,replication,fast_path", "[replication]"); + } + + private void assertValidProperty(Set properties) + { + assertValidProperty(Guardrails::setKeyspacePropertiesWarned, Guardrails::getKeyspacePropertiesWarned, properties); + assertValidProperty(Guardrails::setKeyspacePropertiesIgnored, Guardrails::getKeyspacePropertiesIgnored, properties); + assertValidProperty(Guardrails::setKeyspacePropertiesDisallowed, Guardrails::getKeyspacePropertiesDisallowed, properties); + } + + private void assertValidPropertyCSV(String csv) + { + csv = sortCSV(csv); + assertValidProperty(Guardrails::setKeyspacePropertiesWarnedCSV, g -> sortCSV(g.getKeyspacePropertiesWarnedCSV()), csv); + assertValidProperty(Guardrails::setKeyspacePropertiesIgnoredCSV, g -> sortCSV(g.getKeyspacePropertiesIgnoredCSV()), csv); + assertValidProperty(Guardrails::setKeyspacePropertiesDisallowedCSV, g -> sortCSV(g.getKeyspacePropertiesDisallowedCSV()), csv); + } + + private void assertInvalidProperty(Set properties, Set invalidProperties) + { + if (invalidProperties.contains("replication")) + { + String replicationMessage = "Invalid value for %s: '%s' is a required keyspace property"; + assertInvalidProperty(Guardrails::setKeyspacePropertiesWarned, properties, format(replicationMessage, WARNED_PROPERTY_NAME, "replication"), WARNED_PROPERTY_NAME); + assertInvalidProperty(Guardrails::setKeyspacePropertiesIgnored, properties, format(replicationMessage, IGNORED_PROPERTY_NAME, "replication"), IGNORED_PROPERTY_NAME); + assertInvalidProperty(Guardrails::setKeyspacePropertiesDisallowed, properties, format(replicationMessage, DISALLOWED_PROPERTY_NAME, "replication"), DISALLOWED_PROPERTY_NAME); + } + else + { + String message = GuardrailsOptions.invalidValueMessage(WARNED_PROPERTY_NAME, invalidProperties, "keyspace"); + assertInvalidProperty(Guardrails::setKeyspacePropertiesWarned, properties, message, WARNED_PROPERTY_NAME); + + message = GuardrailsOptions.invalidValueMessage(IGNORED_PROPERTY_NAME, invalidProperties, "keyspace"); + assertInvalidProperty(Guardrails::setKeyspacePropertiesIgnored, properties, message, IGNORED_PROPERTY_NAME); + + message = GuardrailsOptions.invalidValueMessage(DISALLOWED_PROPERTY_NAME, invalidProperties, "keyspace"); + assertInvalidProperty(Guardrails::setKeyspacePropertiesDisallowed, properties, message, DISALLOWED_PROPERTY_NAME); + } + } + + private void assertInvalidPropertyCSV(String properties, String rejected) + { + // Parse the rejected string to extract individual properties + // Expected format is "[prop1, prop2]" so we need to extract the properties inside brackets + String cleanRejected = rejected.substring(1, rejected.length() - 1); // Remove brackets + Set rejectedSet = Arrays.stream(cleanRejected.split(",")) + .map(String::trim) + .collect(Collectors.toSet()); + + if (rejectedSet.contains("replication")) + { + String replicationMessage = "Invalid value for %s: '%s' is a required keyspace property"; + assertInvalidProperty(Guardrails::setKeyspacePropertiesWarnedCSV, properties, format(replicationMessage, WARNED_PROPERTY_NAME, "replication"), WARNED_PROPERTY_NAME, rejected); + assertInvalidProperty(Guardrails::setKeyspacePropertiesIgnoredCSV, properties, format(replicationMessage, IGNORED_PROPERTY_NAME, "replication"), IGNORED_PROPERTY_NAME, rejected); + assertInvalidProperty(Guardrails::setKeyspacePropertiesDisallowedCSV, properties, format(replicationMessage, DISALLOWED_PROPERTY_NAME, "replication"), DISALLOWED_PROPERTY_NAME, rejected); + } + else + { + String message = GuardrailsOptions.invalidValueMessage(WARNED_PROPERTY_NAME, rejectedSet, "keyspace"); + assertInvalidProperty(Guardrails::setKeyspacePropertiesWarnedCSV, properties, message, WARNED_PROPERTY_NAME, rejected); + + message = GuardrailsOptions.invalidValueMessage(IGNORED_PROPERTY_NAME, rejectedSet, "keyspace"); + assertInvalidProperty(Guardrails::setKeyspacePropertiesIgnoredCSV, properties, message, IGNORED_PROPERTY_NAME, rejected); + + message = GuardrailsOptions.invalidValueMessage(DISALLOWED_PROPERTY_NAME, rejectedSet, "keyspace"); + assertInvalidProperty(Guardrails::setKeyspacePropertiesDisallowedCSV, properties, message, DISALLOWED_PROPERTY_NAME, rejected); + } + } + + @Test + public void testCreateKeyspaceWithWarned() throws Throwable + { + String ks = createKeyspaceName(); + String statement = format(CREATE_KEYSPACE, ks, REPLICATION + " AND " + DURABLE_WRITES); + assertWarns(statement, format(WARN_MESSAGE, "durable_writes")); + } + + @Test + public void testCreateKeyspaceWithIgnored() throws Throwable + { + guardrails().setKeyspacePropertiesIgnored("durable_writes"); + guardrails().setKeyspacePropertiesWarned(Collections.emptySet()); + String ks = createKeyspaceName(); + String statement = format(CREATE_KEYSPACE, ks, REPLICATION + " AND " + DURABLE_WRITES); + assertWarns(statement, format(IGNORE_MESSAGE, "durable_writes")); + } + + @Test + public void testCreateKeyspaceWithDisallowed() throws Throwable + { + guardrails().setKeyspacePropertiesDisallowed("durable_writes"); + String ks = createKeyspaceName(); + String statement = format(CREATE_KEYSPACE, ks, REPLICATION + " AND " + DURABLE_WRITES); + assertFails(statement, format(FAIL_MESSAGE, "durable_writes")); + } + + @Test + public void testAlterKeyspaceWithWarned() throws Throwable + { + String ks = createKeyspaceName(); + execute(format(CREATE_KEYSPACE, ks, "WITH replication = {'class': 'NetworkTopologyStrategy', 'replication_factor': 1}")); + String statement = format(ALTER_KEYSPACE, ks, DURABLE_WRITES); + assertWarns(statement, format(WARN_MESSAGE, "durable_writes")); + } + + @Test + public void testAlterKeyspaceWithDisallowed() throws Throwable + { + guardrails().setKeyspacePropertiesDisallowed("durable_writes"); + String ks = createKeyspaceName(); + execute(format(CREATE_KEYSPACE, ks, "WITH replication = {'class': 'NetworkTopologyStrategy', 'replication_factor': 1}")); + String statement = format(ALTER_KEYSPACE, ks, DURABLE_WRITES); + assertFails(statement, format(FAIL_MESSAGE, "durable_writes")); + } + + @Test + public void testDisallowedPropertyTakesPrecedenceOverWarned() throws Throwable + { + // Set a property as both warned and disallowed - disallowed should take precedence + guardrails().setKeyspacePropertiesWarned("durable_writes"); + guardrails().setKeyspacePropertiesDisallowed("durable_writes"); + String ks = createKeyspaceName(); + String statement = format(CREATE_KEYSPACE, ks, REPLICATION + " AND " + DURABLE_WRITES); + assertFails(statement, format(FAIL_MESSAGE, "durable_writes")); + } + + @Test + public void testValidKeyspaceCreation() throws Throwable + { + // Test normal keyspace creation case without any guardrail violations + guardrails().setKeyspacePropertiesDisallowed(Collections.emptySet()); + guardrails().setKeyspacePropertiesWarned(Collections.emptySet()); + String ks = createKeyspaceName(); + String statement = format(CREATE_KEYSPACE, ks, REPLICATION); + assertValid(statement); + } + + private Set getValidKeyspaceProperties() + { + return Sets.difference(KeyspaceAttributes.allKeywords(), KeyspaceAttributes.requiredKeywords()); + } +} diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailTablePropertiesTest.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailTablePropertiesTest.java index 5754c51923..cdd34f8c1d 100644 --- a/test/unit/org/apache/cassandra/db/guardrails/GuardrailTablePropertiesTest.java +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailTablePropertiesTest.java @@ -29,7 +29,7 @@ import java.util.stream.Collectors; import com.google.common.collect.ImmutableSet; import org.junit.Before; import org.junit.Test; - +import org.apache.cassandra.config.GuardrailsOptions; import org.apache.cassandra.cql3.statements.schema.TableAttributes; import static java.lang.String.format; @@ -111,17 +111,32 @@ public class GuardrailTablePropertiesTest extends GuardrailTester private void assertInvalidProperty(Set properties, Set rejected) { - String message = "Invalid value for %s: '%s' do not parse as valid table properties"; + String message = GuardrailsOptions.invalidValueMessage(WARNED_PROPERTY_NAME, rejected, "table"); assertInvalidProperty(Guardrails::setTablePropertiesWarned, properties, message, WARNED_PROPERTY_NAME, rejected); + + message = GuardrailsOptions.invalidValueMessage(IGNORED_PROPERTY_NAME, rejected, "table"); assertInvalidProperty(Guardrails::setTablePropertiesIgnored, properties, message, IGNORED_PROPERTY_NAME, rejected); + + message = GuardrailsOptions.invalidValueMessage(DISALLOWED_PROPERTY_NAME, rejected, "table"); assertInvalidProperty(Guardrails::setTablePropertiesDisallowed, properties, message, DISALLOWED_PROPERTY_NAME, rejected); } private void assertInvalidPropertyCSV(String properties, String rejected) { - String message = "Invalid value for %s: '%s' do not parse as valid table properties"; + // Parse the rejected string to extract individual properties + // Expected format is "[prop1, prop2]" so we need to extract the properties inside brackets + String cleanRejected = rejected.substring(1, rejected.length() - 1); // Remove brackets + Set rejectedSet = Arrays.stream(cleanRejected.split(",")) + .map(String::trim) + .collect(Collectors.toSet()); + + String message = GuardrailsOptions.invalidValueMessage(WARNED_PROPERTY_NAME, rejectedSet, "table"); assertInvalidProperty(Guardrails::setTablePropertiesWarnedCSV, properties, message, WARNED_PROPERTY_NAME, rejected); + + message = GuardrailsOptions.invalidValueMessage(IGNORED_PROPERTY_NAME, rejectedSet, "table"); assertInvalidProperty(Guardrails::setTablePropertiesIgnoredCSV, properties, message, IGNORED_PROPERTY_NAME, rejected); + + message = GuardrailsOptions.invalidValueMessage(DISALLOWED_PROPERTY_NAME, rejectedSet, "table"); assertInvalidProperty(Guardrails::setTablePropertiesDisallowedCSV, properties, message, DISALLOWED_PROPERTY_NAME, rejected); } diff --git a/test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java b/test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java index 0095b492fa..d88949b473 100644 --- a/test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java +++ b/test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java @@ -312,6 +312,9 @@ public class GuardrailsConfigCommandsTest extends CQLTester "vector_dimensions_warn_threshold -1 \n"; private static final String ALL_VALUES_GETTER_OUTPUT = + "keyspace_properties_disallowed [] \n" + + "keyspace_properties_ignored [] \n" + + "keyspace_properties_warned [] \n" + "read_consistency_levels_disallowed [] \n" + "read_consistency_levels_warned [] \n" + "table_properties_disallowed [] \n" +