Skip to content

Commit 482dc5c

Browse files
ejona86ejona86
authored
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
1 parent efe9ccc commit 482dc5c

9 files changed

+71
-45
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.google.common.annotations.VisibleForTesting;
2323
import com.google.common.collect.ImmutableList;
2424
import com.google.common.collect.ImmutableMap;
25+
import com.google.common.net.InetAddresses;
2526
import io.grpc.EquivalentAddressGroup;
2627
import java.net.InetSocketAddress;
2728
import java.util.List;
@@ -78,7 +79,8 @@ static LbEndpoint create(EquivalentAddressGroup eag, int loadBalancingWeight,
7879
@VisibleForTesting
7980
static LbEndpoint create(String address, int port, int loadBalancingWeight, boolean isHealthy,
8081
String hostname, ImmutableMap<String, Object> endpointMetadata) {
81-
return LbEndpoint.create(new EquivalentAddressGroup(new InetSocketAddress(address, port)),
82+
return LbEndpoint.create(
83+
new EquivalentAddressGroup(new InetSocketAddress(InetAddresses.forString(address), port)),
8284
loadBalancingWeight, isHealthy, hostname, endpointMetadata);
8385
}
8486
}

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import io.grpc.xds.client.Locality;
4141
import io.grpc.xds.client.XdsClient.ResourceUpdate;
4242
import io.grpc.xds.client.XdsResourceType;
43+
import java.net.InetAddress;
4344
import java.net.InetSocketAddress;
4445
import java.util.ArrayList;
4546
import java.util.Collections;
@@ -245,10 +246,16 @@ static StructOrError<LocalityLbEndpoints> parseLocalityLbEndpoints(
245246
proto.getPriority(), localityMetadata));
246247
}
247248

