api: add a field in Listener proto to be able to reverse the order of TCP write filters

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];
 }

ci/.bazelrc

@@ -0,0 +1 @@
+import %workspace%/tools/bazel.rc

include/envoy/network/connection.h

@@ -261,6 +261,18 @@ 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.
+   */
+  virtual void setWriteFilterOrder(bool reversed) PURE;
+
+  /**
+   * @return bool indicates whether write filters should be in the reversed order of the filter
+   *         chain config.
+   */
+  virtual bool reverseWriteFilterOrder() const PURE;
 };
 
 typedef std::unique_ptr<Connection> ConnectionPtr;

include/envoy/network/listener.h

@@ -72,6 +72,12 @@ 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.
+   */
+  virtual bool reverseWriteFilterOrder() const PURE;
 };
 
 /**

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};
 };
 
 /**

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) {

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

source/server/http/admin.h

@@ -259,6 +259,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_;

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, false)),
+      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()) {

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_;

test/common/network/filter_manager_impl_test.cc

@@ -88,15 +88,15 @@ TEST_F(NetworkFilterManagerTest, All) {
 
   write_buffer_.add("foo");
   write_end_stream_ = false;
-  EXPECT_CALL(*write_filter, onWrite(BufferStringEqual("foo"), false))
+  EXPECT_CALL(*filter, onWrite(BufferStringEqual("foo"), false))
       .WillOnce(Return(FilterStatus::StopIteration));
   manager.onWrite();
 
   write_buffer_.add("bar");
-  EXPECT_CALL(*write_filter, onWrite(BufferStringEqual("foobar"), false))
-      .WillOnce(Return(FilterStatus::Continue));
   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();
 }
 
@@ -136,15 +136,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();
 }
 

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 {

test/integration/fake_upstream.h

@@ -578,6 +578,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_;

test/mocks/network/mocks.h

@@ -92,6 +92,8 @@ 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());
+  MOCK_METHOD1(setWriteFilterOrder, void(bool reversed));
+  bool reverseWriteFilterOrder() const override { return true; }
 };
 
 /**
@@ -135,6 +137,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 +373,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_;

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_;