fix(router): update scope_key_builder when srds config is updated (en…

…voyproxy#26860)

* fix(router): update scope_key_builder when srds config is updated

Signed-off-by: Bing Han <[email protected]>

changelogs/current.yaml

@@ -61,6 +61,11 @@ bug_fixes:
 - area: tls
   change: |
     Fix build FIPS compliance when using both FIPS mode and Wasm extensions (``--define boringssl=fips`` and ``--define wasm=v8``).
+- area: router
+  change: |
+    Fixed the bug that updating :ref:`scope_key_builder
+    <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.ScopedRoutes.scope_key_builder>`
+    of SRDS config doesn't work and multiple HCM share the same ``scope_key_builder``.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

envoy/router/scopes.h

@@ -80,29 +80,39 @@ class StringKeyFragment : public ScopeKeyFragmentBase {
 };
 
 /**
- * The scoped routing configuration.
+ * The scoped key builder.
  */
-class ScopedConfig : public Envoy::Config::ConfigProvider::Config {
+class ScopeKeyBuilder {
 public:
-  ~ScopedConfig() override = default;
+  virtual ~ScopeKeyBuilder() = default;
 
   /**
-   * Based on the incoming HTTP request headers, returns the configuration to use for selecting a
-   * target route.
+   * Based on the incoming HTTP request headers, returns the hash value of its scope key.
    * @param headers the request headers to match the scoped routing configuration against.
-   * @return ConfigConstSharedPtr the router's Config matching the request headers.
+   * @return unique_ptr of the scope key computed from header.
    */
-  virtual ConfigConstSharedPtr getRouteConfig(const Http::HeaderMap& headers) const PURE;
+  virtual ScopeKeyPtr computeScopeKey(const Http::HeaderMap&) const PURE;
+};
+
+/**
+ * The scoped routing configuration.
+ */
+class ScopedConfig : public Envoy::Config::ConfigProvider::Config {
+public:
+  ~ScopedConfig() override = default;
 
   /**
-   * Based on the incoming HTTP request headers, returns the hash value of its scope key.
-   * @param headers the request headers to match the scoped routing configuration against.
-   * @return unique_ptr of the scope key computed from header.
+   * Based on the scope key, returns the configuration to use for selecting a target route.
+   * The scope key can be got via ScopeKeyBuilder.
+   *
+   * @param scope_key the scope key. null config will be returned when null.
+   * @return ConfigConstSharedPtr the router's Config matching the request headers.
    */
-  virtual ScopeKeyPtr computeScopeKey(const Http::HeaderMap&) const { return {}; }
+  virtual ConfigConstSharedPtr getRouteConfig(const ScopeKeyPtr& scope_key) const PURE;
 };
 
 using ScopedConfigConstSharedPtr = std::shared_ptr<const ScopedConfig>;
+using ScopeKeyBuilderPtr = std::unique_ptr<const ScopeKeyBuilder>;
 
 } // namespace Router
 } // namespace Envoy

source/common/http/BUILD

