Feature make oauth `audience` configurable by himadripal · Pull Request #9839 · apache/iceberg

himadripal · 2024-02-29T19:47:53Z

himadripal · 2024-02-29T19:47:59Z

Although the issue discusses about making audience configurable but there are few other optional parameter exists in the specification (i.e resource,actor_token etc. ) which are not configurable right now. This is an attempt to make all optional OAuth params configurable. Idea is if you have those optional parameters configured in properties, we build map out of it, picking only optional params from the properties and pass it along.
scope is also an optional parameter, I think it can also be moved to using this map.
@nastra @rdblue @syun64 Please take a look and provide feedback. @RussellSpitzer @flyrain as well

himadripal · 2024-03-03T18:51:54Z

Can I get someone's attention for a review on this one please?

flyrain · 2024-03-04T18:47:27Z

Thanks @himadripal for working on it. Let me take a look.

flyrain

Thanks @himadripal working on it. LGTM overall. Left some comments.

flyrain · 2024-03-04T22:49:58Z

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

    ConfigResponse config;
    OAuthTokenResponse authResponse;
    String credential = props.get(OAuth2Properties.CREDENTIAL);
+    // CONSIDER : putting scope in optional param map - reduce wiring on scope


Ideally, we should have a Java class synced with the OAuthClientCredentialsRequest in Rest spec. It'd be nice to pass around an object of the class. The class itself can even be autogenerated. Also by doing it this way, the Java code is always synced with spec. And every time we made a change like this, only the spec needs to be changed. cc @nastra

However, that's fairly a big change. We will potentially need to deprecate some methods. Another option is to put every optional parameter(including scope, token type) in a map at this PR, then refactor it later.

WDYT? Also cc @danielcweeks @nastra @syun64

We will also need an update on spec for these optional parameters, but I'm OK with another PR.

Ideally, we should have a Java class synced with the OAuthClientCredentialsRequest in Rest spec

A java class for all optional params is good suggestion, that way, we can implement individual parameter level default and validation if needed.

However, that's fairly a big change. We will potentially need to deprecate some methods. Another option is to put every optional parameter(including scope, token type) in a map at this PR, then refactor it later.

Yes, this will need changes in public API and deprecate some methods. I tried putting scope in the map and some of the tests were failing, I'll take a look again. How about we put all the optional param mentioned here in the map including scope and I'll create an issue for removing scope as individual param.

Looking a bit more. The second option(putting the scope into the optional params) also needs to deprecate several methods, e.g., exchangeToken(), fetchToken(), etc, since their signatures will change. I think it's not worthy to do that now given that we will refactor later to use a OAuthClientCredentialsRequest object. Let's move forward with this approach.

flyrain · 2024-03-04T23:29:15Z

aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java

+                          ImmutableMap.of(),
+                          token,
+                          null,
+                          credential(),
+                          SCOPE,
+                          oauth2ServerUri(),
+                          optionalOAuthParams())));


Can we extract this part so that the following one at line 231 can reuse it?

one in line 231, do not have token provided (token = null), we can still create a private method with token as param, but this one looks more readable. WDYT?

himadripal · 2024-03-06T19:49:33Z

Does all of these optional parameters need to be added in the OAuthTokenResponse as well? @flyrain

flyrain

+1. Thanks @himadripal for working on it. As a followup, would you mind create issues?

Adding audience, resource in the rest spec.
Enriching Rest catalog configure doc with more options, in spark-configuration.md, we usually use the spark as the reference implementation, so it's OK to only change the spark side.

flyrain · 2024-03-08T00:52:40Z

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

    ConfigResponse config;
    OAuthTokenResponse authResponse;
    String credential = props.get(OAuth2Properties.CREDENTIAL);
+    // CONSIDER : putting scope in optional param map - reduce wiring on scope


Looking a bit more. The second option(putting the scope into the optional params) also needs to deprecate several methods, e.g., exchangeToken(), fetchToken(), etc, since their signatures will change. I think it's not worthy to do that now given that we will refactor later to use a OAuthClientCredentialsRequest object. Let's move forward with this approach.

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java

nastra · 2024-03-08T08:42:12Z

overall I think adding these optional OAuth2 params make sense to me. I'll also link to the OAuth2 spec for other reviewers to better understand what those optional parameters do.
I think we should decide how we want to deal with AuthSession configuration paramters as it causes lots of changes to add a single parameter to AuthSession.

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java

…nal parameters

…ast to dedup keys restore mockito.argThat

fix breaking api changes

flyrain · 2024-03-11T21:09:14Z

+1 Thanks @himadripal for the PR. Thanks @nastra for the review.

github-actions bot added core AWS labels Feb 29, 2024

flyrain reviewed Mar 5, 2024

View reviewed changes

flyrain approved these changes Mar 8, 2024

View reviewed changes

nastra reviewed Mar 8, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java Outdated Show resolved Hide resolved

nastra reviewed Mar 8, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java Show resolved Hide resolved

nastra reviewed Mar 8, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java Show resolved Hide resolved

himadripal commented Mar 8, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java Show resolved Hide resolved

hpal added 9 commits March 11, 2024 11:54

make audience configurable, also make arrangement to pass all optio…

ea72231

…nal parameters

refreshToken also needs audience, add tests

0f72cc1

fix imports

bd908d9

make sure no breaking public api changes.

7f4556c

create a helper method to build optional param map from properties

effc916

add more comments, convert TODO to comment.

0cc33c9

style fix

23e7452

remove scope as it is already being populated, fixed tests.

3d4987b

add scope as well in the optionalOAuthParam map and use buildKeepingL…

523a6ea

…ast to dedup keys restore mockito.argThat

flyrain mentioned this pull request Mar 11, 2024

feat: implement OAuth for catalog rest client apache/iceberg-rust#254

Merged

adress review comments and address review comments

47da67a

fix breaking api changes

himadripal force-pushed the feature_make_oauth_audiance_configurable branch from ee89988 to 47da67a Compare March 11, 2024 19:29

flyrain merged commit 5f655a3 into apache:main Mar 11, 2024

tomtongue pushed a commit to tomtongue/iceberg that referenced this pull request Mar 12, 2024

Make OAuth audience and resource configurable (apache#9839)

aa49c5e

nastra mentioned this pull request Apr 17, 2024

Core: Introduce AuthConfig #10161

Merged

bk-mz mentioned this pull request Jun 7, 2024

Improve MetricsReporter loading with class loader fallback bk-mz/iceberg#1

Closed

szehon-ho pushed a commit to szehon-ho/iceberg that referenced this pull request Sep 16, 2024

Make OAuth audience and resource configurable (apache#9839)

b204803

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Make OAuth audience and resource configurable (apache#9839)

8ad7a69

nastra mentioned this pull request Aug 1, 2025

AWS: Fix OAuth2 additional params inclusion #13718

Merged

Conversation

himadripal commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

himadripal commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

himadripal commented Mar 3, 2024

Uh oh!

flyrain commented Mar 4, 2024

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

flyrain Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

himadripal Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

flyrain Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

himadripal Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himadripal commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nastra commented Mar 8, 2024

Uh oh!

Uh oh!

flyrain commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

himadripal commented Feb 29, 2024 •

edited

Loading

himadripal commented Feb 29, 2024 •

edited

Loading

himadripal Mar 6, 2024 •

edited

Loading

himadripal Mar 6, 2024 •

edited

Loading

himadripal commented Mar 6, 2024 •

edited

Loading