diff --git a/DEPRECATED.md b/DEPRECATED.md index c854e2b2205a..4ee038be8875 100644 --- a/DEPRECATED.md +++ b/DEPRECATED.md @@ -8,7 +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. +* Order of execution of the network write 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_write_filter_order` in [lds.proto] (https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/lds.proto) to temporarily support both old and new behaviors. Note this boolean field is deprecated. +* Order of execution of the HTTP 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. * Use of the v1 REST_LEGACY ApiConfigSource is deprecated. * Use of std::hash in the ring hash load balancer is deprecated. diff --git a/api/envoy/api/v2/lds.proto b/api/envoy/api/v2/lds.proto index 0099eeabaa52..22547864bcbb 100644 --- a/api/envoy/api/v2/lds.proto +++ b/api/envoy/api/v2/lds.proto @@ -163,4 +163,12 @@ message Listener { // On macOS, only values of 0, 1, and unset are valid; other values may result in an error. // To set the queue length on macOS, set the net.inet.tcp.fastopen_backlog kernel parameter. google.protobuf.UInt32Value tcp_fast_open_queue_length = 12; + + // If true, the order of write filters will be reversed to that of filters + // configured in the 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 write filters to that of read 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_write_filter_order = 14 [deprecated = true]; } diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index dbc231036843..1545d4b30e9f 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -261,6 +261,22 @@ class Connection : public Event::DeferredDeletable, public FilterManager { * @return std::chrono::milliseconds The delayed close timeout value. */ virtual std::chrono::milliseconds delayedCloseTimeout() const PURE; + + /** + * Set the order of the write filters, indicating whether it is reversed to the filter chain + * config. + */ + // TODO(qiannawang): this method is deprecated and to be moved soon. See + // https://github.com/envoyproxy/envoy/pull/4889 for more details. + virtual void setWriteFilterOrder(bool reversed) PURE; + + /** + * @return bool indicates whether write filters should be in the reversed order of the filter + * chain config. + */ + // TODO(qiannawang): this method is deprecated and to be moved soon. See + // https://github.com/envoyproxy/envoy/pull/4889 for more details. + virtual bool reverseWriteFilterOrder() const PURE; }; typedef std::unique_ptr ConnectionPtr; diff --git a/include/envoy/network/listener.h b/include/envoy/network/listener.h index 67d9a082b8ee..51e7fc9d0f19 100644 --- a/include/envoy/network/listener.h +++ b/include/envoy/network/listener.h @@ -72,6 +72,14 @@ class ListenerConfig { * @return const std::string& the listener's name. */ virtual const std::string& name() const PURE; + + /** + * @return bool indicates whether write filters should be in the reversed order of the filter + * chain config. + */ + // TODO(qiannawang): this method is deprecated and to be moved soon. See + // https://github.com/envoyproxy/envoy/pull/4889 for more details. + virtual bool reverseWriteFilterOrder() const PURE; }; /** diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 5d85b7fff685..66ca6b7cbd09 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -95,6 +95,8 @@ class ConnectionImpl : public virtual Connection, absl::string_view requestedServerName() const override { return socket_->requestedServerName(); } StreamInfo::StreamInfo& streamInfo() override { return stream_info_; } const StreamInfo::StreamInfo& streamInfo() const override { return stream_info_; } + void setWriteFilterOrder(bool reversed) override { reverse_write_filter_order_ = reversed; } + bool reverseWriteFilterOrder() const override { return reverse_write_filter_order_; } // Network::BufferSource BufferSource::StreamBuffer getReadBuffer() override { return {read_buffer_, read_end_stream_}; } @@ -191,6 +193,7 @@ class ConnectionImpl : public virtual Connection, // readDisabled(true) this allows the connection to only resume reads when readDisabled(false) // has been called N times. uint32_t read_disable_count_{0}; + bool reverse_write_filter_order_{false}; }; /** diff --git a/source/common/network/filter_manager_impl.cc b/source/common/network/filter_manager_impl.cc index 0d89ce4de24f..5b796dd14217 100644 --- a/source/common/network/filter_manager_impl.cc +++ b/source/common/network/filter_manager_impl.cc @@ -11,7 +11,11 @@ namespace Network { void FilterManagerImpl::addWriteFilter(WriteFilterSharedPtr filter) { ASSERT(connection_.state() == Connection::State::Open); - downstream_filters_.emplace_back(filter); + if (connection_.reverseWriteFilterOrder()) { + downstream_filters_.emplace_front(filter); + } else { + downstream_filters_.emplace_back(filter); + } } void FilterManagerImpl::addFilter(FilterSharedPtr filter) { diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index e0dd557cde8d..f56a1f4ee435 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -135,7 +135,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( Server::Configuration::FactoryContext& context, Http::DateProvider& date_provider, Router::RouteConfigProviderManager& route_config_provider_manager) : context_(context), reverse_encode_order_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( - config, bugfix_reverse_encode_order, false)), + config, bugfix_reverse_encode_order, true)), stats_prefix_(fmt::format("http.{}.", config.stat_prefix())), stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())), tracing_stats_( diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index f2c830476f39..156aa7742c6f 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -217,6 +217,7 @@ void ConnectionHandlerImpl::ActiveListener::newConnection(Network::ConnectionSoc Network::ConnectionPtr new_connection = parent_.dispatcher_.createServerConnection(std::move(socket), std::move(transport_socket)); new_connection->setBufferLimits(config_.perConnectionBufferLimitBytes()); + new_connection->setWriteFilterOrder(config_.reverseWriteFilterOrder()); const bool empty_filter_chain = !config_.filterChainFactory().createNetworkFilterChain( *new_connection, filter_chain->networkFilterFactories()); diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 46c869cc1fa6..28f45fe26b59 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -262,6 +262,7 @@ class AdminImpl : public Admin, Stats::Scope& listenerScope() override { return *scope_; } uint64_t listenerTag() const override { return 0; } const std::string& name() const override { return name_; } + bool reverseWriteFilterOrder() const override { return false; } AdminImpl& parent_; const std::string name_; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index cef9d05a80c1..a95f8e0d0f34 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -128,8 +128,10 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_original_dst, false)), per_connection_buffer_limit_bytes_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)), - listener_tag_(parent_.factory_.nextListenerTag()), name_(name), modifiable_(modifiable), - workers_started_(workers_started), hash_(hash), + listener_tag_(parent_.factory_.nextListenerTag()), name_(name), + reverse_write_filter_order_( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, bugfix_reverse_write_filter_order, true)), + modifiable_(modifiable), workers_started_(workers_started), hash_(hash), local_drain_manager_(parent.factory_.createDrainManager(config.drain_type())), config_(config), version_info_(version_info) { if (config.has_transparent()) { diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 528c9361eed4..665f5fa82231 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -248,6 +248,7 @@ class ListenerImpl : public Network::ListenerConfig, Stats::Scope& listenerScope() override { return *listener_scope_; } uint64_t listenerTag() const override { return listener_tag_; } const std::string& name() const override { return name_; } + bool reverseWriteFilterOrder() const override { return reverse_write_filter_order_; } // Server::Configuration::ListenerFactoryContext AccessLog::AccessLogManager& accessLogManager() override { @@ -377,6 +378,7 @@ class ListenerImpl : public Network::ListenerConfig, const uint32_t per_connection_buffer_limit_bytes_; const uint64_t listener_tag_; const std::string name_; + const bool reverse_write_filter_order_; const bool modifiable_; const bool workers_started_; const uint64_t hash_; diff --git a/test/common/network/filter_manager_impl_test.cc b/test/common/network/filter_manager_impl_test.cc index b3a26858caa7..b339e162b75b 100644 --- a/test/common/network/filter_manager_impl_test.cc +++ b/test/common/network/filter_manager_impl_test.cc @@ -86,6 +86,57 @@ TEST_F(NetworkFilterManagerTest, All) { .WillOnce(Return(FilterStatus::Continue)); read_filter->callbacks_->continueReading(); + write_buffer_.add("foo"); + write_end_stream_ = false; + EXPECT_CALL(*filter, onWrite(BufferStringEqual("foo"), false)) + .WillOnce(Return(FilterStatus::StopIteration)); + manager.onWrite(); + + write_buffer_.add("bar"); + EXPECT_CALL(*filter, onWrite(BufferStringEqual("foobar"), false)) + .WillOnce(Return(FilterStatus::Continue)); + EXPECT_CALL(*write_filter, onWrite(BufferStringEqual("foobar"), false)) + .WillOnce(Return(FilterStatus::Continue)); + manager.onWrite(); +} + +// AllWithInorderedWriteFilters verifies the same case of All, except that the write filters are +// placed in the same order to the configuration. +TEST_F(NetworkFilterManagerTest, AllWithInorderedWriteFilters) { + InSequence s; + + Upstream::HostDescription* host_description(new NiceMock()); + MockReadFilter* read_filter(new MockReadFilter()); + MockWriteFilter* write_filter(new MockWriteFilter()); + MockFilter* filter(new LocalMockFilter()); + + NiceMock connection; + connection.setWriteFilterOrder(false); + FilterManagerImpl manager(connection, *this); + manager.addReadFilter(ReadFilterSharedPtr{read_filter}); + manager.addWriteFilter(WriteFilterSharedPtr{write_filter}); + manager.addFilter(FilterSharedPtr{filter}); + + read_filter->callbacks_->upstreamHost(Upstream::HostDescriptionConstSharedPtr{host_description}); + EXPECT_EQ(read_filter->callbacks_->upstreamHost(), filter->callbacks_->upstreamHost()); + + EXPECT_CALL(*read_filter, onNewConnection()).WillOnce(Return(FilterStatus::StopIteration)); + EXPECT_EQ(manager.initializeReadFilters(), true); + + EXPECT_CALL(*filter, onNewConnection()).WillOnce(Return(FilterStatus::Continue)); + read_filter->callbacks_->continueReading(); + + read_buffer_.add("hello"); + read_end_stream_ = false; + EXPECT_CALL(*read_filter, onData(BufferStringEqual("hello"), false)) + .WillOnce(Return(FilterStatus::StopIteration)); + manager.onRead(); + + read_buffer_.add("world"); + EXPECT_CALL(*filter, onData(BufferStringEqual("helloworld"), false)) + .WillOnce(Return(FilterStatus::Continue)); + read_filter->callbacks_->continueReading(); + write_buffer_.add("foo"); write_end_stream_ = false; EXPECT_CALL(*write_filter, onWrite(BufferStringEqual("foo"), false)) @@ -136,15 +187,15 @@ TEST_F(NetworkFilterManagerTest, EndStream) { write_buffer_.add("foo"); write_end_stream_ = true; - EXPECT_CALL(*write_filter, onWrite(BufferStringEqual("foo"), true)) + EXPECT_CALL(*filter, onWrite(BufferStringEqual("foo"), true)) .WillOnce(Return(FilterStatus::StopIteration)); manager.onWrite(); write_buffer_.add("bar"); - EXPECT_CALL(*write_filter, onWrite(BufferStringEqual("foobar"), true)) - .WillOnce(Return(FilterStatus::Continue)); EXPECT_CALL(*filter, onWrite(BufferStringEqual("foobar"), true)) .WillOnce(Return(FilterStatus::Continue)); + EXPECT_CALL(*write_filter, onWrite(BufferStringEqual("foobar"), true)) + .WillOnce(Return(FilterStatus::Continue)); manager.onWrite(); } diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc index 18224ccbb581..ca9457d71946 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc @@ -72,6 +72,7 @@ class ProxyProtocolTest : public testing::TestWithParam, Stats::Scope& listenerScope() override { return parent_.stats_store_; } uint64_t listenerTag() const override { return 0; } const std::string& name() const override { return name_; } + bool reverseWriteFilterOrder() const override { return true; } FakeUpstream& parent_; std::string name_; diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index cb9266211d6c..4413113bb67a 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -92,6 +92,12 @@ class MockConnection : public Connection, public MockConnectionBase { MOCK_CONST_METHOD0(streamInfo, const StreamInfo::StreamInfo&()); MOCK_METHOD1(setDelayedCloseTimeout, void(std::chrono::milliseconds)); MOCK_CONST_METHOD0(delayedCloseTimeout, std::chrono::milliseconds()); + + void setWriteFilterOrder(bool reversed) override { reversed_write_filter_order_ = reversed; } + bool reverseWriteFilterOrder() const override { return reversed_write_filter_order_; } + +private: + bool reversed_write_filter_order_{true}; }; /** @@ -135,6 +141,8 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MOCK_CONST_METHOD0(streamInfo, const StreamInfo::StreamInfo&()); MOCK_METHOD1(setDelayedCloseTimeout, void(std::chrono::milliseconds)); MOCK_CONST_METHOD0(delayedCloseTimeout, std::chrono::milliseconds()); + MOCK_METHOD1(setWriteFilterOrder, void(bool reversed)); + bool reverseWriteFilterOrder() const override { return true; } // Network::ClientConnection MOCK_METHOD0(connect, void()); @@ -369,6 +377,7 @@ class MockListenerConfig : public ListenerConfig { MOCK_METHOD0(listenerScope, Stats::Scope&()); MOCK_CONST_METHOD0(listenerTag, uint64_t()); MOCK_CONST_METHOD0(name, const std::string&()); + MOCK_CONST_METHOD0(reverseWriteFilterOrder, bool()); testing::NiceMock filter_chain_factory_; testing::NiceMock socket_; diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index bf3fc0069e67..3ba28d472769 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -51,6 +51,7 @@ class ConnectionHandlerTest : public testing::Test, protected Logger::LoggableaddOrUpdateListener(parseListenerFromV2Yaml(json), "", true)); + EXPECT_TRUE(manager_->listeners().front().get().reverseWriteFilterOrder()); +} + TEST_F(ListenerManagerImplTest, ModifyOnlyDrainType) { InSequence s;