feat(router): support per-key rate-limit overrides with regex pattern matching#2683
Merged
feat(router): support per-key rate-limit overrides with regex pattern matching#2683
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded per-key rate-limit overrides: config types, schema, fixtures, docs, and tests updated; Cosmo rate limiter now accepts, compiles, and applies regex-based overrides (first-match) against the key suffix; graph server wires overrides into the limiter. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
endigma
force-pushed
the
jesse/eng-9286-rate-limit-overrides
branch
from
4010d6d to
a9655c3
Compare
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
Router image scan passed✅ No security vulnerabilities found in image: |
Member
Author
|
Resolves #2395 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2683 +/- ##
==========================================
- Coverage 63.34% 63.14% -0.20%
==========================================
Files 249 249
Lines 26643 26661 +18
==========================================
- Hits 16876 16835 -41
- Misses 8404 8449 +45
- Partials 1363 1377 +14
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/graph_server.go (1)
1487-1499:⚠️ Potential issue | 🟠 MajorClean up partially initialized graph resources before returning here.
NewCosmoRateLimiternow adds a late failure point after the connector, pubsub providers, caches, and metric stores are already initialized. Becausegmis only appended tos.graphMuxListat Line 1638, this direct return skips the normal shutdown path and leaks those resources when an override regex is invalid or rate-limiter construction otherwise fails. Please route this through the samebuildGraphMuxcleanup path used for other late init failures.Based on learnings, ensure that buildGraphMux error paths clean up partially initialized resources (caches, metric stores, pub/sub providers, connectors) before returning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/graph_server.go` around lines 1487 - 1499, The rate-limiter failure path currently returns directly after NewCosmoRateLimiter fails and thus leaks partially-initialized resources; instead route this failure through the same buildGraphMux cleanup path used for other late-init errors: when NewCosmoRateLimiter(...) returns an error, invoke the existing buildGraphMux error/cleanup routine (the same code used elsewhere to unwind gm before appending to s.graphMuxList) rather than returning immediately, or call the shared teardown function that releases connector, pubsub, cache and metric store resources for the in-progress gm; also ensure buildGraphMux's error paths (the routines that run when late init fails) explicitly free caches, metric stores, pub/sub providers and connectors so no resources leak on rate-limiter construction failures.
🧹 Nitpick comments (2)
router/pkg/config/config.schema.json (1)
2309-2312: Prevent accidental catch-all override by disallowing emptymatching.
matching: ""is a valid regex and can silently match every key, shadowing later overrides. Add a minimum length so catch-all intent must be explicit (e.g..*).Based on learnings: In the Cosmo router project, parameter validation for configuration is handled at the JSON schema level rather than through runtime validation methods on structs.Suggested schema tweak
"matching": { "type": "string", + "minLength": 1, "description": "A regex pattern matched against the resolved rate-limit key." },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config.schema.json` around lines 2309 - 2312, The "matching" property currently allows an empty string which acts as a silent catch-all; update the JSON schema for the "matching" property to forbid empty values (e.g., add "minLength": 1 or a pattern that requires at least one character) so an explicit catch-all must be expressed (for example ".*"); modify the schema entry for "matching" to include this constraint to prevent accidental empty-regex overrides.router-tests/security/ratelimit_test.go (1)
725-733: Consider adding a precedence case for overlapping overrides.This test proves match vs non-match behavior, but it doesn’t lock in the “first matching override wins” rule. A second matching override with a different limit would make precedence explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/security/ratelimit_test.go` around lines 725 - 733, Add a precedence case by adding a second overlapping RateLimitOverride in the Overrides slice (using the same or broader Matching pattern as the existing "^.*:premium-.*" but with a different Rate/Burst/Period) and update the test assertions to verify that the first matching override in the slice is applied (i.e., the limit equals the first override's Rate/Burst rather than the later one); reference the Overrides []config.RateLimitOverride literal and the Matching/Rate/Burst/Period fields to implement this additional override and expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-website/router/configuration.mdx`:
- Around line 1947-1950: The override example's matching regex (the matching
field shown as "^premium-.*") doesn't account for the resolved rate-limit key
format which uses the default prefix format "<key_prefix>:<suffix>"; update the
example in the overrides block so the matching pattern looks for the
colon-separated resolved key (e.g., change "^premium-.*" to "^premium:.*" or to
a pattern that includes the prefix, and optionally add a clarifying note near
the earlier explanation that matching is performed against the resolved key
which includes the prefix).
In `@docs-website/router/security/hardening-guide.mdx`:
- Around line 91-116: The example under rate_limit.simple_strategy.overrides
shows matching for "^premium-.*" and "^internal-.*" but doesn't show how those
suffixes get into the resolved key; update the snippet to either add a
key_suffix_expression that produces keys like "premium-<id>" / "internal-<id>"
or add an inline internal link to the configuration reference that defines the
full generated key format. Specifically modify the rate_limit block to include a
key_suffix_expression example (or a short reference link) so the overrides'
matching regexes can actually match, keeping the symbols rate_limit,
simple_strategy, overrides, matching and key_suffix_expression in the
documentation for discoverability.
---
Outside diff comments:
In `@router/core/graph_server.go`:
- Around line 1487-1499: The rate-limiter failure path currently returns
directly after NewCosmoRateLimiter fails and thus leaks partially-initialized
resources; instead route this failure through the same buildGraphMux cleanup
path used for other late-init errors: when NewCosmoRateLimiter(...) returns an
error, invoke the existing buildGraphMux error/cleanup routine (the same code
used elsewhere to unwind gm before appending to s.graphMuxList) rather than
returning immediately, or call the shared teardown function that releases
connector, pubsub, cache and metric store resources for the in-progress gm; also
ensure buildGraphMux's error paths (the routines that run when late init fails)
explicitly free caches, metric stores, pub/sub providers and connectors so no
resources leak on rate-limiter construction failures.
---
Nitpick comments:
In `@router-tests/security/ratelimit_test.go`:
- Around line 725-733: Add a precedence case by adding a second overlapping
RateLimitOverride in the Overrides slice (using the same or broader Matching
pattern as the existing "^.*:premium-.*" but with a different Rate/Burst/Period)
and update the test assertions to verify that the first matching override in the
slice is applied (i.e., the limit equals the first override's Rate/Burst rather
than the later one); reference the Overrides []config.RateLimitOverride literal
and the Matching/Rate/Burst/Period fields to implement this additional override
and expectation.
In `@router/pkg/config/config.schema.json`:
- Around line 2309-2312: The "matching" property currently allows an empty
string which acts as a silent catch-all; update the JSON schema for the
"matching" property to forbid empty values (e.g., add "minLength": 1 or a
pattern that requires at least one character) so an explicit catch-all must be
expressed (for example ".*"); modify the schema entry for "matching" to include
this constraint to prevent accidental empty-regex overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9cf235d-4645-4f91-a87a-aa59150ee09c
📒 Files selected for processing (11)
docs-website/router/configuration.mdxdocs-website/router/security/hardening-guide.mdxrouter-tests/security/ratelimit_test.gorouter/core/graph_server.gorouter/core/ratelimiter.gorouter/core/ratelimiter_test.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/core/ratelimiter.go (1)
95-98: RenameresolveLimitparameter fromkeytosuffixfor clarity.At Line 95, the parameter is named
key, but call sites (Line 126) pass the suffix. Renaming reduces ambiguity and helps prevent regressions back to full-key matching.Also applies to: 126-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/ratelimiter.go` around lines 95 - 98, Rename the parameter in CosmoRateLimiter.resolveLimit from key to suffix to match its actual usage; update the function signature of resolveLimit(suffix string, defaultLimit redis_rate.Limit) and all call sites that pass a suffix to use the new parameter name, and ensure the internal pattern matching still uses the renamed variable (e.g., in resolveLimit and any callers of CosmoRateLimiter.resolveLimit) so the logic remains identical but clearer that only the suffix is matched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/core/ratelimiter.go`:
- Around line 95-98: Rename the parameter in CosmoRateLimiter.resolveLimit from
key to suffix to match its actual usage; update the function signature of
resolveLimit(suffix string, defaultLimit redis_rate.Limit) and all call sites
that pass a suffix to use the new parameter name, and ensure the internal
pattern matching still uses the renamed variable (e.g., in resolveLimit and any
callers of CosmoRateLimiter.resolveLimit) so the logic remains identical but
clearer that only the suffix is matched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92c2754d-b25f-432d-8af7-dab25eb0f629
📒 Files selected for processing (4)
docs-website/router/configuration.mdxdocs-website/router/security/hardening-guide.mdxrouter/core/ratelimiter.gorouter/core/ratelimiter_test.go
✅ Files skipped from review due to trivial changes (1)
- docs-website/router/security/hardening-guide.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- docs-website/router/configuration.mdx
- router/core/ratelimiter_test.go
SkArchon
reviewed
Mar 24, 2026
SkArchon
reviewed
Mar 24, 2026
Contributor
|
Will take another look when the PR is ready for review |
endigma
requested review from
a team,
StarpTech,
devsergiy and
jensneuse
as code owners
SkArchon
requested changes
Mar 30, 2026
Contributor
SkArchon
left a comment
SkArchon
left a comment
There was a problem hiding this comment.
Just one doc change and two small nits.
- Add test assertions verifying premium user is also rate-limited after exceeding their override allowance - Update docs example to use JWT claim expression instead of header, with MCP/internal consumer type scenario
This was referenced Mar 31, 2026
|
@endigma - thank you so much. This is great. |
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
3 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.
Adds an
overridesarray tosimple_strategyin the rate limiter config. Each override specifies amatchingregex pattern tested against the resolved rate-limit key; the first match wins and unmatched keys fall back to global defaults.Includes documentation updates to the hardening guide and configuration reference.
Summary by CodeRabbit
New Features
Documentation
Tests
Checklist