diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index ec3e417e53a..7153393bdfc 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -67,7 +67,6 @@ import io.grpc.xds.client.XdsInitializationException; import io.grpc.xds.client.XdsLogger; import io.grpc.xds.client.XdsLogger.XdsLogLevel; -import java.net.URI; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -109,7 +108,6 @@ final class XdsNameResolver extends NameResolver { private final XdsLogger logger; @Nullable private final String targetAuthority; - private final String target; private final String serviceAuthority; // Encoded version of the service authority as per // https://datatracker.ietf.org/doc/html/rfc3986#section-3.2. @@ -140,12 +138,12 @@ final class XdsNameResolver extends NameResolver { private ResolveState resolveState; XdsNameResolver( - URI targetUri, String name, @Nullable String overrideAuthority, - ServiceConfigParser serviceConfigParser, + String target, @Nullable String targetAuthority, String name, + @Nullable String overrideAuthority, ServiceConfigParser serviceConfigParser, SynchronizationContext syncContext, ScheduledExecutorService scheduler, @Nullable Map bootstrapOverride, MetricRecorder metricRecorder, Args nameResolverArgs) { - this(targetUri, targetUri.getAuthority(), name, overrideAuthority, serviceConfigParser, + this(target, targetAuthority, name, overrideAuthority, serviceConfigParser, syncContext, scheduler, bootstrapOverride == null ? SharedXdsClientPoolProvider.getDefaultProvider() @@ -156,14 +154,13 @@ final class XdsNameResolver extends NameResolver { @VisibleForTesting XdsNameResolver( - URI targetUri, @Nullable String targetAuthority, String name, + String target, @Nullable String targetAuthority, String name, @Nullable String overrideAuthority, ServiceConfigParser serviceConfigParser, SynchronizationContext syncContext, ScheduledExecutorService scheduler, XdsClientPoolFactory xdsClientPoolFactory, ThreadSafeRandom random, FilterRegistry filterRegistry, @Nullable Map bootstrapOverride, MetricRecorder metricRecorder, Args nameResolverArgs) { this.targetAuthority = targetAuthority; - target = targetUri.toString(); // The name might have multiple slashes so encode it before verifying. serviceAuthority = checkNotNull(name, "name"); diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolverProvider.java b/xds/src/main/java/io/grpc/xds/XdsNameResolverProvider.java index e3462276b17..89e51694a69 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolverProvider.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolverProvider.java @@ -22,6 +22,7 @@ import io.grpc.Internal; import io.grpc.NameResolver.Args; import io.grpc.NameResolverProvider; +import io.grpc.Uri; import io.grpc.xds.client.XdsClient; import java.net.InetSocketAddress; import java.net.SocketAddress; @@ -86,16 +87,39 @@ public XdsNameResolver newNameResolver(URI targetUri, Args args) { targetPath, targetUri); String name = targetPath.substring(1); - return new XdsNameResolver( - targetUri, name, args.getOverrideAuthority(), - args.getServiceConfigParser(), args.getSynchronizationContext(), - args.getScheduledExecutorService(), - bootstrapOverride, - args.getMetricRecorder(), args); + return newNameResolver(targetUri.toString(), targetUri.getAuthority(), name, args); } return null; } + @Override + public XdsNameResolver newNameResolver(Uri targetUri, Args args) { + if (scheme.equals(targetUri.getScheme())) { + Preconditions.checkArgument( + targetUri.isPathAbsolute(), + "the path component of the target (%s) must start with '/'", + targetUri); + return newNameResolver( + targetUri.toString(), targetUri.getAuthority(), targetUri.getPath().substring(1), args); + } + return null; + } + + private XdsNameResolver newNameResolver( + String targetUri, String targetAuthority, String name, Args args) { + return new XdsNameResolver( + targetUri.toString(), + targetAuthority, + name, + args.getOverrideAuthority(), + args.getServiceConfigParser(), + args.getSynchronizationContext(), + args.getScheduledExecutorService(), + bootstrapOverride, + args.getMetricRecorder(), + args); + } + @Override public String getDefaultScheme() { return scheme; diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java index 33aae268c60..8998a2bae99 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverProviderTest.java @@ -29,18 +29,22 @@ import io.grpc.NameResolverProvider; import io.grpc.NameResolverRegistry; import io.grpc.SynchronizationContext; +import io.grpc.Uri; import io.grpc.internal.FakeClock; import io.grpc.internal.GrpcUtil; import java.net.URI; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Map; 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; /** Unit tests for {@link XdsNameResolverProvider}. */ -@RunWith(JUnit4.class) +@RunWith(Parameterized.class) public class XdsNameResolverProviderTest { private final SynchronizationContext syncContext = new SynchronizationContext( new Thread.UncaughtExceptionHandler() { @@ -63,6 +67,13 @@ public void uncaughtException(Thread t, Throwable e) { private XdsNameResolverProvider provider = new XdsNameResolverProvider(); + @Parameters(name = "enableRfc3986UrisParam={0}") + public static Iterable data() { + return Arrays.asList(new Object[][] {{true}, {false}}); + } + + @Parameter public boolean enableRfc3986UrisParam; + @Test public void provided() { for (NameResolverProvider current @@ -81,48 +92,46 @@ public void isAvailable() { } @Test - public void newNameResolver() { - assertThat( - provider.newNameResolver(URI.create("xds://1.1.1.1/foo.googleapis.com"), args)) + public void newNameResolver_returnsExpectedType() { + assertThat(newNameResolver(provider, "xds://1.1.1.1/foo.googleapis.com", args)) .isInstanceOf(XdsNameResolver.class); - assertThat( - provider.newNameResolver(URI.create("xds:///foo.googleapis.com"), args)) + assertThat(newNameResolver(provider, "xds:///foo.googleapis.com", args)) .isInstanceOf(XdsNameResolver.class); - assertThat( - provider.newNameResolver(URI.create("notxds://1.1.1.1/foo.googleapis.com"), - args)) - .isNull(); + } + + @Test + public void newNameResolver_matchesExpectedScheme() { + assertThat(newNameResolver(provider, "notxds://1.1.1.1/foo.googleapis.com", args)).isNull(); } @Test public void validName_withAuthority() { - XdsNameResolver resolver = - provider.newNameResolver( - URI.create("xds://trafficdirector.google.com/foo.googleapis.com"), args); + NameResolver resolver = + newNameResolver(provider, "xds://trafficdirector.google.com/foo.googleapis.com", args); assertThat(resolver).isNotNull(); assertThat(resolver.getServiceAuthority()).isEqualTo("foo.googleapis.com"); } @Test public void validName_noAuthority() { - XdsNameResolver resolver = - provider.newNameResolver(URI.create("xds:///foo.googleapis.com"), args); + NameResolver resolver = newNameResolver(provider, "xds:///foo.googleapis.com", args); assertThat(resolver).isNotNull(); assertThat(resolver.getServiceAuthority()).isEqualTo("foo.googleapis.com"); } @Test public void validName_urlExtractedAuthorityInvalidWithoutEncoding() { - XdsNameResolver resolver = - provider.newNameResolver(URI.create("xds:///1234/path/foo.googleapis.com:8080"), args); + NameResolver resolver = + newNameResolver(provider, "xds:///1234/path/foo.googleapis.com:8080", args); assertThat(resolver).isNotNull(); assertThat(resolver.getServiceAuthority()).isEqualTo("1234%2Fpath%2Ffoo.googleapis.com:8080"); } @Test public void validName_urlwithTargetAuthorityAndExtractedAuthorityInvalidWithoutEncoding() { - XdsNameResolver resolver = provider.newNameResolver(URI.create( - "xds://trafficdirector.google.com/1234/path/foo.googleapis.com:8080"), args); + NameResolver resolver = + newNameResolver( + provider, "xds://trafficdirector.google.com/1234/path/foo.googleapis.com:8080", args); assertThat(resolver).isNotNull(); assertThat(resolver.getServiceAuthority()).isEqualTo("1234%2Fpath%2Ffoo.googleapis.com:8080"); } @@ -135,18 +144,14 @@ public void newProvider_multipleScheme() { XdsNameResolverProvider provider1 = XdsNameResolverProvider.createForTest("new-xds-scheme", new HashMap()); registry.register(provider1); - assertThat(registry.asFactory() - .newNameResolver(URI.create("xds:///localhost"), args)).isNotNull(); - assertThat(registry.asFactory() - .newNameResolver(URI.create("new-xds-scheme:///localhost"), args)).isNotNull(); - assertThat(registry.asFactory() - .newNameResolver(URI.create("no-scheme:///localhost"), args)).isNotNull(); + assertThat(newNameResolver(registry.asFactory(), "xds:///localhost", args)).isNotNull(); + assertThat(newNameResolver(registry.asFactory(), "new-xds-scheme:///localhost", args)) + .isNotNull(); + assertThat(newNameResolver(registry.asFactory(), "no-scheme:///localhost", args)).isNotNull(); registry.deregister(provider1); - assertThat(registry.asFactory() - .newNameResolver(URI.create("new-xds-scheme:///localhost"), args)).isNull(); + assertThat(newNameResolver(registry.asFactory(), "new-xds-scheme:///localhost", args)).isNull(); registry.deregister(provider0); - assertThat(registry.asFactory() - .newNameResolver(URI.create("xds:///localhost"), args)).isNotNull(); + assertThat(newNameResolver(registry.asFactory(), "xds:///localhost", args)).isNotNull(); } @Test @@ -176,4 +181,11 @@ public void newProvider_overrideBootstrap() { resolver.shutdown(); registry.deregister(provider); } + + private NameResolver newNameResolver( + NameResolver.Factory factory, String uriString, NameResolver.Args args) { + return enableRfc3986UrisParam + ? factory.newNameResolver(Uri.create(uriString), args) + : factory.newNameResolver(URI.create(uriString), args); + } } diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index 3f50d92c2b5..36d54507044 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -104,8 +104,6 @@ import io.grpc.xds.client.XdsClient; import io.grpc.xds.client.XdsResourceType; import java.io.IOException; -import java.net.URI; -import java.net.URISyntaxException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -201,7 +199,7 @@ public ConfigOrError parseServiceConfig(Map rawServiceConfig) { private XdsNameResolver resolver; private TestCall testCall; private boolean originalEnableTimeout; - private URI targetUri; + private String targetUri = AUTHORITY; private final NameResolver.Args nameResolverArgs = NameResolver.Args.newBuilder() .setDefaultPort(8080) .setProxyDetector(GrpcUtil.DEFAULT_PROXY_DETECTOR) @@ -216,12 +214,6 @@ public ConfigOrError parseServiceConfig(Map rawServiceConfig) { public void setUp() { lenient().doReturn(Status.OK).when(mockListener).onResult2(any()); - try { - targetUri = new URI(AUTHORITY); - } catch (URISyntaxException e) { - targetUri = null; - } - originalEnableTimeout = XdsNameResolver.enableTimeout; XdsNameResolver.enableTimeout = true;