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: ability to create REST catalog with external AuthManager #12655

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adutra
Copy link
Contributor

@adutra adutra commented Mar 26, 2025

I am proposing this change because It seems that it would benefit Trino devs (see #12363 (comment)), and I just received yet another request for this feature, this time coming from Apache Polaris devs.

@github-actions github-actions bot added the core label Mar 26, 2025
@gh-yzou
Copy link

gh-yzou commented Mar 26, 2025

@adutra Thanks a lot for putting on this PR! Yes, have a way to inject the AuthManager would be very useful for Polaris Spark client.

Polaris provides native support for Iceberg APIs, and it also provides set of APIs that is owned by Polaris. The Spark plugin provided will help Spark talk to Polaris service for both Iceberg APIs and Polaris specific APIs, and we want to be able to use the same RESTClient and Authentication to talk to all those endpoints. In that way, it could help us to save the extra token refresh overhead and maximize the reuse at client side.

This PR helps providing a way to inject a authManager, which could be helpful for us to reuse the authentication across all endpoints. @nastra I am wondering what do you think about this ?

@ajantha-bhat ajantha-bhat added this to the Iceberg 1.9.0 milestone Mar 27, 2025
@nastra
Copy link
Contributor

nastra commented Mar 27, 2025

setting a custom auth manager impl should already be possible today by providing rest.auth.type = my.custom.AuthManagerImpl so not sure I follow why we need to add some additional constructors?

@adutra
Copy link
Contributor Author

adutra commented Mar 27, 2025

It's not a custom vs built-in problem, it's a shared vs non-shared problem.

Consider this hypothetical example: let's suppose we have a custom manager that authenticates using OAuth, Okta and the Authorization Code flow. This means that the user will be redirected to their browser for login before being able to use the catalog. If this is happening in a Spark or Trino session, chances are that Spark/Trino will need a token, and the catalog will also need a token. Without the ability to share the same auth manager, the user would be asked to login twice: once for Spark/Trino, once for the RESTCatalog.

@danielcweeks
Copy link
Contributor

danielcweeks commented Mar 28, 2025

@adutra can you please update the description to indicate what changes are proposed and how this is intended to be used. The scenario isn't entirely clear for the Polaris case or what exactly Trino is running into. The comment you linked even states:

So looks like the problem is outside of iceberg but in TrinoRestCatalog

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.

5 participants