-
Notifications
You must be signed in to change notification settings - Fork 330
add refresh credentials property to loadTableResult #2341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d6066fa
5c05a0d
b9386a2
0e388b5
149b1fc
32ca5c2
d8d11b3
7259b61
d449ccf
211d315
77d0968
c93d2b3
dd18fef
8ae8f0e
2d6dd03
2cbfbd5
4159327
cae0632
2a123d1
d8026f2
a6ac0cf
9b9dc41
e59fc7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,17 @@ public String genericTables(Namespace ns) { | |
| "polaris", "v1", prefix, "namespaces", RESTUtil.encodeNamespace(ns), "generic-tables"); | ||
| } | ||
|
|
||
| public String credentialsPath(TableIdentifier ident) { | ||
| return SLASH.join( | ||
| "v1", | ||
| prefix, | ||
| "namespaces", | ||
| RESTUtil.encodeNamespace(ident.namespace()), | ||
| "tables", | ||
| RESTUtil.encodeString(ident.name()), | ||
| "credentials"); | ||
|
Comment on lines
+60
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to confirm: relative URIs are correct in this case, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, from what I understand the recent version of the client automatically appends the base URI if a relative path is returned from the server. |
||
| } | ||
|
|
||
| public String genericTable(TableIdentifier ident) { | ||
| return SLASH.join( | ||
| "polaris", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| package org.apache.polaris.core.storage; | ||
|
|
||
| import jakarta.annotation.Nonnull; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import org.apache.polaris.core.PolarisCallContext; | ||
| import org.apache.polaris.core.entity.PolarisEntityType; | ||
|
|
@@ -37,6 +38,10 @@ public interface PolarisCredentialVendor { | |
| * allowedWriteLocations | ||
| * @param allowedReadLocations a set of allowed to read locations | ||
| * @param allowedWriteLocations a set of allowed to write locations | ||
| * @param refreshCredentialsEndpoint an optional endpoint to use for refreshing credentials. If | ||
| * supported by the storage type it will be returned to the client in the appropriate | ||
| * properties. The endpoint may be relative to the base URI and the client is responsible for | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we return anything except relative path ? my understanding is base uri is always catalog uri https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java#L92 is this statement in intentionally open ended since spec is not explicit ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At one point the client required full paths, now it works with partial paths. I'm not sure if the spec identifies one as the correct method over the other so I left it like this. If one is definitely the "correct" way I can update the comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jasonf20 : do you know the Iceberg version / PR where relative paths started working?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was linked in the previous PR. #1164 (comment) Seems like it's since 1.8.0
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh no this made a lot of things super tricky, i checked iceberg python for refresh keyword but i didn't any prs for that so i am assuming that it doesn't support, taking only the iceberg java sdk reference seems like the only broken release would be 1.7 Its a bit surprising to me, but i think for now we can park this by just documenting this gotcha, WDYT @dimas-b ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting full URI is a bit trickier - as discussed in #1164. From my POV, let's merge this "as is" and open a GH issue that it does not work properly for Iceberg < 1.8. When someone has time, let's improve. Getting this feature enabled for Iceberg clients >= 1.8 is valuable, I think.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since old client version behave differently, I think we ought to have a feature flag now... just in case someone runs into client incompatibility issues and wants to disable credential refresh properties completely (i.e. revert to old Polaris behaviour). WDYT?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be ideal IMHO (specially now), but i think the jobs without us sending the credentials endpoint would still have failed because my understanding is we would have not made the /credentials call unless the creds in loadTable expires https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java#L63, its just we might see an HTTP error rather than the creds expired exception.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think a feature flag is needed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added a commit that does 4. |
||
| * handling the relative path | ||
| * @return an enum map containing the scoped credentials | ||
| */ | ||
| @Nonnull | ||
|
|
@@ -47,5 +52,6 @@ ScopedCredentialsResult getSubscopedCredsForEntity( | |
| PolarisEntityType entityType, | ||
| boolean allowListOperation, | ||
| @Nonnull Set<String> allowedReadLocations, | ||
| @Nonnull Set<String> allowedWriteLocations); | ||
| @Nonnull Set<String> allowedWriteLocations, | ||
| Optional<String> refreshCredentialsEndpoint); | ||
dimas-b marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to document this. Unfortunately, the best place to doc this is a page for storage setting and credential vending, which doesn't exist yet. Here is the issue filed long time ago, #1325. Not a blocker for this PR though.