Fix table engine and database engine grants with source access checks#100746
Conversation
Re-introduce dynamic engine lookup from StorageFactory (replacing the hardcoded source_table_engines list) and additionally grant database engines from DatabaseFactory when table_engines_require_grant=false. The previous attempt (ClickHouse#98984) missed database engines (Atomic, Shared, Replicated, etc.) which are registered in DatabaseFactory, not StorageFactory. CREATE DATABASE checks TABLE_ENGINE access in InterpreterCreateQuery, so omitting them caused ACCESS_DENIED on CREATE DATABASE. Closes ClickHouse#71544
|
Workflow [PR], commit [90027fc] Summary: ✅ AI ReviewSummaryThis PR correctly restores dynamic engine-based implicit Findings❌ Blockers
Tests
ClickHouse Rules
Final VerdictStatus: Minimum required actions:
|
clickhouse-gh
Bot
added
the
pr-bugfix
…ine-grants-v2 # Conflicts: # tests/integration/test_grant_and_revoke/test_without_table_engine_grant.py
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| instance.query("DROP USER IF EXISTS A, B") | ||
| instance.query("CREATE USER A") | ||
| instance.query("CREATE USER B") | ||
| instance.query("GRANT TABLE ENGINE ON * TO A WITH GRANT OPTION") |
There was a problem hiding this comment.
This regression test currently verifies only that GRANT TABLE ENGINE ... WITH GRANT OPTION can be delegated once.
The bug fixed in ContextAccess::addImplicitAccessRights was specifically about revoke-based source synchronization wiping root_with_grant_option. Please extend this test to cover that path explicitly, e.g. grant READ, WRITE ON URL to A, then REVOKE READ, WRITE ON URL FROM A, and assert A can still execute GRANT TABLE ENGINE ON * TO B.
|
The flaky test failures in this PR ( |
|
The |
|
The MSan stress test failure (MemorySanitizer: use-of-uninitialized-value, STID 4179-5154 or 4148-3044) is a known pre-existing issue unrelated to this PR. Fix: #102158 |
Database engines that share names with source storage engines (PostgreSQL, MySQL, SQLite, S3, MaterializedPostgreSQL) were unconditionally granted TABLE_ENGINE, overriding the source-aware checks from the StorageFactory loop. Skip names already present in StorageFactory — those were already handled correctly.
Database engines that connect to external sources (PostgreSQL, MySQL, S3, etc.) now declare their source dependency via source_access_type, mirroring how StorageFactory works. The implicit grants logic in addImplicitAccessRights applies the same READ/WRITE → TABLE_ENGINE bridge for both factories. Previously, source checks for database engines relied on name collisions with StorageFactory — correct by coincidence, not by design. Database-only engines like Filesystem bypassed source access checks entirely. Source database engine registrations: - MySQL → MYSQL, PostgreSQL → POSTGRES, MaterializedPostgreSQL → POSTGRES - SQLite → SQLITE, S3 → S3, HDFS → HDFS, Filesystem → FILE DataLakeCatalog left without source_access_type (polymorphic; underlying source depends on runtime catalog type).
pufit
changed the title
Clang errors on designated initializers in a default argument when a
member has a default member initializer inside the enclosing class.
Use `= {}` instead, which value-initializes all fields from their
in-class defaults.
| @@ -1,5 +1,6 @@ | |||
| #pragma once | |||
|
|
|||
| #include <Access/Common/AccessType.h> | |||
There was a problem hiding this comment.
DatabaseFactory.h is included by many translation units, and this new direct include of AccessType.h pulls in a heavy access-rights dependency chain (macros, alias maps, boost helpers) into every includer.
This is a compilation-time regression risk in a hot header. You only need the type name for std::optional<AccessTypeObjects::Source> here.
Please avoid the transitive include by introducing a lightweight forward declaration header for source objects (or equivalent forward declaration of AccessTypeObjects::Source) and include full AccessType.h only in .cpp files that require definitions.
Clang requires default member initializers to be visible inside the
enclosing class definition. Using `= {}` still triggers this because
all fields rely on their default member initializers. List all 4
fields explicitly so the compiler doesn't need in-class defaults.
|
The flaky check failure is fixed in #102148, let's update the branch. |
|
@groeneai can you check tests here? |
|
@pufit I checked the CI results on the latest commit ( ✅ CI is cleanAll regular checks pass — builds, stateless tests, integration tests, stress tests, AST fuzzers, BuzzHouse, unit tests, performance comparison, compatibility checks — all green. Zero test failures on the latest commit outside of bugfix validation and flaky checks. Bugfix validation — correct ✅All 20 Remaining flaky check failures — pre-existing, unrelated
Neither is related to your changes. Earlier failures — all resolved
LGTM from a CI perspective — no PR-caused failures remaining. 👍 |
Split flaky check and targeted check into separate roles: Flaky check — runs only new/modified tests from the PR with thread fuzzer enabled. Tests run in parallel with themselves (shared queue) to ensure new tests are parallel-safe under randomized thread ordering. Targeted check — runs previously failed and coverage-relevant tests with per-worker partitioning (--no-self-parallel) so the same test never runs concurrently on multiple workers, avoiding conflicts on shared resources (hardcoded users, roles, settings profiles, etc.). Both use --test-runs 50. Changes: - Add --no-self-parallel flag to clickhouse-test: partitions tests by name hash so all copies go to the same worker - Flaky check: only changed tests, thread fuzzer, shared queue - Targeted check: relevant tests, no thread fuzzer, per-worker queues, 50 runs, single clickhouse-test invocation - Restore original thread fuzzer parameters (reverts reductions from 06c945a and 9790435) - Add amd_binary to flaky check, use AMD_LARGE for targeted debug - Targeted check uses --flaky-check --no-self-parallel PRs affected by tests running in parallel with themselves: #102015 #101093 #99653 #101503 #101108 #101884 #100746 #101447 #100941 #101099 #100203 #96130 #102038
Split flaky check and targeted check into separate roles: Flaky check — runs only new/modified tests from the PR with thread fuzzer enabled. Tests can run in parallel with themselves (shared queue) to ensure new tests are parallel-safe. Targeted check — runs previously failed and coverage-relevant tests with per-worker partitioning (--no-self-parallel) so the same test never runs concurrently on multiple workers, avoiding conflicts on shared resources (hardcoded users, roles, settings profiles, etc.). Both use --test-runs 50. Changes: - Add --no-self-parallel flag to clickhouse-test: partitions tests by name hash so all copies go to the same worker - Flaky check: only changed tests, thread fuzzer, shared queue - Targeted check: relevant tests, no thread fuzzer, per-worker partitioning, 50 runs, single clickhouse-test invocation - Restore original thread fuzzer parameters - Change targeted check to arm_asan_ubsan on ARM_LARGE PRs affected by tests running in parallel with themselves: #102015 #101093 #99653 #101503 #101108 #101884 #100746 #101447 #100941 #101099 #100203 #96130
Resolve conflicts in DatabaseFactory and per-engine registrations caused by the new `is_external` field added on master and the `source_access_type` field added in this branch. Both fields coexist; `is_external` is used by restore, `source_access_type` by `addImplicitAccessRights`.
Co-authored-by: clickhouse-gh[bot] <173705893+clickhouse-gh[bot]@users.noreply.github.com>
|
@pufit follow-up on the CI status for the current commit (
Will ping again only if anything substantive surfaces once the in-flight checks complete. |
| std::unordered_map<std::string, AccessType> result; | ||
| /// Use DB::toString() to build keys so they match what replaceDeprecated() stores | ||
| /// (DB::toString replaces underscores with spaces, e.g. ARROW_FLIGHT -> "ARROW FLIGHT"). | ||
| #define ADD_BACKWARD_COMPAT_SOURCE(name, alias) result.emplace(DB::toString(AccessType::name), AccessType::name); |
There was a problem hiding this comment.
makeBackwardCompatible now builds string_to_accessType keys with DB::toString(AccessType::name). For ARROW_FLIGHT that produces "ARROW FLIGHT", but source parameters are normalized via AccessTypeObjects::unifySource to "ARROW_FLIGHT" (underscore) in AccessFlags::validateParameter.
So with enable_read_write_grants = false, READ/WRITE ON ARROW_FLIGHT will not map back to the deprecated ARROW_FLIGHT access type, and backward-compatible formatting/replication output stays in the new form.
Please include the canonical source name (#name / toStringSource(Source::name)) in this map (or both forms) so ARROW_FLIGHT is converted correctly.
`makeBackwardCompatible` built map keys only from `DB::toString(AccessType::name)` (spaces) but parsing modern `GRANT READ ON ARROW_FLIGHT` stores the parameter in canonical `#name` form via `unifySource`. Lookup missed for ARROW_FLIGHT — the only source whose name has an underscore — so the grant was not converted back to the deprecated form when `enable_read_write_grants=false`. Insert both forms.
LLVM Coverage ReportChanged lines: 99.01% (100/101) · Uncovered code |
robot-ch-test-poll
added
the
pr-synced-to-cloud
6 participants
Re-introduces the dynamic engine lookup from #98984 (reverted in #100684) with a fix for the regression that broke
CREATE DATABASE.The original PR replaced the hardcoded
source_table_engineslist withStorageFactory::getAllStorages(), which correctly covers all table engines. However,CREATE DATABASEalso checksTABLE_ENGINEaccess inInterpreterCreateQuery, and database engines (Atomic,Shared,Replicated, etc.) are registered inDatabaseFactory, notStorageFactory. Whentable_engines_require_grant=false, the old wildcardgrant(TABLE_ENGINE)covered everything implicitly — the new per-engine approach missed database engines entirely, causingACCESS_DENIEDonCREATE DATABASE.What changed
StorageFactory loop (from #98984): replaces the hardcoded
source_table_engineslist with runtimeStorageFactory::getAllStorages(). Each table engine declares itssource_access_typeat registration, so the implicit grants are always in sync. Uses grant-only (no revoke) to preserveWITH GRANT OPTIONstate (#71544).DatabaseFactory:
source_access_type(new): addsstd::optional<AccessTypeObjects::Source> source_access_typetoDatabaseFactory::EngineFeatures, mirroringStorageFactory::StorageFeatures. Source database engines now explicitly declare their source dependency:source_access_typeMYSQLPOSTGRESPOSTGRESSQLITES3HDFSFILENon-source engines (Atomic, Ordinary, Memory, Dictionary, Replicated, Backup) default to
std::nullopt— granted implicitly whentable_engines_require_grant=false, no change in behavior.DataLakeCatalogis left withoutsource_access_type(polymorphic — underlying source depends on runtime catalog type). Noted as TODO.DatabaseFactory loop in
addImplicitAccessRights: now mirrors the StorageFactory loop exactly — iteratesgetDatabaseEngines(), checkssource_access_type, bridgesREAD/WRITE→TABLE_ENGINEfor source engines regardless of thetable_engines_require_grantsetting. The previous implementation relied on name collisions between the two factories (correct by coincidence, not by design).Also carried forward from #98984:
AccessRightsElement.cppwithAPPLY_FOR_SOURCEmacroYTSAURUSandARROW_FLIGHTsource access typesWITH GRANT OPTIONstate (RBAC:GRANT TABLE ENGINE ON *fails with table_engines_require_grant=false #71544)Changelog category (leave one):
Changelog entry:
Replace the hardcoded
source_table_engineslist with runtime lookup viaStorageFactoryandDatabaseFactory. Addsource_access_typetoDatabaseFactory::EngineFeaturesso thatCREATE DATABASEwith source engines (PostgreSQL, MySQL, S3, etc.) requires the same source grants asCREATE TABLE. FixesGRANT TABLE ENGINE ON *failing withtable_engines_require_grant=false. Closes #71544Documentation entry for user-facing changes
Version info
26.5.1.302