From ce515a3d77c2042575827e8bdb1da639fc957491 Mon Sep 17 00:00:00 2001 From: Josh McKenzie Date: Wed, 23 Mar 2022 12:42:36 -0400 Subject: [PATCH] Add guardrail for ALTER TABLE ADD / DROP / REMOVE column operations Patch by Josh McKenzie; reviewed by Jon Meredith for CASSANDRA-17495 --- CHANGES.txt | 1 + NEWS.txt | 2 + conf/cassandra.yaml | 28 +++- .../org/apache/cassandra/config/Config.java | 1 + .../cassandra/config/GuardrailsOptions.java | 14 ++ .../schema/AlterTableStatement.java | 13 ++ .../cassandra/db/guardrails/Guardrails.java | 20 +++ .../db/guardrails/GuardrailsConfig.java | 7 + .../db/guardrails/GuardrailsMBean.java | 14 ++ .../cassandra/service/StorageService.java | 1 - .../guardrails/GuardrailAlterTableTest.java | 133 ++++++++++++++++++ 11 files changed, 232 insertions(+), 2 deletions(-) create mode 100644 test/unit/org/apache/cassandra/db/guardrails/GuardrailAlterTableTest.java diff --git a/CHANGES.txt b/CHANGES.txt index 306acc1ec4..f3c79ce3da 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.2 + * Add guardrail for ALTER TABLE ADD / DROP / REMOVE column operations (CASSANDRA-17495) * Rename DisableFlag class to EnableFlag on guardrails (CASSANDRA-17544) Merged from 4.1: Merged from 4.0: diff --git a/NEWS.txt b/NEWS.txt index fcc5d612b8..d42d03afe6 100644 --- a/NEWS.txt +++ b/NEWS.txt @@ -57,6 +57,8 @@ using the provided 'sstableupgrade' tool. New features ------------ + - New Guardrails added: + - Whether ALTER TABLE commands are allowed to mutate columns Upgrading --------- diff --git a/conf/cassandra.yaml b/conf/cassandra.yaml index ccc941da76..6932a5fbcf 100644 --- a/conf/cassandra.yaml +++ b/conf/cassandra.yaml @@ -1601,58 +1601,75 @@ drop_compact_storage_enabled: false # The two thresholds default to -1 to disable. # keyspaces_warn_threshold: -1 # keyspaces_fail_threshold: -1 +# # Guardrail to warn or fail when creating more user tables than threshold. # The two thresholds default to -1 to disable. # tables_warn_threshold: -1 # tables_fail_threshold: -1 +# # Guardrail to enable or disable the ability to create uncompressed tables # uncompressed_tables_enabled: true +# # Guardrail to warn or fail when creating/altering a table with more columns per table than threshold. # The two thresholds default to -1 to disable. # columns_per_table_warn_threshold: -1 # columns_per_table_fail_threshold: -1 +# # Guardrail to warn or fail when creating more secondary indexes per table than threshold. # The two thresholds default to -1 to disable. # secondary_indexes_per_table_warn_threshold: -1 # secondary_indexes_per_table_fail_threshold: -1 +# # Guardrail to enable or disable the creation of secondary indexes # secondary_indexes_enabled: true +# # Guardrail to warn or fail when creating more materialized views per table than threshold. # The two thresholds default to -1 to disable. # materialized_views_per_table_warn_threshold: -1 # materialized_views_per_table_fail_threshold: -1 +# # Guardrail to warn about, ignore or reject properties when creating tables. By default all properties are allowed. # table_properties_warned: [] # table_properties_ignored: [] # table_properties_disallowed: [] +# # Guardrail to allow/disallow user-provided timestamps. Defaults to true. # user_timestamps_enabled: true +# # Guardrail to allow/disallow GROUP BY functionality. # group_by_enabled: true +# # Guardrail to allow/disallow TRUNCATE and DROP TABLE statements # drop_truncate_table_enabled: true +# # Guardrail to warn or fail when using a page size greater than threshold. # The two thresholds default to -1 to disable. # page_size_warn_threshold: -1 # page_size_fail_threshold: -1 +# # Guardrail to allow/disallow list operations that require read before write, i.e. setting list element by index and # removing list elements by either index or value. Defaults to true. # read_before_write_list_operations_enabled: true +# # Guardrail to warn or fail when querying with an IN restriction selecting more partition keys than threshold. # The two thresholds default to -1 to disable. # partition_keys_in_select_warn_threshold: -1 # partition_keys_in_select_fail_threshold: -1 +# # Guardrail to warn or fail when an IN query creates a cartesian product with a size exceeding threshold, # eg. "a in (1,2,...10) and b in (1,2...10)" results in cartesian product of 100. # The two thresholds default to -1 to disable. # in_select_cartesian_product_warn_threshold: -1 # in_select_cartesian_product_fail_threshold: -1 +# # Guardrail to warn about or reject read consistency levels. By default, all consistency levels are allowed. # read_consistency_levels_warned: [] # read_consistency_levels_disallowed: [] +# # Guardrail to warn about or reject write consistency levels. By default, all consistency levels are allowed. # write_consistency_levels_warned: [] # write_consistency_levels_disallowed: [] +# # Guardrail to warn or fail when encountering larger size of collection data than threshold. # At query time this guardrail is applied only to the collection fragment that is being writen, even though in the case # of non-frozen collections there could be unaccounted parts of the collection on the sstables. This is done this way to @@ -1661,6 +1678,7 @@ drop_compact_storage_enabled: false # The two thresholds default to null to disable. # collection_size_warn_threshold: # collection_size_fail_threshold: +# # Guardrail to warn or fail when encountering more elements in collection than threshold. # At query time this guardrail is applied only to the collection fragment that is being writen, even though in the case # of non-frozen collections there could be unaccounted parts of the collection on the sstables. This is done this way to @@ -1669,12 +1687,18 @@ drop_compact_storage_enabled: false # The two thresholds default to -1 to disable. # items_per_collection_warn_threshold: -1 # items_per_collection_fail_threshold: -1 +# # Guardrail to allow/disallow querying with ALLOW FILTERING. Defaults to true. # allow_filtering_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 # fields_per_udt_fail_threshold: -1 +# +# Guardrail to indicate whether or not users are allowed to use ALTER TABLE commands to make column changes to tables +# alter_table_enabled: true +# # Guardrail to warn or fail when local data disk usage percentage exceeds threshold. Valid values are in [1, 100]. # This is only used for the disks storing data directories, so it won't count any separate disks used for storing # the commitlog, hints nor saved caches. The disk usage is the ratio between the amount of space used by the data @@ -1686,13 +1710,15 @@ drop_compact_storage_enabled: false # The two thresholds default to -1 to disable. # data_disk_usage_percentage_warn_threshold: -1 # data_disk_usage_percentage_fail_threshold: -1 -# Allows defining the max disk size of the data directories when calculating thresholds for +# +# Guardrail that allows users to define the max disk size of the data directories when calculating thresholds for # disk_usage_percentage_warn_threshold and disk_usage_percentage_fail_threshold, so if this is greater than zero they # become percentages of a fixed size on disk instead of percentages of the physically available disk size. This should # be useful when we have a large disk and we only want to use a part of it for Cassandra's data directories. # Valid values are in [1, max available disk size of all data directories]. # Defaults to null to disable and use the physically available disk size of data directories during calculations. # data_disk_usage_max_disk_size: +# # Guardrail to warn or fail when the minimum replication factor is lesser than threshold. # This would also apply to system keyspaces. # Suggested value for use in production: 2 or higher diff --git a/src/java/org/apache/cassandra/config/Config.java b/src/java/org/apache/cassandra/config/Config.java index 24b902978e..af0efbefca 100644 --- a/src/java/org/apache/cassandra/config/Config.java +++ b/src/java/org/apache/cassandra/config/Config.java @@ -825,6 +825,7 @@ public class Config public volatile Set write_consistency_levels_warned = Collections.emptySet(); public volatile Set write_consistency_levels_disallowed = Collections.emptySet(); public volatile boolean user_timestamps_enabled = true; + public volatile boolean alter_table_enabled = true; public volatile boolean group_by_enabled = true; public volatile boolean drop_truncate_table_enabled = true; public volatile boolean secondary_indexes_enabled = true; diff --git a/src/java/org/apache/cassandra/config/GuardrailsOptions.java b/src/java/org/apache/cassandra/config/GuardrailsOptions.java index a5144b941b..301a2527ee 100644 --- a/src/java/org/apache/cassandra/config/GuardrailsOptions.java +++ b/src/java/org/apache/cassandra/config/GuardrailsOptions.java @@ -385,6 +385,20 @@ public class GuardrailsOptions implements GuardrailsConfig x -> config.compact_tables_enabled = x); } + @Override + public boolean getAlterTableEnabled() + { + return config.alter_table_enabled; + } + + public void setAlterTableEnabled(boolean enabled) + { + updatePropertyWithLogging("alter_table_enabled", + enabled, + () -> config.alter_table_enabled, + x -> config.alter_table_enabled = x); + } + @Override public boolean getReadBeforeWriteListOperationsEnabled() { diff --git a/src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java b/src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java index 2736252688..94f72d427a 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java @@ -79,6 +79,7 @@ public abstract class AlterTableStatement extends AlterSchemaStatement { protected final String tableName; private final boolean ifExists; + protected ClientState state; public AlterTableStatement(String keyspaceName, String tableName, boolean ifExists) { @@ -87,6 +88,15 @@ public abstract class AlterTableStatement extends AlterSchemaStatement this.ifExists = ifExists; } + @Override + public void validate(ClientState state) + { + super.validate(state); + + // save the query state to use it for guardrails validation in #apply + this.state = state; + } + public Keyspaces apply(Keyspaces schema) { KeyspaceMetadata keyspace = schema.getNullable(keyspaceName); @@ -187,6 +197,7 @@ public abstract class AlterTableStatement extends AlterSchemaStatement public KeyspaceMetadata apply(KeyspaceMetadata keyspace, TableMetadata table) { + Guardrails.alterTableEnabled.ensureEnabled("ALTER TABLE changing columns", state); TableMetadata.Builder tableBuilder = table.unbuild(); Views.Builder viewsBuilder = keyspace.views.unbuild(); newColumns.forEach(c -> addColumn(keyspace, table, c, ifColumnNotExists, tableBuilder, viewsBuilder)); @@ -289,6 +300,7 @@ public abstract class AlterTableStatement extends AlterSchemaStatement public KeyspaceMetadata apply(KeyspaceMetadata keyspace, TableMetadata table) { + Guardrails.alterTableEnabled.ensureEnabled("ALTER TABLE changing columns", state); TableMetadata.Builder builder = table.unbuild(); removedColumns.forEach(c -> dropColumn(keyspace, table, c, ifColumnExists, builder)); return keyspace.withSwapped(keyspace.tables.withSwapped(builder.build())); @@ -356,6 +368,7 @@ public abstract class AlterTableStatement extends AlterSchemaStatement public KeyspaceMetadata apply(KeyspaceMetadata keyspace, TableMetadata table) { + Guardrails.alterTableEnabled.ensureEnabled("ALTER TABLE changing columns", state); TableMetadata.Builder tableBuilder = table.unbuild(); Views.Builder viewsBuilder = keyspace.views.unbuild(); renamedColumns.forEach((o, n) -> renameColumn(keyspace, table, o, n, ifColumnsExists, tableBuilder, viewsBuilder)); diff --git a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java index a4811ac7f6..a991f69f6e 100644 --- a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java +++ b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java @@ -145,6 +145,14 @@ public final class Guardrails implements GuardrailsMBean state -> CONFIG_PROVIDER.getOrCreate(state).getGroupByEnabled(), "GROUP BY functionality"); + /** + * Guardrail disabling ALTER TABLE column mutation access. + */ + public static final EnableFlag alterTableEnabled = + new EnableFlag("alter_table", + state -> CONFIG_PROVIDER.getOrCreate(state).getAlterTableEnabled(), + "User access to ALTER TABLE statement for column mutation"); + public static final EnableFlag dropTruncateTableEnabled = new EnableFlag("drop_truncate_table_enabled", state -> CONFIG_PROVIDER.getOrCreate(state).getDropTruncateTableEnabled(), @@ -539,6 +547,18 @@ public final class Guardrails implements GuardrailsMBean DEFAULT_CONFIG.setUserTimestampsEnabled(enabled); } + @Override + public boolean getAlterTableEnabled() + { + return DEFAULT_CONFIG.getAlterTableEnabled(); + } + + @Override + public void setAlterTableEnabled(boolean enabled) + { + DEFAULT_CONFIG.setAlterTableEnabled(enabled); + } + @Override public boolean getAllowFilteringEnabled() { diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java index e33ea5ba41..4fcfb18823 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java @@ -132,6 +132,13 @@ public interface GuardrailsConfig */ boolean getUserTimestampsEnabled(); + /** + * Returns whether users are allowed access to the ALTER TABLE statement to mutate columns or not + * + * @return {@code true} if ALTER TABLE ADD/REMOVE/RENAME is allowed, {@code false} otherwise. + */ + boolean getAlterTableEnabled(); + /** * Returns whether tables can be uncompressed * diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java index ad2eddaec0..30be464d02 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java @@ -250,6 +250,20 @@ public interface GuardrailsMBean */ void setCompactTablesEnabled(boolean enabled); + /** + * Gets whether users can use the ALTER TABLE statement to change columns + * + * @return {@code true} if ALTER TABLE is allowed, {@code false} otherwise. + */ + boolean getAlterTableEnabled(); + + /** + * Sets whether users can use the ALTER TABLE statement to change columns + * + * @param enabled {@code true} if changing columns is allowed, {@code false} otherwise. + */ + void setAlterTableEnabled(boolean enabled); + /** * Returns whether GROUP BY queries are allowed. * diff --git a/src/java/org/apache/cassandra/service/StorageService.java b/src/java/org/apache/cassandra/service/StorageService.java index dd35c70fdd..eb662493f0 100644 --- a/src/java/org/apache/cassandra/service/StorageService.java +++ b/src/java/org/apache/cassandra/service/StorageService.java @@ -17,7 +17,6 @@ */ package org.apache.cassandra.service; - import java.io.ByteArrayInputStream; import java.io.DataInputStream; import java.io.IOError; diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailAlterTableTest.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailAlterTableTest.java new file mode 100644 index 0000000000..1aa7095e19 --- /dev/null +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailAlterTableTest.java @@ -0,0 +1,133 @@ +/* + * 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.Before; +import org.junit.Test; + + +/** + * Tests the guardrail for disabling user access to the ALTER TABLE statement, {@link Guardrails#alterTableEnabled}. + * + * NOTE: This test class depends on {@link #currentTable()} method for setup, cleanup, and execution of tests. You'll + * need to refactor this if you add tests that make changes to the current table as the test teardown will no longer match + * setup. + */ +public class GuardrailAlterTableTest extends GuardrailTester +{ + public GuardrailAlterTableTest() + { + super(Guardrails.alterTableEnabled); + } + + @Before + public void setupTest() throws Throwable + { + createTable("CREATE TABLE IF NOT EXISTS %s (k INT, c INT, v TEXT, PRIMARY KEY(k, c))"); + } + + @After + public void afterTest() throws Throwable + { + dropTable("DROP TABLE %s"); + setGuardrail(true); + } + + private void setGuardrail(boolean alterTableEnabled) + { + guardrails().setAlterTableEnabled(alterTableEnabled); + } + + /** + * Confirm that ALTER TABLE queries either work (guardrail enabled) or fail (guardrail disabled) appropriately + * @throws Throwable + */ + @Test + public void testGuardrailEnabledAndDisabled() throws Throwable + { + setGuardrail(false); + assertFails("ALTER TABLE %s ADD test_one text;", "changing columns"); + + setGuardrail(true); + assertValid("ALTER TABLE %s ADD test_two text;"); + + setGuardrail(false); + assertFails("ALTER TABLE %s ADD test_three text;", "changing columns"); + } + + /** + * Confirm the guardrail appropriately catches the ALTER DROP case on a column + * @throws Throwable + */ + @Test + public void testAppliesToAlterDropColumn() throws Throwable + { + setGuardrail(true); + assertValid("ALTER TABLE %s ADD test_one text;"); + + setGuardrail(false); + assertFails("ALTER TABLE %s DROP test_one", "changing columns"); + + setGuardrail(true); + assertValid("ALTER TABLE %s DROP test_one"); + } + + /** + * Confirm the guardrail appropriately catches the ALTER RENAME case on a column + * @throws Throwable + */ + @Test + public void testAppliesToAlterRenameColumn() throws Throwable + { + setGuardrail(false); + assertFails("ALTER TABLE %s RENAME c TO renamed_c", "changing columns"); + + setGuardrail(true); + assertValid("ALTER TABLE %s RENAME c TO renamed_c"); + } + + /** + * Confirm we can always alter properties via the options map regardless of guardrail state + * @throws Throwable + */ + @Test + public void testAlterViaMapAlwaysWorks() throws Throwable + { + setGuardrail(false); + assertValid("ALTER TABLE %s WITH compression = { 'class' : 'SnappyCompressor', 'chunk_length_in_kb' : 32 };"); + + setGuardrail(true); + assertValid("ALTER TABLE %s WITH compression = { 'class' : 'SnappyCompressor', 'chunk_length_in_kb' : 32 };"); + } + + /** + * Confirm the other form of ALTER TABLE property map changing always works regardless of guardrail state + * @throws Throwable + */ + @Test + public void testAlterOptionsAlwaysWorks() throws Throwable + { + setGuardrail(true); + assertValid("ALTER TABLE %s WITH GC_GRACE_SECONDS = 456; "); + + setGuardrail(false); + assertValid("ALTER TABLE %s WITH GC_GRACE_SECONDS = 123; "); + } +}