From 9bb570fded9246762d087c8d2c8fe9733ad0dbff Mon Sep 17 00:00:00 2001 From: Paul Gallagher Date: Fri, 18 Nov 2022 12:36:23 -0500 Subject: [PATCH] Add API field in Router for Upstream Filters. (#23367) Upstream filters specified in the router will apply if there are no filters specified in the cluster. Risk Level: Low. Testing: Unit and Integration testing. Signed-off-by: Paul Gallagher --- .../extensions/filters/http/router/v3/BUILD | 1 + .../filters/http/router/v3/router.proto | 19 ++- envoy/http/filter_factory.h | 6 +- source/common/filter/BUILD | 1 - source/common/router/BUILD | 4 + source/common/router/router.h | 44 ++++- source/common/router/upstream_request.cc | 17 +- source/common/upstream/BUILD | 10 ++ .../upstream_http_factory_context_impl.h | 34 ++++ source/common/upstream/upstream_impl.cc | 1 + source/common/upstream/upstream_impl.h | 29 +--- .../network/http_connection_manager/config.cc | 3 +- .../network/http_connection_manager/config.h | 2 +- source/server/admin/admin.cc | 3 +- source/server/admin/admin.h | 2 +- .../grpc_client_integration_test_harness.h | 11 +- test/common/http/async_client_impl_test.cc | 8 - .../http/conn_manager_impl_fuzz_test.cc | 6 +- test/common/http/conn_manager_impl_test.cc | 97 +++++++---- test/common/http/conn_manager_impl_test_2.cc | 30 ++-- .../http/conn_manager_impl_test_base.cc | 7 +- test/common/http/filter_manager_test.cc | 24 ++- test/common/router/BUILD | 29 ++++ test/common/router/router_test_base.cc | 10 -- .../router/router_upstream_filter_test.cc | 157 ++++++++++++++++++ .../common/router/router_upstream_log_test.cc | 8 - test/common/router/upstream_request_test.cc | 33 ++-- .../http/tcp/upstream_request_test.cc | 29 ++-- test/integration/BUILD | 19 +++ test/integration/filters/BUILD | 15 ++ test/integration/filters/add_header_filter.cc | 43 +++++ .../shadow_policy_integration_test.cc | 71 +++++++- .../upstream_http_filter_integration_test.cc | 137 +++++++++++++++ test/mocks/http/mocks.h | 5 +- test/mocks/upstream/BUILD | 1 + test/mocks/upstream/cluster_info.cc | 14 ++ test/mocks/upstream/cluster_info.h | 4 +- 37 files changed, 769 insertions(+), 165 deletions(-) create mode 100644 source/common/upstream/upstream_http_factory_context_impl.h create mode 100644 test/common/router/router_upstream_filter_test.cc create mode 100644 test/integration/filters/add_header_filter.cc create mode 100644 test/integration/upstream_http_filter_integration_test.cc diff --git a/api/envoy/extensions/filters/http/router/v3/BUILD b/api/envoy/extensions/filters/http/router/v3/BUILD index 0b02b988e42f..3e49a416a43f 100644 --- a/api/envoy/extensions/filters/http/router/v3/BUILD +++ b/api/envoy/extensions/filters/http/router/v3/BUILD @@ -7,6 +7,7 @@ licenses(["notice"]) # Apache 2 api_proto_package( deps = [ "//envoy/config/accesslog/v3:pkg", + "//envoy/extensions/filters/network/http_connection_manager/v3:pkg", "@com_github_cncf_udpa//udpa/annotations:pkg", ], ) diff --git a/api/envoy/extensions/filters/http/router/v3/router.proto b/api/envoy/extensions/filters/http/router/v3/router.proto index 68e48315ccdb..b6c4963a187a 100644 --- a/api/envoy/extensions/filters/http/router/v3/router.proto +++ b/api/envoy/extensions/filters/http/router/v3/router.proto @@ -3,6 +3,7 @@ syntax = "proto3"; package envoy.extensions.filters.http.router.v3; import "envoy/config/accesslog/v3/accesslog.proto"; +import "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto"; import "google/protobuf/wrappers.proto"; @@ -20,7 +21,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // Router :ref:`configuration overview `. // [#extension: envoy.filters.http.router] -// [#next-free-field: 8] +// [#next-free-field: 9] message Router { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.http.router.v2.Router"; @@ -89,4 +90,20 @@ message Router { // :ref:`gRPC stats filter` documentation // for more details. bool suppress_grpc_request_failure_code_stats = 7; + + // .. note:: + // Upstream HTTP filters are currently in alpha. + // + // Optional HTTP filters for the upstream filter chain. + // + // These filters will be applied for all requests that pass through the router. + // They will also be applied to shadowed requests. + // Upstream filters cannot change route or cluster. + // Upstream filters specified on the cluster will override these filters. + // + // If using upstream filters, please be aware that local errors sent by + // upstream filters will not trigger retries, and local errors sent by + // upstream filters will count as a final response if hedging is configured. + // [#extension-category: envoy.filters.http.upstream] + repeated network.http_connection_manager.v3.HttpFilter upstream_http_filters = 8; } diff --git a/envoy/http/filter_factory.h b/envoy/http/filter_factory.h index 08dc05a3e10f..eec92abd9600 100644 --- a/envoy/http/filter_factory.h +++ b/envoy/http/filter_factory.h @@ -67,8 +67,12 @@ 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. + * @return whather a filter chain has been created. */ - virtual void createFilterChain(FilterChainManager& manager) const PURE; + virtual bool createFilterChain(FilterChainManager& manager, + bool only_create_if_configured = false) const PURE; /** * Called when a new upgrade stream is created on the connection. diff --git a/source/common/filter/BUILD b/source/common/filter/BUILD index 458f107ee9aa..2a2007f99aa6 100644 --- a/source/common/filter/BUILD +++ b/source/common/filter/BUILD @@ -20,7 +20,6 @@ envoy_cc_library( "//envoy/thread_local:thread_local_interface", "//source/common/common:containers_lib", "//source/common/config:subscription_base_interface", - "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/grpc:common_lib", "//source/common/init:manager_lib", diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 5160a42a4f50..b8c15a688ca1 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -292,11 +292,13 @@ envoy_cc_library( "//envoy/http:codec_interface", "//envoy/http:codes_interface", "//envoy/http:conn_pool_interface", + "//envoy/http:filter_factory_interface", "//envoy/http:filter_interface", "//envoy/http:stateful_session_interface", "//envoy/local_info:local_info_interface", "//envoy/router:shadow_writer_interface", "//envoy/runtime:runtime_interface", + "//envoy/server:factory_context_interface", "//envoy/server:filter_config_interface", "//envoy/stats:stats_interface", "//envoy/stats:stats_macros", @@ -317,6 +319,7 @@ envoy_cc_library( "//source/common/config:utility_lib", "//source/common/grpc:common_lib", "//source/common/http:codes_lib", + "//source/common/http:filter_chain_helper_lib", "//source/common/http:filter_manager_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", @@ -330,6 +333,7 @@ envoy_cc_library( "//source/common/stream_info:uint32_accessor_lib", "//source/common/tracing:http_tracer_lib", "//source/common/upstream:load_balancer_lib", + "//source/common/upstream:upstream_http_factory_context_lib", "//source/extensions/common/proxy_protocol:proxy_protocol_header_lib", "//source/extensions/filters/http/common:factory_base_lib", "@envoy_api//envoy/extensions/filters/http/router/v3:pkg_cc_proto", diff --git a/source/common/router/router.h b/source/common/router/router.h index fba1b0071cef..d6e09f295130 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -12,10 +12,12 @@ #include "envoy/http/codec.h" #include "envoy/http/codes.h" #include "envoy/http/filter.h" +#include "envoy/http/filter_factory.h" #include "envoy/http/stateful_session.h" #include "envoy/local_info/local_info.h" #include "envoy/router/shadow_writer.h" #include "envoy/runtime/runtime.h" +#include "envoy/server/factory_context.h" #include "envoy/server/filter_config.h" #include "envoy/stats/scope.h" #include "envoy/stats/stats_macros.h" @@ -30,6 +32,7 @@ #include "source/common/common/logger.h" #include "source/common/config/utility.h" #include "source/common/config/well_known_names.h" +#include "source/common/http/filter_chain_helper.h" #include "source/common/http/utility.h" #include "source/common/router/config_impl.h" #include "source/common/router/context_impl.h" @@ -37,6 +40,7 @@ #include "source/common/stats/symbol_table.h" #include "source/common/stream_info/stream_info_impl.h" #include "source/common/upstream/load_balancer_impl.h" +#include "source/common/upstream/upstream_http_factory_context_impl.h" namespace Envoy { namespace Router { @@ -195,7 +199,7 @@ class FilterUtility { /** * Configuration for the router filter. */ -class FilterConfig { +class FilterConfig : Http::FilterChainFactory { public: FilterConfig(Stats::StatName stat_prefix, const LocalInfo::LocalInfo& local_info, Stats::Scope& scope, Upstream::ClusterManager& cm, Runtime::Loader& runtime, @@ -235,7 +239,43 @@ class FilterConfig { for (const auto& upstream_log : config.upstream_log()) { upstream_logs_.push_back(AccessLog::AccessLogFactory::fromProto(upstream_log, context)); } + if (config.upstream_http_filters_size() > 0) { + auto& server_factory_ctx = context.getServerFactoryContext(); + const Http::FilterChainUtility::FiltersList& upstream_http_filters = + config.upstream_http_filters(); + std::shared_ptr filter_config_provider_manager = + Http::FilterChainUtility::createSingletonUpstreamFilterConfigProviderManager( + server_factory_ctx); + std::string prefix = context.scope().symbolTable().toString(context.scope().prefix()); + upstream_ctx_ = std::make_unique( + server_factory_ctx, context.initManager(), context.scope()); + Http::FilterChainHelper + helper(*filter_config_provider_manager, server_factory_ctx, *upstream_ctx_, prefix); + helper.processFilters(upstream_http_filters, "router upstream http", "router upstream http", + upstream_http_filter_factories_); + } + } + + bool createFilterChain(Http::FilterChainManager& manager, + bool only_create_if_configured = false) 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; + } + Http::FilterChainUtility::createFilterChainForFactories(manager, + upstream_http_filter_factories_); + return true; + } + + bool createUpgradeFilterChain(absl::string_view, const UpgradeMap*, + Http::FilterChainManager&) const override { + // Upgrade filter chains not yet supported for upstream filters. + return false; } + using HeaderVector = std::vector; using HeaderVectorPtr = std::unique_ptr; @@ -261,6 +301,8 @@ class FilterConfig { Http::Context& http_context_; Stats::StatName zone_name_; Stats::StatName empty_stat_name_; + std::unique_ptr upstream_ctx_; + Http::FilterChainUtility::FilterFactoriesList upstream_http_filter_factories_; private: ShadowWriterPtr shadow_writer_; diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index cb592138041c..0d680148c5de 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -125,10 +125,21 @@ UpstreamRequest::UpstreamRequest(RouterFilterInterface& parent, parent_.callbacks()->connection(), parent_.callbacks()->streamId(), parent_.callbacks()->account(), true, parent_.callbacks()->decoderBufferLimit(), *parent_.cluster(), *this); - parent_.cluster()->createFilterChain(*filter_manager_); - // The cluster will always create a codec filter, which sets the upstream + // Attempt to create custom cluster-specified filter chain + bool created = parent_.cluster()->createFilterChain(*filter_manager_, + /*only_create_if_configured=*/true); + if (!created) { + // Attempt to create custom router-specified filter chain. + created = parent_.config().createFilterChain(*filter_manager_); + } + 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 = parent_.cluster()->createFilterChain(*filter_manager_, false); + } + // There will always be a codec filter present, which sets the upstream // interface. Fast-fail any tests that don't set up mocks correctly. - ASSERT(upstream_interface_.has_value()); + ASSERT(created && upstream_interface_.has_value()); } UpstreamRequest::~UpstreamRequest() { cleanUp(); } diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 76c958f39aee..2b65374c3610 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -583,3 +583,13 @@ envoy_cc_library( "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", ], ) + +envoy_cc_library( + name = "upstream_http_factory_context_lib", + hdrs = ["upstream_http_factory_context_impl.h"], + deps = [ + "//envoy/init:manager_interface", + "//envoy/server:factory_context_interface", + "//envoy/stats:stats_interface", + ], +) diff --git a/source/common/upstream/upstream_http_factory_context_impl.h b/source/common/upstream/upstream_http_factory_context_impl.h new file mode 100644 index 000000000000..44192552c1bc --- /dev/null +++ b/source/common/upstream/upstream_http_factory_context_impl.h @@ -0,0 +1,34 @@ +#pragma once + +#include "envoy/init/manager.h" +#include "envoy/server/factory_context.h" +#include "envoy/stats/scope.h" + +namespace Envoy { +namespace Upstream { + +/* + * Upstream Factory Context used by both Clusters and Routers to configure + * upstream filters. + */ +class UpstreamHttpFactoryContextImpl : public Server::Configuration::UpstreamHttpFactoryContext { +public: + UpstreamHttpFactoryContextImpl(Server::Configuration::ServerFactoryContext& context, + Init::Manager& init_manager, Stats::Scope& scope) + : server_context_(context), init_manager_(init_manager), scope_(scope) {} + + Server::Configuration::ServerFactoryContext& getServerFactoryContext() const override { + return server_context_; + } + + Init::Manager& initManager() override { return init_manager_; } + Stats::Scope& scope() override { return scope_; } + +private: + Server::Configuration::ServerFactoryContext& server_context_; + Init::Manager& init_manager_; + Stats::Scope& scope_; +}; + +} // namespace Upstream +} // namespace Envoy diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index f35d9ab2a629..b88491845f76 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1132,6 +1132,7 @@ ClusterInfoImpl::ClusterInfoImpl( if (http_protocol_options_) { Http::FilterChainUtility::FiltersList http_filters = http_protocol_options_->http_filters_; + has_configured_http_filters_ = !http_filters.empty(); if (http_filters.empty()) { auto* codec_filter = http_filters.Add(); codec_filter->set_name("envoy.filters.http.upstream_codec"); diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index e44289d419b7..ff447c6b1df7 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -55,6 +55,7 @@ #include "source/common/upstream/outlier_detection_impl.h" #include "source/common/upstream/resource_manager_impl.h" #include "source/common/upstream/transport_socket_match_impl.h" +#include "source/common/upstream/upstream_http_factory_context_impl.h" #include "source/extensions/upstreams/http/config.h" #include "source/server/transport_socket_config_impl.h" @@ -676,25 +677,6 @@ class MainPrioritySetImpl : public PrioritySetImpl, public Logger::Loggable downstream_protocol) const override; // Http::FilterChainFactory - void createFilterChain(Http::FilterChainManager& manager) const override { + bool createFilterChain(Http::FilterChainManager& manager, + bool only_create_if_configured) const override { + if (!has_configured_http_filters_ && only_create_if_configured) { + return false; + } Http::FilterChainUtility::createFilterChainForFactories(manager, http_filter_factories_); + return true; } bool createUpgradeFilterChain(absl::string_view, const UpgradeMap*, Http::FilterChainManager&) const override { @@ -952,6 +939,8 @@ class ClusterInfoImpl : public ClusterInfo, const std::unique_ptr factory_context_; std::vector filter_factories_; Http::FilterChainUtility::FilterFactoriesList http_filter_factories_; + // true iff the cluster proto specified upstream http filters. + bool has_configured_http_filters_{false}; mutable Http::Http1::CodecStats::AtomicPtr http1_codec_stats_; mutable Http::Http2::CodecStats::AtomicPtr http2_codec_stats_; mutable Http::Http3::CodecStats::AtomicPtr http3_codec_stats_; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 975bcae1d23f..c19f032cf73e 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -687,8 +687,9 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, PANIC_DUE_TO_CORRUPT_ENUM; } -void HttpConnectionManagerConfig::createFilterChain(Http::FilterChainManager& manager) const { +bool HttpConnectionManagerConfig::createFilterChain(Http::FilterChainManager& manager, bool) const { Http::FilterChainUtility::createFilterChainForFactories(manager, filter_factories_); + return true; } bool HttpConnectionManagerConfig::createUpgradeFilterChain( diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 1b34652bc912..39d861831741 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -134,7 +134,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, FilterConfigProviderManager& filter_config_provider_manager); // Http::FilterChainFactory - void createFilterChain(Http::FilterChainManager& manager) const override; + bool createFilterChain(Http::FilterChainManager& manager, bool = false) const override; using FilterFactoriesList = std::list>; struct FilterConfig { diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 3cd6059be906..9b58a1e0b9b5 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -261,11 +261,12 @@ bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, return true; } -void AdminImpl::createFilterChain(Http::FilterChainManager& manager) const { +bool AdminImpl::createFilterChain(Http::FilterChainManager& manager, bool) const { Http::FilterFactoryCb factory = [this](Http::FilterChainFactoryCallbacks& callbacks) { callbacks.addStreamFilter(std::make_shared(createRequestFunction())); }; manager.applyFilterFactoryCb({}, factory); + return true; } namespace { diff --git a/source/server/admin/admin.h b/source/server/admin/admin.h index e693e47b6cb4..829a1ac385ec 100644 --- a/source/server/admin/admin.h +++ b/source/server/admin/admin.h @@ -112,7 +112,7 @@ class AdminImpl : public Admin, Network::UdpReadFilterCallbacks&) override {} // Http::FilterChainFactory - void createFilterChain(Http::FilterChainManager& manager) const override; + bool createFilterChain(Http::FilterChainManager& manager, bool) const override; bool createUpgradeFilterChain(absl::string_view, const Http::FilterChainFactory::UpgradeMap*, Http::FilterChainManager&) const override { return false; diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index 49947606770f..40fb518b68bd 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -246,16 +246,7 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest { : method_descriptor_(helloworld::Greeter::descriptor()->FindMethodByName("SayHello")), api_(Api::createApiForTest(*stats_store_, test_time_.timeSystem())), dispatcher_(api_->allocateDispatcher("test_thread")), - http_context_(stats_store_->symbolTable()), router_context_(stats_store_->symbolTable()) { - ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, createFilterChain(_)) - .WillByDefault(Invoke([&](Http::FilterChainManager& manager) -> void { - Http::FilterFactoryCb factory_cb = - [](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - })); - } + http_context_(stats_store_->symbolTable()), router_context_(stats_store_->symbolTable()) {} virtual void initialize() { if (fake_upstream_ == nullptr) { diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index 63c0606cfef1..822b4cbe4422 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -54,14 +54,6 @@ class AsyncClientImplTest : public testing::Test { message_->headers().setPath("/"); ON_CALL(*cm_.thread_local_cluster_.conn_pool_.host_, locality()) .WillByDefault(ReturnRef(envoy::config::core::v3::Locality().default_instance())); - ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, createFilterChain(_)) - .WillByDefault(Invoke([&](Http::FilterChainManager& manager) -> void { - Http::FilterFactoryCb factory_cb = - [](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - })); cm_.initializeThreadLocalClusters({"fake_cluster"}); HttpTestUtility::addDefaultHeaders(headers_); } diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index d53ae6df5c4a..2080958c5e43 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -73,7 +73,7 @@ class FuzzConfig : public ConnectionManagerConfig { encoder_filter_ = new NiceMock(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([this](FilterChainManager& manager) -> void { + .WillOnce(Invoke([this](FilterChainManager& manager) -> bool { FilterFactoryCb decoder_filter_factory = [this](FilterChainFactoryCallbacks& callbacks) { callbacks.addStreamDecoderFilter(StreamDecoderFilterSharedPtr{decoder_filter_}); }; @@ -83,6 +83,7 @@ class FuzzConfig : public ConnectionManagerConfig { manager.applyFilterFactoryCb({}, decoder_filter_factory); manager.applyFilterFactoryCb({}, encoder_filter_factory); + return true; })); EXPECT_CALL(*decoder_filter_, setDecoderFilterCallbacks(_)) .WillOnce(Invoke([this](StreamDecoderFilterCallbacks& callbacks) -> void { @@ -93,8 +94,7 @@ class FuzzConfig : public ConnectionManagerConfig { EXPECT_CALL(filter_factory_, createUpgradeFilterChain("WebSocket", _, _)) .WillRepeatedly(Invoke([&](absl::string_view, const Http::FilterChainFactory::UpgradeMap*, FilterChainManager& manager) -> bool { - filter_factory_.createFilterChain(manager); - return true; + return filter_factory_.createFilterChain(manager); })); } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 89d863226304..833a156a2dac 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -40,9 +40,10 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponse) { EXPECT_CALL(filter_factory_, createFilterChain(_)) .Times(2) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)).Times(2); @@ -101,9 +102,10 @@ TEST_F(HttpConnectionManagerImplTest, 1xxResponse) { EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); @@ -249,9 +251,10 @@ TEST_F(HttpConnectionManagerImplTest, 1xxResponseWithDecoderPause) { EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); @@ -401,9 +404,10 @@ TEST_F(HttpConnectionManagerImplTest, InvalidPathWithDualFilter) { // This test also verifies that decoder/encoder filters have onDestroy() called only once. auto* filter = new MockStreamFilter(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createStreamFilterFactoryCb(StreamFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); @@ -445,9 +449,10 @@ TEST_F(HttpConnectionManagerImplTest, PathFailedtoSanitize) { // This test also verifies that decoder/encoder filters have onDestroy() called only once. auto* filter = new MockStreamFilter(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createStreamFilterFactoryCb(StreamFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); @@ -478,9 +483,10 @@ TEST_F(HttpConnectionManagerImplTest, FilterShouldUseSantizedPath) { auto* filter = new MockStreamFilter(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, decodeComplete()); @@ -540,7 +546,7 @@ TEST_F(HttpConnectionManagerImplTest, RouteShouldUseSantizedPath) { return route; })); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager&) -> void {})); + .WillOnce(Invoke([&](FilterChainManager&) -> bool { return false; })); // Kick off the incoming data. Buffer::OwnedImpl fake_input("1234"); @@ -617,9 +623,10 @@ TEST_F(HttpConnectionManagerImplTest, AllNormalizationsWithEscapedSlashesForward auto* filter = new MockStreamFilter(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, decodeComplete()); @@ -1141,9 +1148,10 @@ TEST_F(HttpConnectionManagerImplTest, FilterShouldUseNormalizedHost) { auto* filter = new MockStreamFilter(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, decodeComplete()); @@ -1204,7 +1212,7 @@ TEST_F(HttpConnectionManagerImplTest, RouteShouldUseNormalizedHost) { return route; })); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager&) -> void {})); + .WillOnce(Invoke([&](FilterChainManager&) -> bool { return false; })); // Kick off the incoming data. Buffer::OwnedImpl fake_input("1234"); @@ -1398,9 +1406,10 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) { std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -1468,9 +1477,10 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -1536,9 +1546,10 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -1602,9 +1613,10 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -1685,9 +1697,10 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -1769,9 +1782,10 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -1853,9 +1867,10 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -1911,9 +1926,10 @@ TEST_F(HttpConnectionManagerImplTest, std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -1952,9 +1968,10 @@ TEST_F(HttpConnectionManagerImplTest, NoHCMTracingConfigAndActiveSpanWouldBeNull std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); // Treat request as internal, otherwise x-request-id header will be overwritten. @@ -2008,12 +2025,13 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLog) { std::shared_ptr handler(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); + return true; })); EXPECT_CALL(*handler, log(_, _, _, _)) @@ -2065,12 +2083,13 @@ TEST_F(HttpConnectionManagerImplTest, TestFilterCanEnrichAccessLogs) { std::shared_ptr handler(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); + return true; })); EXPECT_CALL(*filter, onStreamComplete()).WillOnce(Invoke([&]() { @@ -2116,12 +2135,13 @@ TEST_F(HttpConnectionManagerImplTest, TestDownstreamDisconnectAccessLog) { std::shared_ptr handler(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); + return true; })); EXPECT_CALL(*handler, log(_, _, _, _)) @@ -2159,12 +2179,13 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithTrailers) { std::shared_ptr handler(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); + return true; })); EXPECT_CALL(*handler, log(_, _, _, _)) @@ -2211,12 +2232,13 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) { std::shared_ptr handler(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); + return true; })); EXPECT_CALL(*handler, log(_, _, _, _)) @@ -2267,9 +2289,10 @@ class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest auto* filter = new MockStreamFilter(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createStreamFilterFactoryCb(StreamFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); @@ -2334,12 +2357,13 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogSsl) { std::shared_ptr handler(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); + return true; })); EXPECT_CALL(*handler, log(_, _, _, _)) @@ -2394,9 +2418,10 @@ TEST_F(HttpConnectionManagerImplTest, DoNotStartSpanIfTracingIsNotEnabled) { std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*codec_, dispatch(_)) @@ -2510,9 +2535,10 @@ TEST_F(HttpConnectionManagerImplTest, AccessEncoderRouteBeforeHeadersArriveOnIdl std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb factory = createEncoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status { @@ -2592,12 +2618,13 @@ TEST_F(HttpConnectionManagerImplTest, TestStreamIdleAccessLog) { })); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); + return true; })); Buffer::OwnedImpl fake_input("1234"); @@ -2992,9 +3019,10 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterUpstreamHeaders) std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); @@ -3043,9 +3071,10 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterBidiData) { std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); @@ -3301,9 +3330,10 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnEncodeHeaders) { setup(false, ""); std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(response_encoder_, encodeHeaders(_, _)); @@ -3828,9 +3858,10 @@ TEST_F(HttpConnectionManagerImplTest, DrainClose) { MockStreamDecoderFilter* filter = new NiceMock(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, decodeHeaders(_, true)) diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 74e6daf74c28..ac20e5ba87c7 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -74,10 +74,11 @@ TEST_F(HttpConnectionManagerImplTest, ResponseStartBeforeRequestComplete) { // before the request completes, but don't finish the reply until after the request completes. MockStreamDecoderFilter* filter = new NiceMock(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, decodeHeaders(_, false)) @@ -195,12 +196,13 @@ TEST_F(HttpConnectionManagerImplTest, TestDownstreamProtocolErrorAfterHeadersAcc std::shared_ptr handler(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { FilterFactoryCb filter_factory = createDecoderFilterFactoryCb(filter); FilterFactoryCb handler_factory = createLogHandlerFactoryCb(handler); manager.applyFilterFactoryCb({}, filter_factory); manager.applyFilterFactoryCb({}, handler_factory); + return true; })); EXPECT_CALL(*handler, log(_, _, _, _)) @@ -284,9 +286,10 @@ TEST_F(HttpConnectionManagerImplTest, IdleTimeout) { MockStreamDecoderFilter* filter = new NiceMock(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*idle_timer, disableTimer()); @@ -392,9 +395,10 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionDuration) { MockStreamDecoderFilter* filter = new NiceMock(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, decodeHeaders(_, false)) @@ -2240,9 +2244,10 @@ TEST_F(HttpConnectionManagerImplTest, DisableHttp1KeepAliveWhenOverloaded) { std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*codec_, dispatch(_)) @@ -2286,9 +2291,10 @@ TEST_F(HttpConnectionManagerImplTest, DisableHttp2KeepAliveWhenOverloaded) { std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*codec_, dispatch(_)) @@ -2414,9 +2420,10 @@ TEST_F(HttpConnectionManagerImplTest, DisableKeepAliveWhenDraining) { std::shared_ptr filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*codec_, dispatch(_)) @@ -2740,9 +2747,10 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponseUsingHttp3) { EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(StreamDecoderFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); @@ -3176,9 +3184,10 @@ TEST_F(HttpConnectionManagerImplTest, HeaderValidatorRejectHttp1) { // This test also verifies that decoder/encoder filters have onDestroy() called only once. auto* filter = new MockStreamFilter(); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillOnce(Invoke([&](FilterChainManager& manager) -> void { + .WillOnce(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createStreamFilterFactoryCb(StreamFilterSharedPtr{filter}); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(*filter, setEncoderFilterCallbacks(_)); @@ -3369,9 +3378,10 @@ TEST_F(HttpConnectionManagerImplTest, HeaderValidatorAccept) { EXPECT_CALL(*filter, setDecoderFilterCallbacks(_)); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); diff --git a/test/common/http/conn_manager_impl_test_base.cc b/test/common/http/conn_manager_impl_test_base.cc index 1048162bd18f..b03dbe0dbf69 100644 --- a/test/common/http/conn_manager_impl_test_base.cc +++ b/test/common/http/conn_manager_impl_test_base.cc @@ -112,22 +112,27 @@ void HttpConnectionManagerImplTest::setupFilterChain(int num_decoder_filters, for (int req = 0; req < num_requests; req++) { EXPECT_CALL(filter_factory_, createFilterChain(_)) .WillOnce(Invoke([num_decoder_filters, num_encoder_filters, req, - this](FilterChainManager& manager) -> void { + this](FilterChainManager& manager) -> bool { + bool applied_filters = false; if (log_handler_.get()) { auto factory = createLogHandlerFactoryCb(log_handler_); manager.applyFilterFactoryCb({}, factory); + applied_filters = true; } for (int i = 0; i < num_decoder_filters; i++) { auto factory = createDecoderFilterFactoryCb( StreamDecoderFilterSharedPtr{decoder_filters_[req * num_decoder_filters + i]}); manager.applyFilterFactoryCb({}, factory); + applied_filters = true; } for (int i = 0; i < num_encoder_filters; i++) { auto factory = createEncoderFilterFactoryCb( StreamEncoderFilterSharedPtr{encoder_filters_[req * num_encoder_filters + i]}); manager.applyFilterFactoryCb({}, factory); + applied_filters = true; } + return applied_filters; })); for (int i = 0; i < num_decoder_filters; i++) { diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index 05f37e70b8a8..7f3244df3416 100644 --- a/test/common/http/filter_manager_test.cc +++ b/test/common/http/filter_manager_test.cc @@ -93,9 +93,10 @@ TEST_F(FilterManagerTest, SendLocalReplyDuringDecodingGrpcClassiciation) { .WillByDefault(Return(makeOptRef(*grpc_headers))); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto factory = createDecoderFilterFactoryCb(filter); manager.applyFilterFactoryCb({}, factory); + return true; })); filter_manager_->createFilterChain(); @@ -141,12 +142,13 @@ TEST_F(FilterManagerTest, SendLocalReplyDuringEncodingGrpcClassiciation) { })); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto decoder_factory = createDecoderFilterFactoryCb(decoder_filter); manager.applyFilterFactoryCb({}, decoder_factory); auto stream_factory = createStreamFilterFactoryCb(encoder_filter); manager.applyFilterFactoryCb({}, stream_factory); + return true; })); RequestHeaderMapPtr grpc_headers{ @@ -186,13 +188,14 @@ TEST_F(FilterManagerTest, OnLocalReply) { ON_CALL(filter_manager_callbacks_, requestHeaders()).WillByDefault(Return(makeOptRef(*headers))); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto decoder_factory = createDecoderFilterFactoryCb(decoder_filter); manager.applyFilterFactoryCb({}, decoder_factory); auto stream_factory = createStreamFilterFactoryCb(stream_filter); manager.applyFilterFactoryCb({}, stream_factory); auto encoder_factory = createEncoderFilterFactoryCb(encoder_filter); manager.applyFilterFactoryCb({}, encoder_factory); + return true; })); filter_manager_->createFilterChain(); @@ -245,13 +248,14 @@ TEST_F(FilterManagerTest, MultipleOnLocalReply) { ON_CALL(filter_manager_callbacks_, requestHeaders()).WillByDefault(Return(makeOptRef(*headers))); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto decoder_factory = createDecoderFilterFactoryCb(decoder_filter); manager.applyFilterFactoryCb({}, decoder_factory); auto stream_factory = createStreamFilterFactoryCb(stream_filter); manager.applyFilterFactoryCb({}, stream_factory); auto encoder_factory = createEncoderFilterFactoryCb(encoder_filter); manager.applyFilterFactoryCb({}, encoder_factory); + return true; })); filter_manager_->createFilterChain(); @@ -303,9 +307,10 @@ TEST_F(FilterManagerTest, ResetIdleTimer) { std::shared_ptr decoder_filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto decoder_factory = createDecoderFilterFactoryCb(decoder_filter); manager.applyFilterFactoryCb({}, decoder_factory); + return true; })); filter_manager_->createFilterChain(); @@ -321,9 +326,10 @@ TEST_F(FilterManagerTest, SetAndGetUpstreamOverrideHost) { std::shared_ptr decoder_filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto decoder_factory = createDecoderFilterFactoryCb(decoder_filter); manager.applyFilterFactoryCb({}, decoder_factory); + return true; })); filter_manager_->createFilterChain(); @@ -341,9 +347,10 @@ TEST_F(FilterManagerTest, GetRouteLevelFilterConfig) { std::shared_ptr decoder_filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto decoder_factory = createDecoderFilterFactoryCb(decoder_filter); manager.applyFilterFactoryCb({"custom-name", "filter-name"}, decoder_factory); + return true; })); filter_manager_->createFilterChain(); @@ -401,9 +408,10 @@ TEST_F(FilterManagerTest, GetRouteLevelFilterConfigForNullRoute) { std::shared_ptr decoder_filter(new NiceMock()); EXPECT_CALL(filter_factory_, createFilterChain(_)) - .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> void { + .WillRepeatedly(Invoke([&](FilterChainManager& manager) -> bool { auto decoder_factory = createDecoderFilterFactoryCb(decoder_filter); manager.applyFilterFactoryCb({"custom-name", "filter-name"}, decoder_factory); + return true; })); filter_manager_->createFilterChain(); diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 2857dfa43459..0142b99687fe 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -401,6 +401,35 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "router_upstream_filter_test", + srcs = ["router_upstream_filter_test.cc"], + deps = [ + "//source/common/buffer:buffer_lib", + "//source/common/network:utility_lib", + "//source/common/router:router_lib", + "//source/common/upstream:upstream_includes", + "//source/common/upstream:upstream_lib", + "//source/extensions/access_loggers/file:config", + "//source/extensions/upstreams/http/generic:config", + "//test/common/http:common_lib", + "//test/integration/filters:add_header_filter_config_lib", + "//test/mocks/access_log:access_log_mocks", + "//test/mocks/filesystem:filesystem_mocks", + "//test/mocks/http:http_mocks", + "//test/mocks/local_info:local_info_mocks", + "//test/mocks/network:network_mocks", + "//test/mocks/router:router_mocks", + "//test/mocks/runtime:runtime_mocks", + "//test/mocks/server:factory_context_mocks", + "//test/mocks/ssl:ssl_mocks", + "//test/test_common:test_runtime_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/extensions/filters/http/router/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "shadow_writer_impl_test", srcs = ["shadow_writer_impl_test.cc"], diff --git a/test/common/router/router_test_base.cc b/test/common/router/router_test_base.cc index f16690ba4751..f5634d19b204 100644 --- a/test/common/router/router_test_base.cc +++ b/test/common/router/router_test_base.cc @@ -42,16 +42,6 @@ RouterTestBase::RouterTestBase(bool start_child_span, bool suppress_envoy_header EXPECT_CALL(callbacks_.route_->route_entry_.early_data_policy_, allowsEarlyDataForRequest(_)) .WillRepeatedly(Invoke(Http::Utility::isSafeRequest)); - // All router based tests will fail if the codec filter is not created in the - // filter chain. By default, create a filter chain with just a codec filter. - ON_CALL(*cm_.thread_local_cluster_.cluster_.info_, createFilterChain(_)) - .WillByDefault(Invoke([&](Http::FilterChainManager& manager) -> void { - Http::FilterFactoryCb factory_cb = - [](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - })); } void RouterTestBase::expectResponseTimerCreate() { diff --git a/test/common/router/router_upstream_filter_test.cc b/test/common/router/router_upstream_filter_test.cc new file mode 100644 index 000000000000..c5ebb0b5c38a --- /dev/null +++ b/test/common/router/router_upstream_filter_test.cc @@ -0,0 +1,157 @@ +#include "envoy/extensions/filters/http/router/v3/router.pb.h" +#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" + +#include "source/common/router/router.h" +#include "source/common/router/upstream_codec_filter.h" + +#include "test/common/http/common.h" +#include "test/mocks/access_log/mocks.h" +#include "test/mocks/filesystem/mocks.h" +#include "test/mocks/http/mocks.h" +#include "test/mocks/local_info/mocks.h" +#include "test/mocks/network/mocks.h" +#include "test/mocks/router/mocks.h" +#include "test/mocks/runtime/mocks.h" +#include "test/mocks/server/factory_context.h" +#include "test/test_common/test_runtime.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::Invoke; +using testing::NiceMock; +using testing::Return; +using testing::ReturnRef; +namespace Envoy { +namespace Router { +namespace { + +using envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter; + +class TestFilter : public Filter { +public: + using Filter::Filter; + + // Filter + RetryStatePtr createRetryState(const RetryPolicy&, Http::RequestHeaderMap&, + const Upstream::ClusterInfo&, const VirtualCluster*, + RouteStatsContextOptRef, Runtime::Loader&, + Random::RandomGenerator&, Event::Dispatcher&, TimeSource&, + Upstream::ResourcePriority) override { + EXPECT_EQ(nullptr, retry_state_); + retry_state_ = new NiceMock(); + return RetryStatePtr{retry_state_}; + } + + const Network::Connection* downstreamConnection() const override { + return &downstream_connection_; + } + + NiceMock downstream_connection_; + MockRetryState* retry_state_{}; +}; + +class RouterUpstreamFilterTest : public testing::Test { +public: + void init(std::vector upstream_filters) { + envoy::extensions::filters::http::router::v3::Router router_proto; + static const std::string cluster_name = "cluster_0"; + + cluster_info_ = std::make_shared>(); + ON_CALL(*cluster_info_, name()).WillByDefault(ReturnRef(cluster_name)); + ON_CALL(*cluster_info_, observabilityName()).WillByDefault(ReturnRef(cluster_name)); + ON_CALL(callbacks_.stream_info_, upstreamClusterInfo()).WillByDefault(Return(cluster_info_)); + EXPECT_CALL(callbacks_.dispatcher_, deferredDelete_).Times(testing::AnyNumber()); + for (const auto& filter : upstream_filters) { + *router_proto.add_upstream_http_filters() = filter; + } + + Stats::StatNameManagedStorage prefix("prefix", context_.scope().symbolTable()); + config_ = std::make_shared(prefix.statName(), context_, + ShadowWriterPtr(new MockShadowWriter()), router_proto); + router_ = std::make_shared(*config_, config_->default_stats_); + router_->setDecoderFilterCallbacks(callbacks_); + EXPECT_CALL(callbacks_.dispatcher_, pushTrackedObject(_)).Times(testing::AnyNumber()); + EXPECT_CALL(callbacks_.dispatcher_, popTrackedObject(_)).Times(testing::AnyNumber()); + + upstream_locality_.set_zone("to_az"); + context_.cluster_manager_.initializeThreadLocalClusters({"fake_cluster"}); + ON_CALL(*context_.cluster_manager_.thread_local_cluster_.conn_pool_.host_, address()) + .WillByDefault(Return(host_address_)); + ON_CALL(*context_.cluster_manager_.thread_local_cluster_.conn_pool_.host_, locality()) + .WillByDefault(ReturnRef(upstream_locality_)); + router_->downstream_connection_.stream_info_.downstream_connection_info_provider_ + ->setLocalAddress(host_address_); + router_->downstream_connection_.stream_info_.downstream_connection_info_provider_ + ->setRemoteAddress(Network::Utility::parseInternetAddressAndPort("1.2.3.4:80")); + } + + Http::TestRequestHeaderMapImpl run() { + NiceMock encoder; + Http::ResponseDecoder* response_decoder = nullptr; + + EXPECT_CALL(context_.cluster_manager_.thread_local_cluster_.conn_pool_, newStream(_, _, _)) + .WillOnce(Invoke([&](Http::ResponseDecoder& decoder, + Http::ConnectionPool::Callbacks& callbacks, + const Http::ConnectionPool::Instance::StreamOptions&) + -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + EXPECT_CALL(encoder.stream_, connectionInfoProvider()) + .WillRepeatedly(ReturnRef(connection_info1_)); + callbacks.onPoolReady(encoder, + context_.cluster_manager_.thread_local_cluster_.conn_pool_.host_, + stream_info_, Http::Protocol::Http10); + return nullptr; + })); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_->decodeHeaders(headers, true); + + EXPECT_CALL(*router_->retry_state_, shouldRetryHeaders(_, _, _)) + .WillOnce(Return(RetryStatus::No)); + + Http::ResponseHeaderMapPtr response_headers(new Http::TestResponseHeaderMapImpl()); + response_headers->setStatus(200); + + EXPECT_CALL(context_.cluster_manager_.thread_local_cluster_.conn_pool_.host_->outlier_detector_, + putHttpResponseCode(200)); + // NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) + response_decoder->decodeHeaders(std::move(response_headers), true); + return headers; + } + + NiceMock context_; + + envoy::config::core::v3::Locality upstream_locality_; + Network::Address::InstanceConstSharedPtr host_address_{ + Network::Utility::resolveUrl("tcp://10.0.0.5:9211")}; + Network::Address::InstanceConstSharedPtr upstream_local_address1_{ + Network::Utility::resolveUrl("tcp://10.0.0.5:10211")}; + Network::ConnectionInfoSetterImpl connection_info1_{upstream_local_address1_, + upstream_local_address1_}; + + NiceMock callbacks_; + std::shared_ptr config_; + std::shared_ptr router_; + std::shared_ptr> cluster_info_; + NiceMock stream_info_; +}; + +TEST_F(RouterUpstreamFilterTest, UpstreamFilter) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.no_extension_lookup_by_name", "false"}}); + + HttpFilter add_header_filter; + add_header_filter.set_name("add-header-filter"); + HttpFilter codec_filter; + codec_filter.set_name("envoy.filters.http.upstream_codec"); + init({add_header_filter, codec_filter}); + auto headers = run(); + EXPECT_FALSE(headers.get(Http::LowerCaseString("x-header-to-add")).empty()); +} +} // namespace +} // namespace Router +} // namespace Envoy diff --git a/test/common/router/router_upstream_log_test.cc b/test/common/router/router_upstream_log_test.cc index 63012741c8c5..0ccdefe4aab9 100644 --- a/test/common/router/router_upstream_log_test.cc +++ b/test/common/router/router_upstream_log_test.cc @@ -121,14 +121,6 @@ class RouterUpstreamLogTest : public testing::Test { ->setLocalAddress(host_address_); router_->downstream_connection_.stream_info_.downstream_connection_info_provider_ ->setRemoteAddress(Network::Utility::parseInternetAddressAndPort("1.2.3.4:80")); - ON_CALL(*context_.cluster_manager_.thread_local_cluster_.cluster_.info_, createFilterChain(_)) - .WillByDefault(Invoke([&](Http::FilterChainManager& manager) -> void { - Http::FilterFactoryCb factory_cb = - [](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - })); } void expectResponseTimerCreate() { diff --git a/test/common/router/upstream_request_test.cc b/test/common/router/upstream_request_test.cc index 88ab1bff7e3d..da05aeee445b 100644 --- a/test/common/router/upstream_request_test.cc +++ b/test/common/router/upstream_request_test.cc @@ -28,15 +28,6 @@ class UpstreamRequestTest : public testing::Test { HttpTestUtility::addDefaultHeaders(downstream_request_header_map_); ON_CALL(router_filter_interface_, downstreamHeaders()) .WillByDefault(Return(&downstream_request_header_map_)); - - ON_CALL(*router_filter_interface_.cluster_info_, createFilterChain) - .WillByDefault(Invoke([&](Http::FilterChainManager& manager) -> void { - Http::FilterFactoryCb factory_cb = - [](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - })); } void initialize() { @@ -123,15 +114,21 @@ TEST_F(UpstreamRequestTest, AcceptRouterHeaders) { new NiceMock()); EXPECT_CALL(*router_filter_interface_.cluster_info_, createFilterChain) - .WillOnce(Invoke([&](Http::FilterChainManager& manager) -> void { - auto factory = createDecoderFilterFactoryCb(filter); - manager.applyFilterFactoryCb({}, factory); - Http::FilterFactoryCb factory_cb = - [](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - })); + .Times(2) + .WillRepeatedly( + Invoke([&](Http::FilterChainManager& manager, bool only_create_if_configured) -> 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; + })); initialize(); ASSERT_TRUE(filter->callbacks_ != nullptr); diff --git a/test/extensions/upstreams/http/tcp/upstream_request_test.cc b/test/extensions/upstreams/http/tcp/upstream_request_test.cc index 4ad701b13acd..02796da47bdf 100644 --- a/test/extensions/upstreams/http/tcp/upstream_request_test.cc +++ b/test/extensions/upstreams/http/tcp/upstream_request_test.cc @@ -38,14 +38,6 @@ class TcpConnPoolTest : public ::testing::Test { TcpConnPoolTest() : host_(std::make_shared>()) { NiceMock route_entry; NiceMock cm; - ON_CALL(*cm.thread_local_cluster_.cluster_.info_, createFilterChain(_)) - .WillByDefault(Invoke([&](Envoy::Http::FilterChainManager& manager) -> void { - Envoy::Http::FilterFactoryCb factory_cb = - [](Envoy::Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - })); cm.initializeThreadLocalClusters({"fake_cluster"}); EXPECT_CALL(cm.thread_local_cluster_, tcpConnPool(_, _)) .WillOnce(Return(Upstream::TcpPoolData([]() {}, &mock_pool_))); @@ -102,14 +94,19 @@ TEST_F(TcpConnPoolTest, Cancel) { class TcpUpstreamTest : public ::testing::Test { public: TcpUpstreamTest() { - ON_CALL(*mock_router_filter_.cluster_info_, createFilterChain(_)) - .WillByDefault(Invoke([&](Envoy::Http::FilterChainManager& manager) -> void { - Envoy::Http::FilterFactoryCb factory_cb = - [](Envoy::Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); - }; - manager.applyFilterFactoryCb({}, factory_cb); - })); + ON_CALL(*mock_router_filter_.cluster_info_, createFilterChain(_, _)) + .WillByDefault(Invoke( + [&](Envoy::Http::FilterChainManager& manager, bool only_create_if_configured) -> 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; + })); EXPECT_CALL(mock_router_filter_, downstreamHeaders()) .Times(AnyNumber()) .WillRepeatedly(Return(&request_)); diff --git a/test/integration/BUILD b/test/integration/BUILD index 6916ea960669..59dd919b486e 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -441,9 +441,28 @@ envoy_cc_test( deps = [ ":http_integration_lib", ":integration_lib", + "//test/integration/filters:add_header_filter_config_lib", "//test/integration/filters:on_local_reply_filter_config_lib", "//test/integration/filters:repick_cluster_filter_lib", "@envoy_api//envoy/extensions/access_loggers/file/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/http/router/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", + ], +) + +envoy_cc_test( + name = "upstream_http_filter_integration_test", + srcs = ["upstream_http_filter_integration_test.cc"], + deps = [ + ":http_integration_lib", + ":integration_lib", + "//test/integration/filters:add_header_filter_config_lib", + "//test/integration/filters:on_local_reply_filter_config_lib", + "//test/integration/filters:repick_cluster_filter_lib", + "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/access_loggers/file/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/http/router/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto", ], ) diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index f85648e1aa3e..a229ba07879b 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -51,6 +51,21 @@ envoy_cc_test_library( ], ) +envoy_cc_test_library( + name = "add_header_filter_config_lib", + srcs = ["add_header_filter.cc"], + deps = [ + ":common_lib", + "//envoy/http:filter_interface", + "//envoy/registry", + "//envoy/server:filter_config_interface", + "//source/extensions/filters/http/common:factory_base_lib", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/extensions/filters/http/common:empty_http_filter_config_lib", + ], + alwayslink = 1, +) + envoy_cc_test_library( name = "header_to_proxy_filter_lib", srcs = [ diff --git a/test/integration/filters/add_header_filter.cc b/test/integration/filters/add_header_filter.cc new file mode 100644 index 000000000000..392a0a87d465 --- /dev/null +++ b/test/integration/filters/add_header_filter.cc @@ -0,0 +1,43 @@ + + +#include "envoy/http/filter.h" +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "source/extensions/filters/http/common/factory_base.h" +#include "source/extensions/filters/http/common/pass_through_filter.h" + +#include "test/integration/filters/common.h" + +namespace Envoy { + +class AddHeaderFilter : public Http::PassThroughFilter { +public: + static constexpr absl::string_view kHeader = "x-header-to-add"; + AddHeaderFilter() = default; + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override { + headers.addCopy(Http::LowerCaseString(kHeader), "value"); + return Http::FilterHeadersStatus::Continue; + } +}; + +class AddHeaderFilterConfig : public Extensions::HttpFilters::Common::EmptyHttpDualFilterConfig { +public: + AddHeaderFilterConfig() : EmptyHttpDualFilterConfig("add-header-filter") {} + Http::FilterFactoryCb createDualFilter(const std::string&, + Server::Configuration::ServerFactoryContext&) override { + return [](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(std::make_shared<::Envoy::AddHeaderFilter>()); + }; + } +}; + +// perform static registration +static Registry::RegisterFactory + register_; +static Registry::RegisterFactory + register_upstream_; + +} // namespace Envoy diff --git a/test/integration/shadow_policy_integration_test.cc b/test/integration/shadow_policy_integration_test.cc index 3e60007ab230..2603e23dacaf 100644 --- a/test/integration/shadow_policy_integration_test.cc +++ b/test/integration/shadow_policy_integration_test.cc @@ -1,4 +1,8 @@ +#include + #include "envoy/extensions/access_loggers/file/v3/file.pb.h" +#include "envoy/extensions/filters/http/router/v3/router.pb.h" +#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" #include "test/integration/filters/repick_cluster_filter.h" #include "test/integration/http_integration.h" @@ -15,7 +19,10 @@ class ShadowPolicyIntegrationTest : public testing::TestWithParamadd_clusters(); cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); @@ -90,7 +97,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, ShadowPolicyIntegrationTest, // Test request mirroring / shadowing with the cluster name in policy. TEST_P(ShadowPolicyIntegrationTest, RequestMirrorPolicyWithCluster) { - intitialConfigSetup("cluster_1", ""); + initialConfigSetup("cluster_1", ""); initialize(); sendRequestAndValidateResponse(); @@ -99,9 +106,59 @@ TEST_P(ShadowPolicyIntegrationTest, RequestMirrorPolicyWithCluster) { EXPECT_EQ(test_server_->counter("cluster.cluster_0.upstream_cx_total")->value(), 1); } +// Test request mirroring / shadowing with upstream filters in the router. +TEST_P(ShadowPolicyIntegrationTest, RequestMirrorPolicyWithRouterUpstreamFilters) { + initialConfigSetup("cluster_1", ""); + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + auto* router_filter_config = hcm.mutable_http_filters(hcm.http_filters_size() - 1); + envoy::extensions::filters::http::router::v3::Router router_filter; + router_filter_config->typed_config().UnpackTo(&router_filter); + router_filter.add_upstream_http_filters()->set_name("add-body-filter"); + router_filter.add_upstream_http_filters()->set_name("envoy.filters.http.upstream_codec"); + router_filter_config->mutable_typed_config()->PackFrom(router_filter); + }); + filter_name_ = "add-body-filter"; + initialize(); + sendRequestAndValidateResponse(); + + EXPECT_EQ(upstream_headers_->getContentLengthValue(), "4"); + EXPECT_EQ(mirror_headers_->getContentLengthValue(), "4"); +} + +// Test that a cluster-specified filter will override router-specified filters. +TEST_P(ShadowPolicyIntegrationTest, ClusterFilterOverridesRouterFilter) { + initialConfigSetup("cluster_1", ""); + // main cluster adds body: + cluster_with_custom_filter_ = 0; + filter_name_ = "add-body-filter"; + + // router filter upstream filter adds header: + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + auto* router_filter_config = hcm.mutable_http_filters(hcm.http_filters_size() - 1); + envoy::extensions::filters::http::router::v3::Router router_filter; + router_filter_config->typed_config().UnpackTo(&router_filter); + router_filter.add_upstream_http_filters()->set_name("add-header-filter"); + router_filter.add_upstream_http_filters()->set_name("envoy.filters.http.upstream_codec"); + router_filter_config->mutable_typed_config()->PackFrom(router_filter); + }); + + initialize(); + sendRequestAndValidateResponse(); + // cluster_0 (main cluster) hits AddBodyFilter + EXPECT_EQ(upstream_headers_->getContentLengthValue(), "4"); + EXPECT_TRUE(upstream_headers_->get(Http::LowerCaseString("x-header-to-add")).empty()); + // cluster_1 (shadow_cluster) hits AddHeaderFilter. + EXPECT_EQ(mirror_headers_->getContentLengthValue(), ""); + EXPECT_FALSE(mirror_headers_->get(Http::LowerCaseString("x-header-to-add")).empty()); +} + // Test request mirroring / shadowing with the cluster header. TEST_P(ShadowPolicyIntegrationTest, RequestMirrorPolicyWithClusterHeaderWithFilter) { - intitialConfigSetup("", "cluster_header_1"); + initialConfigSetup("", "cluster_header_1"); // Add a filter to set cluster_header in headers. config_helper_.addFilter("name: repick-cluster-filter"); @@ -112,7 +169,7 @@ TEST_P(ShadowPolicyIntegrationTest, RequestMirrorPolicyWithClusterHeaderWithFilt // Test request mirroring / shadowing with the original cluster having a local reply filter. TEST_P(ShadowPolicyIntegrationTest, OriginalClusterWithLocalReply) { - intitialConfigSetup("cluster_1", ""); + initialConfigSetup("cluster_1", ""); cluster_with_custom_filter_ = 0; initialize(); @@ -124,7 +181,7 @@ TEST_P(ShadowPolicyIntegrationTest, OriginalClusterWithLocalReply) { // Test request mirroring / shadowing with the mirror cluster having a local reply filter. TEST_P(ShadowPolicyIntegrationTest, MirrorClusterWithLocalReply) { - intitialConfigSetup("cluster_1", ""); + initialConfigSetup("cluster_1", ""); cluster_with_custom_filter_ = 1; initialize(); @@ -135,7 +192,7 @@ TEST_P(ShadowPolicyIntegrationTest, MirrorClusterWithLocalReply) { } TEST_P(ShadowPolicyIntegrationTest, OriginalClusterWithAddBody) { - intitialConfigSetup("cluster_1", ""); + initialConfigSetup("cluster_1", ""); cluster_with_custom_filter_ = 0; filter_name_ = "add-body-filter"; @@ -164,7 +221,7 @@ TEST_P(ShadowPolicyIntegrationTest, MirrorClusterWithAddBody) { typed_config->PackFrom(router_config); }); - intitialConfigSetup("cluster_1", ""); + initialConfigSetup("cluster_1", ""); cluster_with_custom_filter_ = 1; filter_name_ = "add-body-filter"; diff --git a/test/integration/upstream_http_filter_integration_test.cc b/test/integration/upstream_http_filter_integration_test.cc new file mode 100644 index 000000000000..af550a5ccf7d --- /dev/null +++ b/test/integration/upstream_http_filter_integration_test.cc @@ -0,0 +1,137 @@ +#include +#include +#include + +#include "envoy/config/bootstrap/v3/bootstrap.pb.h" +#include "envoy/extensions/access_loggers/file/v3/file.pb.h" +#include "envoy/extensions/filters/http/router/v3/router.pb.h" +#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" + +#include "test/config/utility.h" +#include "test/integration/filters/repick_cluster_filter.h" +#include "test/integration/http_integration.h" + +#include "http_integration.h" + +namespace Envoy { +namespace { + +using HttpFilterProto = + envoy::extensions::filters::network::http_connection_manager::v3::HttpFilter; +class UpstreamHttpFilterIntegrationTest + : public testing::TestWithParam, + public HttpIntegrationTest { +public: + UpstreamHttpFilterIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP2, GetParam()) { + setUpstreamProtocol(Http::CodecType::HTTP2); + autonomous_upstream_ = true; + } + + void addFilters(const std::vector& cluster_upstream_filters, + const std::vector& router_upstream_filters) { + if (!cluster_upstream_filters.empty()) { + config_helper_.addConfigModifier( + [cluster_upstream_filters](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0); + ConfigHelper::HttpProtocolOptions protocol_options = + MessageUtil::anyConvert( + (*cluster->mutable_typed_extension_protocol_options()) + ["envoy.extensions.upstreams.http.v3.HttpProtocolOptions"]); + for (const auto& filter : cluster_upstream_filters) { + *protocol_options.add_http_filters() = filter; + } + (*cluster->mutable_typed_extension_protocol_options()) + ["envoy.extensions.upstreams.http.v3.HttpProtocolOptions"] + .PackFrom(protocol_options); + // std::cout << cluster->DebugString() << std::endl; + // std::cout << bootstrap.DebugString() << std::endl; + }); + } + if (!router_upstream_filters.empty()) { + config_helper_.addConfigModifier( + [router_upstream_filters](envoy::extensions::filters::network::http_connection_manager:: + v3::HttpConnectionManager& hcm) -> void { + HttpFilterProto& router_filter = *hcm.mutable_http_filters(0); + ASSERT_EQ(router_filter.name(), "envoy.filters.http.router"); + envoy::extensions::filters::http::router::v3::Router router; + router_filter.typed_config().UnpackTo(&router); + for (const auto& filter : router_upstream_filters) { + *router.add_upstream_http_filters() = filter; + } + router_filter.mutable_typed_config()->PackFrom(router); + std::cout << hcm.DebugString() << std::endl; + }); + } + } +}; + +TEST_P(UpstreamHttpFilterIntegrationTest, ClusterOnlyFilters) { + HttpFilterProto add_header_filter; + add_header_filter.set_name("add-header-filter"); + HttpFilterProto codec_filter; + codec_filter.set_name("envoy.filters.http.upstream_codec"); + addFilters({add_header_filter, codec_filter}, {}); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + auto upstream_headers = + reinterpret_cast(fake_upstreams_[0].get())->lastRequestHeaders(); + std::cout << *upstream_headers; + ASSERT_TRUE(upstream_headers != nullptr); + cleanupUpstreamAndDownstream(); + EXPECT_FALSE(upstream_headers->get(Http::LowerCaseString("x-header-to-add")).empty()); +} + +TEST_P(UpstreamHttpFilterIntegrationTest, RouterOnlyFilters) { + HttpFilterProto add_header_filter; + add_header_filter.set_name("add-header-filter"); + HttpFilterProto codec_filter; + codec_filter.set_name("envoy.filters.http.upstream_codec"); + addFilters({}, {add_header_filter, codec_filter}); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + auto upstream_headers = + reinterpret_cast(fake_upstreams_[0].get())->lastRequestHeaders(); + ASSERT_TRUE(upstream_headers != nullptr); + cleanupUpstreamAndDownstream(); + EXPECT_FALSE(upstream_headers->get(Http::LowerCaseString("x-header-to-add")).empty()); +} + +// Only cluster-specified filters should be applied. +TEST_P(UpstreamHttpFilterIntegrationTest, RouterAndClusterFilters) { + HttpFilterProto add_header_filter; + add_header_filter.set_name("add-header-filter"); + HttpFilterProto add_body_filter; + add_body_filter.set_name("add-body-filter"); + HttpFilterProto codec_filter; + codec_filter.set_name("envoy.filters.http.upstream_codec"); + addFilters({add_header_filter, codec_filter}, {add_body_filter, codec_filter}); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + auto upstream_headers = + reinterpret_cast(fake_upstreams_[0].get())->lastRequestHeaders(); + ASSERT_TRUE(upstream_headers != nullptr); + cleanupUpstreamAndDownstream(); + EXPECT_FALSE(upstream_headers->get(Http::LowerCaseString("x-header-to-add")).empty()); +} + +INSTANTIATE_TEST_SUITE_P(IpVersions, UpstreamHttpFilterIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +} // namespace +} // namespace Envoy diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 9bb2d0a82aad..ed9aeac9e470 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -203,7 +203,10 @@ class MockFilterChainFactory : public FilterChainFactory { ~MockFilterChainFactory() override; // Http::FilterChainFactory - MOCK_METHOD(void, createFilterChain, (FilterChainManager & manager), (const)); + bool createFilterChain(FilterChainManager& manager, bool) const override { + return createFilterChain(manager); + } + MOCK_METHOD(bool, createFilterChain, (FilterChainManager & manager), (const)); MOCK_METHOD(bool, createUpgradeFilterChain, (absl::string_view upgrade_type, const FilterChainFactory::UpgradeMap* upgrade_map, FilterChainManager& manager), diff --git a/test/mocks/upstream/BUILD b/test/mocks/upstream/BUILD index 624a336837ac..ec4625080fb1 100644 --- a/test/mocks/upstream/BUILD +++ b/test/mocks/upstream/BUILD @@ -22,6 +22,7 @@ envoy_cc_mock( "//source/common/http/http1:codec_stats_lib", "//source/common/http/http2:codec_stats_lib", "//source/common/network:raw_buffer_socket_lib", + "//source/common/router:upstream_codec_filter_lib", "//source/common/upstream:upstream_includes", "//source/common/upstream:upstream_lib", "//test/mocks/runtime:runtime_mocks", diff --git a/test/mocks/upstream/cluster_info.cc b/test/mocks/upstream/cluster_info.cc index a210516d5e07..4ac62f1fcff6 100644 --- a/test/mocks/upstream/cluster_info.cc +++ b/test/mocks/upstream/cluster_info.cc @@ -9,6 +9,7 @@ #include "source/common/config/metadata.h" #include "source/common/http/utility.h" #include "source/common/network/raw_buffer_socket.h" +#include "source/common/router/upstream_codec_filter.h" #include "source/common/upstream/upstream_impl.h" using testing::_; @@ -146,6 +147,19 @@ MockClusterInfo::MockClusterInfo() ON_CALL(*this, clusterType()).WillByDefault(ReturnRef(cluster_type_)); 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) -> 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; + })); } MockClusterInfo::~MockClusterInfo() = default; diff --git a/test/mocks/upstream/cluster_info.h b/test/mocks/upstream/cluster_info.h index a0a31e0d6c0f..b90b77d9e525 100644 --- a/test/mocks/upstream/cluster_info.h +++ b/test/mocks/upstream/cluster_info.h @@ -177,7 +177,9 @@ class MockClusterInfo : public ClusterInfo { MOCK_METHOD(std::vector, upstreamHttpProtocol, (absl::optional), (const)); - MOCK_METHOD(void, createFilterChain, (Http::FilterChainManager & manager), (const)); + MOCK_METHOD(bool, createFilterChain, + (Http::FilterChainManager & manager, bool only_create_if_configured), + (const, override)); MOCK_METHOD(bool, createUpgradeFilterChain, (absl::string_view upgrade_type, const Http::FilterChainFactory::UpgradeMap* upgrade_map,