guardrails: restrict replication strategy (RS)

Replacing `restrict_replication_simplestrategy` config option with
2 config options: `replication_strategy_{warn,fail}_list`, which
allow us to impose soft limits (issue a warning) and hard limits (not
execute CQL) on replication strategy when creating/altering a keyspace.
The reason to rather replace than extend `restrict_replication_simplestrategy` config
option is that it was not used and we wanted to generalize it.
Only soft guardrail is enabled by default and it is set to SimpleStrategy,
which means that we'll generate a CQL warning whenever replication strategy
is set to SimpleStrategy. For new cloud deployments we'll move
SimpleStrategy from warn to the fail list.
Guardrails violations will be tracked by metrics.

Resolves #5224
Refs #8892 (the replication strategy part, not the RF part)

Closes scylladb/scylladb#15399

conf/scylla.yaml

@@ -623,3 +623,15 @@ strict_is_not_null_in_views: true
 # minimum_replication_factor_warn_threshold:  3
 # maximum_replication_factor_warn_threshold: -1
 # maximum_replication_factor_fail_threshold: -1
+
+# Guardrails to warn about or disallow creating a keyspace with specific replication strategy.
+# Each of these 2 settings is a list storing replication strategies considered harmful.
+# The replication strategies to choose from are:
+# 1) SimpleStrategy,
+# 2) NetworkTopologyStrategy,
+# 3) LocalStrategy,
+# 4) EverywhereStrategy
+#
+# replication_strategy_warn_list:
+#  - SimpleStrategy
+# replication_strategy_fail_list:

cql3/query_processor.cc

@@ -477,6 +477,18 @@ query_processor::query_processor(service::storage_proxy& proxy, data_dictionary:
                             _cql_stats.maximum_replication_factor_fail_violations,
                             sm::description("Counts the number of maximum_replication_factor_fail_threshold guardrail violations, "
                                             "i.e. attempts to create a keyspace with RF on one of the DCs above the set guardrail.")),
+
+                    sm::make_counter(
+                            "replication_strategy_warn_list_violations",
+                            _cql_stats.replication_strategy_warn_list_violations,
+                            sm::description("Counts the number of replication_strategy_warn_list guardrail violations, "
+                                            "i.e. attempts to set a discouraged replication strategy in a keyspace via CREATE/ALTER KEYSPACE.")),
+
+                    sm::make_counter(
+                            "replication_strategy_fail_list_violations",
+                            _cql_stats.replication_strategy_fail_list_violations,
+                            sm::description("Counts the number of replication_strategy_fail_list guardrail violations, "
+                                            "i.e. attempts to set a forbidden replication strategy in a keyspace via CREATE/ALTER KEYSPACE.")),
             });
 
     _mnotifier.register_listener(_migration_subscriber.get());

cql3/statements/alter_keyspace_statement.cc

