yayaha commented on code in PR #17912:
URL: https://github.com/apache/iceberg/pull/17912#discussion_r4528131080
##########
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:
non-blocking: With the opt-in change, `rest.credentials.endpoint` alone no
longer selects `VendedCredentialsProvider`, so this PR does not need basic-key
support. The basic-key support also changes existing behavior. With basic keys
and `client.refresh-credentials-endpoint` in the properties, the provider now
never calls the endpoint. Can we move this part and its tests to a separate PR?
Then this PR stays the fix for #17810.
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestVendedCredentialsProvider.java:
##########
@@ -120,6 +122,78 @@ public void noS3Credentials() {
}
}
+ @Test
+ void fetchesCredentialsWithoutSessionTokenOrExpiry() {
Review Comment:
This test calls only `fetchCredentials()`, which does not check the keys, so
it passes without the change. `accessKeyIdAndSecretAccessKeyWithoutToken`
already covers a response with basic keys. The diff also removed the only check
of `Invalid S3 Credentials: s3.session-token not set`. Can we replace this test
with a response that has an expiry but no token, and assert that message?
##########
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:
nit: `AwsClientProperties` uses `Strings.isNullOrEmpty` here, so an empty
`client.refresh-credentials-endpoint` turns S3 client refresh off. This check
uses `== null`. An empty `gcs.oauth2.refresh-credentials-endpoint` then turns
GCS client refresh on, and the URL is the shared endpoint or the catalog root.
Can we use `Strings.isNullOrEmpty` here too, so both clients treat an empty
value the same way?
##########
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:
nit: The Javadoc does not say what `endpointPath` is resolved against, or
that the method can return null. All four callers handle a null result, so the
contract matters. Can we state it?
```suggestion
/**
* Returns {@link RESTCatalogProperties#CREDENTIALS_ENDPOINT} when it is
set. Otherwise returns
* {@code endpointPath} resolved against the catalog URI, or null when
{@code endpointPath} is
* 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]