@@ -252,6 +252,7 @@ envoy_cc_library(
         "//envoy/http:original_ip_detection_interface",
         "//envoy/http:request_id_extension_interface",
         "//envoy/router:rds_interface",
+        "//envoy/router:scopes_interface",
         "//source/common/local_reply:local_reply_lib",
         "//source/common/network:utility_lib",
         "//source/common/stats:symbol_table_lib",

source/common/http/conn_manager_config.h

@@ -8,6 +8,7 @@
 #include "envoy/http/original_ip_detection.h"
 #include "envoy/http/request_id_extension.h"
 #include "envoy/router/rds.h"
+#include "envoy/router/scopes.h"
 #include "envoy/stats/scope.h"
 #include "envoy/tracing/tracer.h"
 #include "envoy/type/v3/percent.pb.h"
@@ -339,6 +340,12 @@ class ConnectionManagerConfig {
    */
   virtual Config::ConfigProvider* scopedRouteConfigProvider() PURE;
 
+  /**
+   * @return OptRef<Router::ScopeKeyBuilder> the scope key builder to calculate the scope key.
+   * This will return nullptr when scoped routing is not enabled.
+   */
+  virtual OptRef<const Router::ScopeKeyBuilder> scopeKeyBuilder() PURE;
+
   /**
    * @return const std::string& the server name to write into responses.
    */

source/common/http/conn_manager_impl.cc

@@ -670,9 +670,8 @@ void ConnectionManagerImpl::RdsRouteConfigUpdateRequester::requestRouteConfigUpd
     const auto& host_header = absl::AsciiStrToLower(parent_.request_headers_->getHostValue());
     requestVhdsUpdate(host_header, thread_local_dispatcher, std::move(route_config_updated_cb));
     return;
-  } else if (parent_.snapped_scoped_routes_config_ != nullptr) {
-    Router::ScopeKeyPtr scope_key =
-        parent_.snapped_scoped_routes_config_->computeScopeKey(*parent_.request_headers_);
+  } else if (scope_key_builder_.has_value()) {
+    Router::ScopeKeyPtr scope_key = scope_key_builder_->computeScopeKey(*parent_.request_headers_);
     // If scope_key is not null, the scope exists but RouteConfiguration is not initialized.
     if (scope_key != nullptr) {
       requestSrdsUpdate(std::move(scope_key), thread_local_dispatcher,
@@ -757,10 +756,13 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
           connection_manager.config_.makeHeaderValidator(connection_manager.codec_->protocol())) {
   ASSERT(!connection_manager.config_.isRoutable() ||
              ((connection_manager.config_.routeConfigProvider() == nullptr &&
-               connection_manager.config_.scopedRouteConfigProvider() != nullptr) ||
+               connection_manager.config_.scopedRouteConfigProvider() != nullptr &&
+               connection_manager.config_.scopeKeyBuilder().has_value()) ||
               (connection_manager.config_.routeConfigProvider() != nullptr &&
-               connection_manager.config_.scopedRouteConfigProvider() == nullptr)),
-         "Either routeConfigProvider or scopedRouteConfigProvider should be set in "
+               connection_manager.config_.scopedRouteConfigProvider() == nullptr &&
+               !connection_manager.config_.scopeKeyBuilder().has_value())),
+         "Either routeConfigProvider or (scopedRouteConfigProvider and scopeKeyBuilder) should be "
+         "set in "
          "ConnectionManagerImpl.");
   for (const AccessLog::InstanceSharedPtr& access_log : connection_manager_.config_.accessLogs()) {
     filter_manager_.addAccessLogHandler(access_log);
@@ -775,10 +777,12 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
         std::make_unique<ConnectionManagerImpl::RdsRouteConfigUpdateRequester>(
             connection_manager.config_.routeConfigProvider(), *this);
   } else if (connection_manager_.config_.isRoutable() &&
-             connection_manager.config_.scopedRouteConfigProvider() != nullptr) {
+             connection_manager.config_.scopedRouteConfigProvider() != nullptr &&
+             connection_manager.config_.scopeKeyBuilder().has_value()) {
     route_config_update_requester_ =
         std::make_unique<ConnectionManagerImpl::RdsRouteConfigUpdateRequester>(
-            connection_manager.config_.scopedRouteConfigProvider(), *this);
+            connection_manager.config_.scopedRouteConfigProvider(),
+            connection_manager.config_.scopeKeyBuilder(), *this);
   }
   ScopeTrackerScopeState scope(this,
                                connection_manager_.read_callbacks_->connection().dispatcher());
