yayaha commented on code in PR #17912:
URL: https://github.com/apache/iceberg/pull/17912#discussion_r4499034601
##########
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:
question: This sets `rest.credentials.endpoint` for every table with storage
credentials. Before, the clients refreshed only when the server sent
`client.refresh-credentials-endpoint` or
`gcs.oauth2.refresh-credentials-endpoint`. Now a server that vends credentials
but does not serve `.../credentials` gets refresh calls that fail. Is that
intended? If not, can we also require `Endpoint.V1_TABLE_CREDENTIALS` in
`endpoints`, or a legacy refresh endpoint in the table config?
##########
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:
nit: Can we use `RESTCatalogProperties.CREDENTIALS_ENDPOINT` and
`AwsClientProperties.REFRESH_CREDENTIALS_ENDPOINT` here? Both are compile-time
constants, so `@ValueSource` accepts them, and a rename then updates the test
too. The same applies to the `@ValueSource` in `TestGCSFileIOCredentialRefresh`.
##########
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:
nit: Four classes repeat the same rule: use `rest.credentials.endpoint`, or
else resolve the legacy property against the catalog URI. The copies are here,
in `AwsClientProperties`, `GCPProperties`, and
`OAuth2RefreshCredentialsHandler`. Can we move it to one helper in `RESTUtil`,
for example `credentialsEndpoint(properties, legacyEndpoint)`? Then the
precedence changes in one place for S3 and GCS.
##########
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:
nit: `ResourcePaths` builds the other table routes, for example
`remoteSign(identifier)`. Can we add a `credentials(TableIdentifier)` method
there and use it here and in the test? Then this route follows the same prefix
and encoding rules in one place.
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestVendedCredentialsProvider.java:
##########
@@ -120,6 +120,54 @@ public void noS3Credentials() {
}
}
+ @Test
+ public void resolvesRelativeCredentialsUri() {
Review Comment:
nit: The repository guidelines ask for package-private test methods in new
tests. Can we remove `public` from `resolvesRelativeCredentialsUri` and
`usesCatalogCredentialsEndpoint` here and in
`TestOAuth2RefreshCredentialsHandler`?
--
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]
