zhangxinyao88 commented on code in PR #17912:
URL: https://github.com/apache/iceberg/pull/17912#discussion_r4504703845
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1320,7 +1320,17 @@ private FileIO tableFileIO(
RemoteSigningConfigParser.toJson(remoteSigningConfig));
}
- return newFileIO(context, fullConf.buildKeepingLast(), storageCredentials);
+ Map<String, String> ioProperties =
Maps.newHashMap(fullConf.buildKeepingLast());
+ ioProperties.remove(RESTCatalogProperties.CREDENTIALS_ENDPOINT);
+ if (!storageCredentials.isEmpty()) {
+ ioProperties.put(
+ RESTCatalogProperties.CREDENTIALS_ENDPOINT,
Review Comment:
> Now a server that vends credentials but does not serve `.../credentials`
gets refresh calls that fail. Is that intended?
That was a gap. The catalog now sets the shared endpoint only when storage
credentials are present and the server advertises `V1_TABLE_CREDENTIALS`. The
legacy endpoint settings still work as fallbacks.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1320,7 +1320,17 @@ private FileIO tableFileIO(
RemoteSigningConfigParser.toJson(remoteSigningConfig));
}
- return newFileIO(context, fullConf.buildKeepingLast(), storageCredentials);
+ Map<String, String> ioProperties =
Maps.newHashMap(fullConf.buildKeepingLast());
+ ioProperties.remove(RESTCatalogProperties.CREDENTIALS_ENDPOINT);
+ if (!storageCredentials.isEmpty()) {
Review Comment:
> what happens if a credentials endpoint returns just basic keys without
session token or expiry? won't VendedCredentialsProvider just reject those?
Yes, it did. The provider now accepts basic keys from both the initial
properties and the endpoint response, with regression tests for both.
I also kept the existing opt-in for client refresh, so the shared endpoint
alone no longer switches the credentials provider. While checking this, I found
that the providers reject multiple credentials of the same storage type and
opened [#18825](https://github.com/apache/iceberg/issues/18825) to track that
separately.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java:
##########
@@ -56,7 +58,14 @@ public class VendedCredentialsProvider implements
AwsCredentialsProvider, SdkAut
private VendedCredentialsProvider(Map<String, String> properties) {
Preconditions.checkArgument(null != properties, "Invalid properties:
null");
- Preconditions.checkArgument(null != properties.get(URI), "Invalid
credentials endpoint: null");
+ this.credentialsEndpoint =
+ properties.containsKey(RESTCatalogProperties.CREDENTIALS_ENDPOINT)
+ ? properties.get(RESTCatalogProperties.CREDENTIALS_ENDPOINT)
Review Comment:
> Can we move it to one helper in `RESTUtil`, for example
`credentialsEndpoint(properties, legacyEndpoint)`?
Moved the endpoint resolution into `RESTUtil.resolveCredentialEndpoint`. All
four callers use it, with tests for shared endpoint precedence and the legacy
fallback.
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/OAuth2RefreshCredentialsHandler.java:
##########
@@ -51,13 +52,15 @@ public class OAuth2RefreshCredentialsHandler
private AuthSession authSession;
private OAuth2RefreshCredentialsHandler(Map<String, String> properties) {
- Preconditions.checkArgument(
- null !=
properties.get(GCPProperties.GCS_OAUTH2_REFRESH_CREDENTIALS_ENDPOINT),
- "Invalid credentials endpoint: null");
+ this.credentialsEndpoint =
+ properties.containsKey(RESTCatalogProperties.CREDENTIALS_ENDPOINT)
+ ? properties.get(RESTCatalogProperties.CREDENTIALS_ENDPOINT)
+ : RESTUtil.resolveEndpoint(
+ properties.get(CatalogProperties.URI),
+
properties.get(GCPProperties.GCS_OAUTH2_REFRESH_CREDENTIALS_ENDPOINT));
Review Comment:
> This was all the provider specific property key inspection stays here and
the RESTUtil is generic enough, wdyt ?
Used this approach. Each caller reads its own property and passes the value
to the helper.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java:
##########
@@ -56,7 +58,14 @@ public class VendedCredentialsProvider implements
AwsCredentialsProvider, SdkAut
private VendedCredentialsProvider(Map<String, String> properties) {
Preconditions.checkArgument(null != properties, "Invalid properties:
null");
- Preconditions.checkArgument(null != properties.get(URI), "Invalid
credentials endpoint: null");
+ this.credentialsEndpoint =
+ properties.containsKey(RESTCatalogProperties.CREDENTIALS_ENDPOINT)
+ ? properties.get(RESTCatalogProperties.CREDENTIALS_ENDPOINT)
+ : RESTUtil.resolveEndpoint(
+ properties.get(CatalogProperties.URI),
+ properties.getOrDefault(
+ URI,
properties.get(AwsClientProperties.REFRESH_CREDENTIALS_ENDPOINT)));
Review Comment:
> can we use PropertyUtils here
Changed to `PropertyUtil.propertyAsString`. The shared REST helper uses it
too.
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1320,7 +1320,17 @@ private FileIO tableFileIO(
RemoteSigningConfigParser.toJson(remoteSigningConfig));
}
- return newFileIO(context, fullConf.buildKeepingLast(), storageCredentials);
+ Map<String, String> ioProperties =
Maps.newHashMap(fullConf.buildKeepingLast());
+ ioProperties.remove(RESTCatalogProperties.CREDENTIALS_ENDPOINT);
+ if (!storageCredentials.isEmpty()) {
+ ioProperties.put(
+ RESTCatalogProperties.CREDENTIALS_ENDPOINT,
+ RESTUtil.resolveEndpoint(
+ properties().get(CatalogProperties.URI),
+ paths.table(tableIdentifier) + "/credentials"));
Review Comment:
> Can we add a `credentials(TableIdentifier)` method there and use it here
and in the test?
Added `ResourcePaths.credentials()` and used it in both places. It builds on
`table(identifier)` for the prefix and encoding.
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3FileIOCredentialRefresh.java:
##########
@@ -71,8 +74,9 @@ public void before() {
mockServer.reset();
}
- @Test
- public void credentialRefreshSchedulesNextRefresh() {
+ @ParameterizedTest
+ @ValueSource(strings = {"rest.credentials.endpoint",
"client.refresh-credentials-endpoint"})
Review Comment:
> Can we use `RESTCatalogProperties.CREDENTIALS_ENDPOINT` and
`AwsClientProperties.REFRESH_CREDENTIALS_ENDPOINT` here?
Changed both S3 and GCS tests to use the constants.
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestVendedCredentialsProvider.java:
##########
@@ -120,6 +120,54 @@ public void noS3Credentials() {
}
}
+ @Test
+ public void resolvesRelativeCredentialsUri() {
Review Comment:
> Can we remove `public` from `resolvesRelativeCredentialsUri` and
`usesCatalogCredentialsEndpoint` here and in
`TestOAuth2RefreshCredentialsHandler`?
Removed `public` from both methods in both classes.
--
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]
