common: added a field in HCM proto to be able to reverse the order of HTTP encoder filters. #4721

qiannawang · 2018-10-14T22:11:06Z

Description: Added a field in HCM proto to be able to reverse the order of HTTP encoder filters. The field is set false by default, indicating HTTP encoder 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

Signed-off-by: Qi (Anna) Wang qiwang@google.com

By default (false), they have the same order as configed in the filter chain. If true, their order will be reversed. Signed-off-by: Qi (Anna) Wang <qiwang@qiwang.sfo.corp.google.com>

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

qiannawang · 2018-10-15T16:11:46Z

@htuch could you PTAL?

rshriram · 2018-10-15T19:41:56Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+
+  // If true, the order of encoder filters will be reversed to that of filters
+  // configured in the HTTP filter chain.
+  bool reverse_encoders_order = 27;


what is the use case? it would be good to have some explanation associated with such settings. Otherwise, we end up having a plethora of "invert_x", "reverse_y" boolean options all over the config

This is a bug fixing in envoy. The design of envoy was to have reserved order of encoder filters to the configuration in the filter chain. However, the current implementation in envoy is not following this order. In order to avoid breaking anyone who depends on this bug, we decided to fix this as an opt-in behavior.

For more details, see #4599

Does it make sense to link #4599 in the comment for this? It adds valuable context.

I don't think we have to go to this extreme to preserve backward compatibility for incorrect behavior. It seems counter intuitive, given that the goal of the API is to describe the system (not workarounds for "correct" behavior). Put another way, if those other systems depended on the incorrect behavior, then those systems are also not functioning correctly are they?

As @htuch suggested, we will mark this field deprecated and put in the release note too.

htuch

Thanks! Looks good, some feedback.

htuch · 2018-10-15T20:22:42Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+
+  // If true, the order of encoder filters will be reversed to that of filters
+  // configured in the HTTP filter chain.
+  bool reverse_encoders_order = 27;


Prefer reverse_encode_order for naming. Also, I think we should annotation this as deprecated and update https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md

Should we make this BoolValue and change the default to true in near future (after next release)?

Done with the renaming.

@htuch would you recommend to add the "deprecated" annotation in this change or in another PR when we deprecate the field in that release?

@lizan what would be the difference between bool and BoolValue in this case?

bool is always default to false and you can't differentiate false and unset. with BoolValue you know if it is unset or true or false.

The question is what will be the process when we remove this deprecated field. If at that moment this will be true (true is expected behavior, right?) then BoolValue makes more sense. @htuch?

hmm, the h1_capture_direct_response_fuzz_test test is hitting tsan failure after I set the bool filed [depreated = true]. I am wondering if the BoolValue would help with the failure.

https://circleci.com/gh/envoyproxy/envoy/108918?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

It is a flaky one and fix in recent commit, merge master would help.

