Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Nov 12, 2018
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fcd4fa0
Resolve reviewer comments by providing an interface in connector reve…
Nov 6, 2018
fd65ede
Remove ci/.bazelrc
Nov 7, 2018
72e1e77
Added TODOs for deprecation.
Nov 8, 2018
fd13ec1
Further fixed reviewer comments, and set default value true.
Nov 8, 2018
0acee1a
Add new tests based on reviewers comments.
Nov 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion DEPRECATED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a PR to default this true for consistency, given the new default approach that was agreed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fields for write and encoder filters, respectively, are default true in this PR. Should I explain more explicitly about the default value in the DEPRECATED.md file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine as is then.

* Use of the v1 REST_LEGACY ApiConfigSource is deprecated.
* Use of std::hash in the ring hash load balancer is deprecated.

Expand Down
8 changes: 8 additions & 0 deletions api/envoy/api/v2/lds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
16 changes: 16 additions & 0 deletions include/envoy/network/connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 8 additions & 0 deletions include/envoy/network/listener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

/**
Expand Down
3 changes: 3 additions & 0 deletions source/common/network/connection_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_}; }
Expand Down Expand Up @@ -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};
};

/**
Expand Down
6 changes: 5 additions & 1 deletion source/common/network/filter_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/network/http_connection_manager/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It scares me that this change has no test implications? I guess because we have no integration test which covers this case? Can you potentially open a tech debt issue for someone to add a multi-filter encode/decode integration test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are tests validating this. No test violation because we have set the default value true in the test, exercising the reversed order, see

envoy/test/common/http/conn_manager_impl_test.cc

Line 261 in bacaa4c

bool reverse_encode_order_{true};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are not actually validating this code change here, which is the real HCM config. (The tests use a mock/fake config.). This is why no other test changes were required when you made this change. Please add a config test here that would have failed with this change: https://github.com/envoyproxy/envoy/blob/master/test/extensions/filters/network/http_connection_manager/config_test.cc. Also please open the integration test tech debt issue I mentioned. We should have an integration test that uses multiple encode/decode filters. This should be a lot easier now given all the work that both @alyssawilk and @snowp have done with fake filters recently.

Copy link
Contributor Author

@qiannawang qiannawang Nov 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test case in config_test.cc to validate the hcm config, as well as a case in listener_manager_impl_test.cc to validate the listener config.

IMO, the tests in
https://github.com/envoyproxy/envoy/blob/master/test/common/http/conn_manager_impl_test.cc
plus,
https://github.com/envoyproxy/envoy/blob/master/test/extensions/filters/network/http_connection_manager/config_test.cc
are sufficient. The former validates cases of multiple (mock) encode/decode filters, with a fake hcm config. The latter (newly added case) validates the hcm config is set from the proto.

I think the case, which is not tested as an e2e definition, is to consume the real hcm config from the proto and bring up real filters. If we are going to use fake filters in the e2e, the test cases seem already sufficient for me.

Let me know if my understanding is correct. Happy to file a new issue for test tech debt if there is a need.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think this is fine, via the work that @snowp is doing now in the other PR we should be able to easily get coverage of multiple encoder filters in an integration test which is great.

stats_prefix_(fmt::format("http.{}.", config.stat_prefix())),
stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())),
tracing_stats_(
Expand Down
1 change: 1 addition & 0 deletions source/server/connection_handler_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
1 change: 1 addition & 0 deletions source/server/http/admin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down
6 changes: 4 additions & 2 deletions source/server/listener_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
2 changes: 2 additions & 0 deletions source/server/listener_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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_;
Expand Down
12 changes: 6 additions & 6 deletions test/common/network/filter_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
}

Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions test/integration/fake_upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down
5 changes: 5 additions & 0 deletions test/mocks/network/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
};

/**
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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_;
Expand Down
1 change: 1 addition & 0 deletions test/server/connection_handler_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down