Copilot commented on code in PR #11294:
URL: https://github.com/apache/gravitino/pull/11294#discussion_r3354111928
##########
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' (or similar) to be clear and professional.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -83,6 +93,103 @@ 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.
+ *
+ * <p>Refresh credential endpoints are added only for credential types whose
Iceberg client
+ * modules support vended credential refresh: S3 ({@link
+ * org.apache.gravitino.credential.S3TokenCredential}, {@link
+ * org.apache.gravitino.credential.AwsIrsaCredential}), GCS ({@link
+ * org.apache.gravitino.credential.GCSTokenCredential}), and ADLS ({@link
+ * org.apache.gravitino.credential.ADLSTokenCredential}). OSS temporary
credentials ({@link
+ * org.apache.gravitino.credential.OSSTokenCredential}) are omitted because
Iceberg's Aliyun OSS
+ * FileIO does not consume {@code client.refresh-credentials-endpoint}; only
the initial STS
+ * properties are returned.
+ *
+ * @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));
+ config.putAll(buildRefreshProps(catalogName, tableIdentifier, config));
+
+ String location = tableMetadata.location();
+ String prefix = location.endsWith("/") ? location : location + "/";
+ return toRESTCredential(prefix, config);
+ }
+
+ /**
+ * Builds an Iceberg REST {@link
org.apache.iceberg.rest.credentials.Credential} from a storage
+ * prefix and credential config map.
+ *
+ * @param prefix storage location prefix for the credential
+ * @param config Iceberg credential config properties
+ * @return Iceberg REST credential
+ */
+ public static org.apache.iceberg.rest.credentials.Credential
toRESTCredential(
+ String prefix, Map<String, String> config) {
+ Map<String, String> credentialConfig = ImmutableMap.copyOf(config);
+ return new org.apache.iceberg.rest.credentials.Credential() {
+ @Override
+ public String prefix() {
+ return prefix;
+ }
+
+ @Override
+ public Map<String, String> config() {
+ return credentialConfig;
+ }
+
+ @Override
+ public void validate() {}
+ };
+ }
+
+ /**
+ * Builds Iceberg REST 1.11 {@code storage-credentials} from an upstream
REST catalog proxy.
+ *
+ * <p>Upstream credentials are read from {@link
SupportsStorageCredentials#credentials()}. When
+ * present, they are filtered and rewritten with IRC-local refresh endpoints
via {@link
+ * CredentialPropertyUtils}. Credential properties in {@link
FileIO#properties()} must be handled
+ * separately by the caller.
+ *
+ * @param catalogName IRC catalog name used to build refresh paths
+ * @param tableIdentifier table receiving the credentials
+ * @param fileIO table FileIO returned by the upstream catalog
+ * @return rewritten storage credentials for the downstream client, or an
empty list
+ */
+ public static List<org.apache.iceberg.rest.credentials.Credential>
buildStorageCreds(
+ String catalogName, TableIdentifier tableIdentifier, FileIO fileIO) {
+ if (!(fileIO instanceof SupportsStorageCredentials)) {
+ return Collections.emptyList();
+ }
+
+ List<StorageCredential> credentials = ((SupportsStorageCredentials)
fileIO).credentials();
+ if (credentials == null || credentials.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<org.apache.iceberg.rest.credentials.Credential> restCredentials = new
ArrayList<>();
+ for (StorageCredential credential : credentials) {
+ Map<String, String> filteredConfig =
+
CredentialPropertyUtils.filterCredentialProperties(credential.config());
+ filteredConfig.putAll(buildRefreshProps(catalogName, tableIdentifier,
filteredConfig));
+ restCredentials.add(
+ toRESTCredential(credential.prefix(),
ImmutableMap.copyOf(filteredConfig)));
+ }
Review Comment:
Unlike `toRESTCredential(..., TableMetadata)`, `buildStorageCreds` does not
normalize `StorageCredential.prefix()` to ensure it ends with '/'. If an
upstream `StorageCredential` prefix is missing the trailing slash, downstream
Iceberg clients may fail to match the credential to a location prefix
consistently. Consider normalizing `credential.prefix()` the same way (append
'/' if needed) before building the REST credential.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestCatalogWrapperForREST.java:
##########
@@ -27,8 +27,13 @@
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
import com.google.common.collect.ImmutableMap;
+import com.sun.net.httpserver.HttpServer;
+import java.io.OutputStream;
+import java.net.InetSocketAddress;
+import java.nio.charset.StandardCharsets;
Review Comment:
Using `com.sun.net.httpserver.HttpServer` in tests couples the suite to the
JDK's built-in HTTP server (and, in JPMS/limited-module environments, the
`jdk.httpserver` module). To improve portability and reduce potential
environment-related test failures, consider switching to a dedicated test HTTP
server dependency already used in the repo (e.g., OkHttp
MockWebServer/WireMock), or ensure the build config explicitly enables the
required module when running 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]
