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

alternator: hide internal tags from users

Browse Source 
The "tags" mechanism in Alternator is a convenient way to attach metadata
to Alternator tables. Recently we have started using it more and more for
internal metadata storage:

  * UpdateTimeToLive stores the attribute in a tag system:ttl_attribute
  * CreateTable stores provisioned throughput in tags
    system:provisioned_rcu and system:provisioned_wcu
  * CreateTable stores the table's creation time in a tag called
    system:table_creation_time.

We do not want any of these internal tags to be visible to a
ListTagsOfResource request, because if they are visible (as before this
patch), systems such as Terraform can get confused when they suddenly
see a tag which they didn't set - and may even attempt to delete it
(as reported in issue #24098).

Moreover, we don't want any of these internal tags to be writable
with TagResource or UntagResource: If a user wants to change the TTL
setting they should do it via UpdateTimeToLive - not by writing
directly to tags.

So in this patch we forbid read or write to *any* tag that begins
with the "system:" prefix, except one: "system:write_isolation".
That tag is deliberately intended to be writable by the user, as
a configuration mechanism, and is never created internally by
Scylla. We should have perhaps chosen a different prefix for
configurable vs. internal tags, or chosen more unique prefixes -
but let's not change these historic names now.

This patch also adds regression tests for the internal tags features,
failing before this patch and passing after:
1. internal tags, specifically system:ttl_attribute, are not visible
   in ListTagsOfResource, and cannot be modified by TagResource or
   UntagResource.
2. system:write_isolation is not internal, and be written by either
   TagResource or UntagResource, and read with ListTagsOfResource.

This patch also fixes a bug in the test where we added more checks
for system:write_isolation - test_tag_resource_write_isolation_values.
This test forgot to remove the system:write_isolation tags from
test_table when it ended, which would lead to other tests that run
later to run with a non-default write isolation - something which we
never intended.

Fixes #24098.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Closes scylladb/scylladb#24299
This commit is contained in:
Nadav Har'El
2025-05-28 16:28:21 +03:00
committed by Botond Dénes
parent 37e6ff1a3c
commit 6cbcabd100
3 changed files with 71 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
alternator/executor.cc
View File

@@ -1034,6 +1034,17 @@ void rmw_operation::set_default_write_isolation(std::string_view value) {
default_write_isolation = parse_write_isolation(value);
}
// Alternator uses tags whose keys start with the "system:" prefix for
// internal purposes. Those should not be readable by ListTagsOfResource,
// nor writable with TagResource or UntagResource (see #24098).
// Only a few specific system tags, currently only system:write_isolation,
// are deliberately intended to be set and read by the user, so are not
// considered "internal".
static bool tag_key_is_internal(std::string_view tag_key) {
return tag_key.starts_with("system:") &&
tag_key != rmw_operation::WRITE_ISOLATION_TAG_KEY;
}
enum class update_tags_action { add_tags, delete_tags };
static void update_tags_map(const rjson::value& tags, std::map<sstring, sstring>& tags_map, update_tags_action action) {
if (action == update_tags_action::add_tags) {
@@ -1058,6 +1069,9 @@ static void update_tags_map(const rjson::value& tags, std::map<sstring, sstring>
if (!validate_legal_tag_chars(tag_key)) {
throw api_error::validation("A tag Key can only contain letters, spaces, and [+-=._:/]");
}
if (tag_key_is_internal(tag_key)) {
throw api_error::validation(fmt::format("Tag key '{}' is reserved for internal use", tag_key));
}
// Note tag values are limited similarly to tag keys, but have a
// longer length limit, and *can* be empty.
if (tag_value.size() > 256) {
@@ -1070,7 +1084,11 @@ static void update_tags_map(const rjson::value& tags, std::map<sstring, sstring>
}
} else if (action == update_tags_action::delete_tags) {
for (auto it = tags.Begin(); it != tags.End(); ++it) {
tags_map.erase(sstring(it->GetString(), it->GetStringLength()));
auto tag_key = rjson::to_string_view(*it);
if (tag_key_is_internal(tag_key)) {
throw api_error::validation(fmt::format("Tag key '{}' is reserved for internal use", tag_key));
}
tags_map.erase(sstring(tag_key));
}
}
@@ -1145,6 +1163,9 @@ future<executor::request_return_type> executor::list_tags_of_resource(client_sta
rjson::value& tags = ret["Tags"];
for (auto& tag_entry : tags_map) {
if (tag_key_is_internal(tag_entry.first)) {
continue;
}
rjson::value new_entry = rjson::empty_object();
rjson::add(new_entry, "Key", rjson::from_string(tag_entry.first));
rjson::add(new_entry, "Value", rjson::from_string(tag_entry.second));

11
test/alternator/test_tag.py
View File

@@ -144,6 +144,13 @@ def test_tag_resource_write_isolation_values(scylla_only, test_table):
test_table.meta.client.tag_resource(ResourceArn=arn, Tags=[{'Key':'system:write_isolation', 'Value':i}])
with pytest.raises(ClientError, match='ValidationException'):
test_table.meta.client.tag_resource(ResourceArn=arn, Tags=[{'Key':'system:write_isolation', 'Value':'bah'}])
# Verify that reading system:write_isolation is possible (we didn't
# accidentally prevent it while fixing #24098)
keys = [tag['Key'] for tag in test_table.meta.client.list_tags_of_resource(ResourceArn=arn)['Tags']]
assert 'system:write_isolation' in keys
# Finally remove the system:write_isolation tag so to not modify the
# default behavior of test_table.
test_table.meta.client.untag_resource(ResourceArn=arn, TagKeys=['system:write_isolation'])
# Test that if trying to create a table with forbidden tags (in this test,
# a list of tags longer than the maximum allowed of 50 tags), the table
@@ -168,9 +175,9 @@ def test_too_long_tags_from_creation(dynamodb):
dynamodb.meta.client.describe_table(TableName=name)
# This test is similar to the above, but uses another case of forbidden tags -
# here an illegal value for the system::write_isolation tag. This is a
# here an illegal value for the system:write_isolation tag. This is a
# scylla_only test because only Alternator checks the validity of the
# system::write_isolation tag.
# system:write_isolation tag.
# Reproduces issue #6809, where the table creation appeared to fail, but it
# was actually created (without the tag).
def test_forbidden_tags_from_creation(scylla_only, dynamodb):

42
test/alternator/test_ttl.py
View File

@@ -13,8 +13,7 @@ from decimal import Decimal
import pytest
from botocore.exceptions import ClientError
from test.alternator.util import new_test_table, random_string, full_query, unique_table_name, is_aws, \
client_no_transform
from .util import new_test_table, random_string, full_query, unique_table_name, is_aws, client_no_transform, multiset
# All tests in this file are expected to fail with tablets due to #16567.
# To ensure that Alternator TTL is still being tested, instead of
@@ -809,3 +808,42 @@ def test_ttl_expiration_long(dynamodb, waits_for_expiration):
break
time.sleep(max_duration/100.0)
assert count == 99*N
# Alternator uses a tag "system:ttl_attribute" to store the TTL attribute
# chosen by UpdateTimeToLive. However, this tag is not supposed to be
# readable or writable by the user directly - it should be read or written
# only with the usual UpdateTimeToLive and DescribeTimeToLive operations.
# The following two test confirms that this is the case. The first test
# checks that the internal tag is invisible, i.e., not returned by
# ListTagsOfResource. Basically we check that enabling TTL does not add
# any tags to the list of tags.
# Reproduces issue #24098.
def test_ttl_tag_is_invisible(dynamodb):
with new_test_table(dynamodb,
Tags=TAGS,
KeySchema=[{ 'AttributeName': 'p', 'KeyType': 'HASH' }],
AttributeDefinitions=[{ 'AttributeName': 'p', 'AttributeType': 'S' }]
) as table:
client = table.meta.client
client.update_time_to_live(TableName=table.name,
TimeToLiveSpecification={'AttributeName': 'x', 'Enabled': True})
# Verify that TTL is set for this table, but no extra tags
# like "system:ttl_attribute" (or anything else) are visible
# in ListTagsOfResource:
assert client.describe_time_to_live(TableName=table.name)['TimeToLiveDescription'] == {'TimeToLiveStatus': 'ENABLED', 'AttributeName': 'x'}
arn = client.describe_table(TableName=table.name)['Table']['TableArn']
assert multiset(TAGS) == multiset(client.list_tags_of_resource(ResourceArn=arn)['Tags'])
# Now check that the internal tag system:ttl_attribute cannot be written with
# TagResource or UntagResource (it can only be modified by UpdateTimeToLive).
# This is an Scylla-only test because in DynamoDB, there is nothing
# special about the tag name "system:ttl_attribute", and it can be written.
# Reproduces issue #24098.
def test_ttl_tag_is_unwritable(test_table, scylla_only):
tag_name = 'system:ttl_attribute'
client = test_table.meta.client
arn = client.describe_table(TableName=test_table.name)['Table']['TableArn']
with pytest.raises(ClientError, match='ValidationException.*internal'):
client.tag_resource(ResourceArn=arn, Tags=[{'Key': tag_name, 'Value': 'x'}])
with pytest.raises(ClientError, match='ValidationException.*internal'):
client.untag_resource(ResourceArn=arn, TagKeys=[tag_name])