Repository navigation
fix: preserve shadow mode for limit overrides - #1274
PatilHrushikesh wants to merge 2 commits into
Conversation
638da3e to
daa86e0
Compare
Signed-off-by: Hrushikesh Patil <hrushikesh.patil@nutanix.com> Signed-off-by: Cursor <cursoragent@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
daa86e0 to
e5f5f1c
Compare
| } | ||
|
|
||
| if limitOverride != nil { | ||
| rateLimit = this.applyLimitOverride(overrideKey, rateLimit, limitOverride) |
There was a problem hiding this comment.
This now inherits every property of the matched descriptor, not only shadow_mode. The most important case is a descriptor matching unlimited: true (for example key6 in test/config/basic_config.yaml:63). constructLimitsToCheck (service/ratelimit.go:176-179) turns that into limitsToCheck[i] = nil, so the Envoy override is ignored and the request is never counted. Before this PR the override was enforced. Name/Replaces also now take part in the replacement pass at service/ratelimit.go:183-192, so an overridden descriptor can be dropped, or can drop another. QuotaMode changes the response aggregation. The PR body lists these as intended, but Envoy documents the override as replacing the configured limit, and existing users will see different enforcement with no release note. Please either (a) limit what carries over to ShadowMode (plus Metadata, if that's wanted), or (b) keep the full carry-over but add tests for the unlimited and replaces cases and a README/changelog note. If you keep (b), consider whether Unlimited should lose to an explicit override.
| overrideKey = this.descriptorKey(domain, descriptor) | ||
| // Drop the per-request override for lookup so matched descriptor | ||
| // properties (e.g. shadow_mode) are preserved; reapply after matching. | ||
| descriptor = &pb_struct.RateLimitDescriptor{Entries: descriptor.Entries} |
There was a problem hiding this comment.
Rebinding the parameter doesn't touch the caller's request.Descriptors[i]. That matters because GenerateCacheKey (base_limiter.go:70) and GetHitsAddends read the original. The shared Entries slice is only read below.
But nothing after this line reads descriptor.Limit: the loop at 495 and the share_threshold builder at 642 use only Entries. The old code skipped matching because of its early return, not because the loop looked at the override. So this allocation, and the comment about "dropping the override for lookup", can go. Probably keeping overrideKey computed here is enough?
| // The matched configuration supplies enforcement properties, but a fallback | ||
| // descriptor must not collapse distinct request values onto its default key. | ||
| rateLimit.FullKey = overrideKey | ||
| rateLimit.Stats = this.statsManager.NewStats(overrideKey) |
There was a problem hiding this comment.
This throws away the key work done before calling this method: the detailed_metric, value_to_metric and share_threshold paths (lines 633-678) have already called NewStats for their keys, and this replaces them. Two side effects. First, those earlier NewStats calls still register counters that are never incremented. Second, a share_threshold descriptor gets per-value stats while its Redis counter is still shared through ShareThresholdKeyPattern (cache_key.go:76-80). That matches the old override path's metric naming, so it isn't a regression, but it's worth one line in the comment above. Alternatively, short-circuit the metric-key rebuilding when limitOverride != nil.
|
|
||
| rateLimit.Limit = &pb.RateLimitResponse_RateLimit{ | ||
| RequestsPerUnit: override.GetRequestsPerUnit(), | ||
| Unit: unit, |
There was a problem hiding this comment.
NewRateLimit sets Limit.Name, but this literal drops it, while rateLimit.Name keeps the matched name. If the name should survive the override, carry it into the response limit too.
Unit: unit,
Name: rateLimit.Limit.GetName(),| }, | ||
| }) | ||
| assert.NotNil(rl) | ||
| assert.True(rl.ShadowMode) |
There was a problem hiding this comment.
This would fail against the old code, so it's a real regression test. Please also assert rl.FullKey / a stats counter here. The PR body's second claim (no collapse onto the default key) is otherwise covered only indirectly by TestConfigLimitOverride, which never matches a shadow/default leaf. Also add a case where the override hits an unlimited: true descriptor, so whichever behaviour you choose is pinned down.
|
Can you fix the DCO? |
An Envoy override should replace the numeric limit and preserve shadow mode, without inheriting unlimited/quota/name/replaces or rebuilding metrics that are discarded for the override key. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Hrushikesh Patil <hrushikesh.patil@nutanix.com>
1e4aadc to
2c73c74
Compare
Problem
When Envoy sends a per-request limit override, RLS returned a newly constructed limit with
shadow_modedisabled. This caused dynamicFromMetadataquotas configured withshadowMode: trueto return 429 instead of recording usage without enforcement.The override path also replaced the original descriptor metric key with the matched configured descriptor key, which could collapse distinct descriptor values onto the same counter.
Fix
shadow_mode, quota mode, and unlimited behavior.Validation
go test ./test/configgo test ./...