listener: allow setting only a default filter chain #14025
Conversation
Signed-off-by: Taylor Barrella <tabarr@google.com>
|
cc @lambdai |
Signed-off-by: Taylor Barrella <tabarr@google.com>
Signed-off-by: Taylor Barrella <tabarr@google.com>
Signed-off-by: Taylor Barrella <tabarr@google.com>
Signed-off-by: Taylor Barrella <tabarr@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
| @@ -55,6 +62,12 @@ bool needTlsInspector(const envoy::config::listener::v3::Listener& config) { | |||
| filter.name() == "envoy.listener.tls_inspector"; | |||
| }); | |||
| } | |||
|
|
|||
| bool usesProxyProto(const envoy::config::listener::v3::Listener& config) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Makes sense to me. I'm reluctant to change two behaviors in one commit; opened #14085 for now
There was a problem hiding this comment.
@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 was a problem hiding this comment.
There is no enforcement at this moment. And don't add enforcement in this PR.
However, could you add the warning message?
There was a problem hiding this comment.
I didn't add a printed warning since deprecation should serve that purpose, but I added a comment referencing #14085
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Taylor Barrella <tabarr@google.com>
Signed-off-by: Taylor Barrella <tabarr@google.com> Signed-off-by: Qin Qin <qqin@google.com>
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 inlistener_impl.ccand test filesDocs Changes: N/A
Release Notes:
Fixes #13685