Skip to content
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

Merged
merged 21 commits into from May 20, 2021

Conversation

igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Apr 29, 2021

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

  • 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.
  • Special care is taken to make sure that the lock is released when the token is being refreshed

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.
@google-cla google-cla bot added the cla: yes label Apr 29, 2021
@igorbernstein2 igorbernstein2 changed the title feat: add pre-emptive async oauth token refreshes [draft] feat: enable pre-emptive async oauth token refreshes [draft] Apr 29, 2021
@codecov
Copy link

@codecov codecov bot commented May 3, 2021

Codecov Report

Merging #646 (a17186e) into master (292e81a) will decrease coverage by 0.35%.
The diff coverage is 79.13%.

Current head a17186e differs from pull request most recent head a5a5a47. Consider uploading reports for the commit a5a5a47 to get more accurate results
Impacted file tree graph

@@             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     
Impacted Files Coverage Δ Complexity Δ
...google/auth/oauth2/ExternalAccountCredentials.java 91.60% <0.00%> (-6.16%) 29.00 <0.00> (ø)
...java/com/google/auth/oauth2/OAuth2Credentials.java 82.87% <84.61%> (+0.01%) 39.00 <25.00> (+1.00)
..._http/java/com/google/auth/oauth2/AccessToken.java 78.94% <0.00%> (-10.53%) 10.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 469b160...a5a5a47. Read the comment docs.

@@ -208,6 +210,27 @@ private ImpersonatedCredentials initializeImpersonatedCredentials() {
.build();
}

@Override
Copy link
Contributor Author

@igorbernstein2 igorbernstein2 May 3, 2021

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

@igorbernstein2 igorbernstein2 changed the title feat: enable pre-emptive async oauth token refreshes [draft] feat: enable pre-emptive async oauth token refreshes May 4, 2021
@igorbernstein2 igorbernstein2 marked this pull request as ready for review May 4, 2021
@igorbernstein2 igorbernstein2 requested a review from as a code owner May 4, 2021
Copy link
Member

@TimurSadykov TimurSadykov left a comment

Overall looks good, lets clarify if refresh slight behavior change is blocking.

Some coverage of refreshTask would be great.

Copy link
Contributor Author

@igorbernstein2 igorbernstein2 left a comment

Thanks for reviewing! I think addressed all of your comments, PTAL

@paulmordont
Copy link

@paulmordont paulmordont commented May 5, 2021

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 :)

Copy link
Member

@TimurSadykov TimurSadykov left a comment

Looks good to me, but please let's make sure @silvolu also take a look before merge.

@igorbernstein2 igorbernstein2 requested a review from silvolu May 5, 2021
@lesv
Copy link
Contributor

@lesv lesv commented May 11, 2021

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?

@igorbernstein2
Copy link
Contributor Author

@igorbernstein2 igorbernstein2 commented May 12, 2021

I created cl/373364841, I had to hand edit a couple of irrelevant files:

  • added missing RunWith on unrelated tests
  • added missing deps in BUILD

I requested a global tap presubmit

Also, I updated the codestyle suggestions from critique

lsirac
lsirac approved these changes May 13, 2021
lesv
lesv approved these changes May 19, 2021
@igorbernstein2 igorbernstein2 merged commit e3f4c7e into googleapis:master May 20, 2021
13 checks passed
@igorbernstein2 igorbernstein2 deleted the async-refresh branch May 20, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue May 20, 2021
🤖 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).
gcf-merge-on-green bot pushed a commit to googleapis/java-bigtable-hbase that referenced this issue Sep 24, 2021
…3228)

Now that auth library bundles [pre-emptive refresh](googleapis/google-auth-library-java#646), RefreshingOAuth2CredentialsInterceptor can be removed. Also, remove retry on UNAUTHENTICATED
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants