From 0bbac4053470ebc3bfb357c15a4a81f44eb7a98e Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 7 Apr 2020 10:17:16 -0400 Subject: [PATCH 1/2] api: fix v3 delta gRPC ADS support. Fixes #10651. Risk level: Low Testing: Extended api_version_integration_test to cover this regression. Signed-off-by: Harvey Tuch --- source/common/upstream/cluster_manager_impl.cc | 8 ++++++-- test/integration/api_version_integration_test.cc | 15 ++++++++++----- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index a8da8b3c1d1e7..ac03d7d71526c 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -268,7 +268,11 @@ ClusterManagerImpl::ClusterManagerImpl( ->create(), main_thread_dispatcher, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( - "envoy.service.discovery.v2.AggregatedDiscoveryService.DeltaAggregatedResources"), + dyn_resources.ads_config().transport_api_version() == + envoy::config::core::v3::ApiVersion::V3 + ? "envoy.service.discovery.v3.AggregatedDiscoveryService.DeltaAggregatedResources" + : "envoy.service.discovery.v2.AggregatedDiscoveryService." + "DeltaAggregatedResources"), dyn_resources.ads_config().transport_api_version(), random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), local_info); } else { @@ -1369,4 +1373,4 @@ ProdClusterManagerFactory::createCds(const envoy::config::core::v3::ConfigSource } } // namespace Upstream -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/test/integration/api_version_integration_test.cc b/test/integration/api_version_integration_test.cc index 6e31f652e42b6..d7681cfd4e98f 100644 --- a/test/integration/api_version_integration_test.cc +++ b/test/integration/api_version_integration_test.cc @@ -112,8 +112,12 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, std::string actual_type_url; const char ads_v2_sotw_endpoint[] = "/envoy.service.discovery.v2.AggregatedDiscoveryService/StreamAggregatedResources"; - const char ads_v3_delta_endpoint[] = + const char ads_v3_sotw_endpoint[] = "/envoy.service.discovery.v3.AggregatedDiscoveryService/StreamAggregatedResources"; + const char ads_v2_delta_endpoint[] = + "/envoy.service.discovery.v2.AggregatedDiscoveryService/DeltaAggregatedResources"; + const char ads_v3_delta_endpoint[] = + "/envoy.service.discovery.v3.AggregatedDiscoveryService/DeltaAggregatedResources"; switch (transportApiVersion()) { case envoy::config::core::v3::ApiVersion::AUTO: case envoy::config::core::v3::ApiVersion::V2: { @@ -133,7 +137,7 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, EXPECT_TRUE(!hasHiddenEnvoyDeprecated(delta_discovery_request)); xds_stream_->startGrpcStream(); actual_type_url = delta_discovery_request.type_url(); - expected_endpoint = expected_v2_delta_endpoint; + expected_endpoint = ads() ? ads_v2_delta_endpoint : expected_v2_delta_endpoint; break; } case envoy::config::core::v3::ApiConfigSource::REST: { @@ -158,7 +162,7 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, VERIFY_ASSERTION(xds_stream_->waitForGrpcMessage(*dispatcher_, discovery_request)); EXPECT_TRUE(!hasHiddenEnvoyDeprecated(discovery_request)); actual_type_url = discovery_request.type_url(); - expected_endpoint = ads() ? ads_v3_delta_endpoint : expected_v3_sotw_endpoint; + expected_endpoint = ads() ? ads_v3_sotw_endpoint : expected_v3_sotw_endpoint; break; } case envoy::config::core::v3::ApiConfigSource::DELTA_GRPC: { @@ -167,7 +171,7 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, VERIFY_ASSERTION(xds_stream_->waitForGrpcMessage(*dispatcher_, delta_discovery_request)); EXPECT_TRUE(!hasHiddenEnvoyDeprecated(delta_discovery_request)); actual_type_url = delta_discovery_request.type_url(); - expected_endpoint = expected_v3_delta_endpoint; + expected_endpoint = ads() ? ads_v3_delta_endpoint : expected_v3_delta_endpoint; break; } case envoy::config::core::v3::ApiConfigSource::REST: { @@ -258,7 +262,8 @@ INSTANTIATE_TEST_SUITE_P( AdsApiConfigSourcesExplicitApiVersions, ApiVersionIntegrationTest, testing::Combine(testing::Values(TestEnvironment::getIpVersionsForTest()[0]), testing::Values(true), - testing::Values(envoy::config::core::v3::ApiConfigSource::GRPC), + testing::Values(envoy::config::core::v3::ApiConfigSource::GRPC, + envoy::config::core::v3::ApiConfigSource::DELTA_GRPC), testing::Values(envoy::config::core::v3::ApiVersion::V2, envoy::config::core::v3::ApiVersion::V3), testing::Values(envoy::config::core::v3::ApiVersion::V2, From ed05ff478a7d95b9d5c199653caf218f1bd7c205 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 7 Apr 2020 13:25:16 -0400 Subject: [PATCH 2/2] Add TODOs. Signed-off-by: Harvey Tuch --- source/common/upstream/cluster_manager_impl.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index ac03d7d71526c..a777f4f0138f5 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -270,6 +270,8 @@ ClusterManagerImpl::ClusterManagerImpl( *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( dyn_resources.ads_config().transport_api_version() == envoy::config::core::v3::ApiVersion::V3 + // TODO(htuch): consolidate with type_to_endpoint.cc, once we sort out the future + // direction of that module re: https://github.com/envoyproxy/envoy/issues/10650. ? "envoy.service.discovery.v3.AggregatedDiscoveryService.DeltaAggregatedResources" : "envoy.service.discovery.v2.AggregatedDiscoveryService." "DeltaAggregatedResources"), @@ -285,6 +287,8 @@ ClusterManagerImpl::ClusterManagerImpl( *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( dyn_resources.ads_config().transport_api_version() == envoy::config::core::v3::ApiVersion::V3 + // TODO(htuch): consolidate with type_to_endpoint.cc, once we sort out the future + // direction of that module re: https://github.com/envoyproxy/envoy/issues/10650. ? "envoy.service.discovery.v3.AggregatedDiscoveryService." "StreamAggregatedResources" : "envoy.service.discovery.v2.AggregatedDiscoveryService."