Support for MatcherTree Re-Entry#38564
Closed
bsurber wants to merge 2 commits into
Closed
Conversation
Contributor
Author
|
The evaluateMatch interface feels a bit awkward but it does avoid adding templating to the MaybeMatchResult struct (required if we want to add the reentrant unique_ptr from the MatchResult directly into the MaybeMatchResult). Suggestions are welcome once the PR is fully opened. |
bsurber
force-pushed
the
matcher-tree-reentry
branch
from
February 25, 2025 20:53
45ffca7 to
6a4e290
Compare
…e.g. ListMatcher. Signed-off-by: Brian Surber <[email protected]>
bsurber
force-pushed
the
matcher-tree-reentry
branch
from
February 26, 2025 21:47
6a4e290 to
6e6d681
Compare
…-entry into nested matchers. Signed-off-by: Brian Surber <[email protected]>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
ravenblackx
added a commit
that referenced
this pull request
May 28, 2025
…38726) Commit Message: - Add a `keep_matching` field to OnMatch to mark Matchers that should be recorded when matched but should not be the final matched + enforced action. - Implement support for `keep_matching` using MatcherTree re-entry. - Re-entry is implemented in PR #38564 as well for a separate review if this PR gets closed. - ListMatcher was chosen as the simplest example usecase / proof-of-concept for re-entry, but implementation in other MatcherTree children will follow in separate PRs (namely prefix_map_matcher). - Centralize recursion handling & re-entry logic into `evaluateMatch(...)`, leaving each MatchTree child class to implement the simplest possible `match(...)` logic (find a match and return it + optionally a reentrant). - Previously recursion handling was inconsistent across the MatchTree children, and this saves each child from having to re-implement recursion, `keep_handling` processing, etc. - TrieMatcher also required a reentrant implementation as part of moving recursion logic out of its `match(...)` logic. Additional Description: - The final goal is to implement preview / darklaunch Matchers whose actions are logged or added to metadata, but not enforced. - An initial implementation of preview matched actions just using re-entry was drafted for the rate_limit_quota filter in PR #38576, prompting the request to instead add this `keep_matching` feature to the Matcher API for re-usability. - This change will also depend on a duplicate, matcher proto change in [github.com/cnf/xds](cncf/xds#117). Risk Level: moderate - includes changes to widely-used libraries Testing: Unit + Integration testing, loadtesting WIP Docs Changes: Release Notes: Platform Specific Features: ~[Optional Runtime guard:]~ ~[Optional Fixes #Issue]~ ~[Optional Fixes commit #PR or SHA]~ ~[Optional Deprecated:]~ [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md): The new `keep_matching` field is not marked as WIP as its implementation should be tested for any breaking changes. --------- Signed-off-by: Brian Surber <[email protected]> Signed-off-by: Raven Black <[email protected]> Co-authored-by: Raven Black <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message:
Add support for re-entry into a MatcherTree, which can be enabled for specific classes as appropriate. This allows for continuing evaluation of an input against the remainder of a MatcherTree when both a) the MatcherTree type could feasibly match a single input to multiple matchers, and b) having additional matches is relevant to the calling filter.
Included is the implementation for ListMatcher as an example. Separate PRs will follow with implementations for other MatcherTree children classes where appropriate: e.g. PrefixMapMatcher will support re-entry but it wouldn't make sense for ExactMapMatcher.
Additional Description: An example of how this can be used can be found in a branching PR #38576, enabling a preview / darklaunch mode for matchers in the rate_limit_quota filter.
Risk Level: Moderate - changes to common library, though no changes to existing behaviors
Testing: Unit & integration testing
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]