This is an automated email from the ASF dual-hosted git repository. adelapena 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 a41040c Add guardrails for read/write consistency levels a41040c is described below commit a41040ccdcec651bffb4d23843ab9be2d96ba1d1 Author: Andrés de la Peña <a.penya.gar...@gmail.com> AuthorDate: Fri Mar 11 15:36:39 2022 +0000 Add guardrails for read/write consistency levels patch by Andrés de la Peña; reviewed by Caleb Rackliffe for CASSANDRA-17188 Co-authored-by: Andrés de la Peña <a.penya.gar...@gmail.com> Co-authored-by: Aleksandr Sorokoumov <aleksandr.sorokou...@gmail.com> --- CHANGES.txt | 1 + conf/cassandra.yaml | 9 +- src/java/org/apache/cassandra/config/Config.java | 5 + .../apache/cassandra/config/GuardrailsOptions.java | 84 +++++++++ .../cassandra/cql3/statements/BatchStatement.java | 4 + .../cql3/statements/ModificationStatement.java | 3 + .../cassandra/cql3/statements/SelectStatement.java | 1 + .../org/apache/cassandra/db/ConsistencyLevel.java | 7 + .../apache/cassandra/db/guardrails/Guardrails.java | 166 +++++++++++++++- .../cassandra/db/guardrails/GuardrailsConfig.java | 27 +++ .../cassandra/db/guardrails/GuardrailsMBean.java | 102 ++++++++++ .../org/apache/cassandra/db/guardrails/Values.java | 45 ++++- .../config/DatabaseDescriptorRefTest.java | 2 + .../GuardrailConsistencyLevelsTester.java | 131 +++++++++++++ .../GuardrailReadConsistencyLevelsTest.java | 118 ++++++++++++ .../guardrails/GuardrailTablePropertiesTest.java | 51 ++++- .../cassandra/db/guardrails/GuardrailTester.java | 63 ++++++- .../GuardrailWriteConsistencyLevelsTest.java | 208 +++++++++++++++++++++ .../cassandra/db/guardrails/GuardrailsTest.java | 124 +++++++----- 19 files changed, 1082 insertions(+), 69 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 8a7f5d5..f22e088 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 4.1 + * Add guardrails for read/write consistency levels (CASSANDRA-17188) * Add guardrail for SELECT IN terms and their cartesian product (CASSANDRA-17187) * remove unused imports in cqlsh.py and cqlshlib (CASSANDRA-17413) * deprecate property windows_timer_interval (CASSANDRA-17404) diff --git a/conf/cassandra.yaml b/conf/cassandra.yaml index ef03795..8df5771 100644 --- a/conf/cassandra.yaml +++ b/conf/cassandra.yaml @@ -1604,7 +1604,8 @@ drop_compact_storage_enabled: false # The two thresholds default to -1 to disable. # materialized_views_per_table_warn_threshold: -1 # materialized_views_per_table_fail_threshold: -1 -# Guardrail to ignore or reject properties when creating tables. By default all properties are allowed. +# 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. @@ -1625,6 +1626,12 @@ drop_compact_storage_enabled: false # 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: [] # Startup Checks are executed as part of Cassandra startup process, not all of them # are configurable (so you can disable them) but these which are enumerated bellow. diff --git a/src/java/org/apache/cassandra/config/Config.java b/src/java/org/apache/cassandra/config/Config.java index 4c0f29a..671e49d 100644 --- a/src/java/org/apache/cassandra/config/Config.java +++ b/src/java/org/apache/cassandra/config/Config.java @@ -788,8 +788,13 @@ public class Config public volatile int partition_keys_in_select_fail_threshold = DISABLED_GUARDRAIL; public volatile int in_select_cartesian_product_warn_threshold = DISABLED_GUARDRAIL; public volatile int in_select_cartesian_product_fail_threshold = DISABLED_GUARDRAIL; + public volatile Set<String> table_properties_warned = Collections.emptySet(); public volatile Set<String> table_properties_ignored = Collections.emptySet(); public volatile Set<String> table_properties_disallowed = Collections.emptySet(); + public volatile Set<ConsistencyLevel> read_consistency_levels_warned = Collections.emptySet(); + public volatile Set<ConsistencyLevel> read_consistency_levels_disallowed = Collections.emptySet(); + public volatile Set<ConsistencyLevel> write_consistency_levels_warned = Collections.emptySet(); + public volatile Set<ConsistencyLevel> write_consistency_levels_disallowed = Collections.emptySet(); public volatile boolean user_timestamps_enabled = true; public volatile boolean read_before_write_list_operations_enabled = true; diff --git a/src/java/org/apache/cassandra/config/GuardrailsOptions.java b/src/java/org/apache/cassandra/config/GuardrailsOptions.java index d4de0f2..a8b7ada 100644 --- a/src/java/org/apache/cassandra/config/GuardrailsOptions.java +++ b/src/java/org/apache/cassandra/config/GuardrailsOptions.java @@ -18,6 +18,7 @@ package org.apache.cassandra.config; +import java.util.Collections; import java.util.Set; import java.util.function.Consumer; import java.util.function.Supplier; @@ -27,6 +28,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.apache.cassandra.cql3.statements.schema.TableAttributes; +import org.apache.cassandra.db.ConsistencyLevel; import org.apache.cassandra.db.guardrails.Guardrails; import org.apache.cassandra.db.guardrails.GuardrailsConfig; @@ -64,12 +66,17 @@ public class GuardrailsOptions implements GuardrailsConfig validateIntThreshold(config.columns_per_table_warn_threshold, config.columns_per_table_fail_threshold, "columns_per_table"); validateIntThreshold(config.secondary_indexes_per_table_warn_threshold, config.secondary_indexes_per_table_fail_threshold, "secondary_indexes_per_table"); validateIntThreshold(config.materialized_views_per_table_warn_threshold, config.materialized_views_per_table_fail_threshold, "materialized_views_per_table"); + 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"); validateIntThreshold(config.page_size_warn_threshold, config.page_size_fail_threshold, "page_size"); validateIntThreshold(config.partition_keys_in_select_warn_threshold, config.partition_keys_in_select_fail_threshold, "partition_keys_in_select"); validateIntThreshold(config.in_select_cartesian_product_warn_threshold, config.in_select_cartesian_product_fail_threshold, "in_select_cartesian_product"); + config.read_consistency_levels_warned = validateConsistencyLevels(config.read_consistency_levels_warned, "read_consistency_levels_warned"); + config.read_consistency_levels_disallowed = validateConsistencyLevels(config.read_consistency_levels_disallowed, "read_consistency_levels_disallowed"); + config.write_consistency_levels_warned = validateConsistencyLevels(config.write_consistency_levels_warned, "write_consistency_levels_warned"); + config.write_consistency_levels_disallowed = validateConsistencyLevels(config.write_consistency_levels_disallowed, "write_consistency_levels_disallowed"); } @Override @@ -267,6 +274,20 @@ public class GuardrailsOptions implements GuardrailsConfig } @Override + public Set<String> getTablePropertiesWarned() + { + return config.table_properties_warned; + } + + public void setTablePropertiesWarned(Set<String> properties) + { + updatePropertyWithLogging("table_properties_warned", + validateTableProperties(properties, "table_properties_warned"), + () -> config.table_properties_warned, + x -> config.table_properties_warned = x); + } + + @Override public Set<String> getTablePropertiesIgnored() { return config.table_properties_ignored; @@ -347,6 +368,61 @@ public class GuardrailsOptions implements GuardrailsConfig x -> config.in_select_cartesian_product_fail_threshold = x); } + public Set<ConsistencyLevel> getReadConsistencyLevelsWarned() + { + return config.read_consistency_levels_warned; + } + + public void setReadConsistencyLevelsWarned(Set<ConsistencyLevel> consistencyLevels) + { + updatePropertyWithLogging("read_consistency_levels_warned", + validateConsistencyLevels(consistencyLevels, "read_consistency_levels_warned"), + () -> config.read_consistency_levels_warned, + x -> config.read_consistency_levels_warned = x); + } + + @Override + public Set<ConsistencyLevel> getReadConsistencyLevelsDisallowed() + { + return config.read_consistency_levels_disallowed; + } + + public void setReadConsistencyLevelsDisallowed(Set<ConsistencyLevel> consistencyLevels) + { + updatePropertyWithLogging("read_consistency_levels_disallowed", + validateConsistencyLevels(consistencyLevels, "read_consistency_levels_disallowed"), + () -> config.read_consistency_levels_disallowed, + x -> config.read_consistency_levels_disallowed = x); + } + + @Override + public Set<ConsistencyLevel> getWriteConsistencyLevelsWarned() + { + return config.write_consistency_levels_warned; + } + + public void setWriteConsistencyLevelsWarned(Set<ConsistencyLevel> consistencyLevels) + { + updatePropertyWithLogging("write_consistency_levels_warned", + validateConsistencyLevels(consistencyLevels, "write_consistency_levels_warned"), + () -> config.write_consistency_levels_warned, + x -> config.write_consistency_levels_warned = x); + } + + @Override + public Set<ConsistencyLevel> getWriteConsistencyLevelsDisallowed() + { + return config.write_consistency_levels_disallowed; + } + + public void setWriteConsistencyLevelsDisallowed(Set<ConsistencyLevel> consistencyLevels) + { + updatePropertyWithLogging("write_consistency_levels_disallowed", + validateConsistencyLevels(consistencyLevels, "write_consistency_levels_disallowed"), + () -> config.write_consistency_levels_disallowed, + x -> config.write_consistency_levels_disallowed = x); + } + private static <T> void updatePropertyWithLogging(String propertyName, T newValue, Supplier<T> getter, Consumer<T> setter) { T oldValue = getter.get(); @@ -415,4 +491,12 @@ public class GuardrailsOptions implements GuardrailsConfig return lowerCaseProperties; } + + private static Set<ConsistencyLevel> validateConsistencyLevels(Set<ConsistencyLevel> consistencyLevels, String name) + { + if (consistencyLevels == null) + throw new IllegalArgumentException(format("Invalid value for %s: null is not allowed", name)); + + return consistencyLevels.isEmpty() ? Collections.emptySet() : Sets.immutableEnumSet(consistencyLevels); + } } diff --git a/src/java/org/apache/cassandra/cql3/statements/BatchStatement.java b/src/java/org/apache/cassandra/cql3/statements/BatchStatement.java index 4f537ad..89ece02 100644 --- a/src/java/org/apache/cassandra/cql3/statements/BatchStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/BatchStatement.java @@ -31,6 +31,7 @@ import org.slf4j.helpers.MessageFormatter; import org.apache.cassandra.audit.AuditLogContext; import org.apache.cassandra.audit.AuditLogEntryType; +import org.apache.cassandra.db.guardrails.Guardrails; import org.apache.cassandra.schema.TableId; import org.apache.cassandra.schema.TableMetadata; import org.apache.cassandra.schema.ColumnMetadata; @@ -410,6 +411,9 @@ public class BatchStatement implements CQLStatement if (options.getSerialConsistency() == null) throw new InvalidRequestException("Invalid empty serial consistency level"); + Guardrails.writeConsistencyLevels.guard(EnumSet.of(options.getConsistency(), options.getSerialConsistency()), + queryState.getClientState()); + if (hasConditions) return executeWithConditions(options, queryState, queryStartNanoTime); diff --git a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java index 52578a6..7a467ce 100644 --- a/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java @@ -460,6 +460,9 @@ public abstract class ModificationStatement implements CQLStatement.SingleKeyspa if (options.getConsistency() == null) throw new InvalidRequestException("Invalid empty consistency level"); + Guardrails.writeConsistencyLevels.guard(EnumSet.of(options.getConsistency(), options.getSerialConsistency()), + queryState.getClientState()); + return hasConditions() ? executeWithCondition(queryState, options, queryStartNanoTime) : executeWithoutCondition(queryState, options, queryStartNanoTime); diff --git a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java index e5f4d75..54ea8c8 100644 --- a/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/SelectStatement.java @@ -240,6 +240,7 @@ public class SelectStatement implements CQLStatement.SingleKeyspaceCqlStatement checkNotNull(cl, "Invalid empty consistency level"); cl.validateForRead(); + Guardrails.readConsistencyLevels.guard(EnumSet.of(cl), state.getClientState()); int nowInSec = options.getNowInSeconds(state); int userLimit = getLimit(options); diff --git a/src/java/org/apache/cassandra/db/ConsistencyLevel.java b/src/java/org/apache/cassandra/db/ConsistencyLevel.java index ccbd918..843ccb9 100644 --- a/src/java/org/apache/cassandra/db/ConsistencyLevel.java +++ b/src/java/org/apache/cassandra/db/ConsistencyLevel.java @@ -18,6 +18,8 @@ package org.apache.cassandra.db; +import java.util.Locale; + import com.carrotsearch.hppc.ObjectIntHashMap; import org.apache.cassandra.locator.Endpoints; import org.apache.cassandra.locator.InOurDc; @@ -81,6 +83,11 @@ public enum ConsistencyLevel return codeIdx[code]; } + public static ConsistencyLevel fromString(String str) + { + return valueOf(str.toUpperCase(Locale.US)); + } + public static int quorumFor(AbstractReplicationStrategy replicationStrategy) { return (replicationStrategy.getReplicationFactor().allReplicas / 2) + 1; diff --git a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java index 13ac27c..b100df5 100644 --- a/src/java/org/apache/cassandra/db/guardrails/Guardrails.java +++ b/src/java/org/apache/cassandra/db/guardrails/Guardrails.java @@ -18,13 +18,18 @@ package org.apache.cassandra.db.guardrails; +import java.util.Collections; import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; +import org.apache.commons.lang3.StringUtils; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.config.GuardrailsOptions; +import org.apache.cassandra.db.ConsistencyLevel; import org.apache.cassandra.service.ClientState; import org.apache.cassandra.utils.MBeanWrapper; @@ -101,10 +106,11 @@ public final class Guardrails implements GuardrailsMBean threshold, what)); /** - * Guardrail ignoring/disallowing the usage of certain table properties. + * Guardrail warning about, ignoring or rejecting the usage of certain table properties. */ public static final Values<String> tableProperties = - new Values<>(state -> CONFIG_PROVIDER.getOrCreate(state).getTablePropertiesIgnored(), + new Values<>(state -> CONFIG_PROVIDER.getOrCreate(state).getTablePropertiesWarned(), + state -> CONFIG_PROVIDER.getOrCreate(state).getTablePropertiesIgnored(), state -> CONFIG_PROVIDER.getOrCreate(state).getTablePropertiesDisallowed(), "Table Properties"); @@ -161,6 +167,23 @@ public final class Guardrails implements GuardrailsMBean : format("Aborting query because the cartesian product of the IN restrictions on %s " + "produces %d values, this exceeds fail threshold of %s.", what, value, threshold)); + /** + * Guardrail on read consistency levels. + */ + public static final Values<ConsistencyLevel> readConsistencyLevels = + new Values<>(state -> CONFIG_PROVIDER.getOrCreate(state).getReadConsistencyLevelsWarned(), + state -> Collections.emptySet(), + state -> CONFIG_PROVIDER.getOrCreate(state).getReadConsistencyLevelsDisallowed(), + "read consistency levels"); + + /** + * Guardrail on write consistency levels. + */ + public static final Values<ConsistencyLevel> writeConsistencyLevels = + new Values<>(state -> CONFIG_PROVIDER.getOrCreate(state).getWriteConsistencyLevelsWarned(), + state -> Collections.emptySet(), + state -> CONFIG_PROVIDER.getOrCreate(state).getWriteConsistencyLevelsDisallowed(), + "write consistency levels"); private Guardrails() { @@ -281,6 +304,35 @@ public final class Guardrails implements GuardrailsMBean } @Override + public Set<String> getTablePropertiesWarned() + { + return DEFAULT_CONFIG.getTablePropertiesWarned(); + } + + @Override + public String getTablePropertiesWarnedCSV() + { + return toCSV(DEFAULT_CONFIG.getTablePropertiesWarned()); + } + + public void setTablePropertiesWarned(String... properties) + { + setTablePropertiesWarned(ImmutableSet.copyOf(properties)); + } + + @Override + public void setTablePropertiesWarned(Set<String> properties) + { + DEFAULT_CONFIG.setTablePropertiesWarned(properties); + } + + @Override + public void setTablePropertiesWarnedCSV(String properties) + { + setTablePropertiesWarned(fromCSV(properties)); + } + + @Override public Set<String> getTablePropertiesDisallowed() { return DEFAULT_CONFIG.getTablePropertiesDisallowed(); @@ -368,6 +420,7 @@ public final class Guardrails implements GuardrailsMBean DEFAULT_CONFIG.setPageSizeThreshold(warn, fail); } + @Override public boolean getReadBeforeWriteListOperationsEnabled() { return DEFAULT_CONFIG.getReadBeforeWriteListOperationsEnabled(); @@ -415,13 +468,118 @@ public final class Guardrails implements GuardrailsMBean DEFAULT_CONFIG.setInSelectCartesianProductThreshold(warn, fail); } + public Set<ConsistencyLevel> getReadConsistencyLevelsWarned() + { + return DEFAULT_CONFIG.getReadConsistencyLevelsWarned(); + } + + @Override + public String getReadConsistencyLevelsWarnedCSV() + { + return toCSV(DEFAULT_CONFIG.getReadConsistencyLevelsWarned(), ConsistencyLevel::toString); + } + + @Override + public void setReadConsistencyLevelsWarned(Set<ConsistencyLevel> consistencyLevels) + { + DEFAULT_CONFIG.setReadConsistencyLevelsWarned(consistencyLevels); + } + + @Override + public void setReadConsistencyLevelsWarnedCSV(String consistencyLevels) + { + DEFAULT_CONFIG.setReadConsistencyLevelsWarned(fromCSV(consistencyLevels, ConsistencyLevel::fromString)); + } + + @Override + public Set<ConsistencyLevel> getReadConsistencyLevelsDisallowed() + { + return DEFAULT_CONFIG.getReadConsistencyLevelsDisallowed(); + } + + @Override + public String getReadConsistencyLevelsDisallowedCSV() + { + return toCSV(DEFAULT_CONFIG.getReadConsistencyLevelsDisallowed(), ConsistencyLevel::toString); + } + + @Override + public void setReadConsistencyLevelsDisallowed(Set<ConsistencyLevel> consistencyLevels) + { + DEFAULT_CONFIG.setReadConsistencyLevelsDisallowed(consistencyLevels); + } + + @Override + public void setReadConsistencyLevelsDisallowedCSV(String consistencyLevels) + { + DEFAULT_CONFIG.setReadConsistencyLevelsDisallowed(fromCSV(consistencyLevels, ConsistencyLevel::fromString)); + } + + @Override + public Set<ConsistencyLevel> getWriteConsistencyLevelsWarned() + { + return DEFAULT_CONFIG.getWriteConsistencyLevelsWarned(); + } + + @Override + public String getWriteConsistencyLevelsWarnedCSV() + { + return toCSV(DEFAULT_CONFIG.getWriteConsistencyLevelsWarned(), ConsistencyLevel::toString); + } + + @Override + public void setWriteConsistencyLevelsWarned(Set<ConsistencyLevel> consistencyLevels) + { + DEFAULT_CONFIG.setWriteConsistencyLevelsWarned(consistencyLevels); + } + + @Override + public void setWriteConsistencyLevelsWarnedCSV(String consistencyLevels) + { + DEFAULT_CONFIG.setWriteConsistencyLevelsWarned(fromCSV(consistencyLevels, ConsistencyLevel::fromString)); + } + + @Override + public Set<ConsistencyLevel> getWriteConsistencyLevelsDisallowed() + { + return DEFAULT_CONFIG.getWriteConsistencyLevelsDisallowed(); + } + + @Override + public String getWriteConsistencyLevelsDisallowedCSV() + { + return toCSV(DEFAULT_CONFIG.getWriteConsistencyLevelsDisallowed(), ConsistencyLevel::toString); + } + + @Override + public void setWriteConsistencyLevelsDisallowed(Set<ConsistencyLevel> consistencyLevels) + { + DEFAULT_CONFIG.setWriteConsistencyLevelsDisallowed(consistencyLevels); + } + + @Override + public void setWriteConsistencyLevelsDisallowedCSV(String consistencyLevels) + { + DEFAULT_CONFIG.setWriteConsistencyLevelsDisallowed(fromCSV(consistencyLevels, ConsistencyLevel::fromString)); + } + private static String toCSV(Set<String> values) { - return values == null ? "" : String.join(",", values); + return values == null || values.isEmpty() ? "" : String.join(",", values); + } + + private static <T> String toCSV(Set<T> values, Function<T, String> formatter) + { + return values == null || values.isEmpty() ? "" : values.stream().map(formatter).collect(Collectors.joining(",")); } private static Set<String> fromCSV(String csv) { - return csv == null ? null : ImmutableSet.copyOf(csv.split(",")); + return StringUtils.isEmpty(csv) ? Collections.emptySet() : ImmutableSet.copyOf(csv.split(",")); + } + + private static <T> Set<T> fromCSV(String csv, Function<String, T> parser) + { + return StringUtils.isEmpty(csv) ? Collections.emptySet() : fromCSV(csv).stream().map(parser).collect(Collectors.toSet()); } } diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java index 2586436..fc671e7 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java @@ -20,6 +20,8 @@ package org.apache.cassandra.db.guardrails; import java.util.Set; +import org.apache.cassandra.db.ConsistencyLevel; + /** * Configuration settings for guardrails. * @@ -112,6 +114,11 @@ public interface GuardrailsConfig int getMaterializedViewsPerTableFailThreshold(); /** + * @return The table properties that are warned about when creating or altering a table. + */ + Set<String> getTablePropertiesWarned(); + + /** * @return The table properties that are ignored when creating or altering a table. */ Set<String> getTablePropertiesIgnored(); @@ -156,4 +163,24 @@ public interface GuardrailsConfig * -1 means disabled. */ public int getInSelectCartesianProductFailThreshold(); + + /** + * @return The consistency levels that are warned about when reading. + */ + Set<ConsistencyLevel> getReadConsistencyLevelsWarned(); + + /** + * @return The consistency levels that are disallowed when reading. + */ + Set<ConsistencyLevel> getReadConsistencyLevelsDisallowed(); + + /** + * @return The consistency levels that are warned about when writing. + */ + Set<ConsistencyLevel> getWriteConsistencyLevelsWarned(); + + /** + * @return The consistency levels that are disallowed when writing. + */ + Set<ConsistencyLevel> getWriteConsistencyLevelsDisallowed(); } diff --git a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java index b6ed551..6b59b05 100644 --- a/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java +++ b/src/java/org/apache/cassandra/db/guardrails/GuardrailsMBean.java @@ -20,6 +20,8 @@ package org.apache.cassandra.db.guardrails; import java.util.Set; +import org.apache.cassandra.db.ConsistencyLevel; + /** * JMX entrypoint for updating the default guardrails configuration parsed from {@code cassandra.yaml}. * <p> @@ -136,6 +138,26 @@ public interface GuardrailsMBean void setMaterializedViewsPerTableThreshold(int warn, int fail); /** + * @return properties that are warned about when creating or altering a table. + */ + Set<String> getTablePropertiesWarned(); + + /** + * @return Comma-separated list of properties that are warned about when creating or altering a table. + */ + String getTablePropertiesWarnedCSV(); + + /** + * @param properties properties that are warned about when creating or altering a table. + */ + void setTablePropertiesWarned(Set<String> properties); + + /** + * @param properties Comma-separated list of properties that are warned about when creating or altering a table. + */ + void setTablePropertiesWarnedCSV(String properties); + + /** * @return properties that are not allowed when creating or altering a table. */ Set<String> getTablePropertiesDisallowed(); @@ -260,4 +282,84 @@ public interface GuardrailsMBean * -1 means disabled. */ public void setInSelectCartesianProductThreshold(int warn, int fail); + + /** + * @return consistency levels that are warned about when reading. + */ + Set<ConsistencyLevel> getReadConsistencyLevelsWarned(); + + /** + * @return Comma-separated list of consistency levels that are warned about when reading. + */ + String getReadConsistencyLevelsWarnedCSV(); + + /** + * @param consistencyLevels consistency levels that are warned about when reading. + */ + void setReadConsistencyLevelsWarned(Set<ConsistencyLevel> consistencyLevels); + + /** + * @param consistencyLevels Comma-separated list of consistency levels that are warned about when reading. + */ + void setReadConsistencyLevelsWarnedCSV(String consistencyLevels); + + /** + * @return consistency levels that are not allowed when reading. + */ + Set<ConsistencyLevel> getReadConsistencyLevelsDisallowed(); + + /** + * @return Comma-separated list of consistency levels that are not allowed when reading. + */ + String getReadConsistencyLevelsDisallowedCSV(); + + /** + * @param consistencyLevels consistency levels that are not allowed when reading. + */ + void setReadConsistencyLevelsDisallowed(Set<ConsistencyLevel> consistencyLevels); + + /** + * @param consistencyLevels Comma-separated list of consistency levels that are not allowed when reading. + */ + void setReadConsistencyLevelsDisallowedCSV(String consistencyLevels); + + /** + * @return consistency levels that are warned about when writing. + */ + Set<ConsistencyLevel> getWriteConsistencyLevelsWarned(); + + /** + * @return Comma-separated list of consistency levels that are warned about when writing. + */ + String getWriteConsistencyLevelsWarnedCSV(); + + /** + * @param consistencyLevels consistency levels that are warned about when writing. + */ + void setWriteConsistencyLevelsWarned(Set<ConsistencyLevel> consistencyLevels); + + /** + * @param consistencyLevels Comma-separated list of consistency levels that are warned about when writing. + */ + void setWriteConsistencyLevelsWarnedCSV(String consistencyLevels); + + /** + * @return consistency levels that are not allowed when writing. + */ + Set<ConsistencyLevel> getWriteConsistencyLevelsDisallowed(); + + /** + * @return Comma-separated list of consistency levels that are not allowed when writing. + */ + String getWriteConsistencyLevelsDisallowedCSV(); + + /** + * @param consistencyLevels consistency levels that are not allowed when writing. + */ + void setWriteConsistencyLevelsDisallowed(Set<ConsistencyLevel> consistencyLevels); + + /** + * @param consistencyLevels Comma-separated list of consistency levels that are not allowed when writing. + */ + void setWriteConsistencyLevelsDisallowedCSV(String consistencyLevels); } diff --git a/src/java/org/apache/cassandra/db/guardrails/Values.java b/src/java/org/apache/cassandra/db/guardrails/Values.java index f1a62bf..36bb171 100644 --- a/src/java/org/apache/cassandra/db/guardrails/Values.java +++ b/src/java/org/apache/cassandra/db/guardrails/Values.java @@ -31,12 +31,14 @@ import org.apache.cassandra.service.ClientState; import static java.lang.String.format; /** - * A guardrail that warns about but ignores some specific values, and rejects the use of some other values. + * A guardrail that warns about some specific values, warns about but ignores some other values, and/or rejects the use + * of some other values. * - * @param <T> The type of the values of which certain are disallowed. + * @param <T> The type of the values of which certain are warned, ignored and/or disallowed. */ public class Values<T> extends Guardrail { + private final Function<ClientState, Set<T>> warnedValues; private final Function<ClientState, Set<T>> ignoredValues; private final Function<ClientState, Set<T>> disallowedValues; private final String what; @@ -44,22 +46,45 @@ public class Values<T> extends Guardrail /** * Creates a new values guardrail. * - * @param ignoredValues a {@link ClientState}-based of the values that are ignored. - * @param disallowedValues a {@link ClientState}-based of the values that are disallowed. + * @param warnedValues a {@link ClientState}-based provider of the values for which a warning is triggered. + * @param ignoredValues a {@link ClientState}-based provider of the values that are ignored. + * @param disallowedValues a {@link ClientState}-based provider of the values that are disallowed. * @param what The feature that is guarded by this guardrail (for reporting in error messages). */ - public Values(Function<ClientState, Set<T>> ignoredValues, + public Values(Function<ClientState, Set<T>> warnedValues, + Function<ClientState, Set<T>> ignoredValues, Function<ClientState, Set<T>> disallowedValues, String what) { + this.warnedValues = warnedValues; this.ignoredValues = ignoredValues; this.disallowedValues = disallowedValues; this.what = what; } /** - * Triggers a warning for each of the provided values that are disallowed by this guardrail and triggers an action - * to ignore them. If the values are disallowed it will abort the operation. + * Triggers a warning for each of the provided values that is discouraged by this guardrail. If any of the values + * is disallowed it will abort the operation. + * <p> + * This assumes that there aren't any values to be ignored, thus it doesn't require an ignore action. If this is + * not the case and the provided value is set up to be ignored this will throw an assertion error. + * + * @param values The values to check. + * @param state The client state, used to skip the check if the query is internal or is done by a superuser. + * A {@code null} value means that the check should be done regardless of the query. + */ + public void guard(Set<T> values, @Nullable ClientState state) + { + guard(values, x -> { + throw new AssertionError(format("There isn't an ignore action for %s, but value %s is setup to be ignored", + what, x)); + }, state); + } + + /** + * Triggers a warning for each of the provided values that is discouraged by this guardrail. Also triggers a warning + * for each of the provided values that is ignored by this guardrail and triggers the provided action to ignore it. + * If any of the values is disallowed it will abort the operation. * * @param values The values to check. * @param ignoreAction An action called on the subset of {@code values} that should be ignored. This action @@ -86,5 +111,11 @@ public class Values<T> extends Guardrail toIgnore.stream().sorted().collect(Collectors.toList()), what, ignored)); toIgnore.forEach(ignoreAction); } + + Set<T> warned = warnedValues.apply(state); + Set<T> toWarn = Sets.intersection(values, warned); + if (!toWarn.isEmpty()) + warn(format("Provided values %s are not recommended for %s (warned values are: %s)", + toWarn.stream().sorted().collect(Collectors.toList()), what, warned)); } } diff --git a/test/unit/org/apache/cassandra/config/DatabaseDescriptorRefTest.java b/test/unit/org/apache/cassandra/config/DatabaseDescriptorRefTest.java index da06cb1..c1e771b 100644 --- a/test/unit/org/apache/cassandra/config/DatabaseDescriptorRefTest.java +++ b/test/unit/org/apache/cassandra/config/DatabaseDescriptorRefTest.java @@ -97,10 +97,12 @@ public class DatabaseDescriptorRefTest "org.apache.cassandra.config.EncryptionOptions$ServerEncryptionOptions$OutgoingEncryptedPortSource", "org.apache.cassandra.db.guardrails.GuardrailsConfig", "org.apache.cassandra.db.guardrails.GuardrailsConfigMBean", + "org.apache.cassandra.db.guardrails.GuardrailsConfig$ConsistencyLevels", "org.apache.cassandra.db.guardrails.GuardrailsConfig$IntThreshold", "org.apache.cassandra.db.guardrails.GuardrailsConfig$TableProperties", "org.apache.cassandra.config.GuardrailsOptions", "org.apache.cassandra.config.GuardrailsOptions$Config", + "org.apache.cassandra.config.GuardrailsOptions$ConsistencyLevels", "org.apache.cassandra.config.GuardrailsOptions$IntThreshold", "org.apache.cassandra.config.GuardrailsOptions$TableProperties", "org.apache.cassandra.config.GuardrailsOptions$Threshold", diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailConsistencyLevelsTester.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailConsistencyLevelsTester.java new file mode 100644 index 0000000..c81f49d --- /dev/null +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailConsistencyLevelsTester.java @@ -0,0 +1,131 @@ +/* + * 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 java.util.Collections; +import java.util.EnumSet; +import java.util.Set; +import java.util.function.BiConsumer; +import java.util.function.Function; +import java.util.stream.Collectors; + +import com.google.common.collect.ImmutableSet; +import org.junit.Before; +import org.junit.Test; + +import org.apache.cassandra.db.ConsistencyLevel; + +/** + * Utilty class for testing the guardrails for read/write consistency levels. + */ +public abstract class GuardrailConsistencyLevelsTester extends GuardrailTester +{ + private final String warnedPropertyName; + private final String disallowePropertyName; + private final Function<Guardrails, Set<ConsistencyLevel>> warnedGetter; + private final Function<Guardrails, Set<ConsistencyLevel>> disallowedGetter; + private final Function<Guardrails, String> warnedCSVGetter; + private final Function<Guardrails, String> disallowedCSVGetter; + private final BiConsumer<Guardrails, Set<ConsistencyLevel>> warnedSetter; + private final BiConsumer<Guardrails, Set<ConsistencyLevel>> disallowedSetter; + private final BiConsumer<Guardrails, String> warnedCSVSetter; + private final BiConsumer<Guardrails, String> disallowedCSVSetter; + + public GuardrailConsistencyLevelsTester(String warnedPropertyName, + String disallowePropertyName, + Function<Guardrails, Set<ConsistencyLevel>> warnedGetter, + Function<Guardrails, Set<ConsistencyLevel>> disallowedGetter, + Function<Guardrails, String> warnedCSVGetter, + Function<Guardrails, String> disallowedCSVGetter, + BiConsumer<Guardrails, Set<ConsistencyLevel>> warnedSetter, + BiConsumer<Guardrails, Set<ConsistencyLevel>> disallowedSetter, + BiConsumer<Guardrails, String> warnedCSVSetter, + BiConsumer<Guardrails, String> disallowedCSVSetter) + { + this.warnedPropertyName = warnedPropertyName; + this.disallowePropertyName = disallowePropertyName; + this.warnedGetter = warnedGetter; + this.disallowedGetter = disallowedGetter; + this.warnedCSVGetter = g -> sortCSV(warnedCSVGetter.apply(g)); + this.disallowedCSVGetter = g -> sortCSV(disallowedCSVGetter.apply(g)); + this.warnedSetter = warnedSetter; + this.disallowedSetter = disallowedSetter; + this.warnedCSVSetter = warnedCSVSetter; + this.disallowedCSVSetter = disallowedCSVSetter; + } + + @Before + public void before() + { + warnConsistencyLevels(); + disableConsistencyLevels(); + } + + protected void warnConsistencyLevels(ConsistencyLevel... consistencyLevels) + { + warnedSetter.accept(guardrails(), ImmutableSet.copyOf(consistencyLevels)); + } + + protected void disableConsistencyLevels(ConsistencyLevel... consistencyLevels) + { + disallowedSetter.accept(guardrails(), ImmutableSet.copyOf(consistencyLevels)); + } + + @Test + public void testConfigValidation() + { + String message = "Invalid value for %s: null is not allowed"; + assertInvalidProperty(warnedSetter, null, message, warnedPropertyName); + assertInvalidProperty(disallowedSetter, null, message, disallowePropertyName); + + assertValidProperty(Collections.emptySet()); + assertValidProperty(EnumSet.allOf(ConsistencyLevel.class)); + + assertValidPropertyCSV(""); + assertValidPropertyCSV(EnumSet.allOf(ConsistencyLevel.class) + .stream() + .map(ConsistencyLevel::toString) + .collect(Collectors.joining(","))); + + assertInvalidPropertyCSV("invalid", "INVALID"); + assertInvalidPropertyCSV("ONE,invalid1,invalid2", "INVALID1"); + assertInvalidPropertyCSV("invalid1,invalid2,ONE", "INVALID1"); + assertInvalidPropertyCSV("invalid1,ONE,invalid2", "INVALID1"); + } + + private void assertValidProperty(Set<ConsistencyLevel> properties) + { + assertValidProperty(warnedSetter, warnedGetter, properties); + assertValidProperty(disallowedSetter, disallowedGetter, properties); + } + + private void assertValidPropertyCSV(String csv) + { + csv = sortCSV(csv); + assertValidProperty(warnedCSVSetter, warnedCSVGetter, csv); + assertValidProperty(disallowedCSVSetter, disallowedCSVGetter, csv); + } + + private void assertInvalidPropertyCSV(String properties, String rejected) + { + String message = "No enum constant org.apache.cassandra.db.ConsistencyLevel.%s"; + assertInvalidProperty(warnedCSVSetter, properties, message, rejected); + assertInvalidProperty(disallowedCSVSetter, properties, message, rejected); + } +} diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailReadConsistencyLevelsTest.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailReadConsistencyLevelsTest.java new file mode 100644 index 0000000..dccd306 --- /dev/null +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailReadConsistencyLevelsTest.java @@ -0,0 +1,118 @@ +/* + * 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.Test; + +import org.apache.cassandra.db.ConsistencyLevel; + +import static java.lang.String.format; +import static org.apache.cassandra.db.ConsistencyLevel.ALL; +import static org.apache.cassandra.db.ConsistencyLevel.EACH_QUORUM; +import static org.apache.cassandra.db.ConsistencyLevel.LOCAL_ONE; +import static org.apache.cassandra.db.ConsistencyLevel.LOCAL_QUORUM; +import static org.apache.cassandra.db.ConsistencyLevel.ONE; +import static org.apache.cassandra.db.ConsistencyLevel.QUORUM; + +/** + * Tests the guardrail for read consistency levels, {@link Guardrails#readConsistencyLevels}. + */ +public class GuardrailReadConsistencyLevelsTest extends GuardrailConsistencyLevelsTester +{ + public GuardrailReadConsistencyLevelsTest() + { + super("read_consistency_levels_warned", + "read_consistency_levels_disallowed", + Guardrails::getReadConsistencyLevelsWarned, + Guardrails::getReadConsistencyLevelsDisallowed, + Guardrails::getReadConsistencyLevelsWarnedCSV, + Guardrails::getReadConsistencyLevelsDisallowedCSV, + Guardrails::setReadConsistencyLevelsWarned, + Guardrails::setReadConsistencyLevelsDisallowed, + Guardrails::setReadConsistencyLevelsWarnedCSV, + Guardrails::setReadConsistencyLevelsDisallowedCSV); + } + + @Test + public void testSelect() throws Throwable + { + createTable("CREATE TABLE IF NOT EXISTS %s (k INT, c INT, v INT, PRIMARY KEY(k, c))"); + + execute("INSERT INTO %s (k, c, v) VALUES (0, 0, 0)"); + execute("INSERT INTO %s (k, c, v) VALUES (0, 1, 1)"); + execute("INSERT INTO %s (k, c, v) VALUES (1, 0, 2)"); + execute("INSERT INTO %s (k, c, v) VALUES (1, 1, 3)"); + + testQuery("SELECT * FROM %s"); + testQuery("SELECT * FROM %s WHERE k = 0"); + testQuery("SELECT * FROM %s WHERE k = 0 AND c = 0"); + } + + private void testQuery(String query) throws Throwable + { + testQuery(query, ONE); + testQuery(query, ALL); + testQuery(query, QUORUM); + testQuery(query, EACH_QUORUM); + testQuery(query, LOCAL_ONE); + testQuery(query, LOCAL_QUORUM); + } + + private void testQuery(String query, ConsistencyLevel cl) throws Throwable + { + warnConsistencyLevels(); + disableConsistencyLevels(); + assertValid(query, cl); + + warnConsistencyLevels(cl); + assertWarns(query, cl); + + disableConsistencyLevels(cl); + assertFails(query, cl); + } + + private void assertValid(String query, ConsistencyLevel cl) throws Throwable + { + assertValid(() -> execute(userClientState, query, cl)); + } + + private void assertWarns(String query, ConsistencyLevel cl) throws Throwable + { + assertWarns(() -> execute(userClientState, query, cl), + format("Provided values [%s] are not recommended for read consistency levels (warned values are: %s)", + cl, guardrails().getReadConsistencyLevelsWarned())); + + assertExcludedUsers(query, cl); + } + + private void assertFails(String query, ConsistencyLevel cl) throws Throwable + { + assertFails(() -> execute(userClientState, query, cl), + format("Provided values [%s] are not allowed for read consistency levels (disallowed values are: %s)", + cl, guardrails().getReadConsistencyLevelsDisallowed())); + + assertExcludedUsers(query, cl); + } + + private void assertExcludedUsers(String query, ConsistencyLevel cl) throws Throwable + { + assertValid(() -> execute(superClientState, query, cl)); + assertValid(() -> execute(systemClientState, query, cl)); + } +} diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailTablePropertiesTest.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailTablePropertiesTest.java index 03e94dc..018700a 100644 --- a/test/unit/org/apache/cassandra/db/guardrails/GuardrailTablePropertiesTest.java +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailTablePropertiesTest.java @@ -45,51 +45,81 @@ public class GuardrailTablePropertiesTest extends GuardrailTester "WHERE pk IS NOT null and ck IS NOT null PRIMARY KEY(ck, pk) %s"; private static final String ALTER_VIEW = "ALTER MATERIALIZED VIEW %s.%s WITH %s"; + private static final String WARNED_PROPERTY_NAME = "table_properties_warned"; private static final String IGNORED_PROPERTY_NAME = "table_properties_ignored"; private static final String DISALLOWED_PROPERTY_NAME = "table_properties_disallowed"; @Before public void before() { - // only allow "gc_grace_seconds" and "comments" - Set<String> allowed = new HashSet<>(Arrays.asList("gc_grace_seconds", "comment")); + // only allow "gc_grace_seconds", "comments" and "default_time_to_live" + Set<String> allowed = new HashSet<>(Arrays.asList("gc_grace_seconds", "comment", "default_time_to_live")); guardrails().setTablePropertiesDisallowed(TableAttributes.validKeywords() .stream() .filter(p -> !allowed.contains(p)) .map(String::toUpperCase) .collect(Collectors.toSet())); - // but actually warn about "comment" - guardrails().setTablePropertiesIgnored(Collections.singleton("comment")); + // but actually ignore "comment" and warn about "default_time_to_live" + guardrails().setTablePropertiesIgnored("comment"); + guardrails().setTablePropertiesWarned("default_time_to_live"); } @Test public void testConfigValidation() { String message = "Invalid value for %s: null is not allowed"; + assertInvalidProperty(Guardrails::setTablePropertiesWarned, (Set<String>) null, message, WARNED_PROPERTY_NAME); assertInvalidProperty(Guardrails::setTablePropertiesIgnored, (Set<String>) null, message, IGNORED_PROPERTY_NAME); assertInvalidProperty(Guardrails::setTablePropertiesDisallowed, (Set<String>) null, message, DISALLOWED_PROPERTY_NAME); assertValidProperty(Collections.emptySet()); assertValidProperty(TableAttributes.allKeywords()); + + assertValidPropertyCSV(""); + assertValidPropertyCSV(String.join(",", TableAttributes.allKeywords())); + assertInvalidProperty(Collections.singleton("invalid"), Collections.singleton("invalid")); assertInvalidProperty(ImmutableSet.of("comment", "invalid1", "invalid2"), ImmutableSet.of("invalid1", "invalid2")); assertInvalidProperty(ImmutableSet.of("invalid1", "invalid2", "comment"), ImmutableSet.of("invalid1", "invalid2")); assertInvalidProperty(ImmutableSet.of("invalid1", "comment", "invalid2"), ImmutableSet.of("invalid1", "invalid2")); + + assertInvalidPropertyCSV("invalid", "[invalid]"); + assertInvalidPropertyCSV("comment,invalid1,invalid2", "[invalid1, invalid2]"); + assertInvalidPropertyCSV("invalid1,invalid2,comment", "[invalid1, invalid2]"); + assertInvalidPropertyCSV("invalid1,comment,invalid2", "[invalid1, invalid2]"); } private void assertValidProperty(Set<String> properties) { - assertValidProperty(Guardrails::setTablePropertiesIgnored, properties); - assertValidProperty(Guardrails::setTablePropertiesDisallowed, properties); + assertValidProperty(Guardrails::setTablePropertiesWarned, Guardrails::getTablePropertiesWarned, properties); + assertValidProperty(Guardrails::setTablePropertiesIgnored, Guardrails::getTablePropertiesIgnored, properties); + assertValidProperty(Guardrails::setTablePropertiesDisallowed, Guardrails::getTablePropertiesDisallowed, properties); + } + + private void assertValidPropertyCSV(String csv) + { + csv = sortCSV(csv); + assertValidProperty(Guardrails::setTablePropertiesWarnedCSV, g -> sortCSV(g.getTablePropertiesWarnedCSV()), csv); + assertValidProperty(Guardrails::setTablePropertiesIgnoredCSV, g -> sortCSV(g.getTablePropertiesIgnoredCSV()), csv); + assertValidProperty(Guardrails::setTablePropertiesDisallowedCSV, g -> sortCSV(g.getTablePropertiesDisallowedCSV()), csv); } private void assertInvalidProperty(Set<String> properties, Set<String> rejected) { String message = "Invalid value for %s: '%s' do not parse as valid table properties"; + assertInvalidProperty(Guardrails::setTablePropertiesWarned, properties, message, WARNED_PROPERTY_NAME, rejected); assertInvalidProperty(Guardrails::setTablePropertiesIgnored, properties, message, IGNORED_PROPERTY_NAME, rejected); 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"; + assertInvalidProperty(Guardrails::setTablePropertiesWarnedCSV, properties, message, WARNED_PROPERTY_NAME, rejected); + assertInvalidProperty(Guardrails::setTablePropertiesIgnoredCSV, properties, message, IGNORED_PROPERTY_NAME, rejected); + assertInvalidProperty(Guardrails::setTablePropertiesDisallowedCSV, properties, message, DISALLOWED_PROPERTY_NAME, rejected); + } + @Test public void testTableProperties() throws Throwable { @@ -112,6 +142,13 @@ public class GuardrailTablePropertiesTest extends GuardrailTester keyspace(), tableName.get()).one().getString("comment")); + // default_time_to_live is "warned". So it should warn, and getting the default ttl on the created table should + // not be empty, since we don't ignore it. + assertWarns(() -> tableName.set(createTableWithProperties("with default_time_to_live = 1000")), "[default_time_to_live]"); + assertEquals(1000, executeNet("SELECT default_time_to_live FROM system_schema.tables WHERE keyspace_name=? AND table_name=?", + keyspace(), + tableName.get()).one().getInt("default_time_to_live")); + // alter column is allowed assertValid(this::createTableWithProperties); assertValid("ALTER TABLE %s ADD v1 int"); @@ -130,8 +167,6 @@ public class GuardrailTablePropertiesTest extends GuardrailTester // alter mv properties except "gc_grace_seconds" is not allowed assertValid(() -> alterViewWithProperties("gc_grace_seconds = 1000")); - assertFails(() -> alterViewWithProperties("compaction = { 'class': 'SizeTieredCompactionStrategy' } AND default_time_to_live = 1"), - "[compaction, default_time_to_live]"); assertFails(() -> alterViewWithProperties("compaction = { 'class': 'SizeTieredCompactionStrategy' } AND crc_check_chance = 1"), "[compaction, crc_check_chance]"); } diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailTester.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailTester.java index 9e3a9f5..797476d 100644 --- a/test/unit/org/apache/cassandra/db/guardrails/GuardrailTester.java +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailTester.java @@ -22,11 +22,15 @@ import java.net.InetSocketAddress; import java.nio.ByteBuffer; import java.util.Collections; import java.util.List; +import java.util.TreeSet; import java.util.function.BiConsumer; import java.util.function.Consumer; +import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; +import com.google.common.collect.ImmutableSet; +import org.junit.Assert; import org.junit.Before; import org.junit.BeforeClass; @@ -37,13 +41,16 @@ import org.apache.cassandra.cql3.CQLStatement; import org.apache.cassandra.cql3.CQLTester; import org.apache.cassandra.cql3.QueryOptions; import org.apache.cassandra.cql3.QueryProcessor; +import org.apache.cassandra.db.ConsistencyLevel; import org.apache.cassandra.db.view.View; import org.apache.cassandra.exceptions.InvalidRequestException; import org.apache.cassandra.index.sasi.SASIIndex; import org.apache.cassandra.service.ClientState; import org.apache.cassandra.service.ClientWarn; import org.apache.cassandra.service.QueryState; +import org.apache.cassandra.transport.ProtocolVersion; import org.apache.cassandra.transport.messages.ResultMessage; +import org.apache.cassandra.utils.Clock; import org.assertj.core.api.Assertions; import static java.lang.String.format; @@ -105,6 +112,12 @@ public abstract class GuardrailTester extends CQLTester setter.accept(guardrails(), value); } + protected <T> void assertValidProperty(BiConsumer<Guardrails, T> setter, Function<Guardrails, T> getter, T value) + { + setter.accept(guardrails(), value); + assertEquals(value, getter.apply(guardrails())); + } + protected <T> void assertInvalidProperty(BiConsumer<Guardrails, T> setter, T value, String message, @@ -222,6 +235,23 @@ public abstract class GuardrailTester extends CQLTester assertFails(() -> execute(userClientState, query), messages); } + protected void assertThrows(CheckedFunction function, Class<? extends Throwable> exception, String message) + { + try + { + function.apply(); + fail("Expected to fail, but it did not"); + } + catch (Throwable e) + { + if (!exception.isAssignableFrom(e.getClass())) + Assert.fail(format("Expected to fail with %s but got %s", exception.getName(), e.getClass().getName())); + + assertTrue(format("Error message '%s' does not contain expected message '%s'", e.getMessage(), message), + e.getMessage().contains(message)); + } + } + private void assertWarnings(String... messages) { List<String> warnings = getWarnings(); @@ -283,14 +313,43 @@ public abstract class GuardrailTester extends CQLTester protected ResultMessage execute(ClientState state, String query, List<ByteBuffer> values) { + QueryOptions options = QueryOptions.forInternalCalls(values); + + return execute(state, query, options); + } + + protected ResultMessage execute(ClientState state, String query, ConsistencyLevel cl) + { + return execute(state, query, cl, null); + } + + protected ResultMessage execute(ClientState state, String query, ConsistencyLevel cl, ConsistencyLevel serialCl) + { + QueryOptions options = QueryOptions.create(cl, + Collections.emptyList(), + false, + 10, + null, + serialCl, + ProtocolVersion.CURRENT, + KEYSPACE); + + return execute(state, query, options); + } + + protected ResultMessage execute(ClientState state, String query, QueryOptions options) + { QueryState queryState = new QueryState(state); String formattedQuery = formatQuery(query); CQLStatement statement = QueryProcessor.parseStatement(formattedQuery, queryState.getClientState()); statement.validate(state); - QueryOptions options = QueryOptions.forInternalCalls(values); + return statement.execute(queryState, options, Clock.Global.nanoTime()); + } - return statement.executeLocally(queryState, options); + protected static String sortCSV(String csv) + { + return String.join(",", (new TreeSet<>(ImmutableSet.copyOf((csv.split(",")))))); } } diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailWriteConsistencyLevelsTest.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailWriteConsistencyLevelsTest.java new file mode 100644 index 0000000..3c38a26 --- /dev/null +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailWriteConsistencyLevelsTest.java @@ -0,0 +1,208 @@ +/* + * 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.Before; +import org.junit.Test; + +import org.apache.cassandra.db.ConsistencyLevel; + +import static java.lang.String.format; +import static org.apache.cassandra.db.ConsistencyLevel.ALL; +import static org.apache.cassandra.db.ConsistencyLevel.ANY; +import static org.apache.cassandra.db.ConsistencyLevel.LOCAL_ONE; +import static org.apache.cassandra.db.ConsistencyLevel.LOCAL_QUORUM; +import static org.apache.cassandra.db.ConsistencyLevel.LOCAL_SERIAL; +import static org.apache.cassandra.db.ConsistencyLevel.ONE; +import static org.apache.cassandra.db.ConsistencyLevel.QUORUM; +import static org.apache.cassandra.db.ConsistencyLevel.SERIAL; + +/** + * Tests the guardrail for write consistency levels, {@link Guardrails#writeConsistencyLevels}. + */ +public class GuardrailWriteConsistencyLevelsTest extends GuardrailConsistencyLevelsTester +{ + public GuardrailWriteConsistencyLevelsTest() + { + super("write_consistency_levels_warned", + "write_consistency_levels_disallowed", + Guardrails::getWriteConsistencyLevelsWarned, + Guardrails::getWriteConsistencyLevelsDisallowed, + Guardrails::getWriteConsistencyLevelsWarnedCSV, + Guardrails::getWriteConsistencyLevelsDisallowedCSV, + Guardrails::setWriteConsistencyLevelsWarned, + Guardrails::setWriteConsistencyLevelsDisallowed, + Guardrails::setWriteConsistencyLevelsWarnedCSV, + Guardrails::setWriteConsistencyLevelsDisallowedCSV); + } + + @Before + public void before() + { + super.before(); + createTable("CREATE TABLE IF NOT EXISTS %s (k INT, c INT, v TEXT, PRIMARY KEY(k, c))"); + } + + @Test + public void testInsert() throws Throwable + { + testQuery("INSERT INTO %s (k, c, v) VALUES (1, 2, 'val')"); + testLWTQuery("INSERT INTO %s (k, c, v) VALUES (1, 2, 'val') IF NOT EXISTS"); + } + + @Test + public void testUpdate() throws Throwable + { + testQuery("UPDATE %s SET v = 'val2' WHERE k = 1 AND c = 2"); + testLWTQuery("UPDATE %s SET v = 'val2' WHERE k = 1 AND c = 2 IF EXISTS"); + } + + @Test + public void testDelete() throws Throwable + { + testQuery("DELETE FROM %s WHERE k=1"); + testLWTQuery("DELETE FROM %s WHERE k=1 AND c=2 IF EXISTS"); + } + + @Test + public void testBatch() throws Throwable + { + testQuery("BEGIN BATCH INSERT INTO %s (k, c, v) VALUES (1, 2, 'val') APPLY BATCH"); + testQuery("BEGIN BATCH UPDATE %s SET v = 'val2' WHERE k = 1 AND c = 2 APPLY BATCH"); + testQuery("BEGIN BATCH DELETE FROM %s WHERE k=1 APPLY BATCH"); + + testLWTQuery("BEGIN BATCH INSERT INTO %s (k, c, v) VALUES (1, 2, 'val') IF NOT EXISTS APPLY BATCH"); + testLWTQuery("BEGIN BATCH UPDATE %s SET v = 'val2' WHERE k = 1 AND c = 2 IF EXISTS APPLY BATCH"); + testLWTQuery("BEGIN BATCH DELETE FROM %s WHERE k=1 AND c=2 IF EXISTS APPLY BATCH"); + } + + private void testQuery(String query) throws Throwable + { + testQuery(query, ONE); + testQuery(query, ALL); + testQuery(query, ANY); + testQuery(query, QUORUM); + testQuery(query, LOCAL_ONE); + testQuery(query, LOCAL_QUORUM); + } + + private void testQuery(String query, ConsistencyLevel cl) throws Throwable + { + warnConsistencyLevels(); + disableConsistencyLevels(); + assertValid(query, cl, null); + + warnConsistencyLevels(cl); + assertWarns(query, cl, null, cl); + + warnConsistencyLevels(); + disableConsistencyLevels(cl); + assertFails(query, cl, null, cl); + } + + private void testLWTQuery(String query) throws Throwable + { + testLWTQuery(query, ONE); + testLWTQuery(query, ALL); + testLWTQuery(query, QUORUM); + testLWTQuery(query, LOCAL_ONE); + testLWTQuery(query, LOCAL_QUORUM); + } + + private void testLWTQuery(String query, ConsistencyLevel cl) throws Throwable + { + disableConsistencyLevels(); + + warnConsistencyLevels(); + assertValid(query, cl, SERIAL); + assertValid(query, cl, LOCAL_SERIAL); + assertValid(query, cl, null); + + warnConsistencyLevels(cl); + assertWarns(query, cl, SERIAL, cl); + assertWarns(query, cl, LOCAL_SERIAL, cl); + assertWarns(query, cl, null, cl); + + warnConsistencyLevels(SERIAL); + assertWarns(query, cl, SERIAL, SERIAL); + assertValid(query, cl, LOCAL_SERIAL); + assertWarns(query, cl, null, SERIAL); + + warnConsistencyLevels(LOCAL_SERIAL); + assertValid(query, cl, SERIAL); + assertWarns(query, cl, LOCAL_SERIAL, LOCAL_SERIAL); + assertValid(query, cl, null); + + warnConsistencyLevels(SERIAL, LOCAL_SERIAL); + assertWarns(query, cl, SERIAL, SERIAL); + assertWarns(query, cl, LOCAL_SERIAL, LOCAL_SERIAL); + assertWarns(query, cl, null, SERIAL); + + warnConsistencyLevels(); + + disableConsistencyLevels(cl); + assertFails(query, cl, SERIAL, cl); + assertFails(query, cl, LOCAL_SERIAL, cl); + assertFails(query, cl, null, cl); + + disableConsistencyLevels(SERIAL); + assertFails(query, cl, SERIAL, SERIAL); + assertValid(query, cl, LOCAL_SERIAL); + assertFails(query, cl, null, SERIAL); + + disableConsistencyLevels(LOCAL_SERIAL); + assertValid(query, cl, SERIAL); + assertFails(query, cl, LOCAL_SERIAL, LOCAL_SERIAL); + assertValid(query, cl, null); + + disableConsistencyLevels(SERIAL, LOCAL_SERIAL); + assertFails(query, cl, SERIAL, SERIAL); + assertFails(query, cl, LOCAL_SERIAL, LOCAL_SERIAL); + assertFails(query, cl, null, SERIAL); + } + + private void assertValid(String query, ConsistencyLevel cl, ConsistencyLevel serialCl) throws Throwable + { + assertValid(() -> execute(userClientState, query, cl, serialCl)); + } + + private void assertWarns(String query, ConsistencyLevel cl, ConsistencyLevel serialCl, ConsistencyLevel warnedCl) throws Throwable + { + assertWarns(() -> execute(userClientState, query, cl, serialCl), + format("Provided values [%s] are not recommended for write consistency levels (warned values are: %s)", + warnedCl, guardrails().getWriteConsistencyLevelsWarned())); + + assertExcludedUsers(query, cl, serialCl); + } + + private void assertFails(String query, ConsistencyLevel cl, ConsistencyLevel serialCl, ConsistencyLevel rejectedCl) throws Throwable + { + assertFails(() -> execute(userClientState, query, cl, serialCl), + format("Provided values [%s] are not allowed for write consistency levels (disallowed values are: %s)", + rejectedCl, guardrails().getWriteConsistencyLevelsDisallowed())); + + assertExcludedUsers(query, cl, serialCl); + } + + private void assertExcludedUsers(String query, ConsistencyLevel cl, ConsistencyLevel serialCl) throws Throwable + { + assertValid(() -> execute(superClientState, query, cl, serialCl)); + assertValid(() -> execute(systemClientState, query, cl, serialCl)); + } +} diff --git a/test/unit/org/apache/cassandra/db/guardrails/GuardrailsTest.java b/test/unit/org/apache/cassandra/db/guardrails/GuardrailsTest.java index e7eaeb4..6cebccc 100644 --- a/test/unit/org/apache/cassandra/db/guardrails/GuardrailsTest.java +++ b/test/unit/org/apache/cassandra/db/guardrails/GuardrailsTest.java @@ -18,10 +18,12 @@ package org.apache.cassandra.db.guardrails; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashSet; +import java.util.List; import java.util.Set; import java.util.function.Consumer; @@ -155,10 +157,63 @@ public class GuardrailsTest extends GuardrailTester } @Test - public void testDisallowedValues() throws Throwable + public void testValuesWarned() throws Throwable + { + // Using a sorted set below to ensure the order in the warning message checked below is not random + Values<Integer> warned = new Values<>(state -> insertionOrderedSet(4, 6, 20), + state -> Collections.emptySet(), + state -> Collections.emptySet(), + "integer"); + + Consumer<Integer> action = i -> Assert.fail("The ignore action shouldn't have been triggered"); + assertValid(() -> warned.guard(set(3), action, userClientState)); + assertWarns(() -> warned.guard(set(4), action, userClientState), + "Provided values [4] are not recommended for integer (warned values are: [4, 6, 20])"); + assertWarns(() -> warned.guard(set(4, 6), action, null), + "Provided values [4, 6] are not recommended for integer (warned values are: [4, 6, 20])"); + assertWarns(() -> warned.guard(set(4, 5, 6, 7), action, null), + "Provided values [4, 6] are not recommended for integer (warned values are: [4, 6, 20])"); + } + + @Test + public void testValuesIgnored() throws Throwable + { + // Using a sorted set below to ensure the order in the error message checked below are not random + Values<Integer> ignored = new Values<>(state -> Collections.emptySet(), + state -> insertionOrderedSet(4, 6, 20), + state -> Collections.emptySet(), + "integer"); + + Set<Integer> triggeredOn = set(); + assertValid(() -> ignored.guard(set(3), triggeredOn::add, userClientState)); + assertEquals(set(), triggeredOn); + + assertWarns(() -> ignored.guard(set(4), triggeredOn::add, userClientState), + "Ignoring provided values [4] as they are not supported for integer (ignored values are: [4, 6, 20])"); + assertEquals(set(4), triggeredOn); + triggeredOn.clear(); + + assertWarns(() -> ignored.guard(set(4, 6), triggeredOn::add, null), + "Ignoring provided values [4, 6] as they are not supported for integer (ignored values are: [4, 6, 20])"); + assertEquals(set(4, 6), triggeredOn); + triggeredOn.clear(); + + assertWarns(() -> ignored.guard(set(4, 5, 6, 7), triggeredOn::add, null), + "Ignoring provided values [4, 6] as they are not supported for integer (ignored values are: [4, 6, 20])"); + assertEquals(set(4, 6), triggeredOn); + triggeredOn.clear(); + + assertThrows(() -> ignored.guard(set(4), userClientState), + AssertionError.class, + "There isn't an ignore action for integer, but value 4 is setup to be ignored"); + } + + @Test + public void testValuesDisallowed() throws Throwable { // Using a sorted set below to ensure the order in the error message checked below are not random Values<Integer> disallowed = new Values<>(state -> Collections.emptySet(), + state -> Collections.emptySet(), state -> insertionOrderedSet(4, 6, 20), "integer"); @@ -179,64 +234,39 @@ public class GuardrailsTest extends GuardrailTester } @Test - public void testDisallowedValuesUsers() throws Throwable + public void testValuesUsers() throws Throwable { - Values<Integer> disallowed = new Values<>(state -> Collections.emptySet(), - state -> Collections.singleton(2), + Values<Integer> disallowed = new Values<>(state -> Collections.singleton(2), + state -> Collections.singleton(3), + state -> Collections.singleton(4), "integer"); Consumer<Integer> action = i -> Assert.fail("The ignore action shouldn't have been triggered"); + assertValid(() -> disallowed.guard(set(1), action, null)); assertValid(() -> disallowed.guard(set(1), action, userClientState)); assertValid(() -> disallowed.guard(set(1), action, systemClientState)); assertValid(() -> disallowed.guard(set(1), action, superClientState)); - String message = "Provided values [2] are not allowed for integer (disallowed values are: [2])"; - assertFails(() -> disallowed.guard(set(2), action, null), message); - assertFails(() -> disallowed.guard(set(2), action, userClientState), message); + String message = "Provided values [2] are not recommended for integer (warned values are: [2])"; + assertWarns(() -> disallowed.guard(set(2), action, null), message); + assertWarns(() -> disallowed.guard(set(2), action, userClientState), message); assertValid(() -> disallowed.guard(set(2), action, systemClientState)); assertValid(() -> disallowed.guard(set(2), action, superClientState)); - Set<Integer> allowedValues = set(1); - assertValid(() -> disallowed.guard(allowedValues, action, null)); - assertValid(() -> disallowed.guard(allowedValues, action, userClientState)); - assertValid(() -> disallowed.guard(allowedValues, action, systemClientState)); - assertValid(() -> disallowed.guard(allowedValues, action, superClientState)); - - Set<Integer> disallowedValues = set(2); - message = "Provided values [2] are not allowed for integer (disallowed values are: [2])"; - assertFails(() -> disallowed.guard(disallowedValues, action, null), message); - assertFails(() -> disallowed.guard(disallowedValues, action, userClientState), message); - assertValid(() -> disallowed.guard(disallowedValues, action, systemClientState)); - assertValid(() -> disallowed.guard(disallowedValues, action, superClientState)); - } - - @Test - public void testIgnoredValues() throws Throwable - { - // Using a sorted set below to ensure the order in the error message checked below are not random - Values<Integer> ignored = new Values<>(state -> insertionOrderedSet(4, 6, 20), - state -> Collections.emptySet(), - "integer"); - - Set<Integer> triggeredOn = set(); - assertValid(() -> ignored.guard(set(3), triggeredOn::add, userClientState)); - assertEquals(set(), triggeredOn); - - assertWarns(() -> ignored.guard(set(4), triggeredOn::add, userClientState), - "Ignoring provided values [4] as they are not supported for integer (ignored values are: [4, 6, 20])"); - assertEquals(set(4), triggeredOn); - triggeredOn.clear(); - - assertWarns(() -> ignored.guard(set(4, 6), triggeredOn::add, null), - "Ignoring provided values [4, 6] as they are not supported for integer (ignored values are: [4, 6, 20])"); - assertEquals(set(4, 6), triggeredOn); - triggeredOn.clear(); - - assertWarns(() -> ignored.guard(set(4, 5, 6, 7), triggeredOn::add, null), - "Ignoring provided values [4, 6] as they are not supported for integer (ignored values are: [4, 6, 20])"); - assertEquals(set(4, 6), triggeredOn); - triggeredOn.clear(); + message = "Ignoring provided values [3] as they are not supported for integer (ignored values are: [3])"; + List<Integer> triggeredOn = new ArrayList<>(); + assertWarns(() -> disallowed.guard(set(3), triggeredOn::add, null), message); + assertWarns(() -> disallowed.guard(set(3), triggeredOn::add, userClientState), message); + assertValid(() -> disallowed.guard(set(3), triggeredOn::add, systemClientState)); + assertValid(() -> disallowed.guard(set(3), triggeredOn::add, superClientState)); + Assert.assertEquals(list(3, 3), triggeredOn); + + message = "Provided values [4] are not allowed for integer (disallowed values are: [4])"; + assertFails(() -> disallowed.guard(set(4), action, null), message); + assertFails(() -> disallowed.guard(set(4), action, userClientState), message); + assertValid(() -> disallowed.guard(set(4), action, systemClientState)); + assertValid(() -> disallowed.guard(set(4), action, superClientState)); } private static Set<Integer> set(Integer value) --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org