googleapis / google-auth-library-java

Open source Auth client library for Java
https://developers.google.com/identity
BSD 3-Clause "New" or "Revised" License
410 stars 229 forks source link

fix: Retry sign blob call with exponential backoff #1452

Closed lqiu96 closed 3 months ago

lqiu96 commented 3 months ago

Fixes https://github.com/googleapis/google-auth-library-java/issues/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 commented 3 months ago

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.

sonarcloud[bot] commented 3 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
1.2% Duplication on New Code

See analysis details on SonarCloud

lqiu96 commented 3 months ago

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