Skip to content

config: enforcing terminal filters are the final filter in their respective chains #7779

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

Jump to bottom
Merged
merged 9 commits into from
Aug 13, 2019
Merged

config: enforcing terminal filters are the final filter in their respective chains #7779

merged 9 commits into from
Aug 13, 2019

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Jul 31, 2019 •
edited
Loading

An (no longer annoyingly one-off) solution to the common problem folks run into with Envoy configs where they add their filters behind the router filter and don't get why things aren't working. Ditto for HCM, tcp_proxy etc for L4

Risk Level: Low (except for folks with broken config)
Testing: new UT
Docs Changes: n/a
Release Notes: n/a
Fixes #7767

@alyssawilk
hcm: enforcing the router being the final filter in a chain
6fd5120 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
mattklein123 reviewed Jul 31, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Agreed we should have a dependency language, and we have discussed this before, but also agreed that this is a constant source of mistakes. Do you think we should do something similar with tcp_proxy at the network level?

/wait-any

test/extensions/filters/network/http_connection_manager/config_test.cc Outdated
@@ -166,6 +196,9 @@ stat_prefix: router
request_headers_for_tags:
- foo
http_filters:
- name: envoy.health_check
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed - I just wanted the comparable test to have 2 filters in the right order to verify that worked.
Can revert if you'd prefer - obviously we have enough integration tests of multiple filters

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine either way. I guess I have a mild preference to revert but I don't feel strongly about it.

@mattklein123 mattklein123 self-assigned this Jul 31, 2019
@alyssawilk
reviewer comments
2681f7a 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk changed the title http: enforcing the router being the final filter in a chain config: enforcing router and tcp_proxy being the final filter in their respective chains Jul 31, 2019
@alyssawilk
Copy link
Contributor Author

alyssawilk commented Jul 31, 2019 •
edited
Loading

Though in retrospect if we're doing tcp proxy should I also do HCM?

@alyssawilk
Copy link
Contributor Author

I'm going to go with no, just to avoid sliding down this slippery slope any further. I think if it causes envoy maintainer burden we can always add it later. :-)

mattklein123
mattklein123 previously approved these changes Jul 31, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Cool, thanks, agreed let's do this and see how it goes.

@mattklein123 mattklein123 mentioned this pull request Jul 31, 2019
Closed
@mattklein123
Copy link
Member

@alyssawilk please check out my comment here #7767 (comment) before you merge.

@alyssawilk
slipped off the slope directly into yak shaving
756e84b 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk changed the title config: enforcing router and tcp_proxy being the final filter in their respective chains config: enforcing terminal filters are the final filter in their respective chains Jul 31, 2019
@alyssawilk alyssawilk mentioned this pull request Jul 31, 2019
Closed
@alyssawilk alyssawilk added the no stalebot Disables stalebot from closing an issue label Jul 31, 2019
zuercher
zuercher previously approved these changes Aug 1, 2019
View reviewed changes
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thank you!

I took a quick glance over the other network and http filters, and the only other terminal filter that I see is envoy.echo. That said, it's also the least likely to be combined with other filters.

The only other thought I had is whether it's worth it document these or if just preventing the incorrect usage is enough.

@zuercher
Copy link
Member

zuercher commented Aug 1, 2019

Oh actually, should we require a terminal filter at the end of a filter chain?

@mattklein123
Copy link
Member

Oh actually, should we require a terminal filter at the end of a filter chain?

+1 this SGTM

@alyssawilk
Copy link
Contributor Author

+1 on terminal filter, though that may make this a breaking change for folks. I'll definitely add this to the version history. I don't think it merits envoy-announce but please chime in if you think it does.

@alyssawilk
Merge branch 'master' into router_is_last
da16f3e 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
terminal iff last
aa6a58a 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
and new unit tests
a8c6957 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
mattklein123 requested changes Aug 12, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks this is great. 1 small question and also needs a master merge.

/wait

source/extensions/filters/network/http_connection_manager/config.cc Outdated
processFilter(filters[i], i, "http", filter_factories_);
bool is_terminal = false;
processFilter(filters[i], i, "http", filter_factories_, is_terminal);
if (is_terminal && i != filters.size() - 1) {
Copy link
Member

Choose a reason for hiding this comment

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

This logic is repeated in multiple places (and I think not fully repeated in the upgrade path below). Can we put into a helper function potentially and use that everywhere?

@mattklein123 mattklein123 removed the no stalebot Disables stalebot from closing an issue label Aug 12, 2019
@alyssawilk
Merge branch 'refs/heads/master' into router_is_last
f7cb281 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@lizan lizan mentioned this pull request Aug 12, 2019
Closed
@alyssawilk
utility
15c3a81 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
mattklein123 previously approved these changes Aug 12, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks this is really awesome. @zuercher any further comments?

@alyssawilk
Copy link
Contributor Author

Argh.
I think to make this work I'm going to have to land this as 3 PRs - the API change without enforcement, update echo2 / envoy-filer-example, and then add enforcement.

@alyssawilk
doing the repo dance
4bbc8c3 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor Author

ok, undid the "and the last filter must be terminal" for network with some TODOs. I'll land an update to echo2, then reinstate what I had last commit (unless there's a better way to handle the various repos)

mattklein123
mattklein123 approved these changes Aug 13, 2019
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM post security release

@alyssawilk alyssawilk merged commit 3f5580f into envoyproxy:master Aug 13, 2019
@alyssawilk alyssawilk mentioned this pull request Aug 13, 2019
Merged
alyssawilk added a commit to envoyproxy/envoy-filter-example that referenced this pull request Aug 13, 2019
@alyssawilk
echo: tagging echo2 as a terminal filter (#96)
cfee93d 
follow up to envoyproxy/envoy#7779 

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk mentioned this pull request Aug 13, 2019
Merged
@alyssawilk alyssawilk deleted the router_is_last branch December 9, 2019 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@zuercher zuercher zuercher left review comments

@lizan lizan Awaiting requested review from lizan

Assignees

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP filters that put after envoy.router don't work but envoy can still boot up with that configuration
3 participants
@alyssawilk @mattklein123 @zuercher