From ae56f30170c490e1efca21fa6ae4e022d5133d1a Mon Sep 17 00:00:00 2001
From: Steven Hawkins <shawkins@redhat.com>
Date: Wed, 25 Sep 2024 09:21:40 +0200
Subject: [PATCH] fix: use the Retry-After header value
closes: #6366
Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
---
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 24daa32301b..5a6e41f4e2d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@
* Fix #6247: Support for proxy authentication from proxy URL user info
* Fix #6342: UnmatchedFieldTypeModule prevents certain jackson features from working
* Fix #6354: Prevent deadlock in okhttp AsyncBody.cancel
+* Fix #6366: Allow Retry-After header to be considered in retries
### 6.13.3 (2024-08-13)
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<HttpResponse<AsyncBody>> consumeBytesOnce(StandardHttp
private <V> CompletableFuture<V> retryWithExponentialBackoff(
StandardHttpRequest request, Supplier<CompletableFuture<V>> action, java.util.function.Consumer<V> onCancel,
Function<V, HttpResponse<?>> responseExtractor) {
- final URI uri = request.uri();
final RequestConfig requestConfig = getTag(RequestConfig.class);
final ExponentialBackoffIntervalCalculator retryIntervalCalculator = ExponentialBackoffIntervalCalculator
.from(requestConfig);
@@ -164,34 +162,39 @@ private <V> CompletableFuture<V> 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);
});
}
+ <V> long shouldRetry(StandardHttpRequest request, Function<V, HttpResponse<?>> 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 <T> CompletableFuture<T> withTimeout(CompletableFuture<T> 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 <T> void retryWithExponentialBackoff(CompletableFuture<T> 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 <T> void retryWithExponentialBackoff(CompletableFuture<T> result,
@FunctionalInterface
public interface ShouldRetry<T> {
- 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<String, List<String>> 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<CompletableFuture<Void>> action = CompletableFuture::new;
final CompletableFuture<Void> onCancel = new CompletableFuture<>();
final ExponentialBackoffIntervalCalculator retryIntervalCalculator = new ExponentialBackoffIntervalCalculator(1, 1);
- final AsyncUtils.ShouldRetry<Void> shouldRetry = (v, t, retryInterval) -> true;
+ final AsyncUtils.ShouldRetry<Void> shouldRetry = (v, t, retryInterval) -> retryInterval;
// When
final CompletableFuture<Void> result = retryWithExponentialBackoff(action, onCancel::complete, Duration.ofMillis(1),
retryIntervalCalculator, shouldRetry);
@@ -98,7 +98,7 @@ void retryWithExponentialBackoff_withCancelledFuture_onCancel() {
final Supplier<CompletableFuture<Void>> actionSupplier = () -> action;
final CompletableFuture<Void> onCancel = new CompletableFuture<>();
final ExponentialBackoffIntervalCalculator retryIntervalCalculator = new ExponentialBackoffIntervalCalculator(1, 0);
- final AsyncUtils.ShouldRetry<Void> shouldRetry = (v, t, retryInterval) -> false;
+ final AsyncUtils.ShouldRetry<Void> shouldRetry = (v, t, retryInterval) -> -1;
// When
final CompletableFuture<Void> result = retryWithExponentialBackoff(actionSupplier, onCancel::complete,
Duration.ofMillis(100), retryIntervalCalculator, shouldRetry);
@@ -119,7 +119,7 @@ void retryWithExponentialBackoff_withCompletedResult_onCancel() throws Exception
final Supplier<CompletableFuture<Boolean>> actionSupplier = () -> action;
final CompletableFuture<Boolean> onCancel = new CompletableFuture<>();
final ExponentialBackoffIntervalCalculator retryIntervalCalculator = new ExponentialBackoffIntervalCalculator(1, 1);
- final AsyncUtils.ShouldRetry<Boolean> shouldRetry = (v, t, retryInterval) -> true;
+ final AsyncUtils.ShouldRetry<Boolean> shouldRetry = (v, t, retryInterval) -> retryInterval;
// When
CompletableFuture<Boolean> result = retryWithExponentialBackoff(actionSupplier, onCancel::complete,
Duration.ofMillis(100), retryIntervalCalculator, shouldRetry);
@@ -140,7 +140,7 @@ void retryWithExponentialBackoff_complete() {
final Supplier<CompletableFuture<Void>> actionSupplier = () -> action;
final CompletableFuture<Void> onCancel = new CompletableFuture<>();
final ExponentialBackoffIntervalCalculator retryIntervalCalculator = new ExponentialBackoffIntervalCalculator(1, 0);
- final AsyncUtils.ShouldRetry<Void> shouldRetry = (v, t, retryInterval) -> false;
+ final AsyncUtils.ShouldRetry<Void> shouldRetry = (v, t, retryInterval) -> -1;
// When
final CompletableFuture<Void> result = retryWithExponentialBackoff(actionSupplier, onCancel::complete,
Duration.ofMillis(100), retryIntervalCalculator, shouldRetry);