Copilot commented on code in PR #11294:
URL: https://github.com/apache/gravitino/pull/11294#discussion_r3346859144
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -692,6 +759,13 @@ private LoadTableResponse createTableInternal(Namespace
namespace, CreateTableRe
properties, key ->
catalogPropertiesToClientKeys.contains(key)))
// Keep only credential fields from FileIO properties before
returning them to the client.
.addAllConfig(CredentialPropertyUtils.filterCredentialProperties(properties))
+ .addAllConfig(
+ CredentialPropertyUtils.buildRefreshCredentialEndpoints(
+
RESTUtil.encodeString(catalogCredentialManager.catalogName()),
+ RESTUtil.encodeNamespace(
+ ident.namespace(),
IcebergRESTUtils.NAMESPACE_SEPARATOR_URLENCODED_UTF_8),
+ RESTUtil.encodeString(ident.name()),
+
CredentialPropertyUtils.filterCredentialProperties(properties)))
Review Comment:
The credential properties are filtered twice (lines 762 and 769), which
duplicates work and slightly increases allocation churn. Consider storing
`CredentialPropertyUtils.filterCredentialProperties(properties)` in a local
variable and reusing it for both `.addAllConfig(...)` calls (same pattern also
appears in the stage/create and load paths).
##########
api/src/main/java/org/apache/gravitino/credential/AwsIrsaCredential.java:
##########
@@ -118,12 +118,15 @@ public String sessionToken() {
return sessionToken;
}
- private void validate(String accessKeyId, String secretAccessKey, String
sessionToken) {
+ private void validate(
+ String accessKeyId, String secretAccessKey, String sessionToken, long
expireTimeInMs) {
Preconditions.checkArgument(
StringUtils.isNotBlank(accessKeyId), "Access key Id should not be
empty");
Preconditions.checkArgument(
StringUtils.isNotBlank(secretAccessKey), "Secret access key should not
be empty");
Preconditions.checkArgument(
StringUtils.isNotBlank(sessionToken), "Session token should not be
empty");
+ Preconditions.checkArgument(
+ expireTimeInMs > 0, "The expire time of AwsIrsaCredential should great
than 0");
Review Comment:
The error message has a grammatical issue: “should great than 0” should be
“should be greater than 0” (and consider “expiration time” instead of “expire
time” for clarity).
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -192,38 +201,92 @@ public LoadTableResponse updateTable(
/**
* Get table credentials.
*
- * @param identifier The table identifier for which to load credentials
- * @return A {@link
org.apache.iceberg.rest.responses.LoadCredentialsResponse} object containing
- * the credentials.
+ * @param identifier table identifier
+ * @param privilege used for local credential vending; ignored for REST
catalog backends
+ * @return table credentials response
*/
public LoadCredentialsResponse getTableCredentials(
TableIdentifier identifier, CredentialPrivilege privilege) {
+ if (isRESTCatalog()) {
+ return getRESTTableCredentials((RESTCatalog) getCatalog(), identifier);
+ } else {
+ return getLocalTableCredentials(identifier, privilege);
+ }
+ }
+
+ private LoadCredentialsResponse getLocalTableCredentials(
+ TableIdentifier identifier, CredentialPrivilege privilege) {
try {
LoadTableResponse loadTableResponse = super.loadTable(identifier);
- Credential credential = getCredential(loadTableResponse, privilege);
- org.apache.iceberg.rest.credentials.Credential icebergCredential =
- new org.apache.iceberg.rest.credentials.Credential() {
- @Override
- public String prefix() {
- return "";
- }
-
- @Override
- public Map<String, String> config() {
- // Convert Gravitino credentials to the Iceberg REST credential
payload format.
- return CredentialPropertyUtils.toIcebergProperties(credential);
- }
-
- @Override
- public void validate() {}
- };
- return
ImmutableLoadCredentialsResponse.builder().addCredentials(icebergCredential).build();
+ Credential credential = getCredential(loadTableResponse.tableMetadata(),
privilege);
+ return ImmutableLoadCredentialsResponse.builder()
+ .addCredentials(
+ IcebergRESTUtils.toRESTCredential(
+ catalogCredentialManager.catalogName(),
+ identifier,
+ credential,
+ loadTableResponse.tableMetadata()))
+ .build();
} catch (ServiceUnavailableException e) {
LOG.warn("Service unavailable when loading table credentials for table:
{}", identifier, e);
return ImmutableLoadCredentialsResponse.builder().build();
}
}
+ private static LoadCredentialsResponse getRESTTableCredentials(
+ RESTCatalog restCatalog, TableIdentifier identifier) {
+ Map<String, String> properties = Maps.newHashMap(restCatalog.properties());
+ String credentialsPath =
+ ResourcePaths.forCatalogProperties(properties).table(identifier) +
"/credentials";
+
+ AuthManager authManager = null;
+ RESTClient client = null;
+ AuthSession authSession = null;
+ try {
+ authManager = AuthManagers.loadAuthManager(restCatalog.name(),
properties);
+ client =
+ HTTPClient.builder(properties)
+ .uri(properties.get(CatalogProperties.URI))
+ .withHeaders(RESTUtil.configHeaders(properties))
+ .build();
+ authSession = authManager.catalogSession(client, properties);
+ return client
+ .withAuthSession(authSession)
+ .get(
+ credentialsPath,
+ LoadCredentialsResponse.class,
+ Collections.emptyMap(),
+ ErrorHandlers.tableErrorHandler());
+ } finally {
+ if (authSession != null) {
+ try {
+ authSession.close();
+ } catch (Exception e) {
+ LOG.warn(
+ "Failed to close auth session when loading credentials for
table: {}", identifier, e);
+ }
+ }
+
+ if (client != null) {
+ try {
+ client.close();
+ } catch (Exception e) {
+ LOG.warn(
+ "Failed to close REST client when loading credentials for table:
{}", identifier, e);
+ }
+ }
+
+ if (authManager != null) {
+ try {
+ authManager.close();
+ } catch (Exception e) {
+ LOG.warn(
+ "Failed to close auth manager when loading credentials for
table: {}", identifier, e);
+ }
+ }
+ }
Review Comment:
This manual close logic is fairly verbose and easy to regress. If
`AuthManager`, `RESTClient`, and `AuthSession` are `AutoCloseable` (they appear
to be), consider rewriting this using nested try-with-resources to reduce
boilerplate and make the close order explicit.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestCatalogWrapperForREST.java:
##########
@@ -101,6 +118,292 @@ void testIsLocalOrHdfsLocation() {
Assertions.assertFalse(CatalogWrapperForREST.isLocalOrHdfsLocation(" "));
}
+ @Test
+ void testRESTTableCredentials() throws Exception {
+ TableIdentifier table = TableIdentifier.of(Namespace.of("db"), "tbl");
+ String expectedPath = "/v1/upstream/namespaces/db/tables/tbl/credentials";
+ String upstreamJson =
+
"{\"storage-credentials\":[{\"prefix\":\"s3://upstream/db/tbl/\",\"config\":{"
+ + "\"s3.access-key-id\":\"upstream-key\","
+ + "\"s3.secret-access-key\":\"upstream-secret\","
+ + "\"s3.session-token\":\"upstream-token\"}}]}";
+
+ AtomicReference<String> requestPath = new AtomicReference<>();
+ HttpServer server = HttpServer.create(new InetSocketAddress(0), 0);
+ server.createContext(
+ "/",
+ exchange -> {
+ requestPath.set(exchange.getRequestURI().getPath());
+ byte[] body = upstreamJson.getBytes(StandardCharsets.UTF_8);
+ exchange.getResponseHeaders().add("Content-Type",
"application/json");
+ exchange.sendResponseHeaders(200, body.length);
+ try (OutputStream os = exchange.getResponseBody()) {
+ os.write(body);
+ }
+ });
Review Comment:
These tests depend on `com.sun.net.httpserver.HttpServer` (module
`jdk.httpserver`), which can be unavailable in some runtime/build
configurations (e.g., custom JRE images). To improve portability and reduce
flakiness, consider using a dedicated HTTP test server dependency (e.g.,
WireMock / OkHttp MockWebServer) for these REST-catalog interaction tests.
--
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]
