feat(ratelimit): conditional-form rate limit policies — conditions tree + group_by + 7-field limits - #886
Conversation
…ee + group_by + 7-field limits
RateLimitPolicy gains a second, mutually-exclusive form (fixed at
creation): a lua-resty-expr-equivalent condition node tree (leaves
{dimension,operator,negate,value} + and/or groups, depth<=3, leaves<=16),
group_by bucketing over {team,member,api_key,model,provider} with
canonical segment order, the full 7-field RateLimit as limits, and a
reserved action enum. Classic rows keep serializing byte-identically;
stored rows are never rewritten.
- schema: root oneOf enforces the form XOR in both validator sets;
untagged ConditionNode variants are closed in both sets (serde
swallows unknown fields inside untagged content silently)
- load: validate_semantics() rejects whole rows on tree caps, the
operator x dimension matrix, or uncompilable regexes (fail-open per
row, never a half-enforced policy); regexes precompile into a
process-wide cache
- quota gate: unified match_policy_layer() for both forms and both
scan phases; model-property policies reserve where the concrete
model is known (request gate for direct dispatch, per-target for
routing/ensemble parents, per member for ensembles) — auth threaded
through reserve_model_only/reserve_routing_target for identity
dimensions
- buckets: policy:v2:<id>[:dim=value...]; storage layer untouched
- 429s from policy layers now carry error.policy {id,name}; the
rejections counter moves to the gate covering every endpoint and
gains layer/policy_id labels
- filesource: conditional rows accept api_key/model names in
condition values (same sugar as scope_ref)
Ref api7/AISIX-Cloud#892
… tpm settle, per-target routing Five scenarios against a real aisix + etcd: OR-group matching with error.policy attribution on the 429; leaf negate (!in) exemption; group_by [member] bucketing per user_id (not per key); tpm commit visibility across calls; a model-property policy with group_by [model] reserving per routing target (over-limit target fails over, all over -> 429, the group parent's own matching name burns no bucket, direct dispatch shares the target bucket). Ref api7/AISIX-Cloud#892
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR adds conditional rate-limit policies with condition evaluation, grouping, semantic validation, authenticated quota matching, policy-attributed errors and metrics, and end-to-end coverage. Classic policy compatibility remains supported. ChangesConditional rate-limit enforcement
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant Quota
participant ConditionEvaluator
participant Metrics
Client->>Proxy: Authenticated request
Proxy->>Quota: Reserve model or routing quota
Quota->>ConditionEvaluator: Evaluate policy conditions
ConditionEvaluator-->>Quota: Matching policy and bucket
Quota->>Metrics: Record rejection labels
Quota-->>Proxy: Reservation result or policy rate-limit error
Proxy-->>Client: Response or attributed 429 error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
crates/aisix-core/src/models/rate_limit_policy.rs (1)
503-636: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the
limits-less conditional row.
validate_semanticshas a dedicated arm returningconditional policy requires \limits`(Line 346). No test reaches it. A row carryingconditionsorgroup_bywithoutlimits` takes that arm, and the message differs from the classic-branch message. Pin the arm so a future refactor cannot reroute such a row into the classic branch.💚 Proposed test addition
assert!(dup_group_by .validate_semantics() .unwrap_err() .contains("repeats")); + + // `conditions` without `limits` is conditional-but-incomplete, + // not a classic row. + let no_limits: RateLimitPolicy = serde_json::from_value(json!({ + "name": "no-limits", + "conditions": [ + { "dimension": "team", "operator": "in", "value": ["t-1"] } + ] + })) + .unwrap(); + assert!(no_limits + .validate_semantics() + .unwrap_err() + .contains("requires `limits`")); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/models/rate_limit_policy.rs` around lines 503 - 636, Add a test beside validate_semantics_rejects_mixed_and_incomplete_forms that deserializes a policy containing conditions or group_by but no limits, calls validate_semantics, and asserts the error contains “conditional policy requires `limits`”. Ensure the case specifically exercises the conditional validation arm rather than the classic incomplete-policy path.crates/aisix-core/src/models/policy_conditions.rs (1)
484-495: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid the per-call key allocation on the cache-hit path.
compiled_regexruns once per matching regex leaf per request at the quota gate. Each call allocatespattern.to_string()to build the lookup key, even when the entry is already cached. Key the cache by pattern only and hold both case variants in the value to remove that allocation.♻️ Proposed refactor to remove the hot-path allocation
-type CachedRegex = Option<Arc<regex::Regex>>; -static REGEX_CACHE: Lazy<DashMap<(String, bool), CachedRegex>> = Lazy::new(DashMap::new); +type CachedRegex = Option<Arc<regex::Regex>>; +/// `[case_sensitive, case_insensitive]` per distinct pattern. +static REGEX_CACHE: Lazy<DashMap<String, [Option<CachedRegex>; 2]>> = Lazy::new(DashMap::new); fn compiled_regex(pattern: &str, case_insensitive: bool) -> Option<Arc<regex::Regex>> { - if let Some(hit) = REGEX_CACHE.get(&(pattern.to_string(), case_insensitive)) { - return hit.clone(); - } + let slot = usize::from(case_insensitive); + if let Some(entry) = REGEX_CACHE.get(pattern) { + if let Some(hit) = entry[slot].as_ref() { + return hit.clone(); + } + } let compiled = regex::RegexBuilder::new(pattern) .case_insensitive(case_insensitive) .build() .ok() .map(Arc::new); - REGEX_CACHE.insert((pattern.to_string(), case_insensitive), compiled.clone()); + REGEX_CACHE.entry(pattern.to_string()).or_default()[slot] = Some(compiled.clone()); compiled }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/models/policy_conditions.rs` around lines 484 - 495, Refactor compiled_regex so REGEX_CACHE is keyed by the pattern without allocating pattern.to_string() on lookup, while the cached value stores both case-sensitive and case-insensitive compiled regex variants. Update lookup, compilation, and insertion in compiled_regex to select the requested case variant and preserve existing caching behavior.crates/aisix-core/src/filesource/desugar.rs (1)
245-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the human-readable kind label instead of deriving it twice.
The mapping from a kind key to its prose word exists in two places:
resolve_entity_nameat Lines 257-261 and thefieldlabel at Lines 299-303. A third kind would need edits in both. Pass the label into the helper.♻️ Proposed refactor
fn resolve_entity_name( maps: &IdentityMaps, kind: &str, + kind_label: &str, name: &str, field: &str, ) -> Result<String, String> { maps.get(kind) .and_then(|m| m.get(name)) .cloned() .ok_or_else(|| { format!( - "{field} references unknown {} {name:?} ({})", - if kind == "api_keys" { - "api key" - } else { - "model" - }, + "{field} references unknown {kind_label} {name:?} ({})", known_names(maps, kind) ) }) }Then bind both the kind and its label at each dimension match:
let (kind, kind_label, dim) = match obj.get("dimension").and_then(Value::as_str) { Some("api_key") => ("api_keys", "api key", "api_key"), Some("model") => ("models", "model", "model"), _ => continue, }; let field = format!("`conditions` {dim} value");Update the
scope_refcall site at Line 240 to pass its label as well.Also applies to: 292-304
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/filesource/desugar.rs` around lines 245 - 265, Update resolve_entity_name to accept a human-readable kind label parameter and use it directly in the unknown-entity error, removing its internal kind-to-label conditional. In the dimension match, bind both the kind key and label (for example, “api_keys” with “api key” and “models” with “model”), and pass the label through the resolve_entity_name and scope_ref call sites while preserving existing field formatting.crates/aisix-core/src/filesource/tests.rs (1)
229-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
_format_versionso the test isolates the failure it asserts.This fixture omits
_format_version.load_from_strpushes amissing mandatory _format_versionfile error and continues, so the returned error vector carries two unrelated errors. The assertion still passes, but the test no longer proves that the unknown-model reference alone fails the load. Add the version key.💚 Proposed fix
let file = r#" +_format_version: "1" + provider_keys: - display_name: pk secret: sk-x🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/filesource/tests.rs` around lines 229 - 249, Add the mandatory _format_version key to the YAML fixture in the load test before asserting the unknown-model error, so load returns only the intended bad-ref failure and the assertion isolates the no-such-model validation.crates/aisix-proxy/src/quota.rs (1)
481-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the shared policy-reservation loop.
This loop repeats the
reserve_layerspolicy scan exactly: iteratesnap.rate_limit_policies.entries(), skip suspended rows, callmatch_policy_layer,pre_commit, and map the error throughreject. Only thePolicyPhasediffers. A shared helper that takes the phase keeps the two phases from drifting when the scan changes.♻️ Sketch of the extracted helper
async fn reserve_policy_layers( state: &ProxyState, input: &ConditionInput<'_>, phase: PolicyPhase, reservations: &mut Vec<aisix_ratelimit::Reservation>, ) -> Result<(), ProxyError> { let snap = state.snapshot.load(); let now = chrono::Utc::now(); for entry in snap.rate_limit_policies.entries() { let policy = &entry.value; if policy.suspended_at(now) { continue; } let Some(layer) = match_policy_layer(policy, &entry.id, input, phase) else { continue; }; let r = state .limiter .pre_commit(&layer.bucket_key, &layer.limits) .await .map_err(|e| reject(state, e, "policy", Some((&entry.id, &policy.name))))?; reservations.push(r); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-proxy/src/quota.rs` around lines 481 - 499, Extract the duplicated policy-reservation scan from reserve_layers and the shown model-target flow into a shared async reserve_policy_layers helper that accepts the ConditionInput, PolicyPhase, and mutable reservations vector. Preserve the existing suspended-policy filtering, match_policy_layer call, limiter.pre_commit behavior, and reject error mapping, passing the appropriate phase from each caller.crates/aisix-proxy/src/ensemble.rs (1)
155-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the discarded quota error for the ensemble sub-call.
map_err(|_| ...)drops theProxyError. With conditional policies the dropped error can beProxyError::PolicyRateLimit, which names the policy that throttled this member. The replacement message says only "rate limit exceeded for an ensemble sub-call", so an operator cannot tell from this site which policy or which layer fired. Keeping the generic client-visible message is correct; emit the cause totracingalongside it.♻️ Proposed change
let reservation = crate::quota::reserve_model_only(self.state, self.auth, target, &entry.id, model) .await - .map_err(|_| { + .map_err(|e| { + tracing::warn!( + member = %target, + error = %e, + "ensemble member exceeded its own rate limit" + ); BridgeError::upstream_status( 429, "rate limit exceeded for an ensemble sub-call", ) })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-proxy/src/ensemble.rs` around lines 155 - 163, Update the error closure in the ensemble sub-call quota reservation around reserve_model_only to bind the discarded ProxyError, log it with tracing while preserving the existing generic 429 client-facing message, and continue mapping the failure to BridgeError::upstream_status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-etcd/src/loader.rs`:
- Around line 325-347: Update build_snapshot and its surrounding validation flow
so validate_semantics failures are accounted for before the snapshot is
returned, preventing stats.accepted, partial_rows, and rejection state from
treating semantically invalid policies as successful. Ensure
Supervisor::apply_put preserves the rejection and removes stale
partial-compatibility state for these failures, and add focused loader and
Supervisor::apply_put tests covering semantic rejection.
In `@tests/e2e/src/cases/multidim-ratelimit-policy-e2e.test.ts`:
- Around line 384-385: Update the readiness call in the test flow around
waitModelsListed and awaitWindowHeadroom to include mdrl-rt-group alongside
mdrl-rt-a and mdrl-rt-b, ensuring dispatch waits until the routing group is
available before testing target failover.
---
Nitpick comments:
In `@crates/aisix-core/src/filesource/desugar.rs`:
- Around line 245-265: Update resolve_entity_name to accept a human-readable
kind label parameter and use it directly in the unknown-entity error, removing
its internal kind-to-label conditional. In the dimension match, bind both the
kind key and label (for example, “api_keys” with “api key” and “models” with
“model”), and pass the label through the resolve_entity_name and scope_ref call
sites while preserving existing field formatting.
In `@crates/aisix-core/src/filesource/tests.rs`:
- Around line 229-249: Add the mandatory _format_version key to the YAML fixture
in the load test before asserting the unknown-model error, so load returns only
the intended bad-ref failure and the assertion isolates the no-such-model
validation.
In `@crates/aisix-core/src/models/policy_conditions.rs`:
- Around line 484-495: Refactor compiled_regex so REGEX_CACHE is keyed by the
pattern without allocating pattern.to_string() on lookup, while the cached value
stores both case-sensitive and case-insensitive compiled regex variants. Update
lookup, compilation, and insertion in compiled_regex to select the requested
case variant and preserve existing caching behavior.
In `@crates/aisix-core/src/models/rate_limit_policy.rs`:
- Around line 503-636: Add a test beside
validate_semantics_rejects_mixed_and_incomplete_forms that deserializes a policy
containing conditions or group_by but no limits, calls validate_semantics, and
asserts the error contains “conditional policy requires `limits`”. Ensure the
case specifically exercises the conditional validation arm rather than the
classic incomplete-policy path.
In `@crates/aisix-proxy/src/ensemble.rs`:
- Around line 155-163: Update the error closure in the ensemble sub-call quota
reservation around reserve_model_only to bind the discarded ProxyError, log it
with tracing while preserving the existing generic 429 client-facing message,
and continue mapping the failure to BridgeError::upstream_status.
In `@crates/aisix-proxy/src/quota.rs`:
- Around line 481-499: Extract the duplicated policy-reservation scan from
reserve_layers and the shown model-target flow into a shared async
reserve_policy_layers helper that accepts the ConditionInput, PolicyPhase, and
mutable reservations vector. Preserve the existing suspended-policy filtering,
match_policy_layer call, limiter.pre_commit behavior, and reject error mapping,
passing the appropriate phase from each caller.
🪄 Autofix
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: 34a6d708-a7ba-4473-b2a0-2db3a0bda849
📒 Files selected for processing (21)
crates/aisix-core/src/filesource/desugar.rscrates/aisix-core/src/filesource/mod.rscrates/aisix-core/src/filesource/tests.rscrates/aisix-core/src/models/mod.rscrates/aisix-core/src/models/policy_conditions.rscrates/aisix-core/src/models/rate_limit_policy.rscrates/aisix-core/src/models/schema.rscrates/aisix-etcd/src/loader.rscrates/aisix-obs/src/metrics.rscrates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/count_tokens.rscrates/aisix-proxy/src/ensemble.rscrates/aisix-proxy/src/error.rscrates/aisix-proxy/src/error_translate.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/quota.rscrates/aisix-proxy/src/responses.rscrates/aisix-proxy/src/stream_failover.rsschemas/resources/rate_limit_policy.schema.jsontests/e2e/src/cases/multidim-ratelimit-policy-e2e.test.ts
…ning Review round (CodeRabbit + independent audit): - loader: semantic validation moves INSIDE validate_and_parse (new semantic hook), rejecting BEFORE accept accounting — previously a semantically invalid row was double-booked (accepted + rejected) and the watch path's apply_put, which gates success on stats.accepted, reported success without retaining the rejection for the heartbeat. Loader + supervisor tests pin the corrected accounting and the last-good-value retention. - chat routing loops: a per-target PolicyRateLimit is no longer flattened away on exhaustion — the last reserve rejection (policy layer only; the inline model layer keeps its established shape) is surfaced un-flattened so the all-targets-exhausted 429 keeps error.policy. PolicyRateLimit's Display now names the policy, so every flattening path (attempt records, mid-stream failover, ensemble logs) carries the attribution in the message too. The routing e2e now asserts attribution on the exhausted 429. - e2e: the routing-group readiness race is closed with a no-access probe key (groups are invisible to /v1/models); the unknown-model fixture pins _format_version + a valid provider key so it proves exactly one load error. - hardening: regex caches are per-variant (borrowed &str lookups, no per-eval allocation) with a 1024-entry clear-on-overflow backstop; group_by bucket segments percent-escape ':'/'='/'%' so file-source operator-chosen ids cannot alias two value tuples onto one bucket; the policy scan loop is extracted (reserve_policy_layers) so the request and per-target phases cannot drift; the group-negate semantics on model-less requests and the is_routing_request invariant on messages/responses/count_tokens are pinned in comments. Ref api7/AISIX-Cloud#892
|
Independent audit (per repo review policy) ran against MEDIUM — semantic rejection mis-accounted as a successful apply (same root as the CodeRabbit thread on MEDIUM — chat routing exhaustion lost LOW×5: e2e group-readiness race (fixed via no-access probe key — groups are invisible to Observation: All six CodeRabbit nitpicks were also taken (limits-less form test, cache-hit allocation, desugar label, |
Conflict in chat.rs' error tail, resolved by deleting both sides: - #886 moved rate-limit rejection counting to `quota::reject`, which every endpoint funnels through and which knows the offending layer, and removed it from chat's `record_error` to avoid double-booking. Keeping this branch's `note_ratelimit_rejection` would have reintroduced exactly that double count. - `record_error`'s remaining job — the legacy `record_request` — is now done by `request_metrics::record` further down the same arm. So neither helper has anything left to do.
1 participant
Implements the data-plane half of the multi-dimensional rate limit policy redesign (design: api7/rfcs
features/aisix-multi-dimensional-rate-limit, approved in rfcs#246/#248). Fixes api7/AISIX-Cloud#892.What
rate_limit_policiesrows gain a second, mutually-exclusive conditional form, fixed at creation:{ "name": "algo-team-premium-models", "conditions": [ // node tree; top level = implicit AND { "dimension": "team", "operator": "in", "value": ["<team-uuid>"] }, { "logic": "or", "children": [ // explicit AND/OR groups, nestable { "dimension": "model_name", "operator": "~~", "value": "^gpt-4\\.1" }, { "dimension": "provider", "operator": "==", "value": "anthropic" } ]} ], "group_by": ["team"], // bucket split; [] = one shared bucket "limits": { "rpm": 1000, "tpm": 1000000 }, // full 7-field RateLimit "action": "reject" // reserved enum, v1 = reject }team/member/api_key/model, UUID values, operators==/~=/in+negate) and string (model_name/provider, additionally~~/~*).has, numeric comparisons andipmatchare wire vocabulary reserved for future dimensions — validation admits them on nothing yet, the evaluator is already total.inlists ≤ 64 values, regex ≤ 256 bytes and must compile. Caps beyond JSON-Schema expressiveness are enforced byvalidate_semantics()at load; a failing row is rejected whole (fail-open for that row only, never a half-enforced policy). Regexes precompile into a process-wide cache — no compile in the hot path.falseeven undernegate; OR siblings still match. Agroup_bydimension the request doesn't carry makes the policy inapplicable (same rule that keepsteam_memberoff user-less keys).policy:v2:<policy_id>plus one:<dim>=<value>segment pergroup_bydimension in canonical order (declared order never changes bucket identity). The limiter/storage layer (local + Redis, Lua scripts) is untouched — bucket keys were always opaque strings.model/model_name/providerin the tree orgroup_by) reserve where the concrete model is known — the request gate for a direct dispatch, per target for routing/ensemble parents (an over-limit target is a failed attempt that fails over; the parent's own name never burns a bucket). Everything else reserves once at the request gate. The two policy scan loops (reserve_layers/reserve_model_only) had already drifted once (schedules); they now share onematch_policy_layer().schedules(AISIX-Cloud#1104) composes unchanged: a suspended row reserves nothing regardless of form.Attribution + metrics
error.policy: {id, name}in the OpenAI envelope (additive, same convention as the budget block; the Anthropic envelope keeps its strict shape). With several policies live an unattributed 429 is undebuggable.aisix_ratelimit_rejections_totalmoves to the quota gate — it previously counted only chat-endpoint rejections — and gainslayer(api_key/model/mcp/policy) andpolicy_idlabels.Compatibility
scope) and drops that row only — the documented short cross-plane window during rollout; classic rows keep enforcing throughout.oneOfin both validator sets. The untaggedConditionNodevariants are closed against unknown fields in both sets (serde silently swallows unknowns inside untagged content — the schema is the only guard).schemas/resources/rate_limit_policy.schema.jsonregenerated (dump-schema).api_key/modelnames in condition values (same sugar asscope_ref), resolved recursively through group nodes.Paired CP work
This adds a user-facing config surface, so per the config-knob rule it implies a CP PR (api7/AISIX-Cloud) in the same effort:
cp-admin.yamlschema with these exact field names/enums + regenerated bindings, Go validation + etcd projection, the dashboard condition-tree builder, and CP↔DP e2e. That PR follows once this merges and thedevimage is published; field naming here matches the RFC-pinned wire shape thatcp-admin.yamlwill mirror.Tests
aisix+ etcd,tests/e2e/src/cases/multidim-ratelimit-policy-e2e.test.ts): OR-group match/miss witherror.policyattribution,negateexemption,group_by [member]per-user buckets,tpmcommit visibility, per-target routing failover → all-over 429 → direct dispatch sharing the target bucket.Summary by CodeRabbit
New Features
Bug Fixes