Scale rate-limit with ingress-pods

[dbaumgarten]

Is your feature request related to a problem? Please describe.
This is a continuation of #4603 .
Now that #4660 is merged, there is support for basic rate-limiting via annotations.

The problem that remains is that the rate-limit is applied per-pod.
If there are for example three ingress-pods, it will effectively triple the amount of allowed request.
In case the number of ingress-pods is static, users could simply manually compensate for this.

However when autoscaling for the ingress-pods is enabled, this is not possible anymore.

Just yesterday we (again) had the case that we were hit by a DoS-Attack. Nginx scaled up to handle the traffic and started to block requests, but because of the scaling the blocking was not as effective as we hoped.

Describe the solution you'd like
I would suppose to add an additional annotation that enables the following behaviour:
The rate-limit provided by the user is divided by the amount of currently active ingress pods.
This way the total amount of allowed request always stays the same.

Yes, this will only work when incoming traffic is distributed evenly to all active nginx-pods. This will not always be the case (there could be session-presistence in the LB in front of nginx), but I suspect that this is the case most of the time.

Simple things should be simple and complicated things should be possible:
Users with a simple network-setting could use this annotation and everyone who needs more control could use nginx+ (see alternatives below).

If you are ok with this, I would implement this and open a PR.

Describe alternatives you've considered
a) Implement a kubernetes-controller that monitors the amount of running nginx-pods and automatically adjusts the ingress-objects.
-> Would surely work, but that is one more moving part that could break. And as the nginx-controller already has all the required information it feels weird to introduce an additional component.

b) Use nginx+ with zone sync
-> Would probably work, but as nginx+ is quite expensive it's not really a solution for everyone. However this might be the only viable option for users with non-trivial networking setups.

[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]

Given the explanation of the problem is the expectation that:

• whenever the deployment scales (number of replicas changes) the ingress controller will reconfigure to update the rate limit applied in NGINX configuration
• this will need to be processed individually for each minion (and potentially extended to VirtualServer / VirtualServerRoute)
• default that the calculation is not enabled

Something that I am not entirely sure of, can a watcher be placed on the deployment itself so that there is an event that can be responded to? It is that part I am curious about.
It would be very interesting if only a change in the replica count itself would be the trigger, but that would involve state tracking by this controller.

[dbaumgarten]

Correct,

the Ingress Controller would reconfigure the config for each Ingress that uses the feature whenever it scales. VirtualServers could also bei extended to support this.
This behaviour should be disabled by default.

My idea would be to use the endpointslice of the controller's service. The controller is already watching endpointslices. And the active endpoints of the service are what actually counts. The replica count of the deployment doesn't really matter. What counts is how many replicas are actually receiving traffic.

[dbaumgarten]

Hey @brianehlert ,

I have prepared a draft for this proposal: https://github.com/dbaumgarten/kubernetes-ingress/tree/feature/ratelimit-scaling
It is still incomplete as it misses tests and CRD-Support, but apart from that it seems to work pretty well.

The required changes were surprisingly small: dbaumgarten@4c728fa

What do you think about it?

[dbaumgarten]

Hi @brianehlert
I really donÄt want to be annyoing, but this is an important issue for us and it has been a while.
Any feedback on may draft?
Do you see any chance something like this could be merged?

[shaun-nx]

Hi @dbaumgarten
Sorry for the delay! We're happy to add this capability into the Ingress resource if you are also happy to extend this to our CRDs too (VS and VSR).

We currently enable rate limiting for VS and VSR through our Policy resource, so it would be best for this feature to be set there.

Thanks!

[dbaumgarten]

I have prepared a PR including support für VirtualServers.
Works like a charm.
However I have trouble with writing tests for my changes to controller.go. Such a testcase would need to somehow get a dummy-instance of the LoadbalancerController and I could find no existing way to do this? Did I miss anything?
Is this stuff only tested with the integration-tests?

[vepatel]

Hi @dbaumgarten, we will hopefully be reviewing this PR before end of this month, appreciate your patience on this!

[dbaumgarten]

@vepatel The month is almost over. Any news?

[j1m-ryan]

Great work on this PR! I've tested this for both VirtualServers and Ingresses, and the maths scales for changes in NIC pod configurations.

However, there are a couple of points I'd like to discuss:

• The Kubernetes load balancer won't intelligently send traffic to the NIC pods based on remaining rate limit capacity. This can lead to scenarios where, especially after scaling up nic pods, the corresponding scaled down rate limit might inadvertently block more traffic than intended. This issue exists without scaling the request limit also of course, but a feature which scales it down will exacerbate it. I acknowledge you've mentioned this in the linked issue, but it's worth highlighting as it could lead to issues from other users. To mitigate this, we should explicitly address this limitation in our documentation to set clear expectations with this feature.
• The NGINX+ solution involving zone synchronization that you mentioned appears to be an ideal fix for this issue. I would feel more confident moving forward with this PR if we have a preliminary design for the plus version, even if it's not yet implemented, since it addresses the same challenge. I do not expect you to do this however.

Thanks for your efforts on this! You are welcome to join our community call on Tuesday to discuss this further. Meeting info is the README.

[dbaumgarten]

Hi,
I am aware that this is not a perfect solution. It will only work perfectly if the loadbalancer will distribute all requests completely evenly accross the ingress-pods. In reality this will not always be the case (for example when there are long-lived TCP-connections involved that carry lots of requests). In the worst case it could over-block some requests when a client has bad-luck and constantly sends requests to the same pod.

However if the intention of the rate-limit is to block out DOS-Attacks then the ratelimit is usually high enough to allow legitimate traffic even if load is not distributed perfectly. And especially in case of DOS-Attacks it is important that you can scale to take the load without breaking your ratelimiting.

NGINX+ zone-sync feature is indeed the optimal solution to the problem as it guarantees proper ratelimits across any number of pods. If have evaluted it in the past (using snippets) and it works fine. Unfortunately NGINX+ comes at quite the price.
For many users a free solution that works "good enough" would be nice. And if they need a perfect solution they can always switch to NGINX+.

So my suggestion would be:

1. Improve to docs to inform the user that the rate-limit scaling might not work perfectly in every case
2. Add a feature for nginx-plus that enables zone-sync for the ratelimit-zones created via annotations

I will try to join the Community meeting on 2024-05-20, so we can discuss this further if required.

[dbaumgarten]

As agreed on on the community meeting: If have clarified the documentation about this feature.
I think this feature is complete now and can be merged :)