@@ -1094,7 +1098,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapSharedPt
   if (connection_manager_.config_.isRoutable()) {
     if (connection_manager_.config_.routeConfigProvider() != nullptr) {
       snapped_route_config_ = connection_manager_.config_.routeConfigProvider()->configCast();
-    } else if (connection_manager_.config_.scopedRouteConfigProvider() != nullptr) {
+    } else if (connection_manager_.config_.scopedRouteConfigProvider() != nullptr &&
+               connection_manager_.config_.scopeKeyBuilder().has_value()) {
       snapped_scoped_routes_config_ =
           connection_manager_.config_.scopedRouteConfigProvider()->config<Router::ScopedConfig>();
       snapScopedRouteConfig();
@@ -1406,7 +1411,9 @@ void ConnectionManagerImpl::startDrainSequence() {
 void ConnectionManagerImpl::ActiveStream::snapScopedRouteConfig() {
   // NOTE: if a RDS subscription hasn't got a RouteConfiguration back, a Router::NullConfigImpl is
   // returned, in that case we let it pass.
-  snapped_route_config_ = snapped_scoped_routes_config_->getRouteConfig(*request_headers_);
+  auto scope_key =
+      connection_manager_.config_.scopeKeyBuilder()->computeScopeKey(*request_headers_);
+  snapped_route_config_ = snapped_scoped_routes_config_->getRouteConfig(scope_key);
   if (snapped_route_config_ == nullptr) {
     ENVOY_STREAM_LOG(trace, "can't find SRDS scope.", *this);
     // TODO(stevenzzzz): Consider to pass an error message to router filter, so that it can
@@ -1515,7 +1522,8 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute(const Router::Route
   Router::RouteConstSharedPtr route;
   if (request_headers_ != nullptr) {
     if (connection_manager_.config_.isRoutable() &&
-        connection_manager_.config_.scopedRouteConfigProvider() != nullptr) {
+        connection_manager_.config_.scopedRouteConfigProvider() != nullptr &&
+        connection_manager_.config_.scopeKeyBuilder().has_value()) {
       // NOTE: re-select scope as well in case the scope key header has been changed by a filter.
       snapScopedRouteConfig();
     }

source/common/http/conn_manager_impl.h

@@ -126,14 +126,15 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
         : route_config_provider_(route_config_provider), parent_(parent) {}
 
     RdsRouteConfigUpdateRequester(Config::ConfigProvider* scoped_route_config_provider,
+                                  OptRef<const Router::ScopeKeyBuilder> scope_key_builder,
                                   ActiveStream& parent)
         // Expect the dynamic cast to succeed because only ScopedRdsConfigProvider is fully
         // implemented. Inline provider will be cast to nullptr here but it is not full implemented
         // and can't not be used at this point. Should change this implementation if we have a
         // functional inline scope route provider in the future.
         : scoped_route_config_provider_(
               dynamic_cast<Router::ScopedRdsConfigProvider*>(scoped_route_config_provider)),
-          parent_(parent) {}
+          scope_key_builder_(scope_key_builder), parent_(parent) {}
 
     void
     requestRouteConfigUpdate(Http::RouteConfigUpdatedCallbackSharedPtr route_config_updated_cb);
@@ -147,6 +148,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
   private:
     Router::RouteConfigProvider* route_config_provider_;
     Router::ScopedRdsConfigProvider* scoped_route_config_provider_;
+    OptRef<const Router::ScopeKeyBuilder> scope_key_builder_;
     ActiveStream& parent_;
   };
 

source/common/router/BUILD

@@ -226,6 +226,7 @@ envoy_cc_library(
         "//envoy/config:config_provider_interface",
         "//envoy/config:subscription_interface",
         "//envoy/router:route_config_provider_manager_interface",
+        "//envoy/router:scopes_interface",
         "//envoy/stats:stats_interface",
         "//source/common/common:assert_lib",
         "//source/common/common:cleanup_lib",

source/common/router/scoped_config_impl.cc

@@ -145,9 +145,7 @@ void ScopedConfigImpl::removeRoutingScopes(const std::vector<std::string>& scope
   }
 }
 
-Router::ConfigConstSharedPtr
-ScopedConfigImpl::getRouteConfig(const Http::HeaderMap& headers) const {
-  ScopeKeyPtr scope_key = scope_key_builder_.computeScopeKey(headers);
+Router::ConfigConstSharedPtr ScopedConfigImpl::getRouteConfig(const ScopeKeyPtr& scope_key) const {
   if (scope_key == nullptr) {
     return nullptr;
   }
@@ -158,14 +156,5 @@ ScopedConfigImpl::getRouteConfig(const Http::HeaderMap& headers) const {
   return nullptr;
 }
 
-ScopeKeyPtr ScopedConfigImpl::computeScopeKey(const Http::HeaderMap& headers) const {
-  ScopeKeyPtr scope_key = scope_key_builder_.computeScopeKey(headers);
-  if (scope_key &&
-      scoped_route_info_by_key_.find(scope_key->hash()) != scoped_route_info_by_key_.end()) {
-    return scope_key;
-  }
-  return nullptr;
-}
-
 } // namespace Router
 } // namespace Envoy

source/common/router/scoped_config_impl.h

@@ -55,14 +55,10 @@ class HeaderValueExtractorImpl : public FragmentBuilderBase {
 /**
  * Base class for ScopeKeyBuilder implementations.
  */
-class ScopeKeyBuilderBase {
+class ScopeKeyBuilderBase : public ScopeKeyBuilder {
 public:
   explicit ScopeKeyBuilderBase(ScopedRoutes::ScopeKeyBuilder&& config)
       : config_(std::move(config)) {}
-  virtual ~ScopeKeyBuilderBase() = default;
-
-  // Computes scope key for given headers, returns nullptr if a key can't be computed.
-  virtual ScopeKeyPtr computeScopeKey(const Http::HeaderMap& headers) const PURE;
 
 protected:
   const ScopedRoutes::ScopeKeyBuilder config_;
@@ -111,12 +107,9 @@ using ScopedRouteMap = std::map<std::string, ScopedRouteInfoConstSharedPtr>;
  */
 class ScopedConfigImpl : public ScopedConfig {
 public:
-  explicit ScopedConfigImpl(ScopedRoutes::ScopeKeyBuilder&& scope_key_builder)
-      : scope_key_builder_(std::move(scope_key_builder)) {}
+  ScopedConfigImpl() = default;
 
-  ScopedConfigImpl(ScopedRoutes::ScopeKeyBuilder&& scope_key_builder,
-                   const std::vector<ScopedRouteInfoConstSharedPtr>& scoped_route_infos)
-      : scope_key_builder_(std::move(scope_key_builder)) {
+  ScopedConfigImpl(const std::vector<ScopedRouteInfoConstSharedPtr>& scoped_route_infos) {
     addOrUpdateRoutingScopes(scoped_route_infos);
   }
 
@@ -126,12 +119,9 @@ class ScopedConfigImpl : public ScopedConfig {
   void removeRoutingScopes(const std::vector<std::string>& scope_names);
 
   // Envoy::Router::ScopedConfig
-  Router::ConfigConstSharedPtr getRouteConfig(const Http::HeaderMap& headers) const override;
-  // The return value is not null only if the scope corresponding to the header exists.
-  ScopeKeyPtr computeScopeKey(const Http::HeaderMap& headers) const override;
+  Router::ConfigConstSharedPtr getRouteConfig(const ScopeKeyPtr& scope_key) const override;
 
 private:
-  ScopeKeyBuilderImpl scope_key_builder_;
   // From scope name to cached ScopedRouteInfo.
   absl::flat_hash_map<std::string, ScopedRouteInfoConstSharedPtr> scoped_route_info_by_name_;
   // Hash by ScopeKey hash to lookup in constant time.
@@ -143,7 +133,7 @@ class ScopedConfigImpl : public ScopedConfig {
  */
 class NullScopedConfigImpl : public ScopedConfig {
 public:
-  Router::ConfigConstSharedPtr getRouteConfig(const Http::HeaderMap&) const override {
+  Router::ConfigConstSharedPtr getRouteConfig(const ScopeKeyPtr&) const override {
     return std::make_shared<const NullConfigImpl>();
   }
 };