xds: Don't allow hostnames in address field (#12123)

* xds: Don't allow hostnames in address field

gRFC A27 specifies they must be IPv4 or IPv6 addresses. Certainly doing
a DNS lookup hidden inside the config object is asking for trouble.

The tests were accidentally doing a lot of failing DNS requests greatly
slowing them down. On my desktop, which made the problem most obvious
with five search paths in /etc/resolv.conf, :grpc-xds:test decreased
from 66s to 29s. The majority of that is XdsDependencyManagerTest which
went from 33s to .1s, as it generated a UUID for the in-process
transport each test and then used it as a hostname, which defeated
Java's DNS (negative) cache. The slowness was noticed because
XdsDependencyManagerTest should have run quickly as a single thread
without I/O, but was particularly slow on my desktop.

The cleanup caused me to audit serverName and the weird places it went.
I think some of them were tricks for XdsClientFallbackTest to squirrel
away something distinguishing, although reusing the serverName is asking
for confusion as is including the tricks in "shared" utilities.
XdsClientFallbackTest does have some non-trivial changes, but this seems
to fix some pre-existing bugs in the tests.

* Add failing hostname unit test

xds/src/main/java/io/grpc/xds/Endpoints.java

@@ -22,6 +22,7 @@ import com.google.auto.value.AutoValue;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.net.InetAddresses;
 import io.grpc.EquivalentAddressGroup;
 import java.net.InetSocketAddress;
 import java.util.List;
@@ -78,7 +79,8 @@ final class Endpoints {
     @VisibleForTesting
     static LbEndpoint create(String address, int port, int loadBalancingWeight, boolean isHealthy,
         String hostname, ImmutableMap<String, Object> endpointMetadata) {
-      return LbEndpoint.create(new EquivalentAddressGroup(new InetSocketAddress(address, port)),
+      return LbEndpoint.create(
+          new EquivalentAddressGroup(new InetSocketAddress(InetAddresses.forString(address), port)),
           loadBalancingWeight, isHealthy, hostname, endpointMetadata);
     }
   }

xds/src/main/java/io/grpc/xds/XdsEndpointResource.java

@@ -40,6 +40,7 @@ import io.grpc.xds.XdsEndpointResource.EdsUpdate;
 import io.grpc.xds.client.Locality;
 import io.grpc.xds.client.XdsClient.ResourceUpdate;
 import io.grpc.xds.client.XdsResourceType;
+import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -245,10 +246,16 @@ class XdsEndpointResource extends XdsResourceType<EdsUpdate> {
         proto.getPriority(), localityMetadata));
   }
 
