From 0fa8276d2dcd5765be2fea89fde2149f404dc51a Mon Sep 17 00:00:00 2001 From: ravi-signal <99042880+ravi-signal@users.noreply.github.com> Date: Thu, 14 Sep 2023 16:07:35 -0500 Subject: [PATCH] retry hCaptcha errors Co-authored-by: Jon Chambers <63609320+jon-signal@users.noreply.github.com> --- .../textsecuregcm/WhisperServerService.java | 10 ++-- .../textsecuregcm/captcha/HCaptchaClient.java | 50 +++++++++++++++---- .../configuration/HCaptchaConfiguration.java | 29 ++++++++++- .../http/FaultTolerantHttpClient.java | 25 +++++++--- .../captcha/HCaptchaClientTest.java | 17 ++++--- .../http/FaultTolerantHttpClientTest.java | 36 +++++++++++++ 6 files changed, 140 insertions(+), 27 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 7502db26f..5d2513626 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -406,6 +406,8 @@ public void run(WhisperServerConfiguration config, Environment environment) thro .scheduledExecutorService(name(getClass(), "secureValueRecoveryServiceRetry-%d")).threads(1).build(); ScheduledExecutorService storageServiceRetryExecutor = environment.lifecycle() .scheduledExecutorService(name(getClass(), "storageServiceRetry-%d")).threads(1).build(); + ScheduledExecutorService hcaptchaRetryExecutor = environment.lifecycle() + .scheduledExecutorService(name(getClass(), "hCaptchaRetry-%d")).threads(1).build(); Scheduler messageDeliveryScheduler = Schedulers.fromExecutorService( ExecutorServiceMetrics.monitor(Metrics.globalRegistry, @@ -569,9 +571,11 @@ public void run(WhisperServerConfiguration config, Environment environment) thro config.getRecaptchaConfiguration().projectPath(), config.getRecaptchaConfiguration().credentialConfigurationJson(), dynamicConfigurationManager); - HttpClient hcaptchaHttpClient = HttpClient.newBuilder().version(HttpClient.Version.HTTP_2) - .connectTimeout(Duration.ofSeconds(10)).build(); - HCaptchaClient hCaptchaClient = new HCaptchaClient(config.getHCaptchaConfiguration().apiKey().value(), hcaptchaHttpClient, + HCaptchaClient hCaptchaClient = new HCaptchaClient( + config.getHCaptchaConfiguration().getApiKey().value(), + hcaptchaRetryExecutor, + config.getHCaptchaConfiguration().getCircuitBreaker(), + config.getHCaptchaConfiguration().getRetry(), dynamicConfigurationManager); HttpClient shortCodeRetrieverHttpClient = HttpClient.newBuilder().version(HttpClient.Version.HTTP_2) .connectTimeout(Duration.ofSeconds(10)).build(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java index f74601669..b78fa60ad 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClient.java @@ -7,6 +7,7 @@ import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; +import com.google.common.annotations.VisibleForTesting; import io.micrometer.core.instrument.Metrics; import java.io.IOException; import java.math.BigDecimal; @@ -16,15 +17,25 @@ import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.nio.charset.StandardCharsets; +import java.time.Duration; import java.util.Collections; import java.util.Optional; import java.util.Set; +import java.util.concurrent.CompletionException; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import javax.ws.rs.core.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration; +import org.whispersystems.textsecuregcm.configuration.RetryConfiguration; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; +import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; +import org.whispersystems.textsecuregcm.util.ExceptionUtils; import org.whispersystems.textsecuregcm.util.SystemMapper; public class HCaptchaClient implements CaptchaClient { @@ -34,18 +45,38 @@ public class HCaptchaClient implements CaptchaClient { private static final String ASSESSMENT_REASON_COUNTER_NAME = name(HCaptchaClient.class, "assessmentReason"); private static final String INVALID_REASON_COUNTER_NAME = name(HCaptchaClient.class, "invalidReason"); private final String apiKey; - private final HttpClient client; + private final FaultTolerantHttpClient client; private final DynamicConfigurationManager dynamicConfigurationManager; - public HCaptchaClient( - final String apiKey, - final HttpClient client, + @VisibleForTesting + HCaptchaClient(final String apiKey, + final FaultTolerantHttpClient faultTolerantHttpClient, final DynamicConfigurationManager dynamicConfigurationManager) { this.apiKey = apiKey; - this.client = client; + this.client = faultTolerantHttpClient; this.dynamicConfigurationManager = dynamicConfigurationManager; } + public HCaptchaClient( + final String apiKey, + final ScheduledExecutorService retryExecutor, + final CircuitBreakerConfiguration circuitBreakerConfiguration, + final RetryConfiguration retryConfiguration, + final DynamicConfigurationManager dynamicConfigurationManager) { + this(apiKey, + FaultTolerantHttpClient.newBuilder() + .withName("hcaptcha") + .withCircuitBreaker(circuitBreakerConfiguration) + .withExecutor(Executors.newCachedThreadPool()) + .withRetryExecutor(retryExecutor) + .withRetry(retryConfiguration) + .withRetryOnException(ex -> ex instanceof IOException) + .withConnectTimeout(Duration.ofSeconds(10)) + .withVersion(HttpClient.Version.HTTP_2) + .build(), + dynamicConfigurationManager); + } + @Override public String scheme() { return PREFIX; @@ -82,11 +113,12 @@ public AssessmentResult verify( .POST(HttpRequest.BodyPublishers.ofString(body)) .build(); - HttpResponse response; + final HttpResponse response; try { - response = this.client.send(request, HttpResponse.BodyHandlers.ofString()); - } catch (InterruptedException e) { - throw new IOException(e); + response = this.client.sendAsync(request, HttpResponse.BodyHandlers.ofString()).join(); + } catch (CompletionException e) { + logger.warn("failed to make http request to hCaptcha: {}", e.getMessage()); + throw new IOException(ExceptionUtils.unwrap(e)); } if (response.statusCode() != Response.Status.OK.getStatusCode()) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java index 6c2ea26bf..d5a334e3d 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/configuration/HCaptchaConfiguration.java @@ -5,8 +5,35 @@ package org.whispersystems.textsecuregcm.configuration; +import com.fasterxml.jackson.annotation.JsonProperty; import javax.validation.constraints.NotNull; import org.whispersystems.textsecuregcm.configuration.secrets.SecretString; -public record HCaptchaConfiguration(@NotNull SecretString apiKey) { +public class HCaptchaConfiguration { + + @JsonProperty + @NotNull + SecretString apiKey; + + @JsonProperty + @NotNull + CircuitBreakerConfiguration circuitBreaker = new CircuitBreakerConfiguration(); + + @JsonProperty + @NotNull + RetryConfiguration retry = new RetryConfiguration(); + + + public SecretString getApiKey() { + return apiKey; + } + + public CircuitBreakerConfiguration getCircuitBreaker() { + return circuitBreaker; + } + + public RetryConfiguration getRetry() { + return retry; + } + } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java b/service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java index 21b608a2b..ef729c136 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClient.java @@ -5,6 +5,7 @@ package org.whispersystems.textsecuregcm.http; +import com.google.common.annotations.VisibleForTesting; import io.github.resilience4j.circuitbreaker.CircuitBreaker; import io.github.resilience4j.retry.Retry; import io.github.resilience4j.retry.RetryConfig; @@ -18,12 +19,14 @@ import java.util.concurrent.CompletionStage; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; +import java.util.function.Predicate; import java.util.function.Supplier; import org.glassfish.jersey.SslConfigurator; import org.whispersystems.textsecuregcm.configuration.CircuitBreakerConfiguration; import org.whispersystems.textsecuregcm.configuration.RetryConfiguration; import org.whispersystems.textsecuregcm.util.CertificateUtil; import org.whispersystems.textsecuregcm.util.CircuitBreakerUtil; +import org.whispersystems.textsecuregcm.util.ExceptionUtils; public class FaultTolerantHttpClient { @@ -40,9 +43,10 @@ public static Builder newBuilder() { return new Builder(); } - private FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExecutorService retryExecutor, + @VisibleForTesting + FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExecutorService retryExecutor, Duration defaultRequestTimeout, RetryConfiguration retryConfiguration, - CircuitBreakerConfiguration circuitBreakerConfiguration) { + final Predicate retryOnException, CircuitBreakerConfiguration circuitBreakerConfiguration) { this.httpClient = httpClient; this.retryExecutor = retryExecutor; @@ -55,9 +59,12 @@ private FaultTolerantHttpClient(String name, HttpClient httpClient, ScheduledExe if (this.retryExecutor == null) { throw new IllegalArgumentException("retryExecutor must be specified with retryConfiguration"); } - RetryConfig retryConfig = retryConfiguration.toRetryConfigBuilder() - .retryOnResult(o -> o.statusCode() >= 500).build(); - this.retry = Retry.of(name + "-retry", retryConfig); + final RetryConfig.Builder retryConfig = retryConfiguration.toRetryConfigBuilder() + .retryOnResult(o -> o.statusCode() >= 500); + if (retryOnException != null) { + retryConfig.retryOnException(retryOnException); + } + this.retry = Retry.of(name + "-retry", retryConfig.build()); CircuitBreakerUtil.registerMetrics(retry, FaultTolerantHttpClient.class); } else { this.retry = null; @@ -101,6 +108,7 @@ public static class Builder { private KeyStore trustStore; private String securityProtocol = SECURITY_PROTOCOL_TLS_1_2; private RetryConfiguration retryConfiguration; + private Predicate retryOnException; private CircuitBreakerConfiguration circuitBreakerConfiguration; private Builder() { @@ -161,6 +169,11 @@ public Builder withTrustedServerCertificates(final String... certificatePem) thr return this; } + public Builder withRetryOnException(final Predicate predicate) { + this.retryOnException = throwable -> predicate.test(ExceptionUtils.unwrap(throwable)); + return this; + } + public FaultTolerantHttpClient build() { if (this.circuitBreakerConfiguration == null || this.name == null || this.executor == null) { throw new IllegalArgumentException("Must specify circuit breaker config, name, and executor"); @@ -181,7 +194,7 @@ public FaultTolerantHttpClient build() { builder.sslContext(sslConfigurator.createSSLContext()); return new FaultTolerantHttpClient(name, builder.build(), retryExecutor, requestTimeout, retryConfiguration, - circuitBreakerConfiguration); + retryOnException, circuitBreakerConfiguration); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java index 04e22bc03..6f6d98597 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/captcha/HCaptchaClientTest.java @@ -15,6 +15,7 @@ import java.util.Collections; import java.util.Map; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -22,6 +23,7 @@ import org.junit.jupiter.params.provider.MethodSource; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicCaptchaConfiguration; import org.whispersystems.textsecuregcm.configuration.dynamic.DynamicConfiguration; +import org.whispersystems.textsecuregcm.http.FaultTolerantHttpClient; import org.whispersystems.textsecuregcm.storage.DynamicConfigurationManager; public class HCaptchaClientTest { @@ -44,7 +46,7 @@ static Stream captchaProcessed() { public void captchaProcessed(final boolean success, final float score, final boolean expectedResult) throws IOException, InterruptedException { - final HttpClient client = mockResponder(200, String.format(""" + final FaultTolerantHttpClient client = mockResponder(200, String.format(""" { "success": %b, "score": %f, @@ -64,14 +66,14 @@ public void captchaProcessed(final boolean success, final float score, final boo @Test public void errorResponse() throws IOException, InterruptedException { - final HttpClient httpClient = mockResponder(503, ""); + final FaultTolerantHttpClient httpClient = mockResponder(503, ""); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); assertThrows(IOException.class, () -> client.verify(SITE_KEY, Action.CHALLENGE, TOKEN, null)); } @Test public void invalidScore() throws IOException, InterruptedException { - final HttpClient httpClient = mockResponder(200, """ + final FaultTolerantHttpClient httpClient = mockResponder(200, """ {"success" : true, "score": 1.1} """); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); @@ -80,7 +82,7 @@ public void invalidScore() throws IOException, InterruptedException { @Test public void badBody() throws IOException, InterruptedException { - final HttpClient httpClient = mockResponder(200, """ + final FaultTolerantHttpClient httpClient = mockResponder(200, """ {"success" : true, """); final HCaptchaClient client = new HCaptchaClient("fake", httpClient, mockConfig(true, 0.5)); @@ -102,15 +104,14 @@ public void badSiteKey() throws IOException { } } - private static HttpClient mockResponder(final int statusCode, final String jsonBody) - throws IOException, InterruptedException { - HttpClient httpClient = mock(HttpClient.class); + private static FaultTolerantHttpClient mockResponder(final int statusCode, final String jsonBody) { + FaultTolerantHttpClient httpClient = mock(FaultTolerantHttpClient.class); @SuppressWarnings("unchecked") final HttpResponse httpResponse = mock(HttpResponse.class); when(httpResponse.body()).thenReturn(jsonBody); when(httpResponse.statusCode()).thenReturn(statusCode); - when(httpClient.send(any(), any())).thenReturn(httpResponse); + when(httpClient.sendAsync(any(), any())).thenReturn(CompletableFuture.completedFuture(httpResponse)); return httpClient; } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java index 81bfb7ef7..f1380dfff 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/http/FaultTolerantHttpClientTest.java @@ -11,6 +11,11 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import com.github.tomakehurst.wiremock.junit5.WireMockExtension; import io.github.resilience4j.circuitbreaker.CallNotPermittedException; @@ -19,6 +24,8 @@ import java.net.http.HttpClient; import java.net.http.HttpRequest; import java.net.http.HttpResponse; +import java.time.Duration; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -114,6 +121,35 @@ void testRetryGet() { wireMock.verify(3, getRequestedFor(urlEqualTo("/failure"))); } + @Test + void testRetryGetOnException() { + final HttpClient mockHttpClient = mock(HttpClient.class); + final FaultTolerantHttpClient client = new FaultTolerantHttpClient( + "test", + mockHttpClient, + retryExecutor, + Duration.ofSeconds(1), + new RetryConfiguration(), + throwable -> throwable instanceof IOException, + new CircuitBreakerConfiguration()); + + when(mockHttpClient.sendAsync(any(), any())) + .thenReturn(CompletableFuture.failedFuture(new IOException("test exception"))); + + HttpRequest request = HttpRequest.newBuilder() + .uri(URI.create("http://localhost:1234/failure")) + .GET() + .build(); + + try { + client.sendAsync(request, HttpResponse.BodyHandlers.ofString()).join(); + throw new AssertionError("Should have failed!"); + } catch (CompletionException e) { + assertThat(e.getCause()).isInstanceOf(IOException.class); + } + verify(mockHttpClient, times(3)).sendAsync(any(), any()); + } + @Test void testNetworkFailureCircuitBreaker() throws InterruptedException { CircuitBreakerConfiguration circuitBreakerConfiguration = new CircuitBreakerConfiguration();