netty: Add RFC 3986 support to the 'unix:' name resolver.

Author: jdcormie

netty/src/main/java/io/grpc/netty/UdsNameResolver.java

@@ -18,6 +18,7 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Strings.isNullOrEmpty;
 
 import com.google.common.base.Preconditions;
 import io.grpc.EquivalentAddressGroup;
@@ -31,8 +32,18 @@ final class UdsNameResolver extends NameResolver {
   private NameResolver.Listener2 listener;
   private final String authority;
 
+  /**
+   * Constructs a new instance of UdsNameResolver.
+   *
+   * @param authority authority of the 'unix:' URI to resolve, or null if target has no authority
+   * @param targetPath path of the 'unix:' URI to resolve
+   */
   UdsNameResolver(String authority, String targetPath, Args args) {
-    checkArgument(authority == null, "non-null authority not supported");
+    // UDS is inherently local. According to https://github.com/grpc/grpc/blob/master/doc/naming.md,
+    // this is expressed in the target URI either by using a blank authority, like "unix:///sock",
+    // or by omitting authority completely, e.g. "unix:/sock".
+    // TODO(jdcormie): Allow the explicit authority string "localhost"?
+    checkArgument(isNullOrEmpty(authority), "authority not supported: %s", authority);
     this.authority = targetPath;
   }
 

netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java

@@ -20,6 +20,7 @@
 import io.grpc.Internal;
 import io.grpc.NameResolver;
 import io.grpc.NameResolverProvider;
+import io.grpc.Uri;
 import io.netty.channel.unix.DomainSocketAddress;
 import java.net.SocketAddress;
 import java.net.URI;
@@ -31,9 +32,21 @@ public final class UdsNameResolverProvider extends NameResolverProvider {
 
   private static final String SCHEME = "unix";
 
+  @Override
+  public NameResolver newNameResolver(Uri targetUri, NameResolver.Args args) {
+    if (SCHEME.equals(targetUri.getScheme())) {
+      return new UdsNameResolver(targetUri.getAuthority(), targetUri.getPath(), args);
+    } else {
+      return null;
+    }
+  }
+
   @Override
   public UdsNameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
     if (SCHEME.equals(targetUri.getScheme())) {
+      // TODO(jdcormie): java.net.URI has a bug where getAuthority() returns null for both the
+      // undefined and zero-length authority. Doesn't matter for now because UdsNameResolver doesn't
+      // distinguish these cases.
       return new UdsNameResolver(targetUri.getAuthority(), getTargetPathFromUri(targetUri), args);
     } else {
       return null;
@@ -44,6 +57,10 @@ static String getTargetPathFromUri(URI targetUri) {
     Preconditions.checkArgument(SCHEME.equals(targetUri.getScheme()), "scheme must be " + SCHEME);
     String targetPath = targetUri.getPath();
     if (targetPath == null) {
+      // TODO(jdcormie): This incorrectly includes '?' and any characters that follow. In the
+      // hierarchical case ('unix:///path'), java.net.URI parses these into a query component that's
+      // distinct from the path. But in the present "opaque" case ('unix:/path'), what may look like
+      // a query is considered part of the SSP.
       targetPath = Preconditions.checkNotNull(targetUri.getSchemeSpecificPart(), "targetPath");
     }
     return targetPath;

netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java

@@ -17,6 +17,7 @@
 package io.grpc.netty;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -26,26 +27,38 @@
 import io.grpc.NameResolver;
 import io.grpc.NameResolver.ServiceConfigParser;
 import io.grpc.SynchronizationContext;
+import io.grpc.Uri;
 import io.grpc.internal.FakeClock;
 import io.grpc.internal.GrpcUtil;
 import io.netty.channel.unix.DomainSocketAddress;
 import java.net.SocketAddress;
 import java.net.URI;
+import java.util.Arrays;
 import java.util.List;
 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.ArgumentCaptor;
 import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
 
 /** Unit tests for {@link UdsNameResolverProvider}. */
-@RunWith(JUnit4.class)
+@RunWith(Parameterized.class)
 public class UdsNameResolverProviderTest {
   private static final int DEFAULT_PORT = 887;
+
+  @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();
 
@@ -73,38 +86,59 @@ public class UdsNameResolverProviderTest {
 
   @Test
   public void testUnixRelativePath() {
-    UdsNameResolver udsNameResolver =
-        udsNameResolverProvider.newNameResolver(URI.create("unix:sock.sock"), args);
+    UdsNameResolver udsNameResolver = newNameResolver("unix:sock.sock", args);
     DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
     assertThat(domainSocketAddress.path()).isEqualTo("sock.sock");
   }
 
   @Test
   public void testUnixAbsolutePath() {
-    UdsNameResolver udsNameResolver =
-        udsNameResolverProvider.newNameResolver(URI.create("unix:/sock.sock"), args);
+    UdsNameResolver udsNameResolver = newNameResolver("unix:/sock.sock", args);
     DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
     assertThat(domainSocketAddress.path()).isEqualTo("/sock.sock");
   }
 
   @Test
   public void testUnixAbsoluteAlternatePath() {
-    UdsNameResolver udsNameResolver =
-        udsNameResolverProvider.newNameResolver(URI.create("unix:///sock.sock"), args);
+    UdsNameResolver udsNameResolver = newNameResolver("unix:///sock.sock", args);
     DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
     assertThat(domainSocketAddress.path()).isEqualTo("/sock.sock");
   }
 
   @Test
   public void testUnixPathWithAuthority() {
     try {
-      udsNameResolverProvider.newNameResolver(URI.create("unix://localhost/sock.sock"), args);
+      newNameResolver("unix://localhost/sock.sock", args);
       fail("exception expected");
     } catch (IllegalArgumentException e) {
       assertThat(e).hasMessageThat().isEqualTo("authority not supported: localhost");
     }
   }
 
+  @Test
+  public void testUnixAbsolutePathDoesNotIncludeQueryOrFragment() {
+    UdsNameResolver udsNameResolver = newNameResolver("unix:///sock.sock?query#fragment", args);
+    DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
+    assertThat(domainSocketAddress.path()).isEqualTo("/sock.sock");
+  }
+
+  @Test
+  public void testUnixRelativePathDoesNotIncludeQueryOrFragment() {
+    // This test fails without RFC 3986 support because of a bug in the legacy java.net.URI-based
+    // NRP implementation.
+    assume().that(enableRfc3986UrisParam).isTrue();
+
+    UdsNameResolver udsNameResolver = newNameResolver("unix:sock.sock?query#fragment", args);
+    DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
+    assertThat(domainSocketAddress.path()).isEqualTo("sock.sock");
+  }
+
+  private UdsNameResolver newNameResolver(String uriString, NameResolver.Args args) {
+    return enableRfc3986UrisParam
+        ? (UdsNameResolver) udsNameResolverProvider.newNameResolver(Uri.create(uriString), args)
+        : udsNameResolverProvider.newNameResolver(URI.create(uriString), args);
+  }
+
   private DomainSocketAddress startAndGetUniqueResolvedAddress(UdsNameResolver udsNameResolver) {
     assertThat(udsNameResolver).isNotNull();
     udsNameResolver.start(mockListener);

netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java

@@ -91,10 +91,10 @@ public void testValidTargetPath() {
   @Test
   public void testNonNullAuthority() {
     try {
-      udsNameResolver = new UdsNameResolver("authority", "sock.sock", args);
+      udsNameResolver = new UdsNameResolver("somehost", "sock.sock", args);
       fail("exception expected");
     } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("non-null authority not supported");
+      assertThat(e).hasMessageThat().isEqualTo("authority not supported: somehost");
     }
   }
 }