-  private static InetSocketAddress getInetSocketAddress(Address address) {
+  private static InetSocketAddress getInetSocketAddress(Address address)
+      throws ResourceInvalidException {
     io.envoyproxy.envoy.config.core.v3.SocketAddress socketAddress = address.getSocketAddress();
-
+    InetAddress parsedAddress;
-    return new InetSocketAddress(socketAddress.getAddress(), socketAddress.getPortValue());
+    try {
+      parsedAddress = InetAddresses.forString(socketAddress.getAddress());
+    } catch (IllegalArgumentException ex) {
+      throw new ResourceInvalidException("Address is not an IP", ex);
+    }
+    return new InetSocketAddress(parsedAddress, socketAddress.getPortValue());
   }
 
   static final class EdsUpdate implements ResourceUpdate {

xds/src/test/java/io/grpc/xds/ControlPlaneRule.java

@@ -266,17 +266,17 @@ public class ControlPlaneRule extends TestWatcher {
   /**
    * Builds a new default EDS configuration.
    */
-  static ClusterLoadAssignment buildClusterLoadAssignment(String hostName, String endpointHostname,
+  static ClusterLoadAssignment buildClusterLoadAssignment(
-                                                          int port) {
+          String hostAddress, String endpointHostname, int port) {
-    return buildClusterLoadAssignment(hostName, endpointHostname, port, EDS_NAME);
+    return buildClusterLoadAssignment(hostAddress, endpointHostname, port, EDS_NAME);
   }
 
-  static ClusterLoadAssignment buildClusterLoadAssignment(String hostName, String endpointHostname,
+  static ClusterLoadAssignment buildClusterLoadAssignment(
-                                                          int port, String edsName) {
+          String hostAddress, String endpointHostname, int port, String edsName) {
 
     Address address = Address.newBuilder()
         .setSocketAddress(
-            SocketAddress.newBuilder().setAddress(hostName).setPortValue(port).build()).build();
+            SocketAddress.newBuilder().setAddress(hostAddress).setPortValue(port).build()).build();
     LocalityLbEndpoints endpoints = LocalityLbEndpoints.newBuilder()
         .setLoadBalancingWeight(UInt32Value.of(10))
         .setPriority(0)
@@ -297,17 +297,12 @@ public class ControlPlaneRule extends TestWatcher {
    * Builds a new client listener.
    */
   static Listener buildClientListener(String name) {
-    return buildClientListener(name, "terminal-filter");
+    return buildClientListener(name, RDS_NAME);
   }
 
-
+  static Listener buildClientListener(String name, String rdsName) {
-  static Listener buildClientListener(String name, String identifier) {
-    return buildClientListener(name, identifier, RDS_NAME);
-  }
-
-  static Listener buildClientListener(String name, String identifier, String rdsName) {
     HttpFilter httpFilter = HttpFilter.newBuilder()
-        .setName(identifier)
+        .setName("terminal-filter")
         .setTypedConfig(Any.pack(Router.newBuilder().build()))
         .setIsOptional(true)
         .build();

xds/src/test/java/io/grpc/xds/GcpAuthenticationFilterTest.java

@@ -53,7 +53,6 @@ import io.grpc.ClientInterceptor;
 import io.grpc.MethodDescriptor;
 import io.grpc.Status;
 import io.grpc.StatusOr;
-import io.grpc.inprocess.InProcessServerBuilder;
 import io.grpc.testing.TestMethodDescriptors;
 import io.grpc.xds.Endpoints.LbEndpoint;
 import io.grpc.xds.Endpoints.LocalityLbEndpoints;
@@ -84,7 +83,6 @@ import org.mockito.Mockito;
 public class GcpAuthenticationFilterTest {
   private static final GcpAuthenticationFilter.Provider FILTER_PROVIDER =
       new GcpAuthenticationFilter.Provider();
-  private static final String serverName = InProcessServerBuilder.generateName();
   private static final LdsUpdate ldsUpdate = getLdsUpdate();
   private static final EdsUpdate edsUpdate = getEdsUpdate();
   private static final RdsUpdate rdsUpdate = getRdsUpdate();
@@ -461,7 +459,7 @@ public class GcpAuthenticationFilterTest {
 
   private static LdsUpdate getLdsUpdate() {
     Filter.NamedFilterConfig routerFilterConfig = new Filter.NamedFilterConfig(
-        serverName, RouterFilter.ROUTER_CONFIG);
+        "router", RouterFilter.ROUTER_CONFIG);
     HttpConnectionManager httpConnectionManager = HttpConnectionManager.forRdsName(
         0L, RDS_NAME, Collections.singletonList(routerFilterConfig));
     return XdsListenerResource.LdsUpdate.forApiListener(httpConnectionManager);
@@ -469,7 +467,7 @@ public class GcpAuthenticationFilterTest {
 
   private static RdsUpdate getRdsUpdate() {
     RouteConfiguration routeConfiguration =
-        buildRouteConfiguration(serverName, RDS_NAME, CLUSTER_NAME);
+        buildRouteConfiguration("my-server", RDS_NAME, CLUSTER_NAME);
     XdsResourceType.Args args = new XdsResourceType.Args(null, "0", "0", null, null, null);
     try {
       return XdsRouteConfigureResource.getInstance().doParse(args, routeConfiguration);
@@ -481,7 +479,7 @@ public class GcpAuthenticationFilterTest {
   private static EdsUpdate getEdsUpdate() {
     Map<Locality, LocalityLbEndpoints> lbEndpointsMap = new HashMap<>();
     LbEndpoint lbEndpoint = LbEndpoint.create(
-        serverName, ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
+        "127.0.0.5", ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
     lbEndpointsMap.put(
         Locality.create("", "", ""),
         LocalityLbEndpoints.create(ImmutableList.of(lbEndpoint), 10, 0, ImmutableMap.of()));

xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java

@@ -1077,6 +1077,30 @@ public class GrpcXdsClientImplDataTest {
             100, 1, ImmutableMap.of()));
   }
 
+  @Test
+  public void parseLocalityLbEndpoints_onlyPermitIp() {
+    io.envoyproxy.envoy.config.endpoint.v3.LocalityLbEndpoints proto =
+        io.envoyproxy.envoy.config.endpoint.v3.LocalityLbEndpoints.newBuilder()
+            .setLocality(Locality.newBuilder()
+                .setRegion("region-foo").setZone("zone-foo").setSubZone("subZone-foo"))
+            .setLoadBalancingWeight(UInt32Value.newBuilder().setValue(100))  // locality weight
+            .setPriority(1)
+            .addLbEndpoints(io.envoyproxy.envoy.config.endpoint.v3.LbEndpoint.newBuilder()
+                .setEndpoint(Endpoint.newBuilder()
+                    .setAddress(Address.newBuilder()
+                        .setSocketAddress(
+                            SocketAddress.newBuilder()
+                                .setAddress("example.com").setPortValue(8888))))
+                .setHealthStatus(io.envoyproxy.envoy.config.core.v3.HealthStatus.HEALTHY)
+                .setLoadBalancingWeight(UInt32Value.newBuilder().setValue(20)))  // endpoint weight
+            .build();
+    ResourceInvalidException ex = assertThrows(
+        ResourceInvalidException.class,
+        () -> XdsEndpointResource.parseLocalityLbEndpoints(proto));
+    assertThat(ex.getMessage()).contains("IP");
+    assertThat(ex.getMessage()).contains("example.com");
+  }
+
   @Test
   public void parseLocalityLbEndpoints_treatUnknownHealthAsHealthy()
       throws ResourceInvalidException {

xds/src/test/java/io/grpc/xds/XdsClientFallbackTest.java

@@ -84,10 +84,10 @@ public class XdsClientFallbackTest {
   private static final String FALLBACK_EDS_NAME = "fallback-" + EDS_NAME;
   private static final HttpConnectionManager MAIN_HTTP_CONNECTION_MANAGER =
       HttpConnectionManager.forRdsName(0, RDS_NAME, ImmutableList.of(
-          new Filter.NamedFilterConfig(MAIN_SERVER, RouterFilter.ROUTER_CONFIG)));
+          new Filter.NamedFilterConfig("terminal-filter", RouterFilter.ROUTER_CONFIG)));
   private static final HttpConnectionManager FALLBACK_HTTP_CONNECTION_MANAGER =
-      HttpConnectionManager.forRdsName(0, RDS_NAME, ImmutableList.of(
+      HttpConnectionManager.forRdsName(0, FALLBACK_RDS_NAME, ImmutableList.of(
-          new Filter.NamedFilterConfig(FALLBACK_SERVER, RouterFilter.ROUTER_CONFIG)));
+          new Filter.NamedFilterConfig("terminal-filter", RouterFilter.ROUTER_CONFIG)));
   private ObjectPool<XdsClient> xdsClientPool;
   private XdsClient xdsClient;
   private boolean originalEnableXdsFallback;
@@ -201,7 +201,7 @@ public class XdsClientFallbackTest {
     String edsName = isMainServer ? EDS_NAME : FALLBACK_EDS_NAME;
 
     controlPlane.setLdsConfig(ControlPlaneRule.buildServerListener(),
-        ControlPlaneRule.buildClientListener(MAIN_SERVER, serverName));
+        ControlPlaneRule.buildClientListener(MAIN_SERVER, rdsName));
 
     controlPlane.setRdsConfig(rdsName,
         XdsTestUtils.buildRouteConfiguration(MAIN_SERVER, rdsName, clusterName));

xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java

@@ -122,7 +122,7 @@ public class XdsDependencyManagerTest {
   private Server xdsServer;
 
   private final FakeClock fakeClock = new FakeClock();
-  private final String serverName = InProcessServerBuilder.generateName();
+  private final String serverName = "the-service-name";
   private final Queue<XdsTestUtils.LrsRpcCall> loadReportCalls = new ArrayDeque<>();
   private final AtomicBoolean adsEnded = new AtomicBoolean(true);
   private final AtomicBoolean lrsEnded = new AtomicBoolean(true);
@@ -153,7 +153,7 @@ public class XdsDependencyManagerTest {
   @Before
   public void setUp() throws Exception {
     xdsServer = cleanupRule.register(InProcessServerBuilder
-        .forName(serverName)
+        .forName("control-plane")
         .addService(controlPlaneService)
         .addService(lrsService)
         .directExecutor()
@@ -163,7 +163,7 @@ public class XdsDependencyManagerTest {
     XdsTestUtils.setAdsConfig(controlPlaneService, serverName);
 
     channel = cleanupRule.register(
-        InProcessChannelBuilder.forName(serverName).directExecutor().build());
+        InProcessChannelBuilder.forName("control-plane").directExecutor().build());
     XdsTransportFactory xdsTransportFactory =
         ignore -> new GrpcXdsTransportFactory.GrpcXdsTransport(channel);
 
@@ -351,10 +351,10 @@ public class XdsDependencyManagerTest {
     // Update config so that one of the 2 "valid" clusters has an EDS resource, the other does not
     // and there is an EDS that doesn't have matching clusters
     ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
-        serverName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, XdsTestUtils.EDS_NAME + 0);
+        "127.0.1.1", ENDPOINT_HOSTNAME, ENDPOINT_PORT, XdsTestUtils.EDS_NAME + 0);
     edsMap.put(XdsTestUtils.EDS_NAME + 0, clusterLoadAssignment);
     clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
-        serverName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, "garbageEds");
+        "127.0.1.2", ENDPOINT_HOSTNAME, ENDPOINT_PORT, "garbageEds");
     edsMap.put("garbageEds", clusterLoadAssignment);
     controlPlaneService.setXdsConfig(ADS_TYPE_URL_EDS, edsMap);
 
@@ -421,7 +421,7 @@ public class XdsDependencyManagerTest {
   @Test
   public void testMissingRds() {
     String rdsName = "badRdsName";
-    Listener clientListener = ControlPlaneRule.buildClientListener(serverName, serverName, rdsName);
+    Listener clientListener = ControlPlaneRule.buildClientListener(serverName, rdsName);
     controlPlaneService.setXdsConfig(ADS_TYPE_URL_LDS,
         ImmutableMap.of(serverName, clientListener));
 
@@ -578,7 +578,7 @@ public class XdsDependencyManagerTest {
 
     Map<String, Message> edsMap = new HashMap<>();
     ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
-        serverName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, edsName);
+        "127.0.1.4", ENDPOINT_HOSTNAME, ENDPOINT_PORT, edsName);
     edsMap.put(edsName, clusterLoadAssignment);
 
     RouteConfiguration routeConfig =

xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java

@@ -1228,7 +1228,7 @@ public class XdsNameResolverTest {
               .roundRobinLbPolicy();
       xdsClient.deliverCdsUpdate(clusterName, forEds.build());
       EdsUpdate edsUpdate = new EdsUpdate(clusterName,
-          XdsTestUtils.createMinimalLbEndpointsMap("host"), Collections.emptyList());
+          XdsTestUtils.createMinimalLbEndpointsMap("127.0.0.3"), Collections.emptyList());
       xdsClient.deliverEdsUpdate(clusterName, edsUpdate);
     }
   }

xds/src/test/java/io/grpc/xds/XdsTestUtils.java

@@ -136,7 +136,7 @@ public class XdsTestUtils {
                            int endpointPort) {
 
     Listener serverListener = ControlPlaneRule.buildServerListener();
-    Listener clientListener = ControlPlaneRule.buildClientListener(serverName, serverName, rdsName);
+    Listener clientListener = ControlPlaneRule.buildClientListener(serverName, rdsName);
     service.setXdsConfig(ADS_TYPE_URL_LDS,
         ImmutableMap.of(SERVER_LISTENER, serverListener, serverName, clientListener));
 
@@ -148,7 +148,7 @@ public class XdsTestUtils {
     service.setXdsConfig(ADS_TYPE_URL_CDS, ImmutableMap.<String, Message>of(clusterName, cluster));
 
     ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
-        serverName, endpointHostname, endpointPort, edsName);
+        "127.0.0.11", endpointHostname, endpointPort, edsName);
     service.setXdsConfig(ADS_TYPE_URL_EDS,
         ImmutableMap.<String, Message>of(edsName, clusterLoadAssignment));
 
@@ -186,7 +186,7 @@ public class XdsTestUtils {
     Map<String, Message> edsMap = new HashMap<>();
     for (String child : children) {
       ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
-          serverName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, getEdsNameForCluster(child));
+          "127.0.0.16", ENDPOINT_HOSTNAME, ENDPOINT_PORT, getEdsNameForCluster(child));
       edsMap.put(getEdsNameForCluster(child), clusterLoadAssignment);
     }
     service.setXdsConfig(ADS_TYPE_URL_EDS, edsMap);
@@ -225,7 +225,7 @@ public class XdsTestUtils {
         continue;
       }
       ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
-          child, ENDPOINT_HOSTNAME, ENDPOINT_PORT, getEdsNameForCluster(child));
+          "127.0.0.15", ENDPOINT_HOSTNAME, ENDPOINT_PORT, getEdsNameForCluster(child));
       edsMap.put(getEdsNameForCluster(child), clusterLoadAssignment);
     }
     service.setXdsConfig(ADS_TYPE_URL_EDS, edsMap);
@@ -236,7 +236,7 @@ public class XdsTestUtils {
     XdsConfig.XdsConfigBuilder builder = new XdsConfig.XdsConfigBuilder();
 
     Filter.NamedFilterConfig routerFilterConfig = new Filter.NamedFilterConfig(
-        serverHostName, RouterFilter.ROUTER_CONFIG);
+        "terminal-filter", RouterFilter.ROUTER_CONFIG);
 
     HttpConnectionManager httpConnectionManager = HttpConnectionManager.forRdsName(
         0L, RDS_NAME, Collections.singletonList(routerFilterConfig));
@@ -257,7 +257,7 @@ public class XdsTestUtils {
     // Need to create endpoints to create locality endpoints map to create edsUpdate
     Map<Locality, LocalityLbEndpoints> lbEndpointsMap = new HashMap<>();
     LbEndpoint lbEndpoint = LbEndpoint.create(
-        serverHostName, ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
+        "127.0.0.11", ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
     lbEndpointsMap.put(
         Locality.create("", "", ""),
         LocalityLbEndpoints.create(ImmutableList.of(lbEndpoint), 10, 0, ImmutableMap.of()));
@@ -280,10 +280,10 @@ public class XdsTestUtils {
     return builder.build();
   }
 
-  static Map<Locality, LocalityLbEndpoints> createMinimalLbEndpointsMap(String serverHostName) {
+  static Map<Locality, LocalityLbEndpoints> createMinimalLbEndpointsMap(String serverAddress) {
     Map<Locality, LocalityLbEndpoints> lbEndpointsMap = new HashMap<>();
     LbEndpoint lbEndpoint = LbEndpoint.create(
-        serverHostName, ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
+        serverAddress, ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
     lbEndpointsMap.put(
         Locality.create("", "", ""),
         LocalityLbEndpoints.create(ImmutableList.of(lbEndpoint), 10, 0, ImmutableMap.of()));
@@ -338,7 +338,7 @@ public class XdsTestUtils {
       clusterMap.put(clusterName, cluster);
 
       ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
-          clusterName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, edsName);
+          "127.0.0.13", ENDPOINT_HOSTNAME, ENDPOINT_PORT, edsName);
       edsMap.put(edsName, clusterLoadAssignment);
     }
   }
@@ -346,7 +346,7 @@ public class XdsTestUtils {
   static Listener buildInlineClientListener(String rdsName, String clusterName, String serverName) {
     HttpFilter
         httpFilter = HttpFilter.newBuilder()
-        .setName(serverName)
+        .setName("terminal-filter")
         .setTypedConfig(Any.pack(Router.newBuilder().build()))
         .setIsOptional(true)
         .build();