schemeshard: add notion and protection of system path names #19620

ijon · 2025-06-11T16:23:45Z

New feature flag enable_system_names_protection prohibits users from creating paths with names reserved for current or future system use.

YDB exclusively reserves names like .sys, .metadata, .tmp, .backups etc. and any names starting with prefixes . and __ydb for its own use.

More details, reasoning and behavior in:

add the concept of "path names reserved for system use" #11673

Also introduces safeguards against carelessly using reserved names in future code.

Special tests verify that:

New scheme create operations include protection for reserved names
Modifications to the reserved name registry will not go unnoticed

The latter is designed to protect well-meaning developers from careless actions.
YDB developers must obtain explicit consent from the project committee to add new names to the reserved list.

github-actions · 2025-06-11T16:26:09Z

🟢 2025-06-11 16:26:08 UTC The validation of the Pull Request description is successful.

Copilot

Pull Request Overview

Add protection against creating or renaming paths with system-reserved names under a new feature flag, propagate the user token to leaf-name validation in create operations, and update alter operations and persistence to accommodate the new ACL and naming logic.

Propagate context.UserToken.Get() to all IsValidLeafName calls in create and copy operations
Remove leaf-name, depth, paths, children and ACL checks from alter operations and adjust persistence signatures
Introduce EnableSystemNamesProtection feature flag, update ticket parser domain constants, and refresh tests

Reviewed Changes

Copilot reviewed 60 out of 60 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
ydb/core/tx/schemeshard/schemeshard__operation_create_bsv.cpp	Pass `UserToken` to leaf-name validation
ydb/core/tx/schemeshard/schemeshard__operation_create_backup_collection.cpp	Pass `UserToken` to leaf-name validation
ydb/core/tx/schemeshard/schemeshard__operation_copy_table.cpp	Pass `UserToken` to leaf-name validation
ydb/core/tx/schemeshard/schemeshard__operation_copy_sequence.cpp	Pass `UserToken` to leaf-name validation
ydb/core/tx/schemeshard/schemeshard__operation_common_resource_pool.h	Rename parameter `acll` → `acl`
ydb/core/tx/schemeshard/schemeshard__operation_blob_depot.cpp	Pass `UserToken` to leaf-name validation
ydb/core/tx/schemeshard/schemeshard__operation_alter_resource_pool.cpp	Remove leaf-name & ACL checks in alter; adjust `PersistResourcePool`
ydb/core/tx/schemeshard/schemeshard__operation_alter_external_table.cpp	Remove leaf-name & ACL checks in alter; strip ACL from persistence
ydb/core/tx/schemeshard/schemeshard__operation_alter_external_data_source.cpp	Remove leaf-name & ACL checks in alter; strip ACL from persistence
ydb/core/tx/schemeshard/schemeshard__operation.cpp	Update `CreateDirs` signature, comment out outdated name checks
ydb/core/tx/schemeshard/schemeshard__op_traits.h	Add operation classification declarations
ydb/core/tx/schemeshard/schemeshard__op_traits.cpp	Implement operation classification functions
ydb/core/tx/schemeshard/olap/operations/create_table.cpp	Pass `UserToken` to leaf-name validation
ydb/core/tx/schemeshard/olap/operations/create_store.cpp	Pass `UserToken` to leaf-name validation
ydb/core/testlib/basics/feature_flags.h	Add `EnableSystemNamesProtection` flag setter
ydb/core/protos/feature_flags.proto	Add `EnableSystemNamesProtection` field to proto
ydb/core/security/ticket_parser_impl.h	Update system-domain token parsing to use `AUTH_DOMAIN_SYSTEM`
ydb/core/kqp/workload_service/ut/kqp_workload_service_actors_ut.cpp	Update test default `userSID` to use `AUTH_DOMAIN_SYSTEM`
ydb/core/kqp/workload_service/ut/common/kqp_workload_service_ut_common.h	Update test default `UserSID` to use `AUTH_DOMAIN_SYSTEM`
ydb/core/kqp/workload_service/ut/common/kqp_workload_service_ut_common.cpp	Refresh tests for new system-domain constant

