Revert "Support api config source without cluster names (envoyproxy#2999

)" (envoyproxy#3018) This reverts commit 01e75f9. Discuession on #envoy-users revealed that this is going to break operationally for folks who are trying to roll forward binaries with the same config, there's no config that can work both before/after. Signed-off-by: Harvey Tuch <[email protected]>
hecsalazar · Apr 6, 2018 · fdcbe10 · fdcbe10
1 parent 8b479b1
commit fdcbe10
Show file tree

Hide file tree

Showing 8 changed files with 113 additions and 270 deletions.
diff --git a/source/common/config/subscription_factory.h b/source/common/config/subscription_factory.h
@@ -52,21 +52,22 @@ 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, api_config_source.cluster_names()[0], dispatcher, random,
+            node, cm, cluster_name, 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::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(),
-                                                           config.api_config_source(), scope)
+            Config::Utility::factoryForApiConfigSource(cm.grpcAsyncClientManager(),
+                                                       config.api_config_source(), scope)
                 ->create(),
             dispatcher, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method),
             stats));

diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc
@@ -78,67 +78,23 @@ void Utility::checkFilesystemSubscriptionBackingPath(const std::string& path) {
   }
 }
 
-void Utility::checkApiConfigSourceNames(
-    const envoy::api::v2::core::ApiConfigSource& api_config_source) {
-  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");
-    }
-  }
-}
-
 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));
-    }
+  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 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));
   }
 }
 
@@ -205,13 +161,36 @@ void Utility::checkObjNameLength(const std::string& error_prefix, const std::str
   }
 }
 
-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);
+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));
+  }
 
-  return async_client_manager.factoryForGrpcService(api_config_source.grpc_services(0), scope,
-                                                    false);
+  return async_client_manager.factoryForGrpcService(grpc_service, scope, false);
 }
 
 } // namespace Config

diff --git a/source/common/config/utility.h b/source/common/config/utility.h
@@ -121,15 +121,6 @@ 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.
@@ -249,9 +240,9 @@ class Utility {
    * @return Grpc::AsyncClientFactoryPtr gRPC async client factory.
    */
   static Grpc::AsyncClientFactoryPtr
-  factoryForGrpcApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
-                                const envoy::api::v2::core::ApiConfigSource& api_config_source,
-                                Stats::Scope& scope);
+  factoryForApiConfigSource(Grpc::AsyncClientManager& async_client_manager,
+                            const envoy::api::v2::core::ApiConfigSource& api_config_source,
+                            Stats::Scope& scope);
 };
 
 } // namespace Config

diff --git a/source/common/upstream/cluster_manager_impl.cc b/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::factoryForGrpcApiConfigSource(
+        Config::Utility::factoryForApiConfigSource(
             *async_client_manager_, bootstrap.dynamic_resources().ads_config(), stats)
             ->create(),
         main_thread_dispatcher,
@@ -291,12 +291,11 @@ 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::factoryForGrpcApiConfigSource(
-                                  *async_client_manager_, load_stats_config, stats)
-                                  ->create(),
-                              main_thread_dispatcher));
+    load_stats_reporter_.reset(new LoadStatsReporter(
+        bootstrap.node(), *this, stats,
+        Config::Utility::factoryForApiConfigSource(*async_client_manager_, load_stats_config, stats)
+            ->create(),
+        main_thread_dispatcher));
   }
 }