Skip to content
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.

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

listener: filter chain unified matchers #18871

Closed
Closed
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
6a37223
[wip] Add filter chain match predicate order
kyessenov Nov 2, 2021
8aa165f
spelling
kyessenov Nov 2, 2021
cfdb93a
review
kyessenov Nov 5, 2021
a13ed92
review
kyessenov Nov 5, 2021
621fcbf
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Nov 8, 2021
2228fef
review
kyessenov Nov 8, 2021
b610420
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Nov 12, 2021
f45bc06
review
kyessenov Nov 15, 2021
bada313
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Nov 15, 2021
3f8f8e4
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Nov 17, 2021
937f7a8
review
kyessenov Nov 17, 2021
9fd0f34
review
kyessenov Nov 17, 2021
595eb18
add move note
kyessenov Nov 22, 2021
7d7909a
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Nov 22, 2021
9a56f14
merge fix
kyessenov Nov 29, 2021
262d084
merge
kyessenov Nov 29, 2021
76e5040
more review
kyessenov Dec 1, 2021
3b92ec5
typo
kyessenov Dec 1, 2021
03fbfc6
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Dec 15, 2021
ddeeaf9
update
kyessenov Dec 15, 2021
be4636d
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Jan 7, 2022
72b978e
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Feb 1, 2022
e981df6
review
kyessenov Feb 1, 2022
a9b056a
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Feb 2, 2022
987a2fd
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov Feb 17, 2022
0185225
try validation
kyessenov Feb 18, 2022
d1c8f75
verify example
kyessenov Feb 23, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/envoy/config/listener/v3/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ api_proto_package(
"//envoy/type/v3:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
"@com_github_cncf_udpa//xds/core/v3:pkg",
"@com_github_cncf_udpa//xds/type/matcher/v3:pkg",
],
)
63 changes: 62 additions & 1 deletion api/envoy/config/listener/v3/listener.proto
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import "google/protobuf/duration.proto";
import "google/protobuf/wrappers.proto";

import "xds/core/v3/collection_entry.proto";
import "xds/type/matcher/v3/matcher.proto";

import "envoy/annotations/deprecation.proto";
import "udpa/annotations/security.proto";
Expand All @@ -36,7 +37,7 @@ message ListenerCollection {
repeated xds.core.v3.CollectionEntry entries = 1;
}

// [#next-free-field: 32]
// [#next-free-field: 33]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to block the PR, but if we wanted to really prove out how usable the API is, I think we could do it without implementing it in Envoy. Snow has a golang implementation of the generic matcher. Istio already has an extensive test suite for filter chain matching, based on a golang implementation of the current FCM logic. So if we were able to implement the new matcher in Istio (which presumably we need to do anyways after this merges), we could get full test coverage as well before committing to the Envoy implementation.

All of that may be harder than just doing the implementation though, just throwing out the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generic matcher does not have custom match implemented in either envoy or golang, so it's not really realistic at this stage.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to envoyproxy/go-control-plane#511. I guess would need to add all the custom matchers though

message Listener {
option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.Listener";

Expand Down Expand Up @@ -120,6 +121,66 @@ message Listener {
// :ref:`FAQ entry <faq_how_to_setup_sni>`.
repeated FilterChain filter_chains = 3;

// Unified matcher resolving the filter chain name from the network properties. This matcher is used as a replacement
// for the per-filter chain match condition
// `filter_chain_match <envoy_v3_api_msg_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 non-empty and unique :ref:`name <envoy_v3_api_field_config.listener.v3.FilterChain.name>` fields and omit
// `filter_chain_match <envoy_v3_api_msg_config.listener.v3.FilterChain.filter_chain_match>` field.
//
// Example 1: The following matcher selects three filter chains as follows:
//
// * if the destination port is 80, then the filter chain "http" is selected;
// * if the destination port is 443 and the source IP is in the range 192.0.0.0/2, then the filter chain "internal" is selected;
// * otherwise, if the destination port is 443, then the filter chain "https" is selected;
// * otherwise, the default filter chain is selected (or the connection is rejected without the default filter chain).
//
// .. code-block:: yaml
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this stanza use the actual config checking/validation version so it doesn't get out of date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, pending relevant protos merged.

//
// filter_chain_matcher:
// matcher_tree:
// input:
// typed_config:
// "@type": type.googleapis.com/envoy.extensions.matching.common_inputs.network.v3.DestinationPortInput
// exact_match_map:
// map:
// "80":
// action:
// name: http
// "443":
// matcher:
// matcher_tree:
// input:
// typed_config:
// "@type": type.googleapis.com/envoy.extensions.matching.common_inputs.network.v3.SourceIPInput
// custom_match:
Copy link
Member

Choose a reason for hiding this comment

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

Even though it's probably discussed in the documentation for the SNI custom matcher, can you clarify below the match semantics for different server names? I assume it would be implemented as some type of trie with most specific matches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it might be better to have a look at the network inputs and trie matcher PRs. The inputs should generally be constant in the input, and matching should be sublinear with IP and domain names. Port ranges should use segment trees for logarithmic performance. ALPN lists are tricky to handle, which needs some core matcher framework support to improve.

// typed_config:
// "@type": type.googleapis.com/xds.type.matcher.v3.IPMatcher
// range_matchers:
// - ranges:
// - address_prefix: 192.0.0.0
// prefix_len: 2
// on_match:
// action:
// name: internal
// - ranges:
// - address_prefix: 0.0.0.0
// on_match:
// action:
// name: https
//
//
// .. note::
//
// Once matched, each connection is permanently bound to its filter chain.
// If the matcher changes but the filter chain remains the same, the
// connections bound to the filter chain are not drained. If, however, the
// filter chain is removed or structurally modified, then the drain for its
// connections is initiated.
//
// [#not-implemented-hide:]
Copy link
Member

Choose a reason for hiding this comment

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

I see this configuration as being one of the ones that will be very difficult for users to actually piece together without examples. Can we make sure that somehow we wire up the extension docs system here so that this somehow lists out all supported match inputs and relevant typed configs, etc.? We need to guide users much more specifically on how to use this new field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, we need more examples. I can add some but I'd really need #19493 merged first for protos to become available. There is an issue with cncf/xds protos not being documented.

repeated xds.type.matcher.v3.Matcher filter_chain_matcher = 32;

// If a connection is redirected using *iptables*, the port on which the proxy
// receives it might be different from the original destination address. When this flag is set to
// true, the listener hands off redirected connections to the listener associated with the
Expand Down
3 changes: 3 additions & 0 deletions api/envoy/config/listener/v3/listener_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,9 @@ message FilterChain {
// [#not-implemented-hide:] The unique name (or empty) by which this filter chain is known. If no
// name is provided, Envoy will allocate an internal UUID for the filter chain. If the filter
// chain is to be dynamically updated or removed via FCDS a unique name must be provided.
// Note: :ref:`filter_chain_matcher
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Note: :ref:`filter_chain_matcher
// Note: use of :ref:`filter_chain_matcher

// <envoy_v3_api_field_config.listener.v3.Listener.filter_chain_matcher>`
// requires that filter chains are uniquely named.
string name = 7;

// [#not-implemented-hide:] The configuration to specify whether the filter chain will be built on-demand.
Expand Down