From ec57fdfd6ad7e42139d338e1905d0f49db610a2e Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Sun, 29 Mar 2020 15:24:18 +0530 Subject: [PATCH 1/2] use cluster check utility in grpc async client Signed-off-by: Rama Chavali --- source/common/grpc/BUILD | 1 + source/common/grpc/async_client_manager_impl.cc | 13 +++---------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 29f31e66d4445..fb5c1801ea6ad 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -49,6 +49,7 @@ envoy_cc_library( "//include/envoy/singleton:manager_interface", "//include/envoy/thread_local:thread_local_interface", "//include/envoy/upstream:cluster_manager_interface", + "//source/common/config:utility_lib", ] + envoy_select_google_grpc([":google_async_client_lib"]), ) diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc index dc039473395cc..936ce6320d8f1 100644 --- a/source/common/grpc/async_client_manager_impl.cc +++ b/source/common/grpc/async_client_manager_impl.cc @@ -3,6 +3,7 @@ #include "envoy/config/core/v3/grpc_service.pb.h" #include "envoy/stats/scope.h" +#include "common/config/utility.h" #include "common/grpc/async_client_impl.h" #ifdef ENVOY_GOOGLE_GRPC @@ -19,16 +20,8 @@ AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm, if (skip_cluster_check) { return; } - - const std::string& cluster_name = config.envoy_grpc().cluster_name(); - auto clusters = cm_.clusters(); - const auto& it = clusters.find(cluster_name); - if (it == clusters.end()) { - throw EnvoyException(fmt::format("Unknown gRPC client cluster '{}'", cluster_name)); - } - if (it->second.get().info()->addedViaApi()) { - throw EnvoyException(fmt::format("gRPC client cluster '{}' is not static", cluster_name)); - } + Config::Utility::checkCluster("AsyncClientFactory", config.envoy_grpc().cluster_name(), cm_, + false); } AsyncClientManagerImpl::AsyncClientManagerImpl(Upstream::ClusterManager& cm, From a1802bff6b138695b7fbddc6eecf332eaa2db42f Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Mon, 30 Mar 2020 15:35:46 +0530 Subject: [PATCH 2/2] add tests Signed-off-by: Rama Chavali --- source/common/config/utility.cc | 8 ++--- test/common/config/utility_test.cc | 32 +++++++++++++++++++ .../grpc/async_client_manager_impl_test.cc | 5 +-- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 7acd462ce677a..44a901cdf7032 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -61,12 +61,12 @@ void Utility::translateApiConfigSource( void Utility::checkCluster(absl::string_view error_prefix, absl::string_view cluster_name, Upstream::ClusterManager& cm, bool allow_added_via_api) { - Upstream::ThreadLocalCluster* cluster = cm.get(cluster_name); - if (cluster == nullptr) { + auto clusters = cm.clusters(); + const auto& it = clusters.find(std::string(cluster_name)); + if (it == clusters.end()) { throw EnvoyException(fmt::format("{}: unknown cluster '{}'", error_prefix, cluster_name)); } - - if (!allow_added_via_api && cluster->info()->addedViaApi()) { + if (!allow_added_via_api && it->second.get().info()->addedViaApi()) { throw EnvoyException(fmt::format("{}: invalid cluster '{}': currently only " "static (non-CDS) clusters are supported", error_prefix, cluster_name)); diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index 9b3f7bf02cbd3..7fe678ec3cbc4 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -514,6 +514,38 @@ TEST(UtilityTest, EmptyToEmptyConfig) { EXPECT_THAT(out, ProtoEq(envoy::extensions::filters::http::cors::v3::Cors())); } +// Validates CheckCluster functionality. +TEST(UtilityTest, CheckCluster) { + Upstream::MockClusterManager cm; + Upstream::ClusterManager::ClusterInfoMap cluster_map; + + // Validate that proper error is thrown, when cluster is not available. + EXPECT_CALL(cm, clusters()).WillOnce(Return(cluster_map)); + EXPECT_THROW_WITH_MESSAGE(Utility::checkCluster("prefix", "foo", cm, false), EnvoyException, + "prefix: unknown cluster 'foo'"); + + // Validate that proper error is thrown, when dynamic cluster is passed when it is not expected. + Upstream::MockClusterMockPrioritySet api_cluster; + cluster_map.emplace("foo", api_cluster); + EXPECT_CALL(cm, clusters()).Times(2).WillRepeatedly(Return(cluster_map)); + EXPECT_CALL(api_cluster, info()); + EXPECT_CALL(*api_cluster.info_, addedViaApi()).WillOnce(Return(true)); + EXPECT_THROW_WITH_MESSAGE(Utility::checkCluster("prefix", "foo", cm, false), EnvoyException, + "prefix: invalid cluster 'foo': currently only " + "static (non-CDS) clusters are supported"); + EXPECT_NO_THROW(Utility::checkCluster("prefix", "foo", cm, true)); + + // Validate that bootstrap cluster does not throw any exceptions. + cluster_map.clear(); + Upstream::MockClusterMockPrioritySet bootstrap_cluster; + cluster_map.emplace("foo", bootstrap_cluster); + EXPECT_CALL(cm, clusters()).Times(2).WillRepeatedly(Return(cluster_map)); + EXPECT_CALL(bootstrap_cluster, info()); + EXPECT_CALL(*bootstrap_cluster.info_, addedViaApi()).WillOnce(Return(false)); + EXPECT_NO_THROW(Utility::checkCluster("prefix", "foo", cm, true)); + EXPECT_NO_THROW(Utility::checkCluster("prefix", "foo", cm, false)); +} + TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, GrpcClusterTestAcrossTypes) { envoy::config::core::v3::ConfigSource config; auto* api_config_source = config.mutable_api_config_source(); diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc index ef09861d0da00..250be8b9c8c00 100644 --- a/test/common/grpc/async_client_manager_impl_test.cc +++ b/test/common/grpc/async_client_manager_impl_test.cc @@ -54,7 +54,7 @@ TEST_F(AsyncClientManagerImplTest, EnvoyGrpcUnknown) { EXPECT_CALL(cm_, clusters()); EXPECT_THROW_WITH_MESSAGE( async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, - "Unknown gRPC client cluster 'foo'"); + "AsyncClientFactory: unknown cluster 'foo'"); } TEST_F(AsyncClientManagerImplTest, EnvoyGrpcDynamicCluster) { @@ -69,7 +69,8 @@ TEST_F(AsyncClientManagerImplTest, EnvoyGrpcDynamicCluster) { EXPECT_CALL(*cluster.info_, addedViaApi()).WillOnce(Return(true)); EXPECT_THROW_WITH_MESSAGE( async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException, - "gRPC client cluster 'foo' is not static"); + "AsyncClientFactory: invalid cluster 'foo': currently only static (non-CDS) clusters are " + "supported"); } TEST_F(AsyncClientManagerImplTest, GoogleGrpc) {