@@ -103,11 +103,11 @@ static logging::logger mylogger("alter_keyspace");
 
 future<::shared_ptr<cql_transport::messages::result_message>>
 cql3::statements::alter_keyspace_statement::execute(query_processor& qp, service::query_state& state, const query_options& options, std::optional<service::group0_guard> guard) const {
-    std::optional<sstring> warning = check_restricted_replication_strategy(qp, keyspace(), *_attrs, qp.get_cql_stats());
-    return schema_altering_statement::execute(qp, state, options, std::move(guard)).then([warning = std::move(warning)] (::shared_ptr<messages::result_message> msg) {
-        if (warning) {
-            msg->add_warning(*warning);
-            mylogger.warn("{}", *warning);
+    std::vector<sstring> warnings = check_against_restricted_replication_strategies(qp, keyspace(), *_attrs, qp.get_cql_stats());
+    return schema_altering_statement::execute(qp, state, options, std::move(guard)).then([warnings = std::move(warnings)] (::shared_ptr<messages::result_message> msg) {
+        for (const auto& warning : warnings) {
+            msg->add_warning(warning);
+            mylogger.warn("{}", warning);
         }
         return msg;
     });

cql3/statements/create_keyspace_statement.cc

@@ -134,50 +134,58 @@ future<> cql3::statements::create_keyspace_statement::grant_permissions_to_creat
 // Check for replication strategy choices which are restricted by the
 // configuration. This check can throw a configuration_exception immediately
 // if the strategy is forbidden by the configuration, or return a warning
-// string if the restriction was set to "warn".
+// string if the restriction was set to warning level.
 // This function is only supposed to check for replication strategies
 // restricted by the configuration. Checks for other types of strategy
 // errors (such as unknown replication strategy name or unknown options
 // to a known replication strategy) are done elsewhere.
-std::optional<sstring> check_restricted_replication_strategy(
+std::vector<sstring> check_against_restricted_replication_strategies(
     query_processor& qp,
     const sstring& keyspace,
     const ks_prop_defs& attrs,
     cql_stats& stats)
 {
     if (!attrs.get_replication_strategy_class()) {
-        return std::nullopt;
+        return {};
     }
-    sstring replication_strategy = locator::abstract_replication_strategy::to_qualified_class_name(
-        *attrs.get_replication_strategy_class());
-    auto& topology = qp.proxy().get_token_metadata_ptr()->get_topology();
-    // SimpleStrategy is not recommended in any setup which already has - or
-    // may have in the future - multiple racks or DCs. So depending on how
-    // protective we are configured, let's prevent it or allow with a warning:
-    if (replication_strategy == "org.apache.cassandra.locator.SimpleStrategy") {
-        switch(qp.db().get_config().restrict_replication_simplestrategy()) {
-        case db::tri_mode_restriction_t::mode::TRUE:
-            throw exceptions::configuration_exception(
-                "SimpleStrategy replication class is not recommended, and "
-                "forbidden by the current configuration. Please use "
-                "NetworkToplogyStrategy instead. You may also override this "
-                "restriction with the restrict_replication_simplestrategy=false "
-                "configuration option.");
-        case db::tri_mode_restriction_t::mode::WARN:
-            return format("SimpleStrategy replication class is not "
-                "recommended, but was used for keyspace {}. The "
-                "restrict_replication_simplestrategy configuration option "
-                "can be changed to silence this warning or make it into an error.",
-                keyspace);
-        case db::tri_mode_restriction_t::mode::FALSE:
+
+    std::vector<sstring> warnings;
+    auto replication_strategy = locator::abstract_replication_strategy::create_replication_strategy(
+            locator::abstract_replication_strategy::to_qualified_class_name(
+                    *attrs.get_replication_strategy_class()), {})->get_type();
+    auto rs_warn_list = qp.db().get_config().replication_strategy_warn_list();
+    auto rs_fail_list = qp.db().get_config().replication_strategy_fail_list();
+
+    if (replication_strategy == locator::replication_strategy_type::simple) {
+        if (auto simple_strategy_restriction = qp.db().get_config().restrict_replication_simplestrategy();
+                simple_strategy_restriction == db::tri_mode_restriction_t::mode::TRUE) {
+            rs_fail_list.emplace_back(locator::replication_strategy_type::simple);
+        } else if (simple_strategy_restriction == db::tri_mode_restriction_t::mode::WARN) {
+            rs_warn_list.emplace_back(locator::replication_strategy_type::simple);
+        } else if (auto &topology = qp.proxy().get_token_metadata_ptr()->get_topology();
+                topology.get_datacenter_endpoints().size() > 1) {
             // Scylla was configured to allow SimpleStrategy, but let's warn
             // if it's used on a cluster which *already* has multiple DCs:
-            if (topology.get_datacenter_endpoints().size() > 1) {
-                return "Using SimpleStrategy in a multi-datacenter environment is not recommended.";
-            }
-            break;
+            warnings.emplace_back("Using SimpleStrategy in a multi-datacenter environment is not recommended.");
         }
     }
+
+    if (auto present_on_fail_list = std::find(rs_fail_list.begin(), rs_fail_list.end(), replication_strategy); present_on_fail_list != rs_fail_list.end()) {
+        ++stats.replication_strategy_fail_list_violations;
+        throw exceptions::configuration_exception(format(
+                "{} replication class is not recommended, and forbidden by the current configuration, "
+                "but was used for keyspace {}. You may override this restriction by modifying "
+                "replication_strategy_fail_list configuration option to not list {}.",
+                *attrs.get_replication_strategy_class(), keyspace, *attrs.get_replication_strategy_class()));
+    }
+    if (auto present_on_warn_list = std::find(rs_warn_list.begin(), rs_warn_list.end(), replication_strategy); present_on_warn_list != rs_warn_list.end()) {
+        ++stats.replication_strategy_warn_list_violations;
+        warnings.push_back(format("{} replication class is not recommended, but was used for keyspace {}. "
+                           "You may suppress this warning by delisting {} from replication_strategy_warn_list configuration option, "
+                           "or make it into an error by listing this replication strategy on replication_strategy_fail_list.",
+                           *attrs.get_replication_strategy_class(), keyspace, *attrs.get_replication_strategy_class()));
+    }
+
     // The {minimum,maximum}_replication_factor_{warn,fail}_threshold configuration option can be used to forbid
     // a smaller/greater replication factor. We assume that all numeric replication
     // options are replication factors - this is true for SimpleStrategy and
@@ -215,33 +223,33 @@ std::optional<sstring> check_restricted_replication_strategy(
                          min_warn >= 0 && rf < min_warn)
                 {
                     ++stats.minimum_replication_factor_warn_violations;
-                    return format("Using Replication Factor {}={} lower than the "
-                                  "minimum_replication_factor_warn_threshold={} is not recommended.",
-                                  opt.first, rf, qp.proxy().data_dictionary().get_config().minimum_replication_factor_warn_threshold());
+                    warnings.push_back(format("Using Replication Factor {}={} lower than the "
+                                              "minimum_replication_factor_warn_threshold={} is not recommended.", opt.first, rf,
+                                              qp.proxy().data_dictionary().get_config().minimum_replication_factor_warn_threshold()));
                 }
                 else if (auto max_warn = qp.proxy().data_dictionary().get_config().maximum_replication_factor_warn_threshold();
                         max_warn >= 0 && rf > max_warn)
                 {
                     ++stats.maximum_replication_factor_warn_violations;
-                    return format("Using Replication Factor {}={} greater than the "
-                                  "maximum_replication_factor_warn_threshold={} is not recommended.",
-                                  opt.first, rf, qp.proxy().data_dictionary().get_config().maximum_replication_factor_warn_threshold());
+                    warnings.push_back(format("Using Replication Factor {}={} greater than the "
+                                              "maximum_replication_factor_warn_threshold={} is not recommended.", opt.first, rf,
+                                              qp.proxy().data_dictionary().get_config().maximum_replication_factor_warn_threshold()));
                 }
             }
         } catch (std::invalid_argument&) {
         } catch (std::out_of_range& ) {
         }
     }
-    return std::nullopt;
+    return warnings;
 }
 
 future<::shared_ptr<messages::result_message>>
 create_keyspace_statement::execute(query_processor& qp, service::query_state& state, const query_options& options, std::optional<service::group0_guard> guard) const {
-    std::optional<sstring> warning = check_restricted_replication_strategy(qp, keyspace(), *_attrs, qp.get_cql_stats());
-    return schema_altering_statement::execute(qp, state, options, std::move(guard)).then([warning = std::move(warning)] (::shared_ptr<messages::result_message> msg) {
-        if (warning) {
-            msg->add_warning(*warning);
-            mylogger.warn("{}", *warning);
+    std::vector<sstring> warnings = check_against_restricted_replication_strategies(qp, keyspace(), *_attrs, qp.get_cql_stats());
+        return schema_altering_statement::execute(qp, state, options, std::move(guard)).then([warnings = std::move(warnings)] (::shared_ptr<messages::result_message> msg) {
+        for (const auto& warning : warnings) {
+            msg->add_warning(warning);
+            mylogger.warn("{}", warning);
         }
         return msg;
     });

cql3/statements/create_keyspace_statement.hh

@@ -76,7 +76,7 @@ public:
     lw_shared_ptr<data_dictionary::keyspace_metadata> get_keyspace_metadata(const locator::token_metadata& tm);
 };
 
-std::optional<sstring> check_restricted_replication_strategy(
+std::vector<sstring> check_against_restricted_replication_strategies(
     query_processor& qp,
     const sstring& keyspace,
     const ks_prop_defs& attrs,

cql3/stats.hh

@@ -85,6 +85,9 @@ struct cql_stats {
     uint64_t maximum_replication_factor_warn_violations = 0;
     uint64_t maximum_replication_factor_fail_violations = 0;
 
+    uint64_t replication_strategy_warn_list_violations = 0;
+    uint64_t replication_strategy_fail_list_violations = 0;
+
 private:
     uint64_t _unpaged_select_queries[(size_t)ks_selector::SIZE] = {0ul};
     uint64_t _query_cnt[(size_t)source_selector::SIZE]

db/config.cc

@@ -146,6 +146,9 @@ const config_type config_type_for<db::seed_provider_type> = config_type("seed pr
 template <>
 const config_type config_type_for<std::vector<enum_option<db::experimental_features_t>>> = config_type(
         "experimental features", printable_vector_to_json<enum_option<db::experimental_features_t>>);
+template <>
+const config_type config_type_for<std::vector<enum_option<db::replication_strategy_restriction_t>>> = config_type(
+        "replication strategy list", printable_vector_to_json<enum_option<db::replication_strategy_restriction_t>>);
 
 template <>
 const config_type config_type_for<enum_option<db::tri_mode_restriction_t>> = config_type(
@@ -231,6 +234,23 @@ public:
     }
 };
 
+template <>
+class convert<enum_option<db::replication_strategy_restriction_t>> {
+public:
+    static bool decode(const Node& node, enum_option<db::replication_strategy_restriction_t>& rhs) {
+        std::string name;
+        if (!convert<std::string>::decode(node, name)) {
+            return false;
+        }
+        try {
+            std::istringstream(name) >> rhs;
+        } catch (boost::program_options::invalid_option_value&) {
+            return false;
+        }
+        return true;
+    }
+};
+
 template <>
 class convert<enum_option<db::tri_mode_restriction_t>> {
 public:
@@ -1057,7 +1077,7 @@ db::config::config(std::shared_ptr<db::extensions> exts)
             "In debug mode, report log-structured allocator sanitizer violations with a backtrace. Slow.")
     , flush_schema_tables_after_modification(this, "flush_schema_tables_after_modification", liveness::LiveUpdate, value_status::Used, true,
         "Flush tables in the system_schema keyspace after schema modification. This is required for crash recovery, but slows down tests and can be disabled for them")
-    , restrict_replication_simplestrategy(this, "restrict_replication_simplestrategy", liveness::LiveUpdate, value_status::Used, db::tri_mode_restriction_t::mode::FALSE, "Controls whether to disable SimpleStrategy replication. Can be true, false, or warn.")
+    , restrict_replication_simplestrategy(this, "restrict_replication_simplestrategy", liveness::LiveUpdate, value_status::Unused, db::tri_mode_restriction_t::mode::FALSE, "Controls whether to disable SimpleStrategy replication. Can be true, false, or warn.")
     , restrict_dtcs(this, "restrict_dtcs", liveness::LiveUpdate, value_status::Unused, db::tri_mode_restriction_t::mode::TRUE, "Controls whether to prevent setting DateTieredCompactionStrategy. Can be true, false, or warn.")
     , restrict_twcs_without_default_ttl(this, "restrict_twcs_without_default_ttl", liveness::LiveUpdate, value_status::Used, db::tri_mode_restriction_t::mode::WARN, "Controls whether to prevent creating TimeWindowCompactionStrategy tables without a default TTL. Can be true, false, or warn.")
     , restrict_future_timestamp(this, "restrict_future_timestamp",liveness::LiveUpdate, value_status::Used, true, "Controls whether to detect and forbid unreasonable USING TIMESTAMP, more than 3 days into the future.")
@@ -1093,6 +1113,8 @@ db::config::config(std::shared_ptr<db::extensions> exts)
     , minimum_replication_factor_warn_threshold(this, "minimum_replication_factor_warn_threshold", liveness::LiveUpdate, value_status::Used,  3, "")
     , maximum_replication_factor_warn_threshold(this, "maximum_replication_factor_warn_threshold", liveness::LiveUpdate, value_status::Used, -1, "")
     , maximum_replication_factor_fail_threshold(this, "maximum_replication_factor_fail_threshold", liveness::LiveUpdate, value_status::Used, -1, "")
+    , replication_strategy_warn_list(this, "replication_strategy_warn_list", liveness::LiveUpdate, value_status::Used, {locator::replication_strategy_type::simple}, "Controls which replication strategies to warn about when creating/altering a keyspace. Doesn't affect the pre-existing keyspaces.")
+    , replication_strategy_fail_list(this, "replication_strategy_fail_list", liveness::LiveUpdate, value_status::Used, {}, "Controls which replication strategies are disallowed to be used when creating/altering a keyspace. Doesn't affect the pre-existing keyspaces.")
     , error_injections_at_startup(this, "error_injections_at_startup", error_injection_value_status, {}, "List of error injections that should be enabled on startup.")
     , default_log_level(this, "default_log_level", value_status::Used)
     , logger_log_level(this, "logger_log_level", value_status::Used)
@@ -1269,6 +1291,13 @@ std::map<sstring, db::experimental_features_t::feature> db::experimental_feature
     };
 }
 
+std::unordered_map<sstring, locator::replication_strategy_type> db::replication_strategy_restriction_t::map() {
+    return {{"SimpleStrategy", locator::replication_strategy_type::simple},
+            {"LocalStrategy", locator::replication_strategy_type::local},
+            {"NetworkTopologyStrategy", locator::replication_strategy_type::network_topology},
+            {"EverywhereStrategy", locator::replication_strategy_type::everywhere_topology}};
+}
+
 std::vector<enum_option<db::experimental_features_t>> db::experimental_features_t::all() {
     std::vector<enum_option<db::experimental_features_t>> ret;
     for (const auto& f : db::experimental_features_t::map()) {

db/config.hh

@@ -17,6 +17,7 @@
 #include <seastar/util/program-options.hh>
 #include <seastar/util/log.hh>
 
+#include "locator/abstract_replication_strategy.hh"
 #include "seastarx.hh"
 #include "utils/config_file.hh"
 #include "utils/enum_option.hh"
@@ -119,6 +120,10 @@ struct tri_mode_restriction_t {
 };
 using tri_mode_restriction = enum_option<tri_mode_restriction_t>;
 
+struct replication_strategy_restriction_t {
+    static std::unordered_map<sstring, locator::replication_strategy_type> map(); // for enum_option<>
+};
+
 constexpr unsigned default_murmur3_partitioner_ignore_msb_bits = 12;
 
 class config final : public utils::config_file {
@@ -455,6 +460,8 @@ public:
     named_value<int> minimum_replication_factor_warn_threshold;
     named_value<int> maximum_replication_factor_warn_threshold;
     named_value<int> maximum_replication_factor_fail_threshold;
+    named_value<std::vector<enum_option<replication_strategy_restriction_t>>> replication_strategy_warn_list;
+    named_value<std::vector<enum_option<replication_strategy_restriction_t>>> replication_strategy_fail_list;
 
     seastar::logging_settings logging_settings(const log_cli::options&) const;
 

test/cql-pytest/test_guardrail_replication_strategy.py

@@ -0,0 +1,131 @@
+import pytest
+from contextlib import ExitStack
+from util import unique_name, config_value_context, new_test_keyspace
+from cassandra.protocol import ConfigurationException
+
+
+def create_ks_and_assert_warnings_and_errors(cql, ks_opts, warnings_count=0, fails_count=0):
+    if fails_count:
+        with pytest.raises(ConfigurationException):
+            with new_test_keyspace(cql, ks_opts):
+                pass
+    else:
+        keyspace = unique_name()
+        response_future = cql.execute_async("CREATE KEYSPACE " + keyspace + " " + ks_opts)
+        response_future.result()
+        cql.execute("DROP KEYSPACE " + keyspace)
+        if warnings_count:
+            assert response_future.warnings is not None and len(response_future.warnings) == warnings_count
+        else:
+            assert response_future.warnings is None
+
+
+def test_given_default_config_when_creating_ks_should_only_produce_warning_for_simple_strategy(cql, this_dc):
+    ks_opts = " WITH REPLICATION = { 'class' : 'SimpleStrategy', 'replication_factor' : 3 }"
+    create_ks_and_assert_warnings_and_errors(cql, ks_opts, warnings_count=1, fails_count=0)
+
+    for key, value in {'NetworkTopologyStrategy': this_dc, 'EverywhereStrategy': 'replication_factor',
+                       'LocalStrategy': 'replication_factor'}.items():
+        create_ks_and_assert_warnings_and_errors(cql, f" WITH REPLICATION = {{ 'class' : '{key}', '{value}' : 3 }}",
+                                                 warnings_count=0, fails_count=0)
+
+
+def test_given_cleared_guardrails_when_creating_ks_should_not_get_warning_nor_error(cql, this_dc):
+    with ExitStack() as config_modifications:
+        config_modifications.enter_context(config_value_context(cql, 'replication_strategy_warn_list', ''))
+        config_modifications.enter_context(config_value_context(cql, 'replication_strategy_fail_list', ''))
+
+        for key, value in {'SimpleStrategy': 'replication_factor', 'NetworkTopologyStrategy': this_dc,
+                           'EverywhereStrategy': 'replication_factor', 'LocalStrategy': 'replication_factor'}.items():
+            create_ks_and_assert_warnings_and_errors(cql, f" WITH REPLICATION = {{ 'class' : '{key}', '{value}' : 3 }}",
+                                                     warnings_count=0, fails_count=0)
+
+
+def test_given_non_empty_warn_list_when_creating_ks_should_only_warn_when_listed_strategy_used(cql, this_dc):
+    with ExitStack() as config_modifications:
+        config_modifications.enter_context(config_value_context(cql, 'replication_strategy_warn_list',
+                                                                'SimpleStrategy, LocalStrategy, '
+                                                                'NetworkTopologyStrategy, EverywhereStrategy'))
+        for key, value in {'SimpleStrategy': 'replication_factor', 'NetworkTopologyStrategy': this_dc,
+                           'EverywhereStrategy': 'replication_factor', 'LocalStrategy': 'replication_factor'}.items():
+            create_ks_and_assert_warnings_and_errors(cql, f" WITH REPLICATION = {{ 'class' : '{key}', '{value}' : 3 }}",
+                                                     warnings_count=1, fails_count=0)
+
+
+def test_given_non_empty_warn_and_fail_lists_when_creating_ks_should_fail_query_when_listed_strategy_used(cql, this_dc):
+    with ExitStack() as config_modifications:
+        config_modifications.enter_context(
+            config_value_context(cql, 'replication_strategy_warn_list', 'SimpleStrategy, EverywhereStrategy'))
+        config_modifications.enter_context(config_value_context(cql, 'replication_strategy_fail_list',
+                                                                'SimpleStrategy, LocalStrategy, '
+                                                                'NetworkTopologyStrategy, EverywhereStrategy'))
+        for key, value in {'SimpleStrategy': 'replication_factor', 'NetworkTopologyStrategy': this_dc,
+                           'EverywhereStrategy': 'replication_factor', 'LocalStrategy': 'replication_factor'}.items():
+            # note: even though warn list is not empty, no warnings should be generated, because failures come first -
+            #  we don't want to issue a warning and also fail the query at the same time
+            create_ks_and_assert_warnings_and_errors(cql, f" WITH REPLICATION = {{ 'class' : '{key}', '{value}' : 3 }}",
+                                                     warnings_count=0, fails_count=1)
+
+
+def test_given_already_existing_ks_when_altering_ks_should_validate_against_discouraged_strategies(cql, this_dc):
+    with ExitStack() as config_modifications:
+        # place 1 strategy on warn list, 1 strategy on fail list and leave remaining strategies unspecified,
+        # i.e. let them be allowed
+        config_modifications.enter_context(
+            config_value_context(cql, 'replication_strategy_warn_list', 'SimpleStrategy'))
+        config_modifications.enter_context(
+            config_value_context(cql, 'replication_strategy_fail_list', 'EverywhereStrategy'))
+
+        # create a ks with "allowed" strategy
+        with new_test_keyspace(cql, " WITH REPLICATION = { 'class' : 'NetworkTopologyStrategy', '" + this_dc + "' : 3 }") as keyspace:
+            # alter this ks to use other strategy that is NOT present on any list
+            response_future = cql.execute_async(
+                "ALTER KEYSPACE " + keyspace + " WITH REPLICATION = { 'class' : 'LocalStrategy', 'replication_factor' : 3 }")
+            response_future.result()
+            assert response_future.warnings is None
+
+            # alter this ks to use strategy that is present on the warn list
+            response_future = cql.execute_async(
+                "ALTER KEYSPACE " + keyspace + " WITH REPLICATION = { 'class' : 'SimpleStrategy', 'replication_factor' : 3 }")
+            response_future.result()
+            assert response_future.warnings is not None and len(response_future.warnings) == 1
+
+            # alter this ks to use strategy that is present on the fail list
+            with pytest.raises(ConfigurationException):
+                cql.execute_async(
+                    "ALTER KEYSPACE " + keyspace + " WITH REPLICATION = { 'class' : 'EverywhereStrategy', 'replication_factor' : 3 }").result()
+
+
+def test_given_rf_and_strategy_guardrails_when_creating_ks_should_print_2_warnings_if_both_violated(cql):
+    with ExitStack() as config_modifications:
+        config_modifications.enter_context(config_value_context(cql, 'replication_strategy_warn_list', 'SimpleStrategy'))
+        config_modifications.enter_context(config_value_context(cql, 'minimum_replication_factor_warn_threshold', '3'))
+        create_ks_and_assert_warnings_and_errors(cql,
+                                                 f" WITH REPLICATION = {{ 'class' : 'SimpleStrategy', 'replication_factor' : 1 }}",
+                                                 warnings_count=2, fails_count=0)
+
+
+def test_given_rf_and_strategy_guardrails_when_violating_fail_rf_limit_and_warn_strategy_limit_should_fail_the_query_without_warning(cql):
+    with ExitStack() as config_modifications:
+        config_modifications.enter_context(config_value_context(cql, 'replication_strategy_warn_list', 'SimpleStrategy'))
+        config_modifications.enter_context(config_value_context(cql, 'minimum_replication_factor_fail_threshold', '3'))
+        create_ks_and_assert_warnings_and_errors(cql, f" WITH REPLICATION = {{ 'class' : 'SimpleStrategy', 'replication_factor' : 1 }}",
+                                                 warnings_count=0, fails_count=1)
+
+
+def test_given_rf_and_strategy_guardrails_when_violating_fail_strategy_limit_should_fail_the_query(cql):
+    with ExitStack() as config_modifications:
+        config_modifications.enter_context(config_value_context(cql, 'replication_strategy_fail_list', 'SimpleStrategy'))
+        config_modifications.enter_context(config_value_context(cql, 'minimum_replication_factor_fail_threshold', '3'))
+        create_ks_and_assert_warnings_and_errors(cql, f" WITH REPLICATION = {{ 'class' : 'SimpleStrategy', 'replication_factor' : 1 }}",
+                                                 warnings_count=0, fails_count=1)
+
+
+def test_given_restrict_replication_simplestrategy_when_it_is_set_should_emulate_old_behavior(cql):
+    with ExitStack() as config_modifications:
+        config_modifications.enter_context(config_value_context(cql, 'restrict_replication_simplestrategy', 'true'))
+        create_ks_and_assert_warnings_and_errors(cql, f" WITH REPLICATION = {{ 'class' : 'SimpleStrategy', 'replication_factor' : 3 }}",
+                                                 warnings_count=0, fails_count=1)
+        config_modifications.enter_context(config_value_context(cql, 'restrict_replication_simplestrategy', 'warn'))
+        create_ks_and_assert_warnings_and_errors(cql, f" WITH REPLICATION = {{ 'class' : 'SimpleStrategy', 'replication_factor' : 3 }}",
+                                                 warnings_count=1, fails_count=0)

test/cql-pytest/test_virtual_tables.py

@@ -78,10 +78,10 @@ def test_system_config_read(scylla_only, cql):
     assert obj[0] and obj[0].isascii() and obj[0].isprintable()
     assert not obj[0].isnumeric()  # issue #11003
     # Check formatting of tri_mode_restriction like
-    # restrict_replication_simplestrategy. These need to be one of
+    # restrict_dtcs. These need to be one of
     # allowed string values 0, 1, true, false or warn - but in particular
     # non-empty and printable ASCII, not garbage.
-    assert 'restrict_replication_simplestrategy' in values
-    obj = json.loads(values['restrict_replication_simplestrategy'])
+    assert 'restrict_dtcs' in values
+    obj = json.loads(values['restrict_dtcs'])
     assert isinstance(obj, str)
     assert obj and obj.isascii() and obj.isprintable()

test/cql-pytest/util.py

@@ -6,6 +6,7 @@
 # Various utility functions which are useful for multiple tests.
 # Note that fixtures aren't here - they are in conftest.py.
 
+from ast import literal_eval
 import string
 import random
 import time
@@ -294,7 +295,10 @@ class config_value_context:
         self._original_value = None
 
     def __enter__(self):
-        self._original_value = self._cql.execute(f"SELECT value FROM system.config WHERE name='{self._key}'").one().value
+        self._original_value = literal_eval(
+            self._cql.execute(f"SELECT value FROM system.config WHERE name='{self._key}'").one().value)
+        if isinstance(self._original_value, list):
+            self._original_value = "".join(c for c in self._original_value if c.isalnum())
         self._cql.execute(f"UPDATE system.config SET value='{self._value}' WHERE name='{self._key}'")
 
     def __exit__(self, exc_type, exc_value, exc_traceback):