srds: remove to-de-removed scopes first and then apply additions to a…

…void scope key conflict. (#9366) Remove scopes first then add scopes to avoid scope key conflict when a new scope has the same key as an existing scope Risk Level: MID Testing: unit test Docs Changes: Release Notes: Fixes: #9349 Signed-off-by: Xin Zhuang <stevenzzz@google.com>
envoyproxy · Dec 18, 2019 · 58802ba · 58802ba
1 parent ec93bfe
commit 58802ba
Show file tree

Hide file tree

Showing 3 changed files with 172 additions and 23 deletions.
diff --git 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<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>>
+ScopedRdsConfigSubscription::removeScopes(
     const Protobuf::RepeatedPtrField<std::string>& scope_names, const std::string& version_info) {
-  bool any_applied = false;
+  std::list<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>>
+      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<envoy::api::v2::Resource>& added_resources,
     const Protobuf::RepeatedPtrField<std::string>& removed_resources,
     const std::string& version_info) {
+  // NOTE: deletes are done before adds/updates.
+
+  absl::flat_hash_map<std::string, ScopedRouteInfoConstSharedPtr> 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<std::string> 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<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>>
+      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<LastConfigInfo>({absl::nullopt, version_info}));

diff --git 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<std::string>& exception_msgs);
   // Removes given scopes from the managed set of scopes.
-  // Returns true if any scope updated, false otherwise.
-  bool removeScopes(const Protobuf::RepeatedPtrField<std::string>& 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<std::unique_ptr<RdsRouteConfigProviderHelper>>
+  removeScopes(const Protobuf::RepeatedPtrField<std::string>& 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
@@ -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<std::string>& 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<ProtobufWkt::Any> resources;
-    resources.Add()->PackFrom(TestUtility::parseYaml<envoy::api::v2::RouteConfiguration>(
-        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<ProtobufWkt::Any> resources;
+      resources.Add()->PackFrom(TestUtility::parseYaml<envoy::api::v2::RouteConfiguration>(
+          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<ScopedConfigImpl>()
                 ->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<ScopedConfigImpl>()
                 ->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<ProtobufWkt::Any> 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<ScopedConfigImpl>(), nullptr);
+  // No RDS "foo_routes" config push happened yet, Router::NullConfig is returned.
+  EXPECT_THAT(getScopedRdsProvider()
+                  ->config<ScopedConfigImpl>()
+                  ->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<ScopedConfigImpl>()
+                ->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<ScopedConfigImpl>()
+                ->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<ScopedConfigImpl>()
+                ->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<ScopedConfigImpl>()
+                ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}})
+                ->name(),
+            "foo_routes");
+  EXPECT_EQ(getScopedRdsProvider()
+                ->config<ScopedConfigImpl>()
+                ->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();