[router] Add SRDS configUpdate impl

source/common/router/scoped_config_impl.cc

@@ -107,6 +107,7 @@ void ThreadLocalScopedConfigImpl::addOrUpdateRoutingScope(
 void ThreadLocalScopedConfigImpl::removeRoutingScope(const std::string& scope_name) {
   const auto iter = scoped_route_info_by_name_.find(scope_name);
   if (iter != scoped_route_info_by_name_.end()) {
+    ASSERT(scoped_route_info_by_key_.count(iter->second->scopeKey().hash()) == 1);
     scoped_route_info_by_key_.erase(iter->second->scopeKey().hash());
     scoped_route_info_by_name_.erase(iter);
   }

source/common/router/scoped_config_impl.h

@@ -77,12 +77,14 @@ class ScopeKey {
 // String fragment.
 class StringKeyFragment : public ScopeKeyFragmentBase {
 public:
-  explicit StringKeyFragment(absl::string_view value) : value_(value) {}
+  explicit StringKeyFragment(absl::string_view value)
+      : value_(value), hash_(HashUtil::xxHash64(value_)) {}
 
-  uint64_t hash() const override { return HashUtil::xxHash64(value_); }
+  uint64_t hash() const override { return hash_; }
 
 private:
   const std::string value_;
+  const uint64_t hash_;
 };
 
 /**
@@ -148,11 +150,11 @@ class ScopedRouteInfo {
                   std::shared_ptr<RouteConfigProvider>&& route_provider)
       : config_proto_(std::move(config_proto)), route_provider_(std::move(route_provider)) {
     ASSERT(route_provider_ != nullptr, "ScopedRouteInfo expects a valid RouteConfigProvider.");
-    ASSERT(!route_provider_->configInfo().has_value() ||
-               route_provider_->config()->name() == config_proto_.route_configuration_name(),
-           absl::StrFormat(
-               "RouteConfigProvider's name '%s' doesn't match route_configuration_name '%s'.",
-               route_provider_->config()->name(), config_proto_.route_configuration_name()));
+    ASSERT(
+        !route_provider_->configInfo().has_value() ||
+            route_provider_->config()->name() == config_proto_.route_configuration_name(),
+        fmt::format("RouteConfigProvider's name '{}' doesn't match route_configuration_name '{}'.",
+                    route_provider_->config()->name(), config_proto_.route_configuration_name()));
     // TODO(stevenzzzz): Maybe worth a KeyBuilder abstraction when there are more than one type of
     // Fragment.
     for (const auto& fragment : config_proto_.key().fragments()) {

source/common/router/scoped_rds.cc

@@ -7,6 +7,7 @@
 
 #include "common/common/assert.h"
 #include "common/common/logger.h"
+#include "common/common/utility.h"
 
 // Types are deeply nested under Envoy::Config::ConfigProvider; use 'using-directives' across all
 // ConfigProvider related types for consistency.
@@ -98,77 +99,109 @@ void ScopedRdsConfigSubscription::onConfigUpdate(
     const Protobuf::RepeatedPtrField<envoy::api::v2::Resource>& added_resources,
     const Protobuf::RepeatedPtrField<std::string>& removed_resources,
     const std::string& version_info) {
-
-  // Dedup.
+  bool any_applied = false;
+  std::vector<std::string> exception_msgs;
   absl::flat_hash_set<std::string> unique_resource_names;
-  for (const auto& resource : added_resources) {
-    // TODO(stevenzzzzz): we need to discuss about what to do when two ScopedRouteConfiguration
-    // which has the same ScopeKey.
-    if (!unique_resource_names.insert(resource.name()).second) {
-      throw EnvoyException(
-          fmt::format("duplicate scoped route configuration {} found", resource.name()));
-    }
-  }
-
-  std::vector<envoy::api::v2::ScopedRouteConfiguration> scoped_route_configs;
-  for (const auto& resource : added_resources) {
-    auto scoped_route_config = MessageUtil::anyConvert<envoy::api::v2::ScopedRouteConfiguration>(
-        resource.resource(), validation_visitor_);
-    MessageUtil::validate(scoped_route_config);
-    scoped_route_configs.emplace_back(std::move(scoped_route_config));
-  }
-
   envoy::config::filter::network::http_connection_manager::v2::Rds rds;
   rds.mutable_config_source()->MergeFrom(rds_config_source_);
-  for (auto& scoped_route_config : scoped_route_configs) {
-    ENVOY_LOG(debug, "srds: add/update scoped_route '{}'", scoped_route_config.name());
-    rds.set_route_config_name(scoped_route_config.route_configuration_name());
-    ScopedRouteInfoConstSharedPtr scoped_route_info = std::make_shared<ScopedRouteInfo>(
-        std::move(scoped_route_config),
-        srds_config_provider_manager_.getRouteConfigProvider(factory_context_, rds, stat_prefix_));
-    scoped_route_map_[scoped_route_info->scopeName()] = scoped_route_info;
-    applyDeltaConfigUpdate([scoped_route_info](const ConfigProvider::ConfigConstSharedPtr& config) {
-      auto* thread_local_scoped_config = const_cast<ThreadLocalScopedConfigImpl*>(
-          static_cast<const ThreadLocalScopedConfigImpl*>(config.get()));
+  for (const auto& resource : added_resources) {
+    envoy::api::v2::ScopedRouteConfiguration scoped_route_config;
+    try {
+      scoped_route_config = MessageUtil::anyConvert<envoy::api::v2::ScopedRouteConfiguration>(
+          resource.resource(), validation_visitor_);
+      MessageUtil::validate(scoped_route_config);
+      if (!unique_resource_names.insert(scoped_route_config.name()).second) {
+        throw EnvoyException(fmt::format("duplicate scoped route configuration '{}' found",
+                                         scoped_route_config.name()));
+      }
+      rds.set_route_config_name(scoped_route_config.route_configuration_name());
+      ScopedRouteInfoConstSharedPtr scoped_route_info = std::make_shared<ScopedRouteInfo>(
+          std::move(scoped_route_config), srds_config_provider_manager_.createRouteConfigProvider(
+                                              factory_context_, rds, stat_prefix_));
+      // Detect if there is key conflict between two scopes.
+      auto iter = scope_name_by_hash_.find(scoped_route_info->scopeKey().hash());
+      if (iter != scope_name_by_hash_.end() && iter->second != scoped_route_info->scopeName()) {
+        throw EnvoyException(
+            fmt::format("scope key conflict found, first scope is '{}', second scope is '{}'",
+                        iter->second, scoped_route_info->scopeName()));
+      }
+      scope_name_by_hash_[scoped_route_info->scopeKey().hash()] = scoped_route_info->scopeName();
+      scoped_route_map_[scoped_route_info->scopeName()] = scoped_route_info;
+      applyDeltaConfigUpdate(
+          [scoped_route_info](const ConfigProvider::ConfigConstSharedPtr& config) {
+            auto* thread_local_scoped_config = const_cast<ThreadLocalScopedConfigImpl*>(
+                static_cast<const ThreadLocalScopedConfigImpl*>(config.get()));
 
-      thread_local_scoped_config->addOrUpdateRoutingScope(scoped_route_info);
-    });
+            thread_local_scoped_config->addOrUpdateRoutingScope(scoped_route_info);
+          });
+      any_applied = true;
+      ENVOY_LOG(debug, "srds: add/update scoped_route '{}'", scoped_route_info->scopeName());
+    } catch (const EnvoyException& e) {
+      exception_msgs.emplace_back(fmt::format("{}", e.what()));
+    }
   }
-  for (const auto& scoped_route_name : removed_resources) {
-    ENVOY_LOG(debug, "srds: remove scoped route '{}'", scoped_route_name);
-    scoped_route_map_.erase(scoped_route_name);
-    applyDeltaConfigUpdate([scoped_route_name](const ConfigProvider::ConfigConstSharedPtr& config) {
-      auto* thread_local_scoped_config = const_cast<ThreadLocalScopedConfigImpl*>(
-          static_cast<const ThreadLocalScopedConfigImpl*>(config.get()));
-      thread_local_scoped_config->removeRoutingScope(scoped_route_name);
-    });
+  for (const auto& scope_name : removed_resources) {
+    auto iter = scoped_route_map_.find(scope_name);
+    if (iter != scoped_route_map_.end()) {
+      scope_name_by_hash_.erase(iter->second->scopeKey().hash());
+      scoped_route_map_.erase(iter);
+      applyDeltaConfigUpdate([scope_name](const ConfigProvider::ConfigConstSharedPtr& config) {
+        auto* thread_local_scoped_config = const_cast<ThreadLocalScopedConfigImpl*>(
+            static_cast<const ThreadLocalScopedConfigImpl*>(config.get()));
+        thread_local_scoped_config->removeRoutingScope(scope_name);
+      });
+      any_applied = true;
+      ENVOY_LOG(debug, "srds: remove scoped route '{}'", scope_name);
+    }
   }
   ConfigSubscriptionCommonBase::onConfigUpdate();
-  setLastConfigInfo(absl::optional<LastConfigInfo>({absl::nullopt, version_info}));
+  if (any_applied) {
+    setLastConfigInfo(absl::optional<LastConfigInfo>({absl::nullopt, version_info}));
+  }
   stats_.config_reload_.inc();
+  if (!exception_msgs.empty()) {
+    throw EnvoyException(fmt::format("Error adding/updating scoped route(s): {}",
+                                     StringUtil::join(exception_msgs, ", ")));
+  }
 }
 
+// TODO(stevenzzzz): consider generalizing this function as it overlaps with
+// CdsApiImpl::onConfigUpdate.
 void ScopedRdsConfigSubscription::onConfigUpdate(
     const Protobuf::RepeatedPtrField<ProtobufWkt::Any>& resources,
     const std::string& version_info) {
-  std::vector<envoy::api::v2::ScopedRouteConfiguration> scoped_routes;
+  absl::flat_hash_map<std::string, envoy::api::v2::ScopedRouteConfiguration> scoped_routes;
+  absl::flat_hash_map<uint64_t, std::string> scope_key_by_key_hash;
   for (const auto& resource_any : resources) {
+    // Throws (thus rejects all) on any error.
     auto scoped_route = MessageUtil::anyConvert<envoy::api::v2::ScopedRouteConfiguration>(
         resource_any, validation_visitor_);
     MessageUtil::validate(scoped_route);
-    scoped_routes.emplace_back(std::move(scoped_route));
+    const std::string scope_name = scoped_route.name();
+    auto scope_config_inserted = scoped_routes.try_emplace(scope_name, std::move(scoped_route));
+    if (!scope_config_inserted.second) {
+      throw EnvoyException(
+          fmt::format("duplicate scoped route configuration '{}' found", scoped_route.name()));
+    }
+    const envoy::api::v2::ScopedRouteConfiguration& scoped_route_config =
+        scope_config_inserted.first->second;
+    uint64_t key_fingerprint = MessageUtil::hash(scoped_route_config.key());
+    if (!scope_key_by_key_hash.try_emplace(key_fingerprint, scope_name).second) {
+      throw EnvoyException(
+          fmt::format("scope key conflict found, first scope is '{}', second scope is '{}'",
+                      scope_key_by_key_hash[key_fingerprint], scope_name));
+    }
   }
   ScopedRouteMap scoped_routes_to_remove = scoped_route_map_;
   Protobuf::RepeatedPtrField<envoy::api::v2::Resource> to_add_repeated;
   Protobuf::RepeatedPtrField<std::string> to_remove_repeated;
-
-  for (auto& scoped_route : scoped_routes) {
-    const std::string& scoped_route_name = scoped_route.name();
-    scoped_routes_to_remove.erase(scoped_route_name);
+  for (auto& iter : scoped_routes) {
+    const std::string& scope_name = iter.first;
+    scoped_routes_to_remove.erase(scope_name);
     auto* to_add = to_add_repeated.Add();
-    to_add->set_name(scoped_route.name());
+    to_add->set_name(scope_name);
     to_add->set_version(version_info);
-    to_add->mutable_resource()->PackFrom(scoped_route);
+    to_add->mutable_resource()->PackFrom(iter.second);
   }
 
   for (const auto& scoped_route : scoped_routes_to_remove) {

source/common/router/scoped_rds.h

@@ -117,7 +117,10 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio
         .name();
   }
 
+  // ScopedRouteInfo by scope name.
   ScopedRouteMap scoped_route_map_;
+  // A map of (hash, scope-name), used to detect the key conflict between scopes.
+  absl::flat_hash_map<uint64_t, std::string> scope_name_by_hash_;
   // For creating RDS subscriptions.
   Server::Configuration::FactoryContext& factory_context_;
   const std::string name_;
@@ -194,33 +197,18 @@ class ScopedRoutesConfigProviderManager : public Envoy::Config::ConfigProviderMa
     return route_config_provider_manager_;
   }
 
-  std::shared_ptr<RouteConfigProvider>
-  getRouteConfigProvider(Server::Configuration::FactoryContext& factory_context,
-                         envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
-                         std::string stat_name) {
-    const uint64_t route_provider_id = MessageUtil::hash(rds);
-    auto iter = cached_route_providers_.find(route_provider_id);
-    if (iter != cached_route_providers_.end()) {
-      return iter->second;
-    }
-    std::shared_ptr<RouteConfigProvider> provider =
-        route_config_provider_manager_.createRdsRouteConfigProvider(rds, factory_context,
-                                                                    stat_name);
-    cached_route_providers_.try_emplace(route_provider_id, provider);
-    return provider;
-  }
-  void
-  deleteRouteConfigSource(envoy::config::filter::network::http_connection_manager::v2::Rds& rds) {
-    const uint64_t route_provider_id = MessageUtil::hash(rds);
-    if (!cached_route_providers_.erase(route_provider_id)) {
-      ENVOY_LOG_MISC(warn, "Try to delete non-existing route config source {}", rds.DebugString());
-    }
+  std::shared_ptr<RouteConfigProvider> createRouteConfigProvider(
+      Server::Configuration::FactoryContext& factory_context,
+      const envoy::config::filter::network::http_connection_manager::v2::Rds& rds,
+      const std::string& stat_name) {
+    return route_config_provider_manager_.createRdsRouteConfigProvider(rds, factory_context,
+                                                                       stat_name);
   }
 
 private:
   RouteConfigProviderManager& route_config_provider_manager_;
   // From ConfigSource fingerprint to shared RouteConfigProvider.
-  absl::flat_hash_map<uint64_t, std::shared_ptr<RouteConfigProvider>> cached_route_providers_;
+  absl::flat_hash_map<std::string, std::shared_ptr<RouteConfigProvider>> cached_route_providers_;
 };
 
 // The optional argument passed to the ConfigProviderManager::create*() functions.

source/extensions/filters/network/http_connection_manager/config.cc

@@ -7,7 +7,6 @@
 
 #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.validate.h"
 #include "envoy/filesystem/filesystem.h"
-#include "envoy/registry/registry.h"
 #include "envoy/server/admin.h"
 
 #include "common/access_log/access_log_impl.h"
@@ -104,7 +103,8 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped(
 
   // This lambda captures the shared_ptrs created above, thus preserving the
   // reference count.
-  // Keep in mind the lambda capture list **doesn't** determine the destruction order, the
+  // Keep in mind the lambda capture list **doesn't** determine the destruction order, but it's fine
+  // as these captured objects are also global singletons.
   return [scoped_routes_config_provider_manager, route_config_provider_manager, date_provider,
           filter_config, &context](Network::FilterManager& filter_manager) -> void {
     filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl(