core,grpclb: DnsNameResolver will use srv record by default if grpclb (#6298)

api/src/test/java/io/grpc/NameResolverRegistryTest.java

@@ -21,6 +21,7 @@ import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 
 import io.grpc.NameResolver.ServiceConfigParser;
+import io.grpc.internal.BaseDnsNameResolverProvider;
 import io.grpc.internal.DnsNameResolverProvider;
 import java.lang.Thread.UncaughtExceptionHandler;
 import java.net.URI;
@@ -152,8 +153,11 @@ public class NameResolverRegistryTest {
   @Test
   public void baseProviders() {
     List<NameResolverProvider> providers = NameResolverRegistry.getDefaultRegistry().providers();
-    assertThat(providers).hasSize(1);
+    assertThat(providers).hasSize(2);
-    assertThat(providers.get(0)).isInstanceOf(DnsNameResolverProvider.class);
+    // 2 name resolvers from grpclb and core
+    for (NameResolverProvider provider : providers) {
+      assertThat(provider).isInstanceOf(BaseDnsNameResolverProvider.class);
+    }
     assertThat(NameResolverRegistry.getDefaultRegistry().asFactory().getDefaultScheme())
         .isEqualTo("dns");
   }

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

@@ -0,0 +1,70 @@
+/*
+ * Copyright 2019 The gRPC Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.grpc.internal;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Stopwatch;
+import io.grpc.InternalServiceProviders;
+import io.grpc.NameResolver;
+import io.grpc.NameResolverProvider;
+import java.net.URI;
+
+/**
+ * Base provider of name resolvers for name agnostic consumption.
+ */
+public abstract class BaseDnsNameResolverProvider extends NameResolverProvider {
+
+  private static final String SCHEME = "dns";
+
+  @VisibleForTesting
+  public static final String ENABLE_GRPCLB_PROPERTY_NAME =
+      "io.grpc.internal.DnsNameResolverProvider.enable_grpclb";
+
+  /** Returns boolean value of system property {@link #ENABLE_GRPCLB_PROPERTY_NAME}. */
+  protected abstract boolean isSrvEnabled();
+
+  @Override
+  public DnsNameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
+    if (SCHEME.equals(targetUri.getScheme())) {
+      String targetPath = Preconditions.checkNotNull(targetUri.getPath(), "targetPath");
+      Preconditions.checkArgument(targetPath.startsWith("/"),
+          "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri);
+      String name = targetPath.substring(1);
+      return new DnsNameResolver(
+          targetUri.getAuthority(),
+          name,
+          args,
+          GrpcUtil.SHARED_CHANNEL_EXECUTOR,
+          Stopwatch.createUnstarted(),
+          InternalServiceProviders.isAndroid(getClass().getClassLoader()),
+          isSrvEnabled());
+    } else {
+      return null;
+    }
+  }
+
+  @Override
+  public String getDefaultScheme() {
+    return SCHEME;
+  }
+
+  @Override
+  protected boolean isAvailable() {
+    return true;
+  }
+}

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

@@ -92,8 +92,6 @@ final class DnsNameResolver extends NameResolver {
       System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_jndi", "true");
   private static final String JNDI_LOCALHOST_PROPERTY =
       System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_jndi_localhost", "false");
-  private static final String JNDI_SRV_PROPERTY =
-      System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_grpclb", "false");
   private static final String JNDI_TXT_PROPERTY =
       System.getProperty("io.grpc.internal.DnsNameResolverProvider.enable_service_config", "false");
 
@@ -117,8 +115,6 @@ final class DnsNameResolver extends NameResolver {
   @VisibleForTesting
   static boolean enableJndiLocalhost = Boolean.parseBoolean(JNDI_LOCALHOST_PROPERTY);
   @VisibleForTesting
-  static boolean enableSrv = Boolean.parseBoolean(JNDI_SRV_PROPERTY);
-  @VisibleForTesting
   static boolean enableTxt = Boolean.parseBoolean(JNDI_TXT_PROPERTY);
 
   private static final ResourceResolverFactory resourceResolverFactory =
@@ -152,6 +148,7 @@ final class DnsNameResolver extends NameResolver {
 
   /** True if using an executor resource that should be released after use. */
   private final boolean usingExecutorResource;
+  private final boolean enableSrv;
 
   private boolean resolving;
 
@@ -159,8 +156,14 @@ final class DnsNameResolver extends NameResolver {
   // from any thread.
   private NameResolver.Listener2 listener;
 
-  DnsNameResolver(@Nullable String nsAuthority, String name, Args args,
+  DnsNameResolver(
-      Resource<Executor> executorResource, Stopwatch stopwatch, boolean isAndroid) {
+      @Nullable String nsAuthority,
+      String name,
+      Args args,
+      Resource<Executor> executorResource,
+      Stopwatch stopwatch,
+      boolean isAndroid,
+      boolean enableSrv) {
     Preconditions.checkNotNull(args, "args");
     // TODO: if a DNS server is provided as nsAuthority, use it.
     // https://www.captechconsulting.com/blogs/accessing-the-dusty-corners-of-dns-with-java
@@ -184,6 +187,7 @@ final class DnsNameResolver extends NameResolver {
         Preconditions.checkNotNull(args.getSynchronizationContext(), "syncContext");
     this.executor = args.getBlockingExecutor();
     this.usingExecutorResource = executor == null;
+    this.enableSrv = enableSrv;
   }
 
   @Override

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

@@ -16,13 +16,6 @@
 
 package io.grpc.internal;
 
-import com.google.common.base.Preconditions;
-import com.google.common.base.Stopwatch;
-import io.grpc.InternalServiceProviders;
-import io.grpc.NameResolver;
-import io.grpc.NameResolverProvider;
-import java.net.URI;
-
 /**
  * A provider for {@link DnsNameResolver}.
  *
@@ -38,41 +31,18 @@ import java.net.URI;
  *   <li>{@code "dns:///foo.googleapis.com"} (without port)</li>
  * </ul>
  */
-public final class DnsNameResolverProvider extends NameResolverProvider {
+public final class DnsNameResolverProvider extends BaseDnsNameResolverProvider {
 
-  private static final String SCHEME = "dns";
+  private static final boolean SRV_ENABLED =
+      Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "false"));
 
   @Override
-  public DnsNameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
+  protected boolean isSrvEnabled() {
-    if (SCHEME.equals(targetUri.getScheme())) {
+    return SRV_ENABLED;
-      String targetPath = Preconditions.checkNotNull(targetUri.getPath(), "targetPath");
-      Preconditions.checkArgument(targetPath.startsWith("/"),
-          "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri);
-      String name = targetPath.substring(1);
-      return new DnsNameResolver(
-          targetUri.getAuthority(),
-          name,
-          args,
-          GrpcUtil.SHARED_CHANNEL_EXECUTOR,
-          Stopwatch.createUnstarted(),
-          InternalServiceProviders.isAndroid(getClass().getClassLoader()));
-    } else {
-      return null;
-    }
   }
 
   @Override
-  public String getDefaultScheme() {
+  public int priority() {
-    return SCHEME;
-  }
-
-  @Override
-  protected boolean isAvailable() {
-    return true;
-  }
-
-  @Override
-  protected int priority() {
     return 5;
   }
 }

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

@@ -16,9 +16,12 @@
 
 package io.grpc.internal;
 
+import static com.google.common.truth.Truth.assertThat;
+import static io.grpc.internal.BaseDnsNameResolverProvider.ENABLE_GRPCLB_PROPERTY_NAME;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assume.assumeTrue;
 import static org.mockito.Mockito.mock;
 
 import io.grpc.NameResolver;
@@ -60,4 +63,11 @@ public class DnsNameResolverProviderTest {
     assertNull(
         provider.newNameResolver(URI.create("notdns:///localhost:443"), args));
   }
+
+  @Test
+  public void isSrvEnabled_falseByDefault() {
+    assumeTrue(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME) == null);
+
+    assertThat(provider.isSrvEnabled()).isFalse();
+  }
 }

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

@@ -182,7 +182,8 @@ public class DnsNameResolverTest {
   private DnsNameResolver newResolver(
       String name, Stopwatch stopwatch, boolean isAndroid, NameResolver.Args args) {
     DnsNameResolver dnsResolver =
-        new DnsNameResolver(null, name, args, fakeExecutorResource, stopwatch, isAndroid);
+        new DnsNameResolver(
+            null, name, args, fakeExecutorResource, stopwatch, isAndroid, /* enableSrv= */ false);
     // By default, using the mocked ResourceResolver to avoid I/O
     dnsResolver.setResourceResolver(new JndiResourceResolver(recordFetcher));
     return dnsResolver;

grpclb/src/main/java/io/grpc/grpclb/SecretGrpclbNameResolverProvider.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright 2019 The gRPC Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.grpc.grpclb;
+
+import io.grpc.internal.BaseDnsNameResolverProvider;
+
+/**
+ * A provider for {@code io.grpc.internal.DnsNameResolver} for gRPC lb.
+ *
+ * <p>It resolves a target URI whose scheme is {@code "dns"}. The (optional) authority of the target
+ * URI is reserved for the address of alternative DNS server (not implemented yet). The path of the
+ * target URI, excluding the leading slash {@code '/'}, is treated as the host name and the optional
+ * port to be resolved by DNS. Example target URIs:
+ *
+ * <ul>
+ *   <li>{@code "dns:///foo.googleapis.com:8080"} (using default DNS)</li>
+ *   <li>{@code "dns://8.8.8.8/foo.googleapis.com:8080"} (using alternative DNS (not implemented
+ *   yet))</li>
+ *   <li>{@code "dns:///foo.googleapis.com"} (without port)</li>
+ * </ul>
+ *
+ * <p>Note: the main difference between {@code io.grpc.DnsNameResolver} is service record is enabled
+ * by default.
+ */
+// Make it package-private so that it cannot be directly referenced by users.  Java service loader
+// requires the provider to be public, but we can hide it under a package-private class.
+final class SecretGrpclbNameResolverProvider {
+
+  private SecretGrpclbNameResolverProvider() {}
+
+  public static final class Provider extends BaseDnsNameResolverProvider {
+
+    private static final boolean SRV_ENABLED =
+        Boolean.parseBoolean(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME, "true"));
+
+    @Override
+    protected boolean isSrvEnabled() {
+      return SRV_ENABLED;
+    }
+
+    @Override
+    public int priority() {
+      // Must be higher than DnsNameResolverProvider#priority.
+      return 6;
+    }
+  }
+}

grpclb/src/main/resources/META-INF/services/io.grpc.NameResolverProvider

@@ -0,0 +1 @@
+io.grpc.grpclb.SecretGrpclbNameResolverProvider$Provider

grpclb/src/test/java/io/grpc/grpclb/SecretGrpclbNameResolverProviderTest.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright 2019 The gRPC Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.grpc.grpclb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static io.grpc.internal.BaseDnsNameResolverProvider.ENABLE_GRPCLB_PROPERTY_NAME;
+import static org.junit.Assume.assumeTrue;
+
+import io.grpc.internal.DnsNameResolverProvider;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class SecretGrpclbNameResolverProviderTest {
+
+  @Test
+  public void priority_shouldBeHigherThanDefaultDnsNameResolver() {
+    DnsNameResolverProvider defaultDnsNameResolver = new DnsNameResolverProvider();
+    SecretGrpclbNameResolverProvider.Provider grpclbDnsNameResolver =
+        new SecretGrpclbNameResolverProvider.Provider();
+
+    assertThat(defaultDnsNameResolver.priority())
+        .isLessThan(grpclbDnsNameResolver.priority());
+  }
+
+  @Test
+  public void isSrvEnabled_trueByDefault() {
+    assumeTrue(System.getProperty(ENABLE_GRPCLB_PROPERTY_NAME) == null);
+
+    SecretGrpclbNameResolverProvider.Provider grpclbDnsNameResolver =
+        new SecretGrpclbNameResolverProvider.Provider();
+
+    assertThat(grpclbDnsNameResolver.isSrvEnabled()).isTrue();
+  }
+}