[#10684] feat(iceberg-rest): Support vended credentials on registerTable endpoint - #10699
Conversation
|
@laserninja @bharos Could u help me review this pull request/ |
| "register_cred_foo2", Namespace.of("register_cred_ns2"), "mock"); | ||
| Assertions.assertEquals(Status.OK.getStatusCode(), response.getStatus()); | ||
| LoadTableResponse loadTableResponse = response.readEntity(LoadTableResponse.class); | ||
| Assertions.assertTrue(!loadTableResponse.config().containsKey(Credential.CREDENTIAL_TYPE)); |
There was a problem hiding this comment.
Fixed — replaced assertTrue(!...) with assertFalse(...).
| } | ||
|
|
||
| private boolean isCredentialVending(String accessDelegation) { | ||
| static boolean isCredentialVending(String accessDelegation) { |
There was a problem hiding this comment.
We are making this package-private
Should we annotate with @VisibleForTesting
There was a problem hiding this comment.
Good catch on the visibility change being undocumented, but the change is actually for production reuse rather than testing — IcebergNamespaceOperations#registerTable (in the same package) now calls this to parse the same X-Iceberg-Access-Delegation header that IcebergTableOperations#createTable and #loadTable already parse. @VisibleForTesting would be misleading because no test depends on its visibility. I've added a JavaDoc explaining the production-reuse rationale instead.
| return injectCredentialConfig( | ||
| TableIdentifier.of(namespace, request.name()), | ||
| loadTableResponse, | ||
| CredentialPrivilege.WRITE); |
There was a problem hiding this comment.
createTable hardcodes WRITE — that makes sense since the client just created the table and likely needs to write data to it. But registerTable is semantically different: the table data already exists and the client is just adding it to the catalog. The typical next action after registration is reading the data. Meanwhile, loadTable accepts the privilege as a parameter and delegates the decision to the caller.
Should registerTable similarly accept/infer the privilege, or is the WRITE default intentional here? At minimum, a brief comment explaining the choice would help future readers.
There was a problem hiding this comment.
You're right, and after digging in I'm switching to READ.
Specifically:
-
registerTable writes no files at the storage layer. Verified end-to-end:
CatalogHandlers.registerTable→BaseMetastoreCatalog.registerTable→ops.commit(null, metadata)→BaseMetastoreTableOperations.writeNewMetadataIfRequired(true, metadata)early-returns the existingmetadata.metadataFileLocation()without writing, thenJdbcTableOperationsjust inserts the catalog row. So the storage-layer asymmetry you flagged withcreateTableis real —createTableproves write capability by writing a metadata.json file,registerTabledoesn't. -
registerTable also doesn't set ownership.
IcebergNamespaceOperationExecutor.registerTableis a straight delegation to the wrapper — unlikeIcebergTableOperationExecutor.createTableandIcebergNamespaceOperationExecutor.createNamespace, which both explicitly setIcebergConstants.OWNERtocontext.userName(). So a user with onlyANY_CREATE_TABLEprivilege never becomes the table owner via register. -
WRITE was therefore a one-off privilege escalation. That same user cannot pass
FILTER_MODIFY_TABLE_AUTHORIZATION_EXPRESSION(ANY(OWNER, METALAKE, CATALOG, SCHEMA, TABLE) || ANY_MODIFY_TABLE) on the registered table via any other API path. A follow-uploadTablewith vending would return READ for them viagetCredentialPrivilege. Hardcoding WRITE here was vending storage credentials that let them perform actions gravitino's authz model explicitly forbids.
Switching to READ. Higher-privileged users (catalog/metalake owners, MODIFY_TABLE holders) who legitimately need to write to a registered table can call loadTable with vending afterwards and get WRITE through the dynamic helper, or be granted MODIFY_TABLE explicitly.
I've added a code comment documenting all of this.
There was a problem hiding this comment.
We set the table owner when registering the table. So the register table requester should have write privilege. It will be consistent with our authz model. WDYT?
There was a problem hiding this comment.
@roryqi Good point, you are right. IcebergNamespaceHookDispatcher.registerTable calls setTableOwner for the registering user, so the user does become the table owner. This is consistent with createTable, which also vends WRITE for the same reason.
Updated to vend WRITE and corrected the comment.
There was a problem hiding this comment.
The PR's new 3-arg registerTable override in CatalogWrapperForTest duplicates the shouldGenerateCredential/injectCredentialConfig pattern from the parent CatalogWrapperForREST. This duplication is necessary because the in-memory catalog can't handle registerTable natively (unlike createTable, where the test calls super.createTable(3-arg) and the parent handles everything).
However, if the credential injection logic ever changes in CatalogWrapperForREST, this test would silently diverge. Consider adding a brief comment in the test explaining why the duplication exists, e.g.:
// Cannot delegate to super.registerTable(3-arg) because the in-memory catalog
// does not support registerTable; must inline credential injection here.There was a problem hiding this comment.
Added an inline comment at the call site explaining the duplication: the in-memory test catalog cannot perform registerTable natively, so the override synthesizes a mock LoadTableResponse instead of delegating to super.registerTable. Because we never go through the parent's registerTable, we must explicitly re-run its vending logic so credential-vending tests still exercise the same code path as production. (Also updated this wrapper to use READ to match the production change.)
| @@ -191,7 +204,7 @@ | |||
| return false; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
These methods go from private → protected, which makes them part of the subclass API contract. This is the cleanest option given the constraints but will need to be maintained as a stable interface going forward. A short JavaDoc note on their intended use (subclass credential injection in cases where super can't be called) would clarify intent.
There was a problem hiding this comment.
Added JavaDoc explaining that the protected visibility is for subclasses that synthesize a LoadTableResponse without delegating to super (test wrappers around in-memory catalogs that can't perform certain operations natively), so they can still apply the same vending and injection logic as production paths.
| @@ -239,7 +252,7 @@ | |||
| return credential; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Same JavaDoc rationale added on shouldGenerateCredential.
| "register_cred_foo3", Namespace.of("register_cred_ns3"), s3Location); | ||
| Assertions.assertEquals(Status.OK.getStatusCode(), response.getStatus()); | ||
| loadTableResponse = response.readEntity(LoadTableResponse.class); | ||
| Assertions.assertEquals( |
There was a problem hiding this comment.
The S3 vending case only checks for Credential.CREDENTIAL_TYPE in the config.
Asserting on a couple more expected credential properties (e.g., whatever DummyCredentialProvider puts in the config) would make the test more robust against regressions where the type is present but the actual credential values are missing.
There was a problem hiding this comment.
Added an additional assertion on Credential.EXPIRE_TIME_IN_MS. DummyCredentialProvider.SimpleCredential is not one of the typed credentials in CredentialPropertyUtils.toIcebergProperties, so it falls through to Credential#toProperties(), which emits exactly credential-type and expire-time-in-ms. Asserting both confirms the full credential block is injected and guards against partial-injection regressions.
|
Can you resolve the merge conflicts please |
5c891db to
ba22fe6
Compare
|
@laserninja rebased onto main, conflicts resolved. |
|
In |
a8248a5 to
67f0f23
Compare
|
@laserninja Good question — the current code does include the When the underlying catalog is a The upstream Iceberg |
|
LGTM. READ privilege is well-reasoned, RESTCatalog path added, JavaDoc and test assertions look good. Thanks for the thorough responses. |
67f0f23 to
dc1b30d
Compare
…sterTable endpoint Wire the X-Iceberg-Access-Delegation header through the registerTable REST endpoint so that clients can request credential vending, matching the existing createTable and loadTable behavior. Vend WRITE credentials because the registering user becomes the table owner (IcebergNamespaceHookDispatcher.setTableOwner), consistent with createTable.
dc1b30d to
ef91041
Compare
|
@roryqi Updated to vend WRITE per your feedback — the registering user becomes the table owner via |
Code Coverage Report
Files
|
|
Thanks @sachinnn99 for your contribution. Thanks @laserninja @laserninja for your review. |
…sterTable endpoint (apache#10699) Resubmitting per @laserninja's comment on apache#10684. This revision aligns the implementation with the established `createTable`/`loadTable` credential-vending pattern in `CatalogWrapperForREST`: the new 3-arg `registerTable` calls `super.registerTable(2-arg)` and uses the same inline `shouldGenerateCredential` / `injectCredentialConfig` conditional that `createTable` and `loadTable` already use, and `CatalogWrapperForTest` now overrides at the **3-arg** level (matching how it overrides `createTable`). ### What changes were proposed in this pull request? Add `X-Iceberg-Access-Delegation` header support to the `registerTable` endpoint, enabling credential vending in the response. Mirrors the existing pattern from `createTable` and `loadTable`. Changes: - `IcebergNamespaceOperations.registerTable`: add `@HeaderParam(X_ICEBERG_ACCESS_DELEGATION)`, compute `isCredentialVending`, build the 3-arg `IcebergRequestContext` - `CatalogWrapperForREST`: add 3-arg `registerTable` that calls `super.registerTable(2-arg)` then conditionally injects credentials inline — structurally identical to the existing 3-arg `createTable` and `loadTable` - `CatalogWrapperForREST`: widen `shouldGenerateCredential` and `injectCredentialConfig` from `private` to `protected` so subclasses (e.g. test wrappers that mock the underlying table operation) can participate in the credential vending flow - `IcebergNamespaceOperationExecutor.registerTable`: pass `context.requestCredentialVending()` through to the 3-arg wrapper form - `IcebergTableOperations.isCredentialVending`: widen `private` → package-private `static` so `IcebergNamespaceOperations` can reuse it without duplicating the validation logic - `CatalogWrapperForTest`: override the **3-arg** `registerTable` (matching the 3-arg `createTable` override level), build the mock response, and inline the same `shouldGenerateCredential` / `injectCredentialConfig` conditional. Honors cloud URIs in `metadataLocation` so vending tests can verify the vended path. The legacy 2-arg override is removed (no longer reachable via the dispatcher chain). ### Why are the changes needed? The Iceberg REST spec defines `X-Iceberg-Access-Delegation` as a valid header on `registerTable`, but the current implementation does not accept it or vend credentials. Clients that register a table and immediately attempt to read its data must make a separate `loadTable` call to obtain credentials. Fix: apache#10684 ### Does this PR introduce _any_ user-facing change? Yes. The `registerTable` REST endpoint now accepts the `X-Iceberg-Access-Delegation` header and returns vended credentials in the response config when requested. Backward compatible — clients that do not send the header get existing behavior. ### How was this patch tested? Added unit tests in `TestIcebergNamespaceOperations`: - `testRegisterTableWithCredentialVending` — verifies no vending without header, no vending for local URI, vending for `s3://` URI - `testRegisterTableRemoteSigningNotSupported` — verifies 406 response for `remote-signing` - `testRegisterTableInvalidAccessDelegation` — verifies 400 response for invalid header values All existing `TestIcebergNamespaceOperations` tests still pass with no regressions.
…sterTable endpoint (apache#10699) Resubmitting per @laserninja's comment on apache#10684. This revision aligns the implementation with the established `createTable`/`loadTable` credential-vending pattern in `CatalogWrapperForREST`: the new 3-arg `registerTable` calls `super.registerTable(2-arg)` and uses the same inline `shouldGenerateCredential` / `injectCredentialConfig` conditional that `createTable` and `loadTable` already use, and `CatalogWrapperForTest` now overrides at the **3-arg** level (matching how it overrides `createTable`). ### What changes were proposed in this pull request? Add `X-Iceberg-Access-Delegation` header support to the `registerTable` endpoint, enabling credential vending in the response. Mirrors the existing pattern from `createTable` and `loadTable`. Changes: - `IcebergNamespaceOperations.registerTable`: add `@HeaderParam(X_ICEBERG_ACCESS_DELEGATION)`, compute `isCredentialVending`, build the 3-arg `IcebergRequestContext` - `CatalogWrapperForREST`: add 3-arg `registerTable` that calls `super.registerTable(2-arg)` then conditionally injects credentials inline — structurally identical to the existing 3-arg `createTable` and `loadTable` - `CatalogWrapperForREST`: widen `shouldGenerateCredential` and `injectCredentialConfig` from `private` to `protected` so subclasses (e.g. test wrappers that mock the underlying table operation) can participate in the credential vending flow - `IcebergNamespaceOperationExecutor.registerTable`: pass `context.requestCredentialVending()` through to the 3-arg wrapper form - `IcebergTableOperations.isCredentialVending`: widen `private` → package-private `static` so `IcebergNamespaceOperations` can reuse it without duplicating the validation logic - `CatalogWrapperForTest`: override the **3-arg** `registerTable` (matching the 3-arg `createTable` override level), build the mock response, and inline the same `shouldGenerateCredential` / `injectCredentialConfig` conditional. Honors cloud URIs in `metadataLocation` so vending tests can verify the vended path. The legacy 2-arg override is removed (no longer reachable via the dispatcher chain). ### Why are the changes needed? The Iceberg REST spec defines `X-Iceberg-Access-Delegation` as a valid header on `registerTable`, but the current implementation does not accept it or vend credentials. Clients that register a table and immediately attempt to read its data must make a separate `loadTable` call to obtain credentials. Fix: apache#10684 ### Does this PR introduce _any_ user-facing change? Yes. The `registerTable` REST endpoint now accepts the `X-Iceberg-Access-Delegation` header and returns vended credentials in the response config when requested. Backward compatible — clients that do not send the header get existing behavior. ### How was this patch tested? Added unit tests in `TestIcebergNamespaceOperations`: - `testRegisterTableWithCredentialVending` — verifies no vending without header, no vending for local URI, vending for `s3://` URI - `testRegisterTableRemoteSigningNotSupported` — verifies 406 response for `remote-signing` - `testRegisterTableInvalidAccessDelegation` — verifies 400 response for invalid header values All existing `TestIcebergNamespaceOperations` tests still pass with no regressions.
4 participants
Resubmitting per @laserninja's comment on #10684. This revision aligns the implementation with the established
createTable/loadTablecredential-vending pattern inCatalogWrapperForREST: the new 3-argregisterTablecallssuper.registerTable(2-arg)and uses the same inlineshouldGenerateCredential/injectCredentialConfigconditional thatcreateTableandloadTablealready use, andCatalogWrapperForTestnow overrides at the 3-arg level (matching how it overridescreateTable).What changes were proposed in this pull request?
Add
X-Iceberg-Access-Delegationheader support to theregisterTableendpoint, enabling credential vending in the response. Mirrors the existing pattern fromcreateTableandloadTable.Changes:
IcebergNamespaceOperations.registerTable: add@HeaderParam(X_ICEBERG_ACCESS_DELEGATION), computeisCredentialVending, build the 3-argIcebergRequestContextCatalogWrapperForREST: add 3-argregisterTablethat callssuper.registerTable(2-arg)then conditionally injects credentials inline — structurally identical to the existing 3-argcreateTableandloadTableCatalogWrapperForREST: widenshouldGenerateCredentialandinjectCredentialConfigfromprivatetoprotectedso subclasses (e.g. test wrappers that mock the underlying table operation) can participate in the credential vending flowIcebergNamespaceOperationExecutor.registerTable: passcontext.requestCredentialVending()through to the 3-arg wrapper formIcebergTableOperations.isCredentialVending: widenprivate→ package-privatestaticsoIcebergNamespaceOperationscan reuse it without duplicating the validation logicCatalogWrapperForTest: override the 3-argregisterTable(matching the 3-argcreateTableoverride level), build the mock response, and inline the sameshouldGenerateCredential/injectCredentialConfigconditional. Honors cloud URIs inmetadataLocationso vending tests can verify the vended path. The legacy 2-arg override is removed (no longer reachable via the dispatcher chain).Why are the changes needed?
The Iceberg REST spec defines
X-Iceberg-Access-Delegationas a valid header onregisterTable, but the current implementation does not accept it or vend credentials. Clients that register a table and immediately attempt to read its data must make a separateloadTablecall to obtain credentials.Fix: #10684
Does this PR introduce any user-facing change?
Yes. The
registerTableREST endpoint now accepts theX-Iceberg-Access-Delegationheader and returns vended credentials in the response config when requested. Backward compatible — clients that do not send the header get existing behavior.How was this patch tested?
Added unit tests in
TestIcebergNamespaceOperations:testRegisterTableWithCredentialVending— verifies no vending without header, no vending for local URI, vending fors3://URItestRegisterTableRemoteSigningNotSupported— verifies 406 response forremote-signingtestRegisterTableInvalidAccessDelegation— verifies 400 response for invalid header valuesAll existing
TestIcebergNamespaceOperationstests still pass with no regressions.