zhangxinyao88 commented on code in PR #17912:
URL: https://github.com/apache/iceberg/pull/17912#discussion_r4534304314
##########
aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java:
##########
@@ -119,6 +126,16 @@ private Optional<RefreshResult<AwsCredentials>>
credentialFromProperties() {
String secretAccessKey =
properties.get(S3FileIOProperties.SECRET_ACCESS_KEY);
String sessionToken = properties.get(S3FileIOProperties.SESSION_TOKEN);
String tokenExpiresAtMillis =
properties.get(S3FileIOProperties.SESSION_TOKEN_EXPIRES_AT_MS);
+ if (!Strings.isNullOrEmpty(accessKeyId)
+ && !Strings.isNullOrEmpty(secretAccessKey)
+ && Strings.isNullOrEmpty(sessionToken)
+ && Strings.isNullOrEmpty(tokenExpiresAtMillis)) {
Review Comment:
I removed the AWS basic-key changes and their tests from this PR in
`0c0816ca`. The endpoint fix can proceed independently.
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestVendedCredentialsProvider.java:
##########
@@ -120,6 +122,78 @@ public void noS3Credentials() {
}
}
+ @Test
+ void fetchesCredentialsWithoutSessionTokenOrExpiry() {
Review Comment:
`0c0816ca` restores the test that checks the missing session token error.
The AWS tests passed.
##########
gcp/src/main/java/org/apache/iceberg/gcp/GCPProperties.java:
##########
@@ -167,10 +166,12 @@ public GCPProperties(Map<String, String> properties) {
new
Date(Long.parseLong(properties.get(GCS_OAUTH2_TOKEN_EXPIRES_AT)));
}
+ // The shared endpoint also serves FileIO refresh and does not opt into
client refresh.
+ String clientRefreshEndpoint =
properties.get(GCS_OAUTH2_REFRESH_CREDENTIALS_ENDPOINT);
gcsOauth2RefreshCredentialsEndpoint =
- RESTUtil.resolveEndpoint(
- properties.get(CatalogProperties.URI),
- properties.get(GCS_OAUTH2_REFRESH_CREDENTIALS_ENDPOINT));
+ clientRefreshEndpoint == null
+ ? null
+ : RESTUtil.resolveCredentialEndpoint(properties,
clientRefreshEndpoint);
Review Comment:
`0c0816ca` treats an empty GCS refresh endpoint as disabled, matching S3. I
added a regression test for an empty endpoint when the shared catalog endpoint
is set.
##########
core/src/main/java/org/apache/iceberg/rest/RESTUtil.java:
##########
@@ -403,6 +404,15 @@ public static Namespace
decodeNamespaceAsPathSegment(String encodedNamespace, St
return Namespace.of(levels);
}
+ /** Returns the shared credentials endpoint, falling back to the resolved
legacy endpoint. */
Review Comment:
Updated the Javadoc in `0c0816ca` to say that the fallback path resolves
against the catalog URI and the result can be null.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
