[#10684] feat(iceberg-rest): Support vended credentials on registerTable endpoint by sachinnn99 · Pull Request #10699 · apache/gravitino

sachinnn99 · 2026-04-07T07:22:07Z

Resubmitting per @laserninja's comment on #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: #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.

roryqi · 2026-04-07T07:35:24Z

@laserninja @bharos Could u help me review this pull request/

bharos · 2026-04-07T16:09:21Z

+            "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));


assertFalse ?

Fixed — replaced assertTrue(!...) with assertFalse(...).

bharos · 2026-04-07T16:10:04Z

  }

-  private boolean isCredentialVending(String accessDelegation) {
+  static boolean isCredentialVending(String accessDelegation) {


We are making this package-private
Should we annotate with @VisibleForTesting

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.

laserninja · 2026-04-07T21:14:38Z

+      return injectCredentialConfig(
+          TableIdentifier.of(namespace, request.name()),
+          loadTableResponse,
+          CredentialPrivilege.WRITE);


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.

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 existing metadata.metadataFileLocation() without writing, then JdbcTableOperations just inserts the catalog row. So the storage-layer asymmetry you flagged with createTable is real — createTable proves write capability by writing a metadata.json file, registerTable doesn't.

registerTable also doesn't set ownership. IcebergNamespaceOperationExecutor.registerTable is a straight delegation to the wrapper — unlike IcebergTableOperationExecutor.createTable and IcebergNamespaceOperationExecutor.createNamespace, which both explicitly set IcebergConstants.OWNER to context.userName(). So a user with only ANY_CREATE_TABLE privilege 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-up loadTable with vending would return READ for them via getCredentialPrivilege. 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.

laserninja · 2026-04-07T21:16:24Z

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.

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.)

laserninja · 2026-04-07T21:22:01Z

@@ -191,7 +204,7 @@ protected boolean useDifferentClassLoader() {
    return false;
  }



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.

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.

laserninja · 2026-04-07T21:22:09Z

@@ -239,7 +252,7 @@ private Credential getCredential(
    return credential;
  }



same as above

Same JavaDoc rationale added on shouldGenerateCredential.

laserninja · 2026-04-07T21:26:15Z

+            "register_cred_foo3", Namespace.of("register_cred_ns3"), s3Location);
+    Assertions.assertEquals(Status.OK.getStatusCode(), response.getStatus());
+    loadTableResponse = response.readEntity(LoadTableResponse.class);
+    Assertions.assertEquals(


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.

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.

laserninja · 2026-04-08T16:48:52Z

Can you resolve the merge conflicts please

sachinnn99 · 2026-04-09T06:01:33Z

@laserninja rebased onto main, conflicts resolved.

laserninja · 2026-04-14T03:55:43Z

In createTable and loadTable, CatalogWrapperForREST checks if (catalog instanceof RESTCatalog) and delegates to *Internal() methods before credential injection. The new registerTable skips this branch and calls super.registerTable() directly. Could you confirm this works correctly when the underlying catalog is a RESTCatalog (i.e., Gravitino fronting another Iceberg REST catalog)? If the upstream catalog already handles credential vending, the shouldGenerateCredential gate (getCatalog() instanceof RESTCatalog → return false) would short-circuit correctly, but it would be good to verify that RESTCatalog.registerTable properly forwards the X-Iceberg-Access-Delegation header upstream.

…sterTable endpoint 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, including the RESTCatalog delegation branch that forwards credential requests to the upstream catalog when Gravitino fronts another REST catalog.

roryqi requested review from bharos and laserninja April 7, 2026 07:35

bharos reviewed Apr 7, 2026

View reviewed changes

laserninja reviewed Apr 7, 2026

View reviewed changes

sachinnn99 force-pushed the feat/10684-register-table-credential-vending branch from 5c891db to ba22fe6 Compare April 9, 2026 06:01

roryqi requested review from bharos and laserninja April 14, 2026 03:42

sachinnn99 force-pushed the feat/10684-register-table-credential-vending branch from ba22fe6 to a8248a5 Compare April 15, 2026 11:48

sachinnn99 force-pushed the feat/10684-register-table-credential-vending branch from a8248a5 to 67f0f23 Compare April 15, 2026 11:52

		@@ -191,7 +204,7 @@ protected boolean useDifferentClassLoader() {
		return false;
		}

		@@ -239,7 +252,7 @@ private Credential getCredential(
		return credential;
		}

Conversation

sachinnn99 commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

roryqi commented Apr 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachinnn99 Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laserninja commented Apr 8, 2026

Uh oh!

sachinnn99 commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laserninja commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sachinnn99 commented Apr 7, 2026 •

edited

Loading

sachinnn99 Apr 8, 2026 •

edited

Loading

sachinnn99 commented Apr 9, 2026 •

edited

Loading