xds: Fix AbstractXdsClient fromTypeUrl to use subscribedResources instead of hardcoded (#9607)

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

@@ -19,14 +19,6 @@ package io.grpc.xds;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
-import static io.grpc.xds.XdsClusterResource.ADS_TYPE_URL_CDS;
-import static io.grpc.xds.XdsClusterResource.ADS_TYPE_URL_CDS_V2;
-import static io.grpc.xds.XdsEndpointResource.ADS_TYPE_URL_EDS;
-import static io.grpc.xds.XdsEndpointResource.ADS_TYPE_URL_EDS_V2;
-import static io.grpc.xds.XdsListenerResource.ADS_TYPE_URL_LDS;
-import static io.grpc.xds.XdsListenerResource.ADS_TYPE_URL_LDS_V2;
-import static io.grpc.xds.XdsRouteConfigureResource.ADS_TYPE_URL_RDS;
-import static io.grpc.xds.XdsRouteConfigureResource.ADS_TYPE_URL_RDS_V2;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Stopwatch;
@@ -54,8 +46,10 @@ import io.grpc.xds.XdsLogger.XdsLogLevel;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
@@ -235,7 +229,9 @@ final class AbstractXdsClient {
         return;
       }
       startRpcStream();
-      for (XdsResourceType<?> type : resourceStore.getXdsResourceTypes()) {
+      Set<XdsResourceType<?>> subscribedResourceTypes =
+          new HashSet<>(resourceStore.getSubscribedResourceTypesWithTypeUrl().values());
+      for (XdsResourceType<?> type : subscribedResourceTypes) {
         Collection<String> resources = resourceStore.getSubscribedResources(serverInfo, type);
         if (resources != null) {
           adsStream.sendDiscoveryRequest(type, resources);
@@ -247,27 +243,8 @@ final class AbstractXdsClient {
 
   @VisibleForTesting
   @Nullable
-  static XdsResourceType<?> fromTypeUrl(String typeUrl) {
+  XdsResourceType<?> fromTypeUrl(String typeUrl) {
-    switch (typeUrl) {
+    return resourceStore.getSubscribedResourceTypesWithTypeUrl().get(typeUrl);
-      case ADS_TYPE_URL_LDS:
-        // fall trough
-      case ADS_TYPE_URL_LDS_V2:
-        return XdsListenerResource.getInstance();
-      case ADS_TYPE_URL_RDS:
-        // fall through
-      case ADS_TYPE_URL_RDS_V2:
-        return XdsRouteConfigureResource.getInstance();
-      case ADS_TYPE_URL_CDS:
-        // fall through
-      case ADS_TYPE_URL_CDS_V2:
-        return XdsClusterResource.getInstance();
-      case ADS_TYPE_URL_EDS:
-        // fall through
-      case ADS_TYPE_URL_EDS_V2:
-        return XdsEndpointResource.getInstance();
-      default:
-        return null;
-    }
   }
 
   private abstract class AbstractAdsStream {

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

@@ -371,6 +371,6 @@ abstract class XdsClient {
     Collection<String> getSubscribedResources(ServerInfo serverInfo,
                                               XdsResourceType<? extends ResourceUpdate> type);
 
-    Collection<XdsResourceType<? extends ResourceUpdate>> getXdsResourceTypes();
+    Map<String, XdsResourceType<?>> getSubscribedResourceTypesWithTypeUrl();
   }
 }

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

@@ -91,6 +91,7 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou
   private final Map<XdsResourceType<? extends ResourceUpdate>,
       Map<String, ResourceSubscriber<? extends ResourceUpdate>>>
       resourceSubscribers = new HashMap<>();
+  private final Map<String, XdsResourceType<?>> subscribedResourceTypeUrls = new HashMap<>();
   private final LoadStatsManager2 loadStatsManager;
   private final Map<ServerInfo, LoadReportClient> serverLrsClientMap = new HashMap<>();
   private final XdsChannelFactory xdsChannelFactory;
@@ -228,8 +229,8 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou
   }
 
   @Override
-  public Collection<XdsResourceType<? extends ResourceUpdate>> getXdsResourceTypes() {
+  public Map<String, XdsResourceType<?>> getSubscribedResourceTypesWithTypeUrl() {
-    return resourceSubscribers.keySet();
+    return Collections.unmodifiableMap(subscribedResourceTypeUrls);
   }
 
   @Nullable
@@ -289,6 +290,8 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou
       public void run() {
         if (!resourceSubscribers.containsKey(type)) {
           resourceSubscribers.put(type, new HashMap<>());
+          subscribedResourceTypeUrls.put(type.typeUrl(), type);
+          subscribedResourceTypeUrls.put(type.typeUrlV2(), type);
         }
         ResourceSubscriber<T> subscriber =
             (ResourceSubscriber<T>) resourceSubscribers.get(type).get(resourceName);;
@@ -319,6 +322,8 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou
         if (!subscriber.isWatched()) {
           subscriber.cancelResourceWatch();
           resourceSubscribers.get(type).remove(resourceName);
+          subscribedResourceTypeUrls.remove(type.typeUrl());
+          subscribedResourceTypeUrls.remove(type.typeUrlV2());
           if (subscriber.xdsChannel != null) {
             subscriber.xdsChannel.adjustResourceSubscription(type);
           }

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

@@ -50,6 +50,7 @@ import io.grpc.xds.Bootstrapper.ServerInfo;
 import io.grpc.xds.XdsClient.ResourceMetadata;
 import io.grpc.xds.XdsClient.ResourceMetadata.ResourceMetadataStatus;
 import io.grpc.xds.XdsNameResolverProvider.XdsClientPoolFactory;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -75,7 +76,7 @@ public class CsdsServiceTest {
           ServerInfo.create(SERVER_URI, InsecureChannelCredentials.create(), true)))
       .node(BOOTSTRAP_NODE)
       .build();
-  private static final XdsClient XDS_CLIENT_NO_RESOURCES = new FakeXdsClient();
+  private static final FakeXdsClient XDS_CLIENT_NO_RESOURCES = new FakeXdsClient();
   private static final XdsResourceType<?> LDS = XdsListenerResource.getInstance();
   private static final XdsResourceType<?> CDS = XdsClusterResource.getInstance();
   private static final XdsResourceType<?> RDS = XdsRouteConfigureResource.getInstance();
@@ -263,7 +264,7 @@ public class CsdsServiceTest {
       assertThat(response.getConfigCount()).isEqualTo(1);
       ClientConfig clientConfig = response.getConfig(0);
       verifyClientConfigNode(clientConfig);
-      verifyClientConfigNoResources(clientConfig);
+      verifyClientConfigNoResources(XDS_CLIENT_NO_RESOURCES, clientConfig);
     }
 
     private void verifyRequestInvalidResponseStatus(Status status) {
@@ -320,7 +321,7 @@ public class CsdsServiceTest {
     @Test
     public void getClientConfigForXdsClient_subscribedResourcesToGenericXdsConfig()
         throws InterruptedException {
-      ClientConfig clientConfig = CsdsService.getClientConfigForXdsClient(new FakeXdsClient() {
+      FakeXdsClient fakeXdsClient = new FakeXdsClient() {
         @Override
         protected Map<XdsResourceType<?>, Map<String, ResourceMetadata>>
             getSubscribedResourcesMetadata() {
@@ -331,7 +332,18 @@ public class CsdsServiceTest {
               .put(EDS, ImmutableMap.of("subscribedResourceName.EDS", METADATA_ACKED_EDS))
               .buildOrThrow();
         }
-      });
+
+        @Override
+        public Map<String, XdsResourceType<?>> getSubscribedResourceTypesWithTypeUrl() {
+          return ImmutableMap.of(
+              LDS.typeUrl(), LDS,
+              RDS.typeUrl(), RDS,
+              CDS.typeUrl(), CDS,
+              EDS.typeUrl(), EDS
+          );
+        }
+      };
+      ClientConfig clientConfig = CsdsService.getClientConfigForXdsClient(fakeXdsClient);
 
       verifyClientConfigNode(clientConfig);
 
@@ -339,7 +351,8 @@ public class CsdsServiceTest {
       // is propagated to the correct resource types.
       int xdsConfigCount = clientConfig.getGenericXdsConfigsCount();
       assertThat(xdsConfigCount).isEqualTo(4);
-      Map<XdsResourceType<?>, GenericXdsConfig> configDumps = mapConfigDumps(clientConfig);
+      Map<XdsResourceType<?>, GenericXdsConfig> configDumps = mapConfigDumps(fakeXdsClient,
+          clientConfig);
       assertThat(configDumps.keySet()).containsExactly(LDS, RDS, CDS, EDS);
 
       // LDS.
@@ -372,7 +385,7 @@ public class CsdsServiceTest {
     public void getClientConfigForXdsClient_noSubscribedResources() throws InterruptedException {
       ClientConfig clientConfig = CsdsService.getClientConfigForXdsClient(XDS_CLIENT_NO_RESOURCES);
       verifyClientConfigNode(clientConfig);
-      verifyClientConfigNoResources(clientConfig);
+      verifyClientConfigNoResources(XDS_CLIENT_NO_RESOURCES, clientConfig);
     }
   }
 
@@ -380,10 +393,11 @@ public class CsdsServiceTest {
    * Assuming {@link MetadataToProtoTests} passes, and metadata converted to corresponding
    * config dumps correctly, perform a minimal verification of the general shape of ClientConfig.
    */
-  private static void verifyClientConfigNoResources(ClientConfig clientConfig) {
+  private static void verifyClientConfigNoResources(FakeXdsClient xdsClient,
+                                                    ClientConfig clientConfig) {
     int xdsConfigCount = clientConfig.getGenericXdsConfigsCount();
     assertThat(xdsConfigCount).isEqualTo(0);
-    Map<XdsResourceType<?>, GenericXdsConfig> configDumps = mapConfigDumps(clientConfig);
+    Map<XdsResourceType<?>, GenericXdsConfig> configDumps = mapConfigDumps(xdsClient, clientConfig);
     assertThat(configDumps).isEmpty();
   }
 
@@ -397,11 +411,13 @@ public class CsdsServiceTest {
     assertThat(node).isEqualTo(BOOTSTRAP_NODE.toEnvoyProtoNode());
   }
 
-  private static Map<XdsResourceType<?>, GenericXdsConfig> mapConfigDumps(ClientConfig config) {
+  private static Map<XdsResourceType<?>, GenericXdsConfig> mapConfigDumps(FakeXdsClient client,
+                                                                          ClientConfig config) {
     Map<XdsResourceType<?>, GenericXdsConfig> xdsConfigMap = new HashMap<>();
     List<GenericXdsConfig> xdsConfigList = config.getGenericXdsConfigsList();
     for (GenericXdsConfig genericXdsConfig : xdsConfigList) {
-      XdsResourceType<?> type = AbstractXdsClient.fromTypeUrl(genericXdsConfig.getTypeUrl());
+      XdsResourceType<?> type = client.getSubscribedResourceTypesWithTypeUrl()
+          .get(genericXdsConfig.getTypeUrl());
       assertThat(type).isNotNull();
       assertThat(xdsConfigMap).doesNotContainKey(type);
       xdsConfigMap.put(type, genericXdsConfig);
@@ -409,7 +425,7 @@ public class CsdsServiceTest {
     return xdsConfigMap;
   }
 
-  private static class FakeXdsClient extends XdsClient {
+  private static class FakeXdsClient extends XdsClient implements XdsClient.ResourceStore {
     protected Map<XdsResourceType<?>, Map<String, ResourceMetadata>>
         getSubscribedResourcesMetadata() {
       return ImmutableMap.of();
@@ -425,6 +441,18 @@ public class CsdsServiceTest {
     BootstrapInfo getBootstrapInfo() {
       return BOOTSTRAP_INFO;
     }
+
+    @Nullable
+    @Override
+    public Collection<String> getSubscribedResources(ServerInfo serverInfo,
+                  XdsResourceType<? extends ResourceUpdate> type) {
+      return null;
+    }
+
+    @Override
+    public Map<String, XdsResourceType<?>> getSubscribedResourceTypesWithTypeUrl() {
+      return ImmutableMap.of();
+    }
   }
 
   private static class FakeXdsClientPoolFactory implements XdsClientPoolFactory {

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

@@ -400,19 +400,26 @@ public abstract class XdsClientImplTestBase {
       int ldsSize, int cdsSize, int rdsSize, int edsSize) {
     Map<XdsResourceType<?>, Map<String, ResourceMetadata>> subscribedResourcesMetadata =
         awaitSubscribedResourcesMetadata();
-    verifyResourceCount(subscribedResourcesMetadata, LDS, ldsSize);
+    Map<String, XdsResourceType<?>> subscribedTypeUrls =
-    verifyResourceCount(subscribedResourcesMetadata, CDS, cdsSize);
+        xdsClient.getSubscribedResourceTypesWithTypeUrl();
-    verifyResourceCount(subscribedResourcesMetadata, RDS, rdsSize);
+    verifyResourceCount(subscribedTypeUrls, subscribedResourcesMetadata, LDS, ldsSize);
-    verifyResourceCount(subscribedResourcesMetadata, EDS, edsSize);
+    verifyResourceCount(subscribedTypeUrls, subscribedResourcesMetadata, CDS, cdsSize);
+    verifyResourceCount(subscribedTypeUrls, subscribedResourcesMetadata, RDS, rdsSize);
+    verifyResourceCount(subscribedTypeUrls, subscribedResourcesMetadata, EDS, edsSize);
   }
 
   private void verifyResourceCount(
+      Map<String, XdsResourceType<?>> subscribedTypeUrls,
       Map<XdsResourceType<?>, Map<String, ResourceMetadata>> subscribedResourcesMetadata,
       XdsResourceType<?> type,
       int size) {
     if (size == 0) {
+      assertThat(subscribedTypeUrls.containsKey(type.typeUrl())).isFalse();
+      assertThat(subscribedTypeUrls.containsKey(type.typeUrlV2())).isFalse();
       assertThat(subscribedResourcesMetadata.containsKey(type)).isFalse();
     } else {
+      assertThat(subscribedTypeUrls.containsKey(type.typeUrl())).isTrue();
+      assertThat(subscribedTypeUrls.containsKey(type.typeUrlV2())).isTrue();
       assertThat(subscribedResourcesMetadata.get(type)).hasSize(size);
     }
   }