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.

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,

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<String> 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<String> queries = ampSplitter.splitToList(rawQuery);
+    if (queries.size() < 2) {
+      return resourceName;
+    }
+    List<String> 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.

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();

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")

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

@@ -760,8 +760,9 @@ public abstract class ClientXdsClientTestBase {
 
   @Test
   public void ldsResourceUpdated_withXdstpResourceName() {
-    String ldsResourceName =
+    String ldsResourceName = useProtocolV3()
-        "xdstp://authority.xds.com/envoy.config.listener.v3.Listener/listener1";
+        ? "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 =
+    String ldsResourceName = useProtocolV3()
-        "xdstp:///envoy.config.listener.v3.Listener/listener1";
+        ? "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());

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.<ServerInfo>of(ServerInfo.create(
                     "td.googleapis.com", InsecureChannelCredentials.create(), true)))))
         .build();
-    expectedLdsResourceName =
+    expectedLdsResourceName = "xdstp://xds.authority.com/envoy.config.listener.v3.Listener/"
-        "xdstp://xds.authority.com/envoy.config.listener.v3.Listener/%5B::FFFF:129.144.52.38%5D:80";
+        + "%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);