doc update for limit-rps/limit-rpm rate limiting not being a 1:1 mapping from ingress-nginx to kgateway#808
Open
wickedlydev wants to merge 6 commits into
Open
doc update for limit-rps/limit-rpm rate limiting not being a 1:1 mapping from ingress-nginx to kgateway#808wickedlydev wants to merge 6 commits into
wickedlydev wants to merge 6 commits into
Conversation
…kgateway Signed-off-by: dev34250 <dev34250@gmail.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds documentation callouts clarifying that Ingress-NGINX limit-rps/limit-rpm rate limiting semantics don’t map 1:1 to kgateway local rate limiting, and points users to global rate limiting for per-client-IP behavior.
Changes:
- Added informational callouts in the Ingress-NGINX provider docs describing per-client-IP vs per-pod behavior.
- Added the same clarification to the rate-limiting migration example.
- Added a cross-reference callout in the kgateway emitter docs pointing to the example.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| content/docs/envoy/main/migrate/providers/ingressnginx/_index.md | Adds callout warning about non-1:1 mapping and suggests global RL for per-client-IP |
| content/docs/envoy/main/migrate/examples/rate-limiting.md | Adds callout clarifying behavioral differences between Ingress-NGINX and kgateway local RL |
| content/docs/envoy/main/migrate/emitters/kgateway/_index.md | Adds callout linking to rate limiting migration example |
| content/docs/envoy/latest/migrate/providers/ingressnginx/_index.md | Same callout as main for latest docs |
| content/docs/envoy/latest/migrate/examples/rate-limiting.md | Same callout as main example for latest docs |
| content/docs/envoy/latest/migrate/emitters/kgateway/_index.md | Same callout link as main for latest docs |
| content/docs/envoy/2.2.x/migrate/providers/ingressnginx/_index.md | Same callout as main for 2.2.x docs |
| content/docs/envoy/2.2.x/migrate/examples/rate-limiting.md | Same callout as main example for 2.2.x docs |
| content/docs/envoy/2.2.x/migrate/emitters/kgateway/_index.md | Same callout link as main for 2.2.x docs |
| content/docs/envoy/2.1.x/migrate/providers/ingressnginx/_index.md | Same callout as main for 2.1.x docs |
| content/docs/envoy/2.1.x/migrate/examples/rate-limiting.md | Same callout as main example for 2.1.x docs |
| content/docs/envoy/2.1.x/migrate/emitters/kgateway/_index.md | Same callout link as main for 2.1.x docs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
artberger
reviewed
Jun 2, 2026
artberger
left a comment
artberger
left a comment
Contributor
There was a problem hiding this comment.
Thanks for opening this, I left some rephrasing suggestions.
Comment on lines
+96
to
+98
| {{< callout type="info" >}} | ||
| **Not an exact 1:1 mapping for `limit-rps`/`limit-rpm`.** See the [rate limiting migration example](../../examples/rate-limiting/) for details. | ||
| {{< /callout >}} |
Contributor
There was a problem hiding this comment.
We have descriptions formatted as colons after other annotations, so I would use the same style here.
e.g.,
- `nginx.ingress.kubernetes.io/limit-rpm`: Not an exact 1:1 mapping for `limit-rpm`. For more information, see the [rate limiting migration example](../../examples/rate-limiting/).
Contributor
Author
There was a problem hiding this comment.
fixed it to be a description of the annotation.
| NGINX's `limit-rps` and `limit-rpm` annotations map to a kgateway `TrafficPolicy` with local rate limiting. | ||
|
|
||
| {{< callout type="info" >}} | ||
| **Not an exact 1:1 mapping.** Ingress-NGINX's `limit-rps`/`limit-rpm` use a shared memory zone keyed by client IP, applying the rate limit **per client IP address, per pod**. The kgateway local token bucket is shared across clients within the scope where the policy is attached (for example, a route or virtual host), rather than per client IP. If you need per-client-IP rate limiting, prefer global rate limiting with a `RemoteAddress` descriptor entry (keys on the effective downstream address). If you must use a `Header` descriptor with `X-Forwarded-For`, only do so when XFF is validated via trusted-proxy or trusted-hops configuration and normalized to the intended client IP (for example, the leftmost untrusted hop); otherwise clients can spoof or rotate the header and it may include multiple IPs. |
Contributor
There was a problem hiding this comment.
It seems like there might be a couple different things in this one note, so maybe reformat this to something like:
## Mapping considerations
Note that the Ingress-NGINX annotation to kgateway TrafficPolicy mapping is not 1:1. The `limit-rps`/`limit-rpm` use a shared memory zone keyed by client IP, applying the rate limit **per client IP address, per pod**. In contrast, the kgateway local token bucket is shared across clients within the scope where the policy is attached (for example, a route or virtual host), rather than per client IP address.
- Per-client IP address: If you need per-client-IP rate limiting, use global rate limiting with a `RemoteAddress` descriptor entry (keys on the effective downstream address).
- Forwarded headers: If you need a `Header` descriptor with `X-Forwarded-For`, only do so when XFF is validated via trusted-proxy or trusted-hops configuration and normalized to the intended client IP address (for example, the leftmost untrusted hop). Otherwise, clients can spoof or rotate the header and it might include multiple IP addreses.
Contributor
Author
There was a problem hiding this comment.
thank you sir for pointing that out.
initially the note was just 2 lines, but copilot review suggestions made me add lines to it and i forgot that it could've been written better in a point-wise format.
Signed-off-by: dev34250 <dev34250@gmail.com>
kristin-kronstain-brown
left a comment
kristin-kronstain-brown
left a comment
Contributor
There was a problem hiding this comment.
Left a couple typo corrections.
Contributor
Author
|
@kristin-kronstain-brown thanks for helping out! fixed the typos |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The migration docs for ingress-nginx annotations suggest that limit-rps and limit-rpm map directly to kgateway's local token bucket rate limiting, but it differs significantly.
ingress-nginx applies limits per client IP address, per pod using a shared memory zone, whereas kgateway's local token bucket applies to all requests reaching the Envoy pod regardless of client IP.
Therefore, added
{{< callout type="info" >}}notices across all four doc versions (2.1.x, 2.2.x, latest, main) in files:fixes kgateway-dev/kgateway#14137
Change Type
Changelog