diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index aa3d8cd80fe00..926362ff38535 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)); } @@ -83,26 +87,21 @@ 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) { + 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) { - 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) { + if (!api_config_source.cluster_names().empty()) { 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( "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"); } @@ -219,11 +218,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) {