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 <[email protected]>
… support for reentry & using it to skip keep_matching matches. Signed-off-by: Brian Surber <[email protected]>
|
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 <[email protected]>
Signed-off-by: Brian Surber <[email protected]>
…k. Remove this commit before merging or once the cncf/xds matcher PR is merged. Signed-off-by: Brian Surber <[email protected]>
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 |
|
Oop, seeing an error in RouteMatcherTest.TestMatchInvalidInput from ci. Looking into it, but it's probably unrelated to this PR: https://github.com/envoyproxy/envoy/commits/main/test/common/router/config_impl_test.cc has recent updates as well that got pulled in here. |
source/common/matcher/list_matcher.h
Outdated
| } | ||
| // No match. | ||
| if (!maybe_match.result()) |
There was a problem hiding this comment.
nit: Do you really need a continue here?
It seems that the logic can be simplified as
if (maybe_match.result()) {
// Provide a reentrant ListMatcher to continue traversal from the next index.
return {MatchState::MatchComplete, matcher.second,
std::make_unique<ListMatcherReentrant<DataType>>(*this, i + 1)};
}
There was a problem hiding this comment.
Sure, it's functionally identical, updated.
Signed-off-by: Brian Surber <[email protected]>
Yes, the actual preview feature in the RLQS filter will come after this PR. |
|
Looks like it probably needs to re-merge main to fix (May also be that something is genuinely broken by updating the xds API version?) |
Signed-off-by: Brian Surber <[email protected]>
|
I did some comparisons against upstream/main. My change to config validation may be what's causing this. The expected error message in RouteMatcherTest.TestMatchInvalidInput matches the envoy exception, but the test case isn't expecting an exception at all (it expects to be able to query for the error post-processing). |
|
Yeah, it looks to be a breaking change from having the envoy exceptions surface during parsing instead of checking against the validator later with |
…ollowing existing convention of checking the validator.errors() result. Signed-off-by: Brian Surber <[email protected]>
|
/retest (Looks like all the fails are different now, so probably flakes.) |
|
/retest |
|
|
|
/coverage |
|
Coverage for this Pull Request will be rendered here: https://storage.googleapis.com/envoy-pr/38726/coverage/index.html The coverage results are (re-)rendered each time the CI |
|
Thanks, I see a couple of tests / test edits I can add around sub-matching. |
…d now that skipped matchers are handled via callback instead of recorded to a vector Signed-off-by: Brian Surber <[email protected]>
…returns no-match Signed-off-by: Brian Surber <[email protected]>
…o includes defining behavior of continued re-entry attempts against the failed matcher. Signed-off-by: Brian Surber <[email protected]>
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.