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

Make default hitsAddend minimum value configurable #729

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gcalmettes
Copy link

@gcalmettes gcalmettes commented Oct 9, 2024

fix: #730

In the current implementation of the ratelimit service, the hitsAddend value that is associated with each request is set to a minimum of 1.
This is done to ensure that if the RateLimit request does not specify any request.hitsAddend (like what the envoy ratelimit filter does by default if no envoy.ratelimit.hits_addend has been defined), the hit is still counted.

This minimum hitsAddend of 1 is enforced via a Max(1, <hitsAddend from request>) function.

This PR adds the possibility to configure the minimum value of the hitsAddend via a settings, so 0 can also be used as minimum hitsAddend value if needed. The current behavior of 1 being the default value is preserved, and if the environment variable is not set, then a value of 1 is applied, like currently.

The ability to configure a minimum hitsAddend to zero can help for example for the following use cases:

@gcalmettes gcalmettes force-pushed the feat/configurable-base-hitsaddend branch 2 times, most recently from c53c751 to cf05af4 Compare October 9, 2024 19:49
Signed-off-by: Guillaume Calmettes <[email protected]>
@gcalmettes
Copy link
Author

gcalmettes commented Oct 10, 2024

@envoyproxy/ratelimit-maintainers

@zirain I tried to ping @envoyproxy/ratelimit-maintainers like described in the CONTRIBUTING.md document but it seems that the handle is not available. Any chance you would know who I could ping for this PR ? Thanks a lot.

@zirain
Copy link
Contributor

zirain commented Oct 10, 2024

I think @mattklein123 is the person, hope he have some free slot to review.

@gcalmettes
Copy link
Author

Any chance you could take a look @mattklein123 ? Thanks a lot.

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

Successfully merging this pull request may close these issues.

Make the default hitsAddend configurable
2 participants