ydb/core/tx/schemeshard/schemeshard__operation_alter_resource_pool.cpp

ydb/core/security/ticket_parser_impl.h

ydb/core/tx/schemeshard/schemeshard__op_traits.h

ydb/core/tx/schemeshard/schemeshard__operation.cpp

ijon · 2025-06-17T10:33:01Z

⚪ 2025-06-16 23:07:10 UTC Pre-commit check linux-x86_64-relwithdebinfo for d5a9cb6 has started.

Упавший тест ydb/public/sdk/cpp/src/client/topic/ut/with_direct_read_ut:TxUsage.Transactions_Conflict_On_SeqNo_Table не относится никак к изменениям и, согласно истории, моргает на разных сборках и разных pr'ах.

github-actions · 2025-06-24T17:17:09Z

⚪ 2025-06-24 17:17:08 UTC Pre-commit check linux-x86_64-release-asan for 69dd5a4 has started.
⚪ 2025-06-24 17:17:19 UTC Artifacts will be uploaded here
⚪ 2025-06-24 17:21:09 UTC ya make is running...
🟡 2025-06-24 19:22:22 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
16343	15939	0	108	260	36

🟢 2025-06-24 19:23:55 UTC Build successful.
🟢 2025-06-24 19:24:23 UTC ydbd size 3.9 GiB changed* by +98.9 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `04c388e`	merge: `69dd5a4`	diff	diff %
ydbd size	4 191 869 120 Bytes	4 191 970 440 Bytes	+98.9 KiB	+0.002%
ydbd stripped size	1 453 112 632 Bytes	1 453 138 424 Bytes	+25.2 KiB	+0.002%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

github-actions · 2025-06-24T17:17:21Z

⚪ 2025-06-24 17:17:20 UTC Pre-commit check linux-x86_64-relwithdebinfo for 69dd5a4 has started.
⚪ 2025-06-24 17:17:31 UTC Artifacts will be uploaded here
⚪ 2025-06-24 17:21:14 UTC ya make is running...
🟡 2025-06-24 18:32:19 UTC Some tests failed, follow the links below. Going to retry failed tests...

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
38660	35935	0	3	2670	52

⚪ 2025-06-24 18:35:37 UTC ya make is running... (failed tests rerun, try 2)
🟡 2025-06-24 18:47:10 UTC Some tests failed, follow the links below. Going to retry failed tests...

Test history | Ya make output | Test bloat | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
541 (only retried tests)	461	0	44	5	31

⚪ 2025-06-24 18:47:21 UTC ya make is running... (failed tests rerun, try 3)
🟢 2025-06-24 18:58:19 UTC Tests successful.

Test history | Ya make output | Test bloat | Test bloat | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
351 (only retried tests)	320	0	0	2	29

🟢 2025-06-24 18:58:27 UTC Build successful.
🟢 2025-06-24 18:58:47 UTC ydbd size 2.2 GiB changed* by +56.2 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `04c388e`	merge: `69dd5a4`	diff	diff %
ydbd size	2 382 766 744 Bytes	2 382 824 256 Bytes	+56.2 KiB	+0.002%
ydbd stripped size	498 847 400 Bytes	498 852 264 Bytes	+4.8 KiB	+0.001%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

For external-data-source, external-table, resource-pool. Current alters for those object check validity of the name, schema limits and rights. It makes no sense for an alter to do those checks as altering object is already exist and alter can't change rights. Apparently, those check were brought from create ops that do require those checks.

github-actions · 2025-06-25T21:47:36Z

⚪ 2025-06-25 21:47:36 UTC Pre-commit check linux-x86_64-release-asan for d70ba41 has started.
⚪ 2025-06-25 21:47:40 UTC Artifacts will be uploaded here
⚪ 2025-06-25 21:51:24 UTC ya make is running...
🟡 2025-06-26 00:22:11 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
16355	16031	0	91	197	36

