Support rate-limiting for ingress-resources

[dbaumgarten]

Is your feature request related to a problem? Please describe.
To properly secure our applications running behind nginx-ingress we need to implement rate-limiting on source-IP basis. Nginx does already support this. However the ingress-controller has no direct support for this. You need to use snippets (in the configmap AND in the ingress-resources).
Additionally rate-limits implemented with snippets do not work well with pod-autoscaling. The rate-limits are per-pod, and if autoscaling adds more pods, it will accidentally also increase the total rate-limit.

Describe the solution you'd like
It would be great to have the rate-limiting feature available via the usual annotations. Something like:

nginx.org/request-limit: "100/s"

Additionally it would be great if the ingress controller would (optionally) scale this value by the amount of currently running ingress-pods.

Describe alternatives you've considered
Implement rate-limiting via snippets and implement a custom controller that dynamically modifies the nginx-configmap according to the current amount of running nginx-pods.

Additional context
I would like to implement this feature myself and open a PR.
But I though it would be good to first ask if you would accept such a PR at all.

[github-actions]

Hi @dbaumgarten thanks for reporting!

Be sure to check out the docs and the Contributing Guidelines while you wait for a human to take a look at this 🙂

Cheers!

[brianehlert]

Thanks @dbaumgarten
I have actually been thinking about taking the Policy resource and its relationship model and extending that to the Ingress resource. Kind of a mash-up idea between Ingress and the project CRDs and also a bit of thinking that Gateway API brings.

Your second statement about global rate limiting is actually something that we have been looking at.
Can I ask how to manage your settings today? (just to get a bit better understanding)

[dbaumgarten]

Re-using the existing policies is an interesting idea. Being able to use policies with ingress-objects would bring a lot of interesting features. (For example currently you also need to use snippets fo IP-based white/black-listing.
It does however sound like something that requires quite a bit more effort, as the mixing of CRDs and Ingress-Objects is something completely new.

Currently we simply have fixed rate-limits in place via snippets. A http-snippet in the configmap for creating the zone and location-snippets in the ingresses to apply the zone to them.
We are also using the horizontal pod autoscaler to automatically scale the nginx-ingress according to the current load.

However in a recent incident we noticed that the rate-limiting becomes less effective the more ingress-replicas are running.
Therefore we started looking for possible solutions.

The "simplest" way would be to create a seperate controller that automatically modifies the configmap accordingly.
However as this would add more moving parts that would need to be maintained, I though it couldn't hurt to ask if that is something that could be added to the nginx-ingress-controller.

Rate-limiting is quite essential for reliably running a service. So supporting this properly would probably help quite a few people.

[shaun-nx]

Hi @dbaumgarten Thank you for opening this issue! I'm going to pass this issue by the team for discussion before we suggest opening an PR.

[brianehlert]

If we can keep this simple and scoped to only the rate limit annotation and corresponding template changes, that would be great.
We have a global rate limit capability in the roadmap that will handle the more complicated bits.
And we would also handle the Policy attachment idea as separate work.

[dbaumgarten]

Is there any hint on how far down the roadmap global rate-limiting is?
Having the annotations is nice, but without working global rate-limiting it is only partially usefull.

[brianehlert]

I totally understand.
The roadmap for the proxy enabling piece (without building something using the key/value store and state sharing) is into mid next year.

My concern about a controller that does math is that there are edges that will need to tune or disable the calculation behavior.
For deployments, this is totally a no brainer and simple to understand. kube-proxy is still at play and thus randomness of distribution across pods.
For daemonset, it gets weird. Works as long as there is no loadbalancer tier session persistence or similar steering behavior of traffic that favors any node. And this is a not insignificant deployment topology.
Some of the clouds also directly now target ingress controller pod IPs through their CNIs.
My concern there is unexpected results from an automatic calculator. Not that you as a customers could not simply adjust the base number accordingly.

This is mainly why I prefer the proxy give the project the tools and I will be checking with them again around that.
We can continue to talk about this here. Happy to also have a call.

We also like to keep PRs small and constrained and would like to see the annotation as a separate PR from the controller doing calculations.
At the minimum if we could split this into at least 2 PRs, that would be great.

[dbaumgarten]

Doing this as separate PRs is absolutely the way to go here. First the basic annotations and then everything else.

I understand that there are potential issues with the auto-scaling-calculations in advanced setups.
However I suspect that the vast mahority of users simple uses a deployment with a (non-steering ) Loadbalancer.
At least for these users that would be a super simple solution.
But I agree that this should be an optional feature (probably enabled via a separate annotation?).

My suggestion would be to start with a PR that adds the basic annotations. Once that is done and accepted I follow up with a proposal for the calculation-approach.

[dbaumgarten]

So, here is my first draft: #4660
What do you think about it?

There are a few things that I am a bit unsure about:

• Naming of the annotations
• Default values
• Behaviour of the delay parameter. Currently it defaults to 0 which results in "nodelay" in the config. There is no way to have neither delay nor nodelay in the config.
• Validation of the annotation values. Should it for example check that the user provided a valid unit for the request-reate (r/s)?
• Possible additional features. Maybe in separate PRs?
  • Whitelisting of IP-Ranges
  • Zone-Sync for nginx-plus

[dbaumgarten]

Any feeback/suggestions on my open questions?

[vepatel]

Hi @dbaumgarten we'll be taking a look at the PR, thanks for your patience!