add refresh credentials property to loadTableResult #1164

wolflex888 · 2025-03-12T21:07:14Z

This PR will add necessary refresh-vended-credentials properties to LoadTableResponse to allow each IoFile will get appropriate properties to initiate VendedCredentialsProvider.

wolflex888 · 2025-03-12T21:08:20Z

...ce/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java

-            return responseBuilder.build();
+            if(accessDelegationModes.contains(AccessDelegationMode.VENDED_CREDENTIALS)){
+              try {
+                String hostName = InetAddress.getLocalHost().getCanonicalHostName();


Is there a better way to grab the server host here? I don't think this is reliable but I do not know a better way to get the host. Potentially, we can get it from the request header? but it looks like the headers are being filtered out.

dimas-b · 2025-03-12T21:36:00Z

...ce/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java

+            if (accessDelegationModes.contains(AccessDelegationMode.VENDED_CREDENTIALS)) {
+              try {
+                // String hostName = InetAddress.getLocalHost().getCanonicalHostName();
+                String hostName = "localhost";


This is not so simple, I'm afraid :) We need to think of proxies too.

At the very minimum, I think we need to get a UriInfo injected and use uriInfo.getBaseUri().

A more robust approach is here: https://github.com/projectnessie/nessie/blob/a9028f12c5152ebc14ba32731b52fdc9a107db94/servers/quarkus-catalog/src/main/java/org/projectnessie/server/catalog/ExternalBaseUriImpl.java#L38

dimas-b · 2025-04-04T23:04:41Z

service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

+    String credentialsEndpoint =
+        String.format(
+            "/v1/%s/namespaces/%s/tables/%s/credentials",
+            catalogName, tableIdentifier.namespace().toString(), tableIdentifier.name());


Does this do proper escaping of special chars in the URI path?

Cf. IcebergCatalogAdapter.decodeNamespace()

dimas-b · 2025-04-04T23:12:10Z

service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

+    Map<String, String> vendedCredentialConfig = new HashMap<>();
+    String credentialsEndpoint =
+        String.format(
+            "/v1/%s/namespaces/%s/tables/%s/credentials",


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).

I moved the string construction to IcebergCatalogAdapter where the decoding code exists, but I feel that's a little bit clumsy to pass it all the way down to SupportsCredentialDelegation. I wonder if you have a more elegant way of doing this

dimas-b · 2025-04-04T23:12:37Z

...vice/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() {


Is this change still needed?

This is removed

dimas-b · 2025-04-04T23:19:07Z

service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

+            catalogName, tableIdentifier.namespace().toString(), tableIdentifier.name());
+    vendedCredentialConfig.put(AwsClientProperties.REFRESH_CREDENTIALS_ENABLED, "true");
+    vendedCredentialConfig.put(
+        AwsClientProperties.REFRESH_CREDENTIALS_ENDPOINT, credentialsEndpoint);


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 apache/iceberg#11281, why do we have to set this property at all? Why is it not discovered automatically?

That was my question to the iceberg team as well, but they insist on having this property set so iceberg knows which endpoint to reach out to.

Regarding the AwsClientProperties, there's also discussion here

collado-mike · 2025-04-10T22:14:09Z

...vice/src/test/java/org/apache/polaris/service/quarkus/auth/JWTSymmetricKeyGeneratorTest.java

@@ -24,6 +24,7 @@
 import com.auth0.jwt.JWTVerifier;
 import com.auth0.jwt.algorithms.Algorithm;
 import com.auth0.jwt.interfaces.DecodedJWT;
+import java.net.URI;


unnecessary import?

This has been removed

collado-mike · 2025-04-10T22:15:25Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

@@ -378,6 +378,7 @@ public Response loadTable(
    Namespace ns = decodeNamespace(namespace);
    TableIdentifier tableIdentifier = TableIdentifier.of(ns, RESTUtil.decodeString(table));

+


I don't know why these extra blank lines are added

this has been removed

collado-mike · 2025-04-10T22:16:38Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

+            String credentialsEndpoint =
+                String.format(
+                    "/v1/%s/namespaces/%s/tables/%s/credentials",
+                    prefix, tableIdentifier.namespace().toString(), tableIdentifier.name());


This needs to be configurable. E.g., some hosts may prefix the URL with /polaris or with /your_account_name or any number of prefixes

does that mean they will do something like /polaris-catalog-name instead of just /catalog-name? or I'm misunderstanding this?

prefix identifies the catalog, does it not? Why would the .../credentials endpoint for a table in one catalog need to be under a different prefix?

collado-mike · 2025-04-10T22:18:25Z

service/common/src/test/java/org/apache/polaris/service/catalog/io/FileIOFactoryTest.java

@@ -25,6 +25,7 @@
 import com.google.common.collect.ImmutableMap;
 import jakarta.annotation.Nonnull;
 import java.lang.reflect.Method;
+import java.net.URI;


unnecessary import?

this is removed.

collado-mike · 2025-04-10T22:18:29Z

service/common/src/testFixtures/java/org/apache/polaris/service/TestServices.java

@@ -21,6 +21,7 @@
 import com.google.auth.oauth2.AccessToken;
 import com.google.auth.oauth2.GoogleCredentials;
 import jakarta.ws.rs.core.SecurityContext;
+import java.net.URI;


sorry, i forgot to do spotlessApply. these should all be fixed now.

collado-mike · 2025-04-10T22:22:50Z

service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

@@ -856,6 +857,15 @@ public Map<String, String> getCredentialConfig(
        storageInfo.get());
  }

+  @Override
+  public Map<String, String> getVendedCredentialConfig(TableIdentifier tableIdentifier, String decodedCredentialsPath) {


Why is this a whole new method rather than just putting this directly in the IcebergCatalogHandler. You're just putting values into a map, so...

I see. I thought it can be more organized that way. I removed it.

dimas-b · 2025-04-14T18:04:43Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

+                        ifNoneMatch,
+                        snapshots,
+                        delegationModes,
+                        RESTUtil.decodeString(credentialsEndpoint))


I believe this class (or class above it in the call chain) should inject credentialsEndpoint in this response. We should not push this parameter down to loadTableWithAccessDelegationIfStale because the lower level class has nothing to do with REST URIs.

I see. does that mean we should be building the response here instead of having a complete response returned from loadTableAccessDelegationIfStale?

I created a function to do the injection at the proper level. Let me know if that's what you have in mind! Thanks for all the feedback! really appreciate it

dimas-b · 2025-05-22T13:12:36Z

@wolflex888 : Are you still working on this? Sorry for the delay with reviews. Could you resolve conflicts, please?

smaheshwar-pltr · 2025-06-26T15:58:58Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

+            String credentialsEndpoint =
+                String.format(
+                    "/v1/%s/namespaces/%s/tables/%s/credentials",
+                    prefix, tableIdentifier.namespace().toString(), tableIdentifier.name());


(Saw that #1164 (comment) got lost but think it still holds)

Agree the uri should be :

public String tableCredentials(TableIdentifier ident) { return SLASH.join( "v1", prefix, "namespaces", RESTUtil.encodeNamespace(ident.namespace()), "tables", RESTUtil.encodeString(ident.name()), "credentials"); }

jasonf20 · 2025-08-04T12:43:31Z

Hi,

Just following up—any updates on this? It should address apache/polaris#2177.

I understand this is an open-source project, but this is a small yet important change, especially given this projects role as a backend for enterprise products like Snowflake Open Catalog. Any update or timeline would be greatly appreciated.

Thank you.

flyrain · 2025-08-04T16:24:04Z

cc @singhpk234

singhpk234 · 2025-08-04T17:44:47Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

+    loadResponseBuilder.addConfig(
+        AwsClientProperties.REFRESH_CREDENTIALS_ENDPOINT, credentialsEndpoint);
+    loadResponseBuilder.addConfig(AwsClientProperties.REFRESH_CREDENTIALS_ENABLED, "true");


ADLS also now supports creds refresh

singhpk234 · 2025-08-04T17:45:46Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

+            String credentialsEndpoint =
+                String.format(
+                    "/v1/%s/namespaces/%s/tables/%s/credentials",
+                    prefix, tableIdentifier.namespace().toString(), tableIdentifier.name());


Agree the uri should be :

public String tableCredentials(TableIdentifier ident) { return SLASH.join( "v1", prefix, "namespaces", RESTUtil.encodeNamespace(ident.namespace()), "tables", RESTUtil.encodeString(ident.name()), "credentials"); }

singhpk234 · 2025-08-04T17:48:42Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

+    LoadTableResponse.Builder loadResponseBuilder =
+        LoadTableResponse.builder().withTableMetadata(originalResponse.tableMetadata());


do we need to update metadata-location too ?

singhpk234 · 2025-08-04T17:49:19Z

...e/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

+    loadResponseBuilder.addConfig(
+        AwsClientProperties.REFRESH_CREDENTIALS_ENDPOINT, credentialsEndpoint);
+    loadResponseBuilder.addConfig(AwsClientProperties.REFRESH_CREDENTIALS_ENABLED, "true");


we should check the credential type too and send this only when storage is AWS

singhpk234 · 2025-08-04T18:09:44Z

@wolflex888 would you mind rebasing i can help in review

dimas-b · 2025-08-12T23:36:51Z

@jasonf20 : If you have the capacity to make an alternative PR for this, I think it might be a reasonable path forward. If you do, please see my comments about URI handling I made under this PR.

jasonf20 · 2025-08-13T08:52:17Z

@dimas-b Please see here: #2341

This PR has gotten a little messy so please leave new comments if I missed anything.

I understand everyone’s busy, but I’m a bit concerned that no one from the Snowflake team or other contributors has taken this on yet. I’ll do my best to assist where I can. I don't have a lot of capacity for this, but long as we keep it simple it should be fine.

wolflex888 · 2025-08-13T09:59:54Z

I'm gonna be closing this PR as this has gotten a little bit messy. we can move forward with #2341

add refresh credentials property to loadTableResult

63c7dc7

github-project-automation bot added this to Basic Kanban Board Mar 12, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 12, 2025

wolflex888 commented Mar 12, 2025

View reviewed changes

wolflex888 marked this pull request as ready for review March 12, 2025 21:08

wolflex888 requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners March 12, 2025 21:08

David Lu added 2 commits March 12, 2025 17:11

remove bad method and replace with localhost

a885ffb

spotless

7912eca

dimas-b reviewed Mar 12, 2025

View reviewed changes

wolflex888 marked this pull request as draft March 13, 2025 14:45

David Lu added 4 commits March 14, 2025 13:31

add request base uri to call context

aa6f1b1

fix tests

9c1c283

add uri builder

0241b80

clean up code

c1f2cbe

wolflex888 marked this pull request as ready for review March 19, 2025 16:53

wolflex888 requested a review from dimas-b March 19, 2025 16:53

wolflex888 requested a review from dimas-b March 24, 2025 16:55

Merge branch 'main' into loadTableResponse-refresh-credential-properties

ab02a2d

dimas-b reviewed Apr 4, 2025

View reviewed changes

David Lu added 5 commits April 10, 2025 16:37

remove getBaseUri

87d8358

merge

07f9dc1

refactor based on merge

7002409

move endpoint construction to IcebergCatalogAdapter

aec31d7

pass down path

bcea6c6

collado-mike reviewed Apr 10, 2025

View reviewed changes

David Lu added 2 commits April 10, 2025 20:35

spotlessApply

6ccc3e1

remove unnecessary method

54ef878

wolflex888 requested review from collado-mike and dimas-b April 14, 2025 16:43

dimas-b reviewed Apr 14, 2025

View reviewed changes

inject refresh credential properties at proper place

315afda

wolflex888 requested a review from dimas-b April 15, 2025 16:35

Merge branch 'main' into loadTableResponse-refresh-credential-properties

d073982

smaheshwar-pltr reviewed Jun 26, 2025

View reviewed changes

singhpk234 reviewed Aug 4, 2025

View reviewed changes

jasonf20 mentioned this pull request Aug 13, 2025

add refresh credentials property to loadTableResult #2341

Open

wolflex888 closed this Aug 13, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Aug 13, 2025

		@@ -378,6 +378,7 @@ public Response loadTable(
		Namespace ns = decodeNamespace(namespace);
		TableIdentifier tableIdentifier = TableIdentifier.of(ns, RESTUtil.decodeString(table));

		LoadTableResponse.Builder loadResponseBuilder =
		LoadTableResponse.builder().withTableMetadata(originalResponse.tableMetadata());

add refresh credentials property to loadTableResult #1164

add refresh credentials property to loadTableResult #1164

Uh oh!

Conversation

wolflex888 commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wolflex888 Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonf20 commented Aug 4, 2025

Uh oh!

flyrain commented Aug 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 commented Aug 4, 2025

wolflex888 Apr 15, 2025 •

edited

Loading

dimas-b commented May 22, 2025 •

edited

Loading

dimas-b commented Aug 12, 2025 •

edited

Loading