docs(schemas): describe multi-value handling on forward_client_headers - #1138
Conversation
`forward_client_headers` appears on four resources, and its handling of a header the caller sent more than once was described nowhere a user reads. The two code paths behind the field also differ, and the difference was equally undocumented: - `provider_key`, `mcp_server` and `a2a_agent` resolve the forwarded set through `resolve_forwarded_client_headers`, which walks `HeaderMap::keys()` and takes `get(name)` — the first value only. - `passthrough_route` builds its outbound request by iterating the inbound `HeaderMap` directly, which yields one pair per value, and appends each — every value is preserved. Each field description now states its own surface's behavior, so all four are accurate rather than uniform. The `provider_key` / `mcp_server` / `a2a_agent` text also names the case an operator is most likely to meet it in: an HTTP/2 caller may split `cookie` across several header fields, and only the first is forwarded. Doc comments only, plus the schemas regenerated from them with `cargo run -p aisix-core --bin dump-schema`. No forwarding behavior changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (14)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. 📝 WalkthroughWalkthroughThe change documents repeated-header forwarding semantics, adds regression coverage for first-value cookie forwarding, and verifies that passthrough routes preserve repeated caller header values. ChangesForwarded header semantics
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This clarifies repeated-header forwarding semantics across resource types and adds regression coverage without changing runtime behavior; no current merge-readiness risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: E2e Test Quality ReviewExplanation Blocking: the PR does not add E2E coverage for the new multi-value forwarding contract. The resolver test directly calls Resolution Add E2E scenarios to the existing
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are documentation-only and accurately describe the existing, verified forwarding behavior on each surface.
Pull request overview
Documents how forward_client_headers behaves when the inbound request contains repeated headers, making an existing but previously undocumented difference between resolver-backed resources and passthrough_route explicit to operators.
Changes:
- Clarify that
provider_key,mcp_server, anda2a_agentforward only the first value of a repeated inbound header (resolver-backed behavior). - Clarify that
passthrough_routepreserves all values for repeated inbound headers (direct header iteration behavior). - Regenerate the published resource schemas to reflect the updated doc comments.
File summaries
| File | Description |
|---|---|
| schemas/resources/provider_key.schema.json | Updates forward_client_headers description to state repeated headers forward first value only. |
| schemas/resources/mcp_server.schema.json | Updates forward_client_headers description to state repeated headers forward first value only. |
| schemas/resources/a2a_agent.schema.json | Updates forward_client_headers description to state repeated headers forward first value only. |
| schemas/resources/passthrough_route.schema.json | Updates forward_client_headers description to state repeated headers preserve all values. |
| crates/aisix-core/src/models/provider_key.rs | Adds the repeated-header forwarding semantics to the model doc comment. |
| crates/aisix-core/src/models/mcp_server.rs | Adds the repeated-header forwarding semantics to the model doc comment. |
| crates/aisix-core/src/models/a2a_agent.rs | Adds the repeated-header forwarding semantics to the model doc comment. |
| crates/aisix-core/src/models/passthrough_route.rs | Adds the repeated-header forwarding semantics to the model doc comment. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The previous commit turned a code comment into a public contract on two planes, with nothing keeping it true. Every fixture in the `forwarded_headers` tests is built with `HeaderMap::insert` via `map()`, which cannot express a header the caller sent twice, so no test went red if `resolve_forwarded_client_headers` stopped collapsing to the first value — and none covered the passthrough path's opposite guarantee. Two tests, one per rule, each verified to fail without the behavior it pins: switching the resolver to per-value iteration turns the first into `["cookie", "cookie"]`, and switching the passthrough loop to `keys()` + `get()` drops `second` from the second. The passthrough case deliberately repeats a header the ProviderKey strips, so it covers the strip-override branch rather than the plain default-forward one. Also narrows the injection comment above the credential inject: "the wire stays single-valued" was true of what that block does and is now easy to read as a claim about the whole request, which the passthrough description explicitly contradicts.
…eaders-first-value
#1137 landed `schemas/resources-lenient/` on main after this branch was cut. Those files are generated from the same doc comments this branch edits, so the merge left them carrying the pre-change descriptions — which the drift gate and the two characterization tests that compare the published set against what the loader compiles both catch. Regenerated with `cargo run -p aisix-core --bin dump-schema`. The four changed lines are the same four descriptions, in the lenient copies.
Problem
forward_client_headerslets an operator name inbound client headers that must reach the upstream. It appears on four resources —provider_key,mcp_server,a2a_agentandpassthrough_route— and what it does with a header the caller sent more than once was described nowhere a user reads. Release QA hit this black box: a caller sendingx-multi: oneandx-multi: twowithforward_client_headers: ["x-multi"]saw the upstream agent receivex-multi: onealone, with nothing in the field's description to say so.The two paths differ, and this describes the difference rather than reconciling it
Auditing the whole family turned up a second, equally undocumented fact: the four surfaces do not agree, because two different code paths implement the forwarding.
provider_key,mcp_serveranda2a_agentresolve the forwarded set throughaisix_core::resolve_forwarded_client_headers, which walksHeaderMap::keys()and readsclient.get(name).keys()yields each name once andget()returns the first value, so a repeated header forwards its first value only.passthrough_routebuilds its outbound request incrates/aisix-proxy/src/passthrough_route.rsby iterating the inboundHeaderMapdirectly.&HeaderMapyields one pair per value, andRequestBuilder::headerappends rather than replaces, so every value is preserved.Confirmed with a throwaway probe over the same two-value
HeaderMap: the resolver produced["x-multi=one"], the passthrough iteration produced["x-multi=one", "x-multi=two"].This PR describes each surface's actual behavior and deliberately does not reconcile the two paths. Making them agree would change forwarding behavior on a surface released in v1.0.0, which is a separate decision and not one a documentation fix should take. The divergence below is therefore intentional and recorded, not an oversight for a later reader to "clean up". No forwarding behavior changes here.
What changed
Four field descriptions, written as doc comments on the models under
crates/aisix-core/src/models/and regenerated intoschemas/resources/withcargo run -p aisix-core --bin dump-schema. The generated JSON was not hand-edited.provider_key,mcp_server,a2a_agent:passthrough_route:The
cookiesentence names the case an operator is most likely to meet this in rather than leaving it as an abstraction. The gateway terminates inbound HTTP/2 (ALPNh2, plus h2c preface sniffing), an HTTP/2 client may legitimately splitcookieacross several header fields, and theh2crate does not reassemble them — so a forwardedcookiefrom such a caller loses all but the first field on the three resolver-backed surfaces. That is a known limitation, stated so an operator can see it before relying on it, not something changed here.The same two strings, mapped to the same surfaces, are going into
openapi/cp-admin.yamlon the control-plane side, so the two planes describe each surface identically.Testing
No behavioural test applies: nothing about runtime behaviour changes. The four edits are doc comments, and the only other change is the schema JSON regenerated from them, so there is no state in which this PR makes a request behave differently — a fail-before / pass-after test would have nothing to assert against.
What was verified instead:
git diff -U0 schemas/is exactly four changed lines, onedescriptionper surface, each with the new paragraph inserted and the rest byte-identical.schema-driftCI gate reproduced locally: re-runningdump-schemaafter the commit leavesgit diff --exit-code schemas/clean.cargo test -p aisix-core --lib models::— 467 passed, andcargo fmt --checkclean.Two tests were added, one per rule, because the descriptions above are now a public contract on two planes and nothing kept them true. Every fixture in the
forwarded_headerstest module is built withHeaderMap::insert, which cannot express a header the caller sent twice, so no existing test went red if the resolver stopped collapsing to the first value; the passthrough guarantee had no coverage either. Each new test was verified to fail without the behavior it pins — switching the resolver to per-value iteration turns the first into["cookie", "cookie"], and switching the passthrough loop tokeys()+get()drops the second value from the other. The passthrough case deliberately repeats a header the ProviderKey strips, so it covers the strip-override branch rather than the plain default-forward one.One comment was narrowed as a consequence: above the credential injection in
passthrough_route.rs, "the wire stays single-valued" was true of what that block does, but reads as a claim about the whole request once the field description promises the opposite. It now says which of the two it means.schemas/resources-lenient/arrived on main from #1137 after this branch was cut. Those files generate from the same doc comments, so main was merged in anddump-schemare-run; the four extra changed lines are the same four descriptions in the lenient copies.Summary by CodeRabbit
Bug Fixes
Documentation