listener: allow setting only a default filter chain by tbarrella · Pull Request #14025 · envoyproxy/envoy

tbarrella · 2020-11-14T01:10:54Z

I didn't add anything to the release notes because it looks like specifying a default filter chain will be new for the upcoming release and this is fixing a bug

Commit Message:
listener: allow setting only a default filter chain

Signed-off-by: Taylor Barrella tabarr@google.com

Additional Description:
Risk Level: Low
Testing: New unit test fails without these changes. I also searched for occurrences of .filter_chains() and only found them in listener_impl.cc and test files
Docs Changes: N/A
Release Notes:
Fixes #13685

Signed-off-by: Taylor Barrella <tabarr@google.com>

tbarrella · 2020-11-14T01:13:19Z

cc @lambdai

lambdai

Thanks! Almost there!

Could you fix and merge master to see if coverage passes? I don't think transport socket coverage should be impacted by your change

source/server/listener_impl.cc

Signed-off-by: Taylor Barrella <tabarr@google.com>

lambdai

LGTM.
Thank you for the fix!

tbarrella · 2020-11-17T19:30:50Z

/retest

repokitteh-read-only · 2020-11-17T19:30:54Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14025 (comment) was created by @tbarrella.

see: more, trace.

mattklein123

Thanks for fixing. One question.

/wait-any

mattklein123 · 2020-11-18T21:46:51Z

source/server/listener_impl.cc

             });
 }
+
+bool usesProxyProto(const envoy::config::listener::v3::Listener& config) {


Doesn't this need to look like needTlsInspector with a search through all filter chains? I don't understand how you can only check the default or the first chain? Was this a bug that already existed?

It seems this is the only file that checks use_proxy_proto, so maybe it was a bug/undocumented expectation. Not sure if the previous behavior should be modified in this change. I just realized it might be more conservative to change the condition from config.has_default_filter_chain() to config.filter_chains().empty() though

To me this is a bug as I don't think it makes any sense. If we don't want to fix this now I would make no changes here and then fix it in a follow up?

If we auto inject proxy proto I think all filter chains should probably be asking for it?

Makes sense to me. I'm reluctant to change two behaviors in one commit; opened #14085 for now

@mattklein123 It's an invalid config if partial of the filter chain set use proxy proto while other filter chain don't. We check the first filter chain here for back compatibility.

There is no enforcement at this moment. And don't add enforcement in this PR.
However, could you add the warning message?

I didn't add a printed warning since deprecation should serve that purpose, but I added a comment referencing #14085

Given that none of this make any sense, do you want to just revert this part of the PR entirely? Also do we want a warning in a PR or do you want to do this as a follow up? Otherwise this change LGTM and thanks for the discussion here.

/wait-any

This is still needed to avoid an indexing error when only a default filter chain is provided. I'd rather leave user-facing warnings etc. as a follow-up (part of the other issue) since this is consistent with existing behavior/isn't a regression or anything

OK fair enough. If you can fix all of ^ as a follow up that would be appreciated.

Signed-off-by: Taylor Barrella <tabarr@google.com>

tbarrella · 2020-11-19T03:30:35Z

/retest

repokitteh-read-only · 2020-11-19T03:30:39Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14025 (comment) was created by @tbarrella.

see: more, trace.

tbarrella · 2020-11-19T16:55:12Z

/retest

repokitteh-read-only · 2020-11-19T16:55:16Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14025 (comment) was created by @tbarrella.

see: more, trace.

mattklein123

Thanks!

Signed-off-by: Taylor Barrella <tabarr@google.com>

Signed-off-by: Taylor Barrella <tabarr@google.com> Signed-off-by: Qin Qin <qqin@google.com>

listener: allow setting only a default filter chain

26a28f1

Signed-off-by: Taylor Barrella <tabarr@google.com>

lambdai suggested changes Nov 16, 2020

View reviewed changes

source/server/listener_impl.cc Outdated Show resolved Hide resolved

source/server/listener_impl.cc Outdated Show resolved Hide resolved

tbarrella added 2 commits November 16, 2020 19:27

Merge remote-tracking branch 'origin/master' into filter-chain

f611de6

Signed-off-by: Taylor Barrella <tabarr@google.com>

Address comments

7d3ed36

Signed-off-by: Taylor Barrella <tabarr@google.com>

tbarrella requested a review from lambdai November 16, 2020 19:32

tbarrella added 2 commits November 17, 2020 00:51

Merge remote-tracking branch 'origin/master' into filter-chain

74ed282

Signed-off-by: Taylor Barrella <tabarr@google.com>

Merge remote-tracking branch 'origin/master' into filter-chain

e36ab02

Signed-off-by: Taylor Barrella <tabarr@google.com>

lambdai previously approved these changes Nov 17, 2020

View reviewed changes

mattklein123 self-assigned this Nov 18, 2020

mattklein123 reviewed Nov 18, 2020

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Nov 18, 2020

mattklein123 added the waiting label Nov 18, 2020

tbarrella mentioned this pull request Nov 18, 2020

listener: deprecate use_proxy_proto #14085

Closed

Adjust condition and add comment

dda71bb

Signed-off-by: Taylor Barrella <tabarr@google.com>

tbarrella dismissed lambdai’s stale review via dda71bb November 19, 2020 00:52

repokitteh-read-only bot added waiting:any and removed waiting waiting:any labels Nov 19, 2020

mattklein123 approved these changes Nov 19, 2020

View reviewed changes

mattklein123 merged commit 6be36de into envoyproxy:master Nov 19, 2020

tbarrella deleted the filter-chain branch November 19, 2020 17:36

andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020

listener: allow setting only a default filter chain (envoyproxy#14025)

a52935a

Signed-off-by: Taylor Barrella <tabarr@google.com>

qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020

listener: allow setting only a default filter chain (envoyproxy#14025)

a5f74a6

Signed-off-by: Taylor Barrella <tabarr@google.com> Signed-off-by: Qin Qin <qqin@google.com>

Conversation

tbarrella commented Nov 14, 2020

Uh oh!

tbarrella commented Nov 14, 2020

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

tbarrella commented Nov 17, 2020

Uh oh!

repokitteh-read-only bot commented Nov 17, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbarrella commented Nov 19, 2020

Uh oh!

repokitteh-read-only bot commented Nov 19, 2020

Uh oh!

tbarrella commented Nov 19, 2020

Uh oh!

repokitteh-read-only bot commented Nov 19, 2020

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