feat(router): update engine, enable grpc header forwarding#2524
feat(router): update engine, enable grpc header forwarding#2524
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds gRPC header-propagation tests, introduces per-subgraph gRPC unary interceptor wiring in test environment, centralizes ignored-header prefix handling (including "Grpc-"), expands the set of non-forwarded headers, and pins a more specific pre-release revision of github.com/wundergraph/graphql-go-tools/v2 in two go.mod files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dkorittki
changed the title
dkorittki
changed the title
❌ Internal Query Planner CI checks failedThe Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2524 +/- ##
==========================================
+ Coverage 61.00% 61.68% +0.67%
==========================================
Files 239 239
Lines 25416 25423 +7
==========================================
+ Hits 15506 15681 +175
+ Misses 8589 8422 -167
+ Partials 1321 1320 -1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
router-tests/testenv/testenv.go (1)
390-395:GRPCInterceptoronSubgraphConfigis silently ignored for non-gRPC subgraphs.Only
cfg.Subgraphs.Projects.GRPCInterceptoris ever wired intomakeSafeGRPCServer. SettingGRPCInterceptoronEmployees,Family, etc. compiles fine but has no effect. Consider adding a doc comment on the field clarifying this, or scoping it to a dedicated gRPC-specific config struct if more gRPC subgraphs are expected in the future.📝 Proposed doc comment
type SubgraphConfig struct { Middleware func(http.Handler) http.Handler - GRPCInterceptor grpc.UnaryServerInterceptor + // GRPCInterceptor is only effective for gRPC-backed subgraphs (currently only Projects). + // Setting it on HTTP subgraphs has no effect. + GRPCInterceptor grpc.UnaryServerInterceptor Delay time.Duration CloseOnStart bool }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/testenv/testenv.go` around lines 390 - 395, The SubgraphConfig.GRPCInterceptor field is defined but only the Projects instance is actually passed into makeSafeGRPCServer so interceptors on other subgraphs (e.g., Employees, Family) are ignored; either document that GRPCInterceptor only applies to the Projects subgraph or refactor by creating a gRPC-specific config (e.g., GRPCSubgraphConfig with GRPCInterceptor) and use that type for subgraphs that start gRPC servers, and update the server bootstrap code (where makeSafeGRPCServer is invoked) to read the interceptor from the new gRPC-specific config instead of the generic SubgraphConfig.GRPCInterceptor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router-tests/grpc_subgraph_test.go`:
- Line 287: Fix the typo in the test name string passed to t.Run in
grpc_subgraph_test.go: change "Should sent http headers as gRPC metadata to
subgraphs" to "Should send http headers as gRPC metadata to subgraphs" so the
test description is grammatically correct; update the string literal inside the
t.Run call accordingly.
- Around line 490-493: Update the misleading comment above the content-type
assertion in the test that checks captured headers (referencing the captured
variable and the require.Equal asserting "application/grpc") to explain that the
original "content-type: application/json" is removed by the router's
unsafe-header filter and never reaches gRPC metadata, and that the gRPC
transport independently sets "content-type: application/grpc" — replace the
phrasing "overwritten by grpc client" with this accurate behavior.
In `@router/go.mod`:
- Line 34: The go.mod currently pins a pseudo-version for
github.com/wundergraph/graphql-go-tools
(v2.0.0-rc.250.0.20260217082956-accd82298d12); add a clear revert-tracking note
so this temporary fork commit isn't forgotten: insert a TODO comment in the repo
near the go.mod change (or create a tracking issue and reference its ID) that
names the module github.com/wundergraph/graphql-go-tools, the exact
pseudo-version string, and links to wundergraph/graphql-go-tools#1382, and
include an action to revert to the official upstream released version once that
PR is merged.
---
Nitpick comments:
In `@router-tests/testenv/testenv.go`:
- Around line 390-395: The SubgraphConfig.GRPCInterceptor field is defined but
only the Projects instance is actually passed into makeSafeGRPCServer so
interceptors on other subgraphs (e.g., Employees, Family) are ignored; either
document that GRPCInterceptor only applies to the Projects subgraph or refactor
by creating a gRPC-specific config (e.g., GRPCSubgraphConfig with
GRPCInterceptor) and use that type for subgraphs that start gRPC servers, and
update the server bootstrap code (where makeSafeGRPCServer is invoked) to read
the interceptor from the new gRPC-specific config instead of the generic
SubgraphConfig.GRPCInterceptor.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router-tests/grpc_subgraph_test.go (1)
423-537: Consider addingrequire.NotEmptyforx-customalongside therequire.Equalfor symmetry, and asserttevalue nuance.Two small observations on this comprehensive test:
Te: trailersis the only TE value permitted in HTTP/2 and is actively used by gRPC. If the router ever decides to allowlist it, this assertion would break. A brief inline comment explaining why it's expected to be stripped (it's classified as hop-by-hop in the router's blocklist) would future-proof the test.The test sends
Host: evil.example.com(line 459) and asserts it's absent. Because Go'snet/httpmovesHostintoRequest.Hostand removes it fromRequest.Header, this assertion passes for a reason different from the router's filtering. The existing comment on line 458 already hints at this, which is good — just noting it for clarity.Neither of these is blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/grpc_subgraph_test.go` around lines 423 - 537, Add a symmetry check and clarifying comment: after the existing require.Equal(t, []string{"value"}, captured.Get("x-custom")) add require.NotEmpty(t, captured.Get("x-custom")) to assert presence explicitly (referencing the captured metadata variable), and next to the TE assertion (the "Te": []string{"trailers"} header and the require.Empty(t, captured.Get("te")) line) insert a brief inline comment explaining that although "trailers" is the only TE value permitted in HTTP/2 and used by gRPC, the router currently classifies TE as hop-by-hop and strips it (so the test expects it absent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router-tests/grpc_subgraph_test.go`:
- Around line 423-537: Add a symmetry check and clarifying comment: after the
existing require.Equal(t, []string{"value"}, captured.Get("x-custom")) add
require.NotEmpty(t, captured.Get("x-custom")) to assert presence explicitly
(referencing the captured metadata variable), and next to the TE assertion (the
"Te": []string{"trailers"} header and the require.Empty(t, captured.Get("te"))
line) insert a brief inline comment explaining that although "trailers" is the
only TE value permitted in HTTP/2 and used by gRPC, the router currently
classifies TE as hop-by-hop and strips it (so the test expects it absent).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/core/header_rule_engine.go (1)
78-89: Consider canonicalizingnameonce for both lookups.The exact-match check (line 79) is case-sensitive while the prefix check on line 84 first canonicalizes. This means a non-canonical user-configured header name such as
rule.Named = "content-type"would not be caught by the exact-match guard (since"content-type" ≠ "Content-Type"inignoredHeaders). The behavior is identical to the pre-existingslices.Containscall this replaced, but the helper is a good opportunity to close the gap.♻️ Proposed refactor
func isIgnoredHeader(name string) bool { - if slices.Contains(ignoredHeaders, name) { + canonicalName := http.CanonicalHeaderKey(name) + if slices.Contains(ignoredHeaders, canonicalName) { return true } - canonicalName := http.CanonicalHeaderKey(name) for _, prefix := range ignoredHeaderPrefixes { if strings.HasPrefix(canonicalName, prefix) { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/header_rule_engine.go` around lines 78 - 89, The exact-match check in isIgnoredHeader currently compares the raw name against ignoredHeaders before canonicalizing, causing case-mismatches; canonicalize name once (call http.CanonicalHeaderKey(name) into canonicalName at top) and use canonicalName for the slices.Contains check against ignoredHeaders and for the prefix loop against ignoredHeaderPrefixes so both exact and prefix checks use the same canonical form (referencing isIgnoredHeader, ignoredHeaders, and ignoredHeaderPrefixes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/core/header_rule_engine.go`:
- Around line 78-89: The exact-match check in isIgnoredHeader currently compares
the raw name against ignoredHeaders before canonicalizing, causing
case-mismatches; canonicalize name once (call http.CanonicalHeaderKey(name) into
canonicalName at top) and use canonicalName for the slices.Contains check
against ignoredHeaders and for the prefix loop against ignoredHeaderPrefixes so
both exact and prefix checks use the same canonical form (referencing
isIgnoredHeader, ignoredHeaders, and ignoredHeaderPrefixes).
There was a problem hiding this comment.
🧹 Nitpick comments (3)
router/core/header_rule_engine.go (2)
41-54:SkippedHeaderslookup bypasses canonicalization — minor correctness gap.
http.CanonicalHeaderKeyis computed after theSkippedHeadersmap lookup (line 44 vs. line 47). Because all keys inSkippedHeadersare canonical (e.g.,"Content-Type"), a non-canonicalnamefrom a user-configured rule (e.g.,rule.Named = "content-type") would miss the map check and fall through to the prefix loop — incorrectly returningfalsefor a header that should be ignored.This doesn't affect the PR's grpc-* goal (the prefix check at line 48 canonicalizes correctly), but it creates a latent inconsistency in the helper. Computing
canonicalNameonce up front and using it for both checks eliminates the inconsistency:♻️ Proposed fix — canonicalize once before both checks
func isIgnoredHeader(name string) bool { - if _, ok := headers.SkippedHeaders[name]; ok { + canonicalName := http.CanonicalHeaderKey(name) + if _, ok := headers.SkippedHeaders[canonicalName]; ok { return true } - canonicalName := http.CanonicalHeaderKey(name) for _, prefix := range headers.IgnoredHeaderPrefixes { if strings.HasPrefix(canonicalName, prefix) { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/header_rule_engine.go` around lines 41 - 54, In isIgnoredHeader, canonicalize the input once (via http.CanonicalHeaderKey) and use that canonicalName for both the headers.SkippedHeaders map lookup and the prefix checks against headers.IgnoredHeaderPrefixes; replace the initial direct lookup of headers.SkippedHeaders[name] with headers.SkippedHeaders[canonicalName] so non-canonical input (e.g., "content-type") correctly matches the canonical keys.
880-908:PropagatedHeadersdoesn't filter ignored headers — minor cache key imprecision.
PropagatedHeadersreturns every header name/pattern from the rules, including those thatisIgnoredHeaderwould block at propagation time (e.g.,grpc-*headers explicitly listed in aNamedrule). Callers that use this output for plan cache keys will include ignored headers in the key, causing unnecessary cache misses when those headers change.This is a minor efficiency concern, not a correctness issue. Filtering out
isIgnoredHeaderentries here would keep planning and transport behaviour aligned:♻️ Sketch of the fix
case config.HeaderRuleOperationPropagate: if rule.Matching != "" { // ... headerNameRegexps = append(headerNameRegexps, graphql_datasource.RegularExpression{...}) } else if rule.Named != "" { + if isIgnoredHeader(rule.Named) { + continue + } headerNames = append(headerNames, rule.Named) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/header_rule_engine.go` around lines 880 - 908, PropagatedHeaders currently returns Named and Matching rules even when those headers are ignored at propagation time; update PropagatedHeaders to filter out ignored entries by using the existing isIgnoredHeader function: skip appending rule.Named when isIgnoredHeader(rule.Named) is true, and for rule.Matching skip appending the compiled regexp if it would match any header that isIgnoredHeader would ignore (implement a small helper like isIgnoredHeaderRegexp or test the compiled regexp against the set of ignored header examples used by isIgnoredHeader); keep the rest of the validation and error handling in PropagatedHeaders unchanged.router/internal/headers/headers.go (1)
40-43: LGTM —"Grpc-"is the correct canonical prefix.
http.CanonicalHeaderKey("grpc-status")→"Grpc-Status", so all case variants (grpc-,GRPC-,Grpc-) will be matched bystrings.HasPrefix(canonicalName, "Grpc-")inisIgnoredHeader. The new variable is well-placed and correctly documented.One minor point: like
SkippedHeaders,IgnoredHeaderPrefixesis an exported mutablevar. If any future code accidentally appends to the slice (e.g.,headers.IgnoredHeaderPrefixes = append(...)), the shared state would silently change. Consider documenting it as "do not modify at runtime" or making it a local constant if external packages don't need to mutate it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/internal/headers/headers.go` around lines 40 - 43, IgnoredHeaderPrefixes is an exported mutable var which can be accidentally modified at runtime; change it to an immutable API by replacing the exported variable with an exported accessor function that returns a fresh copy (e.g., add func IgnoredHeaderPrefixes() []string { return []string{"Grpc-"} }) and make the underlying slice unexported if kept, or remove the var entirely; update any callers (and ensure isIgnoredHeader uses the accessor) so callers get a safe copy instead of mutating shared state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@router/go.mod`:
- Line 34: PR approved: the go.mod pseudo-version for module
github.com/wundergraph/graphql-go-tools/v2 was intentionally bumped to
v2.0.0-rc.251.0.20260218134910-9ce5aa544c05 to pick up the upstream commit prior
to graphql-go-tools#1382; no code changes required now, but retain this version
in go.mod and add a TODO note to revert/update it after upstream PR `#1382` is
merged.
---
Nitpick comments:
In `@router/core/header_rule_engine.go`:
- Around line 41-54: In isIgnoredHeader, canonicalize the input once (via
http.CanonicalHeaderKey) and use that canonicalName for both the
headers.SkippedHeaders map lookup and the prefix checks against
headers.IgnoredHeaderPrefixes; replace the initial direct lookup of
headers.SkippedHeaders[name] with headers.SkippedHeaders[canonicalName] so
non-canonical input (e.g., "content-type") correctly matches the canonical keys.
- Around line 880-908: PropagatedHeaders currently returns Named and Matching
rules even when those headers are ignored at propagation time; update
PropagatedHeaders to filter out ignored entries by using the existing
isIgnoredHeader function: skip appending rule.Named when
isIgnoredHeader(rule.Named) is true, and for rule.Matching skip appending the
compiled regexp if it would match any header that isIgnoredHeader would ignore
(implement a small helper like isIgnoredHeaderRegexp or test the compiled regexp
against the set of ignored header examples used by isIgnoredHeader); keep the
rest of the validation and error handling in PropagatedHeaders unchanged.
In `@router/internal/headers/headers.go`:
- Around line 40-43: IgnoredHeaderPrefixes is an exported mutable var which can
be accidentally modified at runtime; change it to an immutable API by replacing
the exported variable with an exported accessor function that returns a fresh
copy (e.g., add func IgnoredHeaderPrefixes() []string { return []string{"Grpc-"}
}) and make the underlying slice unexported if kept, or remove the var entirely;
update any callers (and ensure isIgnoredHeader uses the accessor) so callers get
a safe copy instead of mutating shared state.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/internal/headers/headers.go (1)
21-28:⚠️ Potential issue | 🟠 MajorSilent breaking change for existing propagation rules targeting Content Negotiation headers.
Adding
Accept,Content-Type,Content-Encoding,Content-Length,Accept-Encoding, andAccept-CharsettoSkippedHeadersmeans any existing operator config withpropagate: named: Accept(or similar) will now silently do nothing at runtime. There is no warning log emitted fromapplyRequestRuleToHeaderwhen a rule is dropped due toisIgnoredHeader. Operators who forward these headers for custom upstreams will have their configs silently broken with no diagnostic.Consider adding a startup-time warning (or at least a comment in config docs) when a configured propagation rule targets a header that will always be ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/internal/headers/headers.go` around lines 21 - 28, The change added headers (Accept, Content-Type, Content-Encoding, Content-Length, Accept-Encoding, Accept-Charset) to SkippedHeaders which causes rules targeting those names to be silently ignored; update startup validation to scan configured propagation rules and log a warning when a rule references any header in SkippedHeaders. Implement this by adding a startup check that iterates over configured request/response propagation rules, uses the same isIgnoredHeader(headerName) logic (or references the SkippedHeaders set) to detect dropped rules, and emits a clear warning (including the rule identifier and header name) via the existing logger so operators are informed; keep applyRequestRuleToHeader unchanged except now these startup warnings surface dropped rules early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/core/header_rule_engine.go`:
- Around line 48-60: The map lookup in isIgnoredHeader uses the raw name, so
headers.SkippedHeaders can be missed for non-canonical inputs; change the lookup
to use the already-computed canonicalName (i.e. check
headers.SkippedHeaders[canonicalName] instead of headers.SkippedHeaders[name])
so Title-Case keys like "Content-Type" match regardless of incoming case; keep
the existing prefix loop on canonicalName as-is and run/update tests for
Accept/Content-Type behavior.
---
Outside diff comments:
In `@router/internal/headers/headers.go`:
- Around line 21-28: The change added headers (Accept, Content-Type,
Content-Encoding, Content-Length, Accept-Encoding, Accept-Charset) to
SkippedHeaders which causes rules targeting those names to be silently ignored;
update startup validation to scan configured propagation rules and log a warning
when a rule references any header in SkippedHeaders. Implement this by adding a
startup check that iterates over configured request/response propagation rules,
uses the same isIgnoredHeader(headerName) logic (or references the
SkippedHeaders set) to detect dropped rules, and emits a clear warning
(including the rule identifier and header name) via the existing logger so
operators are informed; keep applyRequestRuleToHeader unchanged except now these
startup warnings surface dropped rules early.
|
The engine PR has been merged but currently there is a flaky test there preventing the publish of a new release. Need to wait for it before I can update the go.mod here. |
|
New engine version got released. I added it to the router/router-tests go.mod just now. |
This has been taking from another PR so we can close that one #2535
dkorittki
changed the title
dkorittki
changed the title
dkorittki
deleted the
dominik/eng-8955-add-header-filtering-for-grpc-datasources
branch
3 participants
Checklist
Description
This builds upon an engine change, which will pass headers as gRPC metadata to gRPC subgraphs wundergraph/graphql-go-tools#1382. That PR is already merged and released in v2.0.0-rc252, which is declared in go.mod.
gRPC metadata prefixed with
grpc-are reserved by the gRPC protocol. When we convert headers to metadata in the engine we could end up with such reserved metadata keys, if propagation rules (e.g. Matching:.*) match. This change enforces a single, centralized “never forward” rule for these reserved headers.There are two ways to solve this:
I opted for the former. It has the advantage of having consistent behaviour across all subgraphs. Otherwise we would end up with some headers being available on http subgraph A but not on gRPC subgraph B. Also right now the subgraph header builder does not know what kind of subgraph it builds headers for. We would need to change the header builder to build headers conditionally for different subgraph types.
Headers like this will not be forwarded to any subgraph anymore:
GRPC-MyHeadergrpc-AnotherHeaderGrpc-YetAnotherHeaderI also added a whole bunch of tests to ensure that our header propagation rules are respected on gRPC datasources and that all headers from the current ignore list are not being part of the gRPC metadata.
Summary by CodeRabbit
Tests
Bug Fixes / Behavior
Chores