Uh oh!
There was an error while loading. Please reload this page.
feat(guardrails): per-request scoped GuardrailIndex — replaces flat chain (#379 P0c) - #411
Conversation
…hain (#379 P0c) Previously the proxy held a single flat Arc<dyn Guardrail> chain that applied identically to every request. This commit wires in a priority-sorted index that resolves the correct guardrail chain per-request based on attachment scope (env / model / api-key / team) and priority, enabling fine-grained, tenant-aware content control without any hot-path allocation on requests with no guardrails. ## aisix-core - GuardrailAttachment domain model (guardrail_id, scope_type, scope_id, priority, enabled, hook_point, direction) with full serde round-trip - GuardrailScopeType enum (Env/Model/ApiKey/Team) - AisixSnapshot gains guardrail_attachments ResourceTable - Guardrail domain model: three additive optional fields (enforcement_mode, mandatory, direction) — nullable, backward-compat ## aisix-etcd - loader: load "guardrail_attachment/" prefix and populate snapshot.guardrail_attachments on every sync ## aisix-guardrails - index.rs: GuardrailIndex + RequestContext + ScopeKind - Entries pre-sorted by (scope_specificity DESC, priority DESC) - resolve() walks entries in one pass, deduplicates by guardrail_id (highest-priority scope wins), returns a GuardrailChain - is_empty() fast-path skips chain allocation on requests with no applicable rules - 13 truth-table unit tests + 1 benchmark (1000-attachment build + 100 resolves in < 100ms under criterion) - build.rs: build_index_from_snapshot() joins guardrails + guardrail_attachments tables, skips disabled rows, builds runtime guardrails per attachment - build.rs: LiveGuardrailIndex — lazy-rebuild adapter over SnapshotHandle; one mutex + ptr-compare on hot path; full rebuild only when snapshot version changes (same pattern as LiveGuardrailChain) - chain.rs: GuardrailVerdict::Rewrite variant + Cow<ChatFormat> propagation through chain; treated as Allow on the output path ## aisix-proxy - state.rs: ProxyState.guardrail_index: Arc<LiveGuardrailIndex> replaces the old Arc<dyn Guardrail> field; all three constructors initialize a default empty index; with_guardrail_index() replaces with_guardrails() - chat.rs: per-request RequestContext + resolved_chain before input check; Rewrite verdict handled for both input and output paths - lib.rs (tests): seed_guardrail() helper inserts guardrail definition + env-scope attachment into AisixSnapshot so tests exercise the full index-resolution path through the live snapshot handle; all five guardrail test sites updated ## aisix-server - bootstrap: proxy_state.with_guardrail_index(LiveGuardrailIndex::new( snapshot_handle.clone(), bedrock_endpoint_url)) All 305 aisix-proxy tests + 61 aisix-guardrails tests pass. Full workspace compiles cleanly with zero warnings.
Warning Review limit reached
More reviews will be available in 10 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors guardrail configuration from a global statically-wired chain to a per-request index resolved from snapshot attachments. It adds GuardrailAttachment rows, scope-aware priority resolution, input rewrite propagation, live snapshot-backed indexing, and migrates runtime wiring and tests to use snapshot-driven resolution. ChangesGuardrail Per-Request Resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
Finding 1 (HIGH): Fix misleading doc comment on Guardrail.direction. The old comment falsely claimed GuardrailIndex::resolve uses the direction field for routing. Direction-based filtering is not yet implemented; the existing hook_point field on the guardrail definition already provides per-hook-point control for keyword rules. Finding 2 (MEDIUM): Add scope-specificity tiebreaker to index sort. GuardrailIndex::new() now sorts by (priority DESC, scope_specificity DESC) so that equal-priority attachments resolve deterministically: ApiKey wins over Team, Team over Model, Model over Env. Adds test case 14 to cover the equal-priority ApiKey > Env dedup scenario. Finding 3 (MEDIUM): Build LiveGuardrailIndex outside the mutex. current() now releases the lock before calling build_index_from_snapshot() so that a panic inside the build function cannot poison the mutex and crash every subsequent request. A potential concurrent double-build is accepted as the correct trade-off (both builds produce equivalent results). Finding 7 (LOW): Remove duplicate doc comment on Guardrail.config field.
moonming
commented
May 26, 2026
Audit findings — resolutionIndependent audit (full cold review, no shared context) returned 7 findings. All HIGH and MEDIUM findings are addressed in the follow-up commit Finding 1 — HIGH — Fixed ✅
The comment has been corrected to accurately state that direction-based filtering is not yet implemented in Finding 2 — MEDIUM — Fixed ✅Sort used priority only; scope-specificity tiebreaker was missing.
Finding 3 — MEDIUM — Fixed ✅
Refactored to fast-path (lock → version compare → return) + build outside lock + re-acquire to store. A rare concurrent double-build is accepted as correct trade-off; both builds produce equivalent results from the same snapshot version. Finding 4 — MEDIUM — Justified, not fixedThe three Finding 5 — LOW — Post-merge
Finding 6 — LOW — Post-merge
Finding 7 — LOW — Fixed ✅Duplicate doc comment on |
Add `p0c_fields_dont_trip_keyword_config_deny_unknown_fields` test that proves enforcement_mode/mandatory/direction are absorbed by the outer Guardrail struct before the flattened KeywordConfig (which has deny_unknown_fields) ever sees them. Without this test a regression in serde field routing would silently disarm P0c field acceptance. Addresses audit finding (Finding 2) on ai-gateway PR #411.
…clarify PartialEq footgun Two audit fixes (MEDIUM-1 and MEDIUM-2 from second audit): 1. chain.rs check_input: when Rewrite takes precedence over a Bypass that fired earlier in the chain, emit a tracing::info! so the bypass reason is preserved in the audit trail. Previously the bypass was silently dropped when Cow::Owned(rewritten) matched. 2. lib.rs PartialEq for GuardrailVerdict: strengthen the comment on the Rewrite == Rewrite arm from a mild note to a WARNING, so future test authors don't accidentally use assert_eq! and get a permanently-failing assertion with no helpful error message. Use is_rewrite() instead.
… on multi-attachment guardrails ResourceTable name-index is a flat map keyed by guardrail_id. A guardrail with two attachments (e.g. Env-scope + Model-scope) would have the second insert silently overwrite the first. build_index_from_snapshot already uses entries() to avoid this, but future callers using get_by_name would silently lose attachments. Add a WARNING doc comment to make the hazard explicit.
…hema Three CI failures addressed: 1. lint/fmt — cargo fmt applied to all changed files; the fmt reformatter touched build.rs, index.rs, proxy/lib.rs and snapshot.rs (indentation and line-length only). 2. schema drift — ran dump-schema; guardrail.schema.json now includes the three P0c additive fields (direction, enforcement_mode, mandatory) introduced in the previous commit. 3. e2e vitest — the GuardrailIndex build path required explicit GuardrailAttachment rows for any guardrail to fire. Existing E2E tests create guardrail definitions without attachment rows (pre-P0c pattern), so the index resolved to an empty chain and tests timed out waiting for the guardrail to trigger. Fix: in build_index_from_snapshot, after processing all attachment rows, iterate the guardrails table and treat any guardrail with ZERO attachment records as an implicit env-scope entry at priority 0. This preserves pre-P0c "apply globally" behavior during the rolling-upgrade window. Semantic: a guardrail that HAS attachment rows (even all- disabled) is governed by those rows and does NOT receive the fallback — the HashSet now tracks all attachment references regardless of enabled state.
Resolves all 5 findings from the independent audit of commit 4cdfd8f: HIGH (Finding 3): enforcement_mode="monitor" schema and doc comment claimed pass-through behavior, but the DP always blocks regardless of this field. Updated the doc comment with an explicit "not yet implemented" warning; re-ran dump-schema to propagate to the JSON schema. Operators who set "monitor" will now see the disclaimer rather than being silently misled. MEDIUM (Finding 4): mandatory=true doc comment and schema claimed fatal-error semantics, but the field is not yet consulted by the error-path logic. Added "not yet implemented" disclaimer to both the doc comment and the regenerated schema. MEDIUM (Finding 2): backward-compat scope-widening was logged at debug level, invisible in production log streams. Promoted to tracing::info! and added guardrail_name field so operators can identify which guardrail is firing globally during the rolling- upgrade window. MEDIUM (Finding 1 + 5): two unit tests added: - no_attachment_guardrail_fires_globally_backward_compat: asserts that a guardrail with zero attachment rows appears in the index as an env-scope entry AND blocks matching requests. - Extended disabled_attachment_is_skipped_in_index: adds a check_input assertion confirming the guardrail truly does not fire (not just that index.len() == 0).
HIGH-1: Add test covering the mixed enabled+disabled attachment case — one_enabled_one_disabled_attachment_fires_exactly_once verifies that a guardrail with one enabled + one disabled attachment fires exactly once (via the enabled attachment) and does NOT trigger the backward-compat env-scope fallback. This pins the HashSet boundary behavior that the previous commit's comment describes but had no test for. HIGH-2: Correct the is_empty() doc comment on LiveGuardrailIndex. The old comment said "no attachment entries" which excluded backward-compat no-attachment guardrail entries; corrected to "no guardrail entries from either attachment rows or the backward-compat fallback." MEDIUM-1: Add TODO(P0c-cleanup) removal marker on the backward-compat block in build_index_from_snapshot with a link to tracking issue #417. Prevents the fallback from silently persisting indefinitely after the rolling-upgrade window closes. MEDIUM-2: Add tracing::warn! in build_one when enforcement_mode is not "block". Operators who set enforcement_mode="monitor" expecting pass- through behavior will now see a log warning that the setting is not yet implemented and the DP will block regardless.
Uh oh!
There was an error while loading. Please reload this page.
`mandatory` was never a designed feature. It arrived in #411 as one of three schema columns the control plane's P0b added, carrying a doc comment that said so outright: "Not yet implemented — the field is stored and forwarded to the CP dashboard but the DP does not yet consult it; `fail_open` alone governs error behavior in the current release." A behaviour was retro-fitted to it five weeks later in #683, as a follow-up to a security review. Its whole documented job was overriding `fail_open` on the failure path. Once `fail_open` began defaulting to false (#1040), that job was already done by the default, and only two effects remained: resolving a configuration the operator contradicted themselves in (`fail_open: true` plus `mandatory: true`), and punching a hole through `enforcement_mode: monitor` — which nothing ever specified. It fell out of decorator ordering, and it read backwards: a monitored row would pass content it had detected as harmful while refusing all traffic, harmless included, because its provider was briefly unreachable. The mode meant to be safe for evaluating a new rule was the one that could take a deployment down. Nobody could have been relying on it. The dashboard never exposed the field, and the control plane is how every user configures aisix. Monitor mode is unconditional again: a monitored row never blocks, for any reason. An operator who wants an unreachable provider to refuse traffic is asking for enforcement, which is `block` mode with `fail_open: false` — one way to say it instead of two. This also settles what #1384 asked. A monitored row cannot block, so `EndOfStreamCheck` is the correct stream policy for it and there is nothing to hold back. Removed with it: the `MandatoryGuardrail` decorator, the `keep_unavailable_fatal` exception #1040 added to `MonitorGuardrail` to keep this guarantee alive, and the `preserves()` predicate that existed only to keep the two in agreement.
Summary
aisix-core:GuardrailAttachmentdomain model (scope_type, scope_id, priority, enabled, hook_point, direction);GuardrailScopeTypeenum;AisixSnapshot.guardrail_attachmentstable; three additive nullable fields onGuardrail(enforcement_mode, mandatory, direction)aisix-etcd: loader syncsguardrail_attachment/prefix into the snapshot on every tickaisix-guardrails: newindex.rs—GuardrailIndex+RequestContext+ScopeKind;build_index_from_snapshot();LiveGuardrailIndexlazy-rebuild adapter;GuardrailVerdict::Rewritevariant withCow<ChatFormat>propagation through chain; bypass telemetry preserved when shadowed by Rewriteaisix-proxy:ProxyState.guardrail_index: Arc<LiveGuardrailIndex>replaces old flatArc<dyn Guardrail>; per-requestRequestContextconstructed from auth context inchat.rs;Rewriteverdict handled for input and output paths; test helperseed_guardrail()exercises the full index-resolution path through a live snapshot handleDesign notes
The index pre-sorts entries by
(priority DESC, scope_specificity DESC)at build time.resolve()is a single linear scan with aHashSetdedup byguardrail_id— no allocation on requests with zero applicable entries (is_empty()fast-path). Scope specificity order: ApiKey > Team > Model > Env, matching the P0c spec in #379.LiveGuardrailIndexfollows the same lazy-rebuild pattern asLiveGuardrailChain: oneMutex<IndexCache>holding(last_version, Arc<GuardrailIndex>). Hot path is a ptr-compare against the current snapshot version; full rebuild fires only when the snapshot advances. Build happens outside the lock so a panic duringbuild_index_from_snapshotnever poisons the mutex.Benchmark: 1 000-attachment index build + 100 resolves well under 100ms (included in
index.rstests undercriterion).Serde routing note (critical for CP-DP compat)
KeywordConfighas#[serde(deny_unknown_fields)]. The three P0c fields are declared on the outerGuardrailstruct with#[serde(default)], so serde absorbs them at the outer level before the flattened inner type sees the remaining fields. Testp0c_fields_dont_trip_keyword_config_deny_unknown_fieldsinaisix-corepins this routing: if it ever regressed, the parse would return an unknown-field error and the test would catch it before any merge.Test plan
cargo test -p aisix-core— 176 tests includingp0c_fields_dont_trip_keyword_config_deny_unknown_fieldscargo test -p aisix-guardrails— 62 tests (index truth-table + build integration + live-rebuild + chain + bedrock + benchmark)cargo check --workspace— zero errors, zero warningscargo test -p aisix-proxy— proxy integration tests (guardrail block/bypass/rewrite paths via seed_guardrail helper)aisix-e2ecompose stack — guardrail block/bypass flows with real keyword guardrail config pushed via etcd (tracked in E2E: verify DP handles P0c kine fields (enforcement_mode, mandatory, direction) in real guardrail flow #414)Closes / related
Part of #379 P0c checklist. AISIX-Cloud PR #516 (kine projection widening) must not merge before this PR is deployed to all DPs.
E2E coverage gap for enforcement_mode/mandatory/direction in a live DP: #414 (filed as follow-up, not blocking merge given unit serde coverage).
Summary by CodeRabbit
New Features
Improvements