🟢 2025-06-26 00:23:36 UTC Build successful.
🟢 2025-06-26 00:24:04 UTC ydbd size 3.9 GiB changed* by +93.6 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `63058f0`	merge: `d70ba41`	diff	diff %
ydbd size	4 192 746 592 Bytes	4 192 842 416 Bytes	+93.6 KiB	+0.002%
ydbd stripped size	1 453 455 672 Bytes	1 453 476 696 Bytes	+20.5 KiB	+0.001%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

github-actions · 2025-06-25T21:49:33Z

⚪ 2025-06-25 21:49:32 UTC Pre-commit check linux-x86_64-relwithdebinfo for d70ba41 has started.
⚪ 2025-06-25 21:49:43 UTC Artifacts will be uploaded here
⚪ 2025-06-25 21:53:20 UTC ya make is running...
🟡 2025-06-25 23:41:49 UTC Some tests failed, follow the links below. Going to retry failed tests...

Test history | Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
38673	35956	0	4	2671	42

⚪ 2025-06-25 23:45:18 UTC ya make is running... (failed tests rerun, try 2)
🟢 2025-06-25 23:57:12 UTC Tests successful.

Test history | Ya make output | Test bloat | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
432 (only retried tests)	397	0	0	6	29

🟢 2025-06-25 23:57:20 UTC Build successful.
🟢 2025-06-25 23:57:38 UTC ydbd size 2.2 GiB changed* by +53.5 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash	main: `63058f0`	merge: `d70ba41`	diff	diff %
ydbd size	2 383 108 552 Bytes	2 383 163 328 Bytes	+53.5 KiB	+0.002%
ydbd stripped size	498 964 424 Bytes	498 968 840 Bytes	+4.3 KiB	+0.001%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

ydb/core/tx/schemeshard/schemeshard_path.cpp

CyberROFL · 2025-06-26T12:27:56Z

ydb/core/tx/schemeshard/schemeshard__operation.cpp

@@ -868,10 +860,6 @@ bool CreateDirs(const TTxTransaction& tx, const TPath& parentPath, TPath path, T
                .NotResolved();
        }

-        if (checks) {
-            checks.IsValidLeafName();


Что-то я не понял, почему это можно убирать?

Да, это важный момент.

Проверка здесь происходит раньше любой проверки в самих операциях, но и дублирует их. Операции создаваемые имена проверяют (а тесты проверяют, что они проверяют), и дублирующая проверка оказывается не необходимой.
Кроме того, общая проверка не может учитывать послабления, необходимые в отдельных случаях: например, при move таблицы её внутренние объекты должны уметь перемещаться, даже если их имена оказались зарезервированными -- и т.п.

Либо решение о допустимости имени принимает операция/код, который точно знает что происходит, либо эти особые условия нужно абстрагировать и доносить до уровня CreateDirs(). Если второе сделать, то проверку имени можно будет убрать наоборот из самих операций (снова чтобы не было двойных проверок).
Однако пока легче не делать дополнительную проверку на уровне CreateDirs().

Если MkDir умеет в AbortPropose, тогда, наверно, можно эту дублирующую проверку убирать. Только нужно убедиться, что есть тест, создающий объект с невалидным именем и промежуточными директориями.

Если MkDir умеет в AbortPropose

Да, умеет.

что есть тест, создающий объект с невалидным именем и промежуточными директориями

Да, такие тесты делаются -- ut_system_names/ut_system_names.cpp#L862 и ниже.

nikvas0

kqp ok

ijon requested review from a team as code owners June 11, 2025 16:23

ijon marked this pull request as draft June 11, 2025 16:24

ijon force-pushed the schemeshard-add-notion-of-system-path-names branch from 4f116a1 to cc87125 Compare June 11, 2025 16:25

ijon force-pushed the schemeshard-add-notion-of-system-path-names branch from cc87125 to 022912e Compare June 11, 2025 19:16

ydb-platform deleted a comment from github-actions bot Jun 11, 2025

ijon requested a review from Copilot June 11, 2025 19:16