Support api config source without cluster names

source/common/config/subscription_factory.h

@@ -52,22 +52,21 @@ class SubscriptionFactory {
     case envoy::api::v2::core::ConfigSource::kApiConfigSource: {
       const envoy::api::v2::core::ApiConfigSource& api_config_source = config.api_config_source();
       Utility::checkApiConfigSourceSubscriptionBackingCluster(cm.clusters(), api_config_source);
-      const std::string& cluster_name = api_config_source.cluster_names()[0];
       switch (api_config_source.api_type()) {
       case envoy::api::v2::core::ApiConfigSource::REST_LEGACY:
         result.reset(rest_legacy_constructor());
         break;
       case envoy::api::v2::core::ApiConfigSource::REST:
         result.reset(new HttpSubscriptionImpl<ResourceType>(
-            node, cm, cluster_name, dispatcher, random,
+            node, cm, api_config_source.cluster_names()[0], dispatcher, random,
             Utility::apiConfigSourceRefreshDelay(api_config_source),
             *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(rest_method), stats));
         break;
       case envoy::api::v2::core::ApiConfigSource::GRPC: {
         result.reset(new GrpcSubscriptionImpl<ResourceType>(
             node,
-            Config::Utility::factoryForApiConfigSource(cm.grpcAsyncClientManager(),
-                                                       config.api_config_source(), scope)
+            Config::Utility::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(),
+                                                           config.api_config_source(), scope)
                 ->create(),
             dispatcher, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method),
             stats));

source/common/config/utility.cc

@@ -78,23 +78,67 @@ void Utility::checkFilesystemSubscriptionBackingPath(const std::string& path) {
   }
 }
 
-void Utility::checkApiConfigSourceSubscriptionBackingCluster(
-    const Upstream::ClusterManager::ClusterInfoMap& clusters,
+void Utility::checkApiConfigSourceNames(
     const envoy::api::v2::core::ApiConfigSource& api_config_source) {
-  if (api_config_source.cluster_names().size() != 1) {
-    // TODO(htuch): Add support for multiple clusters, #1170.
-    throw EnvoyException(
-        "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
+  const bool is_grpc =
+      (api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
+
+  if (is_grpc) {
+    if (api_config_source.cluster_names().size() != 0) {
+      throw EnvoyException(
+          "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified");
+    }
+    if (api_config_source.grpc_services().size() != 1) {
+      throw EnvoyException(
+          "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified");
+    }
+  } else {
+    if (api_config_source.grpc_services().size() != 0) {
+      throw EnvoyException("envoy::api::v2::core::ConfigSource, if not of type gRPC, must not have "
+                           "a gRPC service specified");
+    }
+    if (api_config_source.cluster_names().size() != 1) {
+      throw EnvoyException(
+          "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
+    }
   }
+}
 
-  const auto& cluster_name = api_config_source.cluster_names()[0];
-  const auto& it = clusters.find(cluster_name);
-  if (it == clusters.end() || it->second.get().info()->addedViaApi() ||
-      it->second.get().info()->type() == envoy::api::v2::Cluster::EDS) {
-    throw EnvoyException(fmt::format(
-        "envoy::api::v2::core::ConfigSource must have a statically "
-        "defined non-EDS cluster: '{}' does not exist, was added via api, or is an EDS cluster",
-        cluster_name));
+void Utility::checkApiConfigSourceSubscriptionBackingCluster(
+    const Upstream::ClusterManager::ClusterInfoMap& clusters,
+    const envoy::api::v2::core::ApiConfigSource& api_config_source) {
+  Utility::checkApiConfigSourceNames(api_config_source);
+
+  const bool is_grpc =
+      (api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
+
+  // We ought to validate the cluster name if and only if there is a cluster name.
+  if (is_grpc) {
+    // Some ApiConfigSources of type GRPC won't have a cluster name, such as if
+    // they've been configured with google_grpc.
+    if (api_config_source.grpc_services()[0].has_envoy_grpc()) {
+      const std::string& cluster_name =
+          api_config_source.grpc_services()[0].envoy_grpc().cluster_name();
+      const auto& it = clusters.find(cluster_name);
+      if (it == clusters.end() || it->second.get().info()->addedViaApi() ||
+          it->second.get().info()->type() == envoy::api::v2::Cluster::EDS) {
+        throw EnvoyException(fmt::format(
+            "envoy::api::v2::core::ConfigSource must have a statically "
+            "defined non-EDS cluster: '{}' does not exist, was added via api, or is an EDS cluster",
+            cluster_name));
+      }
+    }
+  } else {
+    // All ApiConfigSources of type REST and REST_LEGACY should have cluster_names.
+    const std::string& cluster_name = api_config_source.cluster_names()[0];
+    const auto& it = clusters.find(cluster_name);
+    if (it == clusters.end() || it->second.get().info()->addedViaApi() ||
+        it->second.get().info()->type() == envoy::api::v2::Cluster::EDS) {
+      throw EnvoyException(fmt::format(
+          "envoy::api::v2::core::ConfigSource must have a statically "
+          "defined non-EDS cluster: '{}' does not exist, was added via api, or is an EDS cluster",
+          cluster_name));
+    }
   }
 }
 
@@ -161,36 +205,13 @@ void Utility::checkObjNameLength(const std::string& error_prefix, const std::str
   }
 }
 
-Grpc::AsyncClientFactoryPtr
-Utility::factoryForApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
-                                   const envoy::api::v2::core::ApiConfigSource& api_config_source,
-                                   Stats::Scope& scope) {
-  ASSERT(api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
-  envoy::api::v2::core::GrpcService grpc_service;
-  if (api_config_source.cluster_names().empty()) {
-    if (api_config_source.grpc_services().empty()) {
-      throw EnvoyException(
-          fmt::format("Missing gRPC services in envoy::api::v2::core::ApiConfigSource: {}",
-                      api_config_source.DebugString()));
-    }
-    // TODO(htuch): Implement multiple gRPC services.
-    if (api_config_source.grpc_services().size() != 1) {
-      throw EnvoyException(fmt::format("Only singleton gRPC service lists supported in "
-                                       "envoy::api::v2::core::ApiConfigSource: {}",
-                                       api_config_source.DebugString()));
-    }
-    grpc_service.MergeFrom(api_config_source.grpc_services(0));
-  } else {
-    // TODO(htuch): cluster_names is deprecated, remove after 1.6.0.
-    if (api_config_source.cluster_names().size() != 1) {
-      throw EnvoyException(fmt::format("Only singleton cluster name lists supported in "
-                                       "envoy::api::v2::core::ApiConfigSource: {}",
-                                       api_config_source.DebugString()));
-    }
-    grpc_service.mutable_envoy_grpc()->set_cluster_name(api_config_source.cluster_names(0));
-  }
+Grpc::AsyncClientFactoryPtr Utility::factoryForGrpcApiConfigSource(
+    Grpc::AsyncClientManager& async_client_manager,
+    const envoy::api::v2::core::ApiConfigSource& api_config_source, Stats::Scope& scope) {
+  Utility::checkApiConfigSourceNames(api_config_source);
 
-  return async_client_manager.factoryForGrpcService(grpc_service, scope, false);
+  return async_client_manager.factoryForGrpcService(api_config_source.grpc_services(0), scope,
+                                                    false);
 }
 
 } // namespace Config

source/common/config/utility.h

@@ -121,6 +121,15 @@ class Utility {
    */
   static void checkFilesystemSubscriptionBackingPath(const std::string& path);
 
+  /**
+   * Check the grpc_services and cluster_names for API config sanity. Throws on error.
+   * @param api_config_source the config source to validate.
+   * @throws EnvoyException when an API config has the wrong number of gRPC
+   * services or cluster names, depending on expectations set by its API type.
+   */
+  static void
+  checkApiConfigSourceNames(const envoy::api::v2::core::ApiConfigSource& api_config_source);
+
   /**
    * Check the validity of a cluster backing an api config source. Throws on error.
    * @param clusters the clusters currently loaded in the cluster manager.
@@ -243,9 +252,9 @@ class Utility {
    * @return Grpc::AsyncClientFactoryPtr gRPC async client factory.
    */
   static Grpc::AsyncClientFactoryPtr
-  factoryForApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
-                            const envoy::api::v2::core::ApiConfigSource& api_config_source,
-                            Stats::Scope& scope);
+  factoryForGrpcApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
+                                const envoy::api::v2::core::ApiConfigSource& api_config_source,
+                                Stats::Scope& scope);
 };
 
 } // namespace Config

source/common/upstream/cluster_manager_impl.cc

@@ -204,7 +204,7 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
   if (bootstrap.dynamic_resources().has_ads_config()) {
     ads_mux_.reset(new Config::GrpcMuxImpl(
         bootstrap.node(),
-        Config::Utility::factoryForApiConfigSource(
+        Config::Utility::factoryForGrpcApiConfigSource(
             *async_client_manager_, bootstrap.dynamic_resources().ads_config(), stats)
             ->create(),
         main_thread_dispatcher,
@@ -291,11 +291,12 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
 
   if (cm_config.has_load_stats_config()) {
     const auto& load_stats_config = cm_config.load_stats_config();
-    load_stats_reporter_.reset(new LoadStatsReporter(
-        bootstrap.node(), *this, stats,
-        Config::Utility::factoryForApiConfigSource(*async_client_manager_, load_stats_config, stats)
-            ->create(),
-        main_thread_dispatcher));
+    load_stats_reporter_.reset(
+        new LoadStatsReporter(bootstrap.node(), *this, stats,
+                              Config::Utility::factoryForGrpcApiConfigSource(
+                                  *async_client_manager_, load_stats_config, stats)
+                                  ->create(),
+                              main_thread_dispatcher));
   }
 }