http: simplify the logic of upstream filter chain factory (#37518)

Commit Message: http: simplify the logic of upstream filter chain factory Additional Description: 1. simplify the filter chain interface and implementation. The `only_create_if_configured` only make sense for upstream filter chain and it indicates that the factory may create something even nothing is configured. It's complete a implementation detail which shouldn't be a part of interface. 2. reduce memory overhead of cluster because we needn't keep the default codec filter factorycb for every cluster now in most case. (I believe only small parts users/scenarios require to upstream filter.) Risk Level: low. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a. --------- Signed-off-by: wangbaiping(wbpcode) <[email protected]> Signed-off-by: wangbaiping/wbpcode <[email protected]>
envoyproxy · Dec 10, 2024 · b7c429f · b7c429f
1 parent 7a64f89
commit b7c429f
Show file tree

Hide file tree

Showing 23 changed files with 153 additions and 125 deletions.
diff --git a/envoy/http/filter_factory.h b/envoy/http/filter_factory.h
@@ -91,13 +91,11 @@ class FilterChainFactory {
    * Called when a new HTTP stream is created on the connection.
    * @param manager supplies the "sink" that is used for actually creating the filter chain. @see
    *                FilterChainManager.
-   * @param only_create_if_configured if true, only creates filter chain if there is a non-default
-   *                                  configured filter chain. Default false.
    * @param options additional options for creating a filter chain.
    * @return whather a filter chain has been created.
    */
   virtual bool
-  createFilterChain(FilterChainManager& manager, bool only_create_if_configured = false,
+  createFilterChain(FilterChainManager& manager,
                     const FilterChainOptions& options = EmptyFilterChainOptions{}) const PURE;
 
   /**

diff --git a/source/common/http/BUILD b/source/common/http/BUILD
@@ -186,6 +186,7 @@ envoy_cc_library(
         "//source/common/common:minimal_logger_lib",
         "//source/common/filter:config_discovery_lib",
         "//source/extensions/filters/http/common:pass_through_filter_lib",
+        "@envoy_api//envoy/extensions/filters/http/upstream_codec/v3:pkg_cc_proto",
         "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
     ],
 )

diff --git a/source/common/http/filter_chain_helper.cc b/source/common/http/filter_chain_helper.cc
@@ -3,6 +3,7 @@
 #include <memory>
 #include <string>
 
+#include "envoy/extensions/filters/http/upstream_codec/v3/upstream_codec.pb.h"
 #include "envoy/registry/registry.h"
 
 #include "source/common/common/empty_string.h"
@@ -68,5 +69,22 @@ FilterChainUtility::createSingletonDownstreamFilterConfigProviderManager(
   return downstream_filter_config_provider_manager;
 }
 
+absl::Status FilterChainUtility::checkUpstreamHttpFiltersList(const FiltersList& filters) {
+  static const std::string upstream_codec_type_url(
+      envoy::extensions::filters::http::upstream_codec::v3::UpstreamCodec::default_instance()
+          .GetTypeName());
+
+  if (!filters.empty()) {
+    const auto last_type_url = Config::Utility::getFactoryType(filters.rbegin()->typed_config());
+    if (last_type_url != upstream_codec_type_url) {
+      return absl::InvalidArgumentError(
+          fmt::format("The codec filter is the only valid terminal upstream HTTP filter, use '{}'",
+                      upstream_codec_type_url));
+    }
+  }
+
+  return absl::OkStatus();
+}
+
 } // namespace Http
 } // namespace Envoy
diff --git a/source/common/http/filter_chain_helper.h b/source/common/http/filter_chain_helper.h
@@ -22,6 +22,9 @@ using UpstreamFilterConfigProviderManager =
     Filter::FilterConfigProviderManager<Filter::HttpFilterFactoryCb,
                                         Server::Configuration::UpstreamFactoryContext>;
 
+using FiltersList = Protobuf::RepeatedPtrField<
+    envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter>;
+
 // Allows graceful handling of missing configuration for ECDS.
 class MissingConfigFilter : public Http::PassThroughDecoderFilter {
 public:
@@ -48,14 +51,16 @@ class FilterChainUtility : Logger::Loggable<Logger::Id::config> {
     bool disabled{};
   };
 
-  using FilterFactoriesList = std::list<FilterFactoryProvider>;
+  using FilterFactoriesList = std::vector<FilterFactoryProvider>;
   using FiltersList = Protobuf::RepeatedPtrField<
       envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter>;
 
   static void createFilterChainForFactories(Http::FilterChainManager& manager,
                                             const FilterChainOptions& options,
                                             const FilterFactoriesList& filter_factories);
 
+  static absl::Status checkUpstreamHttpFiltersList(const FiltersList& filters);
+
   static std::shared_ptr<DownstreamFilterConfigProviderManager>
   createSingletonDownstreamFilterConfigProviderManager(
       Server::Configuration::ServerFactoryContext& context);
@@ -80,15 +85,13 @@ class FilterChainHelper : Logger::Loggable<Logger::Id::config> {
         server_context_(server_context), cluster_manager_(cluster_manager),
         factory_context_(factory_context), stats_prefix_(stats_prefix) {}
 
-  using FiltersList = Protobuf::RepeatedPtrField<
-      envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter>;
-
   // Process the filters in this filter chain.
   absl::Status processFilters(const FiltersList& filters, const std::string& prefix,
                               const std::string& filter_chain_type,
                               FilterFactoriesList& filter_factories) {
 
     DependencyManager dependency_manager;
+    filter_factories.reserve(filters.size());
     for (int i = 0; i < filters.size(); i++) {
       absl::Status status =
           processFilter(filters[i], i, prefix, filter_chain_type, i == filters.size() - 1,

diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc
@@ -1084,7 +1084,7 @@ void DownstreamFilterManager::executeLocalReplyIfPrepared() {
 }
 
 FilterManager::CreateChainResult DownstreamFilterManager::createDownstreamFilterChain() {
-  return createFilterChain(filter_chain_factory_, false);
+  return createFilterChain(filter_chain_factory_);
 }
 
 void DownstreamFilterManager::sendLocalReplyViaFilterChain(
@@ -1659,8 +1659,7 @@ FilterManager::createUpgradeFilterChain(const FilterChainFactory& filter_chain_f
 }
 
 FilterManager::CreateChainResult
-FilterManager::createFilterChain(const FilterChainFactory& filter_chain_factory,
-                                 bool only_create_if_configured) {
+FilterManager::createFilterChain(const FilterChainFactory& filter_chain_factory) {
   if (state_.create_chain_result_.created()) {
     return state_.create_chain_result_;
   }
@@ -1686,8 +1685,8 @@ FilterManager::createFilterChain(const FilterChainFactory& filter_chain_factory,
     // create the default filter chain.
   }
 
-  state_.create_chain_result_ = CreateChainResult(
-      filter_chain_factory.createFilterChain(*this, only_create_if_configured, options), upgrade);
+  state_.create_chain_result_ =
+      CreateChainResult(filter_chain_factory.createFilterChain(*this, options), upgrade);
   return state_.create_chain_result_;
 }
 

diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h
@@ -877,12 +877,8 @@ class FilterManager : public ScopeTrackedObject,
   /**
    * Set up the Encoder/Decoder filter chain.
    * @param filter_chain_factory the factory to create the filter chain.
-   * @param only_create_if_configured whether to only create the filter chain if it is configured
-   *        explicitly. This only makes sense for upstream HTTP filter chain.
-   *
    */
-  CreateChainResult createFilterChain(const FilterChainFactory& filter_chain_factory,
-                                      bool only_create_if_configured);
+  CreateChainResult createFilterChain(const FilterChainFactory& filter_chain_factory);
 
   OptRef<const Network::Connection> connection() const { return connection_; }
 

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -106,10 +106,10 @@ FilterConfig::FilterConfig(Stats::StatName stat_prefix,
         config.upstream_log_options().upstream_log_flush_interval()));
   }
 
-  if (config.upstream_http_filters_size() > 0) {
-    auto& server_factory_ctx = context.serverFactoryContext();
-    const Http::FilterChainUtility::FiltersList& upstream_http_filters =
-        config.upstream_http_filters();
+  if (!config.upstream_http_filters().empty()) {
+    // TODO(wbpcode): To validate the terminal filter is upstream codec filter by the proto.
+    Server::Configuration::ServerFactoryContext& server_factory_ctx =
+        context.serverFactoryContext();
     std::shared_ptr<Http::UpstreamFilterConfigProviderManager> filter_config_provider_manager =
         Http::FilterChainUtility::createSingletonUpstreamFilterConfigProviderManager(
             server_factory_ctx);
@@ -120,8 +120,8 @@ FilterConfig::FilterConfig(Stats::StatName stat_prefix,
                             Server::Configuration::UpstreamHttpFilterConfigFactory>
         helper(*filter_config_provider_manager, server_factory_ctx,
                context.serverFactoryContext().clusterManager(), *upstream_ctx_, prefix);
-    SET_AND_RETURN_IF_NOT_OK(helper.processFilters(upstream_http_filters, "router upstream http",
-                                                   "router upstream http",
+    SET_AND_RETURN_IF_NOT_OK(helper.processFilters(config.upstream_http_filters(),
+                                                   "router upstream http", "router upstream http",
                                                    upstream_http_filter_factories_),
                              creation_status);
   }

diff --git a/source/common/router/router.h b/source/common/router/router.h
@@ -243,11 +243,10 @@ class FilterConfig : public Http::FilterChainFactory {
 
 public:
   bool createFilterChain(
-      Http::FilterChainManager& manager, bool only_create_if_configured = false,
+      Http::FilterChainManager& manager,
       const Http::FilterChainOptions& options = Http::EmptyFilterChainOptions{}) const override {
     // Currently there is no default filter chain, so only_create_if_configured true doesn't make
     // sense.
-    ASSERT(!only_create_if_configured);
     if (upstream_http_filter_factories_.empty()) {
       return false;
     }

diff --git a/source/common/router/upstream_codec_filter.cc b/source/common/router/upstream_codec_filter.cc
@@ -225,7 +225,7 @@ void UpstreamCodecFilter::CodecBridge::onResetStream(Http::StreamResetReason rea
     // string through the reset stream call, so append it here.
     std::string failure_reason(Http::Utility::resetReasonToString(reason));
     if (!transport_failure_reason.empty()) {
-      absl::StrAppend(&failure_reason, absl::StrCat("|", transport_failure_reason));
+      absl::StrAppend(&failure_reason, "|", transport_failure_reason);
     }
     filter_.deferred_reset_status_ = absl::InternalError(failure_reason);
     return;
@@ -248,5 +248,32 @@ void UpstreamCodecFilter::CodecBridge::onResetStream(Http::StreamResetReason rea
 REGISTER_FACTORY(UpstreamCodecFilterFactory,
                  Server::Configuration::UpstreamHttpFilterConfigFactory);
 
+class DefaultUpstreamHttpFilterChainFactory : public Http::FilterChainFactory {
+public:
+  DefaultUpstreamHttpFilterChainFactory()
+      : factory_([](Http::FilterChainFactoryCallbacks& callbacks) -> void {
+          callbacks.addStreamDecoderFilter(std::make_shared<UpstreamCodecFilter>());
+        }) {}
+
+  bool createFilterChain(
+      Http::FilterChainManager& manager,
+      const Http::FilterChainOptions& = Http::EmptyFilterChainOptions{}) const override {
+    manager.applyFilterFactoryCb({"envoy.filters.http.upstream_codec"}, factory_);
+    return true;
+  }
+  bool createUpgradeFilterChain(absl::string_view, const UpgradeMap*, Http::FilterChainManager&,
+                                const Http::FilterChainOptions&) const override {
+    // Upgrade filter chains not yet supported for upstream HTTP filters.
+    return false;
+  }
+
+private:
+  mutable Http::FilterFactoryCb factory_;
+};
+
+const Http::FilterChainFactory& defaultUpstreamHttpFilterChainFactory() {
+  CONSTRUCT_ON_FIRST_USE(DefaultUpstreamHttpFilterChainFactory);
+}
+
 } // namespace Router
 } // namespace Envoy
diff --git a/source/common/router/upstream_codec_filter.h b/source/common/router/upstream_codec_filter.h
@@ -30,8 +30,7 @@ class UpstreamCodecFilter : public Http::StreamDecoderFilter,
                             public Http::DownstreamWatermarkCallbacks,
                             public Http::UpstreamCallbacks {
 public:
-  UpstreamCodecFilter()
-      : bridge_(*this), deferred_reset_status_(absl::OkStatus()), calling_encode_headers_(false) {}
+  UpstreamCodecFilter() : bridge_(*this), deferred_reset_status_(absl::OkStatus()) {}
 
   // Http::DownstreamWatermarkCallbacks
   void onBelowWriteBufferLowWatermark() override;
@@ -90,7 +89,7 @@ class UpstreamCodecFilter : public Http::StreamDecoderFilter,
   absl::Status deferred_reset_status_;
   absl::optional<bool> latched_end_stream_;
   // Keep small members (bools and enums) at the end of class, to reduce alignment overhead.
-  bool calling_encode_headers_ : 1;
+  bool calling_encode_headers_ = false;
 
 private:
   StreamInfo::UpstreamTiming& upstreamTiming() {
@@ -122,5 +121,7 @@ class UpstreamCodecFilterFactory
 
 DECLARE_FACTORY(UpstreamCodecFilterFactory);
 
+const Http::FilterChainFactory& defaultUpstreamHttpFilterChainFactory();
+
 } // namespace Router
 } // namespace Envoy
diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc
@@ -34,6 +34,7 @@
 #include "source/common/router/config_impl.h"
 #include "source/common/router/debug_config.h"
 #include "source/common/router/router.h"
+#include "source/common/router/upstream_codec_filter.h"
 #include "source/common/stream_info/uint32_accessor_impl.h"
 #include "source/common/tracing/http_tracer_impl.h"
 #include "source/extensions/common/proxy_protocol/proxy_protocol_header.h"
@@ -142,19 +143,16 @@ UpstreamRequest::UpstreamRequest(RouterFilterInterface& parent,
       parent_.callbacks()->streamId(), parent_.callbacks()->account(), true,
       parent_.callbacks()->decoderBufferLimit(), *this);
   // Attempt to create custom cluster-specified filter chain
-  bool created = filter_manager_
-                     ->createFilterChain(*parent_.cluster(),
-                                         /*only_create_if_configured=*/true)
-                     .created();
+  bool created = filter_manager_->createFilterChain(*parent_.cluster()).created();
 
   if (!created) {
     // Attempt to create custom router-specified filter chain.
-    created = filter_manager_->createFilterChain(parent_.config(), false).created();
+    created = filter_manager_->createFilterChain(parent_.config()).created();
   }
   if (!created) {
     // Neither cluster nor router have a custom filter chain; add the default
     // cluster filter chain, which only consists of the codec filter.
-    created = filter_manager_->createFilterChain(*parent_.cluster(), false).created();
+    created = filter_manager_->createFilterChain(defaultUpstreamHttpFilterChainFactory()).created();
   }
   // There will always be a codec filter present, which sets the upstream
   // interface. Fast-fail any tests that don't set up mocks correctly.

diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc
@@ -1255,7 +1255,7 @@ ClusterInfoImpl::ClusterInfoImpl(
                   common_lb_config_->ignore_new_hosts_until_first_hc()),
       set_local_interface_name_on_upstream_connections_(
           config.upstream_connection_options().set_local_interface_name_on_upstream_connections()),
-      added_via_api_(added_via_api), has_configured_http_filters_(false),
+      added_via_api_(added_via_api),
       per_endpoint_stats_(config.has_track_cluster_stats() &&
                           config.track_cluster_stats().per_endpoint_stats()) {
 #ifdef WIN32
@@ -1405,35 +1405,23 @@ ClusterInfoImpl::ClusterInfoImpl(
   }
 
   if (http_protocol_options_) {
-    Http::FilterChainUtility::FiltersList http_filters = http_protocol_options_->http_filters_;
-    has_configured_http_filters_ = !http_filters.empty();
-    static const std::string upstream_codec_type_url(
-        envoy::extensions::filters::http::upstream_codec::v3::UpstreamCodec::default_instance()
-            .GetTypeName());
-    if (http_filters.empty()) {
-      auto* codec_filter = http_filters.Add();
-      codec_filter->set_name("envoy.filters.http.upstream_codec");
-      codec_filter->mutable_typed_config()->set_type_url(
-          absl::StrCat("type.googleapis.com/", upstream_codec_type_url));
-    } else {
-      const auto last_type_url =
-          Config::Utility::getFactoryType(http_filters[http_filters.size() - 1].typed_config());
-      if (last_type_url != upstream_codec_type_url) {
-        creation_status = absl::InvalidArgumentError(fmt::format(
-            "The codec filter is the only valid terminal upstream HTTP filter, use '{}'",
-            upstream_codec_type_url));
+    if (!http_protocol_options_->http_filters_.empty()) {
+      creation_status = Http::FilterChainUtility::checkUpstreamHttpFiltersList(
+          http_protocol_options_->http_filters_);
+      if (!creation_status.ok()) {
         return;
       }
-    }
 
-    std::string prefix = stats_scope_->symbolTable().toString(stats_scope_->prefix());
-    Http::FilterChainHelper<Server::Configuration::UpstreamFactoryContext,
-                            Server::Configuration::UpstreamHttpFilterConfigFactory>
-        helper(*http_filter_config_provider_manager_, upstream_context_.serverFactoryContext(),
-               factory_context.clusterManager(), upstream_context_, prefix);
-    SET_AND_RETURN_IF_NOT_OK(helper.processFilters(http_filters, "upstream http", "upstream http",
-                                                   http_filter_factories_),
-                             creation_status);
+      std::string prefix = stats_scope_->symbolTable().toString(stats_scope_->prefix());
+      Http::FilterChainHelper<Server::Configuration::UpstreamFactoryContext,
+                              Server::Configuration::UpstreamHttpFilterConfigFactory>
+          helper(*http_filter_config_provider_manager_, upstream_context_.serverFactoryContext(),
+                 factory_context.clusterManager(), upstream_context_, prefix);
+      SET_AND_RETURN_IF_NOT_OK(helper.processFilters(http_protocol_options_->http_filters_,
+                                                     "upstream http", "upstream http",
+                                                     http_filter_factories_),
+                               creation_status);
+    }
   }
 }
 

diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h
@@ -1022,11 +1022,12 @@ class ClusterInfoImpl : public ClusterInfo,
   upstreamHttpProtocol(absl::optional<Http::Protocol> downstream_protocol) const override;
 
   // Http::FilterChainFactory
-  bool createFilterChain(Http::FilterChainManager& manager, bool only_create_if_configured,
+  bool createFilterChain(Http::FilterChainManager& manager,
                          const Http::FilterChainOptions&) const override {
-    if (!has_configured_http_filters_ && only_create_if_configured) {
+    if (http_filter_factories_.empty()) {
       return false;
     }
+
     Http::FilterChainUtility::createFilterChainForFactories(
         manager, Http::EmptyFilterChainOptions{}, http_filter_factories_);
     return true;
@@ -1166,8 +1167,6 @@ class ClusterInfoImpl : public ClusterInfo,
   const bool warm_hosts_ : 1;
   const bool set_local_interface_name_on_upstream_connections_ : 1;
   const bool added_via_api_ : 1;
-  // true iff the cluster proto specified upstream http filters.
-  bool has_configured_http_filters_ : 1;
   const bool per_endpoint_stats_ : 1;
 };
 

diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc
@@ -45,7 +45,6 @@ namespace NetworkFilters {
 namespace HttpConnectionManager {
 namespace {
 
-using FilterFactoriesList = std::list<Http::FilterFactoryCb>;
 using FilterFactoryMap = std::map<std::string, HttpConnectionManagerConfig::FilterConfig>;
 
 HttpConnectionManagerConfig::UpgradeMap::const_iterator
@@ -790,7 +789,7 @@ Http::ServerConnectionPtr HttpConnectionManagerConfig::createCodec(
   PANIC_DUE_TO_CORRUPT_ENUM;
 }
 
-bool HttpConnectionManagerConfig::createFilterChain(Http::FilterChainManager& manager, bool,
+bool HttpConnectionManagerConfig::createFilterChain(Http::FilterChainManager& manager,
                                                     const Http::FilterChainOptions& options) const {
   Http::FilterChainUtility::createFilterChainForFactories(manager, options, filter_factories_);
   return true;

diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h
@@ -139,7 +139,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
 
   // Http::FilterChainFactory
   bool createFilterChain(
-      Http::FilterChainManager& manager, bool = false,
+      Http::FilterChainManager& manager,
       const Http::FilterChainOptions& = Http::EmptyFilterChainOptions{}) const override;
   using FilterFactoriesList = Envoy::Http::FilterChainUtility::FilterFactoriesList;
   struct FilterConfig {

diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc
@@ -304,7 +304,7 @@ bool AdminImpl::createNetworkFilterChain(Network::Connection& connection,
   return true;
 }
 
-bool AdminImpl::createFilterChain(Http::FilterChainManager& manager, bool,
+bool AdminImpl::createFilterChain(Http::FilterChainManager& manager,
                                   const Http::FilterChainOptions&) const {
   Http::FilterFactoryCb factory = [this](Http::FilterChainFactoryCallbacks& callbacks) {
     callbacks.addStreamFilter(std::make_shared<AdminFilter>(*this));