248-
private static InetSocketAddress getInetSocketAddress(Address address) {
249+
private static InetSocketAddress getInetSocketAddress(Address address)
250+
throws ResourceInvalidException {
249251
io.envoyproxy.envoy.config.core.v3.SocketAddress socketAddress = address.getSocketAddress();
250-
251-
return new InetSocketAddress(socketAddress.getAddress(), socketAddress.getPortValue());
252+
InetAddress parsedAddress;
253+
try {
254+
parsedAddress = InetAddresses.forString(socketAddress.getAddress());
255+
} catch (IllegalArgumentException ex) {
256+
throw new ResourceInvalidException("Address is not an IP", ex);
257+
}
258+
return new InetSocketAddress(parsedAddress, socketAddress.getPortValue());
252259
}
253260

254261
static final class EdsUpdate implements ResourceUpdate {

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

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -266,17 +266,17 @@ static Cluster buildCluster(String clusterName, String edsName) {
266266
/**
267267
* Builds a new default EDS configuration.
268268
*/
269-
static ClusterLoadAssignment buildClusterLoadAssignment(String hostName, String endpointHostname,
270-
int port) {
271-
return buildClusterLoadAssignment(hostName, endpointHostname, port, EDS_NAME);
269+
static ClusterLoadAssignment buildClusterLoadAssignment(
270+
String hostAddress, String endpointHostname, int port) {
271+
return buildClusterLoadAssignment(hostAddress, endpointHostname, port, EDS_NAME);
272272
}
273273

274-
static ClusterLoadAssignment buildClusterLoadAssignment(String hostName, String endpointHostname,
275-
int port, String edsName) {
274+
static ClusterLoadAssignment buildClusterLoadAssignment(
275+
String hostAddress, String endpointHostname, int port, String edsName) {
276276

277277
Address address = Address.newBuilder()
278278
.setSocketAddress(
279-
SocketAddress.newBuilder().setAddress(hostName).setPortValue(port).build()).build();
279+
SocketAddress.newBuilder().setAddress(hostAddress).setPortValue(port).build()).build();
280280
LocalityLbEndpoints endpoints = LocalityLbEndpoints.newBuilder()
281281
.setLoadBalancingWeight(UInt32Value.of(10))
282282
.setPriority(0)
@@ -297,17 +297,12 @@ static ClusterLoadAssignment buildClusterLoadAssignment(String hostName, String
297297
* Builds a new client listener.
298298
*/
299299
static Listener buildClientListener(String name) {
300-
return buildClientListener(name, "terminal-filter");
300+
return buildClientListener(name, RDS_NAME);
301301
}
302302

303-
304-
static Listener buildClientListener(String name, String identifier) {
305-
return buildClientListener(name, identifier, RDS_NAME);
306-
}
307-
308-
static Listener buildClientListener(String name, String identifier, String rdsName) {
303+
static Listener buildClientListener(String name, String rdsName) {
309304
HttpFilter httpFilter = HttpFilter.newBuilder()
310-
.setName(identifier)
305+
.setName("terminal-filter")
311306
.setTypedConfig(Any.pack(Router.newBuilder().build()))
312307
.setIsOptional(true)
313308
.build();

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

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@
5353
import io.grpc.MethodDescriptor;
5454
import io.grpc.Status;
5555
import io.grpc.StatusOr;
56-
import io.grpc.inprocess.InProcessServerBuilder;
5756
import io.grpc.testing.TestMethodDescriptors;
5857
import io.grpc.xds.Endpoints.LbEndpoint;
5958
import io.grpc.xds.Endpoints.LocalityLbEndpoints;
@@ -84,7 +83,6 @@
8483
public class GcpAuthenticationFilterTest {
8584
private static final GcpAuthenticationFilter.Provider FILTER_PROVIDER =
8685
new GcpAuthenticationFilter.Provider();
87-
private static final String serverName = InProcessServerBuilder.generateName();
8886
private static final LdsUpdate ldsUpdate = getLdsUpdate();
8987
private static final EdsUpdate edsUpdate = getEdsUpdate();
9088
private static final RdsUpdate rdsUpdate = getRdsUpdate();
@@ -461,15 +459,15 @@ public void testLruCacheEvictionOnResize() throws IOException, ResourceInvalidEx
461459

462460
private static LdsUpdate getLdsUpdate() {
463461
Filter.NamedFilterConfig routerFilterConfig = new Filter.NamedFilterConfig(
464-
serverName, RouterFilter.ROUTER_CONFIG);
462+
"router", RouterFilter.ROUTER_CONFIG);
465463
HttpConnectionManager httpConnectionManager = HttpConnectionManager.forRdsName(
466464
0L, RDS_NAME, Collections.singletonList(routerFilterConfig));
467465
return XdsListenerResource.LdsUpdate.forApiListener(httpConnectionManager);
468466
}
469467

470468
private static RdsUpdate getRdsUpdate() {
471469
RouteConfiguration routeConfiguration =
472-
buildRouteConfiguration(serverName, RDS_NAME, CLUSTER_NAME);
470+
buildRouteConfiguration("my-server", RDS_NAME, CLUSTER_NAME);
473471
XdsResourceType.Args args = new XdsResourceType.Args(null, "0", "0", null, null, null);
474472
try {
475473
return XdsRouteConfigureResource.getInstance().doParse(args, routeConfiguration);
@@ -481,7 +479,7 @@ private static RdsUpdate getRdsUpdate() {
481479
private static EdsUpdate getEdsUpdate() {
482480
Map<Locality, LocalityLbEndpoints> lbEndpointsMap = new HashMap<>();
483481
LbEndpoint lbEndpoint = LbEndpoint.create(
484-
serverName, ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
482+
"127.0.0.5", ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
485483
lbEndpointsMap.put(
486484
Locality.create("", "", ""),
487485
LocalityLbEndpoints.create(ImmutableList.of(lbEndpoint), 10, 0, ImmutableMap.of()));

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1077,6 +1077,30 @@ public void parseLocalityLbEndpoints_withHealthyEndpoints() throws ResourceInval
10771077
100, 1, ImmutableMap.of()));
10781078
}
10791079

1080+
@Test
1081+
public void parseLocalityLbEndpoints_onlyPermitIp() {
1082+
io.envoyproxy.envoy.config.endpoint.v3.LocalityLbEndpoints proto =
1083+
io.envoyproxy.envoy.config.endpoint.v3.LocalityLbEndpoints.newBuilder()
1084+
.setLocality(Locality.newBuilder()
1085+
.setRegion("region-foo").setZone("zone-foo").setSubZone("subZone-foo"))
1086+
.setLoadBalancingWeight(UInt32Value.newBuilder().setValue(100)) // locality weight
1087+
.setPriority(1)
1088+
.addLbEndpoints(io.envoyproxy.envoy.config.endpoint.v3.LbEndpoint.newBuilder()
1089+
.setEndpoint(Endpoint.newBuilder()
1090+
.setAddress(Address.newBuilder()
1091+
.setSocketAddress(
1092+
SocketAddress.newBuilder()
1093+
.setAddress("example.com").setPortValue(8888))))
1094+
.setHealthStatus(io.envoyproxy.envoy.config.core.v3.HealthStatus.HEALTHY)
1095+
.setLoadBalancingWeight(UInt32Value.newBuilder().setValue(20))) // endpoint weight
1096+
.build();
1097+
ResourceInvalidException ex = assertThrows(
1098+
ResourceInvalidException.class,
1099+
() -> XdsEndpointResource.parseLocalityLbEndpoints(proto));
1100+
assertThat(ex.getMessage()).contains("IP");
1101+
assertThat(ex.getMessage()).contains("example.com");
1102+
}
1103+
10801104
@Test
10811105
public void parseLocalityLbEndpoints_treatUnknownHealthAsHealthy()
10821106
throws ResourceInvalidException {

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,10 @@ public class XdsClientFallbackTest {
8484
private static final String FALLBACK_EDS_NAME = "fallback-" + EDS_NAME;
8585
private static final HttpConnectionManager MAIN_HTTP_CONNECTION_MANAGER =
8686
HttpConnectionManager.forRdsName(0, RDS_NAME, ImmutableList.of(
87-
new Filter.NamedFilterConfig(MAIN_SERVER, RouterFilter.ROUTER_CONFIG)));
87+
new Filter.NamedFilterConfig("terminal-filter", RouterFilter.ROUTER_CONFIG)));
8888
private static final HttpConnectionManager FALLBACK_HTTP_CONNECTION_MANAGER =
89-
HttpConnectionManager.forRdsName(0, RDS_NAME, ImmutableList.of(
90-
new Filter.NamedFilterConfig(FALLBACK_SERVER, RouterFilter.ROUTER_CONFIG)));
89+
HttpConnectionManager.forRdsName(0, FALLBACK_RDS_NAME, ImmutableList.of(
90+
new Filter.NamedFilterConfig("terminal-filter", RouterFilter.ROUTER_CONFIG)));
9191
private ObjectPool<XdsClient> xdsClientPool;
9292
private XdsClient xdsClient;
9393
private boolean originalEnableXdsFallback;
@@ -201,7 +201,7 @@ private static void setAdsConfig(ControlPlaneRule controlPlane, String serverNam
201201
String edsName = isMainServer ? EDS_NAME : FALLBACK_EDS_NAME;
202202

203203
controlPlane.setLdsConfig(ControlPlaneRule.buildServerListener(),
204-
ControlPlaneRule.buildClientListener(MAIN_SERVER, serverName));
204+
ControlPlaneRule.buildClientListener(MAIN_SERVER, rdsName));
205205

206206
controlPlane.setRdsConfig(rdsName,
207207
XdsTestUtils.buildRouteConfiguration(MAIN_SERVER, rdsName, clusterName));

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ public class XdsDependencyManagerTest {
122122
private Server xdsServer;
123123

124124
private final FakeClock fakeClock = new FakeClock();
125-
private final String serverName = InProcessServerBuilder.generateName();
125+
private final String serverName = "the-service-name";
126126
private final Queue<XdsTestUtils.LrsRpcCall> loadReportCalls = new ArrayDeque<>();
127127
private final AtomicBoolean adsEnded = new AtomicBoolean(true);
128128
private final AtomicBoolean lrsEnded = new AtomicBoolean(true);
@@ -153,7 +153,7 @@ public class XdsDependencyManagerTest {
153153
@Before
154154
public void setUp() throws Exception {
155155
xdsServer = cleanupRule.register(InProcessServerBuilder
156-
.forName(serverName)
156+
.forName("control-plane")
157157
.addService(controlPlaneService)
158158
.addService(lrsService)
159159
.directExecutor()
@@ -163,7 +163,7 @@ public void setUp() throws Exception {
163163
XdsTestUtils.setAdsConfig(controlPlaneService, serverName);
164164

165165
channel = cleanupRule.register(
166-
InProcessChannelBuilder.forName(serverName).directExecutor().build());
166+
InProcessChannelBuilder.forName("control-plane").directExecutor().build());
167167
XdsTransportFactory xdsTransportFactory =
168168
ignore -> new GrpcXdsTransportFactory.GrpcXdsTransport(channel);
169169

@@ -351,10 +351,10 @@ public void testMissingCdsAndEds() {
351351
// Update config so that one of the 2 "valid" clusters has an EDS resource, the other does not
352352
// and there is an EDS that doesn't have matching clusters
353353
ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
354-
serverName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, XdsTestUtils.EDS_NAME + 0);
354+
"127.0.1.1", ENDPOINT_HOSTNAME, ENDPOINT_PORT, XdsTestUtils.EDS_NAME + 0);
355355
edsMap.put(XdsTestUtils.EDS_NAME + 0, clusterLoadAssignment);
356356
clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
357-
serverName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, "garbageEds");
357+
"127.0.1.2", ENDPOINT_HOSTNAME, ENDPOINT_PORT, "garbageEds");
358358
edsMap.put("garbageEds", clusterLoadAssignment);
359359
controlPlaneService.setXdsConfig(ADS_TYPE_URL_EDS, edsMap);
360360

@@ -421,7 +421,7 @@ public void testTcpListenerErrors() {
421421
@Test
422422
public void testMissingRds() {
423423
String rdsName = "badRdsName";
424-
Listener clientListener = ControlPlaneRule.buildClientListener(serverName, serverName, rdsName);
424+
Listener clientListener = ControlPlaneRule.buildClientListener(serverName, rdsName);
425425
controlPlaneService.setXdsConfig(ADS_TYPE_URL_LDS,
426426
ImmutableMap.of(serverName, clientListener));
427427

@@ -578,7 +578,7 @@ public void testMultipleCdsReferToSameEds() {
578578

579579
Map<String, Message> edsMap = new HashMap<>();
580580
ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
581-
serverName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, edsName);
581+
"127.0.1.4", ENDPOINT_HOSTNAME, ENDPOINT_PORT, edsName);
582582
edsMap.put(edsName, clusterLoadAssignment);
583583

584584
RouteConfiguration routeConfig =

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1228,7 +1228,7 @@ private static void createAndDeliverClusterUpdates(
12281228
.roundRobinLbPolicy();
12291229
xdsClient.deliverCdsUpdate(clusterName, forEds.build());
12301230
EdsUpdate edsUpdate = new EdsUpdate(clusterName,
1231-
XdsTestUtils.createMinimalLbEndpointsMap("host"), Collections.emptyList());
1231+
XdsTestUtils.createMinimalLbEndpointsMap("127.0.0.3"), Collections.emptyList());
12321232
xdsClient.deliverEdsUpdate(clusterName, edsUpdate);
12331233
}
12341234
}

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

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ static void setAdsConfig(XdsTestControlPlaneService service, String serverName,
136136
int endpointPort) {
137137

138138
Listener serverListener = ControlPlaneRule.buildServerListener();
139-
Listener clientListener = ControlPlaneRule.buildClientListener(serverName, serverName, rdsName);
139+
Listener clientListener = ControlPlaneRule.buildClientListener(serverName, rdsName);
140140
service.setXdsConfig(ADS_TYPE_URL_LDS,
141141
ImmutableMap.of(SERVER_LISTENER, serverListener, serverName, clientListener));
142142

@@ -148,7 +148,7 @@ static void setAdsConfig(XdsTestControlPlaneService service, String serverName,
148148
service.setXdsConfig(ADS_TYPE_URL_CDS, ImmutableMap.<String, Message>of(clusterName, cluster));
149149

150150
ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
151-
serverName, endpointHostname, endpointPort, edsName);
151+
"127.0.0.11", endpointHostname, endpointPort, edsName);
152152
service.setXdsConfig(ADS_TYPE_URL_EDS,
153153
ImmutableMap.<String, Message>of(edsName, clusterLoadAssignment));
154154

@@ -186,7 +186,7 @@ static void setAggregateCdsConfig(XdsTestControlPlaneService service, String ser
186186
Map<String, Message> edsMap = new HashMap<>();
187187
for (String child : children) {
188188
ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
189-
serverName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, getEdsNameForCluster(child));
189+
"127.0.0.16", ENDPOINT_HOSTNAME, ENDPOINT_PORT, getEdsNameForCluster(child));
190190
edsMap.put(getEdsNameForCluster(child), clusterLoadAssignment);
191191
}
192192
service.setXdsConfig(ADS_TYPE_URL_EDS, edsMap);
@@ -225,7 +225,7 @@ static void addAggregateToExistingConfig(XdsTestControlPlaneService service, Str
225225
continue;
226226
}
227227
ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
228-
child, ENDPOINT_HOSTNAME, ENDPOINT_PORT, getEdsNameForCluster(child));
228+
"127.0.0.15", ENDPOINT_HOSTNAME, ENDPOINT_PORT, getEdsNameForCluster(child));
229229
edsMap.put(getEdsNameForCluster(child), clusterLoadAssignment);
230230
}
231231
service.setXdsConfig(ADS_TYPE_URL_EDS, edsMap);
@@ -236,7 +236,7 @@ static XdsConfig getDefaultXdsConfig(String serverHostName)
236236
XdsConfig.XdsConfigBuilder builder = new XdsConfig.XdsConfigBuilder();
237237

238238
Filter.NamedFilterConfig routerFilterConfig = new Filter.NamedFilterConfig(
239-
serverHostName, RouterFilter.ROUTER_CONFIG);
239+
"terminal-filter", RouterFilter.ROUTER_CONFIG);
240240

241241
HttpConnectionManager httpConnectionManager = HttpConnectionManager.forRdsName(
242242
0L, RDS_NAME, Collections.singletonList(routerFilterConfig));
@@ -257,7 +257,7 @@ static XdsConfig getDefaultXdsConfig(String serverHostName)
257257
// Need to create endpoints to create locality endpoints map to create edsUpdate
258258
Map<Locality, LocalityLbEndpoints> lbEndpointsMap = new HashMap<>();
259259
LbEndpoint lbEndpoint = LbEndpoint.create(
260-
serverHostName, ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
260+
"127.0.0.11", ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
261261
lbEndpointsMap.put(
262262
Locality.create("", "", ""),
263263
LocalityLbEndpoints.create(ImmutableList.of(lbEndpoint), 10, 0, ImmutableMap.of()));
@@ -280,10 +280,10 @@ static XdsConfig getDefaultXdsConfig(String serverHostName)
280280
return builder.build();
281281
}
282282

283-
static Map<Locality, LocalityLbEndpoints> createMinimalLbEndpointsMap(String serverHostName) {
283+
static Map<Locality, LocalityLbEndpoints> createMinimalLbEndpointsMap(String serverAddress) {
284284
Map<Locality, LocalityLbEndpoints> lbEndpointsMap = new HashMap<>();
285285
LbEndpoint lbEndpoint = LbEndpoint.create(
286-
serverHostName, ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
286+
serverAddress, ENDPOINT_PORT, 0, true, ENDPOINT_HOSTNAME, ImmutableMap.of());
287287
lbEndpointsMap.put(
288288
Locality.create("", "", ""),
289289
LocalityLbEndpoints.create(ImmutableList.of(lbEndpoint), 10, 0, ImmutableMap.of()));
@@ -338,15 +338,15 @@ static void addEdsClusters(Map<String, Message> clusterMap, Map<String, Message>
338338
clusterMap.put(clusterName, cluster);
339339

340340
ClusterLoadAssignment clusterLoadAssignment = ControlPlaneRule.buildClusterLoadAssignment(
341-
clusterName, ENDPOINT_HOSTNAME, ENDPOINT_PORT, edsName);
341+
"127.0.0.13", ENDPOINT_HOSTNAME, ENDPOINT_PORT, edsName);
342342
edsMap.put(edsName, clusterLoadAssignment);
343343
}
344344
}
345345

346346
static Listener buildInlineClientListener(String rdsName, String clusterName, String serverName) {
347347
HttpFilter
348348
httpFilter = HttpFilter.newBuilder()
349-
.setName(serverName)
349+
.setName("terminal-filter")
350350
.setTypedConfig(Any.pack(Router.newBuilder().build()))
351351
.setIsOptional(true)
352352
.build();

0 commit comments

Comments
 (0)