Merging does help, but looks like I rebased it incorrectly and the version history is corrupted :(

Any suggestions to fix this? I guess I can close this PR and create a new one, but wonder if there is a simple solution.

Yes you shouldn't do git rebase but a simple git merge upstream/master. you can checkout the original commit and do git merge again, and then force push to fix this PR, or close and create a new PR.

It works! thanks for the suggestion!

source/common/http/conn_manager_config.h

source/common/http/conn_manager_impl.cc

test/common/http/conn_manager_impl_test.cc

htuch · 2018-10-15T20:25:30Z

test/common/http/conn_manager_impl_test.cc

-    for (auto filter : encoder_filters_) {
-      EXPECT_CALL(*filter, onDestroy());
+    if (reverse_encoders_order_) {
+      for (int i = encoder_filters_.size() - 1; i >= 0; --i) {


Are all the callers of this setup with InSequence to be able to enforce EXPECT_CALL ordering?

Yes, as the InSequence is global within a test case for all callers.

test/common/http/conn_manager_impl_test.cc

rshriram · 2018-10-15T23:57:03Z

At the very least, if we are going to do this "bugfix", might as well come clean and declare the field as "bugfix_reverse_encoder_order", so that end users don't mistake this for a "new" feature.

I still think providing backward compatibility for a buggy behavior is a strange thing to do

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

htuch · 2018-10-16T14:51:51Z

Agree with @rshriram that this should be named as a bug fix. I think it's arguably better to support the old behavior here as we can break folks in production who have unexpected encode filter ordering interactions.

In terms of deprecation, let's immediately deprecate the field and update DEPRECATED.md.

htuch

Looks good, should be ready to ship soon.

test/common/http/conn_manager_impl_test.cc

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

…laky test.

htuch · 2018-10-17T16:52:32Z

@lizan we are beginning immediate deprecation. Can you explain how defaulting the field to true at the end of the deprecation cycle, at the same time as we remove it, makes more sense? I don't mind either way TBH, just trying to grok the right thing to do here.

lizan · 2018-10-17T22:30:10Z

@htuch I don't mind either, if we do default to true and remove at same time, then no difference, if we do remove later there will be less surprise.

@qiannawang looks like you reverted too much? I don't see the deprecation now.

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

qiannawang · 2018-10-18T04:18:20Z

@htuch I don't mind either, if we do default to true and remove at same time, then no difference, if we do remove later there will be less surprise.

@qiannawang looks like you reverted too much? I don't see the deprecation now.

I have added back the deprecation, resolved all comments and changed to use BoolValue from bool.

@htuch PTAL?

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

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

htuch

Thanks, this looks almost ready to ship, some final nits.

DEPRECATED.md

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

htuch · 2018-10-18T18:32:35Z

source/common/config/filter_json.cc

@@ -209,6 +209,7 @@ void FilterJson::translateHttpConnectionManager(

  JSON_UTIL_SET_BOOL(json_config, proto_config, use_remote_address);
  JSON_UTIL_SET_BOOL(json_config, proto_config, generate_request_id);
+  JSON_UTIL_SET_BOOL(json_config, proto_config, bugfix_reverse_encode_order);


Why is this needed? This is just for v1 config, which is now deprecated.

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

qiannawang · 2018-10-18T22:07:18Z

All tests pass. @htuch PTAL?

htuch

Thanks @qiannawang, this is great.

… HTTP encoder filters. (envoyproxy#4721) Added a field in HCM proto to be able to reverse the order of HTTP encoder filters. The field is set false by default, indicating HTTP encoder 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: Yang Song <yasong@yasong00.cam.corp.google.com>

Qi (Anna) Wang added 3 commits October 14, 2018 14:47

Added a field in HCM proto to config the order of HTTP encoder filters.

c26435e

By default (false), they have the same order as configed in the filter chain. If true, their order will be reversed. Signed-off-by: Qi (Anna) Wang <qiwang@qiwang.sfo.corp.google.com>

Fixed format

b166274

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

Fixed asan failure in test.

3186414

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

htuch self-assigned this Oct 15, 2018

rshriram reviewed Oct 15, 2018

View reviewed changes

htuch suggested changes Oct 15, 2018

View reviewed changes

Resolved comments from reviewers.

4673764

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

htuch suggested changes Oct 16, 2018

View reviewed changes

test/common/http/conn_manager_impl_test.cc Outdated Show resolved Hide resolved

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto Outdated Show resolved Hide resolved

qiannawang changed the title ~~Added a field in HCM proto to be able to reverse the order of HTTP encoder filters.~~ common: added a field in HCM proto to be able to reverse the order of HTTP encoder filters. Oct 16, 2018

qiannawang requested a review from zuercher as a code owner October 17, 2018 06:31

Merge remote-tracking branch 'upstream/master' into HEAD to resolve f…

23ac82a

…laky test.

Qi (Anna) Wang added 2 commits October 17, 2018 18:05

Resolved 2nd round of reviewer comments.

4e090f9

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

Changed to use BoolValue from bool.

254349e

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

lizan reviewed Oct 18, 2018

View reviewed changes

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto Show resolved Hide resolved

Added more details in comment for bugfix_reverse_encode_order field…

496a190

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

lizan previously approved these changes Oct 18, 2018

View reviewed changes

htuch suggested changes Oct 18, 2018

View reviewed changes

Resolved another round of reviewer comments.

f1361e1

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

qiannawang dismissed lizan’s stale review via f1361e1 October 18, 2018 19:46

htuch approved these changes Oct 18, 2018

View reviewed changes

htuch merged commit 0ccc70a into envoyproxy:master Oct 18, 2018

alyssawilk mentioned this pull request Dec 20, 2018

[v1.9.0 deprecation] Remove features marked deprecated in #4721 #5377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common: added a field in HCM proto to be able to reverse the order of HTTP encoder filters. #4721

common: added a field in HCM proto to be able to reverse the order of HTTP encoder filters. #4721

qiannawang commented Oct 14, 2018 •

edited

Loading

qiannawang commented Oct 15, 2018

rshriram Oct 15, 2018

qiannawang Oct 15, 2018

dnoe Oct 15, 2018

rshriram Oct 15, 2018

qiannawang Oct 16, 2018

htuch left a comment

htuch Oct 15, 2018

lizan Oct 16, 2018

qiannawang Oct 16, 2018

lizan Oct 17, 2018

lizan Oct 17, 2018

qiannawang Oct 17, 2018

lizan Oct 17, 2018

qiannawang Oct 17, 2018

lizan Oct 17, 2018

qiannawang Oct 17, 2018

htuch Oct 15, 2018

qiannawang Oct 16, 2018

rshriram commented Oct 15, 2018

htuch commented Oct 16, 2018

htuch left a comment

htuch commented Oct 17, 2018

lizan commented Oct 17, 2018 •

edited

Loading

qiannawang commented Oct 18, 2018

htuch left a comment

htuch Oct 18, 2018

qiannawang Oct 18, 2018

qiannawang commented Oct 18, 2018

htuch left a comment

common: added a field in HCM proto to be able to reverse the order of HTTP encoder filters. #4721

common: added a field in HCM proto to be able to reverse the order of HTTP encoder filters. #4721

Conversation

qiannawang commented Oct 14, 2018 • edited Loading

qiannawang commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Oct 15, 2018

htuch commented Oct 16, 2018

htuch left a comment

Choose a reason for hiding this comment

htuch commented Oct 17, 2018

lizan commented Oct 17, 2018 • edited Loading

qiannawang commented Oct 18, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiannawang commented Oct 18, 2018

htuch left a comment

Choose a reason for hiding this comment

qiannawang commented Oct 14, 2018 •

edited

Loading

lizan commented Oct 17, 2018 •

edited

Loading