Skip to content

REST: Auto Refresh Creds for long running tasks #13716

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.

Sign up for GitHub

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

Closed
singhpk234 wants to merge 2 commits into from
Closed

REST: Auto Refresh Creds for long running tasks #13716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c130106
Rest: Refresh Creds for long running tasks
sfc-gh-prsingh Jul 31, 2025
d2a07c0
review feedback
sfc-gh-prsingh Aug 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ public Table loadTable(SessionContext context, TableIdentifier identifier) {
tableClient,
paths.table(finalIdentifier),
Map::of,
tableFileIO(context, tableConf, response.credentials()),
tableFileIO(context, identifier, tableConf, response.credentials()),
tableMetadata,
endpoints);

Expand Down Expand Up @@ -992,6 +992,38 @@ private FileIO newFileIO(
}
}

private FileIO tableFileIO(
SessionContext context,
TableIdentifier identifier,
Map<String, String> config,
List<Credential> storageCredentials) {
// Check if either credential refresh is enabled from either client side or server side.
// TODO: convert this to constants.
boolean s3RefreshEnabled =
Copy link
Contributor

@nastra nastra Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue here is that we're "leaking" storage-specific settings into the REST layer, which we typically want to avoid

PropertyUtil.propertyAsBoolean(config, "client.refresh-credentials-enabled", false)
|| PropertyUtil.propertyAsBoolean(
properties(), "client.refresh-credentials-enabled", false);
boolean adlsRefreshEnabled =
PropertyUtil.propertyAsBoolean(config, "adls.refresh-credentials-enabled", false)
|| PropertyUtil.propertyAsBoolean(
properties(), "adls.refresh-credentials-enabled", false);

// Inject the credentials refresh endpoint if, refresh is configured.
// This is done because the server would not know the complete URI of the refresh endpoint.
Copy link
Contributor

@nastra nastra Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would the server not know the URI to refresh credentials for a given table?
When the server returns the table during a table load, it already knows which table this is about and can therefore properly tell the client whether this table uses vended creds and whether those vended creds should be refreshable via a particular endpoint. The server can then either construct what you currently have in tableCredentials(..) or return a different endpoint.

The main point here is that only the server knows/controls what kind of credentials to use for a particular table and how/whether to refresh them.

Copy link
Contributor Author

@singhpk234 singhpk234 Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would the server not know the URI to refresh credentials for a given table?

This is because server (maybe Polaris) which might be sitting behind a series of micro-services post these hops when the call finally lands to server, it doesn't know which uri the client used to reach the server :
Lets take an example :
client supplies :

--conf spark.sql.catalog.<catalog_name>.uri=<sub-domain>.domain.tld/<random>

SDK creates the following URI before hitting LOAD table

<sub-domain>.domain.tld/<random>/v1/<prefix>/namespaces/<ns>/table/tbl

Server would need to create
now we need to know this uri in the server end to create the table level refresh-endpoint at server end
<sub-domain>.domain.tld/<random>/v1/<prefix>/namespaces/<ns>/table/<tbl>/credentials to the client so that client can use it a server just can't send v1/<prefix>/namespaces/<ns>/table/<tbl>/credentials because we need absolute uri, never the less this fixed, hence was requesting if we can have this injected in client end to support these use cases.

I agree that we can make it work with recreating the LoadTableResponse just before returning the response to the client.

or return a different endpoint.

we would inject this only when no refresh endpoint is supplied by server

The main point here is that only the server knows/controls what kind of credentials to use for a particular table and how/whether to refresh them.

Server can still send the refresh enabled and endpoint this is just for cases where server can't reconstruct it but just can say refresh is enabled and then expects the client to create this uri on their own.

Never the less would also help for client override which at the moment can just override it for one table for the whole catalog.

Copy link
Contributor Author

@singhpk234 singhpk234 Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was incorrect here the uri is indeed relative the path construction happens
9e895cb#diff-61702348e74dbbc58dca09591f0d015fe9e6f44d10cf7e2e88dba2b368d3b590R138

actually its relative to catalog uri - https://github.com/apache/iceberg/blame/main/aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java#L92

This would not require server knowing the uri at all which was the base of the problem barring the client side override doesn't work for more than 1 table.

// The IRC relative path for refreshing the credentials for a table is fixed.
if (s3RefreshEnabled && endpoints.contains(Endpoint.V1_TABLE_CREDENTIALS)) {
// respect server-side refresh endpoint configuration.
config.putIfAbsent("client.refresh-credentials-endpoint", paths.tableCredentials(identifier));
} else if (adlsRefreshEnabled && endpoints.contains(Endpoint.V1_TABLE_CREDENTIALS)) {
// respect server-side refresh endpoint configuration.
config.putIfAbsent("adls.refresh-credentials-endpoint", paths.tableCredentials(identifier));
}

// FileIO will use the credential provider which will use the refresh endpoint when
// the vended credentials from load table are close to expiration.
return tableFileIO(context, config, storageCredentials);
}

private FileIO tableFileIO(
SessionContext context, Map<String, String> config, List<Credential> storageCredentials) {
if (config.isEmpty() && ioBuilder == null && storageCredentials.isEmpty()) {
Expand Down
11 changes: 11 additions & 0 deletions core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,17 @@ public String table(TableIdentifier ident) {
RESTUtil.encodeString(ident.name()));
}

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

public String register(Namespace ns) {
return SLASH.join("v1", prefix, "namespaces", RESTUtil.encodeNamespace(ns), "register");
}
Expand Down