From d94b383f0d168f6a126854abc34179752757e529 Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Wed, 17 Jul 2024 17:20:56 +1200 Subject: [PATCH 01/12] Add test which demonstrates what I expect to work --- .../client/http/AbstractInterceptorTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java index f85ded10834..158d64eeea3 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java @@ -25,6 +25,8 @@ import java.net.URI; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.time.temporal.ChronoUnit; import java.util.Collections; import java.util.List; import java.util.Set; @@ -170,6 +172,28 @@ public CompletableFuture afterFailure(BasicBuilder builder, HttpRespons } } + @Test + @DisplayName("afterFailure (HTTP), invoked when remote server offline") + public void afterHttpFailureRemoteOffline() { + // Given + server.shutdown(); + final HttpClient.Builder builder = getHttpClientFactory().newBuilder() + .connectTimeout(1, TimeUnit.SECONDS) + .addOrReplaceInterceptor("test", new Interceptor() { + @Override + public CompletableFuture afterFailure(BasicBuilder builder, HttpResponse response, RequestTags tags) { + return CompletableFuture.completedFuture(false); + } + }); + // When + try (HttpClient client = builder.build()) { + final CompletableFuture> response = client.sendAsync(client.newHttpRequestBuilder().uri(server.url("/not-found")).build(), String.class); + + // Then + assertThat(response).succeedsWithin(Duration.of(10, ChronoUnit.SECONDS)); + } + } + @Test @DisplayName("afterFailure (HTTP), replaces the HttpResponse produced by HttpClient.consumeBytes") public void afterHttpFailureReplacesResponseInConsumeBytes() throws Exception { From 53f5464125e1116acb31e15d76161d7fb4924407 Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Mon, 22 Jul 2024 10:41:03 +1200 Subject: [PATCH 02/12] Introduce afterConnectionFailure to complement `after` & `afterFailure`. Fixes #6143 --- .../kubernetes/client/http/Interceptor.java | 11 ++++ .../client/http/StandardHttpClient.java | 7 +++ .../client/http/AbstractInterceptorTest.java | 51 ++++++++++++++++--- 3 files changed, 63 insertions(+), 6 deletions(-) diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java index 101ddfcef1e..f85ad595930 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java @@ -84,4 +84,15 @@ default CompletableFuture afterFailure(HttpRequest.Builder builder, Htt return afterFailure((BasicBuilder) builder, response, tags); } + /** + * Called after a connection attempt fails. + *

+ * This method will be invoked on each failed connection attempt so can be invoked multiple times for the same request.ID. + * + * @param request the HTTP request. + * @param failure the Java exception that caused the failure. + */ + default void afterConnectionFailure(HttpRequest request, Throwable failure) { + } + } 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 4cfd64d1fe0..4e54081732f 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 @@ -108,6 +108,13 @@ private CompletableFuture> consumeBytesOnce(StandardHttp final Consumer> effectiveConsumer = consumer; CompletableFuture> cf = consumeBytesDirect(effectiveRequest, effectiveConsumer); + cf.exceptionally(throwable -> { + builder.interceptors.forEach((s, interceptor) -> interceptor.afterConnectionFailure(effectiveRequest, throwable)); + if (throwable instanceof CompletionException) { + throw (CompletionException) throwable; + } + throw new CompletionException(throwable); + }); cf.thenAccept( response -> builder.getInterceptors().values().forEach(i -> i.after(effectiveRequest, response, effectiveConsumer))); diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java index 158d64eeea3..5f1a0f61525 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java @@ -32,6 +32,7 @@ import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import static org.assertj.core.api.Assertions.assertThat; @@ -173,24 +174,62 @@ public CompletableFuture afterFailure(BasicBuilder builder, HttpRespons } @Test - @DisplayName("afterFailure (HTTP), invoked when remote server offline") - public void afterHttpFailureRemoteOffline() { + @DisplayName("afterConnectionFailure, invoked when remote server offline") + public void afterConnectionFailureRemoteOffline() { // Given server.shutdown(); + final CountDownLatch connectionFailureCallbackInvoked = new CountDownLatch(1); final HttpClient.Builder builder = getHttpClientFactory().newBuilder() .connectTimeout(1, TimeUnit.SECONDS) .addOrReplaceInterceptor("test", new Interceptor() { @Override - public CompletableFuture afterFailure(BasicBuilder builder, HttpResponse response, RequestTags tags) { - return CompletableFuture.completedFuture(false); + public void afterConnectionFailure(HttpRequest request, Throwable failure) { + connectionFailureCallbackInvoked.countDown(); + } + }); + // When + try (HttpClient client = builder.build()) { + final CompletableFuture> response = client.sendAsync(client.newHttpRequestBuilder() + .timeout(1, TimeUnit.SECONDS) + .uri(server.url("/not-found")).build(), String.class); + + // Then + assertThat(response).failsWithin(Duration.of(30, ChronoUnit.SECONDS)); + assertThat(connectionFailureCallbackInvoked).extracting(CountDownLatch::getCount).isEqualTo(0L); + } + } + + @Test + @DisplayName("afterConnectionFailure, request is retried when remote server offline") + public void afterConnectionFailureRetry() { + // Given + final int originalPort = server.getPort(); + server.shutdown(); + final CountDownLatch afterInvoked = new CountDownLatch(1); + final HttpClient.Builder builder = getHttpClientFactory().newBuilder() + .connectTimeout(1, TimeUnit.SECONDS) + .addOrReplaceInterceptor("test", new Interceptor() { + @Override + public void afterConnectionFailure(HttpRequest request, Throwable failure) { + server = new DefaultMockServer(false); + server.expect().withPath("/intercepted-url").andReturn(200, "This works").once(); + server.start(originalPort); // Need to restart on the original port as we can't alter the request during retry. + } + + @Override + public void after(HttpRequest request, HttpResponse response, Consumer> consumer) { + afterInvoked.countDown(); } }); // When try (HttpClient client = builder.build()) { - final CompletableFuture> response = client.sendAsync(client.newHttpRequestBuilder().uri(server.url("/not-found")).build(), String.class); + final CompletableFuture> response = client.sendAsync(client.newHttpRequestBuilder() + .timeout(1, TimeUnit.SECONDS) + .uri(server.url("/intercepted-url")).build(), String.class); // Then - assertThat(response).succeedsWithin(Duration.of(10, ChronoUnit.SECONDS)); + assertThat(response).succeedsWithin(Duration.of(30, ChronoUnit.SECONDS)); + assertThat(afterInvoked).extracting(CountDownLatch::getCount).isEqualTo(0L); } } From dbc9583753c0af82b0211d6c62777feb4e1b9387 Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Mon, 22 Jul 2024 11:05:46 +1200 Subject: [PATCH 03/12] Add a bit more explanatory Javadoc to Interceptor --- .../kubernetes/client/http/Interceptor.java | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java index f85ad595930..581fca1954c 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java @@ -19,6 +19,13 @@ import java.util.List; import java.util.concurrent.CompletableFuture; +/** + * A collection of callback methods invoked through the various stages of the HTTP request lifecycle. + * Each invocation of {@link Interceptor#before(BasicBuilder, HttpRequest, RequestTags)} will be matched with a call to one of + * {@link Interceptor#afterConnectionFailure(HttpRequest, Throwable)} or + * {@link Interceptor#after(HttpRequest, HttpResponse, AsyncBody.Consumer)}. + * Callbacks that lead to a request being sent allow for that request to be customised. + */ public interface Interceptor { interface RequestTags { @@ -63,7 +70,10 @@ default AsyncBody.Consumer> consumer(AsyncBody.Consumer + * Failure is determined by HTTP status code and will be invoked in addition to {@see Interceptor#after(HttpRequest, + * HttpResponse, AsyncBody.Consumer)} * * @param builder used to modify the request * @param response the failed response @@ -75,7 +85,10 @@ default CompletableFuture afterFailure(BasicBuilder builder, HttpRespon /** * Called after a non-websocket failure - * + *

+ * Failure is determined by HTTP status code and will be invoked in addition to {@see Interceptor#after(HttpRequest, + * HttpResponse, AsyncBody.Consumer)} + * * @param builder used to modify the request * @param response the failed response * @return true if the builder should be used to execute a new request From e2638bd9b62ae665b18f6d1c763c00e34dfb66c1 Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Mon, 22 Jul 2024 14:54:38 +1200 Subject: [PATCH 04/12] Extract private method to ensure changes to DefaultMockServer usage are consistent. --- .../kubernetes/client/http/AbstractInterceptorTest.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java index 5f1a0f61525..fc828738a43 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java @@ -43,7 +43,7 @@ public abstract class AbstractInterceptorTest { @BeforeEach void startServer() { - server = new DefaultMockServer(false); + server = newMockServer(); server.start(); } @@ -211,7 +211,7 @@ public void afterConnectionFailureRetry() { .addOrReplaceInterceptor("test", new Interceptor() { @Override public void afterConnectionFailure(HttpRequest request, Throwable failure) { - server = new DefaultMockServer(false); + server = newMockServer(); server.expect().withPath("/intercepted-url").andReturn(200, "This works").once(); server.start(originalPort); // Need to restart on the original port as we can't alter the request during retry. } @@ -475,4 +475,7 @@ public void before(BasicBuilder builder, HttpRequest request, RequestTags tags) .containsEntry("test-header", Collections.singletonList("Test-Value-Override")); } + private static DefaultMockServer newMockServer() { + return new DefaultMockServer(false); + } } From f854fc954f7660e842a82b13dea321177ad8ec91 Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Mon, 22 Jul 2024 17:04:49 +1200 Subject: [PATCH 05/12] Add test which covers future being completed exceptionally with a CompletionException. This happens in the Jetty implementation but gets wrapped in an IOException by the OkHttp impl but this should be enough for the coverage checker. --- .../client/http/AbstractInterceptorTest.java | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java index fc828738a43..2f5053049ca 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java @@ -22,6 +22,7 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import java.io.InputStream; import java.net.URI; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; @@ -31,6 +32,7 @@ import java.util.List; import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -233,6 +235,37 @@ public void after(HttpRequest request, HttpResponse response, Consumer> response = client.sendAsync(client.newHttpRequestBuilder() + .timeout(1, TimeUnit.SECONDS) + .uri(server.url("/intercepted-url")) + .method("POST", "application/json", new InputStream() { + @Override + public int read() { + throw new CompletionException("boom time", null); // gets propagated by jetty but gets wrapped by OkHttp + } + }, 1L).build(), String.class); + + // Then + assertThat(response).failsWithin(Duration.of(30, ChronoUnit.SECONDS)); + assertThat(connectionFailureCallbackInvoked).extracting(CountDownLatch::getCount).isEqualTo(0L); + } + } + @Test @DisplayName("afterFailure (HTTP), replaces the HttpResponse produced by HttpClient.consumeBytes") public void afterHttpFailureReplacesResponseInConsumeBytes() throws Exception { From 4d0d78fab709b4c91e5747ca7eb8231cbabcbbef Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Tue, 23 Jul 2024 09:22:29 +1200 Subject: [PATCH 06/12] Move afterConnectionFailure callback to `retryWithExponentialBackoff` so its invoked when an established websocket connection fails --- .../client/http/StandardHttpClient.java | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) 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 4e54081732f..1c350f063af 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 @@ -108,13 +108,6 @@ private CompletableFuture> consumeBytesOnce(StandardHttp final Consumer> effectiveConsumer = consumer; CompletableFuture> cf = consumeBytesDirect(effectiveRequest, effectiveConsumer); - cf.exceptionally(throwable -> { - builder.interceptors.forEach((s, interceptor) -> interceptor.afterConnectionFailure(effectiveRequest, throwable)); - if (throwable instanceof CompletionException) { - throw (CompletionException) throwable; - } - throw new CompletionException(throwable); - }); cf.thenAccept( response -> builder.getInterceptors().values().forEach(i -> i.after(effectiveRequest, response, effectiveConsumer))); @@ -184,15 +177,19 @@ private CompletableFuture retryWithExponentialBackoff( } } } else { + final Throwable finalThrowable; if (throwable instanceof CompletionException) { - throwable = throwable.getCause(); + finalThrowable = throwable.getCause(); + } else { + finalThrowable = throwable; } - if (throwable instanceof IOException) { + builder.interceptors.forEach((s, interceptor) -> interceptor.afterConnectionFailure(request, finalThrowable)); + if (finalThrowable 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), - throwable); + finalThrowable); return true; } } From ff3accdf6df3c100cdcbe6b740758c917279fb08 Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Tue, 23 Jul 2024 09:28:44 +1200 Subject: [PATCH 07/12] restart the mock server after a connection failure is detected to speed up tests. Rather than waiting ~20s to give up retrying. --- .../client/http/AbstractInterceptorTest.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java index 2f5053049ca..18385888034 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java @@ -41,6 +41,7 @@ public abstract class AbstractInterceptorTest { + private static final Duration FUTURE_COMPLETION_TIME = Duration.of(10, ChronoUnit.SECONDS); private static DefaultMockServer server; @BeforeEach @@ -179,6 +180,7 @@ public CompletableFuture afterFailure(BasicBuilder builder, HttpRespons @DisplayName("afterConnectionFailure, invoked when remote server offline") public void afterConnectionFailureRemoteOffline() { // Given + final int originalPort = server.getPort(); server.shutdown(); final CountDownLatch connectionFailureCallbackInvoked = new CountDownLatch(1); final HttpClient.Builder builder = getHttpClientFactory().newBuilder() @@ -187,6 +189,8 @@ public void afterConnectionFailureRemoteOffline() { @Override public void afterConnectionFailure(HttpRequest request, Throwable failure) { connectionFailureCallbackInvoked.countDown(); + server = newMockServer(); + server.start(originalPort); // Need to restart on the original port as we can't alter the request during retry. } }); // When @@ -196,7 +200,7 @@ public void afterConnectionFailure(HttpRequest request, Throwable failure) { .uri(server.url("/not-found")).build(), String.class); // Then - assertThat(response).failsWithin(Duration.of(30, ChronoUnit.SECONDS)); + assertThat(response).succeedsWithin(FUTURE_COMPLETION_TIME); assertThat(connectionFailureCallbackInvoked).extracting(CountDownLatch::getCount).isEqualTo(0L); } } @@ -214,8 +218,8 @@ public void afterConnectionFailureRetry() { @Override public void afterConnectionFailure(HttpRequest request, Throwable failure) { server = newMockServer(); - server.expect().withPath("/intercepted-url").andReturn(200, "This works").once(); server.start(originalPort); // Need to restart on the original port as we can't alter the request during retry. + server.expect().withPath("/intercepted-url").andReturn(200, "This works").once(); } @Override @@ -230,7 +234,7 @@ public void after(HttpRequest request, HttpResponse response, Consumer Date: Tue, 23 Jul 2024 09:31:23 +1200 Subject: [PATCH 08/12] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94f2781d77f..5d68ce0ba95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ * Fix #6137: `ConfigBuilder.withAutoConfigure` is not working * Fix #6215: Suppressing rejected execution exception for port forwarder * Fix #6197: JettyHttp client error handling improvements. +* Fix #6143: Expands the HTTP interceptor API to include a call back for failed connection attempts #### Improvements * Fix #6008: removing the optional dependency on bouncy castle From a7c69621c3e1ae7d51510f0a788c9d2e594dcc7d Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Wed, 24 Jul 2024 13:28:12 +1200 Subject: [PATCH 09/12] Clarify javadoc --- .../java/io/fabric8/kubernetes/client/http/Interceptor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java index 581fca1954c..cd5af5100e9 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java @@ -100,7 +100,7 @@ default CompletableFuture afterFailure(HttpRequest.Builder builder, Htt /** * Called after a connection attempt fails. *

- * This method will be invoked on each failed connection attempt so can be invoked multiple times for the same request.ID. + * This method will be invoked on each failed connection attempt. * * @param request the HTTP request. * @param failure the Java exception that caused the failure. From dc27f5eef840cadb213ded4d1b8bed4e1c542d1f Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Thu, 25 Jul 2024 09:04:33 +1200 Subject: [PATCH 10/12] @see -> @link --- .../java/io/fabric8/kubernetes/client/http/Interceptor.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java index cd5af5100e9..16e3bcc2196 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/http/Interceptor.java @@ -72,7 +72,7 @@ default AsyncBody.Consumer> consumer(AsyncBody.Consumer - * Failure is determined by HTTP status code and will be invoked in addition to {@see Interceptor#after(HttpRequest, + * Failure is determined by HTTP status code and will be invoked in addition to {@link Interceptor#after(HttpRequest, * HttpResponse, AsyncBody.Consumer)} * * @param builder used to modify the request @@ -86,7 +86,7 @@ default CompletableFuture afterFailure(BasicBuilder builder, HttpRespon /** * Called after a non-websocket failure *

- * Failure is determined by HTTP status code and will be invoked in addition to {@see Interceptor#after(HttpRequest, + * Failure is determined by HTTP status code and will be invoked in addition to {@link Interceptor#after(HttpRequest, * HttpResponse, AsyncBody.Consumer)} * * @param builder used to modify the request From e9e4b2c95106aa6440a0f86ec7cdddd0a915a287 Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Fri, 9 Aug 2024 11:36:32 +1200 Subject: [PATCH 11/12] Add unit test to ensure we unwrap CompletionExceptions. Really just making the coverage checker happy. --- .../client/http/StandardHttpClient.java | 23 +++++++++++------- .../client/http/StandardHttpClientTest.java | 24 +++++++++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-) 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 1c350f063af..1a415681f0c 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 @@ -177,19 +177,14 @@ private CompletableFuture retryWithExponentialBackoff( } } } else { - final Throwable finalThrowable; - if (throwable instanceof CompletionException) { - finalThrowable = throwable.getCause(); - } else { - finalThrowable = throwable; - } - builder.interceptors.forEach((s, interceptor) -> interceptor.afterConnectionFailure(request, finalThrowable)); - if (finalThrowable instanceof IOException) { + 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), - finalThrowable); + actualCause); return true; } } @@ -197,6 +192,16 @@ private CompletableFuture retryWithExponentialBackoff( }); } + static Throwable unwrapCompletionException(Throwable throwable) { + final Throwable actualCause; + if (throwable instanceof CompletionException) { + actualCause = throwable.getCause(); + } else { + actualCause = throwable; + } + return actualCause; + } + static long retryAfterMillis(HttpResponse httpResponse) { String retryAfter = httpResponse.header(StandardHttpHeaders.RETRY_AFTER); if (retryAfter != null) { 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 038ceed414c..387f6aefacf 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 @@ -34,6 +34,7 @@ import java.util.Collections; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -50,6 +51,7 @@ class StandardHttpClientTest { + public static final String IO_ERROR_MESSAGE = "IO woopsie"; private TestStandardHttpClient client; @BeforeEach @@ -281,4 +283,26 @@ void testDerivedIsClosed() { assertTrue(client.isClosed()); } + @Test + void shouldUnwrapCompletionException() { + // Given + + // When + final Throwable throwable = StandardHttpClient + .unwrapCompletionException(new CompletionException(new IOException(IO_ERROR_MESSAGE))); + + // Then + assertThat(throwable).isInstanceOf(IOException.class).hasMessage(IO_ERROR_MESSAGE); + } + + @Test + void shouldNotUnwrapOtherExceptions() { + // Given + + // When + final Throwable throwable = StandardHttpClient.unwrapCompletionException(new IOException(IO_ERROR_MESSAGE)); + + // Then + assertThat(throwable).isInstanceOf(IOException.class).hasMessage(IO_ERROR_MESSAGE); + } } From b8b79d41e69e7bb023e7e1be99b112e2949be68c Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Fri, 9 Aug 2024 12:10:32 +1200 Subject: [PATCH 12/12] Drop exception handling test from AbstractInterceptorTest as its now better covered elsewhere. --- .../client/http/AbstractInterceptorTest.java | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java index 18385888034..336729f3037 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; -import java.io.InputStream; import java.net.URI; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; @@ -32,7 +31,6 @@ import java.util.List; import java.util.Set; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CompletionException; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -239,37 +237,6 @@ public void after(HttpRequest request, HttpResponse response, Consumer> response = client.sendAsync(client.newHttpRequestBuilder() - .timeout(1, TimeUnit.SECONDS) - .uri(server.url("/intercepted-url")) - .method("POST", "application/json", new InputStream() { - @Override - public int read() { - throw new CompletionException("boom time", null); // gets propagated by jetty but gets wrapped by OkHttp - } - }, 1L).build(), String.class); - - // Then - assertThat(response).failsWithin(FUTURE_COMPLETION_TIME); - assertThat(connectionFailureCallbackInvoked).extracting(CountDownLatch::getCount).isEqualTo(0L); - } - } - @Test @DisplayName("afterFailure (HTTP), replaces the HttpResponse produced by HttpClient.consumeBytes") public void afterHttpFailureReplacesResponseInConsumeBytes() throws Exception {