Core: Support usage of Separate Authorization Server URI in Rest Catalog #8976
Conversation
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Properties.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
|
Incorporated suggested changes @nastra . Thank you for the reviews! |
|
Leaving a comment to keep the PR active - @nastra @danielcweeks |
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
Thanks @syun64 for working on this. LGTM overall. Adding minor comments.
| this.oauth2ServerUri = oauth2ServerUri; | ||
| } | ||
|
|
||
| public AuthSession( |
There was a problem hiding this comment.
Looks like it is not used anywhere. Should we deprecate it? If that's so, can we add deprecation comment?
There was a problem hiding this comment.
I noticed that too. I lack the context to make that decision, so I will leave this comment thread for others to opine
There was a problem hiding this comment.
It doesn't appear to be referenced anymore so you should add a deprecation warning and message (see deprecation docs)
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Properties.java
Outdated
Show resolved
Hide resolved
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
LGTM overall. Added minor comments. Thanks @syun64 for working on it.
| this.oauth2ServerUri = oauth2ServerUri; | ||
| } | ||
|
|
||
| public AuthSession( |
| return properties().get(OAuth2Properties.CREDENTIAL); | ||
| } | ||
|
|
||
| /** Token endpoint URI to fetch token from if the Rest Catalog is not the authorization server. */ |
There was a problem hiding this comment.
"Token endpoint URI" -> "OAuth Server URI" or just "Endpoint"?
|
LGTM - Thanks for adding this! I look forward to using it 🚀 |
| throw new RESTException("Unhandled error: %s", errorResponse); | ||
| } | ||
|
|
||
| private String buildPath(String path) { |
There was a problem hiding this comment.
This is looking a lot cleaner now. However, we've now reduced this to a single call in execute where we chain buildUri(buildPath(path). This can just be collapsed to a single method since they're both private and only called in one spot.
There was a problem hiding this comment.
Made the suggested change @danielcweeks . Thank you for the review!
danielcweeks
left a comment
danielcweeks
left a comment
There was a problem hiding this comment.
LGTM, just waiting on checks
sungwy
changed the title
* configurable token-uri * more tests * lint * minor changes * suppress style * address feedback * adopt review feedback * update config name * lint * adopt review comments
* configurable token-uri * more tests * lint * minor changes * suppress style * address feedback * adopt review feedback * update config name * lint * adopt review comments
* configurable token-uri * more tests * lint * minor changes * suppress style * address feedback * adopt review feedback * update config name * lint * adopt review comments
5 participants
Related: #8869
Introduce
OAuth2Properties.TOKEN_URI = "token-uri"that can be used to override the default behavior that assumes that the Rest Catalog Server is an Auth Server, and that the token endpoint is provided on the Rest Catalog Server.If this value is set, fetchToken and exchangeToken calls will use this endpoint instead to retrieve a valid access token.
HttpClient.execute call has been modified to submit requests against the provided string, if the value is a valid https uri. If not, it will fallback to the previous behavior and attempt to construct the baseUri against the config provided catalog uri.
All tests requiring checks against "v1/oauth/tokens" endpoint has been updated to be parameterized with a configurable endpoint argument, to make sure that all token calls are routed to the separate URI.