Skip to content
Closed
Changes from 4 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
167 changes: 166 additions & 1 deletion api/envoy/config/listener/v3/listener_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ message Filter {
// listed at the end, because that's how we want to list them in the docs.
//
// [#comment:TODO(PiotrSikora): Add support for configurable precedence of the rules]
// [#next-free-field: 14]
// [#next-free-field: 15]
message FilterChainMatch {
option (udpa.annotations.versioning).previous_message_type =
"envoy.api.v2.listener.FilterChainMatch";
Expand All @@ -109,6 +109,131 @@ message FilterChainMatch {
EXTERNAL = 2;
}

// Matches filter chains by the destination port.
Comment thread
kyessenov marked this conversation as resolved.
Outdated
// [#next-free-field: 6]
message DestinationPort {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the two fields in this proto be in a oneof? Or are there cases where they will both be used at the same time?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's technically OR operator, so it's more expressive without oneof, I think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay. Please make that clear in the comments.

// Optional destination port to consider when use_original_dst is set on the
// listener in determining a filter chain match.
google.protobuf.UInt32Value destination_port = 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we are going to make this more generic anyway, can we change this to a range or list similar to source ports?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ack, makes sense.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we have a range, do we need the single port? You can always put a single port into the range definition so there is no backwards compat issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks rather ugly to have one element range for many chains. Majority of listeners with original_dst just want one port.

[(validate.rules).uint32 = {lte: 65535 gte: 1}];

// Match destination port by range.
type.v3.Int32Range destination_port_range = 5;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be a repeated set of ranges?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense.

}

// Matches filter chains by the destination IP address.
message DestinationIP {
// If non-empty, an IP address and prefix length to match addresses when the
// listener is bound to 0.0.0.0/:: or when use_original_dst is specified.
// Empty list matches all.
repeated core.v3.CidrRange prefix_ranges = 1;
}

// Matches filter chains by the directly connected source IP address (this
// will only be different from the source IP address when using a listener
// filter that overrides the source address, such as the :ref:`Proxy Protocol
// listener filter <config_listener_filters_proxy_protocol>`).
message DirectSourceIP {
// The criteria is satisfied if the directly connected source IP address of the downstream
// connection is contained in at least one of the specified subnets. If the list is empty,
// the directly connected source IP address is ignored.
repeated core.v3.CidrRange direct_source_prefix_ranges = 1;
}

// Matches filter chains by the connection source IP type.
message SourceType {
// Specifies the connection source IP match type. Can be any, local or external network.
ConnectionSourceType source_type = 1 [(validate.rules).enum = {defined_only: true}];
}

// Matches filter chains by the source IP address.
message SourceIP {
// The criteria is satisfied if the source IP address of the downstream
// connection is contained in at least one of the specified subnets. If the list is empty, the source IP address is
// ignored.
repeated core.v3.CidrRange source_prefix_ranges = 1;
}

// Matches filter chains by the source port.
message SourcePort {
// The criteria is satisfied if the source port of the downstream connection
// is contained in at least one of the specified ports. If the parameter is
// not specified, the source port is ignored.
repeated uint32 source_ports = 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Repeated set of ranges to make all of the port matching consistent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

[(validate.rules).repeated = {items {uint32 {lte: 65535 gte: 1}}}];
}

// Matches filter chains by the server name indication.
message SNI {
// If non-empty, a list of server names (e.g. SNI for TLS protocol) to consider when determining
// a filter chain match. Those values will be compared against the server names of a new
// connection, when detected by one of the listener filters.
//
// The server name will be matched against all wildcard domains, i.e. ``www.example.com``
// will be first matched against ``www.example.com``, then ``*.example.com``, then ``*.com``.
//
// Note that partial wildcards are not supported, and values like ``*w.example.com`` are invalid.
//
// .. attention::
//
// See the :ref:`FAQ entry <faq_how_to_setup_sni>` on how to configure SNI for more
// information.
repeated string server_names = 1;
}

// Matches filter chains by the transport protocol.
message TransportProtocol {
// If non-empty, a transport protocol to consider when determining a filter chain match.
// This value will be compared against the transport protocol of a new connection, when
// it's detected by one of the listener filters.
//
// Suggested values include:
//
// * ``raw_buffer`` - default, used when no transport protocol is detected,
// * ``tls`` - set by :ref:`envoy.filters.listener.tls_inspector <config_listener_filters_tls_inspector>`
// when TLS protocol is detected.
string transport_protocol = 1;
}

// Matches filter chains by the application protocol (e.g. ALPN for TLS protocol).
message ApplicationProtocol {
// If non-empty, a list of application protocols (e.g. ALPN for TLS protocol) to consider when
// determining a filter chain match. Those values will be compared against the application
// protocols of a new connection, when detected by one of the listener filters.
//
// Suggested values include:
//
// * ``http/1.1`` - set by :ref:`envoy.filters.listener.tls_inspector
// <config_listener_filters_tls_inspector>`,
// * ``h2`` - set by :ref:`envoy.filters.listener.tls_inspector <config_listener_filters_tls_inspector>`
//
// .. attention::
//
// Currently, only :ref:`TLS Inspector <config_listener_filters_tls_inspector>` provides
// application protocol detection based on the requested
// `ALPN <https://en.wikipedia.org/wiki/Application-Layer_Protocol_Negotiation>`_ values.
//
// However, the use of ALPN is pretty much limited to the HTTP/2 traffic on the Internet,
// and matching on values other than ``h2`` is going to lead to a lot of false negatives,
// unless all connecting clients are known to use ALPN.
repeated string application_protocols = 1;
}

// Individual filter chain match predicate.
message FilterChainMatchPredicate {
// The typed config for the match predicate extension. The type URL will be
// used to identify the extension. In the case that the type URL is
// *xds.type.v3.TypedStruct* (or, for historical reasons,
// *udpa.type.v1.TypedStruct*), the inner type URL of *TypedStruct* will be
// utilized. See the :ref:`extension configuration overview
// <config_overview_extension_configuration>` for further details.
google.protobuf.Any typed_config = 1 [(validate.rules).any = {required: true}];

// Flag to continue matching if this match is a wildcard match and there is a
// more specific match.
bool fallthrough = 2;
}

reserved 1;

// Optional destination port to consider when use_original_dst is set on the
Expand Down Expand Up @@ -193,6 +318,46 @@ message FilterChainMatch {
// and matching on values other than ``h2`` is going to lead to a lot of false negatives,
// unless all connecting clients are known to use ALPN.
repeated string application_protocols = 10;

// A list of filter chain matching predicate extensions specifying the order
// of selection for the most specific filter chain. The filter chains in a
// single listener must have the identical order of predicate extension type
// URLs. For backwards compatibility, if the existing fields in the filter
// chain match are specified, the fields are converted to their corresponding
// filter chain matching predicates in the following default matching order:
// 1. :ref:`DestinationPort <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.DestinationPort>`.
// 2. :ref:`DestinationIP <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.DestinationIP>`.
// 3. :ref:`SNI <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.SNI>`.
// 4. :ref:`TransportProtocol <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.TransportProtocol>`.
// 5. :ref:`ApplicationProtocol <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.ApplicationProtocol>`.
// 6. :ref:`DirectSourceIP <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.DirectSourceIP>`.
// 7. :ref:`SourceType <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.SourceType>`.
// 8. :ref:`SourceIP <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.SourceIP>`.
// 9. :ref:`SourcePort <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.SourcePort>`.
//
// At each step, the predicate is evaluated for each filter chain. If the
// predicate returns false, the filter chain is eliminated from subsequent
// consideration. If the predicate returns true, then the decision is based
// on the ``fallthrough`` flag. If there is another more specific match for
// the same predicate and the flag is not set, then the less specific match
// is eliminated. If the flag is set, then it remains as a candidate for the
// next step.
//
// For example, consider SNI ``www.example.com``and two filter chains with
// the predicates ```*.example.com``` and ```*.com```. If ``fallthrough``
// flag is not set then only the filter chain ```*.example.com``` matches. If
// ``fallthrough`` flag is set on ```*.com```, then both filter chains match.
// In general, the order of specificity is domain specific as defined by the
// predicate extension. The flag should be set when matching on multiple
// properties in order for a default chain to apply without explicit specific
// matching of the first properties in the list.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you beef this up even a bit more with a worked example? I'm having a hard time wrapping my head around the case in which someone would want multiple matches, and also, if there are multiple matches, what does that mean for trie/sequential based matching? Does it basically mean that if there are multiple matches we keep recursing downward on N tree branches to see if there are further reductions in the search space?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Related to trie/sequential-based matching, would it make sense for this to use the new generic matching API that @snowp has been working on, rather than inventing yet another matcher structure?

https://github.com/cncf/xds/blob/main/xds/type/matcher/v3/matcher.proto

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mattklein123 Adding more explanations. The extra matches are already inevitable in the existing API because of overlapping wildcards. What we're trying to change is making it more succinct for the control plane to express. I think there is normally no need to have multiple matches at the very end, but in the middle of matching, the "default" case has to be kept around in the search space until the full "special" case is matched. What happens now, is that control plane has to provide many identical "default" cases for each step of "special" case matching, and that quickly proliferates.

I think we can implement the fallthrough wildcard internally with trie upward propagation. E.g. if there is a chain FC1 wildcard "*.com" and trie node "example.com" for FC2, then we automatically add FC1 to node "example.com". This what control plane would have to do explicitly.

@markdroth I took a look at the generic matcher API. I think it's rather difficult to use. Our main issue with the existing state is that it's too hard to use right (e.g. Istio will not migrate to the status quo API and will stay on the "undeprecated" field). Make it more abstract does not seem to help with usability IMHO. It also seems L7-oriented right now, and the set of matchers is rather distinct. I can be convinced the other way, if we can construct some examples that are easy to comprehend.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the semantics of the generic matching API are actually much clearer and easier to understand, because instead of memorizing a set of precedence rules, you can explicitly encode the precedence in the matcher structure. It's actually not abstract at all; it's very explicitly and precise about what it represents.

The generic matching API was specifically designed to be extensible, so that you can plug in whatever inputs and whatever new match types you need. It was intentionally designed not to be L7-specific. For background, see https://docs.google.com/document/d/1G4g-6q0IArz_ERgqixzZCM0-wbexFYuBUvT9hkV7QRU/edit.

I really don't think we should be reinventing this wheel yet again. In the long run, we want all matching in xDS to move to the new generic matching API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It hadn't occurred to me to use the generic matching API for this, but given that it inherently supports sub-linear matching, I tend to agree with @markdroth that we should see if we can use it. Can you mock up what that might look like and we can discuss? If there are usability issues with that API we should fix them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, I understand that any trie decision tree could be encoded in abstract sense. But I'm not seeing the succinct representation that avoids repeated (2^n) filter chains with slightly varied matching conditions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that use of an API extension for an input or a matcher does not imply that the functionality is not a first-class citizen; it simply implies that it's a protocol-specific input or match type that is not built into the generic matching framework itself. For example, HTTP header inputs are an extension, and that is very much a first-class citizen in the API.

The idea of the generic matching API is that you construct a tree of matchers to represent things like AND and OR operations, where each individual node in the tree can be either a MatcherList (for linear matching) or a MatcherTree (for sublinear matching). In either case, if a match is found, we use the corresponding OnMatch, which can be either a protocol-specific action or a nested matcher (i.e., another node in the tree of matchers). If no match is found in a node, there is another OnMatch called on_no_match that will be used if populated; if it's not populated, then the node is considered not to match, and matching resumes from there.

For the example you cited, you'd structure it as the top node being a MatcherList, where the matchers in the list are:

  1. Match on path=/path. If that matches, use a nested matcher that checks for the header X = Y. (If the nested matcher does not match and does not have an on_no_match field, then we will move on to the next entry in the list.)
  2. Match on path prefix /.

Alternatively, if you wanted that second rule to be applied to anything that was not matched previously (i.e., you wanted it to apply to all requests, not just those with prefix="/"), you could move it out of the list and put it in the on_no_match field in the top node.

@kyessenov kyessenov Nov 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to build a matcher tree internally in Envoy? I think we have to use an outer MatcherList because of the defaulting behavior. But then each item might have overlapping predicates. For example,

  1. MatchList item1: Match on destination port 80.
  2. MatchList item2-1000: Match on other ports in the range of 1-10000.
  3. MatchList item: Match on destination port range 80-10000.

The linear semantics is sub-optimal here at the outer level. We want an implicit lookup tree, but we also don't want directly go into individual node 1-1000 without checking the default case 1001, ideally without iteration over items.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a way to build a matcher tree internally in Envoy

You should be able to build up any config internally that you like, though if there are API issues with the type of matching you want to do we should sort that out.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know why we'd want to build the tree internally. I think the goal here should be to allow the API to explicitly configure how the matching should be done, so that there's no more counter-intuitive magic here.

I can tell you from when we implemented this logic in gRPC recently that the current matching behavior here is very difficult to understand. The behavior is that it basically hard-codes the order of matching to the following list:

  1. destination port
  2. destination IP
  3. server name
  4. transport protocol
  5. application protocol
  6. connection source type
  7. source IP
  8. source port

Trying to reason through how a given connection will be matched to a filter chain requires that you memorize that list, and even then, it takes real thought and it's very confusing to implement correctly. I think making this explicit in the API would be a big improvement.

For the destination port example, I think it should be fairly simply to define a new type of sublinear matcher that is keyed on port ranges. You could have one entry for ports 1-79, one for port 80, and one for ports 81-10000. Any given incoming connection would fit into exactly one of those three categories, so there's no need for linear matching behavior.

The only complication I see here is that there will wind up being multiple leaves in the tree for each filter chain, and we don't want to have to duplicate the filter chain for each leaf that uses it. But I think this can be addressed fairly simply by moving the actual filter chain definitions to a separate map, keyed by some opaque name, and then having the matcher leaves refer to the keys in that map. That way, whenever multiple leaves refer to the same chain, the only duplication is the opaque name.

//
// In case, there is more than one filter chain candidates remaining after
// the process completes, the first of the filter chains in the order of
// their declaration is selected.
//
// [#not-implemented-hide:]
repeated FilterChainMatchPredicate predicates = 14;
}

// A filter chain wraps a set of match criteria, an option TLS context, a set of filters, and
Expand Down