diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java index 3c2616827..e247522ce 100644 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java @@ -46,7 +46,6 @@ import java.net.InetSocketAddress; import java.net.MalformedURLException; import java.net.Proxy; -import java.net.Socket; import java.net.URI; import java.net.URL; import java.time.Duration; @@ -58,7 +57,7 @@ import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledFuture; -import java.util.function.Supplier; +import java.util.concurrent.TimeUnit; import java.util.stream.LongStream; import javax.annotation.Nullable; import javax.net.ssl.SSLSocketFactory; @@ -79,10 +78,14 @@ import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; import org.apache.hc.client5.http.impl.classic.HttpClients; +import org.apache.hc.client5.http.impl.io.DefaultHttpClientConnectionOperator; import org.apache.hc.client5.http.impl.io.ManagedHttpClientConnectionFactory; import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager; -import org.apache.hc.client5.http.socket.ConnectionSocketFactory; +import org.apache.hc.client5.http.io.DetachedSocketFactory; +import org.apache.hc.client5.http.io.HttpClientConnectionOperator; import org.apache.hc.client5.http.ssl.DefaultHostnameVerifier; +import org.apache.hc.client5.http.ssl.TlsSocketStrategy; +import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.URIScheme; import org.apache.hc.core5.http.config.RegistryBuilder; import org.apache.hc.core5.http.io.SocketConfig; @@ -394,31 +397,6 @@ public static ClientBuilder clientBuilder() { public static final class ClientBuilder { - // Most of our servers use a keep-alive timeout of one minute, by using a slightly lower value on the - // client side we can avoid unnecessary retries due to race conditions when servers close idle connections - // as clients attempt to use them. - // Note that pooled idle connections use an infinite socket timeout so there is no reason to scale - // this value with configured timeouts. - private static final Timeout IDLE_CONNECTION_TIMEOUT = Timeout.ofSeconds(50); - - // Increased from two seconds to four seconds because we have strong support for retries - // and can optimistically avoid expensive connection checks. Failures caused by NoHttpResponseExceptions - // are possible when the target closes connections prior to this timeout, and can be safely retried. - // Ideally this value would be larger for RPC, however some servers use relatively low defaults: - // apache httpd versions 1.3 and 2.0: 15 seconds: - // https://httpd.apache.org/docs/2.0/mod/core.html#keepalivetimeout - // apache httpd version 2.2 and above: 5 seconds - // https://httpd.apache.org/docs/2.2/mod/core.html#keepalivetimeout - // nodejs http server: 5 seconds - // https://nodejs.org/api/http.html#http_server_keepalivetimeout - // nginx: 75 seconds (good) - // https://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_timeout - // dropwizard: 30 seconds (see idleTimeout in the linked docs) - // https://www.dropwizard.io/en/latest/manual/configuration.html#Connectors - // wc: 60 seconds (internal) - private static final TimeValue CONNECTION_INACTIVITY_CHECK = TimeValue.ofMilliseconds( - Integer.getInteger("dialogue.experimental.inactivity.check.threshold.millis", 4_000)); - @Nullable private ClientConfiguration clientConfiguration; @@ -485,40 +463,56 @@ public CloseableClient build() { InetSocketAddress socksProxyAddress = getSocksProxyAddress(conf); SSLSocketFactory rawSocketFactory = conf.sslSocketFactory(); - Supplier simpleSocketCreator = socksProxyAddress == null - ? () -> new Socket(Proxy.NO_PROXY) - : () -> new Socket(new Proxy(Proxy.Type.SOCKS, socksProxyAddress)); + SSLSocketFactory instrumentedSocketFactory = + MetricRegistries.instrument(conf.taggedMetricRegistry(), rawSocketFactory, name); + DetachedSocketFactory plainSocketFactory = new SocksSupportingDetachedSocketFactory(socksProxyAddress); + + TlsSocketStrategy tlsStrategy = new DialogueTlsSocketStrategy( + instrumentedSocketFactory, + TlsProtocols.get(), + supportedCipherSuites(CipherSuites.allCipherSuites(), rawSocketFactory, name), + new InstrumentedHostnameVerifier(new DefaultHostnameVerifier(), name, conf.taggedMetricRegistry())); ConnectInstrumentation connectInstrumentation = new ConnectInstrumentation(conf.taggedMetricRegistry(), name); + InstrumentedDnsResolver instrumentedDnsResolver = new InstrumentedDnsResolver( + SystemDefaultDnsResolver.INSTANCE, dnsResolver, name, conf.taggedMetricRegistry()); + + HttpClientConnectionOperator operator = + new DefaultHttpClientConnectionOperator( + plainSocketFactory, + null, + instrumentedDnsResolver, + RegistryBuilder.create() + .register(URIScheme.HTTPS.id, tlsStrategy) + .build()) { + private static final String CONNECT_BEGAN_ATTRIBUTE = "onBeforeSocketConnectNanoTime"; + + @Override + protected void onBeforeSocketConnect(HttpContext httpContext, HttpHost endpointHost) { + super.onBeforeSocketConnect(httpContext, endpointHost); + httpContext.setAttribute(CONNECT_BEGAN_ATTRIBUTE, System.nanoTime()); + } + + @Override + protected void onAfterSocketConnect(HttpContext httpContext, HttpHost endpointHost) { + super.onAfterSocketConnect(httpContext, endpointHost); + Object value = httpContext.getAttribute(CONNECT_BEGAN_ATTRIBUTE); + if (value instanceof Long) { + long duration = System.nanoTime() - (long) value; + connectInstrumentation.timer(true, httpContext).update(duration, TimeUnit.NANOSECONDS); + } + } + }; + PoolingHttpClientConnectionManager internalConnectionManager = new PoolingHttpClientConnectionManager( - RegistryBuilder.create() - .register( - URIScheme.HTTP.id, - new InstrumentedPlainConnectionSocketFactory( - simpleSocketCreator, connectInstrumentation)) - .register( - URIScheme.HTTPS.id, - new InstrumentedSslConnectionSocketFactory( - connectInstrumentation, - MetricRegistries.instrument( - conf.taggedMetricRegistry(), rawSocketFactory, name), - TlsProtocols.get(), - supportedCipherSuites( - CipherSuites.allCipherSuites(), rawSocketFactory, name), - new InstrumentedHostnameVerifier( - new DefaultHostnameVerifier(), name, conf.taggedMetricRegistry()), - simpleSocketCreator)) - .build(), + operator, PoolConcurrencyPolicy.LAX, // Allow unnecessary connections to time out reducing system load. PoolReusePolicy.LIFO, // No maximum time to live TimeValue.NEG_ONE_MILLISECOND, - null, - new InstrumentedDnsResolver( - SystemDefaultDnsResolver.INSTANCE, dnsResolver, name, conf.taggedMetricRegistry()), new InstrumentedManagedHttpConnectionFactory( ManagedHttpClientConnectionFactory.INSTANCE, conf.taggedMetricRegistry(), name)); internalConnectionManager.setDefaultSocketConfig(SocketConfig.custom() @@ -530,7 +524,9 @@ public CloseableClient build() { // Doesn't appear to do anything in this release .setSocksProxyAddress(socksProxyAddress) .build()); - internalConnectionManager.setValidateAfterInactivity(CONNECTION_INACTIVITY_CHECK); + DialogueConnectionConfigResolver connectionConfigResolver = + new DialogueConnectionConfigResolver(connectTimeout, socketTimeout); + internalConnectionManager.setConnectionConfigResolver(connectionConfigResolver); internalConnectionManager.setMaxTotal(Integer.MAX_VALUE); internalConnectionManager.setDefaultMaxPerRoute(Integer.MAX_VALUE); @@ -542,7 +538,6 @@ public CloseableClient build() { HttpClientBuilder builder = HttpClients.custom() .setDefaultRequestConfig(RequestConfig.custom() - .setConnectTimeout(connectTimeout) // Don't allow clients to block forever waiting on a connection to become available .setConnectionRequestTimeout(connectTimeout) // The response timeout is used as the socket timeout for the duration of @@ -552,13 +547,14 @@ public CloseableClient build() { .setRedirectsEnabled(false) .setAuthenticationEnabled(conf.proxyCredentials().isPresent()) .setExpectContinueEnabled(false) - .setConnectionKeepAlive(IDLE_CONNECTION_TIMEOUT) + .setConnectionKeepAlive( + InactivityValidationAwareConnectionKeepAliveStrategy.IDLE_CONNECTION_TIMEOUT) .build()) // Connection pool lifecycle must be managed separately. This allows us to configure a more // precise IdleConnectionEvictor. .setConnectionManagerShared(true) .setKeepAliveStrategy( - new InactivityValidationAwareConnectionKeepAliveStrategy(internalConnectionManager, name)) + new InactivityValidationAwareConnectionKeepAliveStrategy(connectionConfigResolver, name)) .setConnectionManager(connectionManager) .setRoutePlanner(new DialogueRoutePlanner(conf.proxy())) .disableAutomaticRetries() diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/DialogueConnectionConfigResolver.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/DialogueConnectionConfigResolver.java new file mode 100644 index 000000000..79d6c66ad --- /dev/null +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/DialogueConnectionConfigResolver.java @@ -0,0 +1,74 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue.hc5; + +import org.apache.hc.client5.http.HttpRoute; +import org.apache.hc.client5.http.config.ConnectionConfig; +import org.apache.hc.core5.function.Resolver; +import org.apache.hc.core5.util.TimeValue; +import org.apache.hc.core5.util.Timeout; + +final class DialogueConnectionConfigResolver implements Resolver { + + // Increased from two seconds to four seconds because we have strong support for retries + // and can optimistically avoid expensive connection checks. Failures caused by NoHttpResponseExceptions + // are possible when the target closes connections prior to this timeout, and can be safely retried. + // Ideally this value would be larger for RPC, however some servers use relatively low defaults: + // apache httpd versions 1.3 and 2.0: 15 seconds: + // https://httpd.apache.org/docs/2.0/mod/core.html#keepalivetimeout + // apache httpd version 2.2 and above: 5 seconds + // https://httpd.apache.org/docs/2.2/mod/core.html#keepalivetimeout + // nodejs http server: 5 seconds + // https://nodejs.org/api/http.html#http_server_keepalivetimeout + // nginx: 75 seconds (good) + // https://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_timeout + // dropwizard: 30 seconds (see idleTimeout in the linked docs) + // https://www.dropwizard.io/en/latest/manual/configuration.html#Connectors + // wc: 60 seconds (internal) + private static final TimeValue CONNECTION_INACTIVITY_CHECK = TimeValue.ofMilliseconds( + Integer.getInteger("dialogue.experimental.inactivity.check.threshold.millis", 4_000)); + + private final Timeout connectTimeout; + private final Timeout socketTimeout; + + // We create a new connectionConfig when the connectionInactivityCheck interval changes + // to avoid allocating a new ConnectionConfig each time the value is queried. + private volatile ConnectionConfig connectionConfig; + + DialogueConnectionConfigResolver(Timeout connectTimeout, Timeout socketTimeout) { + this.connectTimeout = connectTimeout; + this.socketTimeout = socketTimeout; + setValidateAfterInactivity(CONNECTION_INACTIVITY_CHECK); + } + + void setValidateAfterInactivity(TimeValue connectionInactivityCheck) { + connectionConfig = ConnectionConfig.custom() + .setValidateAfterInactivity(connectionInactivityCheck) + .setConnectTimeout(connectTimeout) + .setSocketTimeout(socketTimeout) + .build(); + } + + TimeValue getValidateAfterInactivity() { + return connectionConfig.getValidateAfterInactivity(); + } + + @Override + public ConnectionConfig resolve(HttpRoute _ignored) { + return connectionConfig; + } +} diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/DialogueTlsSocketStrategy.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/DialogueTlsSocketStrategy.java new file mode 100644 index 000000000..95f470485 --- /dev/null +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/DialogueTlsSocketStrategy.java @@ -0,0 +1,120 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue.hc5; + +import com.palantir.logsafe.Preconditions; +import java.io.IOException; +import java.net.Socket; +import java.security.cert.Certificate; +import java.security.cert.X509Certificate; +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLException; +import javax.net.ssl.SSLParameters; +import javax.net.ssl.SSLPeerUnverifiedException; +import javax.net.ssl.SSLSession; +import javax.net.ssl.SSLSocket; +import javax.net.ssl.SSLSocketFactory; +import org.apache.hc.client5.http.config.TlsConfig; +import org.apache.hc.client5.http.ssl.HttpClientHostnameVerifier; +import org.apache.hc.client5.http.ssl.TlsSocketStrategy; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.io.Closer; +import org.apache.hc.core5.util.Timeout; + +/** + * {@link DialogueTlsSocketStrategy} is based closely on + * {@code org.apache.hc.client5.http.ssl.AbstractClientTlsStrategy}, except that it only requires a + * {@link SSLSocketFactory} rather than an {@link javax.net.ssl.SSLContext}. + * We only implement the minimal required {@link TlsSocketStrategy} interface rather than + * {@link org.apache.hc.core5.http.nio.ssl.TlsStrategy}, which isn't required by socket-based clients. + */ +final class DialogueTlsSocketStrategy implements TlsSocketStrategy { + + private final SSLSocketFactory sslSocketFactory; + private final String[] supportedProtocols; + private final String[] supportedCipherSuites; + private final HostnameVerifier hostnameVerifier; + + DialogueTlsSocketStrategy( + SSLSocketFactory sslSocketFactory, + String[] supportedProtocols, + String[] supportedCipherSuites, + HostnameVerifier hostnameVerifier) { + this.sslSocketFactory = Preconditions.checkNotNull(sslSocketFactory, "SSLSocketFactory is required"); + this.supportedProtocols = Preconditions.checkNotNull(supportedProtocols, "supportedProtocols is required"); + this.supportedCipherSuites = + Preconditions.checkNotNull(supportedCipherSuites, "supportedCipherSuites is required"); + this.hostnameVerifier = Preconditions.checkNotNull(hostnameVerifier, "hostnameVerifier is required"); + } + + @Override + public SSLSocket upgrade(Socket socket, String target, int port, Object attachment, HttpContext _context) + throws IOException { + SSLSocket upgradedSocket = (SSLSocket) sslSocketFactory.createSocket(socket, target, port, false); + try { + executeHandshake(upgradedSocket, target, attachment); + return upgradedSocket; + } catch (IOException | RuntimeException ex) { + Closer.closeQuietly(upgradedSocket); + throw ex; + } + } + + private void executeHandshake(SSLSocket upgradedSocket, String target, Object attachment) throws IOException { + SSLParameters sslParameters = upgradedSocket.getSSLParameters(); + sslParameters.setProtocols(supportedProtocols); + sslParameters.setCipherSuites(supportedCipherSuites); + + // If we want to enable the builtin hostname verification support: + // sslParameters.setEndpointIdentificationAlgorithm(URIScheme.HTTPS.id); + + upgradedSocket.setSSLParameters(sslParameters); + + if (attachment instanceof TlsConfig) { + TlsConfig tlsConfig = (TlsConfig) attachment; + Timeout handshakeTimeout = tlsConfig.getHandshakeTimeout(); + if (handshakeTimeout != null) { + upgradedSocket.setSoTimeout(handshakeTimeout.toMillisecondsIntBound()); + } + } + + upgradedSocket.startHandshake(); + verifySession(target, upgradedSocket.getSession(), hostnameVerifier); + } + + private static void verifySession(String hostname, SSLSession sslsession, HostnameVerifier verifier) + throws SSLException { + if (verifier instanceof HttpClientHostnameVerifier) { + X509Certificate x509Certificate = getX509Certificate(sslsession); + ((HttpClientHostnameVerifier) verifier).verify(hostname, x509Certificate); + } else if (!verifier.verify(hostname, sslsession)) { + throw new SSLPeerUnverifiedException("Certificate doesn't match any of the subject alternative names"); + } + } + + private static X509Certificate getX509Certificate(SSLSession sslsession) throws SSLPeerUnverifiedException { + Certificate[] certs = sslsession.getPeerCertificates(); + if (certs.length < 1) { + throw new SSLPeerUnverifiedException("Peer certificate chain is empty"); + } + Certificate peerCertificate = certs[0]; + if (peerCertificate instanceof X509Certificate) { + return (X509Certificate) peerCertificate; + } + throw new SSLPeerUnverifiedException("Unexpected certificate type: " + peerCertificate.getType()); + } +} diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InactivityValidationAwareConnectionKeepAliveStrategy.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InactivityValidationAwareConnectionKeepAliveStrategy.java index cabc5c841..95bb13d1a 100644 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InactivityValidationAwareConnectionKeepAliveStrategy.java +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InactivityValidationAwareConnectionKeepAliveStrategy.java @@ -25,7 +25,6 @@ import java.util.concurrent.atomic.AtomicReference; import org.apache.hc.client5.http.ConnectionKeepAliveStrategy; import org.apache.hc.client5.http.config.RequestConfig; -import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager; import org.apache.hc.client5.http.protocol.HttpClientContext; import org.apache.hc.core5.http.HeaderElement; import org.apache.hc.core5.http.HeaderElements; @@ -33,20 +32,28 @@ import org.apache.hc.core5.http.message.MessageSupport; import org.apache.hc.core5.http.protocol.HttpContext; import org.apache.hc.core5.util.TimeValue; +import org.apache.hc.core5.util.Timeout; /** * An {@link ConnectionKeepAliveStrategy} implementation based on the * {@link org.apache.hc.client5.http.impl.DefaultConnectionKeepAliveStrategy} which - * updates {@link PoolingHttpClientConnectionManager#setValidateAfterInactivity(TimeValue)} - * based on server {@code Keep-Alive} response headers to avoid unnecessary checks when - * the server advertises a persistent connection timeout. + * updates {@code org.apache.hc.client5.http.config.ConnectionConfig#getValidateAfterInactivity()} + * based on server {@code Keep-Alive} response headers to avoid unnecessary checks when the server + * advertises a persistent connection timeout. */ final class InactivityValidationAwareConnectionKeepAliveStrategy implements ConnectionKeepAliveStrategy { private static final SafeLogger log = SafeLoggerFactory.get(InactivityValidationAwareConnectionKeepAliveStrategy.class); private static final String TIMEOUT_ELEMENT = "timeout"; - private final PoolingHttpClientConnectionManager connectionManager; + // Most of our servers use a keep-alive timeout of one minute, by using a slightly lower value on the + // client side we can avoid unnecessary retries due to race conditions when servers close idle connections + // as clients attempt to use them. + // Note that pooled idle connections use an infinite socket timeout so there is no reason to scale + // this value with configured timeouts. + static final Timeout IDLE_CONNECTION_TIMEOUT = Timeout.ofSeconds(50); + + private final DialogueConnectionConfigResolver configResolver; private final String clientName; private final TimeValue defaultValidateAfterInactivity; private final RateLimiter loggingRateLimiter = RateLimiter.create(2); @@ -57,12 +64,12 @@ final class InactivityValidationAwareConnectionKeepAliveStrategy implements Conn private final AtomicReference currentValidationInterval; InactivityValidationAwareConnectionKeepAliveStrategy( - PoolingHttpClientConnectionManager connectionManager, String clientName) { - this.connectionManager = connectionManager; + DialogueConnectionConfigResolver configResolver, String clientName) { + this.configResolver = configResolver; this.clientName = clientName; // Store the initial inactivity interval to restore if responses re received without // keep-alive headers. - this.defaultValidateAfterInactivity = connectionManager.getValidateAfterInactivity(); + this.defaultValidateAfterInactivity = configResolver.getValidateAfterInactivity(); this.currentValidationInterval = new AtomicReference<>(defaultValidateAfterInactivity); } @@ -86,9 +93,12 @@ public TimeValue getKeepAliveDuration(HttpResponse response, HttpContext context } } } - HttpClientContext clientContext = HttpClientContext.adapt(context); - RequestConfig requestConfig = clientContext.getRequestConfig(); + HttpClientContext clientContext = HttpClientContext.castOrCreate(context); updateInactivityValidationInterval(response.getCode(), defaultValidateAfterInactivity); + RequestConfig requestConfig = clientContext.getRequestConfig(); + if (requestConfig == null) { + return IDLE_CONNECTION_TIMEOUT; + } return requestConfig.getConnectionKeepAlive(); } @@ -109,7 +119,7 @@ private void updateInactivityValidationInterval(int statusCode, TimeValue newInt } // Simple volatile write, no need to protect this in the getAndSet check. The getAndSet may race this call // so it's best to completely decouple the two. - connectionManager.setValidateAfterInactivity(newInterval); + configResolver.setValidateAfterInactivity(newInterval); } } } diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InstrumentedPlainConnectionSocketFactory.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InstrumentedPlainConnectionSocketFactory.java deleted file mode 100644 index 765c3b853..000000000 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InstrumentedPlainConnectionSocketFactory.java +++ /dev/null @@ -1,67 +0,0 @@ -/* - * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.palantir.dialogue.hc5; - -import com.codahale.metrics.Timer; -import java.io.IOException; -import java.net.InetSocketAddress; -import java.net.Socket; -import java.util.concurrent.TimeUnit; -import java.util.function.Supplier; -import org.apache.hc.client5.http.socket.PlainConnectionSocketFactory; -import org.apache.hc.core5.http.HttpHost; -import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.hc.core5.util.TimeValue; - -final class InstrumentedPlainConnectionSocketFactory extends PlainConnectionSocketFactory { - - private final Supplier simpleSocketCreator; - private final ConnectInstrumentation connectInstrumentation; - - InstrumentedPlainConnectionSocketFactory( - Supplier simpleSocketCreator, ConnectInstrumentation connectInstrumentation) { - this.simpleSocketCreator = simpleSocketCreator; - this.connectInstrumentation = connectInstrumentation; - } - - @Override - public Socket connectSocket( - TimeValue connectTimeout, - Socket socket, - HttpHost host, - InetSocketAddress remoteAddress, - InetSocketAddress localAddress, - HttpContext context) - throws IOException { - boolean success = false; - long startNanos = System.nanoTime(); - try { - Socket result = super.connectSocket(connectTimeout, socket, host, remoteAddress, localAddress, context); - success = true; - return result; - } finally { - long durationNanos = System.nanoTime() - startNanos; - Timer timer = connectInstrumentation.timer(success, context); - timer.update(durationNanos, TimeUnit.NANOSECONDS); - } - } - - @Override - public Socket createSocket(HttpContext _context) { - return simpleSocketCreator.get(); - } -} diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InstrumentedSslConnectionSocketFactory.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InstrumentedSslConnectionSocketFactory.java deleted file mode 100644 index 51843d95c..000000000 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/InstrumentedSslConnectionSocketFactory.java +++ /dev/null @@ -1,80 +0,0 @@ -/* - * (c) Copyright 2023 Palantir Technologies Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.palantir.dialogue.hc5; - -import com.codahale.metrics.Timer; -import java.io.IOException; -import java.net.InetSocketAddress; -import java.net.Socket; -import java.net.SocketAddress; -import java.util.concurrent.TimeUnit; -import java.util.function.Supplier; -import javax.net.ssl.HostnameVerifier; -import javax.net.ssl.SSLSocketFactory; -import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory; -import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.hc.core5.util.Timeout; - -/** - * InstrumentedSslConnectionSocketFactory extends {@link SSLConnectionSocketFactory} for a couple minor features. - *
    - *
  1. {@link #rawSocketCreator} provided for socks proxy support.
    2. - *
  2. {@link #connectSocket(Socket, InetSocketAddress, Timeout, HttpContext)} - * overridden to add timing metrics around {@link Socket#connect(SocketAddress, int)}
    3. - *
- */ -final class InstrumentedSslConnectionSocketFactory extends SSLConnectionSocketFactory { - private final Supplier rawSocketCreator; - - private final ConnectInstrumentation connectInstrumentation; - - InstrumentedSslConnectionSocketFactory( - ConnectInstrumentation connectInstrumentation, - SSLSocketFactory socketFactory, - String[] supportedProtocols, - String[] supportedCipherSuites, - HostnameVerifier hostnameVerifier, - Supplier rawSocketCreator) { - super(socketFactory, supportedProtocols, supportedCipherSuites, hostnameVerifier); - this.connectInstrumentation = connectInstrumentation; - this.rawSocketCreator = rawSocketCreator; - } - - @Override - public Socket createSocket(HttpContext _context) { - return rawSocketCreator.get(); - } - - @Override - protected void connectSocket( - final Socket sock, - final InetSocketAddress remoteAddress, - final Timeout connectTimeout, - final HttpContext context) - throws IOException { - boolean success = false; - long startNanos = System.nanoTime(); - try { - super.connectSocket(sock, remoteAddress, connectTimeout, context); - success = true; - } finally { - long durationNanos = System.nanoTime() - startNanos; - Timer timer = connectInstrumentation.timer(success, context); - timer.update(durationNanos, TimeUnit.NANOSECONDS); - } - } -} diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/SocksSupportingDetachedSocketFactory.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/SocksSupportingDetachedSocketFactory.java new file mode 100644 index 000000000..e436095cc --- /dev/null +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/SocksSupportingDetachedSocketFactory.java @@ -0,0 +1,41 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue.hc5; + +import java.io.IOException; +import java.net.InetSocketAddress; +import java.net.Proxy; +import java.net.Socket; +import javax.annotation.Nullable; +import org.apache.hc.client5.http.io.DetachedSocketFactory; + +final class SocksSupportingDetachedSocketFactory implements DetachedSocketFactory { + + @Nullable + private final InetSocketAddress socksProxyAddress; + + SocksSupportingDetachedSocketFactory(@Nullable InetSocketAddress socksProxyAddress) { + this.socksProxyAddress = socksProxyAddress; + } + + @Override + public Socket create(Proxy proxy) throws IOException { + return socksProxyAddress == null + ? new Socket(proxy == null ? Proxy.NO_PROXY : proxy) + : new Socket(new Proxy(Proxy.Type.SOCKS, socksProxyAddress)); + } +} diff --git a/dialogue-apache-hc5-client/src/test/java/com/palantir/dialogue/hc5/InactivityValidationAwareConnectionKeepAliveStrategyTest.java b/dialogue-apache-hc5-client/src/test/java/com/palantir/dialogue/hc5/InactivityValidationAwareConnectionKeepAliveStrategyTest.java index 7caa1ace7..1708d3f9c 100644 --- a/dialogue-apache-hc5-client/src/test/java/com/palantir/dialogue/hc5/InactivityValidationAwareConnectionKeepAliveStrategyTest.java +++ b/dialogue-apache-hc5-client/src/test/java/com/palantir/dialogue/hc5/InactivityValidationAwareConnectionKeepAliveStrategyTest.java @@ -17,20 +17,14 @@ package com.palantir.dialogue.hc5; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; -import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager; import org.apache.hc.client5.http.protocol.HttpClientContext; import org.apache.hc.core5.http.message.BasicClassicHttpResponse; import org.apache.hc.core5.util.TimeValue; +import org.apache.hc.core5.util.Timeout; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @ExtendWith(MockitoExtension.class) @@ -39,89 +33,94 @@ class InactivityValidationAwareConnectionKeepAliveStrategyTest { private static final HttpClientContext CONTEXT = new HttpClientContext(); private static final TimeValue INITIAL_TIMEOUT = TimeValue.ofSeconds(5); - @Mock - private PoolingHttpClientConnectionManager manager; + private final DialogueConnectionConfigResolver resolver = + new DialogueConnectionConfigResolver(Timeout.ofSeconds(2), Timeout.ofSeconds(10)); @BeforeEach void beforeEach() { - when(manager.getValidateAfterInactivity()).thenReturn(INITIAL_TIMEOUT); + resolver.setValidateAfterInactivity(INITIAL_TIMEOUT); + } + + private TimeValue getResolverInactivityInterval() { + return resolver.resolve(null).getValidateAfterInactivity(); } @Test void testNoKeepAliveHeader() { InactivityValidationAwareConnectionKeepAliveStrategy strategy = - new InactivityValidationAwareConnectionKeepAliveStrategy(manager, "name"); + new InactivityValidationAwareConnectionKeepAliveStrategy(resolver, "name"); BasicClassicHttpResponse response = new BasicClassicHttpResponse(200); TimeValue value = strategy.getKeepAliveDuration(response, CONTEXT); - assertThat(value).isEqualTo(CONTEXT.getRequestConfig().getConnectionKeepAlive()); - verify(manager).setValidateAfterInactivity(eq(INITIAL_TIMEOUT)); + assertThat(value).isEqualTo(InactivityValidationAwareConnectionKeepAliveStrategy.IDLE_CONNECTION_TIMEOUT); + + assertThat(getResolverInactivityInterval()).isEqualTo(INITIAL_TIMEOUT); } @Test void testKeepAliveHeaderWithTimeout() { InactivityValidationAwareConnectionKeepAliveStrategy strategy = - new InactivityValidationAwareConnectionKeepAliveStrategy(manager, "name"); + new InactivityValidationAwareConnectionKeepAliveStrategy(resolver, "name"); BasicClassicHttpResponse response = new BasicClassicHttpResponse(200); response.addHeader("Keep-Alive", "timeout=60"); TimeValue value = strategy.getKeepAliveDuration(response, CONTEXT); TimeValue expected = TimeValue.ofSeconds(60); assertThat(value).isEqualTo(expected); - verify(manager).setValidateAfterInactivity(eq(expected)); + assertThat(getResolverInactivityInterval()).isEqualTo(expected); } @Test void testKeepAliveHeaderWithTimeoutAndMax() { InactivityValidationAwareConnectionKeepAliveStrategy strategy = - new InactivityValidationAwareConnectionKeepAliveStrategy(manager, "name"); + new InactivityValidationAwareConnectionKeepAliveStrategy(resolver, "name"); BasicClassicHttpResponse response = new BasicClassicHttpResponse(200); response.addHeader("Keep-Alive", "timeout=60, max=10"); TimeValue value = strategy.getKeepAliveDuration(response, CONTEXT); TimeValue expected = TimeValue.ofSeconds(60); assertThat(value).isEqualTo(expected); - verify(manager).setValidateAfterInactivity(eq(expected)); + assertThat(getResolverInactivityInterval()).isEqualTo(expected); } @Test void testKeepAliveHeaderWithTimeoutIgnoredNon2xx() { InactivityValidationAwareConnectionKeepAliveStrategy strategy = - new InactivityValidationAwareConnectionKeepAliveStrategy(manager, "name"); + new InactivityValidationAwareConnectionKeepAliveStrategy(resolver, "name"); BasicClassicHttpResponse response = new BasicClassicHttpResponse(500); response.addHeader("Keep-Alive", "timeout=60"); TimeValue value = strategy.getKeepAliveDuration(response, CONTEXT); assertThat(value).isEqualTo(TimeValue.ofSeconds(60)); - verify(manager, never()).setValidateAfterInactivity(any()); + assertThat(getResolverInactivityInterval()).isEqualTo(INITIAL_TIMEOUT); } @Test void testKeepAliveHeaderWithoutTimeout() { InactivityValidationAwareConnectionKeepAliveStrategy strategy = - new InactivityValidationAwareConnectionKeepAliveStrategy(manager, "name"); + new InactivityValidationAwareConnectionKeepAliveStrategy(resolver, "name"); BasicClassicHttpResponse response = new BasicClassicHttpResponse(200); response.addHeader("Keep-Alive", "max=60"); TimeValue value = strategy.getKeepAliveDuration(response, CONTEXT); - assertThat(value).isEqualTo(CONTEXT.getRequestConfig().getConnectionKeepAlive()); - verify(manager).setValidateAfterInactivity(eq(INITIAL_TIMEOUT)); + assertThat(value).isEqualTo(InactivityValidationAwareConnectionKeepAliveStrategy.IDLE_CONNECTION_TIMEOUT); + assertThat(getResolverInactivityInterval()).isEqualTo(INITIAL_TIMEOUT); } @Test void testKeepAliveHeaderWithZeroTimeout() { InactivityValidationAwareConnectionKeepAliveStrategy strategy = - new InactivityValidationAwareConnectionKeepAliveStrategy(manager, "name"); + new InactivityValidationAwareConnectionKeepAliveStrategy(resolver, "name"); BasicClassicHttpResponse response = new BasicClassicHttpResponse(200); response.addHeader("Keep-Alive", "timeout=0"); TimeValue value = strategy.getKeepAliveDuration(response, CONTEXT); - assertThat(value).isEqualTo(CONTEXT.getRequestConfig().getConnectionKeepAlive()); - verify(manager).setValidateAfterInactivity(eq(INITIAL_TIMEOUT)); + assertThat(value).isEqualTo(InactivityValidationAwareConnectionKeepAliveStrategy.IDLE_CONNECTION_TIMEOUT); + assertThat(getResolverInactivityInterval()).isEqualTo(INITIAL_TIMEOUT); } @Test void testKeepAliveHeaderWithNegativeTimeout() { InactivityValidationAwareConnectionKeepAliveStrategy strategy = - new InactivityValidationAwareConnectionKeepAliveStrategy(manager, "name"); + new InactivityValidationAwareConnectionKeepAliveStrategy(resolver, "name"); BasicClassicHttpResponse response = new BasicClassicHttpResponse(200); response.addHeader("Keep-Alive", "timeout=-1"); TimeValue value = strategy.getKeepAliveDuration(response, CONTEXT); - assertThat(value).isEqualTo(CONTEXT.getRequestConfig().getConnectionKeepAlive()); - verify(manager).setValidateAfterInactivity(eq(INITIAL_TIMEOUT)); + assertThat(value).isEqualTo(InactivityValidationAwareConnectionKeepAliveStrategy.IDLE_CONNECTION_TIMEOUT); + assertThat(getResolverInactivityInterval()).isEqualTo(INITIAL_TIMEOUT); } } diff --git a/versions.lock b/versions.lock index 637eb7064..772d7a1a9 100644 --- a/versions.lock +++ b/versions.lock @@ -47,9 +47,9 @@ com.squareup:javapoet:1.13.0 (2 constraints: 2b113eee) io.dropwizard.metrics:metrics-core:4.2.27 (4 constraints: d3424214) javax.annotation:javax.annotation-api:1.3.2 (1 constraints: 0805fb35) joda-time:joda-time:2.10.14 (1 constraints: 5b160f08) -org.apache.httpcomponents.client5:httpclient5:5.3.1 (1 constraints: 0b050e36) -org.apache.httpcomponents.core5:httpcore5:5.3 (3 constraints: e229bc23) -org.apache.httpcomponents.core5:httpcore5-h2:5.3 (1 constraints: 3f130d3c) +org.apache.httpcomponents.client5:httpclient5:5.4 (1 constraints: ad042a2c) +org.apache.httpcomponents.core5:httpcore5:5.3 (3 constraints: 8129bbe0) +org.apache.httpcomponents.core5:httpcore5-h2:5.3 (1 constraints: de12c415) org.checkerframework:checker-qual:3.43.0 (4 constraints: 5937f041) org.derive4j:derive4j-annotation:1.1.1 (1 constraints: 0505f435) org.eclipse.collections:eclipse-collections:11.1.0 (1 constraints: 1b108aa9) diff --git a/versions.props b/versions.props index c748262cb..79f674849 100644 --- a/versions.props +++ b/versions.props @@ -17,7 +17,7 @@ com.palantir.tritium:* = 0.92.0 com.squareup:javapoet = 1.13.0 com.uber.nullaway:nullaway = 0.11.2 io.dropwizard.metrics:metrics-core = 4.2.27 -org.apache.httpcomponents.client5:* = 5.3.1 +org.apache.httpcomponents.client5:* = 5.4 org.apache.httpcomponents.core5:* = 5.3 org.derive4j:* = 1.1.1 org.immutables:* = 2.10.1