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
feat: enable pre-emptive async oauth token refreshes #646
feat: enable pre-emptive async oauth token refreshes #646
Conversation
This is currently a rough sketch and should not be merged. I just wanted to get some feedback here. The current implementation of oauth refresh offloads the IO to a separate executor, however when a token expires, all calls to getRequestMetadata would hang until the token is refreshed. This PR is a rough sketch to improve the situation by adding a stale state to token. If a token is within a minute of expiration, the first request to notice this, will spawn a refresh on the executor and immediately return with the existing token. This avoids hourly latency spikes for grpc. The implementation uses guava's ListenableFutures to manage the async refresh state. Although the apis are marked BetaApi in guava 19, they are GA in guava 30 * The class introduces 3 distinct states: * Expired - the token is not usable * Stale - the token is useable, but its time to refresh * Fresh - token can be used without any extra effort * All of the funtionality of getRequestMetadata has been extracted to asyncFetch. The new function will check if the token is unfresh and schedule a refresh using the executor * asyncFetch uses ListenableFutures to wrap state: if the token is not expired, an immediate future is returned. If the token is expired the future of the refresh task is returned * A helper refreshAsync & finishRefreshAsync are also introduced. They schedule the actual refresh and propagate the side effects * To handle blocking invocation: the async functionality is re-used but a DirectExecutor is used. All ExecutionErrors are unwrapped. In most cases the stack trace will be preserved because of the DirectExecutor. However if the async & sync methods are interleaved, it's possible that a sync call will await an async refresh task. In this case the callers stacktrace will not be present.
…mplemented on top of 0.8, so now I'm backporting features to minimize the merge conflicts
Codecov Report
@@ Coverage Diff @@
## master #646 +/- ##
============================================
- Coverage 83.59% 83.23% -0.36%
Complexity 606 606
============================================
Files 42 42
Lines 2712 2797 +85
Branches 289 299 +10
============================================
+ Hits 2267 2328 +61
- Misses 303 320 +17
- Partials 142 149 +7
Continue to review full report at Codecov.
|
…and allow subsequent callers to use the stale token
@@ -208,6 +210,27 @@ private ImpersonatedCredentials initializeImpersonatedCredentials() { | |||
.build(); | |||
} | |||
|
|||
@Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewer: before this PR, the blocking version getRequestMetadata() was the primary codepath, so ExternalAccountCredentials only had to override that method. This subclass expected that the async version would just call the sync version. Which I think is a poor assumption since its an implementation detail. I corrected the behavior by overriding both.
Arguably a better approach is to make both getRequestMetadata final and have subclasses use asyncFetch() as the source of truth, but I didn't want to leak guava to the surface of this library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, lets clarify if refresh slight behavior change is blocking.
Some coverage of refreshTask would be great.
oauth2_http/javatests/com/google/auth/oauth2/OAuth2CredentialsTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing! I think addressed all of your comments, PTAL
Hey @igorbernstein2 ! Thx a lot for the PR, from my side everything looks good. Looking forward to testing this in the next bigtable client release :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but please let's make sure @silvolu also take a look before merge.
I'm reviewing this w/ @Neenu1995 and am just getting started, but given that you mention that you are potentially changing behavior, I wonder if there is a way to bring this change back into Google3 and run all the tests that blaze will invoke to see if this is safe? |
I created cl/373364841, I had to hand edit a couple of irrelevant files:
I requested a global tap presubmit Also, I updated the codestyle suggestions from critique |
🤖 I have created a release \*beep\* \*boop\* --- ## [0.26.0](https://www.github.com/googleapis/google-auth-library-java/compare/v0.25.5...v0.26.0) (2021-05-20) ### Features * add `gcf-owl-bot[bot]` to `ignoreAuthors` ([#674](https://www.github.com/googleapis/google-auth-library-java/issues/674)) ([359b20f](https://www.github.com/googleapis/google-auth-library-java/commit/359b20f24f88e09b6b104c61ca63a1b604ea64d2)) * added getter for credentials object in HttpCredentialsAdapter ([#658](https://www.github.com/googleapis/google-auth-library-java/issues/658)) ([5a946ea](https://www.github.com/googleapis/google-auth-library-java/commit/5a946ea5e0d974611f2205f468236db4b931e486)) * enable pre-emptive async oauth token refreshes ([#646](https://www.github.com/googleapis/google-auth-library-java/issues/646)) ([e3f4c7e](https://www.github.com/googleapis/google-auth-library-java/commit/e3f4c7eac0417705553ef8259599ec29fc8ad9b4)) * Returning an issuer claim on request errors ([#656](https://www.github.com/googleapis/google-auth-library-java/issues/656)) ([95d70ae](https://www.github.com/googleapis/google-auth-library-java/commit/95d70ae0f5f4c985455f913ddef14ebe75500656)) ### Bug Fixes * use orginal url as audience for self signed jwt if scheme or host is null ([#642](https://www.github.com/googleapis/google-auth-library-java/issues/642)) ([b4e6f1a](https://www.github.com/googleapis/google-auth-library-java/commit/b4e6f1a0bd17dd31edc85ed4879cea75857fd747)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
…3228) Now that auth library bundles [pre-emptive refresh](googleapis/google-auth-library-java#646), RefreshingOAuth2CredentialsInterceptor can be removed. Also, remove retry on UNAUTHENTICATED
…oogleapis#3228) Now that auth library bundles [pre-emptive refresh](googleapis/google-auth-library-java#646), RefreshingOAuth2CredentialsInterceptor can be removed. Also, remove retry on UNAUTHENTICATED
The current implementation of oauth refresh offloads the IO to a separate executor, however when a token expires, all calls to getRequestMetadata would hang until the token is refreshed.
This PR tries to improve the situation by adding a stale state to token. If a token is within a minute of expiration, the first request to notice this, will spawn a refresh on the executor and immediately return with the existing token. This avoids hourly latency spikes for grpc.
The implementation uses guava's ListenableFutures to manage the async refresh state. Although the apis are marked BetaApi in guava 19, they are GA in guava 30