Copilot commented on code in PR #11294:
URL: https://github.com/apache/gravitino/pull/11294#discussion_r3332332743
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -83,6 +101,63 @@ public String getValue() {
private IcebergRESTUtils() {}
+ /**
+ * Builds an Iceberg REST {@link
org.apache.iceberg.rest.credentials.Credential} for load-table,
+ * scan-plan, or credentials API responses.
+ *
+ * @param catalogName IRC catalog name used in the refresh path
+ * @param tableIdentifier table receiving the credential
+ * @param credential Gravitino credential to vend
+ * @param tableMetadata table metadata used to derive the storage prefix
+ * @return Iceberg REST credential with prefix and config
+ */
+ public static org.apache.iceberg.rest.credentials.Credential
toRestCredential(
+ String catalogName,
+ TableIdentifier tableIdentifier,
+ Credential credential,
+ TableMetadata tableMetadata) {
+ Map<String, String> config =
+ new HashMap<>(CredentialPropertyUtils.toIcebergProperties(credential));
+
+ String refreshProperty = null;
+ if (credential instanceof GCSTokenCredential) {
+ refreshProperty = GCS_OAUTH2_REFRESH_CREDENTIALS_ENDPOINT;
+ } else if (credential instanceof ADLSTokenCredential) {
+ refreshProperty = ADLS_REFRESH_CREDENTIALS_ENDPOINT;
+ } else if (credential instanceof S3TokenCredential
+ || credential instanceof AwsIrsaCredential
+ || credential instanceof OSSTokenCredential) {
+ refreshProperty = CLIENT_REFRESH_CREDENTIALS_ENDPOINT;
+ }
+
+ if (refreshProperty != null) {
+ config.put(
+ refreshProperty,
+ String.format(
+ "v1/%s/namespaces/%s/tables/%s/credentials",
+ RESTUtil.encodeString(catalogName),
+ RESTUtil.encodeNamespace(
+ tableIdentifier.namespace(),
NAMESPACE_SEPARATOR_URLENCODED_UTF_8),
+ RESTUtil.encodeString(tableIdentifier.name())));
+ }
Review Comment:
The returned REST credential exposes a mutable `config` map via `config()`.
Since this is effectively an API payload object, consider returning an
unmodifiable view (or a defensive copy in `config()`) to prevent accidental
mutation after creation (e.g., by downstream code that reuses the Credential
instance).
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergRESTUtils.java:
##########
@@ -87,4 +98,119 @@ void testSerdeIcebergRESTObject() {
Assertions.assertEquals(field, clonedField);
}
}
+
+ @Test
+ void testTableCredentialsPath() {
+ TableIdentifier table = TableIdentifier.of(Namespace.of("ns"), "tbl");
+ TableMetadata tableMetadata = mock(TableMetadata.class);
+ when(tableMetadata.location()).thenReturn("s3://bucket/t/");
+ org.apache.iceberg.rest.credentials.Credential credential =
+ IcebergRESTUtils.toRestCredential(
+ "my_catalog", table, new S3TokenCredential("k", "s", "t", 99L),
tableMetadata);
+ Assertions.assertEquals(
+ "v1/my_catalog/namespaces/ns/tables/tbl/credentials",
+ credential.config().get("client.refresh-credentials-endpoint"));
+ }
+
+ @Test
+ void testToRestCredential() {
+ TableIdentifier table = TableIdentifier.of(Namespace.of("ns"), "tbl");
+ String refreshPath = "v1/cat/namespaces/ns/tables/tbl/credentials";
+ TableMetadata metadataWithSlash = mock(TableMetadata.class);
+ when(metadataWithSlash.location()).thenReturn("s3://bucket/t/");
+ org.apache.iceberg.rest.credentials.Credential credentialWithSlash =
+ IcebergRESTUtils.toRestCredential(
+ "cat", table, new S3TokenCredential("k", "s", "t", 99L),
metadataWithSlash);
+ Assertions.assertEquals("s3://bucket/t/", credentialWithSlash.prefix());
+ Assertions.assertEquals(
+ "99",
credentialWithSlash.config().get("s3.session-token-expires-at-ms"));
+ Assertions.assertEquals(
+ refreshPath,
credentialWithSlash.config().get("client.refresh-credentials-endpoint"));
+
+ TableMetadata metadataWithoutSlash = mock(TableMetadata.class);
+
when(metadataWithoutSlash.location()).thenReturn("s3://bucket/path/to/table");
+ org.apache.iceberg.rest.credentials.Credential credentialWithoutSlash =
+ IcebergRESTUtils.toRestCredential(
+ "cat", table, new S3TokenCredential("k", "s", "t", 99L),
metadataWithoutSlash);
+ Assertions.assertEquals("s3://bucket/path/to/table/",
credentialWithoutSlash.prefix());
+ }
+
+ @Test
+ void testToRestCredentialForS3Token() {
+ TableIdentifier table = TableIdentifier.of(Namespace.of("ns"), "tbl");
+ TableMetadata tableMetadata = mock(TableMetadata.class);
+ when(tableMetadata.location()).thenReturn("s3://bucket/t/");
+ Map<String, String> config =
+ IcebergRESTUtils.toRestCredential(
+ "aws", table, new S3TokenCredential("key", "secret", "token",
1234L), tableMetadata)
+ .config();
+
+ Assertions.assertEquals(
+ "v1/aws/namespaces/ns/tables/tbl/credentials",
+ config.get("client.refresh-credentials-endpoint"));
+ }
Review Comment:
The new/updated logic includes credential-expiration properties (e.g.,
`s3.session-token-expires-at-ms`, `client.security-token-expires-at-ms`,
`gcs.oauth2.token-expires-at`, ADLS `adls.sas-token-expires-at-ms.*`) and also
special-cases `AwsIrsaCredential` for refresh endpoints. Consider extending
tests to assert the presence/values of these expiry properties per credential
type, and add coverage for the `AwsIrsaCredential` branch in
`IcebergRESTUtils.toRestCredential`.
##########
common/src/main/java/org/apache/gravitino/credential/CredentialPropertyUtils.java:
##########
@@ -82,24 +94,25 @@ public class CredentialPropertyUtils {
* @return a map of Iceberg properties derived from the credential
*/
public static Map<String, String> toIcebergProperties(Credential credential)
{
- if (credential instanceof S3TokenCredential
- || credential instanceof S3SecretKeyCredential
- || credential instanceof OSSTokenCredential
- || credential instanceof OSSSecretKeyCredential
- || credential instanceof AzureAccountKeyCredential
- || credential instanceof AwsIrsaCredential) {
- return transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap);
- }
-
- if (credential instanceof GCSTokenCredential) {
+ if (credential instanceof S3TokenCredential || credential instanceof
AwsIrsaCredential) {
+ Map<String, String> icebergProperties =
+ transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap);
+ icebergProperties.put(
+ ICEBERG_S3_TOKEN_EXPIRES_AT_MS,
String.valueOf(credential.expireTimeInMs()));
+ return icebergProperties;
+ } else if (credential instanceof OSSTokenCredential) {
+ Map<String, String> icebergProperties =
+ transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap);
+ icebergProperties.put(
+ ICEBERG_OSS_SECURITY_TOKEN_EXPIRES_AT_MS,
String.valueOf(credential.expireTimeInMs()));
+ return icebergProperties;
+ } else if (credential instanceof GCSTokenCredential) {
Map<String, String> icebergGCSCredentialProperties =
transformProperties(credential.credentialInfo(),
icebergCredentialPropertyMap);
icebergGCSCredentialProperties.put(
- GCS_OAUTH_2_TOKEN_EXPIRES_AT,
String.valueOf(credential.expireTimeInMs()));
+ ICEBERG_GCS_TOKEN_EXPIRES_AT,
String.valueOf(credential.expireTimeInMs()));
return icebergGCSCredentialProperties;
- }
-
- if (credential instanceof ADLSTokenCredential) {
+ } else if (credential instanceof ADLSTokenCredential) {
Review Comment:
These branches mutate the map returned by `transformProperties(...)`. To
make this robust regardless of the concrete map implementation returned (e.g.,
if it ever becomes immutable), consider wrapping it in a mutable `HashMap`
before adding the `*-expires-at*` properties.
--
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]
