Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to set the hits_addend for a given rate_limit request via a hardcoded dynamic metadata field #34184

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

EItanya
Copy link
Contributor

@EItanya EItanya commented May 16, 2024

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

Copy link

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: #34184 was opened by EItanya.

see: more, trace.

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
@EItanya
Copy link
Contributor Author

EItanya commented May 16, 2024

The changes to ratelimit.cc are meant mostly as a conversation starter. I originally debated adding a MetadataKey to the Route spec to be able to generically specify a metadata key, but that felt overkill for such a small feature so I decided to start here. I can also of course make envoy.ratelimit a constant a la envoy.lb, but I wasn't sure of the appetite for that.

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>
@EItanya EItanya marked this pull request as ready for review May 16, 2024 02:29
@mattklein123
Copy link
Member

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

@EItanya
Copy link
Contributor Author

EItanya commented May 20, 2024

Ok, I'll add the documentation, are there any code changes you'd like me to make while I'm at it?

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #34184 was synchronize by EItanya.

see: more, trace.

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
…addend_dynamic_metadata

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
@jmarantz
Copy link
Contributor

/retest

@jmarantz
Copy link
Contributor

format failure

/wait

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

Successfully merging this pull request may close these issues.

None yet

3 participants