From b7c429f534d4ceb72fdaf19a5c53d47d90fca846 Mon Sep 17 00:00:00 2001 From: code Date: Tue, 10 Dec 2024 22:12:14 +0800 Subject: [PATCH] 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) Signed-off-by: wangbaiping/wbpcode --- envoy/http/filter_factory.h | 4 +- source/common/http/BUILD | 1 + source/common/http/filter_chain_helper.cc | 18 ++++++++ source/common/http/filter_chain_helper.h | 11 +++-- source/common/http/filter_manager.cc | 9 ++-- source/common/http/filter_manager.h | 6 +-- source/common/router/router.cc | 12 ++--- source/common/router/router.h | 3 +- source/common/router/upstream_codec_filter.cc | 29 +++++++++++- source/common/router/upstream_codec_filter.h | 7 +-- source/common/router/upstream_request.cc | 10 ++--- source/common/upstream/upstream_impl.cc | 42 +++++++----------- source/common/upstream/upstream_impl.h | 7 ++- .../network/http_connection_manager/config.cc | 3 +- .../network/http_connection_manager/config.h | 2 +- source/server/admin/admin.cc | 2 +- source/server/admin/admin.h | 2 +- test/common/router/upstream_request_test.cc | 28 +++++------- test/common/upstream/upstream_impl_test.cc | 44 +++++++++++++++---- .../http/tcp/upstream_request_test.cc | 16 ++----- test/mocks/http/mocks.h | 3 +- test/mocks/upstream/cluster_info.cc | 16 ++----- test/mocks/upstream/cluster_info.h | 3 +- 23 files changed, 153 insertions(+), 125 deletions(-) diff --git a/envoy/http/filter_factory.h b/envoy/http/filter_factory.h index a53e8977a995..c3fdd9155166 100644 --- 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 index cf716abd8789..b40470475340 100644 --- 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 index 05ad8023d440..132e042135c6 100644 --- a/source/common/http/filter_chain_helper.cc +++ b/source/common/http/filter_chain_helper.cc @@ -3,6 +3,7 @@ #include #include +#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 index 24d0cda93c87..980dea1d60f2 100644 --- a/source/common/http/filter_chain_helper.h +++ b/source/common/http/filter_chain_helper.h @@ -22,6 +22,9 @@ using UpstreamFilterConfigProviderManager = Filter::FilterConfigProviderManager; +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,7 +51,7 @@ class FilterChainUtility : Logger::Loggable { bool disabled{}; }; - using FilterFactoriesList = std::list; + using FilterFactoriesList = std::vector; using FiltersList = Protobuf::RepeatedPtrField< envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter>; @@ -56,6 +59,8 @@ class FilterChainUtility : Logger::Loggable { const FilterChainOptions& options, const FilterFactoriesList& filter_factories); + static absl::Status checkUpstreamHttpFiltersList(const FiltersList& filters); + static std::shared_ptr createSingletonDownstreamFilterConfigProviderManager( Server::Configuration::ServerFactoryContext& context); @@ -80,15 +85,13 @@ class FilterChainHelper : Logger::Loggable { 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 index 858871ae3a4a..a253eb3300e5 100644 --- 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 index bbf5a0336a4a..834b1397a3fc 100644 --- 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 connection() const { return connection_; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index aafe11b7262d..bfe04be1636f 100644 --- 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 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 index 2f3d4e798faa..790522dc1211 100644 --- 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 index 9d61eae56ccc..6c4dd3c0ecc0 100644 --- 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()); + }) {} + + 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 index 01e2304d8216..ff8b4328c09d 100644 --- 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 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 index 603ea0e35eb2..9fbfe4be5f6c 100644 --- 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 index 9b70c89fe5ee..9fb63651910d 100644 --- 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 - 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 + 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 index 4b1dd7af5ee8..8a09b6ecfd40 100644 --- 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 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 index dbd320f173a6..8ff28587b283 100644 --- 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; using FilterFactoryMap = std::map; 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 index 31ce08a81728..75cbdceb33f1 100644 --- 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, // 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 index 24486459e783..7563efc2d0b5 100644 --- 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(*this)); diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index dcf07ad8eecc..1a98fe1be166 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -115,7 +115,7 @@ class AdminImpl : public Admin, bool createQuicListenerFilterChain(Network::QuicListenerFilterManager&) override { return true; } // Http::FilterChainFactory - bool createFilterChain(Http::FilterChainManager& manager, bool, + bool createFilterChain(Http::FilterChainManager& manager, const Http::FilterChainOptions&) const override; bool createUpgradeFilterChain(absl::string_view, const Http::FilterChainFactory::UpgradeMap*, Http::FilterChainManager&, diff --git a/test/common/router/upstream_request_test.cc b/test/common/router/upstream_request_test.cc index 065b05304f99..c050997b04d1 100644 --- a/test/common/router/upstream_request_test.cc +++ b/test/common/router/upstream_request_test.cc @@ -141,22 +141,18 @@ TEST_F(UpstreamRequestTest, AcceptRouterHeaders) { std::shared_ptr filter( new NiceMock()); - EXPECT_CALL(*router_filter_interface_.cluster_info_, createFilterChain) - .Times(2) - .WillRepeatedly(Invoke([&](Http::FilterChainManager& manager, bool only_create_if_configured, - const Http::FilterChainOptions&) -> bool { - if (only_create_if_configured) { - return false; - } - auto factory = createDecoderFilterFactoryCb(filter); - manager.applyFilterFactoryCb({}, factory); - Http::FilterFactoryCb factory_cb = - [](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - return true; - })); + EXPECT_CALL(*router_filter_interface_.cluster_info_, createFilterChain(_, _)) + .WillOnce( + Invoke([&](Http::FilterChainManager& manager, const Http::FilterChainOptions&) -> bool { + auto factory = createDecoderFilterFactoryCb(filter); + manager.applyFilterFactoryCb({}, factory); + Http::FilterFactoryCb factory_cb = + [](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(std::make_shared()); + }; + manager.applyFilterFactoryCb({}, factory_cb); + return true; + })); initialize(); ASSERT_TRUE(filter->callbacks_ != nullptr); diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 05afce97a74c..a977668b8ca7 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -6260,7 +6260,8 @@ TEST_F(ClusterInfoImplTest, DeprecatedMaxRequestsPerConnection) { } TEST_F(ClusterInfoImplTest, FilterChain) { - const std::string yaml = TestEnvironment::substitute(R"EOF( + { + const std::string yaml = TestEnvironment::substitute(R"EOF( name: name connect_timeout: 0.25s type: STRICT_DNS @@ -6270,15 +6271,42 @@ TEST_F(ClusterInfoImplTest, FilterChain) { explicit_http_config: http2_protocol_options: {} )EOF", - Network::Address::IpVersion::v4); + Network::Address::IpVersion::v4); - auto cluster = makeCluster(yaml); - Http::MockFilterChainManager manager; - const Http::EmptyFilterChainOptions options; - EXPECT_FALSE(cluster->info()->createUpgradeFilterChain("foo", nullptr, manager, options)); + auto cluster = makeCluster(yaml); + Http::MockFilterChainManager manager; + const Http::EmptyFilterChainOptions options; + EXPECT_FALSE(cluster->info()->createUpgradeFilterChain("foo", nullptr, manager, options)); + + EXPECT_CALL(manager, applyFilterFactoryCb(_, _)).Times(0); + EXPECT_FALSE(cluster->info()->createFilterChain(manager)); + } + + { + const std::string yaml = TestEnvironment::substitute(R"EOF( + name: name + connect_timeout: 0.25s + type: STRICT_DNS + typed_extension_protocol_options: + envoy.extensions.upstreams.http.v3.HttpProtocolOptions: + "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions + explicit_http_config: + http2_protocol_options: {} + http_filters: + - name: envoy.filters.http.upstream_codec + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.upstream_codec.v3.UpstreamCodec + )EOF", + Network::Address::IpVersion::v4); - EXPECT_CALL(manager, applyFilterFactoryCb(_, _)); - cluster->info()->createFilterChain(manager); + auto cluster = makeCluster(yaml); + Http::MockFilterChainManager manager; + const Http::EmptyFilterChainOptions options; + EXPECT_FALSE(cluster->info()->createUpgradeFilterChain("foo", nullptr, manager, options)); + + EXPECT_CALL(manager, applyFilterFactoryCb(_, _)); + EXPECT_TRUE(cluster->info()->createFilterChain(manager)); + } } class PriorityStateManagerTest : public Event::TestUsingSimulatedTime, diff --git a/test/extensions/upstreams/http/tcp/upstream_request_test.cc b/test/extensions/upstreams/http/tcp/upstream_request_test.cc index 9bacca7d18af..4f001ebfb1d7 100644 --- a/test/extensions/upstreams/http/tcp/upstream_request_test.cc +++ b/test/extensions/upstreams/http/tcp/upstream_request_test.cc @@ -93,20 +93,10 @@ TEST_F(TcpConnPoolTest, Cancel) { class TcpUpstreamTest : public ::testing::Test { public: TcpUpstreamTest() { - ON_CALL(*mock_router_filter_.cluster_info_, createFilterChain(_, _, _)) + ON_CALL(*mock_router_filter_.cluster_info_, createFilterChain(_, _)) .WillByDefault( - Invoke([&](Envoy::Http::FilterChainManager& manager, bool only_create_if_configured, - const Envoy::Http::FilterChainOptions&) -> bool { - if (only_create_if_configured) { - return false; - } - Envoy ::Http::FilterFactoryCb factory_cb = - [](Envoy::Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - return true; - })); + Invoke([&](Envoy::Http::FilterChainManager&, + const Envoy::Http::FilterChainOptions&) -> bool { return false; })); EXPECT_CALL(mock_router_filter_, downstreamHeaders()) .Times(AnyNumber()) .WillRepeatedly(Return(&request_)); diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 06e318a736e7..0822fd1f625d 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -202,8 +202,7 @@ class MockFilterChainFactory : public FilterChainFactory { ~MockFilterChainFactory() override; // Http::FilterChainFactory - bool createFilterChain(FilterChainManager& manager, bool, - const FilterChainOptions&) const override { + bool createFilterChain(FilterChainManager& manager, const FilterChainOptions&) const override { return createFilterChain(manager); } MOCK_METHOD(bool, createFilterChain, (FilterChainManager & manager), (const)); diff --git a/test/mocks/upstream/cluster_info.cc b/test/mocks/upstream/cluster_info.cc index 8177186b0b4e..8f83e5cfe1d2 100644 --- a/test/mocks/upstream/cluster_info.cc +++ b/test/mocks/upstream/cluster_info.cc @@ -158,19 +158,9 @@ MockClusterInfo::MockClusterInfo() })); ON_CALL(*this, upstreamHttpProtocol(_)) .WillByDefault(Return(std::vector{Http::Protocol::Http11})); - ON_CALL(*this, createFilterChain(_, _, _)) - .WillByDefault(Invoke([&](Http::FilterChainManager& manager, bool only_create_if_configured, - const Http::FilterChainOptions&) -> bool { - if (only_create_if_configured) { - return false; - } - Http::FilterFactoryCb factory_cb = - [](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - return true; - })); + ON_CALL(*this, createFilterChain(_, _)) + .WillByDefault(Invoke([&](Http::FilterChainManager&, + const Http::FilterChainOptions&) -> bool { return false; })); ON_CALL(*this, loadBalancerConfig()) .WillByDefault(Return(makeOptRefFromPtr(nullptr))); ON_CALL(*this, makeHeaderValidator(_)).WillByDefault(Invoke([&](Http::Protocol protocol) { diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index 978f9c5826aa..ccc0ff1927aa 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -161,8 +161,7 @@ class MockClusterInfo : public ClusterInfo { (const)); MOCK_METHOD(bool, createFilterChain, - (Http::FilterChainManager & manager, bool only_create_if_configured, - const Http::FilterChainOptions& options), + (Http::FilterChainManager & manager, const Http::FilterChainOptions& options), (const, override)); MOCK_METHOD(bool, createUpgradeFilterChain, (absl::string_view upgrade_type,