Support dynamic limit override in ratelimit filter#11770
Support dynamic limit override in ratelimit filter#11770mattklein123 merged 3 commits intoenvoyproxy:masterfrom
Conversation
|
Originally I had a I intend to fix that bug and create a followup PR to include support for generic value override, but I figured it doesn't worth blocking on it. |
26a932d to
8a7009a
Compare
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
8a7009a to
3facbae
Compare
mattklein123
left a comment
There was a problem hiding this comment.
Thanks really great work. Just a few small comments.
/wait
| // [#protodoc-title: Ratelimit Time Unit] | ||
|
|
||
| // Identifies the unit of of time for rate limit. | ||
| enum RateLimitUnit { |
There was a problem hiding this comment.
This is copied out from rls.proto, right? Can we at least put a note in that proto to use this type in the next major version? I think I would also be OK with a compile breaking (not wire) change to just use this new proto for the RLS service but will defer to @htuch on that.
There was a problem hiding this comment.
Yeah. I wanted to reuse it there initially and then realized that would not be b/c. Left a TODO comment for v4 in rls.proto
| path: | ||
| - key: test | ||
|
|
||
| Will lookup the value of the dynamic metadata, and if it contains the following structure: |
There was a problem hiding this comment.
In terms of structure can you be very clear on what the structure needs to be in terms of field names, types, etc.?
Can we also document this here? https://www.envoyproxy.io/docs/envoy/latest/configuration/advanced/well_known_dynamic_metadata and maybe cross link if appropriate?
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
35ee458 to
431bd5f
Compare
mattklein123
left a comment
There was a problem hiding this comment.
Thanks! (In the future please never force push as it breaks reviews. Just merge main and add commits.)
Signed-off-by: Petr Pchelko ppchelko@wikimedia.org
Commit Message: Support dynamic limit override in ratelimit filter
Additional Description: Provides a way to specify dynamic rate limit override in the rate limit descriptor from static value or from dynamic metadata. New type, RateLimitUnit was created to share across config protocol and rate limit service protocol. A PR for the reference implementation of the rate limit service will follow after the API changes are discussed and accepted.
Risk Level: low
Testing: unit tests
Docs Changes: Documented new proto messages. Added a section on rate limit overrides to rate_limit_filter.rst
Release Notes:
Fixes #11595