From d6e7d0da6bfbd05ac38cce1fd6b61ba0cc4e12ba Mon Sep 17 00:00:00 2001 From: James Buckland Date: Mon, 9 Jul 2018 11:19:43 -0400 Subject: [PATCH 1/3] Remove support for cluster_names field in ApiConfigSource protos of type gRPC. Signed-off-by: James Buckland --- source/common/config/utility.cc | 30 +++++++++++++----------------- test/common/config/utility_test.cc | 26 ++++++++++++++++---------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index aa3d8cd80fe00..4050584760ea3 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -25,15 +25,19 @@ void Utility::translateApiConfigSource(const std::string& cluster, uint32_t refr envoy::api::v2::core::ApiConfigSource& api_config_source) { // TODO(junr03): document the option to chose an api type once we have created // stronger constraints around v2. - if (api_type == ApiType::get().RestLegacy) { - api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::REST_LEGACY); - } else if (api_type == ApiType::get().Rest) { - api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::REST); - } else { - ASSERT(api_type == ApiType::get().Grpc); + if (api_type == ApiType::get().Grpc) { api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); + envoy::api::v2::core::GrpcService* grpc_service = api_config_source.add_grpc_services(); + grpc_service->mutable_envoy_grpc()->set_cluster_name(cluster); + } else { + if (api_type == ApiType::get().RestLegacy) { + api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::REST_LEGACY); + } else if (api_type == ApiType::get().Rest) { + api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::REST); + } + api_config_source.add_cluster_names(cluster); } - api_config_source.add_cluster_names(cluster); + api_config_source.mutable_refresh_delay()->CopyFrom( Protobuf::util::TimeUtil::MillisecondsToDuration(refresh_delay_ms)); } @@ -90,12 +94,8 @@ void Utility::checkApiConfigSourceNames( 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"); + "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified."); } if (api_config_source.grpc_services().size() > 1) { throw EnvoyException( @@ -219,11 +219,7 @@ Grpc::AsyncClientFactoryPtr Utility::factoryForGrpcApiConfigSource( Utility::checkApiConfigSourceNames(api_config_source); envoy::api::v2::core::GrpcService grpc_service; - if (api_config_source.cluster_names().empty()) { - grpc_service.MergeFrom(api_config_source.grpc_services(0)); - } else { - grpc_service.mutable_envoy_grpc()->set_cluster_name(api_config_source.cluster_names(0)); - } + grpc_service.MergeFrom(api_config_source.grpc_services(0)); return async_client_manager.factoryForGrpcService(grpc_service, scope, false); } diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index d25fc023fba19..9e45d3df7e998 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -81,7 +81,8 @@ TEST(UtilityTest, TranslateApiConfigSource) { api_config_source_grpc); EXPECT_EQ(envoy::api::v2::core::ApiConfigSource::GRPC, api_config_source_grpc.api_type()); EXPECT_EQ(30000, DurationUtil::durationToMilliseconds(api_config_source_grpc.refresh_delay())); - EXPECT_EQ("test_grpc_cluster", api_config_source_grpc.cluster_names(0)); + EXPECT_EQ("test_grpc_cluster", + api_config_source_grpc.grpc_services(0).envoy_grpc().cluster_name()); } TEST(UtilityTest, createTagProducer) { @@ -216,8 +217,10 @@ TEST(UtilityTest, FactoryForGrpcApiConfigSource) { api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); api_config_source.add_cluster_names(); // this also logs a warning for setting REST cluster names for a gRPC API config. - EXPECT_NO_THROW( - Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope)); + EXPECT_THROW_WITH_REGEX( + Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope), + EnvoyException, + "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified."); } { @@ -225,11 +228,10 @@ TEST(UtilityTest, FactoryForGrpcApiConfigSource) { api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); api_config_source.add_cluster_names(); api_config_source.add_cluster_names(); - // this also logs a warning for setting REST cluster names for a gRPC API config. EXPECT_THROW_WITH_REGEX( Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope), EnvoyException, - "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified"); + "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified."); } { @@ -272,17 +274,14 @@ TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, GrpcClusterTestAcrossTy // API of type GRPC api_config_source->set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); - api_config_source->add_cluster_names("foo_cluster"); // GRPC cluster without GRPC services. EXPECT_THROW_WITH_MESSAGE( Utility::checkApiConfigSourceSubscriptionBackingCluster(cluster_map, *api_config_source), - EnvoyException, - "envoy::api::v2::core::ConfigSource must have a statically defined non-EDS cluster: " - "'foo_cluster' does not exist, was added via api, or is an EDS cluster"); + EnvoyException, "API configs must have either a gRPC service or a cluster name defined"); // Non-existent cluster. - api_config_source->add_grpc_services(); + api_config_source->add_grpc_services()->mutable_envoy_grpc()->set_cluster_name("foo_cluster"); EXPECT_THROW_WITH_MESSAGE( Utility::checkApiConfigSourceSubscriptionBackingCluster(cluster_map, *api_config_source), EnvoyException, @@ -315,6 +314,13 @@ TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, GrpcClusterTestAcrossTy EXPECT_CALL(*cluster.info_, addedViaApi()); EXPECT_CALL(*cluster.info_, type()); Utility::checkApiConfigSourceSubscriptionBackingCluster(cluster_map, *api_config_source); + + // API with cluster_names set should be rejected. + api_config_source->add_cluster_names("foo_cluster"); + EXPECT_THROW_WITH_MESSAGE( + Utility::checkApiConfigSourceSubscriptionBackingCluster(cluster_map, *api_config_source), + EnvoyException, + "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified."); } TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, RestClusterTestAcrossTypes) { From cdf79cbdb6a2caa9bc1d238a0230cc4fa75d0aaa Mon Sep 17 00:00:00 2001 From: James Buckland Date: Mon, 9 Jul 2018 14:00:53 -0400 Subject: [PATCH 2/3] Prefer .empty() to .size() == 0 Signed-off-by: James Buckland --- source/common/config/utility.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 4050584760ea3..a5758d88703fc 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -87,13 +87,12 @@ void Utility::checkApiConfigSourceNames( 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"); - } + if (api_config_source.cluster_names().empty() && api_config_source.grpc_services().empty()) { + throw EnvoyException("API configs must have either a gRPC service or a cluster name defined"); + } if (is_grpc) { - if (api_config_source.cluster_names().size() != 0) { + if (!api_config_source.cluster_names().empty()) { throw EnvoyException( "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified."); } @@ -102,7 +101,7 @@ void Utility::checkApiConfigSourceNames( "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified"); } } else { - if (api_config_source.grpc_services().size() != 0) { + if (!api_config_source.grpc_services().empty()) { throw EnvoyException("envoy::api::v2::core::ConfigSource, if not of type gRPC, must not have " "a gRPC service specified"); } From 56199c402e551725d829c2efa0e0cab9c9f1e0d3 Mon Sep 17 00:00:00 2001 From: James Buckland Date: Mon, 9 Jul 2018 14:23:22 -0400 Subject: [PATCH 3/3] Fix formatting Signed-off-by: James Buckland --- source/common/config/utility.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index a5758d88703fc..926362ff38535 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -88,8 +88,8 @@ void Utility::checkApiConfigSourceNames( (api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC); if (api_config_source.cluster_names().empty() && api_config_source.grpc_services().empty()) { - throw EnvoyException("API configs must have either a gRPC service or a cluster name defined"); - } + throw EnvoyException("API configs must have either a gRPC service or a cluster name defined"); + } if (is_grpc) { if (!api_config_source.cluster_names().empty()) {