fix: Retry when attempting to get the auth token #1301

hessjcg · 2023-05-24T19:59:37Z

Simplifies the logic when attempting to get the first token for a connection with IAM AuthN enabled. Also, adds logic
to retry the token request 3 times if it initially fails.

Fixes #1288
Fixes #1127

enocom · 2023-05-24T22:11:50Z

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

-            apiFetcher.getInstanceData(
-                instanceName, downscopedCredentials, AuthType.IAM, executor, keyPair);
-      } else {
-        throw new RuntimeException(


We probably still need to throw an exception when no credentials have been provided.

I think that case is covered on line 104. There is no way to create a CloudSQLInstance with authType == IAM leaving this.iamAuthnCredentials with an empty value. Optional.of(null) will throw an exception.

Ah yes. In that case, let's update the exception message above to match what we have here (which is more useful, I think).

enocom · 2023-05-24T22:13:22Z

core/src/main/java/com/google/cloud/sql/core/SqlAdminApiFetcher.java

+   * @param credentials the credentials to refresh
+   * @throws IOException when the credentials.refresh() has failed 3 times
+   */
+  private void refreshWithRetry(OAuth2Credentials credentials) throws IOException {


Should we be adding a test for this? Something like a CredentialRefresher maybe?

Good idea. I'll add one.

Test added. I refactored the retry logic into its own implementation of Callable called RetryingCallable so that it could be tested.

enocom · 2023-05-25T19:52:32Z

Side note: does this issue fix all the Auto IAM AuthN flaky tests?

hessjcg · 2023-05-30T19:01:59Z

Yes, this fixes all the flakey tests.

enocom · 2023-05-30T19:14:08Z

core/src/main/java/com/google/cloud/sql/core/CloudSqlInstance.java

+    throw new RuntimeException(
+        String.format(
+            "[%s] Unable to connect via automatic IAM authentication: "
+                + "Not supporting credentials of type %s",


Should this be "Unsupported credentials of type ..."? "Not supporting" sounds odd.

enocom · 2023-05-30T19:14:41Z

core/src/main/java/com/google/cloud/sql/core/RetryingCallable.java

+ *
+ * @param <T> the result type of the Callable.
+ */
+public class RetryingCallable<T> implements Callable<T> {


Should this be public?

It doesn't need to be public. Fixed.

enocom · 2023-05-30T19:23:07Z

core/src/main/java/com/google/cloud/sql/core/RetryingCallable.java

+  @Override
+  public T call() throws Exception {
+
+    for (int i = retryCount - 1; i >= 0; i--) {


Why the count down instead of the usual count up?

I like that i is the number of tries remaining. It makes line 73 easier to understand. I'm renanming i to retriesLeft

enocom · 2023-05-30T19:23:54Z

core/src/main/java/com/google/cloud/sql/core/SqlAdminApiFetcher.java

+      throw e;
+    } catch (Exception e) {
+      throw new RuntimeException(
+          "Unexpected exception while attempting to refresh oauth credentials", e);


s/oauth/OAuth2/

enocom · 2023-05-30T19:25:02Z

core/src/main/java/com/google/cloud/sql/core/RetryingCallable.java

+   * @param sleepDuration the duration wait after a failed attempt.
+   */
+  public RetryingCallable(Callable<T> call, int retryCount, Duration sleepDuration) {
+    if (retryCount <= 0) {


Shall we test all these error conditions?

Let's put some tests around all this behavior. Should be simple to test.

enocom · 2023-05-30T19:26:23Z

core/src/test/java/com/google/cloud/sql/core/RetryingCallableTest.java

+public class RetryingCallableTest {
+  @Test
+  public void testNoRetryRequired() throws Exception {
+    RetryingCallable<Integer> r = new RetryingCallable<>(() -> 1, 5, Duration.ofSeconds(1));


Do we need to wait a full second in test between retries?

Changed to 100ms.

enocom · 2023-05-30T19:30:00Z

core/src/main/java/com/google/cloud/sql/core/RetryingCallable.java

+public class RetryingCallable<T> implements Callable<T> {
+
+  /** The callable that should be retried. */
+  private final Callable<T> call;


nit: Let's just match the class name generally. So this would be callable.

enocom · 2023-05-30T19:30:32Z

core/src/test/java/com/google/cloud/sql/core/BadConnectionFactory.java

+    return new FailToConnectRequest();
+  }
+
+  private class FailToConnectRequest extends LowLevelHttpRequest {


This can be static.

enocom · 2023-05-30T19:31:18Z

core/src/test/java/com/google/cloud/sql/core/BadConnectionFactory.java

+    @Override
+    public LowLevelHttpResponse execute() throws IOException {
+      try {
+        Thread.sleep(10000);


If we're just going to throw a Socket timeout exception, shall we just skip the sleep here?

enocom · 2023-05-30T19:33:20Z

This PR adds a retry for fetching the OAuth2 token which is used exclusively in Auto IAM AuthN. Why do you think it will fix the integration tests that aren't using Auto IAM AuthN?

enocom

LGTM. I'd like to see us add tests for the various error conditions in RetryingCallable's constructor before merging this.

hessjcg · 2023-06-02T16:34:11Z

Added tests for RetryingCallable constructor illegal arguments.

hessjcg requested a review from a team as a code owner May 24, 2023 19:59

blunderbuss-gcf bot assigned enocom May 24, 2023

hessjcg force-pushed the metadata-retry-flakey-tests branch 2 times, most recently from 1ab7c33 to 42096ea Compare May 24, 2023 20:07

hessjcg mentioned this pull request May 24, 2023

fix: Use existing retry logic on initial connection when IAMAuth is enabled #1300

Closed

hessjcg changed the title ~~chore: retry when refreshing the access token.~~ fix: Add retry logic on initial connection when IAMAuth is enabled May 24, 2023

hessjcg changed the title ~~fix: Add retry logic on initial connection when IAMAuth is enabled~~ fix: Add retry when attempting to get the auth token May 24, 2023

hessjcg changed the title ~~fix: Add retry when attempting to get the auth token~~ fix: Retry when attempting to get the auth token May 24, 2023

enocom reviewed May 24, 2023

View reviewed changes

hessjcg requested a review from enocom May 30, 2023 19:04

enocom reviewed May 30, 2023

View reviewed changes

hessjcg added 6 commits May 30, 2023 14:48

chore: retry when refreshing the access token.

10b3600

chore: fix lint errors.

a86e338

test: Make the retry logic testable in its own class.

c422c57

fix: improve exception messages.

9eed068

fix: improve another exception message.

16220fd

fix: code review comments from eno

0890b55

hessjcg force-pushed the metadata-retry-flakey-tests branch from d7fbf95 to 0890b55 Compare May 30, 2023 20:56

hessjcg requested a review from enocom May 30, 2023 20:56

hessjcg and others added 3 commits May 30, 2023 15:43

fix: correct the assertion.

0ab1b6f

Merge branch 'main' into metadata-retry-flakey-tests

57227fe

fix: make explicit dependency declaration to fix lint.

1c1197a

hessjcg enabled auto-merge (squash) June 2, 2023 15:51

enocom requested changes Jun 2, 2023

View reviewed changes

fix: add tests for constructor illegal arguments.

229aa16

hessjcg requested a review from enocom June 2, 2023 16:34

enocom approved these changes Jun 2, 2023

View reviewed changes

hessjcg merged commit 2694cc5 into main Jun 2, 2023
17 checks passed

hessjcg deleted the metadata-retry-flakey-tests branch June 2, 2023 17:09

infra-db-release-bot mentioned this pull request Jun 2, 2023

chore(main): release 1.12.0 #1312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Retry when attempting to get the auth token #1301

fix: Retry when attempting to get the auth token #1301

hessjcg commented May 24, 2023 •

edited

Loading

enocom May 24, 2023

hessjcg May 24, 2023

enocom May 25, 2023

hessjcg May 30, 2023

enocom May 24, 2023

hessjcg May 24, 2023

hessjcg May 30, 2023

enocom commented May 25, 2023

hessjcg commented May 30, 2023

enocom May 30, 2023

hessjcg May 30, 2023

enocom May 30, 2023

hessjcg May 30, 2023

enocom May 30, 2023

hessjcg May 30, 2023

enocom May 30, 2023

hessjcg May 30, 2023

enocom May 30, 2023

enocom May 30, 2023

enocom May 30, 2023

hessjcg May 30, 2023

enocom May 30, 2023

hessjcg May 30, 2023

enocom May 30, 2023

hessjcg May 30, 2023

enocom May 30, 2023

hessjcg May 30, 2023

enocom commented May 30, 2023

enocom left a comment

hessjcg commented Jun 2, 2023

fix: Retry when attempting to get the auth token #1301

fix: Retry when attempting to get the auth token #1301

Conversation

hessjcg commented May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enocom commented May 25, 2023

hessjcg commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enocom commented May 30, 2023

enocom left a comment

Choose a reason for hiding this comment

hessjcg commented Jun 2, 2023

hessjcg commented May 24, 2023 •

edited

Loading