From b29c3ec021c49506f8cbb342f36a25e22c0a78f7 Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Mon, 24 Jan 2022 16:55:43 -0800 Subject: [PATCH] xds/federation: validate and canonify resource name On reading a new `xdstp`: resource name, do a validation on the URI and canonify the query params. --- .../java/io/grpc/xds/ClientXdsClient.java | 33 ++++-- xds/src/main/java/io/grpc/xds/XdsClient.java | 57 +++++++++ .../java/io/grpc/xds/XdsNameResolver.java | 8 ++ .../io/grpc/xds/ClientXdsClientDataTest.java | 46 ++++++++ .../io/grpc/xds/ClientXdsClientTestBase.java | 111 +++++++++++++++++- .../java/io/grpc/xds/XdsNameResolverTest.java | 6 +- 6 files changed, 246 insertions(+), 15 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java index 44f5c3db20..202bfedcca 100644 --- a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java +++ b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java @@ -298,7 +298,12 @@ final class ClientXdsClient extends XdsClient implements XdsResponseHandler, Res errors.add("LDS response Resource index " + i + " - can't decode Listener: " + e); continue; } - String listenerName = listener.getName(); + if (!isResourceNameValid(listener.getName(), resource.getTypeUrl())) { + errors.add( + "Unsupported resource name: " + listener.getName() + " for type: " + ResourceType.LDS); + continue; + } + String listenerName = canonifyResourceName(listener.getName()); unpackedResources.add(listenerName); // Process Listener into LdsUpdate. @@ -1425,7 +1430,13 @@ final class ClientXdsClient extends XdsClient implements XdsResponseHandler, Res errors.add("RDS response Resource index " + i + " - can't decode RouteConfiguration: " + e); continue; } - String routeConfigName = routeConfig.getName(); + if (!isResourceNameValid(routeConfig.getName(), resource.getTypeUrl())) { + errors.add( + "Unsupported resource name: " + routeConfig.getName() + " for type: " + + ResourceType.RDS); + continue; + } + String routeConfigName = canonifyResourceName(routeConfig.getName()); unpackedResources.add(routeConfigName); // Process RouteConfiguration into RdsUpdate. @@ -1547,7 +1558,12 @@ final class ClientXdsClient extends XdsClient implements XdsResponseHandler, Res errors.add("CDS response Resource index " + i + " - can't decode Cluster: " + e); continue; } - String clusterName = cluster.getName(); + if (!isResourceNameValid(cluster.getName(), resource.getTypeUrl())) { + errors.add( + "Unsupported resource name: " + cluster.getName() + " for type: " + ResourceType.CDS); + continue; + } + String clusterName = canonifyResourceName(cluster.getName()); // Management server is required to always send newly requested resources, even if they // may have been sent previously (proactively). Thus, client does not need to cache @@ -1793,7 +1809,12 @@ final class ClientXdsClient extends XdsClient implements XdsResponseHandler, Res "EDS response Resource index " + i + " - can't decode ClusterLoadAssignment: " + e); continue; } - String clusterName = assignment.getClusterName(); + if (!isResourceNameValid(assignment.getClusterName(), resource.getTypeUrl())) { + errors.add("Unsupported resource name: " + assignment.getClusterName() + " for type: " + + ResourceType.EDS); + continue; + } + String clusterName = canonifyResourceName(assignment.getClusterName()); // Skip information for clusters not requested. // Management server is required to always send newly requested resources, even if they @@ -2382,10 +2403,6 @@ final class ClientXdsClient extends XdsClient implements XdsResponseHandler, Res ResourceSubscriber(ResourceType type, String resource) { syncContext.throwIfNotInThisSynchronizationContext(); this.type = type; - // TODO(zdapeng): Validate authority in resource URI for new-style resource name - // when parsing XDS response. - // TODO(zdapeng): Canonicalize the resource name by sorting the context params in normal - // lexicographic order. this.resource = resource; this.serverInfo = getServerInfo(resource); // Initialize metadata in UNKNOWN state to cover the case when resource subscriber, diff --git a/xds/src/main/java/io/grpc/xds/XdsClient.java b/xds/src/main/java/io/grpc/xds/XdsClient.java index 789e576da0..ba9c58153a 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClient.java +++ b/xds/src/main/java/io/grpc/xds/XdsClient.java @@ -17,9 +17,12 @@ package io.grpc.xds; import static com.google.common.base.Preconditions.checkNotNull; +import static io.grpc.xds.Bootstrapper.XDSTP_SCHEME; import com.google.auto.value.AutoValue; +import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; +import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ListenableFuture; import com.google.protobuf.Any; @@ -32,6 +35,8 @@ import io.grpc.xds.EnvoyServerProtoData.Listener; import io.grpc.xds.EnvoyServerProtoData.UpstreamTlsContext; import io.grpc.xds.LoadStatsManager2.ClusterDropStats; import io.grpc.xds.LoadStatsManager2.ClusterLocalityStats; +import java.net.URI; +import java.net.URISyntaxException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -49,6 +54,58 @@ import javax.annotation.Nullable; */ abstract class XdsClient { + static boolean isResourceNameValid(String resourceName, String typeUrl) { + checkNotNull(resourceName, "resourceName"); + if (!resourceName.startsWith(XDSTP_SCHEME)) { + return true; + } + URI uri; + try { + uri = new URI(resourceName); + } catch (URISyntaxException e) { + return false; + } + String path = uri.getPath(); + // path must be in the form of /{resource type}/{id/*} + Splitter slashSplitter = Splitter.on('/').omitEmptyStrings(); + if (path == null) { + return false; + } + List pathSegs = slashSplitter.splitToList(path); + if (pathSegs.size() < 2) { + return false; + } + String type = pathSegs.get(0); + if (!type.equals(slashSplitter.splitToList(typeUrl).get(1))) { + return false; + } + return true; + } + + static String canonifyResourceName(String resourceName) { + checkNotNull(resourceName, "resourceName"); + if (!resourceName.startsWith(XDSTP_SCHEME)) { + return resourceName; + } + URI uri = URI.create(resourceName); + String rawQuery = uri.getRawQuery(); + Splitter ampSplitter = Splitter.on('&').omitEmptyStrings(); + if (rawQuery == null) { + return resourceName; + } + List queries = ampSplitter.splitToList(rawQuery); + if (queries.size() < 2) { + return resourceName; + } + List canonicalContextParams = new ArrayList<>(queries.size()); + for (String query : queries) { + canonicalContextParams.add(query); + } + Collections.sort(canonicalContextParams); + String canonifiedQuery = Joiner.on('&').join(canonicalContextParams); + return resourceName.replace(rawQuery, canonifiedQuery); + } + @AutoValue abstract static class LdsUpdate implements ResourceUpdate { // Http level api listener configuration. diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 578f879cd0..56b3dbe059 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -49,6 +49,7 @@ import io.grpc.Status.Code; import io.grpc.SynchronizationContext; import io.grpc.internal.GrpcUtil; import io.grpc.internal.ObjectPool; +import io.grpc.xds.AbstractXdsClient.ResourceType; import io.grpc.xds.Bootstrapper.AuthorityInfo; import io.grpc.xds.Bootstrapper.BootstrapInfo; import io.grpc.xds.ClusterSpecifierPlugin.PluginConfig; @@ -195,6 +196,13 @@ final class XdsNameResolver extends NameResolver { replacement = percentEncodePath(replacement); } String ldsResourceName = expandPercentS(listenerNameTemplate, replacement); + if (!XdsClient.isResourceNameValid(ldsResourceName, ResourceType.LDS.typeUrl()) + && !XdsClient.isResourceNameValid(ldsResourceName, ResourceType.LDS.typeUrlV2())) { + listener.onError(Status.INVALID_ARGUMENT.withDescription( + "invalid listener resource URI for service authority: " + serviceAuthority)); + return; + } + ldsResourceName = XdsClient.canonifyResourceName(ldsResourceName); callCounterProvider = SharedCallCounterMap.getInstance(); resolveState = new ResolveState(ldsResourceName); resolveState.start(); diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java index 4916cc726e..c41773dfd9 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java @@ -111,6 +111,7 @@ import io.grpc.lookup.v1.GrpcKeyBuilder.Name; import io.grpc.lookup.v1.NameMatcher; import io.grpc.lookup.v1.RouteLookupClusterSpecifier; import io.grpc.lookup.v1.RouteLookupConfig; +import io.grpc.xds.AbstractXdsClient.ResourceType; import io.grpc.xds.Bootstrapper.ServerInfo; import io.grpc.xds.ClientXdsClient.ResourceInvalidException; import io.grpc.xds.ClientXdsClient.StructOrError; @@ -2573,6 +2574,51 @@ public class ClientXdsClientDataTest { ClientXdsClient.validateUpstreamTlsContext(upstreamTlsContext, null); } + @Test + public void validateResourceName() { + String traditionalResource = "cluster1.google.com"; + assertThat(XdsClient.isResourceNameValid(traditionalResource, ResourceType.CDS.typeUrl())) + .isTrue(); + assertThat(XdsClient.isResourceNameValid(traditionalResource, ResourceType.RDS.typeUrlV2())) + .isTrue(); + + String invalidPath = "xdstp:/abc/efg"; + assertThat(XdsClient.isResourceNameValid(invalidPath, ResourceType.CDS.typeUrl())).isFalse(); + + String invalidPath2 = "xdstp:///envoy.config.route.v3.RouteConfiguration"; + assertThat(XdsClient.isResourceNameValid(invalidPath2, ResourceType.RDS.typeUrl())).isFalse(); + + String typeMatch = "xdstp:///envoy.config.route.v3.RouteConfiguration/foo/route1"; + assertThat(XdsClient.isResourceNameValid(typeMatch, ResourceType.LDS.typeUrl())).isFalse(); + assertThat(XdsClient.isResourceNameValid(typeMatch, ResourceType.RDS.typeUrl())).isTrue(); + assertThat(XdsClient.isResourceNameValid(typeMatch, ResourceType.RDS.typeUrlV2())).isFalse(); + } + + @Test + public void canonifyResourceName() { + String traditionalResource = "cluster1.google.com"; + assertThat(XdsClient.canonifyResourceName(traditionalResource)) + .isEqualTo(traditionalResource); + assertThat(XdsClient.canonifyResourceName(traditionalResource)) + .isEqualTo(traditionalResource); + assertThat(XdsClient.canonifyResourceName(traditionalResource)) + .isEqualTo(traditionalResource); + assertThat(XdsClient.canonifyResourceName(traditionalResource)) + .isEqualTo(traditionalResource); + + String withNoQueries = "xdstp:///envoy.config.route.v3.RouteConfiguration/foo/route1"; + assertThat(XdsClient.canonifyResourceName(withNoQueries)).isEqualTo(withNoQueries); + + String withOneQueries = "xdstp:///envoy.config.route.v3.RouteConfiguration/foo/route1?name=foo"; + assertThat(XdsClient.canonifyResourceName(withOneQueries)).isEqualTo(withOneQueries); + + String withTwoQueries = "xdstp:///envoy.config.route.v3.RouteConfiguration/id/route1?b=1&a=1"; + String expectedCanonifiedName = + "xdstp:///envoy.config.route.v3.RouteConfiguration/id/route1?a=1&b=1"; + assertThat(XdsClient.canonifyResourceName(withTwoQueries)) + .isEqualTo(expectedCanonifiedName); + } + private static Filter buildHttpConnectionManagerFilter(HttpFilter... httpFilters) { return Filter.newBuilder() .setName("envoy.http_connection_manager") diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java index 9e81f45cfc..c0ad0b47c5 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java @@ -760,8 +760,9 @@ public abstract class ClientXdsClientTestBase { @Test public void ldsResourceUpdated_withXdstpResourceName() { - String ldsResourceName = - "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/listener1"; + String ldsResourceName = useProtocolV3() + ? "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/listener1" + : "xdstp://authority.xds.com/envoy.api.v2.Listener/listener1"; DiscoveryRpcCall call = startResourceWatcher(LDS, ldsResourceName, ldsResourceWatcher); assertThat(channelForCustomAuthority).isNotNull(); verifyResourceMetadataRequested(LDS, ldsResourceName); @@ -779,8 +780,9 @@ public abstract class ClientXdsClientTestBase { @Test public void ldsResourceUpdated_withXdstpResourceName_withEmptyAuthority() { - String ldsResourceName = - "xdstp:///envoy.config.listener.v3.Listener/listener1"; + String ldsResourceName = useProtocolV3() + ? "xdstp:///envoy.config.listener.v3.Listener/listener1" + : "xdstp:///envoy.api.v2.Listener/listener1"; DiscoveryRpcCall call = startResourceWatcher(LDS, ldsResourceName, ldsResourceWatcher); assertThat(channelForEmptyAuthority).isNotNull(); verifyResourceMetadataRequested(LDS, ldsResourceName); @@ -796,6 +798,107 @@ public abstract class ClientXdsClientTestBase { LDS, ldsResourceName, testListenerVhosts, VERSION_1, TIME_INCREMENT); } + @Test + public void ldsResourceUpdated_withXdstpResourceName_witUnorderedContextParams() { + String ldsResourceName = useProtocolV3() + ? "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/listener1/a?bar=2&foo=1" + : "xdstp://authority.xds.com/envoy.api.v2.Listener/listener1/a?bar=2&foo=1"; + DiscoveryRpcCall call = startResourceWatcher(LDS, ldsResourceName, ldsResourceWatcher); + assertThat(channelForCustomAuthority).isNotNull(); + + String ldsResourceNameWithUnorderedContextParams = useProtocolV3() + ? "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/listener1/a?foo=1&bar=2" + : "xdstp://authority.xds.com/envoy.api.v2.Listener/listener1/a?foo=1&bar=2"; + Any testListenerVhosts = Any.pack(mf.buildListenerWithApiListener( + ldsResourceNameWithUnorderedContextParams, + mf.buildRouteConfiguration("do not care", mf.buildOpaqueVirtualHosts(VHOST_SIZE)))); + call.sendResponse(LDS, testListenerVhosts, VERSION_1, "0000"); + call.verifyRequest( + LDS, ldsResourceName, VERSION_1, "0000", NODE); + } + + @Test + public void ldsResourceUpdated_withXdstpResourceName_withWrongType() { + String ldsResourceName = useProtocolV3() + ? "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/listener1" + : "xdstp://authority.xds.com/envoy.api.v2.Listener/listener1"; + DiscoveryRpcCall call = startResourceWatcher(LDS, ldsResourceName, ldsResourceWatcher); + assertThat(channelForCustomAuthority).isNotNull(); + + String ldsResourceNameWithWrongType = + "xdstp://authority.xds.com/envoy.config.route.v3.RouteConfiguration/listener1"; + Any testListenerVhosts = Any.pack(mf.buildListenerWithApiListener( + ldsResourceNameWithWrongType, + mf.buildRouteConfiguration("do not care", mf.buildOpaqueVirtualHosts(VHOST_SIZE)))); + call.sendResponse(LDS, testListenerVhosts, VERSION_1, "0000"); + call.verifyRequestNack( + LDS, ldsResourceName, "", "0000", NODE, + ImmutableList.of( + "Unsupported resource name: " + ldsResourceNameWithWrongType + " for type: LDS")); + } + + @Test + public void rdsResourceUpdated_withXdstpResourceName_withWrongType() { + String rdsResourceName = useProtocolV3() + ? "xdstp://authority.xds.com/envoy.config.route.v3.RouteConfiguration/route1" + : "xdstp://authority.xds.com/envoy.api.v2.RouteConfiguration/route1"; + DiscoveryRpcCall call = startResourceWatcher(RDS, rdsResourceName, rdsResourceWatcher); + assertThat(channelForCustomAuthority).isNotNull(); + + String rdsResourceNameWithWrongType = + "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/route1"; + Any testRouteConfig = Any.pack(mf.buildRouteConfiguration( + rdsResourceNameWithWrongType, mf.buildOpaqueVirtualHosts(VHOST_SIZE))); + call.sendResponse(RDS, testRouteConfig, VERSION_1, "0000"); + call.verifyRequestNack( + RDS, rdsResourceName, "", "0000", NODE, + ImmutableList.of( + "Unsupported resource name: " + rdsResourceNameWithWrongType + " for type: RDS")); + } + + @Test + public void cdsResourceUpdated_withXdstpResourceName_withWrongType() { + String cdsResourceName = useProtocolV3() + ? "xdstp://authority.xds.com/envoy.config.cluster.v3.Cluster/cluster1" + : "xdstp://authority.xds.com/envoy.api.v2.Cluster/cluster1"; + DiscoveryRpcCall call = startResourceWatcher(CDS, cdsResourceName, cdsResourceWatcher); + assertThat(channelForCustomAuthority).isNotNull(); + + String cdsResourceNameWithWrongType = + "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/cluster1"; + Any testClusterConfig = Any.pack(mf.buildEdsCluster( + cdsResourceNameWithWrongType, null, "round_robin", null, null, false, null, + "envoy.transport_sockets.tls", null)); + call.sendResponse(CDS, testClusterConfig, VERSION_1, "0000"); + call.verifyRequestNack( + CDS, cdsResourceName, "", "0000", NODE, + ImmutableList.of( + "Unsupported resource name: " + cdsResourceNameWithWrongType + " for type: CDS")); + } + + @Test + public void edsResourceUpdated_withXdstpResourceName_withWrongType() { + String edsResourceName = useProtocolV3() + ? "xdstp://authority.xds.com/envoy.config.endpoint.v3.ClusterLoadAssignment/cluster1" + : "xdstp://authority.xds.com/envoy.api.v2.ClusterLoadAssignment/cluster1"; + DiscoveryRpcCall call = startResourceWatcher(EDS, edsResourceName, edsResourceWatcher); + assertThat(channelForCustomAuthority).isNotNull(); + + String edsResourceNameWithWrongType = + "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/cluster1"; + Any testEdsConfig = Any.pack(mf.buildClusterLoadAssignment( + edsResourceNameWithWrongType, + ImmutableList.of(mf.buildLocalityLbEndpoints( + "region2", "zone2", "subzone2", + mf.buildLbEndpoint("172.44.2.2", 8000, "unknown", 3), 2, 0)), + ImmutableList.of())); + call.sendResponse(EDS, testEdsConfig, VERSION_1, "0000"); + call.verifyRequestNack( + EDS, edsResourceName, "", "0000", NODE, + ImmutableList.of( + "Unsupported resource name: " + edsResourceNameWithWrongType + " for type: EDS")); + } + @Test public void ldsResourceUpdate_withFaultInjection() { Assume.assumeTrue(useProtocolV3()); diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java index 0714d51baa..d42cd07b1b 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverTest.java @@ -270,12 +270,12 @@ public class XdsNameResolverTest { .node(Node.newBuilder().build()) .authorities( ImmutableMap.of(targetAuthority, AuthorityInfo.create( - "xdstp://" + targetAuthority + "/envoy.config.listener.v3.Listener/%s", + "xdstp://" + targetAuthority + "/envoy.config.listener.v3.Listener/%s?foo=1&bar=2", ImmutableList.of(ServerInfo.create( "td.googleapis.com", InsecureChannelCredentials.create(), true))))) .build(); - expectedLdsResourceName = - "xdstp://xds.authority.com/envoy.config.listener.v3.Listener/%5B::FFFF:129.144.52.38%5D:80"; + expectedLdsResourceName = "xdstp://xds.authority.com/envoy.config.listener.v3.Listener/" + + "%5B::FFFF:129.144.52.38%5D:80?bar=2&foo=1"; // query param canonified resolver = new XdsNameResolver( "xds.authority.com", serviceAuthority, serviceConfigParser, syncContext, scheduler, xdsClientPoolFactory, mockRandom, FilterRegistry.getDefaultRegistry(), null);