Commit d55ab6b
authored
[TT-17921] Improve header handling for GraphQL-based APIs (#8602)
## Summary
Values that belong to a single request — the round tripper, the resolved
upstream headers, the
upstream response — were being written into, or keyed on, state that
outlives the request: the
shared per-API `http.Client`, the plan cache, and the upstream
connection-reuse key. Under
concurrency the result was not deterministic: which request's values a
fetch used depended on
timing.
This makes that state request-scoped, and fixes the header handling that
depended on it. Seven
commits, each independently revertable.
`internal/graphengine` goes from 22 to 76 test functions (273 including
subtests), green under
`-race -count=10`.
Behavioural detail and reproduction steps are on the ticket.
## What changed
### scope transports to requests
`reverseProxyPreHandlerV1/V2.PreHandle` assigned `params.RoundTripper`
onto the per-API
`httpClient.Transport` on every request, so concurrent requests
overwrote each other's. The round
tripper and headers config now travel in the request context
(`SetGraphQLEngineTransportContextValue`), and
`GraphQLEngineTransport.RoundTrip` builds a
per-request view from them, falling back to the client's own transport.
### keep subscriptions alive, isolate proxy-only responses
Three defects, two of them introduced by the commit above:
- **WebSocket subscriptions never delivered.** The handovers passed
`WithContext(params.OutRequest.Context())`, but `net/http` calls
`w.cancelCtx()` as soon as
`ServeHTTP` returns — before the hijacked check in `conn.serve` — and
the gateway returns as soon
as the connection is hijacked. The subscription context was cancelled
before anything was sent.
`subscriptionRequestContext` roots the subscription at the long-lived
engine spec context while
carrying the opening request's transport values across. EngineV1 passes
no context: its legacy
data sources drop it, so inheriting the request's cancellation could
only break the subscription.
- **`handleProxyOnly` rewrote the outgoing method**, turning a
proxy-only upstream WebSocket
handshake into a POST, which WebSocket servers reject. Handshakes now
stay `GET`.
- **`GraphQLProxyOnlyContextValues.upstreamResponse` was
unsynchronised.** The transport writes it
on the resolver's trigger goroutine while the engine reads it (~1 in 20
under `-race`). Now
behind a mutex.
### cover the request isolation matrix
`credential_isolation_test.go`: proxy-only, UDG and supergraph × HTTP,
SSE, `graphql-ws` and
`graphql-transport-ws` × EngineV2 and EngineV3. Each test drives more
than one request through one
engine and asserts what each upstream connection actually received, with
negative controls
(`strip_auth_data`, a custom auth header name, a request that sets no
header after one that did),
20-request concurrent bursts, a per-request round tripper test that
shared mutable transport state
cannot pass, and the client WebSocket-upgrade path that the regression
above had slipped through.
### move to the fixed graphql-go-tools, cover the cold path
Both module pins move to the library branch; `coder/websocket` becomes a
direct dependency. Two
races are fixed upstream — v1 `Schema.Hash` writing `s.hash` on first
use, and v2
`ResolveGraphQLResponse` writing `Info` on the cached plan — so the
cold-start concurrent burst
becomes permanent coverage instead of something a warm-up had to work
around.
### resolve dynamic upstream headers before the fetch is keyed
The v1 header modifier merged Tyk's additional headers and stopped,
leaving `tykVariableReplacer`
injected but never read, so `$tyk_context.*` in an upstream header
reached the fetch unresolved on
config version 2. That matters for more than the value that arrives
upstream: the library applies
the modifier before it derives the key that groups equivalent
subscriptions onto one upstream
connection (`connectionKey` in the v1 subscription client,
`UniqueRequestID` in the v2 resolver).
An unresolved template is the same string for every request, so requests
that should have been
distinct were treated as equivalent and pooled together.
Resolving there is only correct now that the library moved header
modification to the execution
phase. TT-14357 (`adca34d46`) had removed it because the modifier ran at
plan post-process time and
the result was cached into the plan, and added
`variableReplaceRoundTripper` as the workaround.
That wrapper is retired here: it ran after the grouping key was derived,
it collapsed multi-value
headers via `Get`/`Set` on the way to the wire, and it expanded
variables in header values that did
not come from the API definition. Its one remaining job,
`request_headers_rewrite`, is resolved
once where the rules are built, from configuration only.
### restore the gateway round tripper for EngineV1 fetches
Regression from `cf506a971`, caught while verifying the above:
`TestGraphQL_InternalDataSource/graphql_engine_v1` fails at the branch
tip and passes at the base —
a config version 1 API with a `tyk://` data source answers 500. The
transport assignment that
commit removed was also the only thing giving EngineV1 the gateway round
tripper, and only that
round tripper can route `tyk://`. Since EngineV1's data sources drop the
execution context, the
round tripper is handed over as a mutex-guarded transport-level
fallback, used only when a request
carries none of its own. EngineV2 and EngineV3 stay fully
request-scoped.
That fallback is the one piece of transport state shared between
requests, and it is only sound
because the round tripper the gateway hands over no longer carries
request-specific state — the
wrapper retired in `4d67f9af4` did. **The two commits must not be
reverted independently**; both
sides carry comments saying so.
### forward a consumer header to the upstream once
Two writers put the consumer's headers on the upstream request and
neither knew about the other:
`propagateAuthHeaders` adds them to the fetch input when
`strip_auth_data` is off, and
`setProxyOnlyHeaders` then forwards every consumer header again with
`Header.Add`. With
`use_immutable_headers` off nothing removed the first copy, so a header
present in both arrived
duplicated. It went unnoticed because the wrapper retired in `4d67f9af4`
had been collapsing
repeated values with `Get`/`Set` since before the second writer was
added.
`setProxyOnlyHeaders` now skips a value the engine already placed there,
compared against a
snapshot taken before forwarding so a consumer that genuinely sent the
same value twice keeps both.
It also hoists the `use_immutable_headers` delete out of the value loop,
where it removed the value
added on the previous pass and left a multi-value consumer header
holding only its last value.
## Behaviour changes worth a reviewer's sign-off
1. **An API reload or release now ends that API's subscriptions.**
Previously they ran on
`context.Background()` and outlived the reload.
2. **No header value is resolved twice** on config version 2 or
3-preview. Values that do not come
from the API definition are no longer expanded, and a value that
resolved into another template
token no longer gets a second pass.
3. **An exact overlap between a configured `request_headers` value and a
consumer value now reaches
the upstream once**, not twice. Differing values still both go upstream
— TT-11990 / TT-12190
semantics are unchanged.
4. **Multi-value headers survive to the upstream.** `Accept-Encoding:
gzip, deflate, br` used to
arrive as `gzip` alone.
<!---TykTechnologies/jira-linter starts here-->
### Ticket Details
<details>
<summary>
<a href="https://tyktech.atlassian.net/browse/TT-17921" title="TT-17921"
target="_blank">TT-17921</a>
</summary>
| | |
|---------|----|
| Status | Merge |
| Summary | Federated supergraph reuses first caller’s JWT across users
and can share HTTP responses via single-flight |
Generated at: 2026-08-20 13:03:45
</details>
<!---TykTechnologies/jira-linter ends here-->1 parent 593a1a1 commit d55ab6b
22 files changed
Lines changed: 3068 additions & 75 deletions
File tree
- gateway
- internal/graphengine
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1089 | 1089 | | |
1090 | 1090 | | |
1091 | 1091 | | |
1092 | | - | |
1093 | | - | |
1094 | | - | |
1095 | | - | |
1096 | | - | |
1097 | | - | |
1098 | | - | |
1099 | | - | |
1100 | | - | |
1101 | | - | |
1102 | | - | |
1103 | | - | |
1104 | | - | |
1105 | | - | |
1106 | | - | |
1107 | 1092 | | |
1108 | 1093 | | |
1109 | 1094 | | |
1110 | 1095 | | |
1111 | 1096 | | |
1112 | 1097 | | |
| 1098 | + | |
| 1099 | + | |
| 1100 | + | |
| 1101 | + | |
| 1102 | + | |
| 1103 | + | |
| 1104 | + | |
| 1105 | + | |
| 1106 | + | |
| 1107 | + | |
| 1108 | + | |
1113 | 1109 | | |
1114 | 1110 | | |
1115 | 1111 | | |
1116 | 1112 | | |
1117 | | - | |
| 1113 | + | |
1118 | 1114 | | |
1119 | 1115 | | |
1120 | 1116 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1116 | 1116 | | |
1117 | 1117 | | |
1118 | 1118 | | |
1119 | | - | |
| 1119 | + | |
| 1120 | + | |
| 1121 | + | |
| 1122 | + | |
| 1123 | + | |
| 1124 | + | |
| 1125 | + | |
1120 | 1126 | | |
1121 | 1127 | | |
1122 | 1128 | | |
| |||
1195 | 1201 | | |
1196 | 1202 | | |
1197 | 1203 | | |
| 1204 | + | |
| 1205 | + | |
| 1206 | + | |
| 1207 | + | |
| 1208 | + | |
| 1209 | + | |
| 1210 | + | |
| 1211 | + | |
| 1212 | + | |
| 1213 | + | |
| 1214 | + | |
| 1215 | + | |
| 1216 | + | |
| 1217 | + | |
| 1218 | + | |
| 1219 | + | |
| 1220 | + | |
| 1221 | + | |
| 1222 | + | |
| 1223 | + | |
| 1224 | + | |
| 1225 | + | |
| 1226 | + | |
| 1227 | + | |
| 1228 | + | |
| 1229 | + | |
| 1230 | + | |
| 1231 | + | |
| 1232 | + | |
| 1233 | + | |
| 1234 | + | |
| 1235 | + | |
| 1236 | + | |
| 1237 | + | |
| 1238 | + | |
| 1239 | + | |
| 1240 | + | |
| 1241 | + | |
| 1242 | + | |
| 1243 | + | |
| 1244 | + | |
| 1245 | + | |
| 1246 | + | |
| 1247 | + | |
| 1248 | + | |
| 1249 | + | |
| 1250 | + | |
| 1251 | + | |
| 1252 | + | |
| 1253 | + | |
| 1254 | + | |
| 1255 | + | |
| 1256 | + | |
| 1257 | + | |
| 1258 | + | |
| 1259 | + | |
| 1260 | + | |
| 1261 | + | |
| 1262 | + | |
| 1263 | + | |
| 1264 | + | |
| 1265 | + | |
| 1266 | + | |
| 1267 | + | |
| 1268 | + | |
| 1269 | + | |
| 1270 | + | |
| 1271 | + | |
| 1272 | + | |
| 1273 | + | |
| 1274 | + | |
| 1275 | + | |
| 1276 | + | |
| 1277 | + | |
| 1278 | + | |
| 1279 | + | |
| 1280 | + | |
| 1281 | + | |
| 1282 | + | |
| 1283 | + | |
| 1284 | + | |
| 1285 | + | |
| 1286 | + | |
| 1287 | + | |
| 1288 | + | |
| 1289 | + | |
| 1290 | + | |
| 1291 | + | |
| 1292 | + | |
| 1293 | + | |
| 1294 | + | |
| 1295 | + | |
| 1296 | + | |
| 1297 | + | |
| 1298 | + | |
| 1299 | + | |
| 1300 | + | |
| 1301 | + | |
| 1302 | + | |
| 1303 | + | |
| 1304 | + | |
1198 | 1305 | | |
1199 | 1306 | | |
1200 | 1307 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
28 | | - | |
| 28 | + | |
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
| |||
97 | 97 | | |
98 | 98 | | |
99 | 99 | | |
100 | | - | |
| 100 | + | |
101 | 101 | | |
102 | 102 | | |
103 | 103 | | |
| 104 | + | |
104 | 105 | | |
105 | 106 | | |
106 | 107 | | |
| |||
270 | 271 | | |
271 | 272 | | |
272 | 273 | | |
273 | | - | |
274 | 274 | | |
275 | 275 | | |
276 | 276 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
177 | 177 | | |
178 | 178 | | |
179 | 179 | | |
180 | | - | |
181 | | - | |
182 | | - | |
183 | | - | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
184 | 184 | | |
185 | 185 | | |
186 | 186 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
| 6 | + | |
6 | 7 | | |
7 | 8 | | |
8 | 9 | | |
| |||
16 | 17 | | |
17 | 18 | | |
18 | 19 | | |
| 20 | + | |
| 21 | + | |
19 | 22 | | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
20 | 71 | | |
21 | 72 | | |
22 | 73 | | |
23 | | - | |
24 | 74 | | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
25 | 94 | | |
26 | 95 | | |
27 | 96 | | |
| |||
0 commit comments