Skip to content

Matcher + MatcherTree support for a keep_matching field in OnMatch#38726

Merged
ravenblackx merged 35 commits into
envoyproxy:mainfrom
bsurber:keep-matching-matchers
May 28, 2025
Merged

Matcher + MatcherTree support for a keep_matching field in OnMatch#38726
ravenblackx merged 35 commits into
envoyproxy:mainfrom
bsurber:keep-matching-matchers

Conversation

@bsurber

@bsurber bsurber commented Mar 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 Support for MatcherTree Re-Entry #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 rate_limit_quota: preview mode for matchers #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.

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: The new keep_matching field is not marked as WIP as its implementation should be tested for any breaking changes.

bsurber added 2 commits March 12, 2025 20:07
@bsurber
Add a new keep_matching field to OnMatch to mark a Matcher whose acti…
07a8bb3 
…on should be recorded but whose enforcement should be skipped.

Signed-off-by: Brian Surber <[email protected]>
@bsurber
Update Envoy's MatchTree interface to support keep_matching by adding…
7d66365 
… support for reentry & using it to skip keep_matching matches.

Signed-off-by: Brian Surber <[email protected]>
@repokitteh-read-only

repokitteh-read-only Bot commented Mar 12, 2025

Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38726 was opened by bsurber.

see: more, trace.

@repokitteh-read-only

repokitteh-read-only Bot commented Mar 12, 2025

Copy link
Copy Markdown

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

🐱

Caused by: #38726 was opened by bsurber.

see: more, trace.

@bsurber

bsurber commented Mar 12, 2025

Copy link
Copy Markdown
Contributor Author

CC'ing @markroth as the original shepherd to request the 1st class feature support in OnMatch

@repokitteh-read-only repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Mar 13, 2025
@repokitteh-read-only

repokitteh-read-only Bot commented Mar 13, 2025

Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #38726 was synchronize by bsurber.

see: more, trace.

bsurber added 3 commits March 14, 2025 20:32
@bsurber
Properly propagate keep_matching in OnMatch factory callbacks & catch…
f973da6 
… a few more incomplete initializations in tests

Signed-off-by: Brian Surber <[email protected]>
@bsurber
Formatting fixes
4ec98f3 
Signed-off-by: Brian Surber <[email protected]>
@bsurber
DONOTSUBMIT: temporarily use the matcher proto change from my xds for…
296ce9b 
…k. Remove this commit before merging or once the cncf/xds matcher PR is merged.

Signed-off-by: Brian Surber <[email protected]>
@bsurber
bsurber force-pushed the keep-matching-matchers branch from d1a3596 to 296ce9b Compare March 14, 2025 20:35
@bsurber
bsurber marked this pull request as ready for review March 17, 2025 18:34
@tyxia

tyxia commented Mar 19, 2025

Copy link
Copy Markdown
Member

@bsurber Please fix the CI failures

@bsurber

bsurber commented Mar 19, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@bsurber Please fix the CI failures

The deps failures are likely to be expected while I have 296ce9b in the branch, but it's needed for testing to pass while waiting for cncf/xds#117 to be merged.

Please review regardless of the dep failures. They'll intentionally block a merge here until that xds PR is reviewed+merged & I can remove the do-not-submit commit.

@bsurber

bsurber commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

at tyxia's request, reassigning to wpcode

@bsurber

bsurber commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

/assign wbpcode

@repokitteh-read-only

repokitteh-read-only Bot commented Mar 24, 2025

Copy link
Copy Markdown

bsurber is not allowed to assign users.

🐱

Caused by: a #38726 (comment) was created by @bsurber.

see: more, trace.

@tyxia

tyxia commented Mar 24, 2025

Copy link
Copy Markdown
Member

/assign @wbpcode

@tyxia tyxia removed their assignment Mar 24, 2025
@wbpcode

wbpcode commented Mar 26, 2025

Copy link
Copy Markdown
Member

I actually didn't get the usage of the new field. If you don't want to enforce the action and only want to log the match result, I think you can use a log action? matcher tree is pretty hard to maintain/follow, let us be cautious to add new feature/field.

/wait-any

ravenblackx
ravenblackx reviewed May 16, 2025
View reviewed changes
Comment thread envoy/matcher/matcher.h Outdated
ravenblackx
ravenblackx reviewed May 16, 2025
View reviewed changes
Comment thread source/common/matcher/matcher.h Outdated
ravenblackx
ravenblackx reviewed May 16, 2025
View reviewed changes
Comment thread source/common/matcher/matcher.h Outdated
@bsurber
minor fixes
a3e01f0 
Signed-off-by: Brian Surber <[email protected]>
@ravenblackx

ravenblackx commented May 19, 2025

Copy link
Copy Markdown
Contributor

May need to merge main to pick up some things that should fix the compile_time_options CI.

wbpcode
wbpcode reviewed May 22, 2025
View reviewed changes

@wbpcode wbpcode left a comment

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.

Thanks for this refactoring. Now, it's much easier to read and understand. Some minor comments are added and LGTM overall.

Comment thread envoy/matcher/matcher.h Outdated
Comment thread envoy/matcher/matcher.h Outdated
@ravenblackx ravenblackx mentioned this pull request May 22, 2025
Merged
@ravenblackx

ravenblackx commented May 23, 2025

Copy link
Copy Markdown
Contributor

/retest

@ravenblackx
ravenblackx requested a review from wbpcode May 23, 2025 14:12
wbpcode
wbpcode reviewed May 28, 2025
View reviewed changes

@wbpcode wbpcode left a comment

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.

