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

fix: Retry sign blob call with exponential backoff #1452

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented Jul 30, 2024

Fixes #1451

The overall changes will be split into two parts. This part is part 1 where retries will be enabled and tests to cover successful and failed calls.

Part 2 will add additional tests to cover retries for the IAM Mock Server. Split it out to a separate PR since the IAM Mock Server will need to be refactored to support retries and variable number of responses. The refactor will touch multiple files that are non-related to adding retries to the Sign Blob RPC.

Changes:

  1. Sign Blob RPC will have retries for certain 5xx status codes
  2. Centralize default retry parameter values in Oauth2Utils for all retryable RPCs

@lqiu96 lqiu96 requested a review from zhumin8 July 30, 2024 20:51
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jul 30, 2024
@lqiu96 lqiu96 marked this pull request as ready for review August 1, 2024 16:00
@lqiu96 lqiu96 requested a review from a team as a code owner August 1, 2024 16:00
Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -279,9 +279,6 @@ public TokenVerifier build() {
/** Custom CacheLoader for mapping certificate urls to the contained public keys. */
static class PublicKeyLoader extends CacheLoader<String, Map<String, PublicKey>> {
private static final int DEFAULT_NUMBER_OF_RETRIES = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious if there is specific guideline for TokenVerifier to default retry 2 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I think these values were randomly selected and used. I've kept it the same for now and we can re-visit retries again in the future.

Copy link

@westarle westarle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good, I would like to see verification in the test:

  1. success case makes a single request
  2. 401s are not retried
  3. 50x errors are retried according to policy to failure
  4. 50x errors are retried according to policy to success

oauth2_http/java/com/google/auth/oauth2/OAuth2Utils.java Outdated Show resolved Hide resolved
@lqiu96
Copy link
Contributor Author

lqiu96 commented Aug 5, 2024

This looks pretty good, I would like to see verification in the test:

  1. success case makes a single request
  2. 401s are not retried
  3. 50x errors are retried according to policy to failure
  4. 50x errors are retried according to policy to success

Yep, those test cases make sense. I was running into some limitations of MockIAMCredentialsServiceTransport class where I couldn't specify that some 5xx errors should be retried and couldn't control how many times it would retry for.

I made some changes that to the class to support retrying and controlling the number of retry attempts. Those changes ended up touch more than just the SignBlob RPC so I made that into Part 2.

I believe this PR only covers cases 1 and 2, but 3 and 4 should be tested in the second PR.

Copy link

sonarcloud bot commented Aug 5, 2024

@lqiu96 lqiu96 requested a review from westarle August 5, 2024 19:12
Copy link

@westarle westarle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If tests in #1455 pass, this LGTM.

@lqiu96
Copy link
Contributor Author

lqiu96 commented Aug 6, 2024

The additional test cases added in #1455 pass. Will first merge in this PR and rebase #1455 with these changes.

@lqiu96 lqiu96 merged commit d42f30a into main Aug 6, 2024
21 checks passed
@lqiu96 lqiu96 deleted the signed_url_retries branch August 6, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SignBlob call to IAM should retry
3 participants