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

qiannawang · 2018-10-29T04:47:25Z

Description: 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

qiannawang · 2018-10-29T17:21:05Z

@mattklein123 @htuch: could you take a look while I am fixing the test?

As suggested in [1], I added field in the FilterChain. However, this proto is not passed to the filter manager class, which determines the order of write filters. I passed a bool variable through many layers and changed 50+ files as you see. I wonder if there is a better solution to pass this info to the filter manager.

[1] #4599

mattklein123 · 2018-10-29T18:03:40Z

@qiannawang hmm yeah. I just took a quick look and agree this is very ugly. The only thing I can think of would be to have a global server/bootstrap option for this, and pass that option into the dispatcher, which could then be passed into Connection/FilterManager construction so it can be hidden from everywhere else?

Or more horribly even a deprecated CLI option so it would be an effective global? Just trying to think of least amount of change that we can do for something we are going to turn around and remove.

@htuch thoughts?

htuch · 2018-10-29T18:45:34Z

I'm not sure why this had to become part of the factory interface for filters? Don't we just want this to be in Network::FilterChain? Most of the delta seems to be because we're modifying the filter API here.

mattklein123 · 2018-10-29T18:57:12Z

It's because the filter manager is owned by the connection which doesn't have access to this config.

htuch · 2018-10-29T22:06:45Z

@mattklein123 somehow the FilterManager mus be obtaining a list of filter factories. Can't we just change this to (filter factory list, reversal flag) without modifying the filter factory API?

mattklein123 · 2018-10-29T22:10:15Z

Unfortunately not unless I am missing something. The "filter manager" is owned by the connection and is used to add filters as part of the listener manager config build.

One option would be to do the logic for this slightly differently. Instead of switching push_front with push_back (or however it was done previously), actually change the order by which the filters are added at the network level. I think this could be done directly from the listener manager w/o all these changes?

mattklein123 · 2018-10-29T22:22:43Z

Sorry nevermind my previous comment, that won't work, as we don't know what a factory will do when it's called in terms of encode/decode/both.

I don't see any way around making all these changes or doing some type of global/bootstrap option.

mattklein123 · 2018-10-29T23:15:01Z

OK another thought after talking with @htuch, you could probably add a temporary function to Connection, such as reverseEncodeOrder() and call it from ListenerImpl::createNetworkFilterChain. That could be plumbed into the filter manager and avoid a bunch of other changes? I think it would still require a Connection interface change unless we somehow get the property into the dispatcher directly.

qiannawang · 2018-11-05T05:01:56Z

thanks for the suggestions, @mattklein123 and @htuch. I will make the suggested changes according.

I will add a field in Listener instead of FilterChain to guard the enable/disable of reversing write filter order.

…rseWriteFilterOrder(). Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

qiannawang · 2018-11-06T06:49:00Z

@htuch @mattklein123 all checks have passed. PTAL?

mattklein123

Thanks, this is great, much simpler. A few small comments.

ci/.bazelrc

test/common/network/filter_manager_impl_test.cc

include/envoy/network/connection.h

Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

include/envoy/network/connection.h

DEPRECATED.md

Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

mattklein123 · 2018-11-08T05:24:52Z

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


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?

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

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.

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.

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.

Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

htuch · 2018-11-12T16:25:12Z

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.


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

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?

That's fine as is then.

mattklein123

Thanks for working on this, LGTM. Will defer to @htuch for further review and merge.

qiannawang · 2018-11-13T00:28:01Z

@mattklein123 @htuch, can I start now the cleanup and remove the proto fields and interfaces I added for this bug fix? Or should I wait for some time? If the latter, how long should I wait?

mattklein123 · 2018-11-13T03:37:57Z

@qiannawang we do deprecation cleanups after we snap the next release. So in this case, after we snap 1.9.0 which will be 1.5 months or so. When we do the release we will automatically create cleanup tickets and assign them to you. Thank you!

qiannawang · 2018-11-13T03:47:43Z

Got it, thanks for the info!

… 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 <qiwang@qiwang-macbookpro.roam.corp.google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

qiannawang requested review from lizan and zuercher as code owners October 29, 2018 04:47

qiannawang force-pushed the httpfilter branch 2 times, most recently from 3799e24 to 3ea5d64 Compare October 29, 2018 08:21

htuch self-assigned this Oct 30, 2018

qiannawang closed this Nov 5, 2018

qiannawang force-pushed the httpfilter branch from 3ea5d64 to 989de30 Compare November 5, 2018 04:55

