From 160c3d6a83f492da75e59e7267c7341e9395c975 Mon Sep 17 00:00:00 2001 From: qiannawang <38573959+qiannawang@users.noreply.github.com> Date: Mon, 12 Nov 2018 11:04:03 -0800 Subject: [PATCH] api: add a field in Listener proto to be able to reverse the order of TCP write filters (#4889) Add a field in listener proto to be able to reverse the order of TCP write filters. The field is set false by default, indicating write 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 Signed-off-by: Fred Douglas --- DEPRECATED.md | 3 +- api/envoy/api/v2/lds.proto | 8 +++ include/envoy/network/connection.h | 16 ++++++ include/envoy/network/listener.h | 8 +++ source/common/network/connection_impl.h | 3 + source/common/network/filter_manager_impl.cc | 6 +- .../network/http_connection_manager/config.cc | 2 +- source/server/connection_handler_impl.cc | 1 + source/server/http/admin.h | 1 + source/server/listener_manager_impl.cc | 6 +- source/server/listener_manager_impl.h | 2 + .../network/filter_manager_impl_test.cc | 57 ++++++++++++++++++- .../proxy_protocol/proxy_protocol_test.cc | 2 + .../http_connection_manager/config_test.cc | 14 +++++ test/integration/fake_upstream.h | 1 + test/mocks/network/mocks.h | 9 +++ test/server/connection_handler_test.cc | 1 + test/server/listener_manager_impl_test.cc | 14 +++++ 18 files changed, 146 insertions(+), 8 deletions(-) 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;