From 7af0f6c61e6f1d65e016a141dfd2490650d62c1a Mon Sep 17 00:00:00 2001 From: Xin Zhuang Date: Mon, 16 Dec 2019 22:33:40 -0500 Subject: [PATCH 1/3] remove scopes first then add scopes to avoid scope key conflict when a new scope has the same key as an existing scope. Signed-off-by: Xin Zhuang --- source/common/router/scoped_rds.cc | 29 ++++++++---- source/common/router/scoped_rds.h | 7 +-- test/common/router/scoped_rds_test.cc | 66 ++++++++++++++++++++++++++- 3 files changed, 89 insertions(+), 13 deletions(-) diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index dce6782a779a3..13032bdca43f3 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -184,12 +184,15 @@ bool ScopedRdsConfigSubscription::addOrUpdateScopes( return any_applied; } -bool ScopedRdsConfigSubscription::removeScopes( +std::list> +ScopedRdsConfigSubscription::removeScopes( const Protobuf::RepeatedPtrField& scope_names, const std::string& version_info) { - bool any_applied = false; + std::list> + to_be_removed_rds_providers; for (const auto& scope_name : scope_names) { auto iter = scoped_route_map_.find(scope_name); if (iter != scoped_route_map_.end()) { + to_be_removed_rds_providers.emplace_back(std::move(route_provider_by_scope_[scope_name])); route_provider_by_scope_.erase(scope_name); scope_name_by_hash_.erase(iter->second->scopeKey().hash()); scoped_route_map_.erase(iter); @@ -200,17 +203,19 @@ bool ScopedRdsConfigSubscription::removeScopes( thread_local_scoped_config->removeRoutingScope(scope_name); return config; }); - any_applied = true; ENVOY_LOG(debug, "srds: remove scoped route '{}', version: {}", scope_name, version_info); } } - return any_applied; + return to_be_removed_rds_providers; } void ScopedRdsConfigSubscription::onConfigUpdate( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField& removed_resources, const std::string& version_info) { + // NOTE: deletes are done before adds/updates. + + absl::flat_hash_map to_be_removed_scopes; // If new route config sources come after the factory_context_.initManager()'s initialize() been // called, that initManager can't accept new targets. Instead we use a local override which will // start new subscriptions but not wait on them to be ready. @@ -247,12 +252,18 @@ void ScopedRdsConfigSubscription::onConfigUpdate( } }); } + std::vector exception_msgs; - bool any_applied = addOrUpdateScopes( - added_resources, - (noop_init_manager == nullptr ? factory_context_.initManager() : *noop_init_manager), - version_info, exception_msgs); - any_applied = removeScopes(removed_resources, version_info) || any_applied; + // Do not delete RDS config providers just yet, in case the to be deleted RDS subscriptions could + // be reused by some to be added scopes. + std::list> + to_be_removed_rds_providers = removeScopes(removed_resources, version_info); + bool any_applied = + addOrUpdateScopes( + added_resources, + (noop_init_manager == nullptr ? factory_context_.initManager() : *noop_init_manager), + version_info, exception_msgs) || + !to_be_removed_rds_providers.empty(); ConfigSubscriptionCommonBase::onConfigUpdate(); if (any_applied) { setLastConfigInfo(absl::optional({absl::nullopt, version_info})); diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index 2a993274d39fe..e3d3470dc9b6f 100644 --- a/source/common/router/scoped_rds.h +++ b/source/common/router/scoped_rds.h @@ -107,7 +107,7 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio const ScopedRouteMap& scopedRouteMap() const { return scoped_route_map_; } private: - // A helper class that takes care the life cycle management of a RDS route provider and the + // A helper class that takes care of the life cycle management of a RDS route provider and the // update callback handle. struct RdsRouteConfigProviderHelper { RdsRouteConfigProviderHelper( @@ -133,8 +133,9 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio std::vector& exception_msgs); // Removes given scopes from the managed set of scopes. // Returns true if any scope updated, false otherwise. - bool removeScopes(const Protobuf::RepeatedPtrField& scope_names, - const std::string& version_info); + std::list> + removeScopes(const Protobuf::RepeatedPtrField& scope_names, + const std::string& version_info); // Envoy::Config::DeltaConfigSubscriptionInstance void start() override { subscription_->start({}); } diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 56fbcb43abb38..84eec25005330 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -414,7 +414,7 @@ route_configuration_name: foo_routes "foo_routes"); } -// Tests that conflict resources are detected. +// Tests that conflict resources in the same push are detected. TEST_F(ScopedRdsTest, MultipleResourcesWithKeyConflict) { setup(); @@ -477,6 +477,70 @@ route_configuration_name: foo_routes "foo_routes"); } +// Tests that scope-key conflict resources in differnt pushes are handled correctly. +TEST_F(ScopedRdsTest, ScopeKeyReuseInDifferentPushes) { + setup(); + + const std::string config_yaml = R"EOF( +name: foo_scope +route_configuration_name: foo_routes +key: + fragments: + - string_key: x-foo-key +)EOF"; + Protobuf::RepeatedPtrField resources; + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml); + EXPECT_CALL(context_init_manager_, state()).WillOnce(Return(Init::Manager::State::Uninitialized)); + EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "1")); + EXPECT_EQ( + 1UL, + factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value()); + // Scope key "x-foo-key" points to nowhere. + EXPECT_NE(getScopedRdsProvider(), nullptr); + EXPECT_NE(getScopedRdsProvider()->config(), nullptr); + // No RDS "foo_routes" config push happened yet, Router::NullConfig is returned. + EXPECT_THAT(getScopedRdsProvider() + ->config() + ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) + ->name(), + ""); + context_init_manager_.initialize(init_watcher_); + init_watcher_.expectReady().Times( + 2); // SRDS "foo_scope" and RDS "foo_routes"(though no real push). + pushRdsConfig("foo_routes", "111"); + EXPECT_EQ(server_factory_context_.scope_.counter("foo.rds.foo_routes.config_reload").value(), + 1UL); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) + ->name(), + "foo_routes"); + const std::string config_yaml2 = R"EOF( +name: foo_scope2 +route_configuration_name: foo_routes +key: + fragments: + - string_key: x-foo-key +)EOF"; + resources.Clear(); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2); + EXPECT_CALL(context_init_manager_, state()).WillOnce(Return(Init::Manager::State::Initialized)); + EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "2")); + EXPECT_EQ( + 2UL, + factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value()); + // foo_scope is deleted, and foo_scope2 is added. + EXPECT_EQ(getScopedRouteMap().size(), 1UL); + EXPECT_EQ(getScopedRouteMap().count("foo_scope"), 0); + EXPECT_EQ(getScopedRouteMap().count("foo_scope2"), 1); + // The same scope-key now points to the same route table. + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) + ->name(), + "foo_routes"); +} + // Tests that only one resource is provided during a config update. TEST_F(ScopedRdsTest, InvalidDuplicateResourceSotw) { setup(); From 5f78c17ee1cfc223e4f9b7b0ca938c508a45749e Mon Sep 17 00:00:00 2001 From: Xin Zhuang Date: Tue, 17 Dec 2019 09:02:31 -0500 Subject: [PATCH 2/3] fix typo Signed-off-by: Xin Zhuang --- test/common/router/scoped_rds_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 84eec25005330..0389ec17831a6 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -477,7 +477,7 @@ route_configuration_name: foo_routes "foo_routes"); } -// Tests that scope-key conflict resources in differnt pushes are handled correctly. +// Tests that scope-key conflict resources in different pushes are handled correctly. TEST_F(ScopedRdsTest, ScopeKeyReuseInDifferentPushes) { setup(); From 9e28188c59ad340bdaa33dc3f5e31bfdc7234b1d Mon Sep 17 00:00:00 2001 From: Xin Zhuang Date: Tue, 17 Dec 2019 15:49:42 -0500 Subject: [PATCH 3/3] add more test scenarios and feedback fixes Signed-off-by: Xin Zhuang --- source/common/router/scoped_rds.cc | 8 +- source/common/router/scoped_rds.h | 3 +- test/common/router/scoped_rds_test.cc | 106 +++++++++++++++++++++----- 3 files changed, 95 insertions(+), 22 deletions(-) diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index 13032bdca43f3..83bb0c0e411aa 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -192,8 +192,12 @@ ScopedRdsConfigSubscription::removeScopes( for (const auto& scope_name : scope_names) { auto iter = scoped_route_map_.find(scope_name); if (iter != scoped_route_map_.end()) { - to_be_removed_rds_providers.emplace_back(std::move(route_provider_by_scope_[scope_name])); - route_provider_by_scope_.erase(scope_name); + auto rds_config_provider_helper_iter = route_provider_by_scope_.find(scope_name); + if (rds_config_provider_helper_iter != route_provider_by_scope_.end()) { + to_be_removed_rds_providers.emplace_back( + std::move(rds_config_provider_helper_iter->second)); + route_provider_by_scope_.erase(rds_config_provider_helper_iter); + } scope_name_by_hash_.erase(iter->second->scopeKey().hash()); scoped_route_map_.erase(iter); applyConfigUpdate([scope_name](ConfigProvider::ConfigConstSharedPtr config) diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index e3d3470dc9b6f..a6e137c76ec0d 100644 --- a/source/common/router/scoped_rds.h +++ b/source/common/router/scoped_rds.h @@ -132,7 +132,8 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio Init::Manager& init_manager, const std::string& version_info, std::vector& exception_msgs); // Removes given scopes from the managed set of scopes. - // Returns true if any scope updated, false otherwise. + // Returns a list of to be removed helpers which is temporally held in the onConfigUpdate method, + // to make sure new scopes sharing the same RDS source configs could reuse the subscriptions. std::list> removeScopes(const Protobuf::RepeatedPtrField& scope_names, const std::string& version_info); diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 0389ec17831a6..796e2a795b24e 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -177,7 +177,8 @@ name: foo_scoped_routes // Helper function which pushes an update to given RDS subscription, the start(_) of the // subscription must have been called. - void pushRdsConfig(const std::string& route_config_name, const std::string& version) { + void pushRdsConfig(const std::vector& route_config_names, + const std::string& version) { const std::string route_config_tmpl = R"EOF( name: {} virtual_hosts: @@ -187,10 +188,12 @@ name: foo_scoped_routes - match: {{ prefix: "/" }} route: {{ cluster: bluh }} )EOF"; - Protobuf::RepeatedPtrField resources; - resources.Add()->PackFrom(TestUtility::parseYaml( - fmt::format(route_config_tmpl, route_config_name))); - rds_subscription_by_name_[route_config_name]->onConfigUpdate(resources, version); + for (const std::string& name : route_config_names) { + Protobuf::RepeatedPtrField resources; + resources.Add()->PackFrom(TestUtility::parseYaml( + fmt::format(route_config_tmpl, name))); + rds_subscription_by_name_[name]->onConfigUpdate(resources, version); + } } ScopedRdsConfigProvider* getScopedRdsProvider() const { @@ -304,7 +307,7 @@ route_configuration_name: foo_routes ->name(), ""); // RDS updates foo_routes. - pushRdsConfig("foo_routes", "111"); + pushRdsConfig({"foo_routes"}, "111"); EXPECT_EQ(getScopedRdsProvider() ->config() ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) @@ -381,7 +384,7 @@ route_configuration_name: foo_routes ->name(), ""); // RDS updates foo_routes. - pushRdsConfig("foo_routes", "111"); + pushRdsConfig({"foo_routes"}, "111"); EXPECT_EQ(getScopedRdsProvider() ->config() ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) @@ -467,7 +470,7 @@ route_configuration_name: foo_routes EXPECT_EQ(getScopedRouteMap().size(), 1UL); EXPECT_EQ(getScopedRouteMap().count("foo_scope"), 1); // Scope key "x-foo-key" points to foo_routes due to partial rejection. - pushRdsConfig("foo_routes", "111"); // Push some real route configuration. + pushRdsConfig({"foo_routes"}, "111"); // Push some real route configuration. EXPECT_EQ(1UL, server_factory_context_.scope_.counter("foo.rds.foo_routes.config_reload").value()); EXPECT_EQ(getScopedRdsProvider() @@ -477,19 +480,27 @@ route_configuration_name: foo_routes "foo_routes"); } -// Tests that scope-key conflict resources in different pushes are handled correctly. +// Tests that scope-key conflict resources in different config updates are handled correctly. TEST_F(ScopedRdsTest, ScopeKeyReuseInDifferentPushes) { setup(); - const std::string config_yaml = R"EOF( -name: foo_scope + const std::string config_yaml1 = R"EOF( +name: foo_scope1 route_configuration_name: foo_routes key: fragments: - string_key: x-foo-key +)EOF"; + const std::string config_yaml2 = R"EOF( +name: foo_scope2 +route_configuration_name: bar_routes +key: + fragments: + - string_key: x-bar-key )EOF"; Protobuf::RepeatedPtrField resources; - parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml1); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2); EXPECT_CALL(context_init_manager_, state()).WillOnce(Return(Init::Manager::State::Uninitialized)); EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "1")); EXPECT_EQ( @@ -506,39 +517,96 @@ route_configuration_name: foo_routes ""); context_init_manager_.initialize(init_watcher_); init_watcher_.expectReady().Times( - 2); // SRDS "foo_scope" and RDS "foo_routes"(though no real push). - pushRdsConfig("foo_routes", "111"); + 3); // SRDS "foo_scope1" and RDS "foo/bar_routes"(though no real push). + pushRdsConfig({"foo_routes", "bar_routes"}, "111"); EXPECT_EQ(server_factory_context_.scope_.counter("foo.rds.foo_routes.config_reload").value(), 1UL); + EXPECT_EQ(server_factory_context_.scope_.counter("foo.rds.bar_routes.config_reload").value(), + 1UL); EXPECT_EQ(getScopedRdsProvider() ->config() ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) ->name(), "foo_routes"); - const std::string config_yaml2 = R"EOF( -name: foo_scope2 + + const std::string config_yaml3 = R"EOF( +name: foo_scope3 route_configuration_name: foo_routes key: fragments: - string_key: x-foo-key )EOF"; resources.Clear(); + + // Remove foo_scope1 and add a new scope3 reuses the same scope_key. parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2); - EXPECT_CALL(context_init_manager_, state()).WillOnce(Return(Init::Manager::State::Initialized)); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml3); + EXPECT_CALL(context_init_manager_, state()) + .Times(2) + .WillRepeatedly(Return(Init::Manager::State::Initialized)); EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "2")); EXPECT_EQ( 2UL, factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value()); // foo_scope is deleted, and foo_scope2 is added. - EXPECT_EQ(getScopedRouteMap().size(), 1UL); - EXPECT_EQ(getScopedRouteMap().count("foo_scope"), 0); + EXPECT_EQ(getScopedRouteMap().size(), 2UL); + EXPECT_EQ(getScopedRouteMap().count("foo_scope1"), 0); EXPECT_EQ(getScopedRouteMap().count("foo_scope2"), 1); + EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1); // The same scope-key now points to the same route table. EXPECT_EQ(getScopedRdsProvider() ->config() ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) ->name(), "foo_routes"); + + // Push a new scope foo_scope4 with the same key as foo_scope2 but a different route-table, this + // ends in an exception. + const std::string config_yaml4 = R"EOF( +name: foo_scope4 +route_configuration_name: foo_routes +key: + fragments: + - string_key: x-bar-key +)EOF"; + resources.Clear(); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml3); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml4); + EXPECT_THROW_WITH_REGEX( + srds_subscription_->onConfigUpdate(resources, "3"), EnvoyException, + "scope key conflict found, first scope is 'foo_scope2', second scope is 'foo_scope4'"); + EXPECT_EQ(getScopedRouteMap().size(), 2UL); + EXPECT_EQ(getScopedRouteMap().count("foo_scope1"), 0); + EXPECT_EQ(getScopedRouteMap().count("foo_scope2"), 1); + EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}) + ->name(), + "bar_routes"); + + // Delete foo_scope2, and push a new foo_scope4 with the same scope key but different route-table. + resources.Clear(); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml3); + parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml4); + EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "4")); + EXPECT_EQ( + factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value(), + 3UL); + EXPECT_EQ(getScopedRouteMap().size(), 2UL); + EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1); + EXPECT_EQ(getScopedRouteMap().count("foo_scope4"), 1); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}) + ->name(), + "foo_routes"); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) + ->name(), + "foo_routes"); } // Tests that only one resource is provided during a config update.