Support api config source without cluster names (#3030)

This reverts #3018, which reverted #2999. Therefore this PR is similar to #2999, except that
77b292c allows REST/gRPC config mismatch errors to warn instead of throwing an error. Hopefully this solves the issue for users who were unable to roll forward their binaries while keeping the same config.

Changelist for #2999:

To support the deprecation of cluster_names in the api_config_source (#2860), I

added a more verbose error in utility.cc for GRPC api_config_sources with any named clusters
hunted down places in tests where api_config_source.cluster_names()[0] was implicitly used (assuming that API would be GRPC and would have cluster names).
renamed factoryForApiConfigSource to factoryForGrpcApiConfigSource, as it already implicitly returns a Grpc::AsyncClientFactoryPtr, and gave a more verbose error for when the user passes a config with cluster_names set.
separated out checkApiConfigSourceSubscriptionBackingCluster into checkApiConfigSourceNames, which does the gRPC services v. cluster names validation, and - checkApiConfigSourceSubscriptionBackingCluster, which actually validates the cluster name against the clusters map.
more completely covered the tests for the branching cases for non-gRPC API configs which happened to have grpc services defined.
Risk Level: Low

Testing: bazel test test/... all passed.

Fixed #2680
Fixed #2902

Signed-off-by: James Buckland <jbuckland@google.com>

DEPRECATED.md

@@ -10,16 +10,16 @@ The following features have been DEPRECATED and will be removed in the specified
 * Admin mutations should be sent as POSTs rather than GETs. HTTP GETs will result in an error
   status code and will not have their intended effect. Prior to 1.7, GETs can be used for
   admin mutations, but a warning is logged.
+* Rate limit service configuration via the `cluster_name` field is deprecated. Use `grpc_service`
+  instead.
+* gRPC service configuration via the `cluster_names` field in `ApiConfigSource` is deprecated. Use
+  `grpc_services` instead. Prior to 1.7, a warning is logged.
 
 ## Version 1.6.0 (March 20, 2018)
 
 * DOWNSTREAM_ADDRESS log formatter is deprecated. Use DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT
   instead.
 * CLIENT_IP header formatter is deprecated. Use DOWNSTREAM_REMOTE_ADDRESS_WITHOUT_PORT instead.
-* Rate limit service configuration via the `cluster_name` field is deprecated. Use `grpc_service`
-  instead.
-* gRPC service configuration via the `cluster_names` field in `ApiConfigSource` is deprecated. Use
-  `grpc_services` instead.
 * 'use_original_dst' field in the v2 LDS API is deprecated. Use listener filters and filter chain
   matching instead.
 * `value` and `regex` fields in the `HeaderMatcher` message is deprecated. Use the `exact_match`

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,16 +78,43 @@ 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 (api_config_source.cluster_names().size() == 0 &&
+      api_config_source.grpc_services().size() == 0) {
+    throw EnvoyException("API configs must have either a gRPC service or a cluster name defined");
   }
 
-  const auto& cluster_name = api_config_source.cluster_names()[0];
+  if (is_grpc) {
+    if (api_config_source.cluster_names().size() != 0) {
+      ENVOY_LOG_MISC(warn, "Setting a cluster name for API config source type "
+                           "envoy::api::v2::core::ConfigSource::GRPC is deprecated");
+    }
+    if (api_config_source.cluster_names().size() > 1) {
+      throw EnvoyException(
+          "envoy::api::v2::core::ConfigSource must have a singleton 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");
+    }
+  }
+}
+
+void Utility::validateClusterName(const Upstream::ClusterManager::ClusterInfoMap& clusters,
+                                  const std::string& 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) {
@@ -98,6 +125,31 @@ void Utility::checkApiConfigSourceSubscriptionBackingCluster(
   }
 }
 
+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);
+
+  if (!api_config_source.cluster_names().empty()) {
+    // All API configs of type REST and REST_LEGACY should have cluster names.
+    // Additionally, some gRPC API configs might have a cluster name set instead
+    // of an envoy gRPC.
+    Utility::validateClusterName(clusters, api_config_source.cluster_names()[0]);
+  } else 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()) {
+      // If an Envoy gRPC exists, we take its cluster name.
+      Utility::validateClusterName(
+          clusters, api_config_source.grpc_services()[0].envoy_grpc().cluster_name());
+    }
+  }
+  // Otherwise, there is no cluster name to validate.
+}
+
 std::chrono::milliseconds Utility::apiConfigSourceRefreshDelay(
     const envoy::api::v2::core::ApiConfigSource& api_config_source) {
   if (!api_config_source.has_refresh_delay()) {
@@ -161,36 +213,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,10 +121,29 @@ 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.
    * @param api_config_source the config source to validate.
+   * @throws EnvoyException when an API config doesn't have a statically defined non-EDS cluster.
+   */
+  static void validateClusterName(const Upstream::ClusterManager::ClusterInfoMap& clusters,
+                                  const std::string& cluster_name);
+
+  /**
+   * Potentially calls Utility::validateClusterName, if a cluster name can be found.
+   * @param clusters the clusters currently loaded in the cluster manager.
+   * @param api_config_source the config source to validate.
+   * @throws EnvoyException when an API config doesn't have a statically defined non-EDS cluster.
    */
   static void checkApiConfigSourceSubscriptionBackingCluster(
       const Upstream::ClusterManager::ClusterInfoMap& clusters,
@@ -240,9 +259,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));
   }
 }