The Alternator TTL scanning code uses an object "scan_ranges_context"
to hold the scanning context. One of the members of this object is
a service::query_state, and that in turn holds a reference to a
service::client_state. The existing constructor created a temporary
client_state object and saved a reference to it - which can result
in use after free as the temporary object is freed as soon as the
constructor ends.
The fix is to save a client_state in the scan_ranges_context object,
instead of a temporary object.
Fixes#19988
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#20418
This patch address two requests made by reviewers of the original "Add
CQL-based RBAC support to Alternator" series. Both requests were about
the error messages produced when access is denied:
1. The error message is improved to use more proper English, and also
to include the name of the role which was denied access.
2. The permission-check and error-message-formatting code is
de-duplicated, using a common function verify_permission().
This de-duplication required moving the access-denied error path to
throwing an exception instead of the previous exception-free
implementation. However, it can be argued that this change is actually
a good thing, because it makes the successful case, when access is
allowed, faster.
The de-duplicated code is shorter and simpler, and allowed changing
the text of the error message in just one place.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#20326
When executing reversed queries, a native revered format shall be used. Therefore, the table schema and the clustering key bounds are reversed before a partition slice and a read command are constructed.
It is, however, possible to run a reversed query passing a table schema but only when there are no restrictions on the clustering keys. In this particular situation, the query returns correct results. Since the current alternator tests in test.py do not imply any restrictions, this situation was not caught during development of https://github.com/scylladb/scylladb/pull/18864.
Hence, additional tests are provided that add clustering keys restrictions when executing reversed queries to capture such errors earlier than in dtests.
Additional manual tests were performed to test a mixed-node cluster (with alternator API enabled in Scylla on each node):
1. 2-node cluster with one node upgraded: reverse read queries performed on an old node
2. 2-node cluster with one node upgraded: reverse read queries performed on a new node
3. 2-node cluster with one node upgraded and all its sstable files deleted to trigger repair: reverse read queries performed on an old node
4. 2-node cluster with one node upgraded and all its sstable files deleted to trigger repair: reverse read queries performed on a new node
All reverse read queries above consists of:
- single-partition reverse reads with no clustering key restrictions, with single column restrictions and multi column restrictions both with and without paging turned on
The exact same tests were also performed on a fully upgraded cluster.
Fixes https://github.com/scylladb/scylladb/issues/20191
No backport is required as this is a complementary patch for the series https://github.com/scylladb/scylladb/pull/18864 that did not require backporting.
Closesscylladb/scylladb#20205
* github.com:scylladb/scylladb:
test_query.py: Test reverse queries with clustering key bounds
alternator::do_query Add additional trace log
alternator::do_query: Use native reversed format
alternator::do_query Rename schema with table_schema
To prevent stalls due to large number of tokens.
For example, large cluster with say 70 nodes can have
more than 16K tokens.
Fixes#19757
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Add static `make` methods to ranges_holder_{primary,secondary}
and use them to make the ranges objects and pass them
to `token_ranges_owned_by_this_shard`, rather than letting
token_ranges_owned_by_this_shard invoke the right constructor
of the ranges_holder class.
Prepare for making `make` async.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Rather than holding a variant member (and defining
both ranges_holder_{primary,secondary} in both
specilizations of the class, just make the internal
ranges_holder class first-class citizens
and parameterize the `token_ranges_owned_by_this_shard`
template by this class type.
Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
Additional log prints information on the read query being executed.
It lists information like whether the query is a reversed one or
not, and table_schema and query_schema versions.
When executing reversed queries, a native revered format shall be used.
Therefore the table schema and the clustering key bounds are reversed
before a partition slice and a read command are constructed.
Similarly as for cql3::statements::select_statement.
In order to increase readability, a schema variable is renamed to
a table_schema to emphesize a table schema is passed to the function
and used across it.
Allows us to introduce a query_schema variable in the next patch.
This patch adds a requirement for the "SELECT" permission on a table to
run a GetRecords on it (the DynamoDB Streams API, i.e., CDC).
The grant is checked on the *CDC log table* - not on the base table,
which allows giving a role the ability to read the base but not is
change stream, or vice versa.
The operations ListStreams, DescribeStreams, GetShardIterators do not
require any permissions to run - they do not read any data, and are
(in my opinion) similar in spirit to DescribeTable, so I think it's fine
not to require any permissions for them.
A test is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "ALTER" permission on a table to
run a UpdateTimeToLive on it. UpdateTimeToLive is similar in purpose to
UpdateTable, so it makes sense to use the same permission "ALTER" as we
do for UpdateTable.
A tests is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "ALTER" permission on a table to
run the TagResource or UntagResource operations on it. CQL does not
have an exact parallel of DynamoDB's tagging feature, but our usual
use of tags as an extension of UpdateTable to change non-standard options
(e.g., write isolation policy or tablets setup), so it makes sense to
require the same permissions we require for UpdateTable - namely "ALTER".
A test for both operations is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "SELECT" permission on a table to
run a BatchGetItem on it. A single batch may ask to write to several
different tables, so we fail the entire batch with AccessDeniedException
if any of the tables mentioned in the batch do not have SELECT permissions
for this role.
A tests is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "MODIFY" permission on a table to
run a BatchWriteItem on it. A single batch may ask to write to several
different tables, so we fail the entire batch with AccessDeniedException
if any of the tables mentioned in the batch do not have MODIFY permissions
for this role.
A tests is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "ALTER" permission on a table to
run a UpdateTable on it.
A tests is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "SELECT" permission on a table to
run a Query or Scan on it.
Both Query and Scan operations call the same do_query() function, so the
permission checks are put there.
Note that Query can read from either the base table or one of its views,
and the permissions on the base and each of the views can be separate
(so we can allow a role to only read one view, for example).
Tests for all of the above are also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "CREATE" permission on ALL
KEYSPACES to run a CreateTable operation.
The CreateTable operation also performs so-called "auto-grant": When a
role creates a table, it is automatically granted full permissions to
read, write, change or delete that new table.
A test for all these things is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "DROP" permission on a table to
run a DeleteTable on it.
Moreover, when a table and its views are deleted, any special permissions
previously GRANTed on this table are removed. This is necessary because
if a role creates a table it is automatically granted permissions on this
table (this is known as "auto-grant" - see the CreateTable patch for
details). If this role deletes this table and later a second role creates
a table with the same name, we don't want the first role to have
permissions on this new table.
Tests for permission enforcements and revocation on delete are also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "MODIFY" permission on a table to
run a UpdateItem on it.
Only the MODIFY permission is required, even if the operation may also
read the old value of the item, such as a read-modify-write operation
or even using ReturnValues='ALL_OLD'.
A test is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "MODIFY" permission on a table to
run a DeleteItem on it.
Only the MODIFY permission is required, even if the operation may also
read the old value of the item (using ReturnValues='ALL_OLD').
A test is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch adds a requirement for the "MODIFY" permission on a table to
run a PutItem on it.
Only the MODIFY permission is required, even if the operation may also
read the old value of the item (using ReturnValues='ALL_OLD').
A test is also added.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
In this patch, we begin to add role-based access control (RBAC)
enforement to Alternator - in this patch only to GetItem.
After the preparation of client_state correctly in the previous patch,
the permission check itself in the get_item() function is very simple.
The bigger part of this patch is a full functional test in
test/alternator/test_cql_rbac.py. The test is quite self-explanatory
and heavily commented. Basically we check that a new role cannot
read with GetItem a pre-existing table, and we can add that ability
by GRANTing (in CQL) the new role the ability to SELECT the table,
the keyspace, all keyspaces, or add that ability to some other role
that this role inherits.
In the following patches, we will add role-based access control to
the Alternator operations, but the functional tests will be shorter -
we don't need to check the role inheritence, "all keyspaces" feature,
and so on, for every operation separately since they all use the
same underlying checking functions which handles these role inheritence
issues in exactly the same way.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Scylla uses a "client_state" object to encapsulate the information of
who the client is - its IP address, which user was authenticated, and so on.
For an unknown reason, Alternator created for each request an "internal"
client_state, meaning that supposedly the client for each request was
some sort of internal process (e.g., repair) rather than a real client.
This was wrong, and we even had a FIXME about not putting the client's
IP address in client_state.
So in this patch, we start using a normal "external" client_state
instead of an "internal" one. The client_state constructors are very
different in the two cases, so a few lines of code had to change.
I hope that this change will cause no functional changes. For example,
Alternator was already setting its own timeouts explicitly and not
relying on the default ones for external clients. However, we need to
fix this for the following patches which introduce permissions checks
(Role-Based Access Control - RBAC) - the client_state methods for
checking permissions become no-ops for *internal* clients (even if the
client_state contains an authenticated users). We need these functions
to do their job - so we need an *external* variant of client_state.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
This patch Updated the get and write batch operations in Alternator to
record latency using the newly added histogram metrics.
It adds logic to increment the counters with the number of items
processed in each batch.
Signed-off-by: Amnon Heiman <amnon@scylladb.com>
Introduced histogram metrics to track latency for Alternator's get and
write batch operations.
Added counters to record the number of items processed in each batch
operation.
Signed-off-by: Amnon Heiman <amnon@scylladb.com>
assert() is traditionally disabled in release builds, but not in
scylladb. This hasn't caused problems so far, but the latest abseil
release includes a commit [1] that causes a 1000 insn/op regression when
NDEBUG is not defined.
Clearly, we must move towards a build system where NDEBUG is defined in
release builds. But we can't just define it blindly without vetting
all the assert() calls, as some were written with the expectation that
they are enabled in release mode.
To solve the conundrum, change all assert() calls to a new SCYLLA_ASSERT()
macro in utils/assert.hh. This macro is always defined and is not conditional
on NDEBUG, so we can later (after vetting Seastar) enable NDEBUG in release
mode.
[1] 66ef711d68Closesscylladb/scylladb#20006
The Alternator command ListTables is supposed to list actual tables
created with CreateTable, and should list things like materialized views
(created for GSI or LSI) or CDC log tables.
We already properly excluded materialized views from the list - and
had the tests to prove it - but forgot both the exclusion and the testing
for CDC log tables - so creating a table xyz with streams enable would
cause ListTables to also list "xyz_scylla_cdc_log".
This patch fixes both oversights: It adds the code to exclude CDC logs
from the output of ListTables, add adds a test which reproduces the bug
before this fix, and verifies the fix works.
Fixes#19911.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#19914
Alternator allows authentication into the existing CQL roles, but
roles which have the flag "login=false" should be refused in
authentication, and this patch adds the missing check.
The patch also adds a regression test for this feature in the
test/alternator test framework, in a new test file
test/alternator/cql_rbac.py. This test file will later include more
tests of how the CQL RBAC commands (CREATE ROLE, GRANT, REVOKE)
affect authentication and authorization in Alternator.
In particular, these tests need to use not just the DynamoDB API but
also CQL, so this new test file includes the "cql" fixture that allows
us to run CQL commands, to create roles, to retrieve their secret keys,
and so on.
Fixesscylladb/scylladb#19735Closesscylladb/scylladb#19740
Alternator's "/localnodes" HTTP request is supposed to return the list of
nodes in the local DC to which the user can send requests.
The existing implementation incorrectly used gossiper::is_alive() to check
for which nodes to return - but "alive" nodes include nodes which are still
joining the cluster and not really usable. These nodes can remain in the
JOINING state for a long time while they are copying data, and an attempt
to send requests to them will fail.
The fix for this bug is trivial: change the call to is_alive() to a call
to is_normal().
But the hard part of this test is the testing:
1. An existing multi-node test for "/localnodes" assummed that right after
a new node was created, it appears on "/localnodes". But after this
patch, it may take a bit more time for the bootstrapping to complete
and the new node to appear in /localnodes - so I had to add a retry loop.
2. I added a test that reproduces the bug fixed here, and verifies its
fix. The test is in the multi-node topology framework. It adds an
injection which delays the bootstrap, which leaves a new node in JOINING
state for a long time. The test then verifies that the new node is
alive (as checked by the REST API), but is not returned by "/localnodes".
3. The new injection for delaying the bootstrap is unfortunately not
very pretty - I had to do it in three places because we have several
code paths of how bootstrap works without repair, with repair, without
Raft and with Raft - and I wanted to delay all of them.
Fixes#19694.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#19725
This short series fixes Alternator's "/localnodes" request to allow a node's external IP address - configured with `broadcast_rpc_address` - to be listed instead of its usual, internal, IP address.
The first patch fixes a bug in gossiper::get_rpc_address(), which the second patch needs to implement the feature. The second patch also contains regression tests.
Fixes#18711.
Closesscylladb/scylladb#18828
* github.com:scylladb/scylladb:
alternator: fix "/localnodes" to use broadcast_rpc_address
gossiper: fix get_rpc_address() for this node
Alternator's non-standard "/localnodes" HTTP request returns a list of
live nodes on this DC, to consider for load balancing. The returned
node addresses should be external IP addresses usable by the clients.
Scylla has a configuration parameter - broadcast_rpc_address - which
defines for a node an external IP address. If such a configuration
exists, we need to use those external IP addresses, not the internal
ones.
Finding these broadcast_rpc_address of all nodes is easy, because the
gossiper already gossips them.
This patch also tests the new feature:
1. The existing single-node test is extended to verify that without
broadcast_rpc_address we get the usual IP address.
2. A new two-node test is added to check that when broadcast_rpc_address
is configured, we get that address and not the usual internal IP
addresses.
Fixes#18711.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
since we've switched almost all callers of the operator<< to {fmt},
let's drop the unused operator<<:s.
the callers in alternator/streams.cc is updated to use `fmt::print()`
to format the `bytes` instances.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#19448
before this change, when linking an executable referencing `marker`,
we could have following error:
```
13:58:02 ld.lld: error: undefined symbol: alternator::event_id::marker
13:58:02 >>> referenced by streams.cc
13:58:02 >>> build/dev/alternator/streams.o:(from_string_helper<rapidjson::GenericValue<rapidjson::UTF8<char>, rjson::internal::throwing_allocator>, alternator::event_id>::Set(rapidjson::GenericValue<rapidjson::UTF8<char>, rjson::internal::throwing_allocator>&, alternator::event_id, rjson::internal::throwing_allocator&))
13:58:02 clang-16: error: linker command failed with exit code 1 (use -v to see invocation)
```
it turns out `event_id::marker` is only declared, but never defined.
please note, the non-inline static member variable in its class
definition is not considered as a definition, see
[class.static.data](https://eel.is/c++draft/class.static.data#3)
> The declaration of a non-inline static data member in its class
> definition is not a definition and may be of an incomplete type
> other than cv void.
so, let's declare it as a `constexpr` instead. it implies `inline`.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#19452
thrift support was deprecated since ScyllaDB 5.2
> Thrift API - legacy ScyllaDB (and Apache Cassandra) API is
> deprecated and will be removed in followup release. Thrift has
> been disabled by default.
so let's drop it. in this change,
* thrift protocol support is dropped
* all references to thrift support in document are dropped
* the "thrift_version" column in system.local table is preserved for backward compatibility, as we could load from an existing system.local table which still contains this clolumn, so we need to write this column as well.
* "/storage_service/rpc_server" is only preserved for backward compatibility with java-based nodetool.
Fixes#3811Fixes#18416
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
- [x] not a fix, no need to backport
Closesscylladb/scylladb#18453
* github.com:scylladb/scylladb:
config: expand on rpc_keepalive's description
api: s/rpc/thrift/
db/system_keyspace: drop thrift_version from system.local table
transport: do not return client_type from cql_server::connection::make_client_key()
treewide: drop thrift support
in 57c408ab, we dropped operator<< for `parsed::path`, but we forgot
to drop the friend declaration for it along with the operator. so in
this change, let's drop the friend declaration.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#19287
thrift support was deprecated since ScyllaDB 5.2
> Thrift API - legacy ScyllaDB (and Apache Cassandra) API is
> deprecated and will be removed in followup release. Thrift has
> been disabled by default.
so let's drop it. in this change,
* thrift protocol support is dropped
* all references to thrift support in document are dropped
* the "thrift_version" column in system.local table is
preserved for backward compatibility, as we could load
from an existing system.local table which still contains
this clolumn, so we need to write this column as well.
* "/storage_service/rpc_server" is only preserved for
backward compatibility with java-based nodetool.
* `rpc_port` and `start_rpc` options are preserved, but
they are marked as "Unused". so that the new release
of scylladb can consume existing scylla.yaml configurations
which might contain these settings. by making them
deprecated, user will be able get warned, and update
their configurations before we actually remove them
in the next major release.
Fixes#3811Fixes#18416
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
... and replace it with boolean enable_tablets option. All the places
in the code are patched to check the latter option instead of the former
feature.
The option is OFF by default, but the default scylla.yaml file sets this
to true, so that newly installed clusters turn tablets ON.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#18898
This patch cleans up a bit the code in Alternator which splits up
the operation's X-Amz-Target header (the second part of it is the
name of the operation, e.g., CreateTable).
The patch doesn't change any functionality or change performance in
any meaningful way. I was just reviewing this code and was annoyed by
the unnecessary variable and unnecessary creation of strings and
vectors for such a simple operation - and wanted to clean it up.
Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Closesscylladb/scylladb#18830
Database used to be (and still is in many ways) an object used to get configuration from. Part of the configuration is the set of pre-configured scheduling groups. That's not nice, services should use each other for some real need, not as proxies to configuration. This patch patches the places that explicitly switch to statement group _not_ to use database to get the group itself.
fixes: #17643Closesscylladb/scylladb#18799
* github.com:scylladb/scylladb:
database: Don't export statement scheduling group
test: Use async attrs and cql-test-env scheduling groups
test: Use get_scheduling_groups() to get scheduling groups
api: Don't switch sched group to start/stop protocol servers
main: Don't switch sched group to start protocol servers
code: Switch to sched group in request_stop_server()
code: Switch to server sched group in start()
protocol_server: Keep scheduling group on board
code: Add scheduling group to controllers
redis: Coroutinize start() method
in theory, std::result_of_t should have been removed in C++20. and
std::invoke_result_t is available since C++17. thanks to libstdc++,
the tree is compiling. but we should not rely on this.
so, in this change, we replace all `std::result_of_t` with
`std::invoke_result_t`. actually, clang + libstdc++ is already warning
us like:
```
In file included from /home/runner/work/scylladb/scylladb/multishard_mutation_query.cc:9:
In file included from /home/runner/work/scylladb/scylladb/schema/schema_registry.hh:11:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/unordered_map:38:
Warning: /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/type_traits:2624:5: warning: 'result_of<void (noop_compacted_fragments_consumer::*(noop_compacted_fragments_consumer &))()>' is deprecated: use 'std::invoke_result' instead [-Wdeprecated-declarations]
2624 | using result_of_t = typename result_of<_Tp>::type;
| ^
/home/runner/work/scylladb/scylladb/mutation/mutation_compactor.hh:518:43: note: in instantiation of template type alias 'result_of_t' requested here
518 | if constexpr (std::is_same_v<std::result_of_t<decltype(&GCConsumer::consume_end_of_stream)(GCConsumer&)>, void>) {
|
```
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
Closesscylladb/scylladb#18835
This method is used to stop protocol server in the runtime (via the
API). Since it's not just "kick it and wait to wrap up", it's needed to
perform this in the inherited sched group too.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
This patch makes all protocol servers implementations use the inherited
sched group in their start methods.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The groups is now mandatory for the real protocol server implementation
to initialize. Previous patch make all of them get the sched group as
constructor argument, so that's where to take it from.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
There are four of them currently -- transport, thrift, alternator and
redis. This patch makes main pass to all the statement scheduling group
as constructor argument. Next patches will make use of it.
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
The existing inet_address::to_string() calls fmt::format("{}", *this)
anyway. However, the to_string() method is declared in .cc file, while
form formatter is in the header and is equipeed with constexprs so
that converting an address to string is done as much as possible
compile-time.
Also, though minor, fmt::to_string(foo) is believed to be even faster
than fmt::format("{}", foo).
Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
Closesscylladb/scylladb#18712
