Auth Manager API part 3: OAuth2 Manager by adutra · Pull Request #11844 · apache/iceberg

adutra · 2024-12-21T10:09:46Z

3rd PR for the Auth Manager API. Previous ones:

This PR introduces the OAuth2Manager along with session caching and credentials refreshing. It is still "unplugged" though. Most of the OAuth2Manager code reflects one-to-one what's currently hard-coded in RESTSessionCatalog (and it's still there).

\cc @nastra @danielcweeks

johnnysohn · 2024-12-28T06:41:48Z

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

+        impl = AuthProperties.AUTH_MANAGER_IMPL_OAUTH2;
+        break;
      default:
        impl = authType;


can we also define something like AUTH_IMPL = "rest.auth.type"; that could be used here for any custom auth manager/provider we want to bring in?

That's precisely the intent here; if you have a custom manager, you would activate it with rest.auth.type=my.custom.AuthManager. Is that OK for you?

Yes that works! I wasn't sure whether you were planning on having authType for as a name of the type

There will be aliases for built-in managers, e.g. you can activate the built-in oauth2 manager with either:

rest.auth.type = oauth2 rest.auth.type = org.apache.iceberg.rest.auth.OAuth2Manager

But for custom ones, you must provide the FQDN of your implementation.

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

danielcweeks · 2025-01-09T23:06:10Z

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

+   * @param executor the executor to use for eviction tasks; if null, the cache will use the
+   *     {@linkplain ForkJoinPool#commonPool() common pool}. The executor will not be closed when
+   *     this cache is closed.
+   * @param nanoTimeSupplier the supplier for nano time; if null, the cache will use {@link


Is there a reason we would need to expose this?

No, not really, I can turn this constructor into package-private. It's meant only for tests.

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

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

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

core/src/test/java/org/apache/iceberg/rest/auth/TestAuthSessionCache.java

nastra

LGTM

danielcweeks · 2025-01-10T17:10:11Z

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

+   * @param sessionTimeout the session timeout. Sessions will become eligible for eviction after
+   *     this duration of inactivity.
+   */
+  public AuthSessionCache(Duration sessionTimeout) {


It looks like we should also make this package private. The other issue is that we shouldn't be using the common pool by default. This should probably be provided by the OAuth2Manager or if we expect there to be only one per manager, then just create a named pool for handling refreshes. We definitely don't want to rely on or possibly tie up the common pool.

@danielcweeks we are already using the common pool. The session cache currently is created as follows:

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

Lines 1144 to 1154 in a2b8008

return Caffeine.newBuilder()

.expireAfterAccess(Duration.ofMillis(expirationIntervalMs))

.removalListener(

(RemovalListener<String, AuthSession>)

(id, auth, cause) -> {

if (auth != null) {

auth.stopRefreshing();

}

})

.build();

}

As you can see, we are not providing an executor explicitly, so evictions are being done on the common pool.

Re: turning this constructor package-private: this won't fly, as SigV4 will also need to create caches, so this constructor needs to be public.

@danielcweeks I searched for all Caffeine caches in Iceberg's repo and found 25 occurrences. Only one occurrence sets the executor, the one in CachingCatalog:

iceberg/core/src/main/java/org/apache/iceberg/CachingCatalog.java

Line 112 in f7d40f0

.executor(Runnable::run) // Makes the callbacks to removal listener synchronous

Consequently, all the others are using the common pool for asynchronous tasks such as eviction.

@danielcweeks do you maintain that you want me to use a different pool here?

Yes, we previously had refresh in a dedicated threadpool, so I don't think the other caffeine examples are what we should point to.

Also, we don't want to rely on the common pool for something important like token refresh. If something ties up the thread pool, we may miss the window to refresh. Also, we don't want to tie up the thread pool with refresh threads if something goes wrong.

This is not about token refresh, this is about auth session eviction.

But fair enough. I'll do it.

danielcweeks · 2025-01-10T17:17:49Z

@adutra Just one last comment on the thread pool, but other than that this look good to go.

adutra · 2025-01-15T17:42:13Z

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

+            .setNameFormat(name + "-auth-session-evict-%d")
+            .setDaemon(true)
+            .build();
+    return Executors.newCachedThreadPool(threadFactory);


I think this is the best executor for this kind of task, as we don't want a core thread kept alive if there isn't anything to do.

We should be using a dedicated pool from our ThreadPools utility. I would suggest: ThreadPools::newExitingWorkerPool(String namePrefix, int poolSize) with a named pool like we had previously.

OK, switched to ThreadPools.newExitingWorkerPool.

github-actions bot added the core label Dec 21, 2024

adutra force-pushed the auth-manager-3 branch 2 times, most recently from 6c3453c to e502b8b Compare December 23, 2024 12:22

Auth Manager API part 3: OAuth2 Manager

b88d99d

adutra force-pushed the auth-manager-3 branch from e502b8b to b88d99d Compare December 23, 2024 12:29

johnnysohn reviewed Dec 28, 2024

View reviewed changes

dimas-b mentioned this pull request Dec 30, 2024

Security improvements in the Iceberg REST specification #10537

Open

6 tasks

adutra mentioned this pull request Jan 7, 2025

Implement Human OAuth2 Flows for OAuth2Manager #10677

Closed

danielcweeks reviewed Jan 9, 2025

View reviewed changes

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

danielcweeks reviewed Jan 9, 2025

View reviewed changes

nastra reviewed Jan 10, 2025

View reviewed changes

review

6bceebf

nastra approved these changes Jan 10, 2025

View reviewed changes

danielcweeks reviewed Jan 10, 2025

View reviewed changes

review: eviction executor

33017a0

adutra commented Jan 15, 2025

View reviewed changes

adutra added 4 commits January 15, 2025 19:01

checkstyle

42161ec

checkstyle

31acb12

use ThreadPools.newExitingWorkerPool

d345ad0

fix test

2ac23cc

danielcweeks approved these changes Jan 16, 2025

View reviewed changes

danielcweeks merged commit a0777bc into apache:main Jan 16, 2025
46 checks passed

adutra deleted the auth-manager-3 branch January 16, 2025 17:07

This was referenced Jan 17, 2025

Auth Manager API part 4: RESTClient, HTTPClient #11992

Merged

Auth Manager API part 5: SigV4 Auth Manager #11995

Merged

adutra mentioned this pull request Feb 7, 2025

AWS, Core, GCP: Auth Manager API enablement #12197

Merged

adutra mentioned this pull request Jul 2, 2025

Core/REST: generify AuthSessionCache #12562

Closed

	return Caffeine.newBuilder()
	.expireAfterAccess(Duration.ofMillis(expirationIntervalMs))
	.removalListener(
	(RemovalListener<String, AuthSession>)
	(id, auth, cause) -> {
	if (auth != null) {
	auth.stopRefreshing();
	}
	})
	.build();
	}

Conversation

adutra commented Dec 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra Jan 15, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcweeks commented Jan 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcweeks Jan 15, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adutra Jan 15, 2025 •

edited

Loading

danielcweeks Jan 15, 2025 •

edited

Loading