From 0ccc70ae77909baadcb07dd0c9ca2ef583dde3b5 Mon Sep 17 00:00:00 2001 From: qiannawang <38573959+qiannawang@users.noreply.github.com> Date: Thu, 18 Oct 2018 16:12:33 -0700 Subject: [PATCH] common: added a field in HCM proto to be able to reverse the order of HTTP encoder filters. (#4721) Added a field in HCM proto to be able to reverse the order of HTTP encoder filters. The field is set false by default, indicating HTTP encoder filters have the same order as configured in the filter chain. If true, their order will be reversed. Risk Level: low Testing: bazel test //test/... Part of #4599 Signed-off-by: Qi (Anna) Wang --- DEPRECATED.md | 2 + .../v2/http_connection_manager.proto | 11 +- source/common/http/conn_manager_config.h | 6 + source/common/http/conn_manager_impl.cc | 6 +- .../network/http_connection_manager/config.cc | 4 +- .../network/http_connection_manager/config.h | 2 + source/server/http/admin.h | 1 + .../http/conn_manager_impl_fuzz_test.cc | 1 + test/common/http/conn_manager_impl_test.cc | 232 ++++++++++++++---- test/common/http/conn_manager_utility_test.cc | 1 + 10 files changed, 213 insertions(+), 53 deletions(-) diff --git a/DEPRECATED.md b/DEPRECATED.md index 447a42885870..674c950365d4 100644 --- a/DEPRECATED.md +++ b/DEPRECATED.md @@ -8,6 +8,8 @@ A logged warning is expected for each deprecated item that is in deprecation win ## Version 1.9.0 (pending) +* Order of execution of the encoder filter chain has been reversed. Prior to this release cycle it was incorrect, see [#4599](https://github.com/envoyproxy/envoy/issues/4599). In the 1.9.0 release cycle we introduced `bugfix_reverse_encode_order` in [http_connection_manager.proto] (https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto) to temporarily support both old and new behaviors. Note this boolean field is deprecated. + ## Version 1.8.0 (Oct 4, 2018) * Use of the v1 API (including `*.deprecated_v1` fields in the v2 API) is deprecated. diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index 4c8c93acce67..c7d8a1a6f50c 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -19,7 +19,7 @@ import "gogoproto/gogo.proto"; // [#protodoc-title: HTTP connection manager] // HTTP connection manager :ref:`configuration overview `. -// [#comment:next free field: 27] +// [#comment:next free field: 28] message HttpConnectionManager { enum CodecType { option (gogoproto.goproto_enum_prefix) = false; @@ -348,6 +348,15 @@ message HttpConnectionManager { repeated HttpFilter filters = 2; }; repeated UpgradeConfig upgrade_configs = 23; + + // If true, the order of encoder filters will be reversed to that of filters + // configured in the HTTP filter chain. Otherwise, it will keep the existing + // order. + // Note: this is a bug fix for Envoy, which is designed to have the reversed + // order of encode filters to that of decode ones, (see + // https://github.com/envoyproxy/envoy/issues/4599 for details). When we remove this field, envoy + // will have the same behavior when it sets true. + google.protobuf.BoolValue bugfix_reverse_encode_order = 27 [deprecated = true]; } message Rds { diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index e10875ecd807..b375c36ae0a8 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -209,6 +209,12 @@ class ConnectionManagerConfig { */ virtual FilterChainFactory& filterFactory() PURE; + /** + * @return whether the connection manager will reverse the order of encoder + * filters in the HTTP filter chain. + */ + virtual bool reverseEncodeOrder() PURE; + /** * @return whether the connection manager will generate a fresh x-request-id if the request does * not have one. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index a3aaa9976ba6..703649b2d4f7 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -458,7 +458,11 @@ void ConnectionManagerImpl::ActiveStream::addStreamEncoderFilterWorker( StreamEncoderFilterSharedPtr filter, bool dual_filter) { ActiveStreamEncoderFilterPtr wrapper(new ActiveStreamEncoderFilter(*this, filter, dual_filter)); filter->setEncoderFilterCallbacks(*wrapper); - wrapper->moveIntoListBack(std::move(wrapper), encoder_filters_); + if (connection_manager_.config_.reverseEncodeOrder()) { + wrapper->moveIntoList(std::move(wrapper), encoder_filters_); + } else { + wrapper->moveIntoListBack(std::move(wrapper), encoder_filters_); + } } void ConnectionManagerImpl::ActiveStream::addAccessLogHandler( diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index c80e3983082e..ef67aedde148 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -134,7 +134,9 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( config, Server::Configuration::FactoryContext& context, Http::DateProvider& date_provider, Router::RouteConfigProviderManager& route_config_provider_manager) - : context_(context), stats_prefix_(fmt::format("http.{}.", config.stat_prefix())), + : context_(context), reverse_encode_order_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( + config, bugfix_reverse_encode_order, false)), + stats_prefix_(fmt::format("http.{}.", config.stat_prefix())), stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())), tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats(stats_prefix_, context_.scope())), diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index a27d609d4d04..5ecf94770069 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -107,6 +107,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::DateProvider& dateProvider() override { return date_provider_; } std::chrono::milliseconds drainTimeout() override { return drain_timeout_; } FilterChainFactory& filterFactory() override { return *this; } + bool reverseEncodeOrder() override { return reverse_encode_order_; } bool generateRequestId() override { return generate_request_id_; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } @@ -145,6 +146,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Server::Configuration::FactoryContext& context_; FilterFactoriesList filter_factories_; std::map> upgrade_filter_factories_; + const bool reverse_encode_order_{}; std::list access_logs_; const std::string stats_prefix_; Http::ConnectionManagerStats stats_; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index a8d5b094a563..e85bb0793395 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -96,6 +96,7 @@ class AdminImpl : public Admin, Http::DateProvider& dateProvider() override { return date_provider_; } std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); } Http::FilterChainFactory& filterFactory() override { return *this; } + bool reverseEncodeOrder() override { return false; } bool generateRequestId() override { return false; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return {}; } diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 7fb359966cdf..22877c9c9aee 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -88,6 +88,7 @@ class FuzzConfig : public ConnectionManagerConfig { DateProvider& dateProvider() override { return date_provider_; } std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); } FilterChainFactory& filterFactory() override { return filter_factory_; } + bool reverseEncodeOrder() override { return true; } bool generateRequestId() override { return true; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 11d6af58dc81..3a836ca60408 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -199,8 +199,13 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi EXPECT_CALL(*filter, onDestroy()); } - for (auto filter : encoder_filters_) { + auto setup_filter_expect = [](MockStreamEncoderFilter* filter) { EXPECT_CALL(*filter, onDestroy()); + }; + if (reverse_encode_order_) { + std::for_each(encoder_filters_.rbegin(), encoder_filters_.rend(), setup_filter_expect); + } else { + std::for_each(encoder_filters_.begin(), encoder_filters_.end(), setup_filter_expect); } EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, deferredDelete_(_)); @@ -257,6 +262,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi DateProvider& dateProvider() override { return date_provider_; } std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); } FilterChainFactory& filterFactory() override { return filter_factory_; } + bool reverseEncodeOrder() override { return reverse_encode_order_; } bool generateRequestId() override { return true; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } @@ -294,6 +300,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi NiceMock filter_callbacks_; MockServerConnection* codec_; NiceMock filter_factory_; + bool reverse_encode_order_{true}; ConnectionManagerStats stats_; ConnectionManagerTracingStats tracing_stats_; NiceMock drain_close_; @@ -506,7 +513,41 @@ TEST_F(HttpConnectionManagerImplTest, PauseResume100Continue) { setUpEncoderAndDecoder(); sendRequestHeadersAndData(); - // Stop the 100-Continue at filter 0. Filter 1 should not yet receive the 100-Continue + // Stop the 100-Continue at encoder filter 1. Encoder filter 0 should not yet receive the + // 100-Continue + EXPECT_CALL(*encoder_filters_[1], encode100ContinueHeaders(_)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + EXPECT_CALL(*encoder_filters_[0], encode100ContinueHeaders(_)).Times(0); + EXPECT_CALL(response_encoder_, encode100ContinueHeaders(_)).Times(0); + HeaderMapPtr continue_headers{new TestHeaderMapImpl{{":status", "100"}}}; + decoder_filters_[1]->callbacks_->encode100ContinueHeaders(std::move(continue_headers)); + + // Have the encoder filter 1 continue. Make sure the 100-Continue is resumed as expected. + EXPECT_CALL(*encoder_filters_[0], encode100ContinueHeaders(_)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(response_encoder_, encode100ContinueHeaders(_)); + encoder_filters_[1]->callbacks_->continueEncoding(); + + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); + HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; + decoder_filters_[1]->callbacks_->encodeHeaders(std::move(response_headers), false); +} + +// PauseResume100ContinueWithInorderEncoders verifies the same case of PauseResume100Continue, +// except that the encode filters are placed in the same order to the configuration. +TEST_F(HttpConnectionManagerImplTest, PauseResume100ContinueWithInorderEncoders) { + reverse_encode_order_ = false; + proxy_100_continue_ = true; + setup(false, "envoy-custom-server", false); + setUpEncoderAndDecoder(); + sendRequestHeadersAndData(); + + // Stop the 100-Continue at encoder filter 0. Encoder filter 1 should not yet receive the + // 100-Continue EXPECT_CALL(*encoder_filters_[0], encode100ContinueHeaders(_)) .WillOnce(Return(FilterHeadersStatus::StopIteration)); EXPECT_CALL(*encoder_filters_[1], encode100ContinueHeaders(_)).Times(0); @@ -514,7 +555,7 @@ TEST_F(HttpConnectionManagerImplTest, PauseResume100Continue) { HeaderMapPtr continue_headers{new TestHeaderMapImpl{{":status", "100"}}}; decoder_filters_[0]->callbacks_->encode100ContinueHeaders(std::move(continue_headers)); - // Have the filter 0 continue. Make sure the 100-Continue is resumed as expected. + // Have the encoder filter 0 continue. Make sure the 100-Continue is resumed as expected. EXPECT_CALL(*encoder_filters_[1], encode100ContinueHeaders(_)) .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(response_encoder_, encode100ContinueHeaders(_)); @@ -2315,10 +2356,10 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddTrailersInTrailersCallback) { conn_manager_->onData(fake_input, false); // set up encodeHeaders expectations - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) - .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); // invoke encodeHeaders @@ -2326,20 +2367,20 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddTrailersInTrailersCallback) { HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}}, false); // set up encodeData expectations - EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) - .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) + .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(response_encoder_, encodeData(_, false)); // invoke encodeData Buffer::OwnedImpl response_body("response"); decoder_filters_[0]->callbacks_->encodeData(response_body, false); // set up encodeTrailer expectations - EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_)) + EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_)) .WillOnce(Return(FilterTrailersStatus::Continue)); - EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_)) + EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_)) .WillOnce(Invoke([&](Http::HeaderMap& trailers) -> FilterTrailersStatus { // assert that the trailers set in the previous filter was ignored Http::LowerCaseString key("foo"); @@ -2401,10 +2442,10 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddTrailersInDataCallbackNoTrailers) conn_manager_->onData(fake_input, false); // set up encodeHeaders expectations - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) - .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); // invoke encodeHeaders @@ -2412,19 +2453,19 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddTrailersInDataCallbackNoTrailers) HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}}, false); // set up encodeData expectations - EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) + EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) .WillOnce(InvokeWithoutArgs([&]() -> FilterDataStatus { - encoder_filters_[0]->callbacks_->addEncodedTrailers().addCopy(trailer_key, trailers_data); + encoder_filters_[1]->callbacks_->addEncodedTrailers().addCopy(trailer_key, trailers_data); return FilterDataStatus::Continue; })); // ensure encodeData calls after setting header sees end_stream = false - EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) + EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(response_encoder_, encodeData(_, false)); // since we added trailers, we should see encodeTrailer callbacks - EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_)).WillOnce(Invoke([&](HeaderMap& trailers) { + EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_)).WillOnce(Invoke([&](HeaderMap& trailers) { // ensure that we see the trailers set in decodeData Http::LowerCaseString key("foo"); auto t = trailers.get(key); @@ -2483,32 +2524,32 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddBodyInTrailersCallback) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input, false); - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) - .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); decoder_filters_[1]->callbacks_->encodeHeaders( HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}}, false); - EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) - .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) + .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(response_encoder_, encodeData(_, false)); Buffer::OwnedImpl response_body("response"); decoder_filters_[1]->callbacks_->encodeData(response_body, false); - EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_)) + EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_)) .WillOnce(InvokeWithoutArgs([&]() -> FilterTrailersStatus { - encoder_filters_[0]->callbacks_->addEncodedData(trailers_data, true); + encoder_filters_[1]->callbacks_->addEncodedData(trailers_data, true); return FilterTrailersStatus::Continue; })); - EXPECT_CALL(*encoder_filters_[1], encodeData(Ref(trailers_data), false)) + EXPECT_CALL(*encoder_filters_[0], encodeData(Ref(trailers_data), false)) .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(response_encoder_, encodeData(_, false)); - EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_)) + EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_)) .WillOnce(Return(FilterTrailersStatus::Continue)); EXPECT_CALL(response_encoder_, encodeTrailers(_)); expectOnDestroy(); @@ -2556,20 +2597,20 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddBodyDuringDecodeData) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input, false); - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::StopIteration)); - EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) + EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) .WillOnce(Return(FilterDataStatus::StopIterationAndBuffer)); - EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) + EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) .WillOnce(Invoke([&](Buffer::Instance& data, bool) -> FilterDataStatus { - encoder_filters_[0]->callbacks_->addEncodedData(data, true); - EXPECT_EQ(encoder_filters_[0]->callbacks_->encodingBuffer()->toString(), "goodbye"); + encoder_filters_[1]->callbacks_->addEncodedData(data, true); + EXPECT_EQ(encoder_filters_[1]->callbacks_->encodingBuffer()->toString(), "goodbye"); return FilterDataStatus::Continue; })); - EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); - EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) + EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(response_encoder_, encodeData(_, true)); expectOnDestroy(); @@ -2610,17 +2651,17 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddBodyInline) { Buffer::OwnedImpl fake_input("1234"); conn_manager_->onData(fake_input, false); - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, true)) + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, true)) .WillOnce(InvokeWithoutArgs([&]() -> FilterHeadersStatus { Buffer::OwnedImpl data("hello"); - encoder_filters_[0]->callbacks_->addEncodedData(data, true); + encoder_filters_[1]->callbacks_->addEncodedData(data, true); EXPECT_EQ(5UL, encoder_filters_[0]->callbacks_->encodingBuffer()->length()); return FilterHeadersStatus::Continue; })); - EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); - EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) + EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(response_encoder_, encodeData(_, true)); expectOnDestroy(); @@ -2896,7 +2937,7 @@ TEST_F(HttpConnectionManagerImplTest, HitFilterWatermarkLimits) { // Start the response HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::StopIteration)); decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false); @@ -2907,14 +2948,14 @@ TEST_F(HttpConnectionManagerImplTest, HitFilterWatermarkLimits) { // callbacks should be called. EXPECT_CALL(callbacks, onAboveWriteBufferHighWatermark()); Buffer::OwnedImpl fake_response("A long enough string to go over watermarks"); - EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) + EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) .WillOnce(Return(FilterDataStatus::StopIterationAndWatermark)); decoder_filters_[0]->callbacks_->encodeData(fake_response, false); // Change the limit so the buffered data is below the new watermark. - buffer_len = encoder_filters_[0]->callbacks_->encodingBuffer()->length(); + buffer_len = encoder_filters_[1]->callbacks_->encodingBuffer()->length(); EXPECT_CALL(callbacks, onBelowWriteBufferLowWatermark()); - encoder_filters_[0]->callbacks_->setEncoderBufferLimit((buffer_len + 1) * 2); + encoder_filters_[1]->callbacks_->setEncoderBufferLimit((buffer_len + 1) * 2); } TEST_F(HttpConnectionManagerImplTest, HitRequestBufferLimits) { @@ -2928,9 +2969,9 @@ TEST_F(HttpConnectionManagerImplTest, HitRequestBufferLimits) { // watermark limit and result in a 413 being sent to the user. Http::TestHeaderMapImpl response_headers{ {":status", "413"}, {"content-length", "17"}, {"content-type", "text/plain"}}; - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(HeaderMapEqualRef(&response_headers), false)) + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(HeaderMapEqualRef(&response_headers), false)) .WillOnce(Return(FilterHeadersStatus::StopIteration)); - EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) + EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) .WillOnce(Return(FilterDataStatus::StopIterationAndWatermark)); Buffer::OwnedImpl data("A longer string"); decoder_filters_[0]->callbacks_->addDecodedData(data, false); @@ -2985,7 +3026,7 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsBeforeHeaders) { // Start the response without processing the request headers through all // filters. HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::StopIteration)); decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false); @@ -2997,7 +3038,7 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsBeforeHeaders) { {":status", "500"}, {"content-length", "21"}, {"content-type", "text/plain"}}; Buffer::OwnedImpl fake_response("A long enough string to go over watermarks"); // Fake response starts doing through the filter. - EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) + EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) .WillOnce(Return(FilterDataStatus::StopIterationAndBuffer)); std::string response_body; // The 500 goes directly to the encoder. @@ -3018,10 +3059,10 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsAfterHeaders) { // Start the response, and make sure the request headers are fully processed. HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) - .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false); @@ -3030,7 +3071,7 @@ TEST_F(HttpConnectionManagerImplTest, HitResponseBufferLimitsAfterHeaders) { const std::string data = "A long enough string to go over watermarks"; Buffer::OwnedImpl fake_response(data); InSequence s; - EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) + EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) .WillOnce(Return(FilterDataStatus::StopIterationAndBuffer)); EXPECT_CALL(stream_, resetStream(_)); decoder_filters_[0]->callbacks_->encodeData(fake_response, false); @@ -3099,23 +3140,23 @@ TEST_F(HttpConnectionManagerImplTest, FilterAddBodyContinuation) { decoder_filters_[0]->callbacks_->addDecodedData(data, true); decoder_filters_[0]->callbacks_->continueDecoding(); - EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, true)) + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, true)) .WillOnce(Return(FilterHeadersStatus::StopIteration)); decoder_filters_[1]->callbacks_->encodeHeaders( HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}}, true); - EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::Continue)); EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); - EXPECT_CALL(*encoder_filters_[1], encodeData(_, true)) + EXPECT_CALL(*encoder_filters_[0], encodeData(_, true)) .WillOnce(Return(FilterDataStatus::Continue)); EXPECT_CALL(response_encoder_, encodeData(_, true)); expectOnDestroy(); Buffer::OwnedImpl data2("hello"); - encoder_filters_[0]->callbacks_->addEncodedData(data2, true); - encoder_filters_[0]->callbacks_->continueEncoding(); + encoder_filters_[1]->callbacks_->addEncodedData(data2, true); + encoder_filters_[1]->callbacks_->continueEncoding(); } TEST_F(HttpConnectionManagerImplTest, MultipleFilters) { @@ -3174,6 +3215,97 @@ TEST_F(HttpConnectionManagerImplTest, MultipleFilters) { .WillOnce(Return(FilterDataStatus::StopIterationNoBuffer)); decoder_filters_[0]->callbacks_->continueDecoding(); + // Now start encoding and mimic trapping in the encoding filter. + EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + EXPECT_CALL(*encoder_filters_[1], encodeData(_, false)) + .WillOnce(Return(FilterDataStatus::StopIterationAndBuffer)); + EXPECT_CALL(*encoder_filters_[1], encodeTrailers(_)) + .WillOnce(Return(FilterTrailersStatus::StopIteration)); + EXPECT_EQ(ssl_connection_.get(), encoder_filters_[1]->callbacks_->connection()->ssl()); + decoder_filters_[2]->callbacks_->encodeHeaders( + HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}}, false); + Buffer::OwnedImpl response_body("response"); + decoder_filters_[2]->callbacks_->encodeData(response_body, false); + decoder_filters_[2]->callbacks_->encodeTrailers( + HeaderMapPtr{new TestHeaderMapImpl{{"some", "trailer"}}}); + EXPECT_EQ(ssl_connection_.get(), decoder_filters_[2]->callbacks_->connection()->ssl()); + + // Now finish the encode. + EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::Continue)); + EXPECT_CALL(response_encoder_, encodeHeaders(_, false)); + EXPECT_CALL(*encoder_filters_[0], encodeData(_, false)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(response_encoder_, encodeData(_, false)); + EXPECT_CALL(*encoder_filters_[0], encodeTrailers(_)) + .WillOnce(Return(FilterTrailersStatus::Continue)); + EXPECT_CALL(response_encoder_, encodeTrailers(_)); + expectOnDestroy(); + encoder_filters_[1]->callbacks_->continueEncoding(); + + EXPECT_EQ(ssl_connection_.get(), encoder_filters_[0]->callbacks_->connection()->ssl()); +} + +// MultipleFiltersWithInorderEncoders verifies the same case of MultipleFilters, except that the +// encode filters are placed in the same order to the configuration. +TEST_F(HttpConnectionManagerImplTest, MultipleFiltersWithInorderEncoders) { + reverse_encode_order_ = false; + InSequence s; + setup(false, ""); + + EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> void { + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}}; + decoder->decodeHeaders(std::move(headers), false); + + Buffer::OwnedImpl fake_data("hello"); + decoder->decodeData(fake_data, false); + + Buffer::OwnedImpl fake_data2("world"); + decoder->decodeData(fake_data2, true); + })); + + setupFilterChain(3, 2); + + // Test route caching. + EXPECT_CALL(*route_config_provider_.route_config_, route(_, _)); + + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false)) + .WillOnce(InvokeWithoutArgs([&]() -> FilterHeadersStatus { + EXPECT_EQ(route_config_provider_.route_config_->route_, + decoder_filters_[0]->callbacks_->route()); + EXPECT_EQ(ssl_connection_.get(), decoder_filters_[0]->callbacks_->connection()->ssl()); + return FilterHeadersStatus::StopIteration; + })); + + EXPECT_CALL(*decoder_filters_[0], decodeData(_, false)) + .WillOnce(Return(FilterDataStatus::StopIterationAndBuffer)); + EXPECT_CALL(*decoder_filters_[0], decodeData(_, true)) + .WillOnce(Return(FilterDataStatus::StopIterationAndBuffer)); + + // Kick off the incoming data. + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + // Mimic a decoder filter that trapped data and now sends it on, since the data was buffered + // by the first filter, we expect to get it in 1 decodeData() call. + EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, false)) + .WillOnce(InvokeWithoutArgs([&]() -> FilterHeadersStatus { + EXPECT_EQ(route_config_provider_.route_config_->route_, + decoder_filters_[1]->callbacks_->route()); + EXPECT_EQ(ssl_connection_.get(), decoder_filters_[1]->callbacks_->connection()->ssl()); + return FilterHeadersStatus::StopIteration; + })); + EXPECT_CALL(*decoder_filters_[1], decodeData(_, true)) + .WillOnce(Return(FilterDataStatus::Continue)); + EXPECT_CALL(*decoder_filters_[2], decodeHeaders(_, false)) + .WillOnce(Return(FilterHeadersStatus::StopIteration)); + EXPECT_CALL(*decoder_filters_[2], decodeData(_, true)) + .WillOnce(Return(FilterDataStatus::StopIterationNoBuffer)); + decoder_filters_[0]->callbacks_->continueDecoding(); + // Now start encoding and mimic trapping in the encoding filter. EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, false)) .WillOnce(Return(FilterHeadersStatus::StopIteration)); diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 7bf0383541f8..c10127c0b5c8 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -49,6 +49,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(dateProvider, DateProvider&()); MOCK_METHOD0(drainTimeout, std::chrono::milliseconds()); MOCK_METHOD0(filterFactory, FilterChainFactory&()); + MOCK_METHOD0(reverseEncodeOrder, bool()); MOCK_METHOD0(generateRequestId, bool()); MOCK_CONST_METHOD0(idleTimeout, absl::optional()); MOCK_CONST_METHOD0(streamIdleTimeout, std::chrono::milliseconds());