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());