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

Author: ejona86

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

@@ -22,6 +22,7 @@
 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 @@ static LbEndpoint create(EquivalentAddressGroup eag, int loadBalancingWeight,
     @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.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 @@ static StructOrError<LocalityLbEndpoints> parseLocalityLbEndpoints(
         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();
-
-    return new InetSocketAddress(socketAddress.getAddress(), socketAddress.getPortValue());
+    InetAddress parsedAddress;
+    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 @@ static Cluster buildCluster(String clusterName, String edsName) {
   /**
    * Builds a new default EDS configuration.
    */
-  static ClusterLoadAssignment buildClusterLoadAssignment(String hostName, String endpointHostname,
-                                                          int port) {
-    return buildClusterLoadAssignment(hostName, endpointHostname, port, EDS_NAME);
+  static ClusterLoadAssignment buildClusterLoadAssignment(
+          String hostAddress, String endpointHostname, int port) {
+    return buildClusterLoadAssignment(hostAddress, endpointHostname, port, EDS_NAME);
   }
 
-  static ClusterLoadAssignment buildClusterLoadAssignment(String hostName, String endpointHostname,
-                                                          int port, String edsName) {
+  static ClusterLoadAssignment buildClusterLoadAssignment(
+          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 @@ static ClusterLoadAssignment buildClusterLoadAssignment(String hostName, String
    * 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 identifier) {
-    return buildClientListener(name, identifier, RDS_NAME);
-  }
-
-  static Listener buildClientListener(String name, String identifier, String rdsName) {
+  static Listener buildClientListener(String name, 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.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 @@
 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,15 +459,15 @@ public void testLruCacheEvictionOnResize() throws IOException, ResourceInvalidEx
 
   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);
   }
 
   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 @@ private static RdsUpdate getRdsUpdate() {
   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 void parseLocalityLbEndpoints_withHealthyEndpoints() throws ResourceInval
             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(
-          new Filter.NamedFilterConfig(FALLBACK_SERVER, RouterFilter.ROUTER_CONFIG)));
+      HttpConnectionManager.forRdsName(0, FALLBACK_RDS_NAME, ImmutableList.of(
+          new Filter.NamedFilterConfig("terminal-filter", RouterFilter.ROUTER_CONFIG)));
   private ObjectPool<XdsClient> xdsClientPool;
   private XdsClient xdsClient;
   private boolean originalEnableXdsFallback;
@@ -201,7 +201,7 @@ private static void setAdsConfig(ControlPlaneRule controlPlane, String serverNam
     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 void setUp() throws Exception {
     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 void testMissingCdsAndEds() {
     // 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 void testTcpListenerErrors() {
   @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 void testMultipleCdsReferToSameEds() {
 
     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 @@ private static void createAndDeliverClusterUpdates(
               .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 @@ static void setAdsConfig(XdsTestControlPlaneService service, String serverName,
                            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 @@ static void setAdsConfig(XdsTestControlPlaneService service, String serverName,
     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 @@ static void setAggregateCdsConfig(XdsTestControlPlaneService service, String ser
     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 @@ static void addAggregateToExistingConfig(XdsTestControlPlaneService service, Str
         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 @@ static XdsConfig getDefaultXdsConfig(String serverHostName)
     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 @@ static XdsConfig getDefaultXdsConfig(String serverHostName)
     // 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 @@ static XdsConfig getDefaultXdsConfig(String serverHostName)
     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,15 +338,15 @@ static void addEdsClusters(Map<String, Message> clusterMap, Map<String, Message>
       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);
     }
   }
 
   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();