dimas-b commented on code in PR #1164: URL: https://github.com/apache/polaris/pull/1164#discussion_r2029538335
########## quarkus/service/src/test/java/org/apache/polaris/service/quarkus/auth/JWTSymmetricKeyGeneratorTest.java: ########## @@ -65,6 +66,11 @@ public PolarisCallContext getPolarisCallContext() { public Map<String, Object> contextVariables() { return Map.of(); } + + @Override + public URI getBaseUri() { Review Comment: Is this change still needed? ########## service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java: ########## @@ -853,6 +854,19 @@ public Map<String, String> getCredentialConfig( storageInfo.get()); } + @Override + public Map<String, String> getVendedCredentialConfig(TableIdentifier tableIdentifier) { + Map<String, String> vendedCredentialConfig = new HashMap<>(); + String credentialsEndpoint = + String.format( + "/v1/%s/namespaces/%s/tables/%s/credentials", Review Comment: `IcebergCatalog` does not deal with the REST API at all, but this config requires understanding namespace path encoding/decoding. I believe it is preferable to inject this config in `IcebergCatalogAdapter` (where the decoding code exists). ########## service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java: ########## @@ -853,6 +854,19 @@ public Map<String, String> getCredentialConfig( storageInfo.get()); } + @Override + public Map<String, String> getVendedCredentialConfig(TableIdentifier tableIdentifier) { + Map<String, String> vendedCredentialConfig = new HashMap<>(); + String credentialsEndpoint = + String.format( + "/v1/%s/namespaces/%s/tables/%s/credentials", + catalogName, tableIdentifier.namespace().toString(), tableIdentifier.name()); + vendedCredentialConfig.put(AwsClientProperties.REFRESH_CREDENTIALS_ENABLED, "true"); + vendedCredentialConfig.put( + AwsClientProperties.REFRESH_CREDENTIALS_ENDPOINT, credentialsEndpoint); Review Comment: Is this property specific to the AWS client? I did not find any formal definition for it in Iceberg. I believe it would be preferable to have a local constant for it in Polaris to avoid the impression that it is supposed to work only for AWS. Given https://github.com/apache/iceberg/pull/11281, why do we have to set this property at all? Why is it not discovered automatically? ########## service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java: ########## @@ -853,6 +854,19 @@ public Map<String, String> getCredentialConfig( storageInfo.get()); } + @Override + public Map<String, String> getVendedCredentialConfig(TableIdentifier tableIdentifier) { + Map<String, String> vendedCredentialConfig = new HashMap<>(); + String credentialsEndpoint = + String.format( + "/v1/%s/namespaces/%s/tables/%s/credentials", + catalogName, tableIdentifier.namespace().toString(), tableIdentifier.name()); Review Comment: Does this do proper escaping of special chars in the URI path? Cf. `IcebergCatalogAdapter.decodeNamespace()` -- 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: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org