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.

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

Conversation

wolflex888
Copy link

Originally, we are expecting loadTable from the REST Service to return token as part of the config in LoadTableResponse. The token should have access to the table the application is interacting with.

In this PR, I am proposing to inject the token from tableSession that's created within RestSessionCatalog.loadTable. This make sense since RestSessionCatalog already make a request to the REST service for a token that is dedicated to interact with the table. We can use it to interact with the server instead of getting another one from the endpoint.

@github-actions github-actions bot added the core label Mar 25, 2025
@@ -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
Author

@wolflex888 wolflex888 Mar 26, 2025

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

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

Copy link
Author

@wolflex888 wolflex888 Mar 26, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants