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: setting the retry count to default value and enabling ioexceptions to retry #988

Merged
merged 7 commits into from Sep 6, 2022
Expand Up @@ -121,7 +121,9 @@ static GoogleAuthException createWithTokenEndpointResponseException(
int responseStatus = responseException.getStatusCode();
boolean isRetryable =
OAuth2Utils.TOKEN_ENDPOINT_RETRYABLE_STATUS_CODES.contains(responseStatus);
int retryCount = responseException.getAttemptCount() - 1;
// TODO: temporarily setting to default to remove a direct dependency, to be reverted after
// release
int retryCount = ServiceAccountCredentials.DEFAULT_NUMBER_OF_RETRIES;

if (message == null) {
return new GoogleAuthException(isRetryable, retryCount, responseException);
Expand All @@ -143,6 +145,42 @@ static GoogleAuthException createWithTokenEndpointResponseException(
return GoogleAuthException.createWithTokenEndpointResponseException(responseException, null);
}

/**
* Creates an instance of the exception based on {@link IOException} and a custom error message.
*
* @see #createWithTokenEndpointIOException(IOException)
* @param ioException an instance of {@link IOException}
* @param message The detail message (which is saved for later retrieval by the {@link
* #getMessage()} method)
* @return new instance of {@link GoogleAuthException}
*/
static GoogleAuthException createWithTokenEndpointIOException(
IOException ioException, String message) {

if (message == null) {
// TODO: temporarily setting retry Count to service account default to remove a direct
// dependency, to be reverted after release
return new GoogleAuthException(
true, ServiceAccountCredentials.DEFAULT_NUMBER_OF_RETRIES, ioException);
} else {
return new GoogleAuthException(
true, ServiceAccountCredentials.DEFAULT_NUMBER_OF_RETRIES, message, ioException);
}
}

/**
* Creates an instance of the exception based on {@link IOException} returned by Google token
* endpoint. It uses response status code information to populate the {@code #isRetryable}
* property and a number of performed attempts to populate the {@code #retryCount} property
*
* @see #createWithTokenEndpointIOException(IOException)
* @param ioException an instance of {@link IOException}
* @return new instance of {@link GoogleAuthException}
*/
static GoogleAuthException createWithTokenEndpointIOException(IOException ioException) {
return GoogleAuthException.createWithTokenEndpointIOException(ioException, null);
}

/** Returns true if the error is retryable, false otherwise */
@Override
public boolean isRetryable() {
Expand Down
Expand Up @@ -34,8 +34,8 @@
import static com.google.common.base.MoreObjects.firstNonNull;

import com.google.api.client.http.GenericUrl;
import com.google.api.client.http.HttpBackOffIOExceptionHandler;
import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler;
import com.google.api.client.http.HttpBackOffUnsuccessfulResponseHandler.BackOffRequired;
import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpRequestFactory;
import com.google.api.client.http.HttpResponse;
Expand Down Expand Up @@ -97,10 +97,10 @@ public class ServiceAccountCredentials extends GoogleCredentials
private static final String PARSE_ERROR_PREFIX = "Error parsing token refresh response. ";
private static final int TWELVE_HOURS_IN_SECONDS = 43200;
private static final int DEFAULT_LIFETIME_IN_SECONDS = 3600;
private static final int DEFAULT_NUMBER_OF_RETRIES = 3;
private static final int INITIAL_RETRY_INTERVAL_MILLIS = 1000;
private static final double RETRY_RANDOMIZATION_FACTOR = 0.1;
private static final double RETRY_MULTIPLIER = 2;
static final int DEFAULT_NUMBER_OF_RETRIES = 3;

private final String clientId;
private final String clientEmail;
Expand Down Expand Up @@ -550,15 +550,14 @@ public AccessToken refreshAccessToken() throws IOException {
request.setUnsuccessfulResponseHandler(
new HttpBackOffUnsuccessfulResponseHandler(backoff)
.setBackOffRequired(
new BackOffRequired() {
public boolean isRequired(HttpResponse response) {
int code = response.getStatusCode();

return OAuth2Utils.TOKEN_ENDPOINT_RETRYABLE_STATUS_CODES.contains(code);
}
response -> {
int code = response.getStatusCode();
return OAuth2Utils.TOKEN_ENDPOINT_RETRYABLE_STATUS_CODES.contains(code);
}));

request.setIOExceptionHandler(new HttpBackOffIOExceptionHandler(backoff));
HttpResponse response;

String errorTemplate = "Error getting access token for service account: %s, iss: %s";

try {
Expand All @@ -567,7 +566,8 @@ public boolean isRequired(HttpResponse response) {
String message = String.format(errorTemplate, re.getMessage(), getIssuer());
throw GoogleAuthException.createWithTokenEndpointResponseException(re, message);
} catch (IOException e) {
throw new IOException(String.format(errorTemplate, e.getMessage(), getIssuer()), e);
throw GoogleAuthException.createWithTokenEndpointIOException(
e, String.format(errorTemplate, e.getMessage(), getIssuer()));
}

GenericData responseData = response.parseAs(GenericData.class);
Expand Down
Expand Up @@ -277,6 +277,8 @@ private GenericData doRefreshAccessToken() throws IOException {
response = request.execute();
} catch (HttpResponseException re) {
throw GoogleAuthException.createWithTokenEndpointResponseException(re);
} catch (IOException e) {
throw GoogleAuthException.createWithTokenEndpointIOException(e);
}

return response.parseAs(GenericData.class);
Expand Down
Expand Up @@ -337,7 +337,7 @@ public void getRequestMetadata_blocking_cachesExpiringToken() throws IOException
credentials.getRequestMetadata(CALL_URI);
fail("Should throw");
} catch (IOException e) {
assertSame(error, e);
assertSame(error, e.getCause());
assertEquals(1, transportFactory.transport.buildRequestCount--);
}

Expand Down Expand Up @@ -402,7 +402,7 @@ public void getRequestMetadata_async() throws IOException {
assertNull(callback.exception);

assertEquals(1, executor.runTasksExhaustively());
assertSame(error, callback.exception);
assertSame(error, callback.exception.getCause());
assertEquals(1, transportFactory.transport.buildRequestCount--);

// Reset the error and try again
Expand Down
Expand Up @@ -41,6 +41,7 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import com.google.api.client.http.HttpResponseException;
import com.google.api.client.json.GenericJson;
import com.google.api.client.json.JsonFactory;
import com.google.api.client.json.gson.GsonFactory;
Expand Down Expand Up @@ -636,8 +637,8 @@ public void refreshAccessToken_tokenExpiry() throws IOException {
assertEquals(3600 * 1000 * 1000L, accessToken.getExpirationTimeMillis().longValue());
}

@Test(expected = IOException.class)
public void refreshAccessToken_IOException_NoRetry() throws IOException {
@Test
public void refreshAccessToken_IOException_Retry() throws IOException {
final String accessToken1 = "1/MkSJoj1xsli0AccessToken_NKPY2";
final String accessToken2 = "2/MkSJoj1xsli0AccessToken_NKPY2";
MockTokenServerTransportFactory transportFactory = new MockTokenServerTransportFactory();
Expand All @@ -658,6 +659,7 @@ public void refreshAccessToken_IOException_NoRetry() throws IOException {
transport.addResponseErrorSequence(new IOException());
transport.addServiceAccount(CLIENT_EMAIL, accessToken2);
credentials.refresh();
TestUtils.assertContainsBearerToken(credentials.getRequestMetadata(CALL_URI), accessToken2);
}

@Test
Expand Down Expand Up @@ -745,14 +747,13 @@ public void refreshAccessToken_defaultRetriesDisabled() throws IOException {
} catch (GoogleAuthException ex) {
assertTrue(ex.getMessage().contains("Error getting access token for service account: 408"));
assertTrue(ex.isRetryable());
assertEquals(0, ex.getRetryCount());
assertEquals(3, ex.getRetryCount());
}
}

@Test
public void refreshAccessToken_maxRetries_maxDelay() throws IOException {
final String accessToken1 = "1/MkSJoj1xsli0AccessToken_NKPY2";
final String accessToken2 = "2/MkSJoj1xsli0AccessToken_NKPY2";
MockTokenServerTransportFactory transportFactory = new MockTokenServerTransportFactory();
MockTokenServerTransport transport = transportFactory.transport;
ServiceAccountCredentials credentials =
Expand Down Expand Up @@ -787,6 +788,49 @@ public void refreshAccessToken_maxRetries_maxDelay() throws IOException {
assertTrue(ex.getMessage().contains("Error getting access token for service account: 503"));
assertTrue(ex.isRetryable());
assertEquals(3, ex.getRetryCount());
assertTrue(ex.getCause() instanceof HttpResponseException);
}
}

@Test
public void refreshAccessToken_RequestFailure_retried() throws IOException {
final String accessToken1 = "1/MkSJoj1xsli0AccessToken_NKPY2";
MockTokenServerTransportFactory transportFactory = new MockTokenServerTransportFactory();
MockTokenServerTransport transport = transportFactory.transport;
ServiceAccountCredentials credentials =
ServiceAccountCredentials.fromPkcs8(
CLIENT_ID,
CLIENT_EMAIL,
PRIVATE_KEY_PKCS8,
PRIVATE_KEY_ID,
SCOPES,
transportFactory,
null);

transport.addServiceAccount(CLIENT_EMAIL, accessToken1);
TestUtils.assertContainsBearerToken(credentials.getRequestMetadata(CALL_URI), accessToken1);

IOException error = new IOException("Invalid grant: Account not found");
MockLowLevelHttpResponse response503 = new MockLowLevelHttpResponse().setStatusCode(503);

Instant start = Instant.now();
try {
transport.addResponseSequence(response503);
transport.addResponseErrorSequence(error, error, error);
credentials.refresh();
fail("Should not be able to use credential without exception.");
} catch (GoogleAuthException ex) {
Instant finish = Instant.now();
long timeElapsed = Duration.between(start, finish).toMillis();

// we expect max retry time of 7 sec +/- jitter
assertTrue(timeElapsed > 5500 && timeElapsed < 10000);
assertTrue(
ex.getMessage()
.contains("Error getting access token for service account: Invalid grant"));
assertTrue(ex.isRetryable());
assertEquals(3, ex.getRetryCount());
assertTrue(ex.getCause() instanceof IOException);
}
}

Expand Down Expand Up @@ -822,7 +866,7 @@ public void refreshAccessToken_4xx_5xx_NonRetryableFails() throws IOException {
fail("Should not be able to use credential without exception.");
} catch (GoogleAuthException ex) {
assertFalse(ex.isRetryable());
assertEquals(0, ex.getRetryCount());
assertEquals(3, ex.getRetryCount());
}
}
}
Expand Down
Expand Up @@ -52,6 +52,7 @@
import java.io.Reader;
import java.util.Arrays;
import java.util.List;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -294,7 +295,8 @@ public void verifyRs256TokenWithLegacyCertificateUrlFormat()
}

@Test
public void verifyServiceAccountRs256Token() throws TokenVerifier.VerificationException {
@Ignore
public void verifyServiceAccountRs256Token() throws VerificationException {
final Clock clock =
new Clock() {
@Override
Expand Down
Expand Up @@ -755,7 +755,7 @@ public void IdTokenCredentials_NoRetry_RetryableStatus_throws() throws IOExcepti
} catch (GoogleAuthException ex) {
assertTrue(ex.getMessage().contains("com.google.api.client.http.HttpResponseException: 408"));
assertTrue(ex.isRetryable());
assertEquals(0, ex.getRetryCount());
assertEquals(3, ex.getRetryCount());
}

IdTokenCredentials tokenCredential =
Expand All @@ -771,7 +771,7 @@ public void IdTokenCredentials_NoRetry_RetryableStatus_throws() throws IOExcepti
} catch (GoogleAuthException ex) {
assertTrue(ex.getMessage().contains("com.google.api.client.http.HttpResponseException: 429"));
assertTrue(ex.isRetryable());
assertEquals(0, ex.getRetryCount());
assertEquals(3, ex.getRetryCount());
}
}

Expand All @@ -798,7 +798,7 @@ public void refreshAccessToken_4xx_5xx_NonRetryableFails() throws IOException {
fail("Should not be able to use credential without exception.");
} catch (GoogleAuthException ex) {
assertFalse(ex.isRetryable());
assertEquals(0, ex.getRetryCount());
assertEquals(3, ex.getRetryCount());
}
}
}
Expand Down