diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index dce6782a779a..83bb0c0e411a 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -184,13 +184,20 @@ 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()) { - 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) @@ -200,17 +207,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 +256,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 2a993274d39f..a6e137c76ec0 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( @@ -132,9 +132,11 @@ 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. - bool removeScopes(const Protobuf::RepeatedPtrField& scope_names, - const std::string& version_info); + // 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); // 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 56fbcb43abb3..796e2a795b24 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"}}) @@ -414,7 +417,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(); @@ -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,6 +480,135 @@ route_configuration_name: foo_routes "foo_routes"); } +// Tests that scope-key conflict resources in different config updates are handled correctly. +TEST_F(ScopedRdsTest, ScopeKeyReuseInDifferentPushes) { + setup(); + + 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_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( + 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( + 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_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); + 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(), 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. TEST_F(ScopedRdsTest, InvalidDuplicateResourceSotw) { setup();