Support default port in NameResolver.

- Channel builders decide the default port based on whether TLS is used.
- Channel builders populate the default port via an Attributes object
  passed to NameResolver.Factory#newNameResolver

core/src/main/java/io/grpc/Attributes.java

@@ -99,8 +99,9 @@ public final class Attributes {
       this.product = new Attributes();
     }
 
-    public <T> void set(Key<T> key, T value) {
+    public <T> Builder set(Key<T> key, T value) {
       product.data.put(key.name, value);
+      return this;
     }
 
     /**

core/src/main/java/io/grpc/DnsNameResolverFactory.java

@@ -49,8 +49,9 @@ import javax.annotation.Nullable;
  *
  * <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, exluding the leading slash {@code '/'}, is treated as the host name to be resolved by
- * DNS. Example target URIs:
+ * target URI, exluding 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
@@ -64,13 +65,13 @@ public final class DnsNameResolverFactory extends NameResolver.Factory {
   private static final DnsNameResolverFactory instance = new DnsNameResolverFactory();
 
   @Override
-  public NameResolver newNameResolver(URI targetUri) {
+  public NameResolver newNameResolver(URI targetUri, Attributes params) {
     if ("dns".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);
+      return new DnsNameResolver(targetUri.getAuthority(), name, params);
     } else {
       return null;
     }
@@ -89,7 +90,7 @@ public final class DnsNameResolverFactory extends NameResolver.Factory {
     private final int port;
     private ExecutorService executor;
 
-    DnsNameResolver(@Nullable String nsAuthority, String name) {
+    DnsNameResolver(@Nullable String nsAuthority, String name, Attributes params) {
       // TODO: if a DNS server is provided as nsAuthority, use it.
       // https://www.captechconsulting.com/blogs/accessing-the-dusty-corners-of-dns-with-java
 
@@ -99,8 +100,17 @@ public final class DnsNameResolverFactory extends NameResolver.Factory {
       authority = Preconditions.checkNotNull(nameUri.getAuthority(),
           "nameUri (%s) doesn't have an authority", nameUri);
       host = Preconditions.checkNotNull(nameUri.getHost(), "host");
-      port = nameUri.getPort();
-      Preconditions.checkArgument(port > 0, "port (%s) must be positive", port);
+      if (nameUri.getPort() == -1) {
+        Integer defaultPort = params.get(NameResolver.Factory.PARAMS_DEFAULT_PORT);
+        if (defaultPort != null) {
+          port = defaultPort;
+        } else {
+          throw new IllegalArgumentException(
+              "name '" + name + "' doesn't contain a port, and default port is not set in params");
+        }
+      } else {
+        port = nameUri.getPort();
+      }
     }
 
     @Override

core/src/main/java/io/grpc/NameResolver.java

@@ -71,15 +71,24 @@ public abstract class NameResolver {
   public abstract void shutdown();
 
   public abstract static class Factory {
+    /**
+     * The port number used in case the target or the underlying naming system doesn't provide a
+     * port number.
+     */
+    public static final Attributes.Key<Integer> PARAMS_DEFAULT_PORT =
+        new Attributes.Key<Integer>("io.grpc.NameResolverDefaultPort");
+
     /**
      * Creates a {@link NameResolver} for the given target URI, or {@code null} if the given URI
      * cannot be resolved by this factory. The decision should be solely based on the scheme of the
      * URI.
      *
      * @param targetUri the target URI to be resolved, whose scheme must not be {@code null}
+     * @param params optional parameters. Canonical keys are defined as {@code PARAMS_*} fields in
+     *               {@link Factory}.
      */
     @Nullable
-    public abstract NameResolver newNameResolver(URI targetUri);
+    public abstract NameResolver newNameResolver(URI targetUri, Attributes params);
   }
 
   /**

core/src/main/java/io/grpc/NameResolverRegistry.java

@@ -74,9 +74,9 @@ public final class NameResolverRegistry extends NameResolver.Factory {
    * <p>The factory that was registered later has higher priority.
    */
   @Override
