From da46505579dca4de3bdbf1c00a93fe5f08045d25 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Sun, 1 Jun 2025 14:40:46 -0700 Subject: [PATCH 1/2] 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. --- xds/src/main/java/io/grpc/xds/Endpoints.java | 4 +++- .../java/io/grpc/xds/XdsEndpointResource.java | 13 ++++++++--- .../java/io/grpc/xds/ControlPlaneRule.java | 23 ++++++++----------- .../grpc/xds/GcpAuthenticationFilterTest.java | 8 +++---- .../io/grpc/xds/XdsClientFallbackTest.java | 8 +++---- .../io/grpc/xds/XdsDependencyManagerTest.java | 14 +++++------ .../java/io/grpc/xds/XdsNameResolverTest.java | 2 +- .../test/java/io/grpc/xds/XdsTestUtils.java | 20 ++++++++-------- 8 files changed, 47 insertions(+), 45 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/Endpoints.java b/xds/src/main/java/io/grpc/xds/Endpoints.java index b0d97d42c11..dcb72f3e90d 100644 --- a/xds/src/main/java/io/grpc/xds/Endpoints.java +++ b/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 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); } } diff --git a/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java b/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java index 11111fa51ca..9ad75595ea6 100644 --- a/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java +++ b/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 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 { diff --git a/xds/src/test/java/io/grpc/xds/ControlPlaneRule.java b/xds/src/test/java/io/grpc/xds/ControlPlaneRule.java index 8c10627d153..11ea957ae35 100644 --- a/xds/src/test/java/io/grpc/xds/ControlPlaneRule.java +++ b/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(); diff --git a/xds/src/test/java/io/grpc/xds/GcpAuthenticationFilterTest.java b/xds/src/test/java/io/grpc/xds/GcpAuthenticationFilterTest.java index cebf739d417..1d6c97d81e6 100644 --- a/xds/src/test/java/io/grpc/xds/GcpAuthenticationFilterTest.java +++ b/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,7 +459,7 @@ 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); @@ -469,7 +467,7 @@ private static LdsUpdate getLdsUpdate() { 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 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())); diff --git a/xds/src/test/java/io/grpc/xds/XdsClientFallbackTest.java b/xds/src/test/java/io/grpc/xds/XdsClientFallbackTest.java index 036b9f6f55d..1e7ce6dc2a2 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientFallbackTest.java +++ b/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 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)); diff --git a/xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java b/xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java index 542471e2734..14f554412a8 100644 --- a/xds/src/test/java/io/grpc/xds/XdsDependencyManagerTest.java +++ b/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 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 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 = diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index 622084d4306..ab966ce0025 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/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); } } diff --git a/xds/src/test/java/io/grpc/xds/XdsTestUtils.java b/xds/src/test/java/io/grpc/xds/XdsTestUtils.java index 52953ef5407..ec1ccd7c6d0 100644 --- a/xds/src/test/java/io/grpc/xds/XdsTestUtils.java +++ b/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.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.of(edsName, clusterLoadAssignment)); @@ -186,7 +186,7 @@ static void setAggregateCdsConfig(XdsTestControlPlaneService service, String ser Map 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 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 createMinimalLbEndpointsMap(String serverHostName) { + static Map createMinimalLbEndpointsMap(String serverAddress) { Map 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 @@ static void addEdsClusters(Map clusterMap, Map 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 @@ static void addEdsClusters(Map clusterMap, Map 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(); From 51cd5bf7943c733a5ecf5c0e890f9b01f9021ee1 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 4 Jun 2025 14:55:16 -0700 Subject: [PATCH 2/2] Add failing hostname unit test --- .../grpc/xds/GrpcXdsClientImplDataTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java index e129e2644e9..6167f491930 100644 --- a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java +++ b/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 {