[OAuth2] Enable configurable early token refresh in Java Client by michaeljmarshall · Pull Request #13951 · apache/pulsar

michaeljmarshall · 2022-01-25T22:02:57Z

Motivation

In some client use cases, it is helpful to start attempting to refresh the OAuth2 token before it has expired. This PR adds that functionality.

The core addition is the ability to refresh the OAuth2 token in the background without affecting the current token. This will improve stability for OAuth2 users, especially if/when their IDP is unavailable.

Modifications

Add a ScheduledThreadPoolExecutor to the AuthenticationOAuth2 and schedule refresh token tasks to refresh a token before it has expired.
Ensure that the refreshing is thread safe.
Add tests to cover the new cases.
Add documentation.

Verifying this change

This PR includes new tests. There are also existing tests that cover some of the changes.

Does this pull request potentially affect one of the following parts:

This PR changes the default behavior of the AuthenticationOAuth2 class and it also adds a new configuration value for end users. By default, it does not change the current behavior.

Documentation

doc

I added docs.

eolivelli

Isn't creating a new thread per each client instance too heavyweight?
Is there a way to grab the timer from the client?
There are systems that need to create multiple clients (like the Pulsar proxy did)

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2.java

lhotari

The lifecycle of the scheduler/executor will need to be managed. This solution would lead to resource leaks. I looked into ways how to possible solve this problem, but it's not easy. The main reason seems to be that the current API for OAuth2 in the Pulsar client is not great. I didn't find a way to extend it without breaking backwards compatibility. I think it's necessary to redesign the Pulsar client's API for configuring OAuth2 and take these requirements into account. It's more or less a blocker for making progress.

I think that the way to specify the refresh interval by using the "early_token_refresh_percent" configuration parameter is not well suited for all use cases. Instead of that, it might be better to have ways to express boundaries using 3 different parameters. There are 2 alternative ways to express this: one approach would be to consider it to be a "soft expiration" where the expiration is limited by the parameters, another approach is to think of it as a "refresh interval".

Let's say that the thinking is around "refresh interval", there could be 3 parameters

"refresh before expiration duration"
- for example, with the value 600 seconds, will start refreshing the token 10 minutes before it expires
  In addition, these parameters could be used to set the boundaries for the value calculated based on the "refresh before expiration duration" parameter:
"minimum refresh interval"
"maximum refresh interval"

michaeljmarshall · 2022-02-08T21:28:35Z

@lhotari - I agree that we need to take care of the life cycle of the thread used for these refresh token commands. In my most recent commit, I changed the behavior so that the application can either supply an executor, or the OAuth2 class will create one (assuming that the feature is enabled, which it is not by default). The one downside here is that this means the feature is not available for brokers, because they create the AuthenticationOAuth2 class via reflection, and the configure method only takes strings. I think we should consider updating how the client authentication providers are configured and then it will probably be easier to use this feature when loading via reflection.

I think that the way to specify the refresh interval by using the "early_token_refresh_percent" configuration parameter is not well suited for all use cases.

Would you mind clarifying this use case with an example? On one hand, I accept that percent may be slightly less straight forward, but on the other hand, I think it handles certain edge cases really well. For example, each retrieved token can have a unique expires_in value. A percentage naturally handles this variability. Further, there is no guarantee that the min and max refresh interval that you described will fall within the expires_in value. Let me know what you think.

lhotari · 2022-02-09T16:05:02Z

...ent/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2Builder.java

