srds: remove scope from scope_name_by_hash_ in case the scope key changes

changelogs/current.yaml

@@ -24,6 +24,9 @@ minor_behavior_changes:
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
+- 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 <deprecated>`

source/common/router/scoped_rds.cc

@@ -279,9 +279,16 @@ absl::StatusOr<bool> 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<RdsRouteConfigProviderHelper> rds_config_provider_helper;

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<ScopedConfigImpl>()
+                ->getRouteConfig(scope_key_builder_->computeScopeKey(
+                    TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-baz-key"}}))
+                ->name(),
+            "foo_routes");
+  EXPECT_THAT(getScopedRdsProvider()->config<ScopedConfigImpl>()->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<ScopedConfigImpl>()->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<ScopedConfigImpl>()
+                ->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<ScopedConfigImpl>(), Not(IsNull()));
+  // No RDS "foo_routes" config push happened yet, Router::NullConfig is returned.
+  EXPECT_THAT(getScopedRdsProvider()
+                  ->config<ScopedConfigImpl>()
+                  ->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<ScopedConfigImpl>()
+                ->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<std::string> 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<ScopedConfigImpl>()
+                ->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<ScopedConfigImpl>()
+                ->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<ScopedConfigImpl>()
+                ->getRouteConfig(scope_key_builder_->computeScopeKey(
+                    TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}))
+                ->name(),
+            "foo_routes");
+  EXPECT_EQ(getScopedRdsProvider()
+                ->config<ScopedConfigImpl>()
+                ->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<ScopedConfigImpl>()
+                ->getRouteConfig(scope_key_builder_->computeScopeKey(
+                    TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-baz-key"}}))
+                ->name(),
+            "foo_routes");
+  EXPECT_THAT(getScopedRdsProvider()->config<ScopedConfigImpl>()->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<ScopedConfigImpl>()->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<ScopedConfigImpl>()
+                ->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.