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

Reply via email to