-  public NameResolver newNameResolver(URI targetUri) {
+  public NameResolver newNameResolver(URI targetUri, Attributes params) {
     for (NameResolver.Factory factory : registry) {
-      NameResolver resolver = factory.newNameResolver(targetUri);
+      NameResolver resolver = factory.newNameResolver(targetUri, params);
       if (resolver != null) {
         return resolver;
       }

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

@@ -35,7 +35,6 @@ import com.google.common.base.Preconditions;
 
 import io.grpc.Attributes;
 import io.grpc.ClientInterceptor;
-import io.grpc.Internal;
 import io.grpc.LoadBalancer;
 import io.grpc.ManagedChannelBuilder;
 import io.grpc.NameResolver;
@@ -165,19 +164,28 @@ public abstract class AbstractManagedChannelImplBuilder
         new ExponentialBackoffPolicy.Provider(),
         nameResolverFactory == null ? NameResolverRegistry.getDefaultRegistry()
             : nameResolverFactory,
+        getNameResolverParams(),
         loadBalancerFactory == null ? SimpleLoadBalancerFactory.getInstance()
             : loadBalancerFactory,
         transportFactory, executor, userAgent, interceptors);
   }
 
   /**
-   * Children of AbstractChannelBuilder should override this method to provide the
-   * {@link ClientTransportFactory} appropriate for this channel.  This method is meant for
-   * Transport implementors and should not be used by normal users.
+   * Subclasses should override this method to provide the {@link ClientTransportFactory}
+   * appropriate for this channel. This method is meant for Transport implementors and should not
+   * be used by normal users.
    */
-  @Internal
   protected abstract ClientTransportFactory buildTransportFactory();
 
+  /**
+   * Subclasses can override this method to provide additional parameters to {@link
+   * NameResolver.Factory#newNameResolver}. The default implementation returns {@link
+   * Attributes.EMPTY}.
+   */
+  protected Attributes getNameResolverParams() {
+    return Attributes.EMPTY;
+  }
+
   private static class AuthorityOverridingTransportFactory implements ClientTransportFactory {
     final ClientTransportFactory factory;
     @Nullable final String authorityOverride;
@@ -222,7 +230,7 @@ public abstract class AbstractManagedChannelImplBuilder
     }
 
     @Override
-    public NameResolver newNameResolver(URI notUsedUri) {
+    public NameResolver newNameResolver(URI notUsedUri, Attributes params) {
       return new NameResolver() {
         @Override
         public String getServiceAuthority() {

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

@@ -100,6 +100,16 @@ public final class GrpcUtil {
   public static final Metadata.Key<String> USER_AGENT_KEY =
           Metadata.Key.of("user-agent", Metadata.ASCII_STRING_MARSHALLER);
 
+  /**
+   * The default port for plain-text connections.
+   */
+  public static final int DEFAULT_PORT_PLAINTEXT = 80;
+
+  /**
+   * The default port for SSL connections.
+   */
+  public static final int DEFAULT_PORT_SSL = 443;
+
   /**
    * Content-Type used for GRPC-over-HTTP/2.
    */

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

@@ -133,7 +133,8 @@ public final class ManagedChannelImpl extends ManagedChannel {
   };
 
   ManagedChannelImpl(String target, BackoffPolicy.Provider backoffPolicyProvider,
-      NameResolver.Factory nameResolverFactory, LoadBalancer.Factory loadBalancerFactory,
+      NameResolver.Factory nameResolverFactory, Attributes nameResolverParams,
+      LoadBalancer.Factory loadBalancerFactory,
       ClientTransportFactory transportFactory, @Nullable Executor executor,
       @Nullable String userAgent, List<ClientInterceptor> interceptors) {
     if (executor == null) {
@@ -152,7 +153,7 @@ public final class ManagedChannelImpl extends ManagedChannel {
     StringBuilder uriSyntaxErrors = new StringBuilder();
     try {
       targetUri = new URI(target);
-      nameResolver = nameResolverFactory.newNameResolver(targetUri);
+      nameResolver = nameResolverFactory.newNameResolver(targetUri, nameResolverParams);
       // For "localhost:8080" this would likely return null, because "localhost" is parsed as the
       // scheme. Will fall into the next branch and try "dns:///localhost:8080".
     } catch (URISyntaxException e) {
@@ -163,7 +164,7 @@ public final class ManagedChannelImpl extends ManagedChannel {
     if (nameResolver == null) {
       try {
         targetUri = new URI("dns:///" + target);
-        nameResolver = nameResolverFactory.newNameResolver(targetUri);
+        nameResolver = nameResolverFactory.newNameResolver(targetUri, nameResolverParams);
       } catch (URISyntaxException e) {
         if (uriSyntaxErrors.length() > 0) {
           uriSyntaxErrors.append("; ");

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

@@ -89,6 +89,8 @@ import java.util.concurrent.atomic.AtomicLong;
 public class ManagedChannelImplTest {
   private static final List<ClientInterceptor> NO_INTERCEPTOR =
       Collections.<ClientInterceptor>emptyList();
+  private static final Attributes NAME_RESOLVER_PARAMS =
+      Attributes.newBuilder().set(NameResolver.Factory.PARAMS_DEFAULT_PORT, 447).build();
   private final MethodDescriptor<String, Integer> method = MethodDescriptor.create(
       MethodDescriptor.MethodType.UNKNOWN, "/service/method",
       new StringMarshaller(), new IntegerMarshaller());
@@ -121,7 +123,7 @@ public class ManagedChannelImplTest {
   private ManagedChannel createChannel(
       NameResolver.Factory nameResolverFactory, List<ClientInterceptor> interceptors) {
     return new ManagedChannelImpl(target, new FakeBackoffPolicyProvider(),
-        nameResolverFactory, SimpleLoadBalancerFactory.getInstance(),
+        nameResolverFactory, NAME_RESOLVER_PARAMS, SimpleLoadBalancerFactory.getInstance(),
         mockTransportFactory, executor, null, interceptors);
   }
 
@@ -327,9 +329,10 @@ public class ManagedChannelImplTest {
     }
 
     @Override
-    public NameResolver newNameResolver(final URI targetUri) {
+    public NameResolver newNameResolver(final URI targetUri, Attributes params) {
       assertEquals("fake", targetUri.getScheme());
       assertEquals(serviceName, targetUri.getAuthority());
+      assertSame(NAME_RESOLVER_PARAMS, params);
       return new NameResolver() {
         @Override public String getServiceAuthority() {
           return serviceName;
@@ -352,7 +355,7 @@ public class ManagedChannelImplTest {
     }
 
     @Override
-    public NameResolver newNameResolver(URI notUsedUri) {
+    public NameResolver newNameResolver(URI notUsedUri, Attributes params) {
       return new NameResolver() {
         @Override public String getServiceAuthority() {
           return "irrelevant-authority";

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

@@ -37,7 +37,9 @@ import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 
+import io.grpc.Attributes;
 import io.grpc.ExperimentalApi;
+import io.grpc.NameResolver;
 import io.grpc.internal.AbstractManagedChannelImplBuilder;
 import io.grpc.internal.AbstractReferenceCounted;
 import io.grpc.internal.ClientTransport;
@@ -193,6 +195,24 @@ public class NettyChannelBuilder extends AbstractManagedChannelImplBuilder<Netty
         eventLoopGroup, flowControlWindow, maxMessageSize);
   }
 
+  @Override
+  protected Attributes getNameResolverParams() {
+    int defaultPort;
+    switch (negotiationType) {
+      case PLAINTEXT:
+      case PLAINTEXT_UPGRADE:
+        defaultPort = GrpcUtil.DEFAULT_PORT_PLAINTEXT;
+        break;
+      case TLS:
+        defaultPort = GrpcUtil.DEFAULT_PORT_SSL;
+        break;
+      default:
+        throw new AssertionError(negotiationType + " not handled");
+    }
+    return Attributes.newBuilder()
+        .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, defaultPort).build();
+  }
+
   @VisibleForTesting
   static ProtocolNegotiator createProtocolNegotiator(
       String authority,

okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java

@@ -41,7 +41,9 @@ import com.squareup.okhttp.CipherSuite;
 import com.squareup.okhttp.ConnectionSpec;
 import com.squareup.okhttp.TlsVersion;
 
+import io.grpc.Attributes;
 import io.grpc.ExperimentalApi;
+import io.grpc.NameResolver;
 import io.grpc.internal.AbstractManagedChannelImplBuilder;
 import io.grpc.internal.AbstractReferenceCounted;
 import io.grpc.internal.ClientTransport;
@@ -198,6 +200,23 @@ public class OkHttpChannelBuilder extends
             createSocketFactory(), connectionSpec, maxMessageSize);
   }
 
+  @Override
+  protected Attributes getNameResolverParams() {
+    int defaultPort;
+    switch (negotiationType) {
+      case PLAINTEXT:
+        defaultPort = GrpcUtil.DEFAULT_PORT_PLAINTEXT;
+        break;
+      case TLS:
+        defaultPort = GrpcUtil.DEFAULT_PORT_SSL;
+        break;
+      default:
+        throw new AssertionError(negotiationType + " not handled");
+    }
+    return Attributes.newBuilder()
+        .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, defaultPort).build();
+  }
+
   private SSLSocketFactory createSocketFactory() {
     switch (negotiationType) {
       case TLS: