core: Use io.grpc.Uri to parse the target, if flagged

Author: jdcormie

core/src/main/java/io/grpc/internal/ManagedChannelImplBuilder.java

@@ -47,6 +47,7 @@
 import io.grpc.NameResolverRegistry;
 import io.grpc.ProxyDetector;
 import io.grpc.StatusOr;
+import io.grpc.Uri;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.SocketAddress;
@@ -105,6 +106,16 @@ public static ManagedChannelBuilder<?> forTarget(String target) {
    */
   static final long IDLE_MODE_MIN_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(1);
 
+  private static boolean enableRfc3986Uris = GrpcUtil.getFlag("GRPC_ENABLE_RFC3986_URIS", false);
+
+  /** Whether to parse targets as RFC 3986 URIs (true), or use {@link java.net.URI} (false). */
+  @VisibleForTesting
+  static boolean setRfc3986UrisEnabled(boolean value) {
+    boolean prevValue = ManagedChannelImplBuilder.enableRfc3986Uris;
+    ManagedChannelImplBuilder.enableRfc3986Uris = value;
+    return prevValue;
+  }
+
   private static final ObjectPool<? extends Executor> DEFAULT_EXECUTOR_POOL =
       SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR);
 
@@ -719,8 +730,11 @@ protected ManagedChannelImplBuilder addMetricSink(MetricSink metricSink) {
   public ManagedChannel build() {
     ClientTransportFactory clientTransportFactory =
         clientTransportFactoryBuilder.buildClientTransportFactory();
-    ResolvedNameResolver resolvedResolver = getNameResolverProvider(
-        target, nameResolverRegistry, clientTransportFactory.getSupportedSocketAddressTypes());
+    ResolvedNameResolver resolvedResolver =
+        enableRfc3986Uris
+            ? getNameResolverProviderRfc3986(target, nameResolverRegistry)
+            : getNameResolverProvider(target, nameResolverRegistry);
+    resolvedResolver.checkAddressTypes(clientTransportFactory.getSupportedSocketAddressTypes());
     return new ManagedChannelOrphanWrapper(new ManagedChannelImpl(
         this,
         clientTransportFactory,
@@ -822,12 +836,25 @@ public ResolvedNameResolver(UriWrapper targetUri, NameResolverProvider provider)
       this.targetUri = checkNotNull(targetUri, "targetUri");
       this.provider = checkNotNull(provider, "provider");
     }
+
+    void checkAddressTypes(
+        Collection<Class<? extends SocketAddress>> channelTransportSocketAddressTypes) {
+      if (channelTransportSocketAddressTypes != null) {
+        Collection<Class<? extends SocketAddress>> nameResolverSocketAddressTypes =
+            provider.getProducedSocketAddressTypes();
+        if (!channelTransportSocketAddressTypes.containsAll(nameResolverSocketAddressTypes)) {
+          throw new IllegalArgumentException(
+              String.format(
+                  "Address types of NameResolver '%s' for '%s' not supported by transport",
+                  provider.getDefaultScheme(), targetUri));
+        }
+      }
+    }
   }
 
   @VisibleForTesting
   static ResolvedNameResolver getNameResolverProvider(
-      String target, NameResolverRegistry nameResolverRegistry,
-      Collection<Class<? extends SocketAddress>> channelTransportSocketAddressTypes) {
+      String target, NameResolverRegistry nameResolverRegistry) {
     // Finding a NameResolver. Try using the target string as the URI. If that fails, try prepending
     // "dns:///".
     NameResolverProvider provider = null;
@@ -863,14 +890,46 @@ static ResolvedNameResolver getNameResolverProvider(
           target, uriSyntaxErrors.length() > 0 ? " (" + uriSyntaxErrors + ")" : ""));
     }
 
-    if (channelTransportSocketAddressTypes != null) {
-      Collection<Class<? extends SocketAddress>> nameResolverSocketAddressTypes
-          = provider.getProducedSocketAddressTypes();
-      if (!channelTransportSocketAddressTypes.containsAll(nameResolverSocketAddressTypes)) {
-        throw new IllegalArgumentException(String.format(
-            "Address types of NameResolver '%s' for '%s' not supported by transport",
-            targetUri.getScheme(), target));
-      }
+    return new ResolvedNameResolver(wrap(targetUri), provider);
+  }
+
+  @VisibleForTesting
+  static ResolvedNameResolver getNameResolverProviderRfc3986(
+      String target, NameResolverRegistry nameResolverRegistry) {
+    // Finding a NameResolver. Try using the target string as the URI. If that fails, try prepending
+    // "dns:///".
+    NameResolverProvider provider = null;
+    Uri targetUri = null;
+    StringBuilder uriSyntaxErrors = new StringBuilder();
+    try {
+      targetUri = Uri.parse(target);
+    } catch (URISyntaxException e) {
+      // Can happen with ip addresses like "[::1]:1234" or 127.0.0.1:1234.
+      uriSyntaxErrors.append(e.getMessage());
+    }
+    if (targetUri != null) {
+      // For "localhost:8080" this would likely cause provider to be null, because "localhost" is
+      // parsed as the scheme. Will hit the next case and try "dns:///localhost:8080".
+      provider = nameResolverRegistry.getProviderForScheme(targetUri.getScheme());
+    }
+
+    if (provider == null && !URI_PATTERN.matcher(target).matches()) {
+      // It doesn't look like a URI target. Maybe it's an authority string. Try with the default
+      // scheme from the registry.
+      targetUri =
+          Uri.newBuilder()
+              .setScheme(nameResolverRegistry.getDefaultScheme())
+              .setHost("")
+              .setPath("/" + target)
+              .build();
+      provider = nameResolverRegistry.getProviderForScheme(targetUri.getScheme());
+    }
+
+    if (provider == null) {
+      throw new IllegalArgumentException(
+          String.format(
+              "Could not find a NameResolverProvider for %s%s",
+              target, uriSyntaxErrors.length() > 0 ? " (" + uriSyntaxErrors + ")" : ""));
     }
 
     return new ResolvedNameResolver(wrap(targetUri), provider);

core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java

@@ -69,13 +69,15 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
 
 /** Unit tests for {@link ManagedChannelImplBuilder}. */
-@RunWith(JUnit4.class)
+@RunWith(Parameterized.class)
 public class ManagedChannelImplBuilderTest {
   private static final int DUMMY_PORT = 42;
   private static final String DUMMY_TARGET = "fake-target";
@@ -98,8 +100,16 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
         }
       };
 
+  @Parameters(name = "enableRfc3986UrisParam={0}")
+  public static Iterable<Object[]> data() {
+    return Arrays.asList(new Object[][] {{true}, {false}});
+  }
+
+  @Parameter public boolean enableRfc3986UrisParam;
+
   @Rule public final MockitoRule mocks = MockitoJUnit.rule();
   @Rule public final GrpcCleanupRule grpcCleanupRule = new GrpcCleanupRule();
+  @Rule public final FlagResetRule flagResetRule = new FlagResetRule();
 
   @Mock private ClientTransportFactory mockClientTransportFactory;
   @Mock private ClientTransportFactoryBuilder mockClientTransportFactoryBuilder;
@@ -117,6 +127,9 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
 
   @Before
   public void setUp() throws Exception {
+    flagResetRule.setFlagForTest(
+        ManagedChannelImplBuilder::setRfc3986UrisEnabled, enableRfc3986UrisParam);
+
     builder = new ManagedChannelImplBuilder(
         DUMMY_TARGET,
         new UnsupportedClientTransportFactoryBuilder(),
@@ -373,8 +386,11 @@ public void transportDoesNotSupportAddressTypes() {
       ManagedChannel unused = grpcCleanupRule.register(builder.build());
       fail("Should fail");
     } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo(
-          "Address types of NameResolver 'dns' for 'valid:1234' not supported by transport");
+      assertThat(e)
+          .hasMessageThat()
+          .isEqualTo(
+              "Address types of NameResolver 'dns' for 'dns:///valid:1234' not supported by"
+                  + " transport");
     }
   }