Skip to content

Commit

Permalink
fix: support HTTP client timeouts (MWTELE-70)
Browse files Browse the repository at this point in the history
  • Loading branch information
jponge committed May 2, 2023
1 parent cba303b commit d7f0253
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 1 deletion.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ The following environment variables are available to be overriden when using `En
| `RHT_INSIGHTS_JAVA_PROXY_PORT` | (empty) | Proxy port, if any |
| `RHT_INSIGHTS_JAVA_CONNECT_PERIOD` | 1 day (`P1D`) | Connect period, see `java.time.Duration::parse` for the syntax |
| `RHT_INSIGHTS_JAVA_UPDATE_PERIOD` | 5 minutes (`PT5M`) | Update period, see `java.time.Duration::parse` for the syntax |
| `RHT_INSIGHTS_JAVA_HTTP_CLIENT_TIMEOUT` | 1 minute (`PT1M`) | HTTP client timeout (connection, request) |
| `RHT_INSIGHTS_JAVA_HTTP_CLIENT_RETRY_INITIAL_DELAY` | 2000 (milliseconds as `long`) | HTTP client exponential backoff: initial retry delay in milliseconds |
| `RHT_INSIGHTS_JAVA_HTTP_CLIENT_RETRY_BACKOFF_FACTOR` | 2.0 (`double`) | HTTP client exponential backoff: factor |
| `RHT_INSIGHTS_JAVA_HTTP_CLIENT_RETRY_MAX_ATTEMPTS` | 10 (`int`) | HTTP client exponential backoff: maximum number of retry attempts |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public class EnvAndSysPropsInsightsConfiguration extends DefaultInsightsConfigur
"RHT_INSIGHTS_JAVA_HTTP_CLIENT_RETRY_BACKOFF_FACTOR";
public static final String ENV_HTTP_CLIENT_RETRY_MAX_ATTEMPTS =
"RHT_INSIGHTS_JAVA_HTTP_CLIENT_RETRY_MAX_ATTEMPTS";
public static final String ENV_HTTP_CLIENT_TIMEOUT = "RHT_INSIGHTS_JAVA_HTTP_CLIENT_TIMEOUT";

public static final String ENV_CERT_HELPER_BINARY = "RHT_INSIGHTS_JAVA_CERT_HELPER_BINARY";

Expand Down Expand Up @@ -147,6 +148,15 @@ public Duration getUpdatePeriod() {
return super.getUpdatePeriod();
}

@Override
public Duration getHttpClientTimeout() {
String value = lookup(ENV_HTTP_CLIENT_TIMEOUT);
if (value != null) {
return Duration.parse(value);
}
return super.getHttpClientTimeout();
}

@Override
public long getHttpClientRetryInitialDelay() {
String value = lookup(ENV_HTTP_CLIENT_RETRY_INITIAL_DELAY);
Expand Down Expand Up @@ -218,6 +228,8 @@ public String toString() {
+ getCertHelperBinary()
+ ", machineIdFilePath = "
+ getMachineIdFilePath()
+ ", httpClientTimeout = "
+ getHttpClientTimeout()
+ '}';
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ default String getCertHelperBinary() {
return DEFAULT_CERT_HELPER_BINARY;
}

default Duration getHttpClientTimeout() {
return Duration.ofMinutes(1);
}

final class ProxyConfiguration {

private final String host;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class EnvVariableConfigurationTest {
.set(ENV_HTTP_CLIENT_RETRY_INITIAL_DELAY, "5000")
.set(ENV_HTTP_CLIENT_RETRY_BACKOFF_FACTOR, "3")
.set(ENV_HTTP_CLIENT_RETRY_MAX_ATTEMPTS, "5")
.set(ENV_HTTP_CLIENT_TIMEOUT, "PT2M")
.set(ENV_CERT_HELPER_BINARY, "/usr/local/bin/yolo");

@BeforeAll
Expand Down Expand Up @@ -140,4 +141,9 @@ void testBackoff() {
void testCertHelper() {
assertEquals("/usr/local/bin/yolo", config.getCertHelperBinary());
}

@Test
void testHttpClientTimeout() {
assertEquals(Duration.ofMinutes(2), config.getHttpClientTimeout());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public class SysPropConfigurationTest {
.set(ENV_HTTP_CLIENT_RETRY_INITIAL_DELAY.toLowerCase().replace("_", "."), "5000")
.set(ENV_HTTP_CLIENT_RETRY_BACKOFF_FACTOR.toLowerCase().replace("_", "."), "3")
.set(ENV_HTTP_CLIENT_RETRY_MAX_ATTEMPTS.toLowerCase().replace("_", "."), "5")
.set(ENV_HTTP_CLIENT_TIMEOUT.toLowerCase().replace("_", "."), "PT2M")
.set(ENV_CERT_HELPER_BINARY.toLowerCase().replace("_", "."), "/usr/local/bin/yolo");

// clean env variables which might interfere this test
Expand Down Expand Up @@ -154,4 +155,9 @@ void testBackoff() {
void testCertHelper() {
assertEquals("/usr/local/bin/yolo", config.getCertHelperBinary());
}

@Test
void testHttpClientTimeout() {
assertEquals(Duration.ofMinutes(2), config.getHttpClientTimeout());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import java.net.MalformedURLException;
import java.net.URISyntaxException;
import java.net.URL;
import java.time.Duration;
import org.junit.jupiter.api.Test;

public class TestDefaultConfiguration {
Expand Down Expand Up @@ -77,6 +78,11 @@ public void testProxyConf() {
assertEquals(port, proxyConfiguration.getPort());
}

@Test
void testHttpClientTimeout() {
assertEquals(Duration.ofMinutes(1), defaultConfig.getHttpClientTimeout());
}

/*
* This validation is not perfect, but will catch most basic mistakes
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ URI assembleURI(String url, String path) {
HttpClient getHttpClient() {
var clientBuilder = HttpClient.newBuilder();

clientBuilder = clientBuilder.connectTimeout(configuration.getHttpClientTimeout());

if (configuration.useMTLS()) {
final var sslParameters = new SSLParameters();
sslParameters.setWantClientAuth(true);
Expand Down Expand Up @@ -119,7 +121,8 @@ protected void sendInsightsReportWithClient(
var requestBuilder =
HttpRequest.newBuilder()
.version(HttpClient.Version.HTTP_1_1)
.header(MultipartBodyBuilder.CONTENT_TYPE_HEADER, bodyBuilder.contentTypeHeaderValue());
.header(MultipartBodyBuilder.CONTENT_TYPE_HEADER, bodyBuilder.contentTypeHeaderValue())
.timeout(configuration.getHttpClientTimeout());

if (!configuration.useMTLS()) {
final var authToken = configuration.getMaybeAuthToken().get();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.net.http.HttpResponse;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
import java.util.*;
import java.util.concurrent.atomic.AtomicReference;
import javax.net.ssl.SSLContext;
Expand All @@ -46,6 +47,7 @@ public void testHttpClientWithMTLS() {
public void testHttpClientWithoutMTLS() {
InsightsConfiguration config = mock(InsightsConfiguration.class);
when(config.getMaybeAuthToken()).thenReturn(Optional.of("randomToken"));
when(config.getHttpClientTimeout()).thenReturn(Duration.ofSeconds(30));

InsightsJdkHttpClient insightsClient = new InsightsJdkHttpClient(logger, config);
HttpClient httpClient = insightsClient.getHttpClient();
Expand All @@ -60,6 +62,7 @@ public void testHttpClientWithProxy() {
InsightsConfiguration config = mock(InsightsConfiguration.class);
when(config.getProxyConfiguration())
.thenReturn(Optional.of(new InsightsConfiguration.ProxyConfiguration("localhost", 8080)));
when(config.getHttpClientTimeout()).thenReturn(Duration.ofSeconds(30));

InsightsJdkHttpClient insightsClient = new InsightsJdkHttpClient(logger, config);
HttpClient httpClient = insightsClient.getHttpClient();
Expand Down Expand Up @@ -183,6 +186,7 @@ public void testSendReportWithoutMtls() throws IOException, InterruptedException
when(config.getMaybeAuthToken()).thenReturn(Optional.of("randomToken"));
when(config.getUploadBaseURL()).thenReturn("https://site.com");
when(config.getUploadUri()).thenReturn("/path");
when(config.getHttpClientTimeout()).thenReturn(Duration.ofSeconds(30));

HttpClient httpClient = mock(HttpClient.class);
PEMSupport pem = new PEMSupport(logger, config);
Expand Down

0 comments on commit d7f0253

Please sign in to comment.