diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 2c1cf161080f..485ecea4d894 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -45,6 +45,9 @@ bug_fixes: change: | Support operations on IP SANs when the IP version is not supported by the host operating system, for example an IPv6 SAN can now be used on a host not supporting IPv6 addresses. +- area: scoped_rds + change: | + Fixes scope key leak and spurious scope key conflicts when an update to an SRDS resource changes the key. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index b60082593413..a2b7929d880e 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -279,9 +279,16 @@ absl::StatusOr ScopedRdsConfigSubscription::addOrUpdateScopes( } const std::string scope_name = scoped_route_config.name(); if (const auto& scope_info_iter = scoped_route_map_.find(scope_name); - scope_info_iter != scoped_route_map_.end() && - scope_info_iter->second->configHash() == MessageUtil::hash(scoped_route_config)) { - continue; + scope_info_iter != scoped_route_map_.end()) { + if (scope_info_iter->second->configHash() == MessageUtil::hash(scoped_route_config)) { + continue; + } + // Remove the old key from scope_names_by_hash_ in case the scope key has changed. (If it + // hasn't, we'll just add it back anyway.) + if (scope_name_by_hash_.find(scope_info_iter->second->scopeKey().hash()) != + scope_name_by_hash_.end()) { + scope_name_by_hash_.erase(scope_info_iter->second->scopeKey().hash()); + } } rds.set_route_config_name(scoped_route_config.route_configuration_name()); std::unique_ptr rds_config_provider_helper; diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 928073348adf..4bb743d5629c 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -950,6 +950,252 @@ route_configuration_name: foo_routes TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}})) ->name(), "foo_routes"); + + // Modify foo_scope4 to use a different scope key. + const std::string config_yaml4_modified = R"EOF( +name: foo_scope4 +route_configuration_name: foo_routes +key: + fragments: + - string_key: x-baz-key +)EOF"; + const auto resource_4_modified = parseScopedRouteConfigurationFromYaml(config_yaml4_modified); + const auto decoded_resources_5 = TestUtility::decodeResources({resource_3, resource_4_modified}); + EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources_5.refvec_, "5").ok()); + EXPECT_EQ(server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value(), + 4UL); + EXPECT_EQ(2UL, all_scopes_.value()); + EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1); + EXPECT_EQ(getScopedRouteMap().count("foo_scope4"), 1); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-baz-key"}})) + ->name(), + "foo_routes"); + EXPECT_THAT(getScopedRdsProvider()->config()->getRouteConfig( + scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}})), + IsNull()); + + // Remove foo_scope4. + const auto decoded_resources_6 = TestUtility::decodeResources({resource_3}); + EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources_6.refvec_, "6").ok()); + EXPECT_EQ(server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value(), + 5UL); + EXPECT_EQ(1UL, all_scopes_.value()); + EXPECT_EQ(getScopedRouteMap().count("foo_scope4"), 0); + EXPECT_THAT(getScopedRdsProvider()->config()->getRouteConfig( + scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-baz-key"}})), + IsNull()); + + // Re-add foo_scope2 which was conflicting with the previous version of foo_scope4. + const auto decoded_resources_7 = TestUtility::decodeResources({resource_2, resource_3}); + EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources_7.refvec_, "7").ok()); + pushRdsConfig({"bar_routes"}, "111"); + EXPECT_EQ(server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value(), + 6UL); + EXPECT_EQ(2UL, all_scopes_.value()); + EXPECT_EQ(getScopedRouteMap().count("foo_scope2"), 1); + EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}})) + ->name(), + "bar_routes"); +} + +TEST_F(ScopedRdsTest, ScopeKeyReuseInDifferentPushesDelta) { + 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"; + const auto resource = parseScopedRouteConfigurationFromYaml(config_yaml1); + const auto resource_2 = parseScopedRouteConfigurationFromYaml(config_yaml2); + const auto decoded_resources = TestUtility::decodeResources({resource, resource_2}); + init_watcher_.expectReady(); + context_init_manager_.initialize(init_watcher_); + EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources.refvec_, {}, "1").ok()); + EXPECT_EQ(1UL, + server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value()); + // Scope key "x-foo-key" points to nowhere. + ASSERT_THAT(getScopedRdsProvider(), Not(IsNull())); + ASSERT_THAT(getScopedRdsProvider()->config(), Not(IsNull())); + // No RDS "foo_routes" config push happened yet, Router::NullConfig is returned. + EXPECT_THAT(getScopedRdsProvider() + ->config() + ->getRouteConfig(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}})) + ->name(), + ""); + pushRdsConfig({"foo_routes", "bar_routes"}, "111"); + EXPECT_EQ(server_factory_context_.store_.counter("foo.rds.foo_routes.config_reload").value(), + 1UL); + EXPECT_EQ(server_factory_context_.store_.counter("foo.rds.bar_routes.config_reload").value(), + 1UL); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"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"; + + // Remove foo_scope1 and add a new scope3 reuses the same scope_key. + const auto resource_3 = parseScopedRouteConfigurationFromYaml(config_yaml3); + const auto decoded_resources_2 = TestUtility::decodeResources({resource_3}); + Protobuf::RepeatedPtrField deletes; + *deletes.Add() = "foo_scope1"; + EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources_2.refvec_, deletes, "2").ok()); + EXPECT_EQ(2UL, + server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value()); + // foo_scope is deleted, and foo_scope2 is added. + EXPECT_EQ(all_scopes_.value(), 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(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"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"; + const auto resource_4 = parseScopedRouteConfigurationFromYaml(config_yaml4); + const auto decoded_resources_3 = TestUtility::decodeResources({resource_4}); + EXPECT_THAT( + srds_subscription_->onConfigUpdate(decoded_resources_3.refvec_, {}, "3").message(), + testing::MatchesRegex( + ".*scope key conflict found, first scope is 'foo_scope2', second scope is 'foo_scope4'")); + EXPECT_EQ(2UL, all_scopes_.value()); + 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(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"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. + const auto decoded_resources_4 = TestUtility::decodeResources({resource_4}); + deletes.Clear(); + *deletes.Add() = "foo_scope2"; + EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources_4.refvec_, deletes, "4").ok()); + EXPECT_EQ(server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value(), + 3UL); + EXPECT_EQ(2UL, all_scopes_.value()); + EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1); + EXPECT_EQ(getScopedRouteMap().count("foo_scope4"), 1); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}})) + ->name(), + "foo_routes"); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}})) + ->name(), + "foo_routes"); + + // Modify foo_scope4 to use a different scope key. + const std::string config_yaml4_modified = R"EOF( +name: foo_scope4 +route_configuration_name: foo_routes +key: + fragments: + - string_key: x-baz-key +)EOF"; + const auto resource_4_modified = parseScopedRouteConfigurationFromYaml(config_yaml4_modified); + const auto decoded_resources_5 = TestUtility::decodeResources({resource_4_modified}); + EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources_5.refvec_, {}, "5").ok()); + EXPECT_EQ(server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value(), + 4UL); + EXPECT_EQ(2UL, all_scopes_.value()); + EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1); + EXPECT_EQ(getScopedRouteMap().count("foo_scope4"), 1); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-baz-key"}})) + ->name(), + "foo_routes"); + EXPECT_THAT(getScopedRdsProvider()->config()->getRouteConfig( + scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}})), + IsNull()); + + // Remove foo_scope4. + deletes.Clear(); + *deletes.Add() = "foo_scope4"; + EXPECT_TRUE(srds_subscription_->onConfigUpdate({}, deletes, "6").ok()); + EXPECT_EQ(server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value(), + 5UL); + EXPECT_EQ(1UL, all_scopes_.value()); + EXPECT_EQ(getScopedRouteMap().count("foo_scope4"), 0); + EXPECT_THAT(getScopedRdsProvider()->config()->getRouteConfig( + scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-baz-key"}})), + IsNull()); + + // Re-add foo_scope2 which was conflicting with the previous version of foo_scope4. + const auto decoded_resources_7 = TestUtility::decodeResources({resource_2}); + EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources_7.refvec_, {}, "7").ok()); + pushRdsConfig({"bar_routes"}, "111"); + EXPECT_EQ(server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload") + .value(), + 6UL); + EXPECT_EQ(2UL, all_scopes_.value()); + EXPECT_EQ(getScopedRouteMap().count("foo_scope2"), 1); + EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1); + EXPECT_EQ(getScopedRdsProvider() + ->config() + ->getRouteConfig(scope_key_builder_->computeScopeKey( + TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}})) + ->name(), + "bar_routes"); } // Tests that only one resource is provided during a config update.