fix: use the Retry-After header value

closes: #6366

Signed-off-by: Steve Hawkins <[email protected]>
Signed-off-by: Marc Nuri <[email protected]>

Author: shawkins

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)
 

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) {

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);
   }
 }

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()

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);