api: add a field in Listener proto to be able to reverse the order of…

… TCP write filters (envoyproxy#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 envoyproxy#4599 Signed-off-by: Qi (Anna) Wang <[email protected]> Signed-off-by: Fred Douglas <[email protected]>
fredlas · Mar 5, 2019 · 160c3d6 · 160c3d6
1 parent e0fc47b
commit 160c3d6
Show file tree

Hide file tree

Showing 18 changed files with 146 additions and 8 deletions.
diff --git 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
@@ -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
@@ -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<Connection> ConnectionPtr;

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<Upstream::MockHostDescription>());
+  MockReadFilter* read_filter(new MockReadFilter());
+  MockWriteFilter* write_filter(new MockWriteFilter());
+  MockFilter* filter(new LocalMockFilter());
+
+  NiceMock<MockConnection> 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
@@ -72,6 +72,7 @@ class ProxyProtocolTest : public testing::TestWithParam<Network::Address::IpVers
   Stats::Scope& listenerScope() override { return stats_store_; }
   uint64_t listenerTag() const override { return 1; }
   const std::string& name() const override { return name_; }
+  bool reverseWriteFilterOrder() const override { return true; }
 
   // Network::FilterChainManager
   const Network::FilterChain* findFilterChain(const Network::ConnectionSocket&) const override {
@@ -896,6 +897,7 @@ class WildcardProxyProtocolTest : public testing::TestWithParam<Network::Address
   Stats::Scope& listenerScope() override { return stats_store_; }
   uint64_t listenerTag() const override { return 1; }
   const std::string& name() const override { return name_; }
+  bool reverseWriteFilterOrder() const override { return true; }
 
   // Network::FilterChainManager
   const Network::FilterChain* findFilterChain(const Network::ConnectionSocket&) const override {

diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc
@@ -452,6 +452,20 @@ TEST_F(HttpConnectionManagerConfigTest, BadAccessLogNestedTypes) {
   EXPECT_THROW(factory.createFilterFactory(*json_config, context_), Json::Exception);
 }
 
+TEST_F(HttpConnectionManagerConfigTest, ReversedEncodeOrderConfig) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  route_config:
+    name: local_route
+  http_filters:
+  - name: envoy.router
+  )EOF";
+
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_);
+  EXPECT_TRUE(config.reverseEncodeOrder());
+}
+
 class FilterChainTest : public HttpConnectionManagerConfigTest {
 public:
   const std::string basic_config_ = R"EOF(

diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h
@@ -579,6 +579,7 @@ class FakeUpstream : Logger::Loggable<Logger::Id::testing>,
     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
@@ -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<MockFilterChainFactory> filter_chain_factory_;
   testing::NiceMock<MockListenSocket> socket_;

diff --git 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::Loggable<L
     Stats::Scope& listenerScope() override { return parent_.stats_store_; }
     uint64_t listenerTag() const override { return tag_; }
     const std::string& name() const override { return name_; }
+    bool reverseWriteFilterOrder() const override { return true; }
 
     ConnectionHandlerTest& parent_;
     Network::MockListenSocket socket_;

diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc
@@ -346,6 +346,20 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, StatsScopeTest) {
   EXPECT_EQ(1UL, server_.stats_store_.counter("listener.127.0.0.1_1234.foo").value());
 }
 
+TEST_F(ListenerManagerImplTest, ReversedWriteFilterOrder) {
+  const std::string json = R"EOF(
+    name: "foo"
+    address:
+      socket_address: { address: 127.0.0.1, port_value: 10000 }
+    filter_chains:
+    - filters:
+  )EOF";
+
+  EXPECT_CALL(listener_factory_, createListenSocket(_, _, true));
+  EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(json), "", true));
+  EXPECT_TRUE(manager_->listeners().front().get().reverseWriteFilterOrder());
+}
+
 TEST_F(ListenerManagerImplTest, ModifyOnlyDrainType) {
   InSequence s;