listeners: add unified matcher for filter chains #20110
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
docs/root/intro/arch_overview/advanced/matching/_include/listener_complicated.yaml
Show resolved
Hide resolved
|
@snowp might make sense for you to take a look from generic matcher perspective as well. |
|
/retest |
|
Retrying Azure Pipelines: |
| const auto& match_result = Matcher::evaluateMatch<Network::MatchingData>(*matcher_, data); | ||
| ASSERT(match_result.match_state_ == Matcher::MatchState::MatchComplete, | ||
| "Matching must complete for network streams."); | ||
| if (match_result.result_) { | ||
| const auto result = match_result.result_(); | ||
| return result->getTyped<FilterChainNameAction>().chain_.get(); | ||
| } | ||
| return default_filter_chain_.get(); |
There was a problem hiding this comment.
Maybe add a trace/debug log here per @howardjohn's point on debugging?
There was a problem hiding this comment.
Added a debug log when matcher refers to a missing chain. I think that's the confusing state that @howardjohn referred to.
| // network properties. This matcher is used as a replacement for the filter chain match condition | ||
| // :ref:`filter_chain_match | ||
| // <envoy_v3_api_field_config.listener.v3.FilterChain.filter_chain_match>`. If specified, all | ||
| // :ref:`filter_chains <envoy_v3_api_field_config.listener.v3.Listener.filter_chains>` must have a |
There was a problem hiding this comment.
What happens if both are defined?
There was a problem hiding this comment.
The filter chain match is ignored when listener matcher is defined. I added a debug log to warn on listener construction.
| The matcher API replaces the existing filter :ref:`filter_chain_match | ||
| <envoy_v3_api_field_config.listener.v3.FilterChain.filter_chain_match>` field. When using the matcher API, the filter | ||
| chain match field is ignored and should not be set. |
There was a problem hiding this comment.
Should this be exposed to the user in some way? ie fail the config or at the very least log something?
There was a problem hiding this comment.
Added a debug log. Failing might complicate migration IMHO, so just ignoring the field seems reasonable as it is an opt-in feature.
| original destination port. The matcher in the listener selects one of the three filter chains ``http``, ``internal``, | ||
| and ``tls`` as follows: | ||
|
|
||
| * If the destination port is ``80``, then the filter chain ``http`` accepts the connection. |
Signed-off-by: Kuat Yessenov <kuat@google.com>
|
@kyessenov I think we forget a release note for this. Do you mind doing a follow up PR to add a release note so that we can have that be part of the next release? Thank you. |
Add unified matcher for network streams, as a replacement for filter chain match. See previous discussion in envoyproxy#18871 Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Andre Vehreschild <vehre@x41-dsec.de>
Add unified matcher for network streams, as a replacement for filter chain match. See previous discussion in envoyproxy#18871 Signed-off-by: Kuat Yessenov <kuat@google.com>
Commit Message: Add unified matcher for network streams, as a replacement for filter chain match. See previous discussion in #18871
Risk Level: low (requires opt-in)
Testing: unit, integration
Docs Changes: yes
Release Notes: yes
Fixes: #3411 #18685