qiannawang reopened this Nov 6, 2018

qiannawang force-pushed the httpfilter branch 2 times, most recently from 49239d0 to a23a597 Compare November 6, 2018 05:14

Resolve reviewer comments by providing an interface in connector reve…

fcd4fa0

…rseWriteFilterOrder(). Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

qiannawang force-pushed the httpfilter branch from a23a597 to fcd4fa0 Compare November 6, 2018 05:20

mattklein123 reviewed Nov 6, 2018

View reviewed changes

ci/.bazelrc Outdated Show resolved Hide resolved

test/common/network/filter_manager_impl_test.cc Show resolved Hide resolved

include/envoy/network/connection.h Show resolved Hide resolved

mattklein123 self-assigned this Nov 6, 2018

mattklein123 added the waiting label Nov 6, 2018

repokitteh-read-only bot removed the waiting label Nov 7, 2018

qiannawang force-pushed the httpfilter branch from 1561c92 to 4d77100 Compare November 7, 2018 06:15

Remove ci/.bazelrc

fd65ede

Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

qiannawang force-pushed the httpfilter branch from 4d77100 to fd65ede Compare November 7, 2018 06:18

mattklein123 added the waiting label Nov 7, 2018

Added TODOs for deprecation.

72e1e77

Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

repokitteh-read-only bot removed the waiting label Nov 8, 2018

mattklein123 reviewed Nov 8, 2018

View reviewed changes

include/envoy/network/connection.h Show resolved Hide resolved

DEPRECATED.md Outdated Show resolved Hide resolved

Further fixed reviewer comments, and set default value true.

fd13ec1

Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

mattklein123 reviewed Nov 8, 2018

View reviewed changes

mattklein123 added the waiting label Nov 8, 2018

repokitteh-read-only bot removed waiting labels Nov 8, 2018

mattklein123 added the waiting label Nov 8, 2018

mattklein123 mentioned this pull request Nov 9, 2018

http: allow decoding/encoding a header only request/response #4885

Merged

Add new tests based on reviewers comments.

0acee1a

Signed-off-by: Qi (Anna) Wang <qiwang@qiwang-macbookpro.roam.corp.google.com>

repokitteh-read-only bot removed the waiting label Nov 12, 2018

htuch reviewed Nov 12, 2018

View reviewed changes

mattklein123 approved these changes Nov 12, 2018

View reviewed changes

htuch approved these changes Nov 12, 2018

View reviewed changes

htuch merged commit 5da782c into envoyproxy:master Nov 12, 2018

qiannawang mentioned this pull request Nov 13, 2018

Envoy http filters have the *same* order in decode and encode methods #4599

Closed

alyssawilk mentioned this pull request Dec 20, 2018

[v1.9.0 deprecation] Remove features marked deprecated in #4889 #5380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

qiannawang commented Oct 29, 2018

qiannawang commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

htuch commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

htuch commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

qiannawang commented Nov 5, 2018

qiannawang commented Nov 6, 2018

mattklein123 left a comment

mattklein123 Nov 8, 2018

qiannawang Nov 8, 2018

mattklein123 Nov 8, 2018

qiannawang Nov 12, 2018 •

edited

Loading

mattklein123 Nov 12, 2018

htuch Nov 12, 2018

qiannawang Nov 12, 2018

htuch Nov 12, 2018

mattklein123 left a comment

qiannawang commented Nov 13, 2018

mattklein123 commented Nov 13, 2018

qiannawang commented Nov 13, 2018

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

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

Conversation

qiannawang commented Oct 29, 2018

qiannawang commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

htuch commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

htuch commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

mattklein123 commented Oct 29, 2018

qiannawang commented Nov 5, 2018

qiannawang commented Nov 6, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Nov 8, 2018

Choose a reason for hiding this comment

qiannawang Nov 8, 2018

Choose a reason for hiding this comment

mattklein123 Nov 8, 2018

Choose a reason for hiding this comment

qiannawang Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

mattklein123 Nov 12, 2018

Choose a reason for hiding this comment

htuch Nov 12, 2018

Choose a reason for hiding this comment

qiannawang Nov 12, 2018

Choose a reason for hiding this comment

htuch Nov 12, 2018

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

qiannawang commented Nov 13, 2018

mattklein123 commented Nov 13, 2018

qiannawang commented Nov 13, 2018

qiannawang Nov 12, 2018 •

edited

Loading