Skip to content

rate_limit_quota: preview mode for matchers#38576

Closed
bsurber wants to merge 3 commits into
envoyproxy:mainfrom
bsurber:rlqs-preview-rules-with-reentry
Closed

rate_limit_quota: preview mode for matchers#38576
bsurber wants to merge 3 commits into
envoyproxy:mainfrom
bsurber:rlqs-preview-rules-with-reentry

Conversation

@bsurber

@bsurber bsurber commented Feb 26, 2025

Copy link
Copy Markdown
Contributor

Commit Message:

  • Preview matchers are provided to enable testing how a config would have treated incoming traffic if the match would have been enforced.
    • If matcher evaluation lands on a preview matcher, the matched bucket is processed as normal (bucket creation, usage aggregation & reporting, etc), but the allow/deny decision is only ever logged, not enforced.
      • After this, the filter re-enters the matcher tree, matching until it lands on an enforceable matcher (subsequent preview matchers are ignored, even if matched).
    • If evaluation lands on an enforceable matcher first, then there is no need for re-entry.
  • If re-entry isn't supported for the given MatcherTree type, then the filter will stop after the first evaluation.

Additional Description: Branched from PR #38564 which provides the necessary re-entry interface for the MatcherTree and an implementation for ListMatcher
Risk Level: minor - all limited to in-development rate_limit_quota filter
Testing: Unit and integration
Docs Changes:
Release Notes:
Platform Specific Features:

@repokitteh-read-only

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: #38576 was opened by bsurber.

see: more, trace.

@repokitteh-read-only

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 @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #38576 was opened by bsurber.

see: more, trace.

…-entry into nested matchers.

Signed-off-by: Brian Surber <[email protected]>
…eir rate limiting decisions.

Signed-off-by: Brian Surber <[email protected]>
@github-actions

Copy link
Copy Markdown

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 github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 31, 2025
@github-actions

github-actions Bot commented Apr 7, 2025

Copy link
Copy Markdown

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!

@github-actions github-actions Bot closed this Apr 7, 2025
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]>
@bsurber

bsurber commented Jun 10, 2025

Copy link
Copy Markdown
Contributor Author

Note: #38726 has the final keep_matching implementation, and an implementation of preview mode for the rate_limit_quota filter will look different enough to warrant a new branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api stale stalebot believes this issue/PR has not been touched recently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants