This is an automated email from the ASF dual-hosted git repository. jmckenzie 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 14fbab15bd Add guardrail to allow disabling SimpleStrategy 14fbab15bd is described below commit 14fbab15bd264dd1cf894bf48170cf4f30ada8a0 Author: Josh McKenzie <jmcken...@apache.org> AuthorDate: Thu May 19 15:48:20 2022 -0400 Add guardrail to allow disabling SimpleStrategy Patch by Josh McKenzie; reviewed by Aleksey Yeschenko for CASSANDRA-17647 --- CHANGES.txt | 1 + NEWS.txt | 1 + conf/cassandra.yaml | 3 + src/java/org/apache/cassandra/config/Config.java | 1 + .../apache/cassandra/config/GuardrailsOptions.java | 14 ++++ .../statements/schema/AlterKeyspaceStatement.java | 5 ++ .../statements/schema/CreateKeyspaceStatement.java | 4 + .../cql3/statements/schema/KeyspaceAttributes.java | 2 +- .../apache/cassandra/db/guardrails/Guardrails.java | 20 +++++ .../cassandra/db/guardrails/GuardrailsConfig.java | 7 ++ .../cassandra/db/guardrails/GuardrailsMBean.java | 14 ++++ .../db/guardrails/GuardrailSimpleStrategyTest.java | 89 ++++++++++++++++++++++ 12 files changed, 160 insertions(+), 1 deletion(-) diff --git a/CHANGES.txt b/CHANGES.txt index c38906747d..6d9feb52ff 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.2 + * Add guardrail to allow disabling of SimpleStrategy (CASSANDRA-17647) * Change default directory permission to 750 in packaging (CASSANDRA-17470) * Adding support for TLS client authentication for internode communication (CASSANDRA-17513) * Add new CQL function maxWritetime (CASSANDRA-17425) diff --git a/NEWS.txt b/NEWS.txt index e44b0e4fe8..2b81a284ad 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -62,6 +62,7 @@ New features non-frozen collections and UDT, and returns the largest timestamp. One should not to use it when upgrading to 4.2. - New Guardrails added: - Whether ALTER TABLE commands are allowed to mutate columns + - Whether SimpleStrategy is allowed on keyspace creation or alteration Upgrading --------- diff --git a/conf/cassandra.yaml b/conf/cassandra.yaml index 409110a024..491740f012 100644 --- a/conf/cassandra.yaml +++ b/conf/cassandra.yaml @@ -1763,6 +1763,9 @@ drop_compact_storage_enabled: false # Guardrail to allow/disallow querying with ALLOW FILTERING. Defaults to true. # allow_filtering_enabled: true # +# Guardrail to allow/disallow setting SimpleStrategy via keyspace creation or alteration. Defaults to true. +# simplestrategy_enabled: true +# # Guardrail to warn or fail when creating a user-defined-type with more fields in than threshold. # Default -1 to disable. # fields_per_udt_warn_threshold: -1 diff --git a/src/java/org/apache/cassandra/config/Config.java b/src/java/org/apache/cassandra/config/Config.java index 9f75a277bc..c3c5b3582c 100644 --- a/src/java/org/apache/cassandra/config/Config.java +++ b/src/java/org/apache/cassandra/config/Config.java @@ -833,6 +833,7 @@ public class Config public volatile boolean compact_tables_enabled = true; public volatile boolean read_before_write_list_operations_enabled = true; public volatile boolean allow_filtering_enabled = true; + public volatile boolean simplestrategy_enabled = true; public volatile DataStorageSpec.LongBytesBound collection_size_warn_threshold = null; public volatile DataStorageSpec.LongBytesBound collection_size_fail_threshold = null; public volatile int items_per_collection_warn_threshold = -1; diff --git a/src/java/org/apache/cassandra/config/GuardrailsOptions.java b/src/java/org/apache/cassandra/config/GuardrailsOptions.java index b14f428ca4..e8d7bda77a 100644 --- a/src/java/org/apache/cassandra/config/GuardrailsOptions.java +++ b/src/java/org/apache/cassandra/config/GuardrailsOptions.java @@ -427,6 +427,20 @@ public class GuardrailsOptions implements GuardrailsConfig x -> config.allow_filtering_enabled = x); } + @Override + public boolean getSimpleStrategyEnabled() + { + return config.simplestrategy_enabled; + } + + public void setSimpleStrategyEnabled(boolean enabled) + { + updatePropertyWithLogging("simplestrategy_enabled", + enabled, + () -> config.simplestrategy_enabled, + x -> config.simplestrategy_enabled = x); + } + @Override public int getInSelectCartesianProductWarnThreshold() { 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 87377d70ec..dec0655e74 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/AlterKeyspaceStatement.java @@ -31,12 +31,14 @@ import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.cql3.CQLStatement; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.db.Keyspace; +import org.apache.cassandra.db.guardrails.Guardrails; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.gms.Gossiper; import org.apache.cassandra.locator.AbstractReplicationStrategy; import org.apache.cassandra.locator.InetAddressAndPort; import org.apache.cassandra.locator.LocalStrategy; import org.apache.cassandra.locator.ReplicationFactor; +import org.apache.cassandra.locator.SimpleStrategy; import org.apache.cassandra.schema.KeyspaceMetadata; import org.apache.cassandra.schema.KeyspaceMetadata.KeyspaceDiff; import org.apache.cassandra.schema.Keyspaces; @@ -76,6 +78,9 @@ public final class AlterKeyspaceStatement extends AlterSchemaStatement KeyspaceMetadata newKeyspace = keyspace.withSwapped(attrs.asAlteredKeyspaceParams(keyspace.params)); + if (attrs.getReplicationStrategyClass() != null && attrs.getReplicationStrategyClass().equals(SimpleStrategy.class.getSimpleName())) + Guardrails.simpleStrategyEnabled.ensureEnabled(state); + if (newKeyspace.params.replication.klass.equals(LocalStrategy.class)) throw ire("Unable to use given strategy class: LocalStrategy is reserved for internal use."); 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 dc82f93a10..ad6bcc472d 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/CreateKeyspaceStatement.java @@ -36,6 +36,7 @@ import org.apache.cassandra.cql3.CQLStatement; import org.apache.cassandra.db.guardrails.Guardrails; import org.apache.cassandra.exceptions.AlreadyExistsException; import org.apache.cassandra.locator.LocalStrategy; +import org.apache.cassandra.locator.SimpleStrategy; import org.apache.cassandra.schema.KeyspaceMetadata; import org.apache.cassandra.schema.KeyspaceParams.Option; import org.apache.cassandra.schema.Keyspaces; @@ -67,6 +68,9 @@ public final class CreateKeyspaceStatement extends AlterSchemaStatement if (!attrs.hasOption(Option.REPLICATION)) throw ire("Missing mandatory option '%s'", Option.REPLICATION); + if (attrs.getReplicationStrategyClass() != null && attrs.getReplicationStrategyClass().equals(SimpleStrategy.class.getSimpleName())) + Guardrails.simpleStrategyEnabled.ensureEnabled("SimpleStrategy", state); + if (schema.containsKeyspace(keyspaceName)) { if (ifNotExists) 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 42fcaf4e69..d4d5b984b3 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/KeyspaceAttributes.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/KeyspaceAttributes.java @@ -50,7 +50,7 @@ public final class KeyspaceAttributes extends PropertyDefinitions throw new ConfigurationException("Missing replication strategy class"); } - private String getReplicationStrategyClass() + public String getReplicationStrategyClass() { return getAllReplicationOptions().get(ReplicationParams.CLASS); } diff --git a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java index 57dd2af5ce..16146fec87 100644 --- a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java +++ b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java @@ -218,6 +218,14 @@ public final class Guardrails implements GuardrailsMBean state -> CONFIG_PROVIDER.getOrCreate(state).getAllowFilteringEnabled(), "Querying with ALLOW FILTERING"); + /** + * Guardrail disabling setting SimpleStrategy via keyspace creation or alteration + */ + public static final EnableFlag simpleStrategyEnabled = + new EnableFlag("simplestrategy", + state -> CONFIG_PROVIDER.getOrCreate(state).getSimpleStrategyEnabled(), + "SimpleStrategy"); + /** * Guardrail on the number of restrictions created by a cartesian product of a CQL's {@code IN} query. */ @@ -571,6 +579,18 @@ public final class Guardrails implements GuardrailsMBean DEFAULT_CONFIG.setAllowFilteringEnabled(enabled); } + @Override + public boolean getSimpleStrategyEnabled() + { + return DEFAULT_CONFIG.getSimpleStrategyEnabled(); + } + + @Override + public void setSimpleStrategyEnabled(boolean enabled) + { + DEFAULT_CONFIG.setSimpleStrategyEnabled(enabled); + } + @Override public boolean getUncompressedTablesEnabled() { diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java index 7f38e80ec3..72eaaa5b48 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java @@ -191,6 +191,13 @@ public interface GuardrailsConfig */ boolean getAllowFilteringEnabled(); + /** + * Returns whether setting SimpleStrategy via keyspace creation or alteration is enabled + * + * @return {@code true} if SimpleStrategy is allowed, {@code false} otherwise. + */ + boolean getSimpleStrategyEnabled(); + /** * @return The threshold to warn when an IN query creates a cartesian product with a size exceeding threshold. * -1 means disabled. diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java index 30be464d02..47db91a6fa 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java @@ -222,6 +222,20 @@ public interface GuardrailsMBean */ void setAllowFilteringEnabled(boolean enabled); + /** + * Returns whether SimpleStrategy is allowed on keyspace creation or alteration + * + * @return {@code true} if SimpleStrategy is allowed; {@code false} otherwise + */ + boolean getSimpleStrategyEnabled(); + + /** + * Sets whether SimpleStrategy is allowed on keyspace creation or alteration + * + * @param enabled {@code true} if SimpleStrategy is allowed, {@code false} otherwise. + */ + void setSimpleStrategyEnabled(boolean enabled); + /** * Returns whether users can disable compression on tables * diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailSimpleStrategyTest.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailSimpleStrategyTest.java new file mode 100644 index 0000000000..3cc6bc747d --- /dev/null +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailSimpleStrategyTest.java @@ -0,0 +1,89 @@ +/* + * 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 org.junit.After; +import org.junit.Test; + +public class GuardrailSimpleStrategyTest extends GuardrailTester +{ + public static String ERROR_MSG = "SimpleStrategy is not allowed"; + + public GuardrailSimpleStrategyTest() + { + super(Guardrails.simpleStrategyEnabled); + } + + private void setGuardrail(boolean enabled) + { + guardrails().setSimpleStrategyEnabled(enabled); + } + + @After + public void afterTest() throws Throwable + { + setGuardrail(true); + execute("DROP KEYSPACE IF EXISTS test_ss;"); + } + + @Test + public void testCanCreateWithGuardrailEnabled() throws Throwable + { + assertValid("CREATE KEYSPACE test_ss WITH replication = {'class': 'SimpleStrategy'};"); + } + + @Test + public void testCanAlterWithGuardrailEnabled() throws Throwable + { + execute("CREATE KEYSPACE test_ss WITH replication = {'class': 'NetworkTopologyStrategy', 'datacenter1':2, 'datacenter2':0};"); + assertValid("ALTER KEYSPACE test_ss WITH replication = {'class': 'SimpleStrategy'};"); + } + + @Test + public void testGuardrailBlocksCreate() throws Throwable + { + setGuardrail(false); + assertFails("CREATE KEYSPACE test_ss WITH replication = {'class': 'SimpleStrategy'};", ERROR_MSG); + } + + @Test + public void testGuardrailBlocksAlter() throws Throwable + { + setGuardrail(false); + execute("CREATE KEYSPACE test_ss WITH replication = {'class': 'NetworkTopologyStrategy', 'datacenter1':2, 'datacenter2':0};"); + assertFails("ALTER KEYSPACE test_ss WITH replication = {'class': 'SimpleStrategy'};", ERROR_MSG); + } + + @Test + public void testToggle() throws Throwable + { + setGuardrail(false); + assertFails("CREATE KEYSPACE test_ss WITH replication = {'class': 'SimpleStrategy'};", ERROR_MSG); + + setGuardrail(true); + assertValid("CREATE KEYSPACE test_ss WITH replication = {'class': 'SimpleStrategy'};"); + execute("ALTER KEYSPACE test_ss WITH replication = {'class': 'NetworkTopologyStrategy', 'datacenter1':2, 'datacenter2':0};"); + + setGuardrail(false); + assertFails("ALTER KEYSPACE test_ss WITH replication = {'class': 'SimpleStrategy'};", ERROR_MSG); + + setGuardrail(true); + assertValid("ALTER KEYSPACE test_ss WITH replication = {'class': 'SimpleStrategy'};"); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org