Implementable RateLimitPolicy proposal #4346
Conversation
github-actions
bot
added
the
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4346 +/- ##
=======================================
Coverage 86.10% 86.11%
=======================================
Files 132 132
Lines 14342 14342
Branches 35 35
=======================================
+ Hits 12349 12350 +1
+ Misses 1790 1789 -1
Partials 203 203 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| ### Versioning and Installation | ||
|
|
||
| The version of the `RateLimitPolicy` API will be `v1alpha1`. |
There was a problem hiding this comment.
do we always aim to alpha1 and then duplicate for alpha2?
|
|
||
| Downsides: | ||
|
|
||
| - Harder to reason about capacity of fleet, especially when auto-scaling is enabled |
There was a problem hiding this comment.
at least now i know a bit about fleets and zones in nginx but actually from the text below i'm not sure i can understand the difference. I understoon at first that zone is the area of shared memory, but below i see ratelimit is set per zone
|
|
||
| - When there is a a Route with a `RateLimitPolicy` attached that sets a rate limit zone named `zone_one` with `rate = 3r/s` and `zoneSize = 5m`, and a Gateway that also has a `RateLimitPolicy` attached that sets a rate limit zone named `zone_one` with `rate = 5/rs` and `zoneSize = 100m`, the effective policy will choose the rate limit zone settings from the Gateway. | ||
| - When there is a Route with a `RateLimitPolicy` attached that sets a rate limit rule with `zoneName = default_zone_five` and `burst=5`, and a Gateway that also has a `RateLimitPolicy` attached that sets a rate limit rule with `zoneName = default_zone_three` and `burst = 2` and `noDelay = true`, the effective policy will choose the rate limit rule settings from the HTTPRoute. | ||
| - A Route without a policy attached will inherit all settings from the Gateway's policy. |
There was a problem hiding this comment.
will be possible to set ignorance like: ratelimit: off for some route to ignore inherited as well?
| // RateLimit contains settings for Rate Limitting. | ||
| type RateLimit struct { | ||
| // Local defines the local rate limit rules for this policy. | ||
| Local *LocalRateLimit `json:"local,omitempty"` | ||
|
|
||
| // Global defines the global rate limit rules for this policy. | ||
| Global *GlobalRateLimit `json:"global,omitempty"` | ||
| } |
There was a problem hiding this comment.
Is a user required to set either one of Local or Global rate limiting?
For example, if I have a spec like this, will this be valid?
apiVersion: gateway.nginx.org/v1alpha1
kind: RateLimitPolicy
metadata:
name: example-rl-policy
namespace: default
spec:
targetRefs:
- group: gateway.networking.k8s.io
kind: Gateway
name: example-gateway
rateLimit:
# Neither local or global is set
| // ZoneName is the name of the zone. | ||
| // | ||
| // Directive: https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_zone | ||
| ZoneName *string `json:"zoneName"` |
There was a problem hiding this comment.
Is ZoneName optional?
From what I understand, using *string instead of string means it's an optional field?
I see this for a few other fields as well. Just want to make sure I understand how the API is structured.
| // RateLimitCondition represents a condition to determine if the request should be rate limited. | ||
| type RateLimitCondition struct { | ||
| // JWT defines a JWT condition to determine if the request should be rate limited. | ||
| // | ||
| // +optional | ||
| JWT *RateLimitJWTCondition `json:"jwt,omitempty"` | ||
| // Variable defines a Variable condition to determine if the request should be rate limited. | ||
| // | ||
| // +optional | ||
| Variable *RateLimitVariableCondition `json:"variable,omitempty"` | ||
| // Default sets the rate limit in this policy to be the default if no conditions are met. In a group of policies with the same condition, | ||
| // only one policy can be the default. | ||
| // | ||
| // +optional | ||
| Default *bool `json:"default,omitempty"` | ||
| } |
There was a problem hiding this comment.
Will a user be allowed to set multiple Rate Limiting conditions?
| ### JWT Claim Condition | ||
|
|
||
| JWT Claim Condition on a RateLimitPolicy would define a condition for a rate limit by JWT claim. For example, a condition could be on the claim `user_details.level` and the match could be `premium`, meaning this RateLimitPolicy would only apply to requests with a JWT claim `user_details.level` with a value `premium`. The following JWT payload would match the condition: |
There was a problem hiding this comment.
Version 5.0.0 of NIC introduced Rate Limiting with JWT Claims.
The PR for that is here: nginx/kubernetes-ingress#7175
It's very much worth studying this PR. From what I remember there were quite a few edge cases that NIC encountered when implementing this. I don't yet know if we will hit the same problems. Mostly want to bring this to everyone's attention
Proposed changes
Problem: A design is needed for the RateLimitPolicy.
Solution: Create an implementable RateLimitPolicy enhancement proposal.
Closes #4059
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.