Conversation
A global rate limit rule that specified sourceCIDR in more than one clientSelector silently kept only the last one. translateRateLimitRule loops over rule.ClientSelectors and appends header, method and query parameter matches to slices on the IR rule, but ir.RateLimitRule.CIDRMatch is a single pointer, so each selector overwrote the previous one. Nothing surfaced the loss: no error, no status condition, and the policy still reported Accepted: True. That made a rule written as "limit each client individually, except this CIDR" enforce only half of itself, which is the case in the issue: a Distinct selector followed by an inverted one kept only the inversion. Reject the configuration rather than silently dropping part of it. Supporting several CIDRs per rule is a larger change than it looks, because each one becomes a masked_remote_address entry in the rate limit service descriptor chain and the nesting order carries meaning, so that is left for a follow-up if maintainers want it. Failing closed at least makes the limitation visible instead of producing a policy that quietly enforces something other than what it says. Fixes envoyproxy#10025 Test Plan: ``` go test ./internal/gatewayapi/... ./internal/xds/translator/... ./internal/ir/... ./api/... ``` All nine packages pass. Added the backendtrafficpolicy-with-ratelimit-invalid-multiple-source-cidr fixture, which renders the policy with Accepted: False, reason: Invalid message: RateLimit: unable to translate rateLimit: only one clientSelector per rule may specify sourceCIDR Existing fixtures that use a single sourceCIDR are unchanged. This change was authored with the assistance of Claude, an AI assistant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: vishals-3 <105816654+vishals-3@users.noreply.github.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Is this not a duplicate of #10046? |
This branch has not been deployed
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
2 participants
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.
Fixes #10025
Root cause
translateRateLimitRuleloops overrule.ClientSelectors. Header, method and query parameter matches append to slices on the IR rule, butir.RateLimitRule.CIDRMatchis a single pointer:so
irRule.CIDRMatch = cidrMatchinside the loop overwrites on every iteration and only the last selector survives. Nothing surfaced the loss — no error, no status condition, and the policy still reportedAccepted: True.That is why the reporter's rule enforced only half of itself: a
Distinctselector followed by an inverted one kept only the inversion.What this changes
Rejects the configuration instead of silently dropping part of it:
Why not support multiple CIDRs
I looked at doing it properly and it is a bigger change than the API shape suggests. Each
sourceCIDRbecomes amasked_remote_addressentry in the rate limit service descriptor chain, the chain is nested rather than flat, and theDistinctarm adds a furtherremote_addresslevel beneath it. Supporting N CIDRs means deciding what the nesting order means when several are present, and what an inverted CIDR nested under a distinct one should do — semantics worth agreeing on before writing code.So this PR fails closed rather than guessing. It makes the limitation visible today, and does not foreclose full support later. Happy to do that follow-up if you would rather have it, and happy to close this in favour of it if you think rejecting is the wrong call.
Testing
All nine packages pass. Added the
backendtrafficpolicy-with-ratelimit-invalid-multiple-source-cidrfixture covering aDistinctselector followed by an inverted one, which is the shape from the issue. Existing fixtures using a singlesourceCIDRare unchanged.🤖 Generated with Claude Code