+     * Set the {@link ClientCredentialsConfiguration} when using the OAuth2 client credentials flow.
+     * @return builder
+     */
+    public AuthenticationOAuth2Builder setClientCredentialsConfiguration(


In Pulsar, builders usually don't use set* naming convention. it would be good to follow a similar naming convention that has been used in other parts of Pulsar. for example, rename setClientCredentialsConfiguration -> clientCredentialsConfiguration.

lhotari · 2022-02-09T16:05:22Z

...ent/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2Builder.java

+     *                                 {@link AuthenticationOAuth2} for details.
+     * @return builder
+     */
+    public AuthenticationOAuth2Builder setEarlyTokenRefreshPercent(double earlyTokenRefreshPercent) {


drop set from the method name

lhotari · 2022-02-09T16:05:31Z

...ent/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2Builder.java

+     *                  {@link AuthenticationOAuth2} will not close it.
+     * @return builder
+     */
+    public AuthenticationOAuth2Builder setEarlyTokenRefreshExecutor(ScheduledThreadPoolExecutor scheduler) {


lhotari · 2022-02-09T16:06:13Z

...ent/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2Builder.java

+
+    public Authentication build() {
+        if (clientCredentialsConfiguration == null) {
+            throw new IllegalArgumentException("ClientCredentialsConfiguration must be set.");


IllegalStateException instead?

lhotari · 2022-02-09T16:13:24Z

...ar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/ClientCredentialsFlow.java


    private final String audience;
-    private final String privateKey;
+    private final String keyFileUrl;


keyFileUrl seems repetitive and conflicting. Is it a File or an URL? What key is it? Perhaps keeping the name privateKey and switching to use types would improve clarity. I'd recommend java.net.URI class also for URLs. that would make this URI privateKey.

lhotari · 2022-02-09T16:14:19Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2.java


    public static final String CONFIG_PARAM_TYPE = "type";
    public static final String TYPE_CLIENT_CREDENTIALS = "client_credentials";
+    public static final int EARLY_TOKEN_REFRESH_PERCENT_DEFAULT = 1; // feature disabled by default


I'll reply separately to the earlyTokenRefreshPercent question

lhotari · 2022-02-09T16:31:27Z

I think that the way to specify the refresh interval by using the "early_token_refresh_percent" configuration parameter is not well suited for all use cases.

Would you mind clarifying this use case with an example? On one hand, I accept that percent may be slightly less straight forward, but on the other hand, I think it handles certain edge cases really well. For example, each retrieved token can have a unique expires_in value. A percentage naturally handles this variability. Further, there is no guarantee that the min and max refresh interval that you described will fall within the expires_in value. Let me know what you think.

The use case would be that you want to maximize the time that a token is active at all times. The reason to do this is that if the identity provider becomes unavailable, there is less risk the whole system becomes unavailable because of the identity provider being unavailable. The system will tolerate a long downtime when the tokens are always active for the maximum period of time. The tradeoff is that there will be more token refreshes made towards the identity provider.

You might want to refresh the token every 10 minutes regardless of the validity of the token, given that the token is valid for more than 10 minutes. The solution I proposed wasn't probably the best way to address this use case.

Another person might want to refresh the token 10 minutes before the validity of the token expires, regardless of how long the token is valid.

For the above usecases, I would assume that configuring the solution would be possible with 2 parameters (naming is hard, so the names are still bad):
"refresh before expiration duration"
"minimum token usage duration"

The problem with the 10 minutes before token expires example is the case where the token is valid for less than 10 minutes. That's why it is necessary to have a second parameter to define how long the token is kept used until a refresh is made.

Let's say that the user has set "refresh before expiration duration" to 10 minutes and "minimum token usage duration" to 2 minutes. If the returned token is valid for 10 minutes, it would start the refresh after 2 minutes.

I simply don't see a use case for the earlyTokenRefreshPercent way of configuring the refresh scheduling. In the requirement we have, it's about exact times instead of percentages. I think that percentages would be confusing for users. "refresh before expiration duration"
and "minimum token usage duration" parameters would be easier to understand.

michaeljmarshall · 2022-02-09T17:44:18Z

@lhotari - thanks for clarifying your point. I hadn't thought of the use case where a user has a refresh period that is independent of the token's time to live. Of the two parameters, which should take precedence? I think we should use the minimum duration resulting from either minimum token usage duration or refresh before expiration duration. The one case that is not covered is when the token's time to live is shorter than these two parameters. We'll need a default behavior in that case. This is where a percentage would be valuable. The current code attempts a refresh when 90% of the tokens time to live has passed. We could use that percent, or use a conservative 50%. How do you think we should handle this edge case?

github-actions · 2022-03-12T01:50:29Z

The pr had no activity for 30 days, mark with Stale label.

github-actions · 2022-05-28T02:12:02Z

The pr had no activity for 30 days, mark with Stale label.

dave2wave · 2023-07-16T19:21:54Z

@michaeljmarshall This PR is likely beyond stale. Would you please close.

lhotari · 2026-03-19T19:21:29Z

I took over completing this PR. However, I couldn't push changes to the PR branch so I opened a new PR #25363

github-actions bot assigned michaeljmarshall Jan 25, 2022

github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jan 25, 2022

michaeljmarshall added area/authn type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Jan 25, 2022

[OAuth2] Enable configurable preemptive token refresh in Java Client

012d641

michaeljmarshall force-pushed the preemptive-token-refresh branch from 54db563 to 012d641 Compare January 25, 2022 22:16

eolivelli requested changes Jan 26, 2022

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2.java Outdated Show resolved Hide resolved

michaeljmarshall mentioned this pull request Jan 27, 2022

Make Backoff class thread-safe #13993

Closed

1 task

michaeljmarshall added 4 commits January 28, 2022 15:27

Use single thread; improve naming; improve thread safety; add docs

af7100b

Add isClosed logic to authentication class

c63c37c

Avoid NPE when early refresh not enabled

cbd1a4f

Minor improvements to javadocs and threadsafety; add tests

bc57b7a

michaeljmarshall changed the title ~~[OAuth2] Enable configurable preemptive token refresh in Java Client~~ [OAuth2] Enable configurable early token refresh in Java Client Feb 4, 2022

Update client documentation

087a8fd

michaeljmarshall marked this pull request as ready for review February 4, 2022 06:28

michaeljmarshall requested review from eolivelli, jiazhai, lhotari and merlimat February 4, 2022 06:28

lhotari reviewed Feb 4, 2022

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2.java Outdated Show resolved Hide resolved

lhotari requested changes Feb 4, 2022

View reviewed changes

User builders; prevent thread leak

2d15f21

lhotari reviewed Feb 9, 2022

View reviewed changes

github-actions bot added the lifecycle/stale label Mar 12, 2022

Technoboy- added this to the 3.2.0 milestone Jul 31, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

lhotari mentioned this pull request Mar 5, 2024

[Enhancement] authentication refresh support for mTLS auth #22125

Open

2 tasks

damienburke mentioned this pull request Mar 20, 2024

[JWT] Enable configurable early token refresh in Java Client #22314

Open

2 tasks

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari added the triage/lhotari/important lhotari's triaging label for important issues or PRs label Oct 14, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

coderzc modified the milestones: 4.1.0, 4.2.0 Sep 1, 2025

lhotari mentioned this pull request Mar 19, 2026

[improve][client] Enable configurable preemptive OAuth2 token refresh #25363

Open

1 task

lhotari closed this Mar 19, 2026

Conversation

michaeljmarshall commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall commented Feb 8, 2022

Uh oh!

lhotari Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari commented Feb 9, 2022

Uh oh!

michaeljmarshall commented Feb 9, 2022

Uh oh!

github-actions bot commented Mar 12, 2022

Uh oh!

github-actions bot commented May 28, 2022

Uh oh!

dave2wave commented Jul 16, 2023

Uh oh!

lhotari commented Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaeljmarshall commented Jan 25, 2022 •

edited

Loading