config: enforcing terminal filters are the final filter in their respective chains by alyssawilk · Pull Request #7779 · envoyproxy/envoy

alyssawilk · 2019-07-31T16:21:02Z

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

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

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

mattklein123 · 2019-07-31T16:30:55Z

test/extensions/filters/network/http_connection_manager/config_test.cc

  request_headers_for_tags:
  - foo
 http_filters:
+- name: envoy.health_check


Why is this change needed?

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

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

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-07-31T17:16:49Z

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

alyssawilk · 2019-07-31T17:20:47Z

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

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

mattklein123 · 2019-07-31T17:46:45Z

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

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

zuercher

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 · 2019-08-01T22:21:30Z

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

mattklein123 · 2019-08-02T02:52:16Z

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

+1 this SGTM

alyssawilk · 2019-08-12T13:23:35Z

+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.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

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

/wait

mattklein123 · 2019-08-12T19:15:05Z

source/extensions/filters/network/http_connection_manager/config.cc

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


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?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

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

alyssawilk · 2019-08-12T22:08:03Z

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.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-08-12T22:23:57Z

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

LGTM post security release

follow up to envoyproxy/envoy#7779 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

hcm: enforcing the router being the final filter in a chain

6fd5120

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 reviewed Jul 31, 2019

View reviewed changes

repokitteh-read-only bot added the waiting:any label Jul 31, 2019

mattklein123 self-assigned this Jul 31, 2019

repokitteh-read-only bot removed waiting:any labels Jul 31, 2019

reviewer comments

2681f7a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

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

mattklein123 previously approved these changes Jul 31, 2019

View reviewed changes

mattklein123 mentioned this pull request Jul 31, 2019

HTTP filters that put after envoy.router don't work but envoy can still boot up with that configuration #7767

Closed

mattklein123 added the waiting label Jul 31, 2019

slipped off the slope directly into yak shaving

756e84b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed mattklein123’s stale review via 756e84b July 31, 2019 20:06

alyssawilk requested review from lizan and zuercher as code owners July 31, 2019 20:06

repokitteh-read-only bot removed the waiting label Jul 31, 2019

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 mentioned this pull request Jul 31, 2019

Cannot connect to gRPC service from envoy when using TLS, gets a response of 503 #7738

Closed

alyssawilk added the no stalebot Disables stalebot from closing an issue label Jul 31, 2019

zuercher previously approved these changes Aug 1, 2019

View reviewed changes

mattklein123 added the waiting label Aug 5, 2019

alyssawilk added 2 commits August 12, 2019 10:54

Merge branch 'master' into router_is_last

da16f3e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

terminal iff last

aa6a58a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed zuercher’s stale review via aa6a58a August 12, 2019 15:38

repokitteh-read-only bot removed the waiting label Aug 12, 2019

and new unit tests

a8c6957

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 requested changes Aug 12, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 12, 2019

mattklein123 removed the no stalebot Disables stalebot from closing an issue label Aug 12, 2019

Merge branch 'refs/heads/master' into router_is_last

f7cb281

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan mentioned this pull request Aug 12, 2019

Source IP ACLs not working properly with TCP Proxy (Proto Proto enabled) #7895

Closed

utility

15c3a81

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Aug 12, 2019

mattklein123 previously approved these changes Aug 12, 2019

View reviewed changes

doing the repo dance

4bbc8c3

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed mattklein123’s stale review via 4bbc8c3 August 12, 2019 22:22

mattklein123 approved these changes Aug 13, 2019

View reviewed changes

alyssawilk merged commit 3f5580f into envoyproxy:master Aug 13, 2019

alyssawilk mentioned this pull request Aug 13, 2019

echo: tagging echo2 as a terminal filter envoyproxy/envoy-filter-example#96

Merged

alyssawilk added a commit to envoyproxy/envoy-filter-example that referenced this pull request Aug 13, 2019

echo: tagging echo2 as a terminal filter (#96)

cfee93d

follow up to envoyproxy/envoy#7779 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk mentioned this pull request Aug 13, 2019

config: fully validating terminal filter ordering for L4 #7904

Merged

alyssawilk deleted the router_is_last branch December 9, 2019 21:11

Conversation

alyssawilk commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Jul 31, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jul 31, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher commented Aug 1, 2019

Uh oh!

mattklein123 commented Aug 2, 2019

Uh oh!

alyssawilk commented Aug 12, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Aug 12, 2019

Uh oh!

alyssawilk commented Aug 12, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alyssawilk commented Jul 31, 2019 •

edited

Loading

alyssawilk commented Jul 31, 2019 •

edited

Loading