Repository navigation
[Enhancement] (nereids)implement alterCatalogPropertiesCommand in ner… - #45164
Merged
Merged
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
|
please refer to #45160 |
msridhar78
force-pushed
the
alter_catalog_properties
branch
2 times, most recently
from
December 20, 2024 06:22
7b3de4b to
97ee4c6
Compare
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 40041 ms |
TPC-DS: Total hot run time: 196392 ms |
ClickBench: Total hot run time: 31.73 s |
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 39651 ms |
TPC-DS: Total hot run time: 195926 ms |
ClickBench: Total hot run time: 33.12 s |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 32728 ms |
TPC-DS: Total hot run time: 197338 ms |
ClickBench: Total hot run time: 30.95 s |
|
|
||
| @Override | ||
| public void doRun(ConnectContext ctx, StmtExecutor executor) throws Exception { | ||
| PropertyAnalyzer.checkCatalogProperties(newProperties, true); |
Contributor
There was a problem hiding this comment.
I think should call super class's validate method here. And please do same for AlterCatalogCommentCommand
Contributor
Author
There was a problem hiding this comment.
done - please review
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 33054 ms |
TPC-DS: Total hot run time: 195693 ms |
ClickBench: Total hot run time: 31.23 s |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 31923 ms |
TPC-DS: Total hot run time: 194028 ms |
ClickBench: Total hot run time: 31 s |
msridhar78
force-pushed
the
alter_catalog_properties
branch
from
February 6, 2025 14:02
7d9ef95 to
750257b
Compare
Contributor
|
run buildall |
TPC-H: Total hot run time: 31329 ms |
TPC-DS: Total hot run time: 191486 ms |
ClickBench: Total hot run time: 31.09 s |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 31577 ms |
TPC-DS: Total hot run time: 192118 ms |
ClickBench: Total hot run time: 31.43 s |
starocean999
approved these changes
Mar 4, 2025
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
LiBinfeng-01
approved these changes
Mar 4, 2025
koarz
pushed a commit
to koarz/doris
that referenced
this pull request
Jun 4, 2025
…eids (apache#45164) Issue Number: close apache#42789
morningman
added a commit
to morningman/doris
that referenced
this pull request
Nov 6, 2026
…udit text ### Background SHOW CREATE CATALOG, SHOW CATALOG, the `catalogs()` table function and the audit text of CREATE CATALOG replace a property value with `*XXX` when `DatasourcePrintableMap.isSensitiveKey` matches its key. Until now the keys came from a list in fe-core and from the keys filesystem plugins report at startup. A connector plugin marks its secret properties `sensitive = true`, but that mark only masks the connector's own log lines. A connector tells the engine about its security-relevant properties through `ConnectorProvider#securitySurface()`. ### What problem does this PR solve? Issue Number: None Related PR: apache#69588 (added the connector security surface this extends), apache#45164 (introduced the unmasked ALTER CATALOG audit text), apache#27115 (masked it for the statement apache#45164 replaced), apache#59502 (added `iceberg.jdbc.password`), apache#54523 (added the Paimon REST DLF keys), apache#65730 (branch-4.1's Lance catalog, whose REST secrets the Lance port will declare on this surface) PR Type: Bugfix Problem Summary: Catalog secrets that only a connector knows are on no masking list, so SHOW CREATE CATALOG and the other renderings print them in clear: `iceberg.jdbc.password`, `paimon.jdbc.password`, the Paimon REST DLF key pair, `mc.access_key`, and credentials passed through under `trino.` or `adbc.`. Separately, `ALTER CATALOG ... SET PROPERTIES` is written to the audit log unmasked, whatever the key. Each connector now declares its secrets on its security surface, taken from its `sensitive` marks, and the surface can also declare a namespace of secret keys for options passed through unread; Fluss's lake options were hard-coded in fe-core for that reason. The engine masks the union, with fe-core's own list kept as the floor. ALTER CATALOG is masked like CREATE CATALOG. ### Details - **Change map:** 42 files, +1035 −76 over two commits: production code +376 −48, tests +645 −24, the connector README +13 −3, the plugin API version in `fe/fe-connector/pom.xml`. The first commit adds secret keys to the surface (reviewed alone before). The second does the rest: 1. Key namespaces on the SPI: `withSensitiveKeyPrefix` and `SensitiveKeyPrefix.covers`, which holds the matching rule; a collection form of `withSensitiveKeys` (`ConnectorSecuritySurface`, +130). 2. Engine: `isSensitiveKey` matches registered namespaces, the Fluss rule becomes one of them; `ConnectorPluginManager` registers them and reads every surface before it publishes a provider (+70). 3. Connectors: every provider declares its secrets; `MetaStoreProviders.sensitivePropertyKeys()` gives the union of a plugin's metastore backends; `IcebergJdbcMetaStoreProvider` reports its keys like its siblings; the keytab paths lose their `sensitive` mark (+140). 4. ALTER CATALOG audit masking (+16). - **Frequency:** Always. Create or alter a catalog that sets one of the keys above, then run SHOW CREATE CATALOG, SHOW CATALOG `<name>` or `select * from catalogs()`, or read the audit log after an ALTER CATALOG ... SET PROPERTIES. - **Root cause:** `isSensitiveKey` consulted only fe-core's list and the filesystem providers' keys. A connector's `sensitive` marks fed only `toMaskedString`; `MetaStoreProvider.sensitivePropertyKeys()` existed for this, but nothing called it, and the Iceberg JDBC backend did not implement it. For ALTER CATALOG, `AlterCatalogPropertiesCommand` does not implement `NeedAuditEncryption` and `LogicalPlanBuilderForEncryption` has no `visitAlterCatalogProperties`, so `AuditLogHelper` records the original text; the statement it replaced in apache#45164 was masked by apache#27115. - **Why existing tests missed it:** Unit tests: `DatasourcePrintableMapTest` checks fe-core's own keys, the provider surface tests checked only the SSRF and driver_url keys, and `EncryptSQLTest` and `RepositoryAuditEncryptionTest` cover CREATE CATALOG, repositories and resources but not ALTER CATALOG. Regression: `audit/test_external_catalog_credential_masking` creates only Iceberg REST catalogs, whose keys are on fe-core's list, and never alters one. - **Fix:** The engine must not name a connector's properties, so the connectors declare their secrets, from the same `sensitive` marks that mask their logs; jdbc and es declare both spellings their legacy prefix allows, and trino and adbc declare the namespace they pass through, a key there being a secret when its rest names `key`, `secret`, `password`, `token` or `credential` (also `authorization` for ADBC). fe-core keeps the connector keys it lists, so a catalog whose plugin is not loaded masks what it masked before. A provider whose `securitySurface()` throws at startup is skipped, as a filesystem provider is, instead of stopping FE. - **Other paths and branches:** Fixed here: every rendering above and FE's statement logging, which share `isSensitiveKey` and `NeedAuditEncryption`. Not covered, since a key name cannot express them safely: raw Iceberg SDK keys copied through (`token`, `credential`), Paimon options re-keyed from `paimon.*`, Hadoop `fs.*` keys, and secrets inside a URL. The ALTER CATALOG gap is on branch-4.0, 4.1 and 4.2 (from apache#45164); branch-3.1 still uses the masked statement. branch-4.1 and 4.2 also print the connector keys above in clear; without the connector SPI their fix differs and is tracked separately. - **Behavior change:** The namespaces also mask non-secrets whose names carry those words, such as `trino.hive.s3.aws-access-key` or a keystore path. Keytab paths, which no release masks, are no longer masked in connector log lines: marked `sensitive`, `hadoop.kerberos.keytab` would be masked in every storage vault and resource. ### Testing All runs were local (macOS), on master `f5c993f2c94` plus this change: - Regression `audit/test_external_catalog_credential_masking`, extended with an Iceberg JDBC catalog: SHOW CREATE CATALOG masks `iceberg.jdbc.password`, and the audit texts of its CREATE CATALOG and of an ALTER CATALOG that sets the password and `s3.secret_key` hold only `*XXX`. Against the local cluster's FE without this change it fails at the first new check, SHOW CREATE CATALOG printing the password; with a FE built from this change it passes. - fe-connector-spi: all 160 tests, with the refreshed surface baseline (+4 lines); the metastore modules: all tests; ten connector modules: 29 classes, 511 tests, including each connector's surface test, which checks real key names. - fe-core: 26 classes, 341 tests, including `ConnectorPluginManagerTest`, `DatasourcePrintableMapTest`, `EncryptSQLTest` and the audit and plugin gate tests; 14 of them again after the last change to registration. - Twenty mutations, each caught by its test: for example, reverting either half of the ALTER CATALOG fix (`EncryptSQLTest` sees the statement in clear), matching a marker anywhere in the key, an empty metastore union, or re-marking a keytab. ### Notes for Reviewers - `SensitiveKeyPrefix.covers` lives on the SPI so that a connector's test can check its declaration against real key names and the engine applies the same rule; the Fluss floor in fe-core uses it too. - The entries both fe-core and a connector declare are deliberate: the floor is frozen, as the comment in `DatasourcePrintableMap` says. - The plugin API stays at 31.0, the first commit's bump. Hudi declares nothing: it owns no catalog properties. ### Release note SHOW CREATE CATALOG, SHOW CATALOG, the `catalogs()` table function and the audit log now mask `iceberg.jdbc.password`, `paimon.jdbc.password`, `paimon.rest.dlf.access-key-id`, `paimon.rest.dlf.access-key-secret`, `mc.access_key`, and the `trino.*` and `adbc.*` properties whose names carry `key`, `secret`, `password`, `token` or `credential` (`authorization` too for `adbc.*`). ALTER CATALOG ... SET PROPERTIES statements are masked in the audit log like CREATE CATALOG. ### Check List (For Author) - Behavior changed: Yes (see Details) - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the code, the tests, and this description - Human review: not stated - Prompts: The author first asked for a generic connector SPI to mask a connector's secret catalog properties instead of adding the Lance keys to fe-core's list. Then, in this PR: let the surface express prefix rules such as Fluss's, mask the union of the engine's and the connectors' keys, wire every connector, keep fe-core's existing entries as the floor, mask whatever is marked `sensitive` but stop marking keytab paths, and fix the unmasked ALTER CATALOG audit text on master only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…eids
What problem does this PR solve?
Issue Number: close #42789
Related PR: #xxx
Problem Summary:
Enhancement] (nereids)implement alterCatalogPropertiesCommand in nereids
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)