listener: filter chain unified matchers #18871

kyessenov · 2021-11-02T19:09:50Z

Commit Message: Generalize filter chain match extensions and allow custom order of specificity matching.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
Fixes: #3411 #18685

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2021-11-02T19:09:57Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18871 was opened by kyessenov.

see: more, trace.

mattklein123 · 2021-11-02T19:13:53Z

cc @tonya11en

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2021-11-02T21:28:24Z

api/envoy/config/listener/v3/listener.proto

+  // 8. :ref:`SourceIP <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.SourceIP>`.
+  // 9. :ref:`SourcePort <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.SourcePort>`.
+  //
+  // Filter chain match conditions that are not in the matching order list are


I'm debating whether we should perform a linear check for predicates that are not in this list instead. That seems more intuitive.

Sorry what do you mean by this exactly?

Imagine someone adds a new filter chain match predicate using the metadata. If we don't assume this order is complete, then there will be multiple matching filter chains left by default for metadata matching.

lambdai · 2021-11-02T21:32:50Z

api/envoy/config/listener/v3/listener.proto

+  // 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.


This is a breaking change. Why do you want to relax the constraint?

My understanding is that changing match order may impact the last select chain. That's fine.

The existing implementation does not guarantee uniqueness, right? E.g. you can have two source port ranges that are overlapping but neither is more specific.

It guarantees. An exception will be thrown upon before executing listener update.

I need to check the source port range, but for other match criteria, e.g. the ip range, we continue to drill down the rest matchers to confirm no 2 chains can be selected

Proof

if (!source_ports_map.try_emplace(source_port, filter_chain).second) { // If we got here and found already configured branch, then it means that this FilterChainMatch // is a duplicate, and that there is some overlap in the repeated fields with already processed // FilterChainMatches. throw EnvoyException(fmt::format("error adding listener '{}': multiple filter chains with " "overlapping matching rules are defined", address_->asString())); }

Mandating that matching predicates form an independent join lattice seems unnecessary. So what if there are two overlapping IP ranges? Maybe it's down-selected further in some other condition.

I tend to agree with @kyessenov though I'm wary of changing the implementation in subtle ways for the default. Is it hard to keep the existing constraint for now, at least for predicates that check this type of thing? It seems not too hard?

I'll have to look at the implementation in more detail to answer. I am not sure how to detect this statically since you can have a product {sni: ["x", "y"], source_ports: [80, 81]} and {sni: ["x", "z"], source_ports: [79, 80]}. For sni "x" and source port "80" there are two chains but it's not obvious from the definition.

This is actually an important usability point. I think the desire to have only one matching chain makes it really hard to use the filter chains. The user expects empty match predicates to match everything, not nothing. So it's up to the user to structure the matches so that only one chain remains in most cases, but we should handle multiple "match-all" chains.

mattklein123

Thanks for working on this. A few questions to get started.

/wait

mattklein123 · 2021-11-03T15:36:07Z

api/envoy/config/listener/v3/listener.proto

+  // 8. :ref:`SourceIP <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.SourceIP>`.
+  // 9. :ref:`SourcePort <envoy_v3_api_msg_config.listener.v3.FilterChainMatch.SourcePort>`.
+  //
+  // Filter chain match conditions that are not in the matching order list are


Sorry what do you mean by this exactly?

mattklein123 · 2021-11-03T15:37:39Z

api/envoy/config/listener/v3/listener.proto

+  // 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.


I tend to agree with @kyessenov though I'm wary of changing the implementation in subtle ways for the default. Is it hard to keep the existing constraint for now, at least for predicates that check this type of thing? It seems not too hard?

mattklein123 · 2021-11-03T15:38:47Z

api/envoy/config/listener/v3/listener_components.proto

+  message DestinationPort {
+    // 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


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

Ack, makes sense.

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?

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

mattklein123 · 2021-11-03T15:42:50Z

api/envoy/config/listener/v3/listener_components.proto

+  // Specifies the filter chain matching predicates to be used in the matching
+  // order. Each predicate extension must be specified at most once. For
+  // backwards compatibility, the existing fields in the filter chain match, if
+  // specified, are converted to their corresponding filter chain matching
+  // predicates in this list.
+  // [#not-implemented-hide:]


I'm confused about the purpose of this field in relation to matching_order. Don't we basically just want a list of type extensions somewhere that will be the configurable matching order? Why do we need matching order above also?

This is actually a set not a list of predicate extension configs for each predicate in the matching order. Not sure how else to express this concept. The order here is irrelevant.

This is related to my comment above about matching order. I'm confused as to why we can't just have everything expressed in a single ordered list? Basically just force people to instantiate the list with the type urls in the order they want? The existing config can be translated to the ordered list?

Each filter chain has different values for the predicates. Do you suggest each filter chain must have the same order for type URLs and then we deduce the global order from that? That makes sense.

I think one annoyance is that empty predicates must be specified.

OK I see the trade off, thanks for explaining. One one hand, it seems simpler if everything is embedded in the match and then we just verify that all of the chains have similar match ordering. On the other hand, there are downsides as you point out. I'm fine either way, but if we keep it like this can we beef up the docs? It's a bit confusing IMO.

Yes, the docs are clearly lacking, and it's really confusing. I think we need to give proper examples and start from use cases. Let me try to get some istio examples, may be that will help clarifying the right API.

howardjohn · 2021-11-04T17:37:29Z

api/envoy/config/listener/v3/listener.proto

+  // specific as defined by the predicate extension.
+  //
+  // 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


Are there cases where this behavior is desirable? This has caused endless pain for Istio and other Envoy users I have talked to. See #12572 for a lot of discussion.

Perhaps this is an opportunity to allow opting into a behavior that makes the matching more similar to other matching systems and that is easier to use without having a bunch of duplicated filter chains to appease the matching system?

@howardjohn The structured match is important for nested trie-matching algorithm, I think we cannot drop that at this point. The specific issue you had is with the defaulting. Each extension should default to allow-all, not deny-all, like in the above example issue you linked. We are fixing that with enforcement that empty matching predicate matches everything. The problem then becomes that multiple match chains will overlap, so we are also addressing that with the declared order.

Its a lot more than default, although that is how it started.

I am not sure the trie algorithm is broken, it just may be slower since you do not filter everything out at each step.

For example, if I want:

FC 1 matches on some complex criteria, say destination_port=80,transport_protocol=tls,application_protocols=[h2] FC 2 matches everything else

If I want to do this, instead of a single FC2, I need to make every possible permutation of FCMs. So I need 65535 (destination ports) * 2 (transport_protocols) * infinity (all permutations of ALPN, which is unbounded.

I know we discussed a port range and we have a default FCM now, but the general problem still persists. If you look at Istio config today we duplicate a bunch of filter simply to appease this rule. The root cause is that any new predicate you add, you then need to take all existing filter chains and duplicate them to have a match + not a match in most cases.

Just consider routing for example... if I have a match for /foo and for Header:foo=bar, then later add a match for /foo && Header:foo=bar, a call to /foo without header foo=bar would not match if it used FCM logic. I don't think any user expects that adding an additional match makes less things match. Certainly control plane authors don't, as none of the Istio or TD control plane maintainers knew about this for a very long time.

We will allow wildcards for ports, transport security, and ALPN, so the first issue is solved. We can also consider complement expression (not-expression), if necessary. Basically, there's nothing wrong with trie as long as the complement fits into the domain well. The idea is to extend the match conditions to avoid duplication of the chains, which makes sense.

Second issue is again similar. We'll just assume any value for path or header matches if not specified, unlike the existing proto. Yes, this was a mistake in the original definition, I think.

Is there something beyond defaulting and more expressive match conditions?

We'll just assume any value for path or header matches if not specified, unlike the existing proto.

I think this may be sufficient, will look into it a bit more

kyessenov · 2021-11-04T21:37:58Z

I think the crux of the problem is that the specificity elimination results in implicit negation of conditions from other filter chains.
For example, consider this example:

match1:
- SNI: *.com
- ports: 0-1000
match2:
- SNI: host.com
- ports: 80

And the following two inputs:

host.com, 81
x.com, 80

If we order SNI first, then the first input is rejected because match 2 eliminates match1 after SNI. If we order port first, then the second input is rejected because match2 eliminates match1 after ports. So it's impossible to express succinctly the desire to have a fallback filter chain. One has to define 4 matches with all combinations, and three duplicate filter chains.

My proposal is to add a boolean (call it continue) to each match condition indicating that specificity matching should not eliminate a particular filter chain:

match1:
- SNI: *.com, continue: true
- ports: 0-1000, continue: true

The algorithm then does not eliminate a wildcard match even if the more specific match exists. Logically, this accounts to duplicating the chain, and replacing the particular value with any other more specific value, which actually matches the user intent here. E.g. the above can be rolled out as:

match1:
- SNI: *.com
- ports: 0-1000
match1a:
- SNI: host.com (as well as other specific values matching the wildcard)
- ports: 0-1000
match1b:
- SNI: *.com (as well as other specific values matching the wildcard)
- ports: 80
match1c:
- SNI: host.com (as well as other specific values matching the wildcard)
- ports: 80

Thoughts about this proposal?

lambdai · 2021-11-04T22:15:13Z

match1:

SNI: *.com, continue: true

ports: 0-1000, continue: true

The continue seems acting as a opt-in fallback. This is nice to have.

A couple of questions

Combining your declared multiple chains are matching, the first is selected. How is the order defined?
What's the computation complexity of selecting the filter chain?

kyessenov · 2021-11-04T22:27:24Z

Combining your declared multiple chains are matching, the first is selected. How is the order defined?

For overlapping values, the choice is based on the order of the chains.

What's the computation complexity of selecting the filter chain?

I think it's the same as if we had unrolled into 4 chains. Perhaps, we can do better in the implementation since we have more awareness of the common filter chains. Do you think there is a trie that can do better in this example?

lambdai · 2021-11-04T23:54:44Z

Combining your declared multiple chains are matching, the first is selected. How is the order defined?

For overlapping values, the choice is based on the order of the chains.

The natural order in the repeated filter_chains field? It may require the control plane to well ordered if the control plane wants a stable order while adding or remove unrelated filter chain. That's not ideal but I don't know how bad it is

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123

Thanks, flushing out a few more questions/comments.

/wait

mattklein123 · 2021-11-08T16:45:04Z

api/envoy/config/listener/v3/listener_components.proto

+  message DestinationPort {
+    // 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


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?

mattklein123 · 2021-11-08T16:45:59Z

api/envoy/config/listener/v3/listener_components.proto

+        [(validate.rules).uint32 = {lte: 65535 gte: 1}];
+
+    // Match destination port by range.
+    type.v3.Int32Range destination_port_range = 5;


Should this be a repeated set of ranges?

Yeah, makes sense.

mattklein123 · 2021-11-08T16:46:27Z

api/envoy/config/listener/v3/listener_components.proto

+    // 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


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

mattklein123 · 2021-11-08T16:49:57Z

api/envoy/config/listener/v3/listener_components.proto

+  // 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.


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?

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

@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.

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.

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.

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.

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:

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.)

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.

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,

MatchList item1: Match on destination port 80.

MatchList item2-1000: Match on other ports in the range of 1-10000.

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.

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.

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:

destination port

destination IP

server name

transport protocol

application protocol

connection source type

source IP

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.

markdroth · 2021-11-08T17:18:42Z

api/envoy/config/listener/v3/listener_components.proto

@@ -109,6 +109,131 @@ message FilterChainMatch {
    EXTERNAL = 2;
  }

+  // Matches filter chains by the destination port.
+  // [#next-free-field: 6]
+  message DestinationPort {


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?

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

Okay. Please make that clear in the comments.

markdroth · 2021-11-08T17:20:57Z

api/envoy/config/listener/v3/listener_components.proto

+  // 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.


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

…hain_match

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2021-11-11T19:26:50Z

I agree about the fixed order being a problem. This PR addressed that specifically plus allowing extensions for other matching behavior. I also think we should have a list of multiple matching conditions for every chain, thanks for pointing that out. I disagree about having to partition the search space explicitly at every leaf. That does not reflect the reality of it's application in service mesh. The problem is that decisions depend on a variety of factors. So the going up one level in the tree to pick the next default is mandatory. In the example I mentioned, imagine there is a submatcher for every matcher that checks, for example, ALPN. If the submatcher fails we do want to backtrack to the port selection. What the matcher API misses is ability to quickly backtrack to the next best wildcard instead of iterating over a list.

…

On Thu, Nov 11, 2021, 11:16 AM Mark D. Roth ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In api/envoy/config/listener/v3/listener_components.proto <#18871 (comment)>: > + // 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. 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18871 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIYRRRQHIRTO6DGB63GUC3ULQI75ANCNFSM5HHGUBSA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

markdroth · 2021-11-11T20:02:46Z

I disagree about having to partition the search space explicitly at every leaf. That does not reflect the reality of it's application in service mesh. The problem is that decisions depend on a variety of factors. So the going up one level in the tree to pick the next default is mandatory. In the example I mentioned, imagine there is a submatcher for every matcher that checks, for example, ALPN. If the submatcher fails we do want to backtrack to the port selection. What the matcher API misses is ability to quickly backtrack to the next best wildcard instead of iterating over a list.

I don't see any reason that the generic matcher API can't do this. It should be possible to write a sublinear matcher whose behavior is to fall back to the next-best match when the best match fails somewhere further down the tree.

I was actually just discussing this with @snowp earlier today. It turns out that the sublinear prefix matcher is actually not yet implemented, so the only sublinear matcher we have today is exact-match, for which there's no need to fall back to the next-best match. But I think that when we do implement the sublinear prefix matcher, we will probably want to fall back to the next-best match, so that MatcherTree effectively works the same way as MatcherList. (In other words, the fact that it uses a sublinear-time algorithm should not affect the logical semantics.) And I think you could do the same thing with (e.g.) an ALPN matcher.

kyessenov · 2021-11-12T02:22:23Z

OK, so I think we're on the same page about the desired semantics. I think the fallback should not be required, it's reasonable to never match wildcard if there is a more specific match. That's how current implementation works in listeners, so there's prior art.

How do we want to do the listener filter chain refactor then? My original motivation was just to add metadata matching to listeners because it's blocking extensibility there, and it has grown way out of scope to cover changes to xDS matchers. Do we need to hold on adding metadata matching to listeners until xDS matchers are improved, and how long would that take to make them default in listeners?

mattklein123 · 2021-11-12T17:43:54Z

How do we want to do the listener filter chain refactor then? My original motivation was just to add metadata matching to listeners because it's blocking extensibility there, and it has grown way out of scope to cover changes to xDS matchers. Do we need to hold on adding metadata matching to listeners until xDS matchers are improved, and how long would that take to make them default in listeners?

The problem is we keep accruing technical debt wrt filter chain matching and I would really rather not add yet more debt. How much work is it really to do it "the right way?" What changes are actually needed to the generic matching infra to support this? cc @snowp

markdroth · 2022-01-28T17:27:27Z

Matt, thanks for your insight here. I think that when I made my comments earlier, I was actually confused between selecting a listener and selecting a filter chain within a listener. I now realize that in this PR we are talking only about the latter, which does make things simpler.

My understanding of the current behavior is that if anything in a single listener changes (including filter chain matchers), Envoy currently drains all connections for that listener. I agree that we can continue to do that -- i.e., go with your option (2) -- and that that should basically preserve the existing behavior.

Thanks, and sorry for the confusion.

kyessenov · 2022-01-31T17:22:12Z

@markdroth Listener filter chain is "intelligent" in the sense that if a filter chain is added, then existing connections are not drained. Note that the filter chain includes the condition as well, which can take priority over conditions for existing connections. So what happens now is actually none of the options 1-3.

This is valuable for "eventual service discovery" systems. Services may come up gradually, and resetting all connections every time an edge event happens causes too much churn. I think it's a strong requirement to preserve connections when new chains are introduced but because of the shared matching tree, this condition is difficult to express precisely.

markdroth · 2022-01-31T18:46:21Z

Ah, okay, I didn't realize that the current behavior was more nuanced. In that case, the proposed option (2) would actually be a regression.

If we have a strong requirement to avoid disrupting existing connections unnecessarily, then I don't see any option here other than to save the info about each connection that we need to reevaluate the match later when we get a config update.

mattklein123 · 2022-01-31T19:51:06Z

If we have a strong requirement to avoid disrupting existing connections unnecessarily, then I don't see any option here other than to save the info about each connection that we need to reevaluate the match later when we get a config update.

I actually think option 1 (do nothing) is a reasonable thing to do in the first version. I don't see a problem with the matches being eventually consistent with regard to which connections are matched to which chain.

…hain_match

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2022-02-01T23:49:03Z

Updated to follow choice (1): do not re-evaluate and maintain connections when conditions change.

…hain_match

mattklein123

Thanks this LGTM modulo figuring out some docs stuff. I'm fine if you want to move to code at this point and we can review it all as a complete PR? Thanks a ton for working on this. This will be a great improvement.

/wait

mattklein123 · 2022-02-02T21:23:54Z

api/envoy/config/listener/v3/listener.proto

+  // * 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


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

Ack, pending relevant protos merged.

mattklein123 · 2022-02-02T21:26:25Z

api/envoy/config/listener/v3/listener.proto

+  //  filter chain is removed or structurally modified, then the drain for its
+  //  connections is initiated.
+  //
+  // [#not-implemented-hide:]


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.

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.

Introduce data inputs for connection matching as part of #18871 Signed-off-by: Kuat Yessenov <kuat@google.com>

…hain_match

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 · 2022-02-22T23:24:53Z

@kyessenov what's the status of this. Is this ready for review?

/wait-any

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2022-02-23T19:09:17Z

Thanks for taking a look. My update is that I started implementation, but pretty early in it. Do you think we can get this merged since the APIs are hidden for now? This is a rather long discussion to track. I verified locally that the hidden example in docs works.

mattklein123 · 2022-02-24T17:48:14Z

@kyessenov at this point this PR is pretty small. Do you want to just close it and then just reopen with the implementation? Then we can review it all together? In general this LGTM.

Add unified matcher for network streams, as a replacement for filter chain match. See previous discussion in #18871 Signed-off-by: Kuat Yessenov <kuat@google.com>

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>

[wip] Add filter chain match predicate order

6a37223

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot added the api label Nov 2, 2021

repokitteh-read-only bot assigned markdroth Nov 2, 2021

spelling

8aa165f

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov commented Nov 2, 2021

View reviewed changes

lambdai reviewed Nov 2, 2021

View reviewed changes

mattklein123 self-assigned this Nov 3, 2021

mattklein123 requested changes Nov 3, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 3, 2021

howardjohn reviewed Nov 4, 2021

View reviewed changes

kyessenov added 2 commits November 5, 2021 10:45

review

cfdb93a

Signed-off-by: Kuat Yessenov <kuat@google.com>

review

a13ed92

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Nov 5, 2021

mattklein123 requested changes Nov 8, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 8, 2021

markdroth reviewed Nov 8, 2021

View reviewed changes

kyessenov added 2 commits November 8, 2021 13:46

Merge remote-tracking branch 'upstream/main' into extensible_filter_c…

621fcbf

…hain_match

review

2228fef

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Nov 8, 2021

mattklein123 added the waiting label Nov 9, 2021

kyessenov added 2 commits February 1, 2022 15:14

Merge remote-tracking branch 'upstream/main' into extensible_filter_c…

72b978e

…hain_match

review

e981df6

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Feb 1, 2022

Merge remote-tracking branch 'upstream/main' into extensible_filter_c…

a9b056a

…hain_match

mattklein123 requested changes Feb 2, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 2, 2022

snowp pushed a commit that referenced this pull request Feb 17, 2022

matchers: add input types for network streams (#19493)

a118134

Introduce data inputs for connection matching as part of #18871 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov added 2 commits February 17, 2022 15:57

Merge remote-tracking branch 'upstream/main' into extensible_filter_c…

987a2fd

…hain_match

try validation

0185225

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Feb 18, 2022

htuch mentioned this pull request Feb 20, 2022

Listener FilterChainMatch using connection metadata #19950

Closed

repokitteh-read-only bot added the waiting:any label Feb 22, 2022

verify example

d1c8f75

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting:any label Feb 23, 2022

kyessenov closed this Feb 24, 2022

kyessenov mentioned this pull request Feb 24, 2022

listeners: add unified matcher for filter chains #20110

Merged

kyessenov mentioned this pull request Mar 14, 2022

docs: add cncf/xds protos #20277

Merged

listener: filter chain unified matchers #18871

listener: filter chain unified matchers #18871

Conversation

kyessenov commented Nov 2, 2021 • edited Loading

repokitteh-read-only bot commented Nov 2, 2021

mattklein123 commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Nov 4, 2021 • edited Loading

lambdai commented Nov 4, 2021

kyessenov commented Nov 4, 2021 • edited Loading

lambdai commented Nov 4, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Nov 11, 2021 via email

markdroth commented Nov 11, 2021

kyessenov commented Nov 12, 2021

mattklein123 commented Nov 12, 2021

markdroth commented Jan 28, 2022

kyessenov commented Jan 31, 2022

markdroth commented Jan 31, 2022

mattklein123 commented Jan 31, 2022

kyessenov commented Feb 1, 2022

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Feb 22, 2022

kyessenov commented Feb 23, 2022

kyessenov commented Nov 2, 2021 •

edited

Loading

kyessenov commented Nov 4, 2021 •

edited

Loading

kyessenov commented Nov 4, 2021 •

edited

Loading

kyessenov Nov 11, 2021 •

edited

Loading