Revert "Dialogue fails fast when existing channels are revoked from D…

…NS (#2263)" This reverts commit fb4a00f.
palantir · May 3, 2024 · 0628380 · 0628380
1 parent c160a08
commit 0628380
Show file tree

Hide file tree

Showing 5 changed files with 4 additions and 121 deletions.
diff --git a/...e-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java b/...e-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java
@@ -15,9 +15,6 @@
  */
 package com.palantir.dialogue.hc5;
 
-import com.codahale.metrics.Meter;
-import com.google.common.base.Strings;
-import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.MultimapBuilder;
@@ -32,7 +29,6 @@
 import com.palantir.dialogue.ResponseAttachments;
 import com.palantir.dialogue.blocking.BlockingChannel;
 import com.palantir.dialogue.core.BaseUrl;
-import com.palantir.dialogue.core.DialogueDnsResolver;
 import com.palantir.logsafe.Arg;
 import com.palantir.logsafe.Preconditions;
 import com.palantir.logsafe.SafeArg;
@@ -50,7 +46,6 @@
 import java.io.OutputStream;
 import java.net.InetAddress;
 import java.net.URL;
-import java.time.Duration;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Iterator;
@@ -61,7 +56,6 @@
 import java.util.OptionalLong;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
-import java.util.function.BooleanSupplier;
 import javax.annotation.Nullable;
 import org.apache.hc.client5.http.ConnectTimeoutException;
 import org.apache.hc.client5.http.classic.ExecRuntime;
@@ -88,8 +82,6 @@ final class ApacheHttpClientBlockingChannel implements BlockingChannel {
     private final Optional<InetAddress> resolvedHost;
     private final ResponseLeakDetector responseLeakDetector;
     private final OptionalInt uriIndexForInstrumentation;
-    private final BooleanSupplier isValidTarget;
-    private final Meter shortCircuitMeter;
 
     ApacheHttpClientBlockingChannel(
             ApacheHttpClientChannels.CloseableClient client,
@@ -102,37 +94,10 @@ final class ApacheHttpClientBlockingChannel implements BlockingChannel {
         this.resolvedHost = resolvedHost;
         this.responseLeakDetector = responseLeakDetector;
         this.uriIndexForInstrumentation = uriIndexForInstrumentation;
-        this.shortCircuitMeter = DialogueClientMetrics.of(
-                        client.clientConfiguration().taggedMetricRegistry())
-                .shortCircuitUnresolvableTarget(client.name());
-        if (resolvedHost.isPresent() && client.dnsResolver().isPresent()) {
-            InetAddress target = resolvedHost.get();
-            DialogueDnsResolver dnsResolver = client.dnsResolver().get();
-            BooleanSupplier validTargetPredicate = Suppliers.memoizeWithExpiration(
-                    () -> isTargetValid(dnsResolver, baseUrl, target), Duration.ofSeconds(1))::get;
-            // If the bound address is not initially resolvable, this predicate will be ignored.
-            isValidTarget = validTargetPredicate.getAsBoolean() ? validTargetPredicate : () -> true;
-        } else {
-            isValidTarget = () -> true;
-        }
-    }
-
-    private static boolean isTargetValid(DialogueDnsResolver dnsResolver, URL baseUrl, InetAddress resolvedHost) {
-        String host = baseUrl.getHost();
-        if (Strings.isNullOrEmpty(host)) {
-            // we don't have enough confidence to prevent requests from flowing.
-            return true;
-        }
-        return dnsResolver.resolve(host).contains(resolvedHost);
     }
 
     @Override
     public Response execute(Endpoint endpoint, Request request) throws IOException {
-        if (!isValidTarget.getAsBoolean()) {
-            shortCircuitMeter.mark();
-            throw new SafeConnectException("Target address can no longer be resolved");
-        }
-
         // Create base request given the URL
         URL target = baseUrl.render(endpoint, request);
         ClassicRequestBuilder builder =

diff --git a/...e-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java b/...e-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java
@@ -182,8 +182,7 @@ static CloseableClient wrap(
                 InstrumentedPoolingHttpClientConnectionManager pool,
                 ScheduledFuture<?> connectionEvictorFuture,
                 ClientConfiguration clientConfiguration,
-                @Nullable ExecutorService executor,
-                Optional<DialogueDnsResolver> dnsResolver) {
+                @Nullable ExecutorService executor) {
             ResponseLeakDetector leakDetector =
                     ResponseLeakDetector.of(clientName, clientConfiguration.taggedMetricRegistry());
             CloseableClientImpl newInstance = new CloseableClientImpl(
@@ -193,8 +192,7 @@ static CloseableClient wrap(
                     connectionEvictorFuture,
                     leakDetector,
                     executor,
-                    clientConfiguration,
-                    dnsResolver);
+                    clientConfiguration);
             if (log.isDebugEnabled()) {
                 // If debug is enabled, log the stack trace.
                 log.debug(
@@ -228,8 +226,6 @@ static CloseableClient wrap(
         abstract ExecutorService executor();
 
         abstract ResponseLeakDetector leakDetector();
-
-        abstract Optional<DialogueDnsResolver> dnsResolver();
     }
 
     private static final class CloseableClientWrapper extends CloseableClient {
@@ -266,11 +262,6 @@ ResponseLeakDetector leakDetector() {
             return delegate.leakDetector();
         }
 
-        @Override
-        Optional<DialogueDnsResolver> dnsResolver() {
-            return delegate.dnsResolver();
-        }
-
         @Override
         public void close() throws IOException {
             delegate.close();
@@ -290,7 +281,6 @@ private static final class CloseableClientImpl extends CloseableClient {
         private final InstrumentedPoolingHttpClientConnectionManager pool;
         private final ResponseLeakDetector leakDetector;
         private final ClientConfiguration clientConfiguration;
-        private final Optional<DialogueDnsResolver> dnsResolver;
 
         @Nullable
         private final ExecutorService executor;
@@ -304,15 +294,13 @@ private CloseableClientImpl(
                 ScheduledFuture<?> connectionEvictorFuture,
                 ResponseLeakDetector leakDetector,
                 @Nullable ExecutorService executor,
-                ClientConfiguration clientConfiguration,
-                Optional<DialogueDnsResolver> dnsResolver) {
+                ClientConfiguration clientConfiguration) {
             this.clientName = clientName;
             this.apacheClient = apacheClient;
             this.pool = pool;
             this.leakDetector = leakDetector;
             this.executor = executor;
             this.clientConfiguration = clientConfiguration;
-            this.dnsResolver = dnsResolver;
             closer.register(() -> connectionEvictorFuture.cancel(true));
             closer.register(apacheClient);
             closer.register(pool::closeUnderlyingConnectionManager);
@@ -346,11 +334,6 @@ ResponseLeakDetector leakDetector() {
             return leakDetector;
         }
 
-        @Override
-        Optional<DialogueDnsResolver> dnsResolver() {
-            return dnsResolver;
-        }
-
         @Override
         public void close() throws IOException {
             if (log.isDebugEnabled()) {
@@ -603,8 +586,7 @@ public CloseableClient build() {
             CloseableHttpClient apacheClient = builder.build();
             ScheduledFuture<?> connectionEvictorFuture =
                     ScheduledIdleConnectionEvictor.schedule(connectionManager, Duration.ofSeconds(5));
-            return CloseableClient.wrap(
-                    apacheClient, name, connectionManager, connectionEvictorFuture, conf, executor, dnsResolver);
+            return CloseableClient.wrap(apacheClient, name, connectionManager, connectionEvictorFuture, conf, executor);
         }
     }
 

diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/SafeConnectException.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/SafeConnectException.java
diff --git a/dialogue-apache-hc5-client/src/main/metrics/dialogue-apache-client-metrics.yml b/dialogue-apache-hc5-client/src/main/metrics/dialogue-apache-client-metrics.yml
@@ -121,11 +121,3 @@ namespaces:
           - name: cipher
             docs: The insecure cipher used to connect to this server
         docs: Meter describing the use of insecure ciphers to connect to this server.
-
-      short.circuit.unresolvable.target:
-        type: meter
-        tags:
-          - name: client-name
-          - name: client-type
-            values: [ apache-hc5 ]
-        docs: Rate that requests are immediately rejected due to an unresolvable target address.
diff --git a/dialogue-clients/metrics.md b/dialogue-clients/metrics.md
@@ -48,9 +48,6 @@ Dialogue client response metrics provided by the Apache client channel.
   - `client-name`
   - `client-type` values (`apache-hc5`)
   - `cipher`: The insecure cipher used to connect to this server
-- `dialogue.client.short.circuit.unresolvable.target` (meter): Rate that requests are immediately rejected due to an unresolvable target address.
-  - `client-name`
-  - `client-type` values (`apache-hc5`)
 
 ### dialogue.client.pool
 Connection pool metrics from the dialogue Apache client.