Add the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field #34184
Conversation
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
EItanya
force-pushed
the
hits_addend_dynamic_metadata
branch
from
a2d61d8
to
673a527
Compare
|
The changes to I'm also not sure exactly where the docs should live for this feature. I definitely think that updating the release notes make sense and I can do that, but it could definitely also make sense to add it to the proper docs. |
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
|
At a high level this seems fine to me. For documentation I would add to the filter specific documentation as well as the release notes. /wait |
|
Ok, I'll add the documentation, are there any code changes you'd like me to make while I'm at it? |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
…addend_dynamic_metadata Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
|
/retest |
|
format failure /wait |
|
@jmarantz is there a good way to ensure all of my tools are up to date? When I run formatting it comes up with no diff. Or potentially does the CI job have the diff I need to apply? |
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
| @@ -66,11 +66,21 @@ void Filter::initiateCall(const Http::RequestHeaderMap& headers) { | |||
| break; | |||
| } | |||
|
|
|||
| double hits_addend = 0; | |||
There was a problem hiding this comment.
nit: spelling (hits_addend)
There was a problem hiding this comment.
This is meant to match the proto field https://github.com/envoyproxy/envoy/pull/34184/files#diff-0bf172f71332e6dc84ac2a45c182b8aa7329e5c213b75879fb472e5f813464b6R54.
uint32 hits_addend = 3;
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
| // This value can be modified by setting "envoy.ratelimit:hits_addend" in the dynamic metadata | ||
| // for a given request. |
There was a problem hiding this comment.
In most of cases, we prefer do similar things in the FilterState rather than dynamic metadata (like various sock options in the filter state and other dynamic configuration from filter state)? Is there any reason prefer the dynamic metadata?
There was a problem hiding this comment.
I based this off of envoy_lb style decisions which can be made via DynamicMetadata. I don't have super strong feelings either way, but there are more easier ways to set DynamicMetadata via other filters so I slightly prefer this method.
There was a problem hiding this comment.
envoy.lb use the metadata because the subset lb construct the match structure from the endpoint metadata.
Then, in this scenario, I think filter state should be preferred for these typed options.
There was a problem hiding this comment.
in this case we use dynamic metadata as it allows to feed data in without the source filter being aware that it is for rate limit.
For example, we can use ext-auth server to add the metadata for the rate limit filter.
Commit Message: Adds the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field: envoy.ratelimit:hits_addend.
Additional Description:
Risk Level: Low
Testing: Added unit test. I have also manually tested this using gloo-edge as the control-plane.
Docs Changes:
Release Notes:
Platform Specific Features: N/A
[Optional Runtime guard:] N/A
[Optional Fixes #Issue] N/A
[Optional Fixes commit #PR or SHA] N/A
[Optional Deprecated:] N/A
[Optional API Considerations:] N/A