Skip to content
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

CORE: Inject OAuth2 Token from TableSession #12635

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

CORE: Inject OAuth2 Token from TableSession #12635

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5545fd
inject token to tableConf from tableSession
Mar 25, 2025
ec5bfe1
spotless
Mar 25, 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
6 changes: 6 additions & 0 deletions 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 @@ -65,6 +65,8 @@
import org.apache.iceberg.rest.auth.AuthManager;
import org.apache.iceberg.rest.auth.AuthManagers;
import org.apache.iceberg.rest.auth.AuthSession;
import org.apache.iceberg.rest.auth.OAuth2Properties;
import org.apache.iceberg.rest.auth.OAuth2Util;
import org.apache.iceberg.rest.requests.CommitTransactionRequest;
import org.apache.iceberg.rest.requests.CreateNamespaceRequest;
import org.apache.iceberg.rest.requests.CreateTableRequest;
Expand Down Expand Up @@ -414,6 +416,10 @@ public Table loadTable(SessionContext context, TableIdentifier identifier) {
authManager.tableSession(finalIdentifier, tableConf, contextualSession);
TableMetadata tableMetadata;

if (tableSession instanceof OAuth2Util.AuthSession) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I fully follow why this change is needed. If a REST server supports a sub-scoped table token, then the server would return this via the token field in LoadTableResponse#config and so you would end up having a properly configured tableSession (which already takes in the tableConf with the potentially sub-scoped table token).

Copy link
Contributor Author

@wolflex888 wolflex888 Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Since we are already getting a tableSession that's dedicated to interact with the table from the REST server. will it make more sense to pass it down to VendedCredentialsProvider instead of expecting LoadTableResponse.config() to include that property? This is just a proposal, I'm still trying to figure out how iceberg should interact with a REST server, Apache Polaris in our case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are already getting a tableSession that's dedicated to interact with the table from the REST server. will it make more sense to pass it down to VendedCredentialsProvider instead of expecting LoadTableResponse.config() to include that property?

this is already being done today in

iceberg/aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java

Lines 203 to 206 in 7d0395d

Optional.ofNullable(allProperties.get(OAuth2Properties.TOKEN))
.ifPresent(
token ->
clientCredentialsProviderProperties.putIfAbsent(OAuth2Properties.TOKEN, token));

Copy link
Contributor Author

@wolflex888 wolflex888 Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

right. but when loadTable is called, the token from the tableSession does not get included in the property that initiates tableFileIO. This PR is proposing to include that token in the properties so the VendedCredentialsProvider can use it to initiate the http client.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wolflex888 I'm not sure I'm following here. The oauth token that is potentially sub-scoped to the loaded table is passed to the Table FileIO instance in https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L973-L980

tableConf.put(OAuth2Properties.TOKEN, ((OAuth2Util.AuthSession) tableSession).token());
}

if (snapshotMode == SnapshotMode.REFS) {
tableMetadata =
TableMetadata.buildFrom(response.tableMetadata())
Expand Down
Loading