RetryOtherValidatingChannel handles mesh URIs (#2276)

RetryOtherValidatingChannel handles mesh URIs
palantir · May 8, 2024 · fc3896c · fc3896c
1 parent 943f0fa
commit fc3896c
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 13 deletions.
diff --git a/changelog/@unreleased/pr-2276.v2.yml b/changelog/@unreleased/pr-2276.v2.yml
@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: RetryOtherValidatingChannel handles mesh URIs without logging warnings
+  links:
+  - https://github.com/palantir/dialogue/pull/2276
diff --git a/...ents/src/test/java/com/palantir/dialogue/otherpackage/DialogueClientsIntegrationTest.java b/...ents/src/test/java/com/palantir/dialogue/otherpackage/DialogueClientsIntegrationTest.java
@@ -341,6 +341,20 @@ void test_unlimited_timeouts() {
                 .doesNotThrowAnyException();
     }
 
+    @Test
+    void test_mesh_uri() {
+        undertowHandler = exchange -> exchange.setStatusCode(204);
+        SampleServiceBlocking client = DialogueClients.create(Refreshable.only(null))
+                .withUserAgent(TestConfigurations.AGENT)
+                .getNonReloading(
+                        SampleServiceBlocking.class,
+                        ServiceConfiguration.builder()
+                                .addUris("mesh-" + getUri(undertow))
+                                .security(TestConfigurations.SSL_CONFIG)
+                                .build());
+        assertThatCode(client::voidToVoid).as("request should succeed").doesNotThrowAnyException();
+    }
+
     @Test
     public void test_sticky_is_sticky() {
         testSticky(ReloadingFactory::getStickyChannels, StickyChannelFactory::getCurrentBest);

diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/Config.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/Config.java
@@ -17,6 +17,7 @@
 package com.palantir.dialogue.core;
 
 import com.github.benmanes.caffeine.cache.Ticker;
+import com.google.common.collect.ImmutableList;
 import com.palantir.conjure.java.client.config.ClientConfiguration;
 import com.palantir.conjure.java.client.config.ClientConfiguration.ClientQoS;
 import com.palantir.logsafe.DoNotLog;
@@ -41,21 +42,32 @@ interface Config {
 
     DialogueChannelFactory channelFactory();
 
-    ClientConfiguration clientConf();
+    ClientConfiguration rawConfig();
+
+    @Value.Derived
+    default ClientConfiguration clientConf() {
+        return ClientConfiguration.builder()
+                .from(rawConfig())
+                .uris(rawConfig().uris().stream()
+                        .map(MeshMode::stripMeshPrefix)
+                        .collect(ImmutableList.toImmutableList()))
+                .build();
+    }
 
     @Value.Default
     default Refreshable<List<TargetUri>> uris() {
-        return Refreshable.only(clientConf().uris().stream().map(TargetUri::of).collect(Collectors.toList()));
+        return Refreshable.only(
+                rawConfig().uris().stream().map(TargetUri::of).collect(Collectors.toUnmodifiableList()));
     }
 
     @Value.Derived
     default MeshMode mesh() {
-        return MeshMode.fromUris(clientConf().uris(), SafeArg.of("channelName", channelName()));
+        return MeshMode.fromUris(rawConfig().uris(), SafeArg.of("channelName", channelName()));
     }
 
     @Value.Derived
     default boolean isConcurrencyLimitingEnabled() {
-        return clientConf().clientQoS() == ClientQoS.ENABLED && mesh() != MeshMode.USE_EXTERNAL_MESH;
+        return rawConfig().clientQoS() == ClientQoS.ENABLED && mesh() != MeshMode.USE_EXTERNAL_MESH;
     }
 
     @Value.Default
@@ -83,15 +95,15 @@ default int maxQueueSize() {
     @Value.Check
     default void check() {
         Preconditions.checkArgument(maxQueueSize() > 0, "maxQueueSize must be positive");
-        Preconditions.checkArgument(clientConf().userAgent().isPresent(), "userAgent must be specified");
+        Preconditions.checkArgument(rawConfig().userAgent().isPresent(), "userAgent must be specified");
         Preconditions.checkArgument(
-                clientConf().retryOnSocketException() == ClientConfiguration.RetryOnSocketException.ENABLED,
+                rawConfig().retryOnSocketException() == ClientConfiguration.RetryOnSocketException.ENABLED,
                 "Retries on socket exceptions cannot be disabled without disabling retries entirely.");
 
         if (uris().get().size() > 1 && overrideSingleHostIndex().isPresent()) {
             throw new SafeIllegalArgumentException(
                     "overrideHostIndex is only permitted when there is a single uri",
-                    SafeArg.of("numUris", clientConf().uris().size()));
+                    SafeArg.of("numUris", rawConfig().uris().size()));
         }
     }
 }
diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java
@@ -93,7 +93,7 @@ public Builder channelName(@Safe String channelName) {
         }
 
         public Builder clientConfiguration(ClientConfiguration value) {
-            builder.clientConf(value);
+            builder.rawConfig(value);
             return this;
         }
 

diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/RetryOtherValidatingChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/RetryOtherValidatingChannel.java
@@ -112,10 +112,14 @@ static String strictParseHost(String uri) {
         throw new SafeIllegalArgumentException("Failed to parse URI", UnsafeArg.of("uri", uri));
     }
 
+    @VisibleForTesting
     @CheckForNull
-    private static String maybeParseHost(String uri) {
+    static String maybeParseHost(String uri) {
+        // n.b. URL cannot handle mesh-http uris. Ideally we'd use the URI type here,
+        // however there are edge cases where it may fail that URL may succeed.
+        String normalized = MeshMode.stripMeshPrefix(uri);
         try {
-            URL parsed = new URL(uri);
+            URL parsed = new URL(normalized);
             return parsed.getHost();
         } catch (MalformedURLException e) {
             return null;

diff --git a/dialogue-core/src/test/java/com/palantir/dialogue/core/HostMetricsChannelTest.java b/dialogue-core/src/test/java/com/palantir/dialogue/core/HostMetricsChannelTest.java
@@ -132,7 +132,7 @@ private ImmutableConfig config(ClientConfiguration rawConfig) {
         return ImmutableConfig.builder()
                 .channelName("channelName")
                 .channelFactory(factory)
-                .clientConf(rawConfig)
+                .rawConfig(rawConfig)
                 .ticker(ticker)
                 .build();
     }

diff --git a/dialogue-core/src/test/java/com/palantir/dialogue/core/RequestSizeMetricsChannelTest.java b/dialogue-core/src/test/java/com/palantir/dialogue/core/RequestSizeMetricsChannelTest.java
@@ -84,7 +84,7 @@ public void close() {}
                 ImmutableConfig.builder()
                         .channelName("channelName")
                         .channelFactory(STUB_FACTORY)
-                        .clientConf(ClientConfiguration.builder()
+                        .rawConfig(ClientConfiguration.builder()
                                 .from(TestConfigurations.create("https://foo:10001"))
                                 .taggedMetricRegistry(registry)
                                 .build())
@@ -149,7 +149,7 @@ public void close() {}
                 ImmutableConfig.builder()
                         .channelName("smallRequestChannelName")
                         .channelFactory(STUB_FACTORY)
-                        .clientConf(ClientConfiguration.builder()
+                        .rawConfig(ClientConfiguration.builder()
                                 .from(TestConfigurations.create("https://foo:10001"))
                                 .taggedMetricRegistry(registry)
                                 .build())

diff --git a/dialogue-core/src/test/java/com/palantir/dialogue/core/RetryOtherValidatingChannelTest.java b/dialogue-core/src/test/java/com/palantir/dialogue/core/RetryOtherValidatingChannelTest.java
@@ -100,4 +100,22 @@ private void execute(@Nullable String retryOtherUri) {
                 .succeedsWithin(Duration.ZERO)
                 .isEqualTo(response);
     }
+
+    @Test
+    void parsesMeshUris() {
+        assertThat(RetryOtherValidatingChannel.maybeParseHost("mesh-http://localhost:1234/api"))
+                .isEqualTo("localhost");
+    }
+
+    @Test
+    void parsesStandardUris() {
+        assertThat(RetryOtherValidatingChannel.maybeParseHost("https://host.palantir.com:1234/api"))
+                .isEqualTo("host.palantir.com");
+    }
+
+    @Test
+    void parsesStandardUrisWithoutPort() {
+        assertThat(RetryOtherValidatingChannel.maybeParseHost("https://host.palantir.com/api"))
+                .isEqualTo("host.palantir.com");
+    }
 }