From 9e9e650f456aabaf5b8bf419b5f9744654e986e1 Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Wed, 25 Sep 2024 03:21:40 -0400 Subject: [PATCH] fix: use the Retry-After header value closes: #6366 Signed-off-by: Steve Hawkins --- CHANGELOG.md | 1 + .../client/http/StandardHttpClient.java | 57 ++++++++++--------- .../kubernetes/client/utils/AsyncUtils.java | 10 +++- .../client/http/StandardHttpClientTest.java | 18 ++++++ .../client/utils/AsyncUtilsTest.java | 8 +-- 5 files changed, 60 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7cb37f87aa..c9497dfd465 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ * Fix #5264: Remove deprecated `Config.errorMessages` field * Fix #6008: removing the optional dependency on bouncy castle * Fix #6230: introduced Quantity.multiply(int) to allow for Quantity multiplication by an integer +* Fix #6366: Allow Retry-After header to be considered in retries * Fix #6247: Support for proxy authentication from proxy URL user info * Fix #6281: use GitHub binary repo for Kube API Tests * Fix #6282: Allow annotated types with Pattern, Min, and Max with Lists and Maps and CRD generation diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/StandardHttpClient.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/StandardHttpClient.java index 1a415681f0c..80c0f42048c 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/StandardHttpClient.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/StandardHttpClient.java @@ -27,7 +27,6 @@ import java.io.Closeable; import java.io.IOException; -import java.net.URI; import java.nio.ByteBuffer; import java.time.Duration; import java.time.ZonedDateTime; @@ -152,7 +151,6 @@ private CompletableFuture> consumeBytesOnce(StandardHttp private CompletableFuture retryWithExponentialBackoff( StandardHttpRequest request, Supplier> action, java.util.function.Consumer onCancel, Function> responseExtractor) { - final URI uri = request.uri(); final RequestConfig requestConfig = getTag(RequestConfig.class); final ExponentialBackoffIntervalCalculator retryIntervalCalculator = ExponentialBackoffIntervalCalculator .from(requestConfig); @@ -164,34 +162,39 @@ private CompletableFuture retryWithExponentialBackoff( } return AsyncUtils.retryWithExponentialBackoff(action, onCancel, timeout, retryIntervalCalculator, (response, throwable, retryInterval) -> { - if (response != null) { - HttpResponse httpResponse = responseExtractor.apply(response); - if (httpResponse != null) { - final int code = httpResponse.code(); - if (code == 429 || code >= 500) { - retryInterval = Math.max(retryAfterMillis(httpResponse), retryInterval); - LOG.debug( - "HTTP operation on url: {} should be retried as the response code was {}, retrying after {} millis", - uri, code, retryInterval); - return true; - } - } - } else { - final Throwable actualCause = unwrapCompletionException(throwable); - builder.interceptors.forEach((s, interceptor) -> interceptor.afterConnectionFailure(request, actualCause)); - if (actualCause instanceof IOException) { - // TODO: may not be specific enough - incorrect ssl settings for example will get caught here - LOG.debug( - String.format("HTTP operation on url: %s should be retried after %d millis because of IOException", - uri, retryInterval), - actualCause); - return true; - } - } - return false; + return shouldRetry(request, responseExtractor, response, throwable, retryInterval); }); } + long shouldRetry(StandardHttpRequest request, Function> responseExtractor, V response, + Throwable throwable, long retryInterval) { + if (response != null) { + HttpResponse httpResponse = responseExtractor.apply(response); + if (httpResponse != null) { + final int code = httpResponse.code(); + if (code == 429 || code >= 500) { + retryInterval = Math.max(retryAfterMillis(httpResponse), retryInterval); + LOG.debug( + "HTTP operation on url: {} should be retried as the response code was {}, retrying after {} millis", + request.uri(), code, retryInterval); + return retryInterval; + } + } + } else { + final Throwable actualCause = unwrapCompletionException(throwable); + builder.interceptors.forEach((s, interceptor) -> interceptor.afterConnectionFailure(request, actualCause)); + if (actualCause instanceof IOException) { + // TODO: may not be specific enough - incorrect ssl settings for example will get caught here + LOG.debug( + String.format("HTTP operation on url: %s should be retried after %d millis because of IOException", + request.uri(), retryInterval), + actualCause); + return retryInterval; + } + } + return -1; + } + static Throwable unwrapCompletionException(Throwable throwable) { final Throwable actualCause; if (throwable instanceof CompletionException) { diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/AsyncUtils.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/AsyncUtils.java index 1c6331bd49b..0f079cb67c5 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/AsyncUtils.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/AsyncUtils.java @@ -49,7 +49,7 @@ public static CompletableFuture withTimeout(CompletableFuture future, /** * Returns a new {@link CompletableFuture} that will complete once the action provided by the action supplier completes. * The action will be retried with an exponential backoff using the {@link ExponentialBackoffIntervalCalculator} as - * long as the {@link ShouldRetry} predicate returns true. + * long as the {@link ShouldRetry} predicate returns a non-negative value. * Each action retrieval retry will time out after the provided timeout {@link Duration}. * * @param action the action supplier. @@ -75,7 +75,8 @@ private static void retryWithExponentialBackoff(CompletableFuture result, withTimeout(action.get(), timeout).whenComplete((r, t) -> { if (retryIntervalCalculator.shouldRetry() && !result.isDone()) { final long retryInterval = retryIntervalCalculator.nextReconnectInterval(); - if (shouldRetry.shouldRetry(r, t, retryInterval)) { + long retryValue = shouldRetry.shouldRetry(r, t, retryInterval); + if (retryValue >= 0) { if (r != null) { onCancel.accept(r); } @@ -95,6 +96,9 @@ private static void retryWithExponentialBackoff(CompletableFuture result, @FunctionalInterface public interface ShouldRetry { - boolean shouldRetry(T result, Throwable exception, long retryInterval); + /** + * @return the retry interval in ms, or a negative value indicating retries should be aborted + */ + long shouldRetry(T result, Throwable exception, long retryInterval); } } diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/StandardHttpClientTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/StandardHttpClientTest.java index 387f6aefacf..24a983e35a8 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/StandardHttpClientTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/StandardHttpClientTest.java @@ -32,7 +32,9 @@ import java.time.format.DateTimeFormatter; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; @@ -178,6 +180,22 @@ void testHttpRetryWithLessFailuresThanRetries() throws Exception { .hasSize(4); } + @Test + void testShouldRetryUsesRetryAfterHeader() throws Exception { + client = client.newBuilder().tag(new RequestConfigBuilder() + .withRequestRetryBackoffLimit(3) + .withRequestRetryBackoffInterval(50).build()) + .build(); + + Map> headers = new HashMap<>(); + headers.put(StandardHttpHeaders.RETRY_AFTER, Arrays.asList("5")); + // the exception type doesn't matter + final WebSocketResponse error = new WebSocketResponse(new WebSocketUpgradeResponse(null, 429, headers), new IOException()); + + assertThat(client.shouldRetry((StandardHttpRequest) client.newHttpRequestBuilder().uri("http://localhost").build(), + r -> r.webSocketUpgradeResponse, error, null, 1000)).isEqualTo(5000); + } + @Test void testWebSocketWithLessFailuresThanRetries() throws Exception { client = client.newBuilder().tag(new RequestConfigBuilder() diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/AsyncUtilsTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/AsyncUtilsTest.java index f648d4c2c5a..c10f482bfd8 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/AsyncUtilsTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/AsyncUtilsTest.java @@ -79,7 +79,7 @@ void retryWithExponentialBackoff_timeout() { final Supplier> action = CompletableFuture::new; final CompletableFuture onCancel = new CompletableFuture<>(); final ExponentialBackoffIntervalCalculator retryIntervalCalculator = new ExponentialBackoffIntervalCalculator(1, 1); - final AsyncUtils.ShouldRetry shouldRetry = (v, t, retryInterval) -> true; + final AsyncUtils.ShouldRetry shouldRetry = (v, t, retryInterval) -> retryInterval; // When final CompletableFuture result = retryWithExponentialBackoff(action, onCancel::complete, Duration.ofMillis(1), retryIntervalCalculator, shouldRetry); @@ -98,7 +98,7 @@ void retryWithExponentialBackoff_withCancelledFuture_onCancel() { final Supplier> actionSupplier = () -> action; final CompletableFuture onCancel = new CompletableFuture<>(); final ExponentialBackoffIntervalCalculator retryIntervalCalculator = new ExponentialBackoffIntervalCalculator(1, 0); - final AsyncUtils.ShouldRetry shouldRetry = (v, t, retryInterval) -> false; + final AsyncUtils.ShouldRetry shouldRetry = (v, t, retryInterval) -> -1; // When final CompletableFuture result = retryWithExponentialBackoff(actionSupplier, onCancel::complete, Duration.ofMillis(100), retryIntervalCalculator, shouldRetry); @@ -119,7 +119,7 @@ void retryWithExponentialBackoff_withCompletedResult_onCancel() throws Exception final Supplier> actionSupplier = () -> action; final CompletableFuture onCancel = new CompletableFuture<>(); final ExponentialBackoffIntervalCalculator retryIntervalCalculator = new ExponentialBackoffIntervalCalculator(1, 1); - final AsyncUtils.ShouldRetry shouldRetry = (v, t, retryInterval) -> true; + final AsyncUtils.ShouldRetry shouldRetry = (v, t, retryInterval) -> retryInterval; // When CompletableFuture result = retryWithExponentialBackoff(actionSupplier, onCancel::complete, Duration.ofMillis(100), retryIntervalCalculator, shouldRetry); @@ -140,7 +140,7 @@ void retryWithExponentialBackoff_complete() { final Supplier> actionSupplier = () -> action; final CompletableFuture onCancel = new CompletableFuture<>(); final ExponentialBackoffIntervalCalculator retryIntervalCalculator = new ExponentialBackoffIntervalCalculator(1, 0); - final AsyncUtils.ShouldRetry shouldRetry = (v, t, retryInterval) -> false; + final AsyncUtils.ShouldRetry shouldRetry = (v, t, retryInterval) -> -1; // When final CompletableFuture result = retryWithExponentialBackoff(actionSupplier, onCancel::complete, Duration.ofMillis(100), retryIntervalCalculator, shouldRetry);