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

fix response rate limit check condition #13811

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

Conversation

narendrapatel
Copy link

@narendrapatel narendrapatel commented Oct 30, 2024

Summary

This change fixes the kong response rate limiting plugin where the limit usage value can go in negative if there are parallel requests.
For example, if below limits are configured:
{ "limit": { "second": null, "minute": 1, "hour": null, "day": null, "month": null, "year": null } }
and we send 4-5 parallel requests then the usage counter will go in negative and skip the zero condition check and by pass the rate limits.
Similar to header_filter.lua where we check for negative value(lv.remaining <= 0), we need to do the same in access.lua.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #[issue number]

@CLAassistant
Copy link

CLAassistant commented Oct 30, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added plugins/response-ratelimiting cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Oct 30, 2024
@narendrapatel narendrapatel marked this pull request as ready for review October 30, 2024 13:44
@team-gateway-bot team-gateway-bot added the author/community PRs from the open-source community (not Kong Inc) label Oct 30, 2024
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice catch, @narendrapatel, thank you for your contribution. Would you consider adding a test case as well? Check out this test case and the test_limit function here. It will be similar to existing cases, but testing that the counter did indeed go negative, plus the 429 (ensure it fails without your fix).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author/community PRs from the open-source community (not Kong Inc) cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee plugins/response-ratelimiting size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants