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.

Sign up for GitHub

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

Jump to bottom

Scale ratelimit with ingress pods #5113

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

Scale ratelimit with ingress pods #5113

wants to merge 5 commits into from

Conversation

dbaumgarten
Copy link
Contributor

Proposed changes

Implements #4894

Add a flag (in VS-Policy and in Ingress-Annotations) to enable ratelimit-scaling. If enabled, the configured ratelimit is divided by the number of currently active ingress-pods. This way the total sum of the ratelimit stays constant, even when autoscaling changes the number of nginx-ingress pods.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@dbaumgarten dbaumgarten requested review from a team as code owners February 16, 2024 12:42
@netlify Netlify
Copy link

netlify bot commented Feb 16, 2024 •
edited

👷 Deploy request for nginx-kubernetes-ingress pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit fbd67bd

@github-actions github-actions bot added documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements labels Feb 16, 2024
ADubhlaoich
ADubhlaoich reviewed Feb 20, 2024
View reviewed changes
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM docs-wise, but most of the PR is code-based, so I am not approving.

@shaun-nx shaun-nx self-assigned this Feb 26, 2024
@brianehlert brianehlert linked an issue Mar 6, 2024 that may be closed by this pull request
Scale rate-limit with ingress-pods #4894
Open
@dbaumgarten
Copy link
Contributor Author

@shaun-nx What do you think? Does this look fine or are any changes required?

@brianehlert
Copy link
Collaborator

Just an FYI as this has been lingering for review.
The NGINX Plus version of the implementation should get accurate rate limiting across all pods with state sync.
That comes with the shared memory zone being configured for the rate limit and then state sharing being enabled.

@github-actions github-actions bot added the go Pull requests that update Go code label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ADubhlaoich ADubhlaoich ADubhlaoich left review comments

@j1m-ryan j1m-ryan j1m-ryan left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@shaun-nx shaun-nx

Labels
documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements go Pull requests that update Go code
Projects
NGINX Ingress Controller
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Scale rate-limit with ingress-pods
5 participants
@dbaumgarten @brianehlert @ADubhlaoich @j1m-ryan @shaun-nx