A106: xDS Unified Matcher and CEL Integration

[sergiitk]

xDS Unified Matcher and CEL Integration

Rendered version: https://github.com/sergiitk/grfc-proposal/blob/a106-xds-unified-matcher-and-cel/A106-xds-unified-matcher-and-cel.md

[sergiitk]

Split from #414

[ejona86]

It is not at all clear how the validator knows if the field is supported. The proto description doesn't really say anything either.

[markdroth]

This should be handled the same way as the set of allowed actions: it's basically determined by the calling filter when it calls the code to parse the matcher tree from the xDS proto. For example, RLQS might decide that it supports keep_matching, but the composite filter might decide that it doesn't. So if we get a matcher with keep_matching in the RLQS filter config, that's fine, but if we get a matcher with keep_matching in the composite filter config, that would generate a validation error.

The gRFC should use the same language to describe how this works for the set of allowed actions and for keep_matching, since they are both handled the same way.

[ejona86]

How does the caller know that the Action is keep_matching=true and it should "behave as if the Matcher did not match"?

I could understand "only consider the last Action as being the one to take," but if there is no on_no_match, then the last action could be for this keep_matching=true case. the keep_matching protobuf doesn't say what happens either if there is no terminal matcher. Does the entire match fail and the action here discarded?

[ejona86]

There should not be any "added" idea here. Actions for keep_matching shouldn't mix with the actual selected action. Envoy uses a callback to learn about keep_matching=true actions. We should say that it is communicated separately in the gRFC.

[markdroth]

The way we implemented this in C-core, the matcher returns a list of matching actions and a bool indicating whether the match succeeded. When we get a matching action, we append it to the list of matching actions to return. If keep_matching is false (the default), then we stop searching at this point and return true to indicate that we found a match. However, if keep_matching is true, then we keep going as if we didn't find a match. If we get to the end without finding any match (not even an on_no_match), then we return false.

If the matcher returns true, then we know that the last entry in the list of returned actions is the one we should actually take, and any earlier entries in the list are the ones that matched but had keep_matching set to true. If the match returns false, then all actions in the list are the ones that matched but had keep_matching set to true.

If a given implementation wants to use a callback API instead, that's fine too. But it would need to pass a bool to the callback indicating whether it's the final match or whether it's a keep_matching match.

[markdroth]

Thanks for writing this up, Sergii! Sorry it took me so long to do a review pass.

The content is very good. Most of my comments are about readability and clarify of edge cases.

Please let me know if you have any questions. Thanks!

[markdroth]

This should be handled the same way as the set of allowed actions: it's basically determined by the calling filter when it calls the code to parse the matcher tree from the xDS proto. For example, RLQS might decide that it supports keep_matching, but the composite filter might decide that it doesn't. So if we get a matcher with keep_matching in the RLQS filter config, that's fine, but if we get a matcher with keep_matching in the composite filter config, that would generate a validation error.

The gRFC should use the same language to describe how this works for the set of allowed actions and for keep_matching, since they are both handled the same way.

[markdroth]

The way we implemented this in C-core, the matcher returns a list of matching actions and a bool indicating whether the match succeeded. When we get a matching action, we append it to the list of matching actions to return. If keep_matching is false (the default), then we stop searching at this point and return true to indicate that we found a match. However, if keep_matching is true, then we keep going as if we didn't find a match. If we get to the end without finding any match (not even an on_no_match), then we return false.

If the matcher returns true, then we know that the last entry in the list of returned actions is the one we should actually take, and any earlier entries in the list are the ones that matched but had keep_matching set to true. If the match returns false, then all actions in the list are the ones that matched but had keep_matching set to true.

If a given implementation wants to use a callback API instead, that's fine too. But it would need to pass a bool to the callback indicating whether it's the final match or whether it's a keep_matching match.

[markdroth]

It cannot NACK the xDS resource on an unsuccessful match, because the actual matching happens only after the xDS resource has already been validated.

More generally, though, I'm not sure we actually need this statement at all, since this doesn't come into play until the matcher returns. At that point, the logic is not part of the matcher; it's part of the component that is calling the matcher.

[markdroth]

There has historically been some confusion about the exact matching semantics here, so I think it's worth describing the intended behavior here, even if it duplicates the description in the proto file. For this field, I would say something like this:

If the matcher does not return a successful match, then the context in which this OnMatch is evaluated is considered not to have matched. Examples:

• In a matcher list, if a predicate matches the input but has an OnMatch containing a nested matcher that does not return a successful match, the matcher list will treat this as if the predicate did not match the input (i.e., it will continue on to the next predicate).
• In a prefix match map, if the longest matching prefix that matches the input has an OnMatch containing a nested matcher that does not return a successful match, the prefix match map will treat this as if the prefix did not match the input (i.e., it will look for a shorter prefix that matches the input).
• In an exact match map, if the prefix that matches the input has an OnMatch containing a nested matcher that does not return a successful match, the exact match map will treat the match as unsuccessful. (In this case, there is no need to continue searching, because there cannot possibly be another match.)

Note that if the context in which the OnMatch is evaluated is considered not to have matched, then on_no_match will be used if present.

[markdroth]

The input extensions should not need to be provided by the individual caller. There should be a central registry of all supported input extensions, and any of them should be usable in any matcher that provides a compatible context.

Instead, the caller needs to provide the matcher context type to the parser, so that it can reject the resource if the matcher specifies an input extension that is not compatible with the context that the caller will use for actually evaluating the matcher. For example, an input extension that gets a header value from an RPC will only be able to get that value from an RPC matcher context; if the caller is instead using (e.g.) a match context for TCP connections, then it's invalid to use that input extension.

[markdroth]

The fact that it's the correct input type for this matcher doesn't matter during evaluation. That should be checked at resource validation time.

[markdroth]

These examples are really useful. I think we should also include some examples that show the edge cases I desribed above (e.g., a nested matcher returning unsuccessful, how that and keep_matching affect use of on_no_match, and how on_no_match influences the final result of the matcher).

[markdroth]

As above, the "CEL:" prefix on all the sub-headings seems redundant.

[markdroth]

This statement reads like we will support everything documented for Envoy. The subsections below clarify this, but to avoid confusion, I suggest changing the wording here to make it clear that we will support only certain subsets of what Envoy provides.

[markdroth]

I think this last statement doesn't really belong in the "Implementation" section. Instead, let's move it up to the "Rationale" section under a sub-heading called "Using Unified Matchers in Other Parts of xDS".

Note that RBAC is only one of many places where the unified matcher API can be used. As I mentioned above, it can also be used for L4 filter chain selection in the gRPC server, and it can be used for route selection. We can say that we are not going to proactively add support for unified matchers in any of those places, but we will structure the code in a reusable way so that we can easily add it in more places in the future if needed.