From 8147542a61ff3a5dc6c52b486e6c674aefbe812d Mon Sep 17 00:00:00 2001 From: Sam Barker Date: Fri, 9 Aug 2024 21:18:06 +1200 Subject: [PATCH] fix: expands the HTTP interceptor API to include a call back for failed connection attempts (6144) Add test which demonstrates what I expect to work --- Introduce afterConnectionFailure to complement `after` & `afterFailure`. Fixes #6143 --- Add a bit more explanatory Javadoc to Interceptor --- Extract private method to ensure changes to DefaultMockServer usage are consistent. --- 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. --- Move afterConnectionFailure callback to `retryWithExponentialBackoff` so its invoked when an established websocket connection fails --- restart the mock server after a connection failure is detected to speed up tests. Rather than waiting ~20s to give up retrying. --- Add changelog entry --- Clarify javadoc --- @see -> @link --- Add unit test to ensure we unwrap CompletionExceptions. Really just making the coverage checker happy. --- Drop exception handling test from AbstractInterceptorTest as its now better covered elsewhere. --- CHANGELOG.md | 1 + .../kubernetes/client/http/Interceptor.java | 28 +++++++- .../client/http/StandardHttpClient.java | 19 +++-- .../client/http/AbstractInterceptorTest.java | 72 ++++++++++++++++++- .../client/http/StandardHttpClientTest.java | 24 +++++++ 5 files changed, 136 insertions(+), 8 deletions(-) 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 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..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 @@ -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 {@link 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 {@link 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 @@ -84,4 +97,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. + * + * @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..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,15 +177,14 @@ private CompletableFuture retryWithExponentialBackoff( } } } else { - if (throwable instanceof CompletionException) { - throwable = throwable.getCause(); - } - if (throwable 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), - throwable); + actualCause); return true; } } @@ -193,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/AbstractInterceptorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/http/AbstractInterceptorTest.java index f85ded10834..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 @@ -25,22 +25,26 @@ 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; 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; public abstract class AbstractInterceptorTest { + private static final Duration FUTURE_COMPLETION_TIME = Duration.of(10, ChronoUnit.SECONDS); private static DefaultMockServer server; @BeforeEach void startServer() { - server = new DefaultMockServer(false); + server = newMockServer(); server.start(); } @@ -170,6 +174,69 @@ public CompletableFuture afterFailure(BasicBuilder builder, HttpRespons } } + @Test + @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() + .connectTimeout(1, TimeUnit.SECONDS) + .addOrReplaceInterceptor("test", new Interceptor() { + @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 + 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).succeedsWithin(FUTURE_COMPLETION_TIME); + 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 = newMockServer(); + 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 + public void after(HttpRequest request, HttpResponse response, Consumer> consumer) { + afterInvoked.countDown(); + } + }); + // When + try (HttpClient client = builder.build()) { + final CompletableFuture> response = client.sendAsync(client.newHttpRequestBuilder() + .timeout(1, TimeUnit.SECONDS) + .uri(server.url("/intercepted-url")).build(), String.class); + + // Then + assertThat(response).succeedsWithin(FUTURE_COMPLETION_TIME); + assertThat(afterInvoked).extracting(CountDownLatch::getCount).isEqualTo(0L); + } + } + @Test @DisplayName("afterFailure (HTTP), replaces the HttpResponse produced by HttpClient.consumeBytes") public void afterHttpFailureReplacesResponseInConsumeBytes() throws Exception { @@ -412,4 +479,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); + } } 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); + } }