Matcher + MatcherTree support for a keep_matching field in OnMatch #38726
Conversation
…on should be recorded but whose enforcement should be skipped. Signed-off-by: Brian Surber <bsurber@google.com>
… support for reentry & using it to skip keep_matching matches. Signed-off-by: Brian Surber <bsurber@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
CC'ing @markroth as the original shepherd to request the 1st class feature support in OnMatch |
repokitteh-read-only
bot
added
the
deps
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
… a few more incomplete initializations in tests Signed-off-by: Brian Surber <bsurber@google.com>
Signed-off-by: Brian Surber <bsurber@google.com>
…k. Remove this commit before merging or once the cncf/xds matcher PR is merged. Signed-off-by: Brian Surber <bsurber@google.com>
bsurber
force-pushed
the
keep-matching-matchers
branch
from
d1a3596 to
296ce9b
Compare
|
@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. |
|
at tyxia's request, reassigning to wpcode |
|
/assign wbpcode |
|
bsurber is not allowed to assign users. |
|
/assign @wbpcode |
|
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 |
Signed-off-by: Brian Surber <bsurber@google.com>
|
May need to merge main to pick up some things that should fix the compile_time_options CI. |
Signed-off-by: Brian Surber <bsurber@google.com>
Signed-off-by: Brian Surber <bsurber@google.com>
Signed-off-by: Brian Surber <bsurber@google.com>
|
/retest |
There was a problem hiding this comment.
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.
Signed-off-by: Raven Black <ravenblack@dropbox.com> Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/lgtm api |
|
/retest |
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 <ravenblack@dropbox.com>
|
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. |
|
@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) |
|
@keithmattix I think it's likely that the cleanup PR will fix this case, because |
|
Yep, I'm making a build right now. Will report back to #39673 with my findings |
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 <ravenblack@dropbox.com>
Commit Message:
keep_matchingfield to OnMatch to mark Matchers that should be recorded when matched but should not be the final matched + enforced action.keep_matchingusing MatcherTree re-entry.evaluateMatch(...), leaving each MatchTree child class to implement the simplest possiblematch(...)logic (find a match and return it + optionally a reentrant).keep_handlingprocessing, etc.match(...)logic.Additional Description:
keep_matchingfeature to the Matcher API for re-usability.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_matchingfield is not marked as WIP as its implementation should be tested for any breaking changes.