bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Merge 'ks_prop_defs: disallow empty replication factor string in NTS' from Jan Ciołek

Browse Source 
A CREATE KEYSPACE query which specifies an empty string ('') as the replication factor value is currently allowed:
```cql
CREATE KEYSPACE bad_ks WITH replication = {'class': 'NetworkTopologyStrategy', 'replication_factor': ''};
```

This is wrong, it's invalid to have an empty replication factor string.
It creates a keyspace without any replication, so the tables inside of it aren't writable.

Trying to create a `SimpleStrategy` keyspace with such replication factor throws an error, `NetworkTopolgyStrategy` should do the same.

The problem was in `prepare_options`, it treated an empty replication factor string as no replication factor.
Changing it to `std::optional` fixes the problem,
Now `std::nullopt` means no replication factor, and `make_optional("")` means that there is a replication factor, but it's described by an empty string.

Fixes: https://github.com/scylladb/scylladb/issues/13986

Closes #13988

* github.com:scylladb/scylladb:
  test/network_topology_strategy_test: Test NTS with replication_factor option in test_invalid_dcs
  ks_prop_defs: disallow empty replication factor string in NTS
This commit is contained in:
Nadav Har'El
2023-05-24 21:39:31 +03:00
parent d2f5a44e3b 07e7724468
commit 7cdee303cf
2 changed files with 7 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
cql3/statements/ks_prop_defs.cc
View File

@@ -34,7 +34,7 @@ static std::map<sstring, sstring> prepare_options(
// but the other strategy used the 'replication_factor' option, it will also be expanded.
// See issue CASSANDRA-14303.
sstring rf;
std::optional<sstring> rf;
auto it = options.find(ks_prop_defs::REPLICATION_FACTOR_KEY);
if (it != options.end()) {
// Expand: the user explicitly provided a 'replication_factor'.
@@ -49,10 +49,10 @@ static std::map<sstring, sstring> prepare_options(
}
}
if (!rf.empty()) {
if (rf.has_value()) {
// The code below may end up not using "rf" at all (if all the DCs
// already have rf settings), so let's validate it once (#8880).
locator::abstract_replication_strategy::validate_replication_factor(rf);
locator::abstract_replication_strategy::validate_replication_factor(*rf);
// We keep previously specified DC factors for safety.
for (const auto& opt : old_options) {
@@ -62,7 +62,7 @@ static std::map<sstring, sstring> prepare_options(
}
for (const auto& dc : tm.get_topology().get_datacenters()) {
options.emplace(dc, rf);
options.emplace(dc, *rf);
}
}

3
test/boost/network_topology_strategy_test.cc
View File

@@ -756,6 +756,9 @@ SEASTAR_TEST_CASE(test_invalid_dcs) {
BOOST_REQUIRE_THROW(e.execute_cql("CREATE KEYSPACE abc WITH REPLICATION "
"= {'class': 'NetworkTopologyStrategy', 'dc1':'" + incorrect + "'}").get(),
exceptions::configuration_exception);
BOOST_REQUIRE_THROW(e.execute_cql("CREATE KEYSPACE abc WITH REPLICATION "
"= {'class': 'NetworkTopologyStrategy', 'replication_factor':'" + incorrect + "'}").get(),
exceptions::configuration_exception);
BOOST_REQUIRE_THROW(e.execute_cql("CREATE KEYSPACE abc WITH REPLICATION "
"= {'class': 'SimpleStrategy', 'replication_factor':'" + incorrect + "'}").get(),
exceptions::configuration_exception);