From 28dcba30aeca47a1938916534de1c75d6ee5d45d Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 1 Sep 2020 01:21:00 +0000 Subject: [PATCH 01/32] first version Signed-off-by: chaoqinli --- source/common/config/api_type_oracle.cc | 21 +++++ source/common/config/api_type_oracle.h | 2 + source/common/config/grpc_mux_impl.cc | 23 +++-- source/common/config/new_grpc_mux_impl.cc | 7 ++ test/common/config/grpc_mux_impl_test.cc | 88 ++++++++++++++++++++ test/common/config/new_grpc_mux_impl_test.cc | 38 +++++++-- test/integration/ads_integration_test.cc | 21 +++++ 7 files changed, 184 insertions(+), 16 deletions(-) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index 161ecf058610..d7d1355fe36c 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -31,5 +31,26 @@ ApiTypeOracle::getEarlierVersionMessageTypeName(const std::string& message_type) } return absl::nullopt; } + +const absl::optional ApiTypeOracle::getEarlierTypeUrl(const std::string& type_url) { + char delimeter = '/'; + size_t type_name_start = type_url.size(); + for (size_t i = 0; i < type_url.size(); i++) { + if (type_url[i] == delimeter) { + type_name_start = i; + } + } + if (type_name_start == type_url.size()) { + return {}; + } + std::string message_type = type_url.substr(type_name_start + 1); + absl::optional old_message_type = + ApiTypeOracle::getEarlierVersionMessageTypeName(message_type); + if (old_message_type) { + return "type.googleapis.com/" + *old_message_type; + } + return {}; +} + } // namespace Config } // namespace Envoy diff --git a/source/common/config/api_type_oracle.h b/source/common/config/api_type_oracle.h index cd0c5971ee52..99fe9026f2ad 100644 --- a/source/common/config/api_type_oracle.h +++ b/source/common/config/api_type_oracle.h @@ -22,6 +22,8 @@ class ApiTypeOracle { static const absl::optional getEarlierVersionMessageTypeName(const std::string& message_type); + + static const absl::optional getEarlierTypeUrl(const std::string& type_url); }; } // namespace Config diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 907bf9148adf..6251ed96a389 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -116,17 +116,24 @@ ScopedResume GrpcMuxImpl::pause(const std::vector type_urls) { void GrpcMuxImpl::onDiscoveryResponse( std::unique_ptr&& message, ControlPlaneStats& control_plane_stats) { - const std::string& type_url = message->type_url(); + std::string type_url = message->type_url(); ENVOY_LOG(debug, "Received gRPC message for {} at version {}", type_url, message->version_info()); if (message->has_control_plane()) { control_plane_stats.identifier_.set(message->control_plane().identifier()); } if (api_state_.count(type_url) == 0) { - ENVOY_LOG(warn, "Ignoring the message for type URL {} as it has no current subscribers.", - type_url); - // TODO(yuval-k): This should never happen. consider dropping the stream as this is a - // protocol violation - return; + absl::optional old_type_url = + ApiTypeOracle::getEarlierTypeUrl(message->type_url()); + if (old_type_url && api_state_.count(*old_type_url)) { + type_url = *old_type_url; + ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); + } else { + // TODO(yuval-k): This should never happen. consider dropping the stream as this is a + // protocol violation + ENVOY_LOG(warn, "Ignoring the message for type URL {} as it has no current subscribers.", + type_url); + return; + } } if (api_state_[type_url].watches_.empty()) { // update the nonce as we are processing this response. @@ -164,10 +171,10 @@ void GrpcMuxImpl::onDiscoveryResponse( OpaqueResourceDecoder& resource_decoder = api_state_[type_url].watches_.front()->resource_decoder_; for (const auto& resource : message->resources()) { - if (type_url != resource.type_url()) { + if (message->type_url() != resource.type_url()) { throw EnvoyException( fmt::format("{} does not match the message-wide type URL {} in DiscoveryResponse {}", - resource.type_url(), type_url, message->DebugString())); + resource.type_url(), message->type_url(), message->DebugString())); } resources.emplace_back( new DecodedResourceImpl(resource_decoder, resource, message->version_info())); diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 131ccd24db51..04737ba91cd6 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -50,6 +50,13 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), message->system_version_info()); auto sub = subscriptions_.find(message->type_url()); + if (sub == subscriptions_.end()) { + absl::optional old_type_url = + ApiTypeOracle::getEarlierTypeUrl(message->type_url()); + if (old_type_url) { + sub = subscriptions_.find(*old_type_url); + } + } if (sub == subscriptions_.end()) { ENVOY_LOG(warn, "Dropping received DeltaDiscoveryResponse (with version {}) for non-existent " diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 5a8bd21840db..23c6bb47e1e7 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -722,6 +722,94 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { "ads: node 'id' and 'cluster' are required. Set it either in 'node' config or via " "--service-node and --service-cluster options."); } + +// Send discovery request with v2 resource type_url, receive discovery response with v3 resource +// type_url. +TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { + setup(); + InSequence s; + TestUtility::TestOpaqueResourceDecoderImpl + resource_decoder("cluster_name"); + const std::string& v2_type_url = Config::TypeUrl::get().ClusterLoadAssignment; + const std::string& v3_type_url = + "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"; + NiceMock foo_callbacks; + auto foo_sub = grpc_mux_->addWatch(v2_type_url, {"x", "y"}, foo_callbacks, resource_decoder); + NiceMock bar_callbacks; + auto bar_sub = grpc_mux_->addWatch(v2_type_url, {"y", "z"}, bar_callbacks, resource_decoder); + EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); + // Should dedupe the "x" resource. + expectSendMessage(v2_type_url, {"y", "z", "x"}, "", true); + grpc_mux_->start(); + + { + auto response = std::make_unique(); + response->set_type_url(v3_type_url); + response->set_version_info("1"); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment; + load_assignment.set_cluster_name("x"); + response->add_resources()->PackFrom(load_assignment); + EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "1")).Times(0); + EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "1")) + .WillOnce(Invoke([&load_assignment](const std::vector& resources, + const std::string&) { + EXPECT_EQ(1, resources.size()); + const auto& expected_assignment = + dynamic_cast( + resources[0].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); + })); + expectSendMessage(v2_type_url, {"y", "z", "x"}, "1"); + grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); + } + + { + auto response = std::make_unique(); + response->set_type_url(v3_type_url); + response->set_version_info("2"); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_x; + load_assignment_x.set_cluster_name("x"); + response->add_resources()->PackFrom(load_assignment_x); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_y; + load_assignment_y.set_cluster_name("y"); + response->add_resources()->PackFrom(load_assignment_y); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_z; + load_assignment_z.set_cluster_name("z"); + response->add_resources()->PackFrom(load_assignment_z); + EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "2")) + .WillOnce(Invoke([&load_assignment_y, &load_assignment_z]( + const std::vector& resources, const std::string&) { + EXPECT_EQ(2, resources.size()); + const auto& expected_assignment = + dynamic_cast( + resources[0].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_y)); + const auto& expected_assignment_1 = + dynamic_cast( + resources[1].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment_1, load_assignment_z)); + })); + EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "2")) + .WillOnce(Invoke([&load_assignment_x, &load_assignment_y]( + const std::vector& resources, const std::string&) { + EXPECT_EQ(2, resources.size()); + const auto& expected_assignment = + dynamic_cast( + resources[0].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_x)); + const auto& expected_assignment_1 = + dynamic_cast( + resources[1].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment_1, load_assignment_y)); + })); + expectSendMessage(v2_type_url, {"y", "z", "x"}, "2"); + grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); + } + + expectSendMessage(v2_type_url, {"x", "y"}, "2"); + expectSendMessage(v2_type_url, {}, "2"); +} + } // namespace } // namespace Config } // namespace Envoy diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index a35a38d57725..fb89ea2f3669 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -162,17 +162,39 @@ TEST_F(NewGrpcMuxImplTest, ConfigUpdateWithNotFoundResponse) { response->set_type_url(type_url); response->set_system_version_info("1"); - response->add_resources(); - response->mutable_resources()->at(0).set_name("not-found"); - response->mutable_resources()->at(0).add_aliases("domain1.test"); + envoy::config::route::v3::VirtualHost vhost; + vhost.set_name("vhost_1"); - grpc_mux_->onDiscoveryResponse(std::move(response), control_plane_stats_); + response->add_resources()->mutable_resource()->PackFrom(vhost); +} - const auto& subscriptions = grpc_mux_->subscriptions(); - auto sub = subscriptions.find(type_url); +// Watch v2 resource type_url, receive discovery response with v3 resource type_url. +TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { + setup(); - EXPECT_TRUE(sub != subscriptions.end()); - watch->update({}); + // Watch for v2 resource type_url. + const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; + auto watch = grpc_mux_->addWatch(type_url, {}, callbacks_, resource_decoder_); + + EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); + grpc_mux_->start(); + { + auto response = std::make_unique(); + response->set_system_version_info("1"); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment; + load_assignment.set_cluster_name("x"); + response->add_resources()->mutable_resource()->PackFrom(load_assignment); + response->set_type_url("type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "1")) + .WillOnce(Invoke([&load_assignment](const std::vector& added_resources, + const Protobuf::RepeatedPtrField&, + const std::string&) { + EXPECT_EQ(1, added_resources.size()); + EXPECT_TRUE( + TestUtility::protoEqual(added_resources[0].get().resource(), load_assignment)); + })); + grpc_mux_->onDiscoveryResponse(std::move(response), control_plane_stats_); + } } } // namespace diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index bf413b9d91d5..9d29c9d2c76e 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -115,6 +115,27 @@ TEST_P(AdsIntegrationTest, Failure) { makeSingleRequest(); } +// Validate that we can recover from failures. +TEST_P(AdsIntegrationTest, MixV2V3TypeUrlInDiscoveryResponse) { + initialize(); + sendDiscoveryResponse(Config::TypeUrl::get().Cluster, + {buildCluster("cluster_0")}, + {buildCluster("cluster_0")}, {}, "1"); + sendDiscoveryResponse( + Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, + {buildClusterLoadAssignment("cluster_0")}, {}, "1"); + sendDiscoveryResponse( + "type.googleapis.com/envoy.config.listener.v3.Listener", + {buildListener("listener_0", "route_config_0")}, + {buildListener("listener_0", "route_config_0")}, {}, "1", false); + sendDiscoveryResponse( + Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")}, + {buildRouteConfig("route_config_0", "cluster_0")}, {}, "1"); + test_server_->waitForCounterGe("listener_manager.listener_create_success", 1); + // Validate that we can process a request. + makeSingleRequest(); +} + // Validate that the request with duplicate listeners is rejected. TEST_P(AdsIntegrationTest, DuplicateWarmingListeners) { initialize(); From 0b277126ba0737465bf8c33ace7f15e36e329a8e Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 1 Sep 2020 03:50:13 +0000 Subject: [PATCH 02/32] add comment Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 1 + source/common/config/new_grpc_mux_impl.cc | 1 + test/common/config/grpc_mux_impl_test.cc | 2 +- test/common/config/new_grpc_mux_impl_test.cc | 1 + test/integration/ads_integration_test.cc | 12 ++++++++---- 5 files changed, 12 insertions(+), 5 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 6251ed96a389..2241b83d24ab 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -121,6 +121,7 @@ void GrpcMuxImpl::onDiscoveryResponse( if (message->has_control_plane()) { control_plane_stats.identifier_.set(message->control_plane().identifier()); } + // If this type url is not watched, try older version of type url. if (api_state_.count(type_url) == 0) { absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(message->type_url()); diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 04737ba91cd6..c75705dea3ba 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -50,6 +50,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), message->system_version_info()); auto sub = subscriptions_.find(message->type_url()); + // If this type url is not watched, try older version type url. if (sub == subscriptions_.end()) { absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(message->type_url()); diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 23c6bb47e1e7..b22dbfd7336a 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -738,11 +738,11 @@ TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { NiceMock bar_callbacks; auto bar_sub = grpc_mux_->addWatch(v2_type_url, {"y", "z"}, bar_callbacks, resource_decoder); EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); - // Should dedupe the "x" resource. expectSendMessage(v2_type_url, {"y", "z", "x"}, "", true); grpc_mux_->start(); { + // Send resource with v3 type url, should invoke onConfigUpdate. auto response = std::make_unique(); response->set_type_url(v3_type_url); response->set_version_info("1"); diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index fb89ea2f3669..e05a1cea5d95 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -184,6 +184,7 @@ TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment; load_assignment.set_cluster_name("x"); response->add_resources()->mutable_resource()->PackFrom(load_assignment); + // Send response that contains resource with v3 type url. response->set_type_url("type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"); EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "1")) .WillOnce(Invoke([&load_assignment](const std::vector& added_resources, diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 9d29c9d2c76e..92ad6bd29756 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -115,19 +115,23 @@ TEST_P(AdsIntegrationTest, Failure) { makeSingleRequest(); } -// Validate that we can recover from failures. +// Validate that xds can support a mix of v2 and v3 type url. TEST_P(AdsIntegrationTest, MixV2V3TypeUrlInDiscoveryResponse) { initialize(); - sendDiscoveryResponse(Config::TypeUrl::get().Cluster, - {buildCluster("cluster_0")}, - {buildCluster("cluster_0")}, {}, "1"); + // Discovery response with v3 type url. + sendDiscoveryResponse( + "type.googleapis.com/envoy.config.cluster.v3.Cluster", {buildCluster("cluster_0")}, + {buildCluster("cluster_0")}, {}, "1", false); + // Discovery response with v2 type url. sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, {buildClusterLoadAssignment("cluster_0")}, {}, "1"); + // Discovery response with v3 type url. sendDiscoveryResponse( "type.googleapis.com/envoy.config.listener.v3.Listener", {buildListener("listener_0", "route_config_0")}, {buildListener("listener_0", "route_config_0")}, {}, "1", false); + // Discovery response with v2 type url. sendDiscoveryResponse( Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")}, {buildRouteConfig("route_config_0", "cluster_0")}, {}, "1"); From e3f3485f95e99d68b3a7de8a83703163105fc849 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 1 Sep 2020 17:01:43 +0000 Subject: [PATCH 03/32] fix no watcher case Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 22 ++++++++++++++-------- source/common/config/new_grpc_mux_impl.cc | 1 + test/integration/ads_integration_test.cc | 5 +++-- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 2241b83d24ab..6c3d3a036757 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -121,13 +121,17 @@ void GrpcMuxImpl::onDiscoveryResponse( if (message->has_control_plane()) { control_plane_stats.identifier_.set(message->control_plane().identifier()); } - // If this type url is not watched, try older version of type url. - if (api_state_.count(type_url) == 0) { + // If this type url is not watched(no subscriber or no watcher), try older version of type url. + if (api_state_.count(type_url) == 0 || + (api_state_[type_url].watches_.empty() && !message->resources().empty())) { absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(message->type_url()); + if (old_type_url) { + ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); + } + if (old_type_url && api_state_.count(*old_type_url)) { type_url = *old_type_url; - ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); } else { // TODO(yuval-k): This should never happen. consider dropping the stream as this is a // protocol violation @@ -146,13 +150,15 @@ void GrpcMuxImpl::onDiscoveryResponse( // xDS server sends an empty list of ClusterLoadAssignment resources. we'll accept // this update. no need to send a discovery request, as we don't watch for anything. api_state_[type_url].request_.set_version_info(message->version_info()); + return; } else { - // No watches and we have resources - this should not happen. send a NACK (by not - // updating the version). - ENVOY_LOG(warn, "Ignoring unwatched type URL {}", type_url); - queueDiscoveryRequest(type_url); + bool type_match_found = false; + if (!type_match_found) { + ENVOY_LOG(warn, "Ignoring unwatched type URL {}", type_url); + queueDiscoveryRequest(type_url); + return; + } } - return; } ScopedResume same_type_resume; // We pause updates of the same type. This is necessary for SotW and GrpcMuxImpl, since unlike diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index c75705dea3ba..ea8aa082c987 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -56,6 +56,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ApiTypeOracle::getEarlierTypeUrl(message->type_url()); if (old_type_url) { sub = subscriptions_.find(*old_type_url); + ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); } } if (sub == subscriptions_.end()) { diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 92ad6bd29756..1d24b6015e34 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -133,8 +133,9 @@ TEST_P(AdsIntegrationTest, MixV2V3TypeUrlInDiscoveryResponse) { {buildListener("listener_0", "route_config_0")}, {}, "1", false); // Discovery response with v2 type url. sendDiscoveryResponse( - Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")}, - {buildRouteConfig("route_config_0", "cluster_0")}, {}, "1"); + "type.googleapis.com/envoy.config.route.v3.RouteConfiguration", + {buildRouteConfig("route_config_0", "cluster_0")}, + {buildRouteConfig("route_config_0", "cluster_0")}, {}, "1", false); test_server_->waitForCounterGe("listener_manager.listener_create_success", 1); // Validate that we can process a request. makeSingleRequest(); From 1190cf216ca72c2091b0a139c07fc18be4a31e69 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 1 Sep 2020 17:48:02 +0000 Subject: [PATCH 04/32] fix no watcher case Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 3 ++- test/common/config/grpc_mux_impl_test.cc | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 6c3d3a036757..221a3de0dd1b 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -132,7 +132,8 @@ void GrpcMuxImpl::onDiscoveryResponse( if (old_type_url && api_state_.count(*old_type_url)) { type_url = *old_type_url; - } else { + } + if (api_state_.count(type_url) == 0) { // TODO(yuval-k): This should never happen. consider dropping the stream as this is a // protocol violation ENVOY_LOG(warn, "Ignoring the message for type URL {} as it has no current subscribers.", diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index b22dbfd7336a..c164cefa31dc 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -759,6 +759,7 @@ TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { resources[0].get().resource()); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); })); + // Send request with v2 type url resources. expectSendMessage(v2_type_url, {"y", "z", "x"}, "1"); grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); } From 17a1002bc0064a34d726c080a406ef060f1c11fb Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Wed, 2 Sep 2020 02:37:33 +0000 Subject: [PATCH 05/32] clean up Signed-off-by: chaoqinli --- source/common/config/api_type_oracle.cc | 10 ++------- source/common/config/grpc_mux_impl.cc | 17 +++++---------- test/common/config/new_grpc_mux_impl_test.cc | 23 ++++++++++++++++++++ 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index d7d1355fe36c..c515e5af3122 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -33,14 +33,8 @@ ApiTypeOracle::getEarlierVersionMessageTypeName(const std::string& message_type) } const absl::optional ApiTypeOracle::getEarlierTypeUrl(const std::string& type_url) { - char delimeter = '/'; - size_t type_name_start = type_url.size(); - for (size_t i = 0; i < type_url.size(); i++) { - if (type_url[i] == delimeter) { - type_name_start = i; - } - } - if (type_name_start == type_url.size()) { + size_t type_name_start = type_url.find('/'); + if (type_name_start == std::string::npos) { return {}; } std::string message_type = type_url.substr(type_name_start + 1); diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 221a3de0dd1b..fe2613914408 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -124,13 +124,9 @@ void GrpcMuxImpl::onDiscoveryResponse( // If this type url is not watched(no subscriber or no watcher), try older version of type url. if (api_state_.count(type_url) == 0 || (api_state_[type_url].watches_.empty() && !message->resources().empty())) { - absl::optional old_type_url = - ApiTypeOracle::getEarlierTypeUrl(message->type_url()); - if (old_type_url) { - ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); - } - + absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); if (old_type_url && api_state_.count(*old_type_url)) { + ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); type_url = *old_type_url; } if (api_state_.count(type_url) == 0) { @@ -153,12 +149,9 @@ void GrpcMuxImpl::onDiscoveryResponse( api_state_[type_url].request_.set_version_info(message->version_info()); return; } else { - bool type_match_found = false; - if (!type_match_found) { - ENVOY_LOG(warn, "Ignoring unwatched type URL {}", type_url); - queueDiscoveryRequest(type_url); - return; - } + ENVOY_LOG(warn, "Ignoring unwatched type URL {}", type_url); + queueDiscoveryRequest(type_url); + return; } } ScopedResume same_type_resume; diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index e05a1cea5d95..a452c37cf8fd 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -177,7 +177,30 @@ TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { auto watch = grpc_mux_->addWatch(type_url, {}, callbacks_, resource_decoder_); EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); + // Cluster is not watched, v3 resource is rejected. grpc_mux_->start(); + { + auto unexpected_response = + std::make_unique(); + envoy::config::cluster::v3::Cluster cluster; + unexpected_response->set_type_url("type.googleapis.com/envoy.config.cluster.v3.Cluster"); + unexpected_response->set_system_version_info("0"); + unexpected_response->add_resources()->mutable_resource()->PackFrom(cluster); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "0")).Times(0); + grpc_mux_->onDiscoveryResponse(std::move(unexpected_response), control_plane_stats_); + } + // Cluster is not watched, v2 resource is rejected. + { + auto unexpected_response = + std::make_unique(); + envoy::config::cluster::v3::Cluster cluster; + unexpected_response->set_type_url(Config::TypeUrl::get().Cluster); + unexpected_response->set_system_version_info("0"); + unexpected_response->add_resources()->mutable_resource()->PackFrom(API_DOWNGRADE(cluster)); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "0")).Times(0); + grpc_mux_->onDiscoveryResponse(std::move(unexpected_response), control_plane_stats_); + } + // ClusterLoadAssignment v2 is watched, v3 resource will be accepted. { auto response = std::make_unique(); response->set_system_version_info("1"); From 333d68f986da4701b9b12f040143735b9ba1c3fc Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Wed, 2 Sep 2020 02:59:41 +0000 Subject: [PATCH 06/32] clean up Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 2 +- source/common/config/new_grpc_mux_impl.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index fe2613914408..a7bc17e670ea 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -125,7 +125,7 @@ void GrpcMuxImpl::onDiscoveryResponse( if (api_state_.count(type_url) == 0 || (api_state_[type_url].watches_.empty() && !message->resources().empty())) { absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); - if (old_type_url && api_state_.count(*old_type_url)) { + if (old_type_url.has_value() && api_state_.count(*old_type_url)) { ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); type_url = *old_type_url; } diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index ea8aa082c987..d47a503c3115 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -54,7 +54,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( if (sub == subscriptions_.end()) { absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(message->type_url()); - if (old_type_url) { + if (old_type_url.has_value()) { sub = subscriptions_.find(*old_type_url); ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); } From ca8e67b3004bab01832f7e9469388d889dd7c2ac Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Wed, 2 Sep 2020 14:20:35 +0000 Subject: [PATCH 07/32] clean up Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 4 ++-- source/common/config/new_grpc_mux_impl.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index a7bc17e670ea..1387316aa4da 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -126,8 +126,8 @@ void GrpcMuxImpl::onDiscoveryResponse( (api_state_[type_url].watches_.empty() && !message->resources().empty())) { absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); if (old_type_url.has_value() && api_state_.count(*old_type_url)) { - ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); - type_url = *old_type_url; + ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), old_type_url.value()); + type_url = old_type_url.value(); } if (api_state_.count(type_url) == 0) { // TODO(yuval-k): This should never happen. consider dropping the stream as this is a diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index d47a503c3115..36c85473ca95 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -55,8 +55,8 @@ void NewGrpcMuxImpl::onDiscoveryResponse( absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(message->type_url()); if (old_type_url.has_value()) { - sub = subscriptions_.find(*old_type_url); - ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), *old_type_url); + sub = subscriptions_.find(old_type_url.value()); + ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), old_type_url.value()); } } if (sub == subscriptions_.end()) { From 53887acb64b1da8dce598f3d0ab52a0c40668394 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Wed, 2 Sep 2020 14:22:07 +0000 Subject: [PATCH 08/32] clean up Signed-off-by: chaoqinli --- source/common/config/api_type_oracle.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index c515e5af3122..9d015c299dd3 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -40,8 +40,8 @@ const absl::optional ApiTypeOracle::getEarlierTypeUrl(const std::st std::string message_type = type_url.substr(type_name_start + 1); absl::optional old_message_type = ApiTypeOracle::getEarlierVersionMessageTypeName(message_type); - if (old_message_type) { - return "type.googleapis.com/" + *old_message_type; + if (old_message_type.has_value()) { + return "type.googleapis.com/" + old_message_type.value(); } return {}; } From de87fd6a21c842627d913470f0e44fe9d78dea1f Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Thu, 3 Sep 2020 17:14:51 +0000 Subject: [PATCH 09/32] support v2 response to v3 request Signed-off-by: chaoqinli --- include/envoy/config/grpc_mux.h | 18 ++++ source/common/config/grpc_mux_impl.cc | 11 ++- source/common/config/grpc_mux_impl.h | 6 ++ source/common/config/new_grpc_mux_impl.cc | 10 +-- source/common/config/new_grpc_mux_impl.h | 4 + test/common/config/grpc_mux_impl_test.cc | 89 ++++++++++++++++++++ test/common/config/new_grpc_mux_impl_test.cc | 34 ++++++++ test/integration/ads_integration_test.cc | 2 +- 8 files changed, 162 insertions(+), 12 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index 0f20aae3cfc5..771718354ceb 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -100,6 +100,24 @@ class GrpcMux { const std::set& resources, SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder) PURE; + + virtual absl::optional getEarlierTypeUrl(const std::string& type_url) PURE; + + virtual void registerVersionedTypeUrl(const std::string& type_url) { + if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { + return; + } + // If type_url is v3, earlier_type_url will contain v2 type url. + absl::optional earlier_type_url = getEarlierTypeUrl(type_url); + // Register v2 to v3 and v3 to v2 type_url mapping in the hash mapp. + if (earlier_type_url.has_value()) { + type_url_mapping_[earlier_type_url.value()] = type_url; + type_url_mapping_[type_url] = earlier_type_url.value(); + } + } + +protected: + absl::flat_hash_map type_url_mapping_; }; using GrpcMuxPtr = std::unique_ptr; diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 1387316aa4da..43360006e581 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -76,6 +76,7 @@ GrpcMuxWatchPtr GrpcMuxImpl::addWatch(const std::string& type_url, api_state_[type_url].request_.mutable_node()->MergeFrom(local_info_.node()); api_state_[type_url].subscribed_ = true; subscriptions_.emplace_back(type_url); + registerVersionedTypeUrl(type_url); } // This will send an updated request on each subscription. @@ -124,10 +125,9 @@ void GrpcMuxImpl::onDiscoveryResponse( // If this type url is not watched(no subscriber or no watcher), try older version of type url. if (api_state_.count(type_url) == 0 || (api_state_[type_url].watches_.empty() && !message->resources().empty())) { - absl::optional old_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); - if (old_type_url.has_value() && api_state_.count(*old_type_url)) { - ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), old_type_url.value()); - type_url = old_type_url.value(); + registerVersionedTypeUrl(type_url); + if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { + type_url = type_url_mapping_[type_url]; } if (api_state_.count(type_url) == 0) { // TODO(yuval-k): This should never happen. consider dropping the stream as this is a @@ -147,12 +147,11 @@ void GrpcMuxImpl::onDiscoveryResponse( // xDS server sends an empty list of ClusterLoadAssignment resources. we'll accept // this update. no need to send a discovery request, as we don't watch for anything. api_state_[type_url].request_.set_version_info(message->version_info()); - return; } else { ENVOY_LOG(warn, "Ignoring unwatched type URL {}", type_url); queueDiscoveryRequest(type_url); - return; } + return; } ScopedResume same_type_resume; // We pause updates of the same type. This is necessary for SotW and GrpcMuxImpl, since unlike diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 232559ad2167..a4a104854c6e 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -65,6 +65,9 @@ class GrpcMuxImpl : public GrpcMux, grpcStreamForTest() { return grpc_stream_; } + absl::optional getEarlierTypeUrl(const std::string& type_url) override { + return ApiTypeOracle::getEarlierTypeUrl(type_url); + } private: void drainRequests(); @@ -168,6 +171,9 @@ class NullGrpcMuxImpl : public GrpcMux, void onEstablishmentFailure() override {} void onDiscoveryResponse(std::unique_ptr&&, ControlPlaneStats&) override {} + absl::optional getEarlierTypeUrl(const std::string& type_url) override { + return ApiTypeOracle::getEarlierTypeUrl(type_url); + } }; } // namespace Config diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 36c85473ca95..e9cfc3ec0387 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -52,11 +52,10 @@ void NewGrpcMuxImpl::onDiscoveryResponse( auto sub = subscriptions_.find(message->type_url()); // If this type url is not watched, try older version type url. if (sub == subscriptions_.end()) { - absl::optional old_type_url = - ApiTypeOracle::getEarlierTypeUrl(message->type_url()); - if (old_type_url.has_value()) { - sub = subscriptions_.find(old_type_url.value()); - ENVOY_LOG(debug, "v3 {} converted to v2 {}.", message->type_url(), old_type_url.value()); + const std::string& type_url = message->type_url(); + registerVersionedTypeUrl(type_url); + if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { + sub = subscriptions_.find(type_url_mapping_[type_url]); } } if (sub == subscriptions_.end()) { @@ -125,6 +124,7 @@ GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, auto entry = subscriptions_.find(type_url); if (entry == subscriptions_.end()) { // We don't yet have a subscription for type_url! Make one! + registerVersionedTypeUrl(type_url); addSubscription(type_url); return addWatch(type_url, resources, callbacks, resource_decoder); } diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 431106a4dd39..8a4fa1b2522d 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -77,6 +77,10 @@ class NewGrpcMuxImpl return subscriptions_; } + absl::optional getEarlierTypeUrl(const std::string& type_url) override { + return ApiTypeOracle::getEarlierTypeUrl(type_url); + } + private: class WatchImpl : public GrpcMuxWatch { public: diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index c164cefa31dc..7606b199682c 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -811,6 +811,95 @@ TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { expectSendMessage(v2_type_url, {}, "2"); } +// Send discovery request with v3 resource type_url, receive discovery response with v2 resource +// type_url. +TEST_F(GrpcMuxImplTest, WatchV3ResourceV2) { + setup(); + InSequence s; + TestUtility::TestOpaqueResourceDecoderImpl + resource_decoder("cluster_name"); + const std::string& v2_type_url = Config::TypeUrl::get().ClusterLoadAssignment; + const std::string& v3_type_url = + "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"; + NiceMock foo_callbacks; + // Watch v3 type url. + auto foo_sub = grpc_mux_->addWatch(v3_type_url, {"x", "y"}, foo_callbacks, resource_decoder); + NiceMock bar_callbacks; + auto bar_sub = grpc_mux_->addWatch(v3_type_url, {"y", "z"}, bar_callbacks, resource_decoder); + EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); + expectSendMessage(v3_type_url, {"y", "z", "x"}, "", true); + grpc_mux_->start(); + + { + // Send resource with v2 type url, should invoke onConfigUpdate. + auto response = std::make_unique(); + response->set_type_url(v2_type_url); + response->set_version_info("1"); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment; + load_assignment.set_cluster_name("x"); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment)); + EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "1")).Times(0); + EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "1")) + .WillOnce(Invoke([&load_assignment](const std::vector& resources, + const std::string&) { + EXPECT_EQ(1, resources.size()); + const auto& expected_assignment = + dynamic_cast( + resources[0].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); + })); + // Send request with v2 type url resources. + expectSendMessage(v3_type_url, {"y", "z", "x"}, "1"); + grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); + } + + { + auto response = std::make_unique(); + response->set_type_url(v2_type_url); + response->set_version_info("2"); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_x; + load_assignment_x.set_cluster_name("x"); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_x)); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_y; + load_assignment_y.set_cluster_name("y"); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_y)); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_z; + load_assignment_z.set_cluster_name("z"); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_z)); + EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "2")) + .WillOnce(Invoke([&load_assignment_y, &load_assignment_z]( + const std::vector& resources, const std::string&) { + EXPECT_EQ(2, resources.size()); + const auto& expected_assignment = + dynamic_cast( + resources[0].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_y)); + const auto& expected_assignment_1 = + dynamic_cast( + resources[1].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment_1, load_assignment_z)); + })); + EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "2")) + .WillOnce(Invoke([&load_assignment_x, &load_assignment_y]( + const std::vector& resources, const std::string&) { + EXPECT_EQ(2, resources.size()); + const auto& expected_assignment = + dynamic_cast( + resources[0].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_x)); + const auto& expected_assignment_1 = + dynamic_cast( + resources[1].get().resource()); + EXPECT_TRUE(TestUtility::protoEqual(expected_assignment_1, load_assignment_y)); + })); + expectSendMessage(v3_type_url, {"y", "z", "x"}, "2"); + grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); + } + + expectSendMessage(v3_type_url, {"x", "y"}, "2"); + expectSendMessage(v3_type_url, {}, "2"); +} + } // namespace } // namespace Config } // namespace Envoy diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index a452c37cf8fd..67e8444975cb 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -221,6 +221,40 @@ TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { } } +// Watch v3 resource type_url, receive discovery response with v2 resource type_url. +TEST_F(NewGrpcMuxImplTest, V2ResourceResponseV3ResourceWatch) { + setup(); + + // Watch for v3 resource type_url. + const std::string& v3_type_url = + "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"; + const std::string& v2_type_url = Config::TypeUrl::get().ClusterLoadAssignment; + auto watch = grpc_mux_->addWatch(v3_type_url, {}, callbacks_, resource_decoder_); + + EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); + + grpc_mux_->start(); + // ClusterLoadAssignment v3 is watched, v2 resource will be accepted. + { + auto response = std::make_unique(); + response->set_system_version_info("1"); + envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment; + load_assignment.set_cluster_name("x"); + response->add_resources()->mutable_resource()->PackFrom(API_DOWNGRADE(load_assignment)); + // Send response that contains resource with v3 type url. + response->set_type_url(v2_type_url); + EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "1")) + .WillOnce(Invoke([&load_assignment](const std::vector& added_resources, + const Protobuf::RepeatedPtrField&, + const std::string&) { + EXPECT_EQ(1, added_resources.size()); + EXPECT_TRUE( + TestUtility::protoEqual(added_resources[0].get().resource(), load_assignment)); + })); + grpc_mux_->onDiscoveryResponse(std::move(response), control_plane_stats_); + } +} + } // namespace } // namespace Config } // namespace Envoy diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 1d24b6015e34..4713a8f069a2 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -131,7 +131,7 @@ TEST_P(AdsIntegrationTest, MixV2V3TypeUrlInDiscoveryResponse) { "type.googleapis.com/envoy.config.listener.v3.Listener", {buildListener("listener_0", "route_config_0")}, {buildListener("listener_0", "route_config_0")}, {}, "1", false); - // Discovery response with v2 type url. + // Discovery response with v3 type url. sendDiscoveryResponse( "type.googleapis.com/envoy.config.route.v3.RouteConfiguration", {buildRouteConfig("route_config_0", "cluster_0")}, From 8153e6c4959044b86028fe5f117d112a648ea54b Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Thu, 3 Sep 2020 17:56:23 +0000 Subject: [PATCH 10/32] fix spelling Signed-off-by: chaoqinli --- include/envoy/config/grpc_mux.h | 2 +- source/common/config/grpc_mux_impl.cc | 2 +- source/common/config/new_grpc_mux_impl.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index 771718354ceb..92b1a99df063 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -109,7 +109,7 @@ class GrpcMux { } // If type_url is v3, earlier_type_url will contain v2 type url. absl::optional earlier_type_url = getEarlierTypeUrl(type_url); - // Register v2 to v3 and v3 to v2 type_url mapping in the hash mapp. + // Register v2 to v3 and v3 to v2 type_url mapping in the hash map. if (earlier_type_url.has_value()) { type_url_mapping_[earlier_type_url.value()] = type_url; type_url_mapping_[type_url] = earlier_type_url.value(); diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 43360006e581..0ad43d5a0e2c 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -122,7 +122,7 @@ void GrpcMuxImpl::onDiscoveryResponse( if (message->has_control_plane()) { control_plane_stats.identifier_.set(message->control_plane().identifier()); } - // If this type url is not watched(no subscriber or no watcher), try older version of type url. + // If this type url is not watched(no subscriber or no watcher), try another version of type url. if (api_state_.count(type_url) == 0 || (api_state_[type_url].watches_.empty() && !message->resources().empty())) { registerVersionedTypeUrl(type_url); diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index e9cfc3ec0387..06cf71c3e990 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -50,7 +50,7 @@ void NewGrpcMuxImpl::onDiscoveryResponse( ENVOY_LOG(debug, "Received DeltaDiscoveryResponse for {} at version {}", message->type_url(), message->system_version_info()); auto sub = subscriptions_.find(message->type_url()); - // If this type url is not watched, try older version type url. + // If this type url is not watched, try another version type url. if (sub == subscriptions_.end()) { const std::string& type_url = message->type_url(); registerVersionedTypeUrl(type_url); From 56edcaf4af2dff239bba340530889b15f1c650d9 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 8 Sep 2020 21:43:59 +0000 Subject: [PATCH 11/32] move impl out of include header Signed-off-by: chaoqinli --- include/envoy/config/grpc_mux.h | 15 --------------- source/common/config/grpc_mux_impl.cc | 14 ++++++++++++++ source/common/config/grpc_mux_impl.h | 7 +------ source/common/config/new_grpc_mux_impl.cc | 13 +++++++++++++ source/common/config/new_grpc_mux_impl.h | 6 ++---- 5 files changed, 30 insertions(+), 25 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index 92b1a99df063..460dfd86cf58 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -101,21 +101,6 @@ class GrpcMux { SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder) PURE; - virtual absl::optional getEarlierTypeUrl(const std::string& type_url) PURE; - - virtual void registerVersionedTypeUrl(const std::string& type_url) { - if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { - return; - } - // If type_url is v3, earlier_type_url will contain v2 type url. - absl::optional earlier_type_url = getEarlierTypeUrl(type_url); - // Register v2 to v3 and v3 to v2 type_url mapping in the hash map. - if (earlier_type_url.has_value()) { - type_url_mapping_[earlier_type_url.value()] = type_url; - type_url_mapping_[type_url] = earlier_type_url.value(); - } - } - protected: absl::flat_hash_map type_url_mapping_; }; diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 0ad43d5a0e2c..d347eaa30613 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -114,6 +114,20 @@ ScopedResume GrpcMuxImpl::pause(const std::vector type_urls) { }); } +void GrpcMuxImpl::registerVersionedTypeUrl(const std::string& type_url) { + if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { + return; + } + // If type_url is v3, earlier_type_url will contain v2 type url. + absl::optional earlier_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); + + // Register v2 to v3 and v3 to v2 type_url mapping in the hash map. + if (earlier_type_url.has_value()) { + type_url_mapping_[earlier_type_url.value()] = type_url; + type_url_mapping_[type_url] = earlier_type_url.value(); + } +} + void GrpcMuxImpl::onDiscoveryResponse( std::unique_ptr&& message, ControlPlaneStats& control_plane_stats) { diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index a4a104854c6e..39e5fcb86ce3 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -55,6 +55,7 @@ class GrpcMuxImpl : public GrpcMux, // Config::GrpcStreamCallbacks void onStreamEstablished() override; void onEstablishmentFailure() override; + void registerVersionedTypeUrl(const std::string& type_url); void onDiscoveryResponse(std::unique_ptr&& message, ControlPlaneStats& control_plane_stats) override; @@ -65,9 +66,6 @@ class GrpcMuxImpl : public GrpcMux, grpcStreamForTest() { return grpc_stream_; } - absl::optional getEarlierTypeUrl(const std::string& type_url) override { - return ApiTypeOracle::getEarlierTypeUrl(type_url); - } private: void drainRequests(); @@ -171,9 +169,6 @@ class NullGrpcMuxImpl : public GrpcMux, void onEstablishmentFailure() override {} void onDiscoveryResponse(std::unique_ptr&&, ControlPlaneStats&) override {} - absl::optional getEarlierTypeUrl(const std::string& type_url) override { - return ApiTypeOracle::getEarlierTypeUrl(type_url); - } }; } // namespace Config diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 06cf71c3e990..5a8f9281c7dd 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -44,6 +44,19 @@ ScopedResume NewGrpcMuxImpl::pause(const std::vector type_urls) { }); } +void NewGrpcMuxImpl::registerVersionedTypeUrl(const std::string& type_url) { + if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { + return; + } + // If type_url is v3, earlier_type_url will contain v2 type url. + absl::optional earlier_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); + // Register v2 to v3 and v3 to v2 type_url mapping in the hash map. + if (earlier_type_url.has_value()) { + type_url_mapping_[earlier_type_url.value()] = type_url; + type_url_mapping_[type_url] = earlier_type_url.value(); + } +} + void NewGrpcMuxImpl::onDiscoveryResponse( std::unique_ptr&& message, ControlPlaneStats&) { diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 8a4fa1b2522d..919c7c8c7381 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -44,6 +44,8 @@ class NewGrpcMuxImpl ScopedResume pause(const std::string& type_url) override; ScopedResume pause(const std::vector type_urls) override; + void registerVersionedTypeUrl(const std::string& type_url); + void onDiscoveryResponse( std::unique_ptr&& message, ControlPlaneStats& control_plane_stats) override; @@ -77,10 +79,6 @@ class NewGrpcMuxImpl return subscriptions_; } - absl::optional getEarlierTypeUrl(const std::string& type_url) override { - return ApiTypeOracle::getEarlierTypeUrl(type_url); - } - private: class WatchImpl : public GrpcMuxWatch { public: From d4d68be1770af68871b056b0a56b1f220e4b9ea0 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 8 Sep 2020 22:19:39 +0000 Subject: [PATCH 12/32] fix watcher empty Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 3 +-- test/integration/ads_integration_test.cc | 11 +++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index d347eaa30613..8ddb454d8af0 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -137,8 +137,7 @@ void GrpcMuxImpl::onDiscoveryResponse( control_plane_stats.identifier_.set(message->control_plane().identifier()); } // If this type url is not watched(no subscriber or no watcher), try another version of type url. - if (api_state_.count(type_url) == 0 || - (api_state_[type_url].watches_.empty() && !message->resources().empty())) { + if (api_state_.count(type_url) == 0) { registerVersionedTypeUrl(type_url); if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { type_url = type_url_mapping_[type_url]; diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 4713a8f069a2..7ccbae797231 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -118,25 +118,24 @@ TEST_P(AdsIntegrationTest, Failure) { // Validate that xds can support a mix of v2 and v3 type url. TEST_P(AdsIntegrationTest, MixV2V3TypeUrlInDiscoveryResponse) { initialize(); + + // Send initial configuration. // Discovery response with v3 type url. sendDiscoveryResponse( "type.googleapis.com/envoy.config.cluster.v3.Cluster", {buildCluster("cluster_0")}, {buildCluster("cluster_0")}, {}, "1", false); - // Discovery response with v2 type url. sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, {buildClusterLoadAssignment("cluster_0")}, {}, "1"); - // Discovery response with v3 type url. sendDiscoveryResponse( "type.googleapis.com/envoy.config.listener.v3.Listener", {buildListener("listener_0", "route_config_0")}, {buildListener("listener_0", "route_config_0")}, {}, "1", false); - // Discovery response with v3 type url. sendDiscoveryResponse( - "type.googleapis.com/envoy.config.route.v3.RouteConfiguration", - {buildRouteConfig("route_config_0", "cluster_0")}, - {buildRouteConfig("route_config_0", "cluster_0")}, {}, "1", false); + Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")}, + {buildRouteConfig("route_config_0", "cluster_0")}, {}, "1"); test_server_->waitForCounterGe("listener_manager.listener_create_success", 1); + // Validate that we can process a request. makeSingleRequest(); } From fd72d9f46b7da31ad76c6770aff91576d1b6b292 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 8 Sep 2020 22:20:23 +0000 Subject: [PATCH 13/32] fix watcher empty Signed-off-by: chaoqinli --- test/integration/ads_integration_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 7ccbae797231..49949c3176e6 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -124,13 +124,16 @@ TEST_P(AdsIntegrationTest, MixV2V3TypeUrlInDiscoveryResponse) { sendDiscoveryResponse( "type.googleapis.com/envoy.config.cluster.v3.Cluster", {buildCluster("cluster_0")}, {buildCluster("cluster_0")}, {}, "1", false); + // Discovery response with v2 type url. sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, {buildClusterLoadAssignment("cluster_0")}, {}, "1"); + // Discovery response with v3 type url. sendDiscoveryResponse( "type.googleapis.com/envoy.config.listener.v3.Listener", {buildListener("listener_0", "route_config_0")}, {buildListener("listener_0", "route_config_0")}, {}, "1", false); + // Discovery response with v2 type url. sendDiscoveryResponse( Config::TypeUrl::get().RouteConfiguration, {buildRouteConfig("route_config_0", "cluster_0")}, {buildRouteConfig("route_config_0", "cluster_0")}, {}, "1"); From 3a374b7576a45a868817876e0572f375d5a8721c Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Fri, 11 Sep 2020 02:30:13 +0000 Subject: [PATCH 14/32] type url map be singleton Signed-off-by: chaoqinli --- include/envoy/config/grpc_mux.h | 4 ++-- source/common/config/grpc_mux_impl.cc | 13 +++++++------ source/common/config/new_grpc_mux_impl.cc | 12 +++++++----- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/include/envoy/config/grpc_mux.h b/include/envoy/config/grpc_mux.h index 460dfd86cf58..3f166245dfbb 100644 --- a/include/envoy/config/grpc_mux.h +++ b/include/envoy/config/grpc_mux.h @@ -101,8 +101,8 @@ class GrpcMux { SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder) PURE; -protected: - absl::flat_hash_map type_url_mapping_; + using TypeUrlMap = absl::flat_hash_map; + static TypeUrlMap& typeUrlMap() { MUTABLE_CONSTRUCT_ON_FIRST_USE(TypeUrlMap, {}); } }; using GrpcMuxPtr = std::unique_ptr; diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 8ddb454d8af0..0a6a30093229 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -115,16 +115,16 @@ ScopedResume GrpcMuxImpl::pause(const std::vector type_urls) { } void GrpcMuxImpl::registerVersionedTypeUrl(const std::string& type_url) { - if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { + TypeUrlMap& type_url_map = typeUrlMap(); + if (type_url_map.find(type_url) != type_url_map.end()) { return; } // If type_url is v3, earlier_type_url will contain v2 type url. absl::optional earlier_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); - // Register v2 to v3 and v3 to v2 type_url mapping in the hash map. if (earlier_type_url.has_value()) { - type_url_mapping_[earlier_type_url.value()] = type_url; - type_url_mapping_[type_url] = earlier_type_url.value(); + type_url_map[earlier_type_url.value()] = type_url; + type_url_map[type_url] = earlier_type_url.value(); } } @@ -139,8 +139,9 @@ void GrpcMuxImpl::onDiscoveryResponse( // If this type url is not watched(no subscriber or no watcher), try another version of type url. if (api_state_.count(type_url) == 0) { registerVersionedTypeUrl(type_url); - if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { - type_url = type_url_mapping_[type_url]; + TypeUrlMap& type_url_map = typeUrlMap(); + if (type_url_map.find(type_url) != type_url_map.end()) { + type_url = type_url_map[type_url]; } if (api_state_.count(type_url) == 0) { // TODO(yuval-k): This should never happen. consider dropping the stream as this is a diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 5a8f9281c7dd..c01799097f3c 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -45,15 +45,16 @@ ScopedResume NewGrpcMuxImpl::pause(const std::vector type_urls) { } void NewGrpcMuxImpl::registerVersionedTypeUrl(const std::string& type_url) { - if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { + TypeUrlMap& type_url_map = typeUrlMap(); + if (type_url_map.find(type_url) != type_url_map.end()) { return; } // If type_url is v3, earlier_type_url will contain v2 type url. absl::optional earlier_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); // Register v2 to v3 and v3 to v2 type_url mapping in the hash map. if (earlier_type_url.has_value()) { - type_url_mapping_[earlier_type_url.value()] = type_url; - type_url_mapping_[type_url] = earlier_type_url.value(); + type_url_map[earlier_type_url.value()] = type_url; + type_url_map[type_url] = earlier_type_url.value(); } } @@ -67,8 +68,9 @@ void NewGrpcMuxImpl::onDiscoveryResponse( if (sub == subscriptions_.end()) { const std::string& type_url = message->type_url(); registerVersionedTypeUrl(type_url); - if (type_url_mapping_.find(type_url) != type_url_mapping_.end()) { - sub = subscriptions_.find(type_url_mapping_[type_url]); + TypeUrlMap& type_url_map = typeUrlMap(); + if (type_url_map.find(type_url) != type_url_map.end()) { + sub = subscriptions_.find(type_url_map[type_url]); } } if (sub == subscriptions_.end()) { From cf0e1ec063bbc63a473fb86fe2627a4a3bfffd4b Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 15 Sep 2020 00:00:49 +0000 Subject: [PATCH 15/32] add runtime flag Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 26 +++++++++++-------- source/common/config/grpc_mux_impl.h | 5 +++- source/common/config/new_grpc_mux_impl.cc | 14 +++++++--- source/common/config/new_grpc_mux_impl.h | 5 +++- .../config/subscription_factory_impl.cc | 14 +++++++--- source/common/runtime/runtime_features.cc | 2 ++ .../common/upstream/cluster_manager_impl.cc | 8 ++++-- source/server/lds_api.cc | 2 +- source/server/lds_api.h | 2 +- test/common/config/grpc_mux_impl_test.cc | 9 ++++--- test/common/config/new_grpc_mux_impl_test.cc | 8 +++--- .../config/subscription_factory_impl_test.cc | 7 ++++- test/integration/ads_integration_test.cc | 2 ++ 13 files changed, 70 insertions(+), 34 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index aca3fefec239..e1a76c16f1ba 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -19,11 +19,13 @@ GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, const Protobuf::MethodDescriptor& service_method, envoy::config::core::v3::ApiVersion transport_api_version, Random::RandomGenerator& random, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node) + const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, + bool enable_type_url_downgrade_and_upgrade) : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, rate_limit_settings), local_info_(local_info), skip_subsequent_node_(skip_subsequent_node), - first_stream_request_(true), transport_api_version_(transport_api_version) { + first_stream_request_(true), transport_api_version_(transport_api_version), + enable_type_url_downgrade_and_upgrade_(enable_type_url_downgrade_and_upgrade) { Config::Utility::checkLocalInfo("ads", local_info); } @@ -76,7 +78,9 @@ GrpcMuxWatchPtr GrpcMuxImpl::addWatch(const std::string& type_url, api_state_[type_url].request_.mutable_node()->MergeFrom(local_info_.node()); api_state_[type_url].subscribed_ = true; subscriptions_.emplace_back(type_url); - registerVersionedTypeUrl(type_url); + if (enable_type_url_downgrade_and_upgrade_) { + registerVersionedTypeUrl(type_url); + } } // This will send an updated request on each subscription. @@ -137,19 +141,19 @@ void GrpcMuxImpl::onDiscoveryResponse( control_plane_stats.identifier_.set(message->control_plane().identifier()); } // If this type url is not watched(no subscriber or no watcher), try another version of type url. - if (api_state_.count(type_url) == 0) { + if (enable_type_url_downgrade_and_upgrade_ && api_state_.count(type_url) == 0) { registerVersionedTypeUrl(type_url); TypeUrlMap& type_url_map = typeUrlMap(); if (type_url_map.find(type_url) != type_url_map.end()) { type_url = type_url_map[type_url]; } - if (api_state_.count(type_url) == 0) { - // TODO(yuval-k): This should never happen. consider dropping the stream as this is a - // protocol violation - ENVOY_LOG(warn, "Ignoring the message for type URL {} as it has no current subscribers.", - type_url); - return; - } + } + if (api_state_.count(type_url) == 0) { + // TODO(yuval-k): This should never happen. consider dropping the stream as this is a + // protocol violation + ENVOY_LOG(warn, "Ignoring the message for type URL {} as it has no current subscribers.", + type_url); + return; } if (api_state_[type_url].watches_.empty()) { // update the nonce as we are processing this response. diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 063fbd6d9933..412f78e0b4ca 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -36,7 +36,8 @@ class GrpcMuxImpl : public GrpcMux, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, envoy::config::core::v3::ApiVersion transport_api_version, Random::RandomGenerator& random, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node); + const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, + const bool enable_type_url_downgrade_and_upgrade = false); ~GrpcMuxImpl() override = default; void start() override; @@ -139,6 +140,7 @@ class GrpcMuxImpl : public GrpcMux, const LocalInfo::LocalInfo& local_info_; const bool skip_subsequent_node_; bool first_stream_request_; + absl::node_hash_map api_state_; // Envoy's dependency ordering. std::list subscriptions_; @@ -148,6 +150,7 @@ class GrpcMuxImpl : public GrpcMux, // This string is a type URL. std::unique_ptr> request_queue_; const envoy::config::core::v3::ApiVersion transport_api_version_; + bool enable_type_url_downgrade_and_upgrade_; }; using GrpcMuxImplPtr = std::unique_ptr; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 752756f0e743..060726cf9d51 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -20,10 +20,12 @@ NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, envoy::config::core::v3::ApiVersion transport_api_version, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, - const LocalInfo::LocalInfo& local_info) + const LocalInfo::LocalInfo& local_info, + bool enable_type_url_downgrade_and_upgrade) : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, rate_limit_settings), - local_info_(local_info), transport_api_version_(transport_api_version) {} + local_info_(local_info), transport_api_version_(transport_api_version), + enable_type_url_downgrade_and_upgrade_(enable_type_url_downgrade_and_upgrade) {} ScopedResume NewGrpcMuxImpl::pause(const std::string& type_url) { return pause(std::vector{type_url}); @@ -45,6 +47,7 @@ ScopedResume NewGrpcMuxImpl::pause(const std::vector type_urls) { } void NewGrpcMuxImpl::registerVersionedTypeUrl(const std::string& type_url) { + TypeUrlMap& type_url_map = typeUrlMap(); if (type_url_map.find(type_url) != type_url_map.end()) { return; @@ -65,7 +68,8 @@ void NewGrpcMuxImpl::onDiscoveryResponse( message->system_version_info()); auto sub = subscriptions_.find(message->type_url()); // If this type url is not watched, try another version type url. - if (sub == subscriptions_.end()) { + if (enable_type_url_downgrade_and_upgrade_ && sub == subscriptions_.end()) { + ENVOY_LOG(debug, "fuck.\n"); const std::string& type_url = message->type_url(); registerVersionedTypeUrl(type_url); TypeUrlMap& type_url_map = typeUrlMap(); @@ -130,7 +134,9 @@ GrpcMuxWatchPtr NewGrpcMuxImpl::addWatch(const std::string& type_url, auto entry = subscriptions_.find(type_url); if (entry == subscriptions_.end()) { // We don't yet have a subscription for type_url! Make one! - registerVersionedTypeUrl(type_url); + if (enable_type_url_downgrade_and_upgrade_) { + registerVersionedTypeUrl(type_url); + } addSubscription(type_url, use_namespace_matching); return addWatch(type_url, resources, callbacks, resource_decoder, use_namespace_matching); } diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 42334cd4bb26..37b1da7c56b5 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -35,7 +35,8 @@ class NewGrpcMuxImpl envoy::config::core::v3::ApiVersion transport_api_version, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, - const LocalInfo::LocalInfo& local_info); + const LocalInfo::LocalInfo& local_info, + const bool enable_type_url_downgrade_and_upgrade = false); GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, @@ -153,6 +154,8 @@ class NewGrpcMuxImpl const LocalInfo::LocalInfo& local_info_; const envoy::config::core::v3::ApiVersion transport_api_version_; + + const bool enable_type_url_downgrade_and_upgrade_; }; using NewGrpcMuxImplPtr = std::unique_ptr; diff --git a/source/common/config/subscription_factory_impl.cc b/source/common/config/subscription_factory_impl.cc index 6495688add61..2cf8adb00b14 100644 --- a/source/common/config/subscription_factory_impl.cc +++ b/source/common/config/subscription_factory_impl.cc @@ -10,6 +10,7 @@ #include "common/config/type_to_endpoint.h" #include "common/config/utility.h" #include "common/protobuf/protobuf.h" +#include "common/runtime/runtime_features.h" namespace Envoy { namespace Config { @@ -28,15 +29,18 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( Config::Utility::checkLocalInfo(type_url, local_info_); std::unique_ptr result; SubscriptionStats stats = Utility::generateStats(scope); + auto& runtime_snapshot = runtime_.snapshot(); const auto transport_api_version = config.api_config_source().transport_api_version(); if (transport_api_version == envoy::config::core::v3::ApiVersion::V2 && - runtime_.snapshot().runtimeFeatureEnabled( + runtime_snapshot.runtimeFeatureEnabled( "envoy.reloadable_features.enable_deprecated_v2_api_warning")) { - runtime_.snapshot().countDeprecatedFeatureUse(); + runtime_snapshot.countDeprecatedFeatureUse(); ENVOY_LOG(warn, "xDS of version v2 has been deprecated and will be removed in subsequent versions"); } + const bool enable_type_url_downgrade_and_upgrade = runtime_snapshot.runtimeFeatureEnabled( + "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade"); switch (config.config_source_specifier_case()) { case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kPath: { @@ -73,7 +77,8 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( dispatcher_, sotwGrpcMethod(type_url, api_config_source.transport_api_version()), api_config_source.transport_api_version(), random_, scope, Utility::parseRateLimitSettings(api_config_source), - api_config_source.set_node_on_first_message_only()), + api_config_source.set_node_on_first_message_only(), + enable_type_url_downgrade_and_upgrade), callbacks, resource_decoder, stats, type_url, dispatcher_, Utility::configSourceInitialFetchTimeout(config), /*is_aggregated*/ false); @@ -85,7 +90,8 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( ->create(), dispatcher_, deltaGrpcMethod(type_url, api_config_source.transport_api_version()), api_config_source.transport_api_version(), random_, scope, - Utility::parseRateLimitSettings(api_config_source), local_info_), + Utility::parseRateLimitSettings(api_config_source), local_info_, + enable_type_url_downgrade_and_upgrade), callbacks, resource_decoder, stats, type_url, dispatcher_, Utility::configSourceInitialFetchTimeout(config), false); } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 8b4ab5679e42..f469ad1b09da 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -94,6 +94,8 @@ constexpr const char* runtime_features[] = { // When features are added here, there should be a tracking bug assigned to the // code owner to flip the default after sufficient testing. constexpr const char* disabled_runtime_features[] = { + // Allow Envoy to upgrade or downgrade version of type url. + "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", // TODO(asraa) flip this feature after codec errors are handled "envoy.reloadable_features.new_codec_behavior", // Sentinel and test flag. diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 56dda590fd41..dc87b4c07b73 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -315,7 +315,9 @@ ClusterManagerImpl::ClusterManagerImpl( : "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); + Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), local_info, + Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade")); } else { ads_mux_ = std::make_shared( local_info, @@ -334,7 +336,9 @@ ClusterManagerImpl::ClusterManagerImpl( "StreamAggregatedResources"), dyn_resources.ads_config().transport_api_version(), random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), - bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only()); + bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only(), + Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade")); } } else { ads_mux_ = std::make_unique(); diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 4a1a65ed125b..14126d005aa5 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -122,4 +122,4 @@ void LdsApiImpl::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason r } } // namespace Server -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/source/server/lds_api.h b/source/server/lds_api.h index 0ace5e7b937c..d96280edb3e6 100644 --- a/source/server/lds_api.h +++ b/source/server/lds_api.h @@ -52,4 +52,4 @@ class LdsApiImpl : public LdsApi, }; } // namespace Server -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 7606b199682c..d431c76f0771 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -56,12 +56,13 @@ class GrpcMuxImplTestBase : public testing::Test { {} - void setup() { + void setup(const bool enable_type_url_downgrade_and_upgrade = false) { grpc_mux_ = std::make_unique( local_info_, std::unique_ptr(async_client_), dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), - envoy::config::core::v3::ApiVersion::AUTO, random_, stats_, rate_limit_settings_, true); + envoy::config::core::v3::ApiVersion::AUTO, random_, stats_, rate_limit_settings_, true, + enable_type_url_downgrade_and_upgrade); } void setup(const RateLimitSettings& custom_rate_limit_settings) { @@ -726,7 +727,7 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { // Send discovery request with v2 resource type_url, receive discovery response with v3 resource // type_url. TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { - setup(); + setup(true); InSequence s; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); @@ -814,7 +815,7 @@ TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { // Send discovery request with v3 resource type_url, receive discovery response with v2 resource // type_url. TEST_F(GrpcMuxImplTest, WatchV3ResourceV2) { - setup(); + setup(true); InSequence s; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index c76894dd6c54..1de2ddce15b4 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -46,13 +46,13 @@ class NewGrpcMuxImplTestBase : public testing::Test { control_plane_connected_state_( stats_.gauge("control_plane.connected_state", Stats::Gauge::ImportMode::NeverImport)) {} - void setup() { + void setup(const bool enable_type_url_downgrade_and_upgrade = false) { grpc_mux_ = std::make_unique( std::unique_ptr(async_client_), dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), envoy::config::core::v3::ApiVersion::AUTO, random_, stats_, rate_limit_settings_, - local_info_); + local_info_, enable_type_url_downgrade_and_upgrade); } NiceMock dispatcher_; @@ -169,7 +169,7 @@ TEST_F(NewGrpcMuxImplTest, ConfigUpdateWithNotFoundResponse) { // Watch v2 resource type_url, receive discovery response with v3 resource type_url. TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { - setup(); + setup(true); // Watch for v2 resource type_url. const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; @@ -222,7 +222,7 @@ TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { // Watch v3 resource type_url, receive discovery response with v2 resource type_url. TEST_F(NewGrpcMuxImplTest, V2ResourceResponseV3ResourceWatch) { - setup(); + setup(true); // Watch for v3 resource type_url. const std::string& v3_type_url = diff --git a/test/common/config/subscription_factory_impl_test.cc b/test/common/config/subscription_factory_impl_test.cc index 35724d3f2bb4..5f251b592010 100644 --- a/test/common/config/subscription_factory_impl_test.cc +++ b/test/common/config/subscription_factory_impl_test.cc @@ -300,7 +300,12 @@ TEST_F(SubscriptionFactoryTest, LogWarningOnDeprecatedApi) { envoy::config::core::v3::ApiVersion::V2); NiceMock snapshot; EXPECT_CALL(runtime_, snapshot()).WillRepeatedly(ReturnRef(snapshot)); - EXPECT_CALL(snapshot, runtimeFeatureEnabled(_)).WillOnce(Return(true)); + EXPECT_CALL(snapshot, runtimeFeatureEnabled( + "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade")) + .WillOnce(Return(false)); + EXPECT_CALL(snapshot, + runtimeFeatureEnabled("envoy.reloadable_features.enable_deprecated_v2_api_warning")) + .WillOnce(Return(true)); EXPECT_CALL(snapshot, countDeprecatedFeatureUse()); Upstream::ClusterManager::ClusterSet primary_clusters; diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 49949c3176e6..58a81ba8122a 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -117,6 +117,8 @@ TEST_P(AdsIntegrationTest, Failure) { // Validate that xds can support a mix of v2 and v3 type url. TEST_P(AdsIntegrationTest, MixV2V3TypeUrlInDiscoveryResponse) { + config_helper_.addRuntimeOverride( + "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", "true"); initialize(); // Send initial configuration. From e0475fdf0148eddb7ad70d15db541363cdad1a33 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 15 Sep 2020 04:41:46 +0000 Subject: [PATCH 16/32] add runtime flag Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 15 +++++++++++++++ source/common/config/grpc_mux_impl.h | 8 +++++++- source/common/config/new_grpc_mux_impl.cc | 13 +++++++++++++ source/common/config/new_grpc_mux_impl.h | 10 +++++++++- source/common/config/subscription_factory_impl.cc | 9 ++------- source/common/upstream/cluster_manager_impl.cc | 8 ++------ .../config/subscription_factory_impl_test.cc | 7 +------ 7 files changed, 49 insertions(+), 21 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index e1a76c16f1ba..dce6f771b074 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -29,6 +29,21 @@ GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Config::Utility::checkLocalInfo("ads", local_info); } +GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, + Grpc::RawAsyncClientPtr async_client, Event::Dispatcher& dispatcher, + const Protobuf::MethodDescriptor& service_method, + envoy::config::core::v3::ApiVersion transport_api_version, + Random::RandomGenerator& random, Stats::Scope& scope, + const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node) + : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, + rate_limit_settings), + local_info_(local_info), skip_subsequent_node_(skip_subsequent_node), + first_stream_request_(true), transport_api_version_(transport_api_version), + enable_type_url_downgrade_and_upgrade_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade")) { + Config::Utility::checkLocalInfo("ads", local_info); +} + void GrpcMuxImpl::start() { grpc_stream_.establishNewStream(); } void GrpcMuxImpl::sendDiscoveryRequest(const std::string& type_url) { diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 412f78e0b4ca..a564bbd5529b 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -20,6 +20,7 @@ #include "common/config/api_version.h" #include "common/config/grpc_stream.h" #include "common/config/utility.h" +#include "common/runtime/runtime_features.h" #include "absl/container/node_hash_map.h" @@ -37,7 +38,12 @@ class GrpcMuxImpl : public GrpcMux, envoy::config::core::v3::ApiVersion transport_api_version, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, - const bool enable_type_url_downgrade_and_upgrade = false); + bool enable_type_url_downgrade_and_upgrade); + GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::RawAsyncClientPtr async_client, + Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, + envoy::config::core::v3::ApiVersion transport_api_version, + Random::RandomGenerator& random, Stats::Scope& scope, + const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node); ~GrpcMuxImpl() override = default; void start() override; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 060726cf9d51..02c6bba59d72 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -27,6 +27,19 @@ NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, local_info_(local_info), transport_api_version_(transport_api_version), enable_type_url_downgrade_and_upgrade_(enable_type_url_downgrade_and_upgrade) {} +NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, + Event::Dispatcher& dispatcher, + const Protobuf::MethodDescriptor& service_method, + envoy::config::core::v3::ApiVersion transport_api_version, + Random::RandomGenerator& random, Stats::Scope& scope, + const RateLimitSettings& rate_limit_settings, + const LocalInfo::LocalInfo& local_info) + : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, + rate_limit_settings), + local_info_(local_info), transport_api_version_(transport_api_version), + enable_type_url_downgrade_and_upgrade_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade")) {} + ScopedResume NewGrpcMuxImpl::pause(const std::string& type_url) { return pause(std::vector{type_url}); } diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 37b1da7c56b5..795f5e851113 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -16,6 +16,7 @@ #include "common/config/pausable_ack_queue.h" #include "common/config/watch_map.h" #include "common/grpc/common.h" +#include "common/runtime/runtime_features.h" namespace Envoy { namespace Config { @@ -36,7 +37,14 @@ class NewGrpcMuxImpl Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, const LocalInfo::LocalInfo& local_info, - const bool enable_type_url_downgrade_and_upgrade = false); + bool enable_type_url_downgrade_and_upgrade); + + NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, Event::Dispatcher& dispatcher, + const Protobuf::MethodDescriptor& service_method, + envoy::config::core::v3::ApiVersion transport_api_version, + Random::RandomGenerator& random, Stats::Scope& scope, + const RateLimitSettings& rate_limit_settings, + const LocalInfo::LocalInfo& local_info); GrpcMuxWatchPtr addWatch(const std::string& type_url, const std::set& resources, SubscriptionCallbacks& callbacks, diff --git a/source/common/config/subscription_factory_impl.cc b/source/common/config/subscription_factory_impl.cc index 2cf8adb00b14..ccfd6f93d6b9 100644 --- a/source/common/config/subscription_factory_impl.cc +++ b/source/common/config/subscription_factory_impl.cc @@ -10,7 +10,6 @@ #include "common/config/type_to_endpoint.h" #include "common/config/utility.h" #include "common/protobuf/protobuf.h" -#include "common/runtime/runtime_features.h" namespace Envoy { namespace Config { @@ -39,8 +38,6 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( ENVOY_LOG(warn, "xDS of version v2 has been deprecated and will be removed in subsequent versions"); } - const bool enable_type_url_downgrade_and_upgrade = runtime_snapshot.runtimeFeatureEnabled( - "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade"); switch (config.config_source_specifier_case()) { case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kPath: { @@ -77,8 +74,7 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( dispatcher_, sotwGrpcMethod(type_url, api_config_source.transport_api_version()), api_config_source.transport_api_version(), random_, scope, Utility::parseRateLimitSettings(api_config_source), - api_config_source.set_node_on_first_message_only(), - enable_type_url_downgrade_and_upgrade), + api_config_source.set_node_on_first_message_only()), callbacks, resource_decoder, stats, type_url, dispatcher_, Utility::configSourceInitialFetchTimeout(config), /*is_aggregated*/ false); @@ -90,8 +86,7 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource( ->create(), dispatcher_, deltaGrpcMethod(type_url, api_config_source.transport_api_version()), api_config_source.transport_api_version(), random_, scope, - Utility::parseRateLimitSettings(api_config_source), local_info_, - enable_type_url_downgrade_and_upgrade), + Utility::parseRateLimitSettings(api_config_source), local_info_), callbacks, resource_decoder, stats, type_url, dispatcher_, Utility::configSourceInitialFetchTimeout(config), false); } diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index dc87b4c07b73..56dda590fd41 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -315,9 +315,7 @@ ClusterManagerImpl::ClusterManagerImpl( : "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, - Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade")); + Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), local_info); } else { ads_mux_ = std::make_shared( local_info, @@ -336,9 +334,7 @@ ClusterManagerImpl::ClusterManagerImpl( "StreamAggregatedResources"), dyn_resources.ads_config().transport_api_version(), random_, stats_, Envoy::Config::Utility::parseRateLimitSettings(dyn_resources.ads_config()), - bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only(), - Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade")); + bootstrap.dynamic_resources().ads_config().set_node_on_first_message_only()); } } else { ads_mux_ = std::make_unique(); diff --git a/test/common/config/subscription_factory_impl_test.cc b/test/common/config/subscription_factory_impl_test.cc index 5f251b592010..35724d3f2bb4 100644 --- a/test/common/config/subscription_factory_impl_test.cc +++ b/test/common/config/subscription_factory_impl_test.cc @@ -300,12 +300,7 @@ TEST_F(SubscriptionFactoryTest, LogWarningOnDeprecatedApi) { envoy::config::core::v3::ApiVersion::V2); NiceMock snapshot; EXPECT_CALL(runtime_, snapshot()).WillRepeatedly(ReturnRef(snapshot)); - EXPECT_CALL(snapshot, runtimeFeatureEnabled( - "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade")) - .WillOnce(Return(false)); - EXPECT_CALL(snapshot, - runtimeFeatureEnabled("envoy.reloadable_features.enable_deprecated_v2_api_warning")) - .WillOnce(Return(true)); + EXPECT_CALL(snapshot, runtimeFeatureEnabled(_)).WillOnce(Return(true)); EXPECT_CALL(snapshot, countDeprecatedFeatureUse()); Upstream::ClusterManager::ClusterSet primary_clusters; From d0ba29a066bb6ad5c8148731f2c9f7e74b785f7d Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 15 Sep 2020 04:44:49 +0000 Subject: [PATCH 17/32] clean up Signed-off-by: chaoqinli --- source/server/lds_api.cc | 2 +- source/server/lds_api.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 14126d005aa5..4a1a65ed125b 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -122,4 +122,4 @@ void LdsApiImpl::onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason r } } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/source/server/lds_api.h b/source/server/lds_api.h index d96280edb3e6..0ace5e7b937c 100644 --- a/source/server/lds_api.h +++ b/source/server/lds_api.h @@ -52,4 +52,4 @@ class LdsApiImpl : public LdsApi, }; } // namespace Server -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From 504d403d7157c4b612ec99bb2fd0a7a4e3a61de5 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 15 Sep 2020 14:58:27 +0000 Subject: [PATCH 18/32] clean up Signed-off-by: chaoqinli --- source/common/config/new_grpc_mux_impl.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 02c6bba59d72..b5f4fa68f56d 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -82,7 +82,6 @@ void NewGrpcMuxImpl::onDiscoveryResponse( auto sub = subscriptions_.find(message->type_url()); // If this type url is not watched, try another version type url. if (enable_type_url_downgrade_and_upgrade_ && sub == subscriptions_.end()) { - ENVOY_LOG(debug, "fuck.\n"); const std::string& type_url = message->type_url(); registerVersionedTypeUrl(type_url); TypeUrlMap& type_url_map = typeUrlMap(); From 8ea1fac7d620cc9fbb2ab5b2646e857f20f1e84d Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 15 Sep 2020 21:24:42 +0000 Subject: [PATCH 19/32] unit test runtime Signed-off-by: chaoqinli --- source/common/config/grpc_mux_impl.cc | 15 --------------- source/common/config/grpc_mux_impl.h | 6 ------ source/common/config/new_grpc_mux_impl.cc | 13 ------------- source/common/config/new_grpc_mux_impl.h | 8 -------- test/common/config/BUILD | 2 ++ test/common/config/grpc_mux_impl_test.cc | 16 +++++++++++----- test/common/config/new_grpc_mux_impl_test.cc | 15 +++++++++++---- 7 files changed, 24 insertions(+), 51 deletions(-) diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index dce6f771b074..15c2a0e56ec0 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -14,21 +14,6 @@ namespace Envoy { namespace Config { -GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, - Grpc::RawAsyncClientPtr async_client, Event::Dispatcher& dispatcher, - const Protobuf::MethodDescriptor& service_method, - envoy::config::core::v3::ApiVersion transport_api_version, - Random::RandomGenerator& random, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, - bool enable_type_url_downgrade_and_upgrade) - : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, - rate_limit_settings), - local_info_(local_info), skip_subsequent_node_(skip_subsequent_node), - first_stream_request_(true), transport_api_version_(transport_api_version), - enable_type_url_downgrade_and_upgrade_(enable_type_url_downgrade_and_upgrade) { - Config::Utility::checkLocalInfo("ads", local_info); -} - GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::RawAsyncClientPtr async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index a564bbd5529b..f5ad9174454b 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -33,12 +33,6 @@ class GrpcMuxImpl : public GrpcMux, public GrpcStreamCallbacks, public Logger::Loggable { public: - GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::RawAsyncClientPtr async_client, - Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, - envoy::config::core::v3::ApiVersion transport_api_version, - Random::RandomGenerator& random, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node, - bool enable_type_url_downgrade_and_upgrade); GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::RawAsyncClientPtr async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, envoy::config::core::v3::ApiVersion transport_api_version, diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index b5f4fa68f56d..0015a2689971 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -14,19 +14,6 @@ namespace Envoy { namespace Config { -NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, - Event::Dispatcher& dispatcher, - const Protobuf::MethodDescriptor& service_method, - envoy::config::core::v3::ApiVersion transport_api_version, - Random::RandomGenerator& random, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, - const LocalInfo::LocalInfo& local_info, - bool enable_type_url_downgrade_and_upgrade) - : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, - rate_limit_settings), - local_info_(local_info), transport_api_version_(transport_api_version), - enable_type_url_downgrade_and_upgrade_(enable_type_url_downgrade_and_upgrade) {} - NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 795f5e851113..8e64f3e4399f 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -31,14 +31,6 @@ class NewGrpcMuxImpl public GrpcStreamCallbacks, Logger::Loggable { public: - NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, Event::Dispatcher& dispatcher, - const Protobuf::MethodDescriptor& service_method, - envoy::config::core::v3::ApiVersion transport_api_version, - Random::RandomGenerator& random, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, - const LocalInfo::LocalInfo& local_info, - bool enable_type_url_downgrade_and_upgrade); - NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, envoy::config::core::v3::ApiVersion transport_api_version, diff --git a/test/common/config/BUILD b/test/common/config/BUILD index e870e01a733a..fc4e272a1148 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -131,6 +131,7 @@ envoy_cc_test( "//test/test_common:logging_lib", "//test/test_common:resources_lib", "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:pkg_cc_proto", "@envoy_api//envoy/config/endpoint/v3:pkg_cc_proto", @@ -156,6 +157,7 @@ envoy_cc_test( "//test/test_common:logging_lib", "//test/test_common:resources_lib", "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/endpoint/v3:pkg_cc_proto", "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index d431c76f0771..36aebc75f9d1 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -24,6 +24,7 @@ #include "test/test_common/logging.h" #include "test/test_common/resources.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/test_time.h" #include "test/test_common/utility.h" @@ -56,13 +57,12 @@ class GrpcMuxImplTestBase : public testing::Test { {} - void setup(const bool enable_type_url_downgrade_and_upgrade = false) { + void setup() { grpc_mux_ = std::make_unique( local_info_, std::unique_ptr(async_client_), dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), - envoy::config::core::v3::ApiVersion::AUTO, random_, stats_, rate_limit_settings_, true, - enable_type_url_downgrade_and_upgrade); + envoy::config::core::v3::ApiVersion::AUTO, random_, stats_, rate_limit_settings_, true); } void setup(const RateLimitSettings& custom_rate_limit_settings) { @@ -727,7 +727,10 @@ TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { // Send discovery request with v2 resource type_url, receive discovery response with v3 resource // type_url. TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { - setup(true); + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", "true"}}); + setup(); InSequence s; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); @@ -815,7 +818,10 @@ TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { // Send discovery request with v3 resource type_url, receive discovery response with v2 resource // type_url. TEST_F(GrpcMuxImplTest, WatchV3ResourceV2) { - setup(true); + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", "true"}}); + setup(); InSequence s; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("cluster_name"); diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index 1de2ddce15b4..959dc854a7c8 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -21,6 +21,7 @@ #include "test/test_common/logging.h" #include "test/test_common/resources.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/test_time.h" #include "test/test_common/utility.h" @@ -46,13 +47,13 @@ class NewGrpcMuxImplTestBase : public testing::Test { control_plane_connected_state_( stats_.gauge("control_plane.connected_state", Stats::Gauge::ImportMode::NeverImport)) {} - void setup(const bool enable_type_url_downgrade_and_upgrade = false) { + void setup() { grpc_mux_ = std::make_unique( std::unique_ptr(async_client_), dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), envoy::config::core::v3::ApiVersion::AUTO, random_, stats_, rate_limit_settings_, - local_info_, enable_type_url_downgrade_and_upgrade); + local_info_); } NiceMock dispatcher_; @@ -169,7 +170,10 @@ TEST_F(NewGrpcMuxImplTest, ConfigUpdateWithNotFoundResponse) { // Watch v2 resource type_url, receive discovery response with v3 resource type_url. TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { - setup(true); + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", "true"}}); + setup(); // Watch for v2 resource type_url. const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; @@ -222,7 +226,10 @@ TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { // Watch v3 resource type_url, receive discovery response with v2 resource type_url. TEST_F(NewGrpcMuxImplTest, V2ResourceResponseV3ResourceWatch) { - setup(true); + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", "true"}}); + setup(); // Watch for v3 resource type_url. const std::string& v3_type_url = From 314d12761f03d7789f7074731af222d9aca5134b Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Thu, 17 Sep 2020 16:21:03 +0000 Subject: [PATCH 20/32] add doc Signed-off-by: chaoqinli --- docs/root/faq/api/control_plane_version_support.rst | 5 +++++ source/common/config/grpc_mux_impl.cc | 4 +++- source/common/config/grpc_mux_impl.h | 1 - source/common/runtime/runtime_features.cc | 3 ++- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/docs/root/faq/api/control_plane_version_support.rst b/docs/root/faq/api/control_plane_version_support.rst index 599ec8d7d8d8..d7b360db2b2a 100644 --- a/docs/root/faq/api/control_plane_version_support.rst +++ b/docs/root/faq/api/control_plane_version_support.rst @@ -30,6 +30,11 @@ typical rollout sequence might look like: 4. Support for v2 is removed in the management server. The management server moves to v3 exclusively internally and can support newer fields. +5. Client can send discovery request with v2 resource type url and process discovery response with + v3 resource type url. Client can also send discovery request with v3 resource type url and process + discovery response with v2 resource type url. The upgrade and downgrade of type url is disabled + by default and protected by a runtime guard *envoy.reloadable_features.enable_type_url_downgrade_and_upgrade*. + If you are operating a managed control plane as-a-service, you will likely need to support a wide range of client versions. In this scenario, you will require long term support for multiple major API transport and resource versions. Strategies for managing this support are described :ref:`here diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 15c2a0e56ec0..b415776d6b8e 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -124,7 +124,7 @@ void GrpcMuxImpl::registerVersionedTypeUrl(const std::string& type_url) { return; } // If type_url is v3, earlier_type_url will contain v2 type url. - absl::optional earlier_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); + const absl::optional earlier_type_url = ApiTypeOracle::getEarlierTypeUrl(type_url); // Register v2 to v3 and v3 to v2 type_url mapping in the hash map. if (earlier_type_url.has_value()) { type_url_map[earlier_type_url.value()] = type_url; @@ -166,6 +166,8 @@ void GrpcMuxImpl::onDiscoveryResponse( // this update. no need to send a discovery request, as we don't watch for anything. api_state_[type_url].request_.set_version_info(message->version_info()); } else { + // No watches and we have resources - this should not happen. send a NACK (by not + // updating the version). ENVOY_LOG(warn, "Ignoring unwatched type URL {}", type_url); queueDiscoveryRequest(type_url); } diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index f5ad9174454b..1006b165c5ba 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -140,7 +140,6 @@ class GrpcMuxImpl : public GrpcMux, const LocalInfo::LocalInfo& local_info_; const bool skip_subsequent_node_; bool first_stream_request_; - absl::node_hash_map api_state_; // Envoy's dependency ordering. std::list subscriptions_; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 2a9118e04723..2bc3f46942d5 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -94,7 +94,8 @@ constexpr const char* runtime_features[] = { // When features are added here, there should be a tracking bug assigned to the // code owner to flip the default after sufficient testing. constexpr const char* disabled_runtime_features[] = { - // Allow Envoy to upgrade or downgrade version of type url. + // Allow Envoy to upgrade or downgrade version of type url, should be removed when support for + // v2 url is removed from codebase. "envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", // TODO(asraa) flip this feature after codec errors are handled "envoy.reloadable_features.new_codec_behavior", From 1371da9fdd8c73fa1a862e3fff73f07dfc158fb9 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Thu, 17 Sep 2020 17:06:07 +0000 Subject: [PATCH 21/32] add helper function Signed-off-by: chaoqinli --- source/common/config/api_type_oracle.cc | 25 ++++++++++++++++--------- source/common/config/api_type_oracle.h | 5 +++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index 9d015c299dd3..21575aea93a5 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -32,16 +32,23 @@ ApiTypeOracle::getEarlierVersionMessageTypeName(const std::string& message_type) return absl::nullopt; } -const absl::optional ApiTypeOracle::getEarlierTypeUrl(const std::string& type_url) { - size_t type_name_start = type_url.find('/'); - if (type_name_start == std::string::npos) { - return {}; +const absl::string_view ApiTypeOracle::typeUrlToDescriptorFullName(absl::string_view type_url) { + const size_t pos = type_url.rfind('/'); + if (pos != absl::string_view::npos) { + type_url = type_url.substr(pos + 1); } - std::string message_type = type_url.substr(type_name_start + 1); - absl::optional old_message_type = - ApiTypeOracle::getEarlierVersionMessageTypeName(message_type); - if (old_message_type.has_value()) { - return "type.googleapis.com/" + old_message_type.value(); + return type_url; +} + +const std::string ApiTypeOracle::descriptorFullNameToTypeUrl(std::string& type) { + return "type.googleapis.com/" + type; +} + +const absl::optional ApiTypeOracle::getEarlierTypeUrl(const std::string& type_url) { + const std::string type{typeUrlToDescriptorFullName(type_url)}; + absl::optional old_type = ApiTypeOracle::getEarlierVersionMessageTypeName(type); + if (old_type.has_value()) { + return descriptorFullNameToTypeUrl(old_type.value()); } return {}; } diff --git a/source/common/config/api_type_oracle.h b/source/common/config/api_type_oracle.h index 99fe9026f2ad..65a3af31641e 100644 --- a/source/common/config/api_type_oracle.h +++ b/source/common/config/api_type_oracle.h @@ -3,6 +3,7 @@ #include "common/protobuf/protobuf.h" #include "absl/types/optional.h" +#include "absl/strings/string_view.h" namespace Envoy { namespace Config { @@ -23,6 +24,10 @@ class ApiTypeOracle { static const absl::optional getEarlierVersionMessageTypeName(const std::string& message_type); + static const absl::string_view typeUrlToDescriptorFullName(absl::string_view type_url); + + static const std::string descriptorFullNameToTypeUrl(std::string& type); + static const absl::optional getEarlierTypeUrl(const std::string& type_url); }; From a8eae1d880f2291c480e6efc6db803c84fe77374 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Thu, 17 Sep 2020 17:58:33 +0000 Subject: [PATCH 22/32] refactor Signed-off-by: chaoqinli --- .../common/config/filesystem_subscription_impl.cc | 4 ++-- source/common/config/utility.cc | 3 ++- source/common/config/utility.h | 6 ++++-- source/common/http/request_id_extension_impl.cc | 3 ++- source/common/protobuf/utility.cc | 13 +++---------- source/common/protobuf/utility.h | 5 ----- source/extensions/filters/http/cache/config.cc | 3 ++- source/extensions/filters/http/compressor/config.cc | 2 +- .../extensions/filters/http/decompressor/config.cc | 2 +- .../network/http_connection_manager/config.cc | 2 +- source/server/admin/config_dump_handler.cc | 2 +- test/common/config/api_type_oracle_test.cc | 3 +++ test/common/protobuf/utility_test.cc | 6 ------ 13 files changed, 22 insertions(+), 32 deletions(-) diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index 13d3829d1cde..3c89af0293d0 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -97,8 +97,8 @@ FilesystemCollectionSubscriptionImpl::refreshInternal(ProtobufTypes::MessagePtr* auto& resource_message = *owned_resource_message; MessageUtil::loadFromFile(path_, resource_message, validation_visitor_, api_); // Dynamically load the collection message. - const std::string collection_type = - std::string(TypeUtil::typeUrlToDescriptorFullName(resource_message.resource().type_url())); + const std::string collection_type = std::string( + Config::ApiTypeOracle::typeUrlToDescriptorFullName(resource_message.resource().type_url())); const Protobuf::Descriptor* collection_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(collection_type); if (collection_descriptor == nullptr) { diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index e42ee777a156..d9ec34245e59 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -271,7 +271,8 @@ void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config, if (!typed_config.value().empty()) { // Unpack methods will only use the fully qualified type name after the last '/'. // https://github.com/protocolbuffers/protobuf/blob/3.6.x/src/google/protobuf/any.proto#L87 - absl::string_view type = TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url()); + absl::string_view type = + Config::ApiTypeOracle::typeUrlToDescriptorFullName(typed_config.type_url()); if (type == typed_struct_type) { udpa::type::v1::TypedStruct typed_struct; diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 61b629f927f1..6e6674bceba0 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -255,12 +255,14 @@ class Utility { udpa::type::v1::TypedStruct::default_instance().GetDescriptor()->full_name(); // Unpack methods will only use the fully qualified type name after the last '/'. // https://github.com/protocolbuffers/protobuf/blob/3.6.x/src/google/protobuf/any.proto#L87 - auto type = std::string(TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url())); + auto type = + std::string(Config::ApiTypeOracle::typeUrlToDescriptorFullName(typed_config.type_url())); if (type == typed_struct_type) { udpa::type::v1::TypedStruct typed_struct; MessageUtil::unpackTo(typed_config, typed_struct); // Not handling nested structs or typed structs in typed structs - return std::string(TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url())); + return std::string( + Config::ApiTypeOracle::typeUrlToDescriptorFullName(typed_struct.type_url())); } return type; } diff --git a/source/common/http/request_id_extension_impl.cc b/source/common/http/request_id_extension_impl.cc index f2917959bcd1..63b4177d8f83 100644 --- a/source/common/http/request_id_extension_impl.cc +++ b/source/common/http/request_id_extension_impl.cc @@ -25,7 +25,8 @@ RequestIDExtensionSharedPtr RequestIDExtensionFactory::fromProto( const envoy::extensions::filters::network::http_connection_manager::v3::RequestIDExtension& config, Server::Configuration::FactoryContext& context) { - const std::string type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; + const std::string type{ + Config::ApiTypeOracle::typeUrlToDescriptorFullName(config.typed_config().type_url())}; auto* factory = Registry::FactoryRegistry::getFactoryByType( type); diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index a986c6732e2f..837b11d4377a 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -581,7 +581,7 @@ std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& messa void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message) { // If we don't have a type URL match, try an earlier version. const absl::string_view any_full_name = - TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); + Config::ApiTypeOracle::typeUrlToDescriptorFullName(any_message.type_url()); if (any_full_name != message.GetDescriptor()->full_name()) { const Protobuf::Descriptor* earlier_version_desc = Config::ApiTypeOracle::getEarlierVersionDescriptor(message.GetDescriptor()->full_name()); @@ -686,7 +686,8 @@ bool redactOpaque(Protobuf::Message* message, bool ancestor_is_sensitive, // Try to find a descriptor for `type_url` in the pool and instantiate a new message of the // correct concrete type. const std::string type_url(reflection->GetString(*message, type_url_field_descriptor)); - const std::string concrete_type_name(TypeUtil::typeUrlToDescriptorFullName(type_url)); + const std::string concrete_type_name( + Config::ApiTypeOracle::typeUrlToDescriptorFullName(type_url)); const auto* concrete_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(concrete_type_name); if (concrete_descriptor == nullptr) { @@ -947,12 +948,4 @@ void TimestampUtil::systemClockToTimestamp(const SystemTime system_clock_time, .count())); } -absl::string_view TypeUtil::typeUrlToDescriptorFullName(absl::string_view type_url) { - const size_t pos = type_url.rfind('/'); - if (pos != absl::string_view::npos) { - type_url = type_url.substr(pos + 1); - } - return type_url; -} - } // namespace Envoy diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 3ba16b3bb910..38519e719068 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -136,11 +136,6 @@ class MissingFieldException : public EnvoyException { MissingFieldException(const std::string& field_name, const Protobuf::Message& message); }; -class TypeUtil { -public: - static absl::string_view typeUrlToDescriptorFullName(absl::string_view type_url); -}; - class RepeatedPtrUtil { public: static std::string join(const Protobuf::RepeatedPtrField& source, diff --git a/source/extensions/filters/http/cache/config.cc b/source/extensions/filters/http/cache/config.cc index 86277189a789..51be94e73c0e 100644 --- a/source/extensions/filters/http/cache/config.cc +++ b/source/extensions/filters/http/cache/config.cc @@ -10,7 +10,8 @@ namespace Cache { Http::FilterFactoryCb CacheFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { - const std::string type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; + const std::string type{ + Config::ApiTypeOracle::typeUrlToDescriptorFullName(config.typed_config().type_url())}; HttpCacheFactory* const http_cache_factory = Registry::FactoryRegistry::getFactoryByType(type); if (http_cache_factory == nullptr) { diff --git a/source/extensions/filters/http/compressor/config.cc b/source/extensions/filters/http/compressor/config.cc index aff3ca5afe4c..6ed29766219d 100644 --- a/source/extensions/filters/http/compressor/config.cc +++ b/source/extensions/filters/http/compressor/config.cc @@ -19,7 +19,7 @@ Http::FilterFactoryCb CompressorFilterFactory::createFilterFactoryFromProtoTyped if (!proto_config.has_compressor_library()) { throw EnvoyException("Compressor filter doesn't have compressor_library defined"); } - const std::string type{TypeUtil::typeUrlToDescriptorFullName( + const std::string type{Config::ApiTypeOracle::typeUrlToDescriptorFullName( proto_config.compressor_library().typed_config().type_url())}; Compression::Compressor::NamedCompressorLibraryConfigFactory* const config_factory = Registry::FactoryRegistry< diff --git a/source/extensions/filters/http/decompressor/config.cc b/source/extensions/filters/http/decompressor/config.cc index fb52ae85c216..6b7927fa5a89 100644 --- a/source/extensions/filters/http/decompressor/config.cc +++ b/source/extensions/filters/http/decompressor/config.cc @@ -14,7 +14,7 @@ namespace Decompressor { Http::FilterFactoryCb DecompressorFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::decompressor::v3::Decompressor& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { - const std::string decompressor_library_type{TypeUtil::typeUrlToDescriptorFullName( + const std::string decompressor_library_type{Config::ApiTypeOracle::typeUrlToDescriptorFullName( proto_config.decompressor_library().typed_config().type_url())}; Compression::Decompressor::NamedDecompressorLibraryConfigFactory* const decompressor_library_factory = Registry::FactoryRegistry< diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 82a65448222f..8a6f8f500e25 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -523,7 +523,7 @@ void HttpConnectionManagerConfig::processDynamicFilterConfig( } std::set require_type_urls; for (const auto& type_url : config_discovery.type_urls()) { - auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); + auto factory_type_url = Config::ApiTypeOracle::typeUrlToDescriptorFullName(type_url); require_type_urls.emplace(factory_type_url); auto* factory = Registry::FactoryRegistry< Server::Configuration::NamedHttpFilterConfigFactory>::getFactoryByType(factory_type_url); diff --git a/source/server/admin/config_dump_handler.cc b/source/server/admin/config_dump_handler.cc index dbfd13a01e2e..c1be9b1b2812 100644 --- a/source/server/admin/config_dump_handler.cc +++ b/source/server/admin/config_dump_handler.cc @@ -79,7 +79,7 @@ void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Messag any_message.MergeFrom(reflection->GetMessage(message, any_field)); Protobuf::DynamicMessageFactory dmf; const absl::string_view inner_type_name = - TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); + Config::ApiTypeOracle::typeUrlToDescriptorFullName(any_message.type_url()); const Protobuf::Descriptor* inner_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( static_cast(inner_type_name)); diff --git a/test/common/config/api_type_oracle_test.cc b/test/common/config/api_type_oracle_test.cc index 327d4dc32e54..246a91a90cc5 100644 --- a/test/common/config/api_type_oracle_test.cc +++ b/test/common/config/api_type_oracle_test.cc @@ -27,6 +27,9 @@ TEST(ApiTypeOracleTest, All) { EXPECT_EQ(envoy::config::filter::http::ip_tagging::v2::IPTagging::descriptor()->full_name(), ApiTypeOracle::getEarlierVersionMessageTypeName(v3_config.GetDescriptor()->full_name()) .value()); + EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", + ApiTypeOracle::typeUrlToDescriptorFullName( + "type.googleapis.com/envoy.config.filter.http.ip_tagging.v2.IPTagging")); } } // namespace diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 2132fd25e2d2..5a199800d71d 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1772,10 +1772,4 @@ TEST(StatusCode, Strings) { ASSERT_EQ("OK", MessageUtil::CodeEnumToString(ProtobufUtil::error::OK)); } -TEST(TypeUtilTest, TypeUrlToDescriptorFullName) { - EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", - TypeUtil::typeUrlToDescriptorFullName( - "type.googleapis.com/envoy.config.filter.http.ip_tagging.v2.IPTagging")); -} - } // namespace Envoy From 1278fdab754f3a93fc80e3c07c17187be46d3d6d Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Thu, 17 Sep 2020 19:53:14 +0000 Subject: [PATCH 23/32] format Signed-off-by: chaoqinli --- source/common/config/api_type_oracle.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/config/api_type_oracle.h b/source/common/config/api_type_oracle.h index 65a3af31641e..ddaa432a6f0f 100644 --- a/source/common/config/api_type_oracle.h +++ b/source/common/config/api_type_oracle.h @@ -2,8 +2,8 @@ #include "common/protobuf/protobuf.h" -#include "absl/types/optional.h" #include "absl/strings/string_view.h" +#include "absl/types/optional.h" namespace Envoy { namespace Config { From ca3ece48deedd8c8576592610de0ad416e3df105 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Thu, 17 Sep 2020 22:51:53 +0000 Subject: [PATCH 24/32] clean up Signed-off-by: chaoqinli --- test/common/config/grpc_mux_impl_test.cc | 138 +++---------------- test/common/config/new_grpc_mux_impl_test.cc | 15 +- test/integration/ads_integration_test.cc | 8 +- 3 files changed, 34 insertions(+), 127 deletions(-) diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 36aebc75f9d1..8c869aa44b1f 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -731,30 +731,27 @@ TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { Runtime::LoaderSingleton::getExisting()->mergeValues( {{"envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", "true"}}); setup(); + InSequence s; - TestUtility::TestOpaqueResourceDecoderImpl - resource_decoder("cluster_name"); const std::string& v2_type_url = Config::TypeUrl::get().ClusterLoadAssignment; const std::string& v3_type_url = - "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"; - NiceMock foo_callbacks; - auto foo_sub = grpc_mux_->addWatch(v2_type_url, {"x", "y"}, foo_callbacks, resource_decoder); - NiceMock bar_callbacks; - auto bar_sub = grpc_mux_->addWatch(v2_type_url, {"y", "z"}, bar_callbacks, resource_decoder); + Config::getTypeUrl( + envoy::config::core::v3::ApiVersion::V3); + TestUtility::TestOpaqueResourceDecoderImpl + resource_decoder("cluster_name"); + auto foo_sub = grpc_mux_->addWatch(v2_type_url, {}, callbacks_, resource_decoder); EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); - expectSendMessage(v2_type_url, {"y", "z", "x"}, "", true); + expectSendMessage(v2_type_url, {}, "", true); grpc_mux_->start(); { - // Send resource with v3 type url, should invoke onConfigUpdate. auto response = std::make_unique(); response->set_type_url(v3_type_url); response->set_version_info("1"); envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment; load_assignment.set_cluster_name("x"); response->add_resources()->PackFrom(load_assignment); - EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "1")).Times(0); - EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "1")) + EXPECT_CALL(callbacks_, onConfigUpdate(_, "1")) .WillOnce(Invoke([&load_assignment](const std::vector& resources, const std::string&) { EXPECT_EQ(1, resources.size()); @@ -763,56 +760,9 @@ TEST_F(GrpcMuxImplTest, WatchV2ResourceV3) { resources[0].get().resource()); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); })); - // Send request with v2 type url resources. - expectSendMessage(v2_type_url, {"y", "z", "x"}, "1"); - grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); - } - - { - auto response = std::make_unique(); - response->set_type_url(v3_type_url); - response->set_version_info("2"); - envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_x; - load_assignment_x.set_cluster_name("x"); - response->add_resources()->PackFrom(load_assignment_x); - envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_y; - load_assignment_y.set_cluster_name("y"); - response->add_resources()->PackFrom(load_assignment_y); - envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_z; - load_assignment_z.set_cluster_name("z"); - response->add_resources()->PackFrom(load_assignment_z); - EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "2")) - .WillOnce(Invoke([&load_assignment_y, &load_assignment_z]( - const std::vector& resources, const std::string&) { - EXPECT_EQ(2, resources.size()); - const auto& expected_assignment = - dynamic_cast( - resources[0].get().resource()); - EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_y)); - const auto& expected_assignment_1 = - dynamic_cast( - resources[1].get().resource()); - EXPECT_TRUE(TestUtility::protoEqual(expected_assignment_1, load_assignment_z)); - })); - EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "2")) - .WillOnce(Invoke([&load_assignment_x, &load_assignment_y]( - const std::vector& resources, const std::string&) { - EXPECT_EQ(2, resources.size()); - const auto& expected_assignment = - dynamic_cast( - resources[0].get().resource()); - EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_x)); - const auto& expected_assignment_1 = - dynamic_cast( - resources[1].get().resource()); - EXPECT_TRUE(TestUtility::protoEqual(expected_assignment_1, load_assignment_y)); - })); - expectSendMessage(v2_type_url, {"y", "z", "x"}, "2"); + expectSendMessage(v2_type_url, {}, "1"); grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); } - - expectSendMessage(v2_type_url, {"x", "y"}, "2"); - expectSendMessage(v2_type_url, {}, "2"); } // Send discovery request with v3 resource type_url, receive discovery response with v2 resource @@ -822,31 +772,28 @@ TEST_F(GrpcMuxImplTest, WatchV3ResourceV2) { Runtime::LoaderSingleton::getExisting()->mergeValues( {{"envoy.reloadable_features.enable_type_url_downgrade_and_upgrade", "true"}}); setup(); + InSequence s; - TestUtility::TestOpaqueResourceDecoderImpl - resource_decoder("cluster_name"); const std::string& v2_type_url = Config::TypeUrl::get().ClusterLoadAssignment; const std::string& v3_type_url = - "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"; - NiceMock foo_callbacks; - // Watch v3 type url. - auto foo_sub = grpc_mux_->addWatch(v3_type_url, {"x", "y"}, foo_callbacks, resource_decoder); - NiceMock bar_callbacks; - auto bar_sub = grpc_mux_->addWatch(v3_type_url, {"y", "z"}, bar_callbacks, resource_decoder); + Config::getTypeUrl( + envoy::config::core::v3::ApiVersion::V3); + TestUtility::TestOpaqueResourceDecoderImpl + resource_decoder("cluster_name"); + auto foo_sub = grpc_mux_->addWatch(v3_type_url, {}, callbacks_, resource_decoder); EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); - expectSendMessage(v3_type_url, {"y", "z", "x"}, "", true); + expectSendMessage(v3_type_url, {}, "", true); grpc_mux_->start(); { - // Send resource with v2 type url, should invoke onConfigUpdate. + auto response = std::make_unique(); response->set_type_url(v2_type_url); response->set_version_info("1"); envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment; load_assignment.set_cluster_name("x"); response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment)); - EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "1")).Times(0); - EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "1")) + EXPECT_CALL(callbacks_, onConfigUpdate(_, "1")) .WillOnce(Invoke([&load_assignment](const std::vector& resources, const std::string&) { EXPECT_EQ(1, resources.size()); @@ -855,56 +802,9 @@ TEST_F(GrpcMuxImplTest, WatchV3ResourceV2) { resources[0].get().resource()); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); })); - // Send request with v2 type url resources. - expectSendMessage(v3_type_url, {"y", "z", "x"}, "1"); - grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); - } - - { - auto response = std::make_unique(); - response->set_type_url(v2_type_url); - response->set_version_info("2"); - envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_x; - load_assignment_x.set_cluster_name("x"); - response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_x)); - envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_y; - load_assignment_y.set_cluster_name("y"); - response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_y)); - envoy::config::endpoint::v3::ClusterLoadAssignment load_assignment_z; - load_assignment_z.set_cluster_name("z"); - response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_z)); - EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "2")) - .WillOnce(Invoke([&load_assignment_y, &load_assignment_z]( - const std::vector& resources, const std::string&) { - EXPECT_EQ(2, resources.size()); - const auto& expected_assignment = - dynamic_cast( - resources[0].get().resource()); - EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_y)); - const auto& expected_assignment_1 = - dynamic_cast( - resources[1].get().resource()); - EXPECT_TRUE(TestUtility::protoEqual(expected_assignment_1, load_assignment_z)); - })); - EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "2")) - .WillOnce(Invoke([&load_assignment_x, &load_assignment_y]( - const std::vector& resources, const std::string&) { - EXPECT_EQ(2, resources.size()); - const auto& expected_assignment = - dynamic_cast( - resources[0].get().resource()); - EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_x)); - const auto& expected_assignment_1 = - dynamic_cast( - resources[1].get().resource()); - EXPECT_TRUE(TestUtility::protoEqual(expected_assignment_1, load_assignment_y)); - })); - expectSendMessage(v3_type_url, {"y", "z", "x"}, "2"); + expectSendMessage(v3_type_url, {}, "1"); grpc_mux_->grpcStreamForTest().onReceiveMessage(std::move(response)); } - - expectSendMessage(v3_type_url, {"x", "y"}, "2"); - expectSendMessage(v3_type_url, {}, "2"); } } // namespace diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index 0f96594cae39..2bcf1ecd75de 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -177,8 +177,11 @@ TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { setup(); // Watch for v2 resource type_url. - const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; - auto watch = grpc_mux_->addWatch(type_url, {}, callbacks_, resource_decoder_); + const std::string& v2_type_url = Config::TypeUrl::get().ClusterLoadAssignment; + const std::string& v3_type_url = + Config::getTypeUrl( + envoy::config::core::v3::ApiVersion::V3); + auto watch = grpc_mux_->addWatch(v2_type_url, {}, callbacks_, resource_decoder_); EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_)); // Cluster is not watched, v3 resource is rejected. @@ -187,7 +190,8 @@ TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { auto unexpected_response = std::make_unique(); envoy::config::cluster::v3::Cluster cluster; - unexpected_response->set_type_url("type.googleapis.com/envoy.config.cluster.v3.Cluster"); + unexpected_response->set_type_url(Config::getTypeUrl( + envoy::config::core::v3::ApiVersion::V3)); unexpected_response->set_system_version_info("0"); unexpected_response->add_resources()->mutable_resource()->PackFrom(cluster); EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "0")).Times(0); @@ -212,7 +216,7 @@ TEST_F(NewGrpcMuxImplTest, V3ResourceResponseV2ResourceWatch) { load_assignment.set_cluster_name("x"); response->add_resources()->mutable_resource()->PackFrom(load_assignment); // Send response that contains resource with v3 type url. - response->set_type_url("type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"); + response->set_type_url(v3_type_url); EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "1")) .WillOnce(Invoke([&load_assignment](const std::vector& added_resources, const Protobuf::RepeatedPtrField&, @@ -234,7 +238,8 @@ TEST_F(NewGrpcMuxImplTest, V2ResourceResponseV3ResourceWatch) { // Watch for v3 resource type_url. const std::string& v3_type_url = - "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment"; + Config::getTypeUrl( + envoy::config::core::v3::ApiVersion::V3); const std::string& v2_type_url = Config::TypeUrl::get().ClusterLoadAssignment; auto watch = grpc_mux_->addWatch(v3_type_url, {}, callbacks_, resource_decoder_); diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 58a81ba8122a..b49b217464bf 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -124,15 +124,17 @@ TEST_P(AdsIntegrationTest, MixV2V3TypeUrlInDiscoveryResponse) { // Send initial configuration. // Discovery response with v3 type url. sendDiscoveryResponse( - "type.googleapis.com/envoy.config.cluster.v3.Cluster", {buildCluster("cluster_0")}, - {buildCluster("cluster_0")}, {}, "1", false); + Config::getTypeUrl( + envoy::config::core::v3::ApiVersion::V3), + {buildCluster("cluster_0")}, {buildCluster("cluster_0")}, {}, "1", false); // Discovery response with v2 type url. sendDiscoveryResponse( Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("cluster_0")}, {buildClusterLoadAssignment("cluster_0")}, {}, "1"); // Discovery response with v3 type url. sendDiscoveryResponse( - "type.googleapis.com/envoy.config.listener.v3.Listener", + Config::getTypeUrl( + envoy::config::core::v3::ApiVersion::V3), {buildListener("listener_0", "route_config_0")}, {buildListener("listener_0", "route_config_0")}, {}, "1", false); // Discovery response with v2 type url. From bfd7e8709f6046ad1811bb678f80798b427e3fd8 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Fri, 18 Sep 2020 16:18:08 +0000 Subject: [PATCH 25/32] test coverage Signed-off-by: chaoqinli --- test/common/protobuf/utility_test.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 5a199800d71d..99cfc1bf6509 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -45,17 +45,18 @@ TEST_F(ProtobufUtilityTest, ConvertPercentNaNDouble) { envoy::config::cluster::v3::Cluster::CommonLbConfig common_config_; common_config_.mutable_healthy_panic_threshold()->set_value( std::numeric_limits::quiet_NaN()); - EXPECT_THROW(PROTOBUF_PERCENT_TO_DOUBLE_OR_DEFAULT(common_config_, healthy_panic_threshold, 0.5), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + PROTOBUF_PERCENT_TO_DOUBLE_OR_DEFAULT(common_config_, healthy_panic_threshold, 0.5), + EnvoyException, "Value not in the range of 0..100 range."); } TEST_F(ProtobufUtilityTest, ConvertPercentNaN) { envoy::config::cluster::v3::Cluster::CommonLbConfig common_config_; common_config_.mutable_healthy_panic_threshold()->set_value( std::numeric_limits::quiet_NaN()); - EXPECT_THROW(PROTOBUF_PERCENT_TO_ROUNDED_INTEGER_OR_DEFAULT(common_config_, - healthy_panic_threshold, 100, 50), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE(PROTOBUF_PERCENT_TO_ROUNDED_INTEGER_OR_DEFAULT( + common_config_, healthy_panic_threshold, 100, 50), + EnvoyException, "Value not in the range of 0..100 range."); } namespace ProtobufPercentHelper { From 804721d0e2001af97500866c27dace78f8cbe15e Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Sat, 19 Sep 2020 05:55:27 +0000 Subject: [PATCH 26/32] type util test Signed-off-by: chaoqinli --- source/common/config/BUILD | 1 + source/common/config/api_type_oracle.cc | 16 ++-------------- source/common/config/api_type_oracle.h | 5 +---- .../config/filesystem_subscription_impl.cc | 2 +- source/common/config/utility.cc | 3 +-- source/common/config/utility.h | 6 ++---- .../common/http/request_id_extension_impl.cc | 2 +- source/common/protobuf/BUILD | 12 ++++++++++++ source/common/protobuf/type_util.cc | 19 +++++++++++++++++++ source/common/protobuf/type_util.h | 17 +++++++++++++++++ source/common/protobuf/utility.cc | 5 ++--- .../extensions/filters/http/cache/config.cc | 2 +- .../filters/http/compressor/config.cc | 2 +- .../filters/http/decompressor/config.cc | 2 +- .../network/http_connection_manager/config.cc | 2 +- source/server/admin/config_dump_handler.cc | 2 +- test/common/config/api_type_oracle_test.cc | 2 +- test/common/protobuf/BUILD | 8 ++++++++ test/common/protobuf/type_util_test.cc | 18 ++++++++++++++++++ 19 files changed, 91 insertions(+), 35 deletions(-) create mode 100644 source/common/protobuf/type_util.cc create mode 100644 source/common/protobuf/type_util.h create mode 100644 test/common/protobuf/type_util_test.cc diff --git a/source/common/config/BUILD b/source/common/config/BUILD index d73a0f3fbeaf..61d0bf4b4445 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -14,6 +14,7 @@ envoy_cc_library( hdrs = ["api_type_oracle.h"], deps = [ "//source/common/protobuf", + "//source/common/protobuf:type_util_lib", "@com_github_cncf_udpa//udpa/annotations:pkg_cc_proto", ], ) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index 21575aea93a5..6c39d9c06d6a 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -32,23 +32,11 @@ ApiTypeOracle::getEarlierVersionMessageTypeName(const std::string& message_type) return absl::nullopt; } -const absl::string_view ApiTypeOracle::typeUrlToDescriptorFullName(absl::string_view type_url) { - const size_t pos = type_url.rfind('/'); - if (pos != absl::string_view::npos) { - type_url = type_url.substr(pos + 1); - } - return type_url; -} - -const std::string ApiTypeOracle::descriptorFullNameToTypeUrl(std::string& type) { - return "type.googleapis.com/" + type; -} - const absl::optional ApiTypeOracle::getEarlierTypeUrl(const std::string& type_url) { - const std::string type{typeUrlToDescriptorFullName(type_url)}; + const std::string type{Config::TypeUtil::typeUrlToDescriptorFullName(type_url)}; absl::optional old_type = ApiTypeOracle::getEarlierVersionMessageTypeName(type); if (old_type.has_value()) { - return descriptorFullNameToTypeUrl(old_type.value()); + return TypeUtil::descriptorFullNameToTypeUrl(old_type.value()); } return {}; } diff --git a/source/common/config/api_type_oracle.h b/source/common/config/api_type_oracle.h index ddaa432a6f0f..ab5e75b113d7 100644 --- a/source/common/config/api_type_oracle.h +++ b/source/common/config/api_type_oracle.h @@ -1,6 +1,7 @@ #pragma once #include "common/protobuf/protobuf.h" +#include "common/protobuf/type_util.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -24,10 +25,6 @@ class ApiTypeOracle { static const absl::optional getEarlierVersionMessageTypeName(const std::string& message_type); - static const absl::string_view typeUrlToDescriptorFullName(absl::string_view type_url); - - static const std::string descriptorFullNameToTypeUrl(std::string& type); - static const absl::optional getEarlierTypeUrl(const std::string& type_url); }; diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index 3c89af0293d0..4a20df0c907e 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -98,7 +98,7 @@ FilesystemCollectionSubscriptionImpl::refreshInternal(ProtobufTypes::MessagePtr* MessageUtil::loadFromFile(path_, resource_message, validation_visitor_, api_); // Dynamically load the collection message. const std::string collection_type = std::string( - Config::ApiTypeOracle::typeUrlToDescriptorFullName(resource_message.resource().type_url())); + Config::TypeUtil::typeUrlToDescriptorFullName(resource_message.resource().type_url())); const Protobuf::Descriptor* collection_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(collection_type); if (collection_descriptor == nullptr) { diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index d9ec34245e59..7d5ab2bc3e89 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -271,8 +271,7 @@ void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config, if (!typed_config.value().empty()) { // Unpack methods will only use the fully qualified type name after the last '/'. // https://github.com/protocolbuffers/protobuf/blob/3.6.x/src/google/protobuf/any.proto#L87 - absl::string_view type = - Config::ApiTypeOracle::typeUrlToDescriptorFullName(typed_config.type_url()); + absl::string_view type = Config::TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url()); if (type == typed_struct_type) { udpa::type::v1::TypedStruct typed_struct; diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 6e6674bceba0..7f8a6bca2640 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -255,14 +255,12 @@ class Utility { udpa::type::v1::TypedStruct::default_instance().GetDescriptor()->full_name(); // Unpack methods will only use the fully qualified type name after the last '/'. // https://github.com/protocolbuffers/protobuf/blob/3.6.x/src/google/protobuf/any.proto#L87 - auto type = - std::string(Config::ApiTypeOracle::typeUrlToDescriptorFullName(typed_config.type_url())); + auto type = std::string(Config::TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url())); if (type == typed_struct_type) { udpa::type::v1::TypedStruct typed_struct; MessageUtil::unpackTo(typed_config, typed_struct); // Not handling nested structs or typed structs in typed structs - return std::string( - Config::ApiTypeOracle::typeUrlToDescriptorFullName(typed_struct.type_url())); + return std::string(Config::TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url())); } return type; } diff --git a/source/common/http/request_id_extension_impl.cc b/source/common/http/request_id_extension_impl.cc index 63b4177d8f83..97499f15eebf 100644 --- a/source/common/http/request_id_extension_impl.cc +++ b/source/common/http/request_id_extension_impl.cc @@ -26,7 +26,7 @@ RequestIDExtensionSharedPtr RequestIDExtensionFactory::fromProto( config, Server::Configuration::FactoryContext& context) { const std::string type{ - Config::ApiTypeOracle::typeUrlToDescriptorFullName(config.typed_config().type_url())}; + Config::TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; auto* factory = Registry::FactoryRegistry::getFactoryByType( type); diff --git a/source/common/protobuf/BUILD b/source/common/protobuf/BUILD index f505161b810f..5b5d16a9c4d5 100644 --- a/source/common/protobuf/BUILD +++ b/source/common/protobuf/BUILD @@ -61,6 +61,7 @@ envoy_cc_library( deps = [ ":message_validator_lib", ":protobuf", + ":type_util_lib", ":well_known_lib", "//include/envoy/api:api_interface", "//include/envoy/protobuf:message_validator_interface", @@ -78,6 +79,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "type_util_lib", + srcs = ["type_util.cc"], + hdrs = ["type_util.h"], + deps = [ + "//source/common/protobuf", + "@com_github_cncf_udpa//udpa/annotations:pkg_cc_proto", + ], +) + + envoy_cc_library( name = "visitor_lib", srcs = ["visitor.cc"], diff --git a/source/common/protobuf/type_util.cc b/source/common/protobuf/type_util.cc new file mode 100644 index 000000000000..5cda3bffc46a --- /dev/null +++ b/source/common/protobuf/type_util.cc @@ -0,0 +1,19 @@ +#include "common/protobuf/type_util.h" + +namespace Envoy { +namespace Config { + +absl::string_view TypeUtil::typeUrlToDescriptorFullName(absl::string_view type_url) { + const size_t pos = type_url.rfind('/'); + if (pos != absl::string_view::npos) { + type_url = type_url.substr(pos + 1); + } + return type_url; +} + +std::string TypeUtil::descriptorFullNameToTypeUrl(absl::string_view type) { + return "type.googleapis.com/" + std::string(type); +} + +} // namespace Config +} // namespace Envoy \ No newline at end of file diff --git a/source/common/protobuf/type_util.h b/source/common/protobuf/type_util.h new file mode 100644 index 000000000000..eccc000d3a0d --- /dev/null +++ b/source/common/protobuf/type_util.h @@ -0,0 +1,17 @@ +#pragma once + +#include "common/protobuf/protobuf.h" + +#include "absl/strings/string_view.h" +#include "absl/types/optional.h" + +namespace Envoy { +namespace Config { +class TypeUtil { +public: + static absl::string_view typeUrlToDescriptorFullName(absl::string_view type_url); + + static std::string descriptorFullNameToTypeUrl(absl::string_view type); +}; +} // namespace Config +} // namespace Envoy \ No newline at end of file diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 837b11d4377a..810a1d216b8c 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -581,7 +581,7 @@ std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& messa void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message) { // If we don't have a type URL match, try an earlier version. const absl::string_view any_full_name = - Config::ApiTypeOracle::typeUrlToDescriptorFullName(any_message.type_url()); + Config::TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); if (any_full_name != message.GetDescriptor()->full_name()) { const Protobuf::Descriptor* earlier_version_desc = Config::ApiTypeOracle::getEarlierVersionDescriptor(message.GetDescriptor()->full_name()); @@ -686,8 +686,7 @@ bool redactOpaque(Protobuf::Message* message, bool ancestor_is_sensitive, // Try to find a descriptor for `type_url` in the pool and instantiate a new message of the // correct concrete type. const std::string type_url(reflection->GetString(*message, type_url_field_descriptor)); - const std::string concrete_type_name( - Config::ApiTypeOracle::typeUrlToDescriptorFullName(type_url)); + const std::string concrete_type_name(Config::TypeUtil::typeUrlToDescriptorFullName(type_url)); const auto* concrete_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(concrete_type_name); if (concrete_descriptor == nullptr) { diff --git a/source/extensions/filters/http/cache/config.cc b/source/extensions/filters/http/cache/config.cc index 51be94e73c0e..7bfaeb0c15a2 100644 --- a/source/extensions/filters/http/cache/config.cc +++ b/source/extensions/filters/http/cache/config.cc @@ -11,7 +11,7 @@ Http::FilterFactoryCb CacheFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { const std::string type{ - Config::ApiTypeOracle::typeUrlToDescriptorFullName(config.typed_config().type_url())}; + Config::TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; HttpCacheFactory* const http_cache_factory = Registry::FactoryRegistry::getFactoryByType(type); if (http_cache_factory == nullptr) { diff --git a/source/extensions/filters/http/compressor/config.cc b/source/extensions/filters/http/compressor/config.cc index 6ed29766219d..8a9500b20169 100644 --- a/source/extensions/filters/http/compressor/config.cc +++ b/source/extensions/filters/http/compressor/config.cc @@ -19,7 +19,7 @@ Http::FilterFactoryCb CompressorFilterFactory::createFilterFactoryFromProtoTyped if (!proto_config.has_compressor_library()) { throw EnvoyException("Compressor filter doesn't have compressor_library defined"); } - const std::string type{Config::ApiTypeOracle::typeUrlToDescriptorFullName( + const std::string type{Config::TypeUtil::typeUrlToDescriptorFullName( proto_config.compressor_library().typed_config().type_url())}; Compression::Compressor::NamedCompressorLibraryConfigFactory* const config_factory = Registry::FactoryRegistry< diff --git a/source/extensions/filters/http/decompressor/config.cc b/source/extensions/filters/http/decompressor/config.cc index 6b7927fa5a89..585073eb5ad5 100644 --- a/source/extensions/filters/http/decompressor/config.cc +++ b/source/extensions/filters/http/decompressor/config.cc @@ -14,7 +14,7 @@ namespace Decompressor { Http::FilterFactoryCb DecompressorFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::decompressor::v3::Decompressor& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { - const std::string decompressor_library_type{Config::ApiTypeOracle::typeUrlToDescriptorFullName( + const std::string decompressor_library_type{Config::TypeUtil::typeUrlToDescriptorFullName( proto_config.decompressor_library().typed_config().type_url())}; Compression::Decompressor::NamedDecompressorLibraryConfigFactory* const decompressor_library_factory = Registry::FactoryRegistry< diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 8a6f8f500e25..18d7843f347a 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -523,7 +523,7 @@ void HttpConnectionManagerConfig::processDynamicFilterConfig( } std::set require_type_urls; for (const auto& type_url : config_discovery.type_urls()) { - auto factory_type_url = Config::ApiTypeOracle::typeUrlToDescriptorFullName(type_url); + auto factory_type_url = Config::TypeUtil::typeUrlToDescriptorFullName(type_url); require_type_urls.emplace(factory_type_url); auto* factory = Registry::FactoryRegistry< Server::Configuration::NamedHttpFilterConfigFactory>::getFactoryByType(factory_type_url); diff --git a/source/server/admin/config_dump_handler.cc b/source/server/admin/config_dump_handler.cc index c1be9b1b2812..4e00beb1c49a 100644 --- a/source/server/admin/config_dump_handler.cc +++ b/source/server/admin/config_dump_handler.cc @@ -79,7 +79,7 @@ void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Messag any_message.MergeFrom(reflection->GetMessage(message, any_field)); Protobuf::DynamicMessageFactory dmf; const absl::string_view inner_type_name = - Config::ApiTypeOracle::typeUrlToDescriptorFullName(any_message.type_url()); + Config::TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); const Protobuf::Descriptor* inner_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( static_cast(inner_type_name)); diff --git a/test/common/config/api_type_oracle_test.cc b/test/common/config/api_type_oracle_test.cc index 246a91a90cc5..a2454953c3bb 100644 --- a/test/common/config/api_type_oracle_test.cc +++ b/test/common/config/api_type_oracle_test.cc @@ -28,7 +28,7 @@ TEST(ApiTypeOracleTest, All) { ApiTypeOracle::getEarlierVersionMessageTypeName(v3_config.GetDescriptor()->full_name()) .value()); EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", - ApiTypeOracle::typeUrlToDescriptorFullName( + TypeUtil::typeUrlToDescriptorFullName( "type.googleapis.com/envoy.config.filter.http.ip_tagging.v2.IPTagging")); } diff --git a/test/common/protobuf/BUILD b/test/common/protobuf/BUILD index bb018981c290..4aa4300922dc 100644 --- a/test/common/protobuf/BUILD +++ b/test/common/protobuf/BUILD @@ -46,6 +46,14 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "type_util_test", + srcs = ["type_util_test.cc"], + deps = [ + "//source/common/protobuf:type_util_lib", + ], +) + envoy_cc_fuzz_test( name = "value_util_fuzz_test", srcs = ["value_util_fuzz_test.cc"], diff --git a/test/common/protobuf/type_util_test.cc b/test/common/protobuf/type_util_test.cc new file mode 100644 index 000000000000..d78395163637 --- /dev/null +++ b/test/common/protobuf/type_util_test.cc @@ -0,0 +1,18 @@ +#include "common/protobuf/type_util.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Config { +namespace { +TEST(TypeUtilTest, TypeUrlHelperFunction) { + EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", + TypeUtil::typeUrlToDescriptorFullName( + "type.googleapis.com/envoy.config.filter.http.ip_tagging.v2.IPTagging")); + EXPECT_EQ( + "type.googleapis.com/envoy.config.filter.http.ip_tagging.v2.IPTagging", + TypeUtil::descriptorFullNameToTypeUrl("envoy.config.filter.http.ip_tagging.v2.IPTagging")); +} +} // namespace +} // namespace Config +} // namespace Envoy \ No newline at end of file From ca31bf9be2a19270b3950d7e4e7e39ee26153ff7 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Sun, 20 Sep 2020 01:02:31 +0000 Subject: [PATCH 27/32] format Signed-off-by: chaoqinli --- source/common/protobuf/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/protobuf/BUILD b/source/common/protobuf/BUILD index 5b5d16a9c4d5..015177dcc186 100644 --- a/source/common/protobuf/BUILD +++ b/source/common/protobuf/BUILD @@ -89,7 +89,6 @@ envoy_cc_library( ], ) - envoy_cc_library( name = "visitor_lib", srcs = ["visitor.cc"], From fd374798709ac0816f28a672f1a120bf8294c03f Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Mon, 21 Sep 2020 04:23:33 +0000 Subject: [PATCH 28/32] format Signed-off-by: chaoqinli --- source/common/protobuf/type_util.cc | 2 +- source/common/protobuf/type_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/protobuf/type_util.cc b/source/common/protobuf/type_util.cc index 5cda3bffc46a..db497ad2e492 100644 --- a/source/common/protobuf/type_util.cc +++ b/source/common/protobuf/type_util.cc @@ -16,4 +16,4 @@ std::string TypeUtil::descriptorFullNameToTypeUrl(absl::string_view type) { } } // namespace Config -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/source/common/protobuf/type_util.h b/source/common/protobuf/type_util.h index eccc000d3a0d..198bb27cf52a 100644 --- a/source/common/protobuf/type_util.h +++ b/source/common/protobuf/type_util.h @@ -14,4 +14,4 @@ class TypeUtil { static std::string descriptorFullNameToTypeUrl(absl::string_view type); }; } // namespace Config -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From 4617ca25fa22f21b8e42395d25f457b76558802e Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Mon, 21 Sep 2020 04:31:41 +0000 Subject: [PATCH 29/32] add doc Signed-off-by: chaoqinli --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 3aac2f958f23..2b98edc643de 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -64,6 +64,7 @@ Removed Config or Runtime New Features ------------ +* xds: added support for mixed v2/v3 discovery response, which enable type url downgrade and upgrade. This feature is disabled by default and is controlled by runtime guard 'envoy.reloadable_features.enable_type_url_downgrade_and_upgrade'. * access log: added a :ref:`dynamic metadata filter` for access logs, which filters whether to log based on matching dynamic metadata. * access log: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as a response flag. * access log: added support for nested objects in :ref:`JSON logging mode `. From 1d9c95cd7069007680489e4d8ebc343a0813f8fd Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Mon, 21 Sep 2020 04:54:01 +0000 Subject: [PATCH 30/32] format Signed-off-by: chaoqinli --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 2b98edc643de..477cd12f8caa 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -64,7 +64,6 @@ Removed Config or Runtime New Features ------------ -* xds: added support for mixed v2/v3 discovery response, which enable type url downgrade and upgrade. This feature is disabled by default and is controlled by runtime guard 'envoy.reloadable_features.enable_type_url_downgrade_and_upgrade'. * access log: added a :ref:`dynamic metadata filter` for access logs, which filters whether to log based on matching dynamic metadata. * access log: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as a response flag. * access log: added support for nested objects in :ref:`JSON logging mode `. @@ -120,6 +119,7 @@ New Features * watchdog: supports an extension point where actions can be registered to fire on watchdog events such as miss, megamiss, kill and multikill. See ref:`watchdog actions`. * watchdog: watchdog action extension that does cpu profiling. See ref:`Profile Action `. * xds: added :ref:`extension config discovery` support for HTTP filters. +* xds: added support for mixed v2/v3 discovery response, which enable type url downgrade and upgrade. This feature is disabled by default and is controlled by runtime guard 'envoy.reloadable_features.enable_type_url_downgrade_and_upgrade'. * zlib: added option to use `zlib-ng `_ as zlib library. Deprecated From eded1755f8a01288f9d38940fef6cb46a9fcc2a5 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 22 Sep 2020 01:35:17 +0000 Subject: [PATCH 31/32] change doc Signed-off-by: chaoqinli --- docs/root/faq/api/control_plane_version_support.rst | 3 +++ docs/root/version_history/current.rst | 2 +- source/common/config/api_type_oracle.cc | 2 +- source/common/config/filesystem_subscription_impl.cc | 4 ++-- source/common/config/utility.cc | 2 +- source/common/config/utility.h | 4 ++-- source/common/http/request_id_extension_impl.cc | 3 +-- source/common/protobuf/type_util.cc | 2 -- source/common/protobuf/type_util.h | 4 ++-- source/common/protobuf/utility.cc | 4 ++-- source/extensions/filters/http/cache/config.cc | 3 +-- source/extensions/filters/http/compressor/config.cc | 2 +- source/extensions/filters/http/decompressor/config.cc | 2 +- .../filters/network/http_connection_manager/config.cc | 2 +- source/server/admin/config_dump_handler.cc | 2 +- 15 files changed, 20 insertions(+), 21 deletions(-) diff --git a/docs/root/faq/api/control_plane_version_support.rst b/docs/root/faq/api/control_plane_version_support.rst index d7b360db2b2a..d718541ee431 100644 --- a/docs/root/faq/api/control_plane_version_support.rst +++ b/docs/root/faq/api/control_plane_version_support.rst @@ -34,6 +34,9 @@ typical rollout sequence might look like: v3 resource type url. Client can also send discovery request with v3 resource type url and process discovery response with v2 resource type url. The upgrade and downgrade of type url is disabled by default and protected by a runtime guard *envoy.reloadable_features.enable_type_url_downgrade_and_upgrade*. + If your management server does not support both v2/v3 at the same time, you can perform a rollout by enabling + this feature, which allow you to rollout a v3-only management server in the presence of v2-only management servers + and have the clients work with both. If you are operating a managed control plane as-a-service, you will likely need to support a wide range of client versions. In this scenario, you will require long term support for multiple major diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 165c40355500..16f3f9e3c319 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -127,7 +127,7 @@ New Features * watchdog: watchdog action extension that does cpu profiling. See ref:`Profile Action `. * watchdog: watchdog action extension that sends SIGABRT to the stuck thread to terminate the process. See ref:`Abort Action `. * xds: added :ref:`extension config discovery` support for HTTP filters. -* xds: added support for mixed v2/v3 discovery response, which enable type url downgrade and upgrade. This feature is disabled by default and is controlled by runtime guard 'envoy.reloadable_features.enable_type_url_downgrade_and_upgrade'. +* xds: added support for mixed v2/v3 discovery response, which enable type url downgrade and upgrade. This feature is disabled by default and is controlled by runtime guard `envoy.reloadable_features.enable_type_url_downgrade_and_upgrade`. * zlib: added option to use `zlib-ng `_ as zlib library. Deprecated diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index 6c39d9c06d6a..f6feba09828a 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -33,7 +33,7 @@ ApiTypeOracle::getEarlierVersionMessageTypeName(const std::string& message_type) } const absl::optional ApiTypeOracle::getEarlierTypeUrl(const std::string& type_url) { - const std::string type{Config::TypeUtil::typeUrlToDescriptorFullName(type_url)}; + const std::string type{TypeUtil::typeUrlToDescriptorFullName(type_url)}; absl::optional old_type = ApiTypeOracle::getEarlierVersionMessageTypeName(type); if (old_type.has_value()) { return TypeUtil::descriptorFullNameToTypeUrl(old_type.value()); diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index 4a20df0c907e..13d3829d1cde 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -97,8 +97,8 @@ FilesystemCollectionSubscriptionImpl::refreshInternal(ProtobufTypes::MessagePtr* auto& resource_message = *owned_resource_message; MessageUtil::loadFromFile(path_, resource_message, validation_visitor_, api_); // Dynamically load the collection message. - const std::string collection_type = std::string( - Config::TypeUtil::typeUrlToDescriptorFullName(resource_message.resource().type_url())); + const std::string collection_type = + std::string(TypeUtil::typeUrlToDescriptorFullName(resource_message.resource().type_url())); const Protobuf::Descriptor* collection_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(collection_type); if (collection_descriptor == nullptr) { diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index 7d5ab2bc3e89..e42ee777a156 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -271,7 +271,7 @@ void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config, if (!typed_config.value().empty()) { // Unpack methods will only use the fully qualified type name after the last '/'. // https://github.com/protocolbuffers/protobuf/blob/3.6.x/src/google/protobuf/any.proto#L87 - absl::string_view type = Config::TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url()); + absl::string_view type = TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url()); if (type == typed_struct_type) { udpa::type::v1::TypedStruct typed_struct; diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 7f8a6bca2640..61b629f927f1 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -255,12 +255,12 @@ class Utility { udpa::type::v1::TypedStruct::default_instance().GetDescriptor()->full_name(); // Unpack methods will only use the fully qualified type name after the last '/'. // https://github.com/protocolbuffers/protobuf/blob/3.6.x/src/google/protobuf/any.proto#L87 - auto type = std::string(Config::TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url())); + auto type = std::string(TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url())); if (type == typed_struct_type) { udpa::type::v1::TypedStruct typed_struct; MessageUtil::unpackTo(typed_config, typed_struct); // Not handling nested structs or typed structs in typed structs - return std::string(Config::TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url())); + return std::string(TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url())); } return type; } diff --git a/source/common/http/request_id_extension_impl.cc b/source/common/http/request_id_extension_impl.cc index 97499f15eebf..f2917959bcd1 100644 --- a/source/common/http/request_id_extension_impl.cc +++ b/source/common/http/request_id_extension_impl.cc @@ -25,8 +25,7 @@ RequestIDExtensionSharedPtr RequestIDExtensionFactory::fromProto( const envoy::extensions::filters::network::http_connection_manager::v3::RequestIDExtension& config, Server::Configuration::FactoryContext& context) { - const std::string type{ - Config::TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; + const std::string type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; auto* factory = Registry::FactoryRegistry::getFactoryByType( type); diff --git a/source/common/protobuf/type_util.cc b/source/common/protobuf/type_util.cc index db497ad2e492..03b5c1f01b34 100644 --- a/source/common/protobuf/type_util.cc +++ b/source/common/protobuf/type_util.cc @@ -1,7 +1,6 @@ #include "common/protobuf/type_util.h" namespace Envoy { -namespace Config { absl::string_view TypeUtil::typeUrlToDescriptorFullName(absl::string_view type_url) { const size_t pos = type_url.rfind('/'); @@ -15,5 +14,4 @@ std::string TypeUtil::descriptorFullNameToTypeUrl(absl::string_view type) { return "type.googleapis.com/" + std::string(type); } -} // namespace Config } // namespace Envoy diff --git a/source/common/protobuf/type_util.h b/source/common/protobuf/type_util.h index 198bb27cf52a..9bcfa0f8c2f6 100644 --- a/source/common/protobuf/type_util.h +++ b/source/common/protobuf/type_util.h @@ -6,12 +6,12 @@ #include "absl/types/optional.h" namespace Envoy { -namespace Config { + class TypeUtil { public: static absl::string_view typeUrlToDescriptorFullName(absl::string_view type_url); static std::string descriptorFullNameToTypeUrl(absl::string_view type); }; -} // namespace Config + } // namespace Envoy diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 810a1d216b8c..287841ce4242 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -581,7 +581,7 @@ std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& messa void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message) { // If we don't have a type URL match, try an earlier version. const absl::string_view any_full_name = - Config::TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); + TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); if (any_full_name != message.GetDescriptor()->full_name()) { const Protobuf::Descriptor* earlier_version_desc = Config::ApiTypeOracle::getEarlierVersionDescriptor(message.GetDescriptor()->full_name()); @@ -686,7 +686,7 @@ bool redactOpaque(Protobuf::Message* message, bool ancestor_is_sensitive, // Try to find a descriptor for `type_url` in the pool and instantiate a new message of the // correct concrete type. const std::string type_url(reflection->GetString(*message, type_url_field_descriptor)); - const std::string concrete_type_name(Config::TypeUtil::typeUrlToDescriptorFullName(type_url)); + const std::string concrete_type_name(TypeUtil::typeUrlToDescriptorFullName(type_url)); const auto* concrete_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(concrete_type_name); if (concrete_descriptor == nullptr) { diff --git a/source/extensions/filters/http/cache/config.cc b/source/extensions/filters/http/cache/config.cc index 7bfaeb0c15a2..86277189a789 100644 --- a/source/extensions/filters/http/cache/config.cc +++ b/source/extensions/filters/http/cache/config.cc @@ -10,8 +10,7 @@ namespace Cache { Http::FilterFactoryCb CacheFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::cache::v3alpha::CacheConfig& config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { - const std::string type{ - Config::TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; + const std::string type{TypeUtil::typeUrlToDescriptorFullName(config.typed_config().type_url())}; HttpCacheFactory* const http_cache_factory = Registry::FactoryRegistry::getFactoryByType(type); if (http_cache_factory == nullptr) { diff --git a/source/extensions/filters/http/compressor/config.cc b/source/extensions/filters/http/compressor/config.cc index 8a9500b20169..aff3ca5afe4c 100644 --- a/source/extensions/filters/http/compressor/config.cc +++ b/source/extensions/filters/http/compressor/config.cc @@ -19,7 +19,7 @@ Http::FilterFactoryCb CompressorFilterFactory::createFilterFactoryFromProtoTyped if (!proto_config.has_compressor_library()) { throw EnvoyException("Compressor filter doesn't have compressor_library defined"); } - const std::string type{Config::TypeUtil::typeUrlToDescriptorFullName( + const std::string type{TypeUtil::typeUrlToDescriptorFullName( proto_config.compressor_library().typed_config().type_url())}; Compression::Compressor::NamedCompressorLibraryConfigFactory* const config_factory = Registry::FactoryRegistry< diff --git a/source/extensions/filters/http/decompressor/config.cc b/source/extensions/filters/http/decompressor/config.cc index 585073eb5ad5..fb52ae85c216 100644 --- a/source/extensions/filters/http/decompressor/config.cc +++ b/source/extensions/filters/http/decompressor/config.cc @@ -14,7 +14,7 @@ namespace Decompressor { Http::FilterFactoryCb DecompressorFilterFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::http::decompressor::v3::Decompressor& proto_config, const std::string& stats_prefix, Server::Configuration::FactoryContext& context) { - const std::string decompressor_library_type{Config::TypeUtil::typeUrlToDescriptorFullName( + const std::string decompressor_library_type{TypeUtil::typeUrlToDescriptorFullName( proto_config.decompressor_library().typed_config().type_url())}; Compression::Decompressor::NamedDecompressorLibraryConfigFactory* const decompressor_library_factory = Registry::FactoryRegistry< diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 18d7843f347a..82a65448222f 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -523,7 +523,7 @@ void HttpConnectionManagerConfig::processDynamicFilterConfig( } std::set require_type_urls; for (const auto& type_url : config_discovery.type_urls()) { - auto factory_type_url = Config::TypeUtil::typeUrlToDescriptorFullName(type_url); + auto factory_type_url = TypeUtil::typeUrlToDescriptorFullName(type_url); require_type_urls.emplace(factory_type_url); auto* factory = Registry::FactoryRegistry< Server::Configuration::NamedHttpFilterConfigFactory>::getFactoryByType(factory_type_url); diff --git a/source/server/admin/config_dump_handler.cc b/source/server/admin/config_dump_handler.cc index 4e00beb1c49a..dbfd13a01e2e 100644 --- a/source/server/admin/config_dump_handler.cc +++ b/source/server/admin/config_dump_handler.cc @@ -79,7 +79,7 @@ void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Messag any_message.MergeFrom(reflection->GetMessage(message, any_field)); Protobuf::DynamicMessageFactory dmf; const absl::string_view inner_type_name = - Config::TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); + TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); const Protobuf::Descriptor* inner_descriptor = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( static_cast(inner_type_name)); From 5d14694550c2081d595fae4d79187444740395e6 Mon Sep 17 00:00:00 2001 From: chaoqinli Date: Tue, 22 Sep 2020 16:20:17 +0000 Subject: [PATCH 32/32] docs Signed-off-by: chaoqinli --- .../faq/api/control_plane_version_support.rst | 16 +++++++++------- test/common/protobuf/utility_test.cc | 11 +++++------ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/docs/root/faq/api/control_plane_version_support.rst b/docs/root/faq/api/control_plane_version_support.rst index d718541ee431..7c6e58ffbeb3 100644 --- a/docs/root/faq/api/control_plane_version_support.rst +++ b/docs/root/faq/api/control_plane_version_support.rst @@ -30,13 +30,15 @@ typical rollout sequence might look like: 4. Support for v2 is removed in the management server. The management server moves to v3 exclusively internally and can support newer fields. -5. Client can send discovery request with v2 resource type url and process discovery response with - v3 resource type url. Client can also send discovery request with v3 resource type url and process - discovery response with v2 resource type url. The upgrade and downgrade of type url is disabled - by default and protected by a runtime guard *envoy.reloadable_features.enable_type_url_downgrade_and_upgrade*. - If your management server does not support both v2/v3 at the same time, you can perform a rollout by enabling - this feature, which allow you to rollout a v3-only management server in the presence of v2-only management servers - and have the clients work with both. +Another approach for type url version migration will be to enable the support of mixed type url +protected by a runtime guard *envoy.reloadable_features.enable_type_url_downgrade_and_upgrade*. +Client can send discovery request with v2 resource type url and process discovery response with +v3 resource type url. Client can also send discovery request with v3 resource type url and process +discovery response with v2 resource type url. The upgrade and downgrade of type url is performed automatically. +If your management server does not support both v2/v3 at the same time, you can have clients +with type url upgrade and downgrade feature enabled. These clients can talk to a mix of management servers +that support either v2 or v3 exclusively. Just like the first approach, no deprecated v2 fields or new v3 fields +can be used at this point. If you are operating a managed control plane as-a-service, you will likely need to support a wide range of client versions. In this scenario, you will require long term support for multiple major diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 99cfc1bf6509..5a199800d71d 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -45,18 +45,17 @@ TEST_F(ProtobufUtilityTest, ConvertPercentNaNDouble) { envoy::config::cluster::v3::Cluster::CommonLbConfig common_config_; common_config_.mutable_healthy_panic_threshold()->set_value( std::numeric_limits::quiet_NaN()); - EXPECT_THROW_WITH_MESSAGE( - PROTOBUF_PERCENT_TO_DOUBLE_OR_DEFAULT(common_config_, healthy_panic_threshold, 0.5), - EnvoyException, "Value not in the range of 0..100 range."); + EXPECT_THROW(PROTOBUF_PERCENT_TO_DOUBLE_OR_DEFAULT(common_config_, healthy_panic_threshold, 0.5), + EnvoyException); } TEST_F(ProtobufUtilityTest, ConvertPercentNaN) { envoy::config::cluster::v3::Cluster::CommonLbConfig common_config_; common_config_.mutable_healthy_panic_threshold()->set_value( std::numeric_limits::quiet_NaN()); - EXPECT_THROW_WITH_MESSAGE(PROTOBUF_PERCENT_TO_ROUNDED_INTEGER_OR_DEFAULT( - common_config_, healthy_panic_threshold, 100, 50), - EnvoyException, "Value not in the range of 0..100 range."); + EXPECT_THROW(PROTOBUF_PERCENT_TO_ROUNDED_INTEGER_OR_DEFAULT(common_config_, + healthy_panic_threshold, 100, 50), + EnvoyException); } namespace ProtobufPercentHelper {