LGTM to the code. cc @ravenblackx you could treat me have given a LGTM to this PR if there is no big change to the code. :)

But the @markdroth have updated a comment to the API comment. May check it and to see whether it's necessary to address it or not.

@ravenblackx
Update API comment per markdroth
acb80ac 
Signed-off-by: Raven Black <[email protected]>

Signed-off-by: Raven Black <[email protected]>
@markdroth

markdroth commented May 28, 2025

Copy link
Copy Markdown
Contributor

/lgtm api

@ravenblackx

ravenblackx commented May 28, 2025

Copy link
Copy Markdown
Contributor

/retest

@ravenblackx
ravenblackx merged commit 426cd86 into envoyproxy:main May 28, 2025
ravenblackx added a commit that referenced this pull request May 29, 2025
@ravenblackx
Improve matcher tests (#39603)
0ece2dc 
Commit Message: Improve matcher tests
Additional Description: This is how gtests are supposed to work.

NOTE: please do not merge until
#38726 is in, to avoid provoking
a merge conflict on a PR that's already been embattled.

Before:
```
./test/common/matcher/test_utility.h:290: Failure
Expected equality of these values:
  result.on_match_->action_cb_().get()->getTyped<StringAction>()
    Which is: 40-byte object <80-BA 85-24 1A-5C 00-00 58-97 EB-3F B2-72 00-00 08-00 00-00 00-00 00-00 6E-6F 5F-6D 61-74 63-68 00-00 00-00 00-00 00-00>
  *stringValue(expected_value)
    Which is: 40-byte object <80-BA 85-24 1A-5C 00-00 18-97 EB-3F B2-72 00-00 0A-00 00-00 00-00 00-00 78-78 6E-6F 5F-6D 61-74 63-68 00-00 00-00 00-00>
```

After:
```
test/common/matcher/prefix_map_matcher_test.cc:44: Failure
Value of: result
Expected: has string action (matcher: "xxno_match")
  Actual: {match_state_=MatchComplete, on_match_={action_cb_={string_value="no_match"}}}
```

(Note, both failures actually are on line 44 of prefix_map_matcher_test,
but only the "after" version says so.)

Risk Level: None, test-only.
Testing: Yes it is.
Docs Changes: n/a
Release Notes: n/a (unless we need a release note in case someone was
using these test helpers in an external extension?)
Platform Specific Features: n/a

---------

Signed-off-by: Raven Black <[email protected]>
@ravenblackx ravenblackx mentioned this pull request May 29, 2025
Merged
@keithmattix

keithmattix commented May 31, 2025

Copy link
Copy Markdown
Contributor

I think this may have broken some of our matcher tree usage in Istio. I'll look into this and follow #39673 to confirm.

@ravenblackx

ravenblackx commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

@keithmattix

I think this may have broken some of our matcher tree usage in Istio. I'll look into this and follow #39673 to confirm.

Please also test between this and #38986 which could be responsible [or might fix it, or you might be able to flip the runtime flag to get the old behavior if that's the responsible one], and after #39673 which also may or may not fix the difference.

More details of what's broken would be useful so we could mitigate, if it's not already fixed.

@keithmattix

keithmattix commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

@ravenblackx thanks for the pointer; we're not using prefix matchers, so I don't think #38986 is the culprit. We are using on_no_match though, so I suspect it has to do with this effort generally. We caught this in our e2es on master; the config didn't change, just the behavior - the request now errors instead of succeeds. I'll have to repro the tests with debug logs on to get more details (whcih I can do later this week)
image

@ravenblackx

ravenblackx commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

@keithmattix I think it's likely that the cleanup PR will fix this case, because exact_match_map was the place where the internal recursion implementation seemed not quite right when I was cleaning it up. If you're investigating I would recommend patching that one in to see if it fixes it (unless it's merged before you get to it anyway in which case you can try it more easily!)

@keithmattix

keithmattix commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

Yep, I'm making a build right now. Will report back to #39673 with my findings

ravenblackx added a commit that referenced this pull request Jun 6, 2025
@ravenblackx
Clean up Matcher::MatchResult's unnecessary templates (#39673)
2fddebb 
Commit Message: Clean up Matcher::MatchResult's unnecessary templates
Additional Description: Since #38726 refactored Matcher impls to recurse
inside the match function, there is no longer any need for MatchResults
to have the capacity to return sub-match-trees, which means they also
don't need to be templates. Cleaning this up makes the code much easier
to follow. This may have also caught a few locations where the internal
recursion wasn't fully updated correctly (minor negative, it does make
one `doMatch()` implementation slightly more complicated, but the
corresponding `match()` implementation is made much simpler, so still an
improvement).
Risk Level: Small, existing tests should be ensuring that behaviors were
unchanged.
Testing: Covered by existing tests, which are also cleaner.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

---------

Signed-off-by: Raven Black <[email protected]>
@bsurber bsurber mentioned this pull request Jun 10, 2025
Closed
@bsurber bsurber mentioned this pull request Sep 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ravenblackx ravenblackx ravenblackx left review comments
@wbpcode wbpcode wbpcode approved these changes
@tyxia tyxia tyxia left review comments
@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner
@botengyao botengyao Awaiting requested review from botengyao botengyao is a code owner
@yanavlasov yanavlasov Awaiting requested review from yanavlasov yanavlasov is a code owner
+1 more reviewer
@markdroth markdroth markdroth left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@wbpcode wbpcode

@markdroth markdroth

@tyxia tyxia

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@bsurber @tyxia @wbpcode @markdroth @phlax @ravenblackx @keithmattix @mattklein123