feat(guardrails): preserve Aliyun's upstream RequestId and correlate it to the gateway request - #781
Merged
Merged
Conversation
…it to the gateway request
The `aliyun_text_moderation` guardrail dropped everything Aliyun reported
about a moderation call: `AliyunResponse` never deserialized `RequestId`,
so a caller's 422 `content_filter` could not be traced to a record in the
Aliyun console.
Capture and log the upstream diagnostics — `RequestId`, `Code`, `Message`,
`RiskLevel` and the matched `Label`s — on every path, and make them joinable
to the caller's `x-aisix-request-id`.
## Where the RequestId comes from
Read `x-acs-request-id` off the response headers, falling back to the body's
`RequestId`. Verified against the live green-cip endpoint: the header is
present on 2xx, on a JSON business error, and on 400/404 responses whose body
is XML — i.e. it survives exactly the cases where the body is unreachable.
The body alone would not: on an HTTP error Aliyun types `Code` as a symbolic
string (`InvalidAccessKeyId.NotFound`) rather than the int it uses on HTTP
200, so the documented shape does not deserialize there.
Timeouts and connection failures produce no id at all; those log an empty
`aliyun_request_id` plus the failure bucket, so "Aliyun never answered" is
distinguishable from "we forgot to log it". An HTTP 200 carrying a non-JSON
body now reports `MalformedResponse` instead of being bucketed as
`ServerError` — the two want different fixes.
## Correlation
Nothing in the data plane put a `request_id` on a log line: `ensure_request_id`
minted the id for the header and telemetry, but no span carried it, so all 38
guardrail log sites across all six providers were uncorrelated. The middleware
now opens a `request{request_id=…}` span, which every guardrail inherits.
Streamed response bodies are polled after the middleware returns, so the four
SSE generators re-attach the span to the body stream, and the realtime session
attaches it to the future axum spawns.
## Log searchability
The fmt subscriber colorized unconditionally. On a piped stderr — every real
deployment — the escapes land between a field's name and its value, so
`grep 'aliyun_request_id=<id>'` matched nothing. Colorize only for a terminal.
## Not exposed to callers
Aliyun's `RequestId` stays out of the response envelope. Callers get
`x-aisix-request-id` and quote it; operators resolve the rest from the log.
This keeps the moderation vendor undisclosed and adds no wire contract.
## Content safety
A real `high` response echoes the offending text back in `RiskWords` and
`RiskPositions` (`"RiskWords": "傻逼,弄死你,死你全家"`). Only `Label` is
deserialized, so per #153 there is nowhere for matched content to leak;
the tests assert it against a real response body.
Tests: unit coverage for 2xx block/pass, 4xx, 5xx, timeout, malformed JSON
and a header-less business error; an E2E asserting one log line carries both
ids, for the non-streaming and streamed-output paths.
Fixes api7/AISIX-Cloud#1060
📝 WalkthroughWalkthroughAliyun moderation now records sanitized upstream diagnostics across success and failure paths, including request IDs, business codes, risk levels, and labels. Proxy request spans are propagated through streaming responses and detached WebSocket tasks so guardrail logs retain gateway request correlation. ChangesAliyun diagnostics
Request-span correlation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant AliyunGuardrail
participant AliyunAPI
participant Logs
Client->>Proxy: send moderation request
Proxy->>AliyunGuardrail: invoke input or output guardrail
AliyunGuardrail->>AliyunAPI: submit moderation request
AliyunAPI-->>AliyunGuardrail: response headers and body
AliyunGuardrail->>Logs: record Aliyun and gateway request IDs
Proxy-->>Client: filtered response or content_filter error
sequenceDiagram
participant RequestMiddleware
participant StreamingHandler
participant StreamAdapter
participant GuardrailLogger
RequestMiddleware->>StreamingHandler: instrument handler with request span
StreamingHandler->>StreamAdapter: wrap deferred response stream
StreamAdapter->>GuardrailLogger: poll stream within request span
GuardrailLogger-->>StreamAdapter: emit correlated end-of-stream diagnostics
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aisix-guardrails/src/aliyun.rs (1)
256-269: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the timeout across response decoding.
tokio::time::timeoutonly wrapssend(), soresp.json().awaitstill runs outside the deadline and can hang after headers arrive. Keep the timeout classification for body-read failures too.🤖 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-guardrails/src/aliyun.rs` around lines 256 - 269, The timeout currently covers only the HTTP send operation; update the request flow around the response handling so response body decoding via resp.json().await also runs within self.timeout. Preserve the existing AliyunFailure::Timeout classification for deadlines and classify body-read failures consistently with the current I/O error path, while retaining diagnostics behavior.Source: Coding guidelines
🤖 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-guardrails/src/aliyun.rs`:
- Around line 277-300: Update the non-success response handling around the
status classification and the corresponding 1167-1216 flow to read the capped
body before branching on 429, 5xx, or other errors. Best-effort parse supported
JSON/XML envelopes and retain only RequestId, Code, and Message, with
authentication material redacted; log these structured allowlisted fields
instead of raw response_body, while preserving the existing AliyunFailure
classification and diagnostic returns.
- Around line 303-316: Update the successful `body.code == 200` handling in the
Aliyun response parsing flow to require a present, non-null, recognized
`RiskLevel` value of none, low, medium, or high; return
`AliyunFailure::MalformedResponse` for missing or unknown levels instead of
defaulting to clean. Apply the same validation in the additional response path
around the referenced logic, and add tests covering missing, null, and unknown
risk levels.
In `@crates/aisix-proxy/src/request_id.rs`:
- Around line 38-50: Update the response-header handling in the request ID
middleware around the extension ID and tracing span setup so the caller-facing
x-aisix-request-id always equals id. Replace the behavior that preserves a
handler-set differing header with unconditional stamping of the extension ID, or
validate and reject mismatches before preserving a header; keep the tracing span
bound to that same ID.
In `@tests/e2e/src/cases/guardrail-aliyun-request-id-e2e.test.ts`:
- Around line 310-348: Expand the request-correlation E2E coverage beyond the
existing chat SSE test to include Messages normalized and passthrough streams,
Responses passthrough and bridge streams, and realtime WebSocket sessions. For
each endpoint, verify the gateway request ID and the corresponding Aliyun
request ID are joined in emitted logs, preserving the existing skip/setup
behavior and using the appropriate streaming or session interaction.
- Around line 310-327: Make the streamed-output test self-contained by waiting
for configuration propagation before issuing its fetch request. Add the same
readiness probe used by the preceding input test within the streaming test, or
move that probe into the shared beforeAll setup, while preserving the existing
skip behavior and request assertions.
- Around line 330-335: Update the streamed output assertions following the
response body read in the e2e test to also verify that wire does not contain
gatewayRequestId, while preserving the existing error-event and RISK_WORDS
assertions.
---
Outside diff comments:
In `@crates/aisix-guardrails/src/aliyun.rs`:
- Around line 256-269: The timeout currently covers only the HTTP send
operation; update the request flow around the response handling so response body
decoding via resp.json().await also runs within self.timeout. Preserve the
existing AliyunFailure::Timeout classification for deadlines and classify
body-read failures consistently with the current I/O error path, while retaining
diagnostics behavior.
🪄 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: 8518d23a-27d3-4697-be92-d3ce27c3c2e9
📒 Files selected for processing (10)
crates/aisix-guardrails/src/aliyun.rscrates/aisix-obs/src/lib.rscrates/aisix-proxy/AGENTS.mdcrates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/realtime.rscrates/aisix-proxy/src/request_id.rscrates/aisix-proxy/src/responses.rscrates/aisix-proxy/src/responses_bridge.rstests/e2e/src/cases/guardrail-aliyun-request-id-e2e.test.ts
…ler's prompt back A wrong access-key SECRET makes green-cip answer `SignatureDoesNotMatch` with the whole StringToSign in `Message` — and the StringToSign embeds our percent-encoded `ServiceParameters`, i.e. the caller's prompt, alongside the AccessKey id. The 4xx path logged that body verbatim, so one fat-fingered credential wrote every moderated prompt into the gateway log. Verified against the live endpoint: a real AccessKey id with a wrong secret returns "Message":"Specified signature is not matched with our calculation. server string to sign is:POST&%2F&AccessKeyId%3DLTAI…%26ServiceParameters%3D%257B %2522content%2522%253A%2520%2522CANARY_USER_PROMPT…" Log only the error `Code` (`SignatureDoesNotMatch`, `InvalidAccessKeyId.NotFound`, `InvalidAction.NotFound`). It is a symbolic error class from a closed vocabulary, so it structurally cannot carry request content, and it still answers what #773 added the body log for: telling a wrong key from a wrong region/endpoint. The body is still read (capped) to extract the code, never logged. The code arrives in two shapes — JSON from the RPC layer, XML from the endpoint layer — so `extract_error_code` handles both, and yields nothing rather than a guess for an unrecognized body. Reported by CodeRabbit on #781. Its suggested fix (allowlist RequestId + Code + Message) would not have closed this: the leak is inside `Message`. Also from that review: - e2e: each case now waits for config propagation in `beforeAll` instead of inheriting the first test's wait, so any one runs alone. - e2e: assert the streamed 422 doesn't carry Aliyun's RequestId either. - e2e: cover /v1/messages streamed output — a second, structurally different generator, since a missing `in_request_span` fails silently per call site.
… the Code The 4xx path reused the crate's log-snippet cap (2 KiB) to read a body it now PARSES rather than logs, and those want different budgets: a snippet can stop anywhere, a parse cannot. `Code` is the last member of the error object, after the `Message` that quotes the StringToSign — so the bigger the caller's prompt, the further out it sits. Measured against the live endpoint: a 1 980-char Chinese prompt yields a 30 457-byte body with `Code` at offset 30 426 (the content is percent-encoded twice on the way in, ~15 bytes per source char). At 2 KiB the JSON truncated mid-`Message`, parsed as nothing, and `aliyun_code` came back empty — so the single most common misconfiguration, a wrong access-key secret, reported no diagnosis at all. The earlier fix only looked right because its test prompt was 28 chars. Give the parse its own 64 KiB budget, which covers the dispatcher's 2 000-char content cap even at 4 bytes per char. The body is still never logged; it is held transiently to pull one symbolic token out. Test: an error body big enough to carry a real prompt, with `Code` last as the live endpoint places it. It fails at the old cap.
This was referenced Aug 4, 2026
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
1 participant
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.
Problem
The
aliyun_text_moderationguardrail dropped everything Aliyun reported about a moderation call.AliyunResponsenever deserializedRequestId, so a caller's422 content_filtercould not be traced to a record in the Aliyun console.Two things had to be true to fix that, and neither was:
RequestIdhad to be captured and logged.x-aisix-request-idthe caller holds — otherwise the operator has an id they cannot look anything up with.Where the RequestId comes from
Read
x-acs-request-idoff the response headers, falling back to the body'sRequestId.Verified against the live green-cip endpoint: the header is present on 2xx, on a JSON business error, and on 400/404 responses whose body is XML — it survives exactly the cases where the body is unreachable. The body alone would not, because Aliyun types
Codeinconsistently:Code200,400)"InvalidAccessKeyId.NotFound")So the documented shape does not deserialize on the error path. Only the HTTP-200 shape is parsed; the error shape keeps its existing raw capped-body log, now with the header-sourced id alongside.
Timeouts and connection failures yield no id at all — those log an empty
aliyun_request_idplus the failure bucket, so "Aliyun never answered" stays distinguishable from "we forgot to log it". An HTTP 200 with a non-JSON body now reportsMalformedResponserather than being bucketed asServerError; the two want different fixes, and the bypass tagaliyun_bad_responseis new.Correlation
Nothing in the data plane put a
request_idon a log line.ensure_request_idminted the id for the response header and telemetry, but no tracing span carried it — so all 38 guardrail log sites, across all six providers, were uncorrelated. This is a whole-class gap, not an Aliyun one.ensure_request_idnow opens arequest{request_id=…}span that every guardrail inherits. Two places fall outside it and re-attach explicitly:The invariant is recorded in a new
crates/aisix-proxy/AGENTS.md, because forgetting it on a future stream builder fails silently.Log searchability
The fmt subscriber colorized unconditionally. On a piped stderr — every real deployment — the ANSI escapes land between a field's name and its value, so
grep 'aliyun_request_id=<id>'matched nothing and fields were searchable only by bare value. Colorize only when stderr is a terminal.Log-format change (worth a release note)
Two visible changes to what the gateway writes to stderr, both intended:
request{request_id=…}:prefix, between the level and the target. A downstream parser anchored on the target still matches; one anchored onlevel target:as adjacent tokens would need adjusting.field=valuestarts working.Behavior for callers
Unchanged. Aliyun's
RequestIdis deliberately not exposed in the response envelope or a header: callers getx-aisix-request-idand quote it, operators resolve the rest from the log. This discloses no moderation vendor and adds no wire contract to maintain.Content safety
Two leaks, one of them live before this PR.
The moderation result. A real
highresponse echoes the offending text back:{ "Label": "inappropriate_oral", "RiskWords": "傻逼,弄死你,死你全家", "RiskPositions": [{ "StartPos": 3, "EndPos": 5, "RiskWord": "傻逼" }] }Only
Labelis deserialized, so per #153 there is nowhere for matched content to leak.The 4xx error body — a pre-existing leak, found in review. A wrong access-key secret makes green-cip answer
SignatureDoesNotMatchquoting the whole StringToSign, which embeds our percent-encodedServiceParameters— the caller's prompt — plus the AccessKey id. RPC v1 signature errors echo the canonicalized request by design. The 4xx path (added in #773) logged that body verbatim, so one fat-fingered credential wrote every moderated prompt into the gateway log.Now only the error
Codeis logged (SignatureDoesNotMatch,InvalidAccessKeyId.NotFound,InvalidAction.NotFound). It is a symbolic error class from a closed vocabulary, so it structurally cannot carry request content, and it still answers what the body log was for: telling a wrong key from a wrong region/endpoint. The body is read (capped) to extract the code, never logged; the code arrives as JSON from the RPC layer and XML from the endpoint layer, so both shapes are handled, with nothing emitted for an unrecognized body.Extracting the code needs its own read budget, separate from the crate's 2 KiB log-snippet cap — a snippet can stop anywhere, a parse cannot.
Codeis the last member of the error object, sitting after the StringToSign echo, so the bigger the caller's prompt the further out it lands. Measured live: a 1 980-char Chinese prompt gives a 30 457-byte body withCodeat offset 30 426 (the content is percent-encoded twice inbound, ~15 bytes per source char). At 2 KiB the JSON truncated mid-Messageandaliyun_codecame back empty — i.e. the most common misconfiguration would have reported no diagnosis. The parse budget is 64 KiB, covering the 2 000-char content cap at 4 bytes per char.What an operator now sees
```text
INFO request{request_id=12179fb5-5c3c-4a2a-a411-d73746379cc6}: aisix_guardrails::aliyun:
aliyun text moderation blocked content row=aliyun-live-guard service="llm_query_moderation"
aliyun_request_id=019F6EF3-6F9A-5A25-9EED-256CB0E26448 aliyun_code=200
aliyun_risk_level=high aliyun_labels=inappropriate_oral,inappropriate_profanity,violent_incidents
```
A block logs at
info(the default level); a clean pass logs the same fields atdebug, so a per-request line is not spent by default.Tests
SignatureDoesNotMatchbody from a live response, asserting the canary prompt / AccessKey id / StringToSign never reach a log; plusextract_error_codeagainst both live error shapes.guardrail-aliyun-request-id-e2e.test.ts): asserts one log line carries both ids — two lines each holding one would not let an operator join them — across the non-streaming path, chat SSE, and /v1/messages streamed output (a second, structurally different generator).--ignored) now also asserts the real endpoint yields the diagnostics we log.Baseline
LiteLLM has no Aliyun guardrail, so there is no direct baseline. Against its general moderation guardrails, all three decisions align with or exceed it: its Bedrock hook redacts matched spans while keeping labels (
redact_nested_match_and_regex_keys, "only non-sensitive labels + scores (no offsets / raw input)"); it correlates guardrail events via a request-root span; and it does not expose guardrail providers' request ids to callers.Docs: paired PR against api7/docs.
Fixes api7/AISIX-Cloud#1060
Fixes api7/AISIX-Cloud#1092
Summary by CodeRabbit