laserninja commented on code in PR #10800:
URL: https://github.com/apache/gravitino/pull/10800#discussion_r3407487476
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java:
##########
@@ -509,27 +517,50 @@ public Response planTableScan(
@Encoded() @PathParam("namespace") @AuthorizationMetadata(type =
EntityType.SCHEMA)
String namespace,
@Encoded() @PathParam("table") @AuthorizationMetadata(type =
EntityType.TABLE) String table,
- PlanTableScanRequest scanRequest) {
+ PlanTableScanRequest scanRequest,
+ @HeaderParam(X_ICEBERG_ACCESS_DELEGATION) String accessDelegation) {
+ boolean isCredentialVending = isCredentialVending(accessDelegation);
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS = RESTUtil.decodeNamespace(namespace);
String tableName = RESTUtil.decodeString(table);
LOG.info(
- "Plan table scan, catalog: {}, namespace: {}, table: {}",
+ "Plan table scan, catalog: {}, namespace: {}, table: {}, "
+ + "accessDelegation: {}, isCredentialVending: {}",
catalogName,
icebergNS,
- tableName);
+ tableName,
+ accessDelegation,
+ isCredentialVending);
try {
return Utils.doAs(
httpRequest,
() -> {
TableIdentifier tableIdentifier = TableIdentifier.of(icebergNS,
tableName);
IcebergRequestContext context =
- new IcebergRequestContext(httpServletRequest(), catalogName);
+ new IcebergRequestContext(httpServletRequest(), catalogName,
isCredentialVending);
PlanTableScanResponse scanResponse =
tableOperationDispatcher.planTableScan(context,
tableIdentifier, scanRequest);
+ if (isCredentialVending) {
+ try {
+ LoadCredentialsResponse credentialsResponse =
+ icebergCatalogWrapperManager
+ .getCatalogWrapper(catalogName)
+ .getCredentialsIfEligible(tableIdentifier, true,
CredentialPrivilege.READ);
Review Comment:
**[HIGH] Bypasses the dispatcher/event-listener layer**
All other credential operations go through `tableOperationDispatcher` (e.g.
`getTableCredentials`), which triggers event listeners and authorization hooks.
Here the endpoint accesses `IcebergCatalogWrapperManager` directly, so:
- Credential vending on `planTableScan` won't emit events
- Authorization hooks won't fire for the credential fetch
The existing `loadTable`/`createTable` endpoints delegate credential vending
to `CatalogWrapperForREST` via the dispatcher. The same pattern should be
followed here — e.g. add a `planTableScan` overload in `CatalogWrapperForREST`
that accepts `requestCredential`, or route through a dispatcher method.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -201,6 +201,49 @@ public void validate() {}
}
}
+ /**
+ * Vend credentials for a table if eligible. Unlike {@link
#getTableCredentials}, this method
+ * respects the same vending gate ({@link #shouldGenerateCredential}) used by
+ * createTable/loadTable, so local/HDFS tables correctly return empty.
+ *
+ * <p>Returns credentials separately (not embedded in a response config map)
because {@link
+ * PlanTableScanResponse} in Iceberg 1.10.1 has no config/credentials field.
+ *
+ * @param identifier the table identifier
+ * @param requestCredential whether the client requested credential vending
+ * @param privilege the credential privilege level (READ for scans)
+ * @return credentials response, empty if the table is not eligible for
vending
+ */
+ public LoadCredentialsResponse getCredentialsIfEligible(
+ TableIdentifier identifier, boolean requestCredential,
CredentialPrivilege privilege) {
+ try {
+ LoadTableResponse loadTableResponse = super.loadTable(identifier);
Review Comment:
**[HIGH] Redundant `loadTable` call**
`planTableScan` already loads and scans the table via
`tableOperationDispatcher.planTableScan(...)`. Then `getCredentialsIfEligible`
calls `super.loadTable(identifier)` *again* — a second full table load just to
check the location and generate credentials. This is wasteful for every
credential-vended scan.
Consider either:
1. Passing the table metadata/location obtained during `planTableScan` into
the credential vending method, or
2. Caching or reusing the already-loaded table within the same request
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergTableOperations.java:
##########
@@ -1046,4 +1046,109 @@ void
testLoadTableSnapshotsAllReturnsAllSnapshots(Namespace namespace) {
allTableResponse.tableMetadata().snapshots().size(),
"Default load and snapshots=all should return the same number of
snapshots");
}
+
+ @ParameterizedTest
+
@MethodSource("org.apache.gravitino.iceberg.service.rest.IcebergRestTestUtil#testNamespaces")
+ void testPlanTableScanWithCredentialVending(Namespace namespace) {
+ verifyCreateNamespaceSucc(namespace);
+ PlanTableScanRequest emptyRequest = PlanTableScanRequest.builder().build();
+
+ // scan without credential vending -- no storage-credentials in response
+ String tableName = "scan_cred_no_header";
+ verifyCreateTableSucc(namespace, tableName);
+ Response noCredResponse = doPlanTableScan(namespace, tableName,
emptyRequest);
+ Assertions.assertEquals(Status.OK.getStatusCode(),
noCredResponse.getStatus());
+ try {
+ JsonNode noCredJson =
JsonUtil.mapper().readTree(noCredResponse.readEntity(String.class));
+ Assertions.assertFalse(
+ noCredJson.has("storage-credentials"),
+ "Response should not have storage-credentials without header");
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ // scan with credential vending on local table -- no storage-credentials
+ String localTableName = "scan_cred_local";
+ Response localCreateResponse =
+ doCreateTableWithCredentialVending(
+ namespace, localTableName, "file:///tmp/" + localTableName);
+ Assertions.assertEquals(Status.OK.getStatusCode(),
localCreateResponse.getStatus());
+ Response localScanResponse =
+ doPlanTableScanWithCredentialVending(namespace, localTableName,
emptyRequest);
+ Assertions.assertEquals(Status.OK.getStatusCode(),
localScanResponse.getStatus());
+ try {
+ JsonNode localScanJson =
+
JsonUtil.mapper().readTree(localScanResponse.readEntity(String.class));
+ Assertions.assertFalse(
+ localScanJson.has("storage-credentials"),
+ "Local table should not have storage-credentials");
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+
+ // scan with credential vending on S3 table -- should have
storage-credentials
+ String s3TableName = "scan_cred_s3";
+ String s3Location = "s3://dummy-bucket/" + s3TableName;
+ Response s3CreateResponse =
+ doCreateTableWithCredentialVending(namespace, s3TableName, s3Location);
+ Assertions.assertEquals(Status.OK.getStatusCode(),
s3CreateResponse.getStatus());
+ Response s3ScanResponse =
+ doPlanTableScanWithCredentialVending(namespace, s3TableName,
emptyRequest);
+ Assertions.assertEquals(Status.OK.getStatusCode(),
s3ScanResponse.getStatus());
+ try {
+ JsonNode s3ScanJson =
JsonUtil.mapper().readTree(s3ScanResponse.readEntity(String.class));
+ Assertions.assertTrue(
+ s3ScanJson.has("storage-credentials"), "S3 table should have
storage-credentials");
+ JsonNode credentials = s3ScanJson.get("storage-credentials");
+ Assertions.assertTrue(credentials.isArray() && credentials.size() > 0);
+ JsonNode firstCred = credentials.get(0);
Review Comment:
**[MEDIUM] Tests don't verify `prefix` or refresh properties**
The tests verify `config` contains the credential type, but don't verify:
1. The `prefix` field is set correctly — currently
`getCredentialsIfEligible` returns `""` (empty string), but the existing
`getTableCredentials`/`injectCredentialConfig` methods use the table's metadata
location as the prefix. This discrepancy should be tested.
2. Refresh properties (e.g. `client.refresh-credentials-endpoint`) are
present — `IcebergRESTUtils.toRESTCredential()` adds these but this code path
uses raw `CredentialPropertyUtils.toIcebergProperties()` which omits them.
3. The overall credential structure matches what `loadTable` with
`vended-credentials` returns for the same table, ensuring consistency.
Consider adding:
```java
Assertions.assertFalse(firstCred.get("prefix").asText().isEmpty(),
"Credential prefix should be the table location");
```
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java:
##########
@@ -623,6 +654,19 @@ private boolean isCredentialVending(String
accessDelegation) {
}
}
+ private Response buildScanResponseWithCredentials(
Review Comment:
**[MEDIUM] JSON-level merge is fragile — add a TODO with issue reference**
Manually constructing JSON via `ObjectNode`/`ArrayNode` is fine as a
temporary workaround for Iceberg 1.10.1's lack of
`PlanTableScanResponse.Builder.withCredentials()`, but:
1. Please add a `// TODO(#XXXX): Replace with
PlanTableScanResponse.Builder.withCredentials() after Iceberg 1.11.0 upgrade`
comment so this is tracked for cleanup.
2. Verify the JSON structure matches the Iceberg REST spec's
`CompletedPlanningResult` schema — specifically the `storage-credentials` array
format.
3. This also bypasses the standard `IcebergRESTUtils.ok()` serialization
path. Any future changes to response serialization (e.g. custom serializers,
response wrapping) won't apply here.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java:
##########
@@ -509,27 +517,50 @@ public Response planTableScan(
@Encoded() @PathParam("namespace") @AuthorizationMetadata(type =
EntityType.SCHEMA)
String namespace,
@Encoded() @PathParam("table") @AuthorizationMetadata(type =
EntityType.TABLE) String table,
- PlanTableScanRequest scanRequest) {
+ PlanTableScanRequest scanRequest,
+ @HeaderParam(X_ICEBERG_ACCESS_DELEGATION) String accessDelegation) {
+ boolean isCredentialVending = isCredentialVending(accessDelegation);
String catalogName = IcebergRESTUtils.getCatalogName(prefix);
Namespace icebergNS = RESTUtil.decodeNamespace(namespace);
String tableName = RESTUtil.decodeString(table);
LOG.info(
- "Plan table scan, catalog: {}, namespace: {}, table: {}",
+ "Plan table scan, catalog: {}, namespace: {}, table: {}, "
+ + "accessDelegation: {}, isCredentialVending: {}",
catalogName,
icebergNS,
- tableName);
+ tableName,
+ accessDelegation,
+ isCredentialVending);
try {
return Utils.doAs(
httpRequest,
() -> {
TableIdentifier tableIdentifier = TableIdentifier.of(icebergNS,
tableName);
IcebergRequestContext context =
- new IcebergRequestContext(httpServletRequest(), catalogName);
+ new IcebergRequestContext(httpServletRequest(), catalogName,
isCredentialVending);
PlanTableScanResponse scanResponse =
tableOperationDispatcher.planTableScan(context,
tableIdentifier, scanRequest);
+ if (isCredentialVending) {
+ try {
+ LoadCredentialsResponse credentialsResponse =
+ icebergCatalogWrapperManager
+ .getCatalogWrapper(catalogName)
+ .getCredentialsIfEligible(tableIdentifier, true,
CredentialPrivilege.READ);
+ if (!credentialsResponse.credentials().isEmpty()) {
+ return buildScanResponseWithCredentials(scanResponse,
credentialsResponse);
+ }
+ } catch (Exception e) {
Review Comment:
**[HIGH] Overly broad `catch (Exception e)` silently swallows errors**
This catches *all* exceptions including `IllegalArgumentException`,
`NullPointerException`, authorization failures, etc. A client requesting
`vended-credentials` that fails due to a misconfiguration would silently get no
credentials with only a WARN log — very hard to debug.
The existing `getTableCredentials` method only catches
`ServiceUnavailableException`. This should follow the same pattern:
```java
} catch (ServiceUnavailableException e) {
```
Other exceptions (auth failures, illegal state, NPE) should propagate so
they surface as proper error responses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
