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

[qiannawang]

Issue Template

Title: Request and response go through the same http filter order in the chain

Description:
We've found that, in the http filter chain, the request and response (to and from the upstream end point) go through the same filter order.

Filters are added to decode and encode in the same order:

envoy/source/common/http/conn_manager_impl.h

Line 313 in b90fd4b

void addStreamFilter(StreamFilterSharedPtr filter) override {

and the encode filter chain goes through in that order:

envoy/source/common/http/conn_manager_impl.cc

Line 1018 in b90fd4b

std::list<ActiveStreamEncoderFilterPtr>::iterator entry = commonEncodePrefix(filter, end_stream);

This seems unexpected, as we thought the decode and encode had the reverse order of the http filters. For example, we config in bootstrap yaml file in the http_filters:
X
Y
Z

The decode order is X, Y, Z, and we thought the encode order was Z, Y, X. but turned out this is not true.

This unexpectation introduces problems to our system, e.g., by measuring the rrt of the request in some filter, header stripping, gzip, etc.

Should we consider to fix the order and make it reverse in encode chain?

[stevenzzzz]

this is surprising and yet true.
I think the straightforward solution is when adding to encode_filters_, use moveIntoList instead of moveIntoListLast().

[mattklein123]

This is an unfortunate oversight that I'm kicking myself for right now. Oops! 😢

The code fix itself is easy, and I agree should be done. The problem here is whether anyone is depending on this odd behavior. I suspect not since no one has complained about it before, but I think we probably have to make this behavior opt-in with some type of deprecation period in which we would do the switch. Though I guess we can send email and solicit feedback on just changing it.

[qiannawang]

I would like to contribute to the fix. Opt-in feature sounds a better solution for me because:

1. it can avoid to break anyone;
2. it help a faster release.

What do you think?

[mattklein123]

Yes I think that's the way to go. I think it should be straightforward. The only tests I can think of that might break are the conn_manager_impl multiple filter tests, but you will need to add some new tests anyway.

[htuch]

@mattklein123 would a HCM filter config field to control behavior make sense, with eventual deprecation? I think we want to have folks experience the correct behavior by default as soon as is reasonable.

[mattklein123]

@mattklein123 would a HCM filter config field to control behavior make sense, with eventual deprecation? I think we want to have folks experience the correct behavior by default as soon as is reasonable.

Yeah that's what I suggested above.

[htuch]

@mattklein123 just wanted to validate we're talking about filter config vs. CLI.

[mattklein123]

Oh yes I meant config, not CLI.

[curiouserrandy]

Have we looked at this behavior for network (L4) filters? Glancing at the code without experience, it seems like the same problem (both read and write filters put on the end of the chain, both executed in start-to-end order.)

[mattklein123]

@curiouserrandy yes we need to fix in both places.

[qiannawang]

is FilterManagerImpl::addWriteFilter the place adding the write filters to the network filter chain?

envoy/source/common/network/filter_manager_impl.cc

Line 14 in fdfa5bd

downstream_filters_.emplace_back(filter);

I believe the change is to replace emplace_back with emplace_front when adding the write filters.

[mattklein123]

@qiannawang yes that's right.

[qiannawang]

Which proto is a good place to add a field to config the order of write filters?

@htuch suggested http_connection_manager.proto for http filters, what about tcp ones?

[htuch]

@qiannawang you want the HCM proto for L7 filters and the Listener FilterChain for L4.

[mattklein123]

@qiannawang are you planning on doing another PR for network filters as well?

[qiannawang]

Yes. Do you have any suggestion of the change to network filters?

[mattklein123]

I would put a similar deprecated boolean field here: https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/listener/listener.proto#L157 to go along with the filters?

[qiannawang]

Close this issue as the fixes have been enabled (by default). The cleanup will be tracked by a new ticket, as explained in #4889 (comment)