Add guardrail to allow disabling SimpleStrategy

Patch by Josh McKenzie; reviewed by Aleksey Yeschenko for CASSANDRA-17647

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)

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
 ---------

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

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;

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()
     {

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.");
 

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)

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);
     }

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()
     {

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.

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
      *

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'};");
+    }
+}