fix: allow header propagation for router-plugins#2877
Open
Noroth wants to merge 1 commit into
Open
Conversation
Noroth
requested review from
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
WalkthroughThis PR adds support for configurable gRPC dial options to router plugins and validates HTTP header propagation to subgraph metadata. The change enables header forwarding from HTTP requests to outgoing gRPC calls through dial option customization, with comprehensive integration tests covering both binary and OCI plugin modes. ChangesgRPC Plugin Dial Options and Header Forwarding
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2877 +/- ##
==========================================
+ Coverage 64.47% 66.24% +1.77%
==========================================
Files 319 258 -61
Lines 45359 27064 -18295
Branches 4927 0 -4927
==========================================
- Hits 29245 17929 -11316
+ Misses 16089 7723 -8366
- Partials 25 1412 +1387
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@router-tests/protocol/router_plugin_test.go`:
- Around line 624-956: Enable telemetry for at least one subtest so the
telemetry-injection regression path is exercised: pick a subtest (e.g., the
"unsafe headers are absent from metadata" case) and modify the testenv.Config
passed to testenv.Run to turn telemetry on (for example by adding Telemetry:
testenv.TelemetryConfig{Enabled: true} or the equivalent flag in your test
harness), keeping the rest of the RouterOptions (captureInterceptor,
core.WithHeaderRules) unchanged so captureInterceptor, testenv.Run and
testenv.Config still validate that telemetry-injected outgoing metadata does not
overwrite/escape header propagation rules.
🪄 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: d7778b56-1b63-420e-bbd2-035501e41492
📒 Files selected for processing (8)
router-tests/protocol/router_plugin_test.gorouter-tests/router_oci_plugin_test.gorouter/core/graph_server.gorouter/core/router.gorouter/core/router_config.gorouter/pkg/grpcconnector/grpccommon/grpc_plugin_client.gorouter/pkg/grpcconnector/grpcplugin/grpc_plugin.gorouter/pkg/grpcconnector/grpcpluginoci/grpc_oci_plugin.go
Comment on lines
+624
to
+956
| func TestRouterPluginWithHeaderForwarding(t *testing.T) { | ||
| t.Run("Should send http headers as gRPC metadata to subgraphs", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // captureInterceptor records the outgoing metadata of subgraph RPCs only. | ||
| // hashicorp/go-plugin issues lifecycle RPCs under /plugin.*; ignoring them | ||
| // keeps the capture aligned with the test request and avoids a data race | ||
| // between the GraphQL call and the plugin's shutdown RPC. | ||
| captureInterceptor := func(captured *metadata.MD) grpc.UnaryClientInterceptor { | ||
| return func(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { | ||
| if strings.HasPrefix(method, "/service.") { | ||
| md, _ := metadata.FromOutgoingContext(ctx) | ||
| *captured = md.Copy() | ||
| } | ||
| return invoker(ctx, method, req, reply, cc, opts...) | ||
| } | ||
| } | ||
|
|
||
| t.Run("header arrives as metadata with correct value", func(t *testing.T) { | ||
| // Assert that headers included in propagation rules are part | ||
| // of gRPC metadata. | ||
| t.Parallel() | ||
|
|
||
| var captured metadata.MD | ||
|
|
||
| testenv.Run(t, &testenv.Config{ | ||
| RouterConfigJSONTemplate: testenv.ConfigWithPluginsJSONTemplate, | ||
| Plugins: testenv.PluginConfig{ | ||
| Enabled: true, | ||
| Path: "../../router/plugins", | ||
| }, | ||
| RouterOptions: []core.Option{ | ||
| core.WithGRPCPluginDialOptions(grpc.WithUnaryInterceptor(captureInterceptor(&captured))), | ||
| core.WithHeaderRules(config.HeaderRules{ | ||
| All: &config.GlobalHeaderRule{ | ||
| Request: []*config.RequestHeaderRule{ | ||
| { | ||
| Operation: config.HeaderRuleOperationPropagate, | ||
| Named: "X-Tenant-Id", | ||
| }, | ||
| { | ||
| Operation: config.HeaderRuleOperationPropagate, | ||
| Named: "X-Region-Name", | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| }, | ||
| }, func(t *testing.T, xEnv *testenv.Environment) { | ||
| xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ | ||
| Query: `query { projects { id name } }`, | ||
| Header: http.Header{ | ||
| "X-Tenant-Id": []string{"acme"}, | ||
| "X-Region-Name": []string{"frankfurt"}, | ||
| }, | ||
| }) | ||
|
|
||
| require.Equal(t, []string{"acme"}, captured.Get("x-tenant-id")) | ||
| require.Equal(t, []string{"frankfurt"}, captured.Get("x-region-name")) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("header not in propagation rules is absent from metadata", func(t *testing.T) { | ||
| // Assert that headers not included in propagation rules are not part | ||
| // of gRPC metadata. | ||
| t.Parallel() | ||
|
|
||
| var captured metadata.MD | ||
|
|
||
| testenv.Run(t, &testenv.Config{ | ||
| RouterConfigJSONTemplate: testenv.ConfigWithPluginsJSONTemplate, | ||
| Plugins: testenv.PluginConfig{ | ||
| Enabled: true, | ||
| Path: "../../router/plugins", | ||
| }, | ||
| RouterOptions: []core.Option{ | ||
| core.WithGRPCPluginDialOptions(grpc.WithUnaryInterceptor(captureInterceptor(&captured))), | ||
| core.WithHeaderRules(config.HeaderRules{ | ||
| All: &config.GlobalHeaderRule{ | ||
| Request: []*config.RequestHeaderRule{ | ||
| { | ||
| Operation: config.HeaderRuleOperationPropagate, | ||
| Named: "X-Allowed", | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| }, | ||
| }, func(t *testing.T, xEnv *testenv.Environment) { | ||
| xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ | ||
| Query: `query { projects { id name } }`, | ||
| Header: http.Header{ | ||
| "X-Allowed": []string{"yes"}, | ||
| "X-Not-Allowed": []string{"secret"}, | ||
| }, | ||
| }) | ||
|
|
||
| require.Equal(t, []string{"yes"}, captured.Get("x-allowed")) | ||
| require.Empty(t, captured.Get("x-not-allowed")) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("header with multiple values arrives as multiple metadata values", func(t *testing.T) { | ||
| // HTTP headers can be set with multiple values. They should appear with all values | ||
| // on the gRPC metadata as well. | ||
| t.Parallel() | ||
|
|
||
| var captured metadata.MD | ||
|
|
||
| testenv.Run(t, &testenv.Config{ | ||
| RouterConfigJSONTemplate: testenv.ConfigWithPluginsJSONTemplate, | ||
| Plugins: testenv.PluginConfig{ | ||
| Enabled: true, | ||
| Path: "../../router/plugins", | ||
| }, | ||
| RouterOptions: []core.Option{ | ||
| core.WithGRPCPluginDialOptions(grpc.WithUnaryInterceptor(captureInterceptor(&captured))), | ||
| core.WithHeaderRules(config.HeaderRules{ | ||
| All: &config.GlobalHeaderRule{ | ||
| Request: []*config.RequestHeaderRule{ | ||
| { | ||
| Operation: config.HeaderRuleOperationPropagate, | ||
| Named: "X-Role", | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| }, | ||
| }, func(t *testing.T, xEnv *testenv.Environment) { | ||
| xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ | ||
| Query: `query { projects { id name } }`, | ||
| Header: http.Header{ | ||
| "X-Role": []string{"admin", "editor"}, | ||
| }, | ||
| }) | ||
|
|
||
| require.Equal(t, []string{"admin", "editor"}, captured.Get("x-role")) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("unsafe headers are absent from metadata", func(t *testing.T) { | ||
| // The router avoids passing certain headers to datasources, | ||
| // see router/core/header_rule_engine.go. | ||
| // This test ensures grpc datasources are covered by this as well. | ||
| t.Parallel() | ||
|
|
||
| var captured metadata.MD | ||
|
|
||
| testenv.Run(t, &testenv.Config{ | ||
| RouterConfigJSONTemplate: testenv.ConfigWithPluginsJSONTemplate, | ||
| Plugins: testenv.PluginConfig{ | ||
| Enabled: true, | ||
| Path: "../../router/plugins", | ||
| }, | ||
| RouterOptions: []core.Option{ | ||
| core.WithGRPCPluginDialOptions(grpc.WithUnaryInterceptor(captureInterceptor(&captured))), | ||
| core.WithHeaderRules(config.HeaderRules{ | ||
| All: &config.GlobalHeaderRule{ | ||
| Request: []*config.RequestHeaderRule{ | ||
| { | ||
| Operation: config.HeaderRuleOperationPropagate, | ||
| Matching: ".*", // mark all headers as forwardable | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| }, | ||
| }, func(t *testing.T, xEnv *testenv.Environment) { | ||
| xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ | ||
| Query: `query { projects { id name } }`, | ||
| Header: http.Header{ | ||
| // safe header — must arrive | ||
| "X-Custom": []string{"value"}, | ||
|
|
||
| // handled by HTTP stack, never in r.Header | ||
| "Host": []string{"evil.example.com"}, | ||
|
|
||
| // hop-by-hop / connection headers | ||
| "Alt-Svc": []string{"h3=\":443\""}, | ||
| "Connection": []string{"keep-alive"}, | ||
| "Keep-Alive": []string{"timeout=5"}, | ||
| "Proxy-Authenticate": []string{"Basic"}, | ||
| "Proxy-Authorization": []string{"Basic dXNlcjpwYXNz"}, | ||
| "Proxy-Connection": []string{"keep-alive"}, | ||
| "Te": []string{"trailers"}, | ||
| "Trailer": []string{"Expires"}, | ||
| "Transfer-Encoding": []string{"chunked"}, | ||
| "Upgrade": []string{"websocket"}, | ||
|
|
||
| // content negotiation | ||
| "Accept": []string{"application/json"}, | ||
| "Accept-Charset": []string{"utf-8"}, | ||
| "Accept-Encoding": []string{"gzip, deflate"}, | ||
| "Content-Encoding": []string{"gzip"}, | ||
| "Content-Length": []string{"42"}, | ||
| "Content-Type": []string{"application/json"}, | ||
|
|
||
| // WebSocket upgrade | ||
| "Sec-Websocket-Extensions": []string{"permessage-deflate"}, | ||
| "Sec-Websocket-Key": []string{"dGhlIHNhbXBsZSBub25jZQ=="}, | ||
| "Sec-Websocket-Protocol": []string{"chat"}, | ||
| "Sec-Websocket-Version": []string{"13"}, | ||
| }, | ||
| }) | ||
|
|
||
| // We only assert what the router writes to outgoing metadata before | ||
| // the gRPC transport runs. content-type, user-agent and HTTP/2 | ||
| // pseudo-headers (:authority, :method, :path, :scheme) are added | ||
| // later by the transport when framing HTTP/2 and are not visible | ||
| // to a client interceptor; verifying them would require a server | ||
| // interceptor inside the plugin binary. | ||
|
|
||
| // custom header should arrive | ||
| require.Equal(t, []string{"value"}, captured.Get("x-custom")) | ||
|
|
||
| // host is handled by the HTTP stack and never forwarded | ||
| require.Empty(t, captured.Get("host")) | ||
|
|
||
| // hop-by-hop / connection headers | ||
| require.Empty(t, captured.Get("alt-svc")) | ||
| require.Empty(t, captured.Get("connection")) | ||
| require.Empty(t, captured.Get("keep-alive")) | ||
| require.Empty(t, captured.Get("proxy-authenticate")) | ||
| require.Empty(t, captured.Get("proxy-authorization")) | ||
| require.Empty(t, captured.Get("proxy-connection")) | ||
| require.Empty(t, captured.Get("te")) | ||
| require.Empty(t, captured.Get("trailer")) | ||
| require.Empty(t, captured.Get("transfer-encoding")) | ||
| require.Empty(t, captured.Get("upgrade")) | ||
|
|
||
| // content negotiation | ||
| require.Empty(t, captured.Get("accept")) | ||
| require.Empty(t, captured.Get("accept-charset")) | ||
| require.Empty(t, captured.Get("accept-encoding")) | ||
| require.Empty(t, captured.Get("content-encoding")) | ||
| require.Empty(t, captured.Get("content-length")) | ||
|
|
||
| // WebSocket upgrade | ||
| require.Empty(t, captured.Get("sec-websocket-extensions")) | ||
| require.Empty(t, captured.Get("sec-websocket-key")) | ||
| require.Empty(t, captured.Get("sec-websocket-protocol")) | ||
| require.Empty(t, captured.Get("sec-websocket-version")) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("grpc-reserved headers never reach the subgraph", func(t *testing.T) { | ||
| // Headers prefixed with "grpc-" are reserved by the gRPC protocol spec. | ||
| // Even when wildcard propagation is configured, they must never appear | ||
| // on the subgraph. | ||
| t.Parallel() | ||
|
|
||
| var captured metadata.MD | ||
|
|
||
| testenv.Run(t, &testenv.Config{ | ||
| RouterConfigJSONTemplate: testenv.ConfigWithPluginsJSONTemplate, | ||
| Plugins: testenv.PluginConfig{ | ||
| Enabled: true, | ||
| Path: "../../router/plugins", | ||
| }, | ||
| RouterOptions: []core.Option{ | ||
| core.WithGRPCPluginDialOptions(grpc.WithUnaryInterceptor(captureInterceptor(&captured))), | ||
| core.WithHeaderRules(config.HeaderRules{ | ||
| All: &config.GlobalHeaderRule{ | ||
| Request: []*config.RequestHeaderRule{ | ||
| { | ||
| Operation: config.HeaderRuleOperationPropagate, | ||
| Matching: ".*", | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| }, | ||
| }, func(t *testing.T, xEnv *testenv.Environment) { | ||
| xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ | ||
| Query: `query { projects { id name } }`, | ||
| Header: http.Header{ | ||
| "Grpc-ReservedHeader": []string{"should be ignored"}, | ||
| }, | ||
| }) | ||
|
|
||
| require.Empty(t, captured.Get("grpc-reservedheader")) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("safe headers are present in metadata", func(t *testing.T) { | ||
| // Ensure that http standard headers, which are safe and useful for subgraphs, | ||
| // are included in the metadata unmodified. | ||
| t.Parallel() | ||
|
|
||
| var captured metadata.MD | ||
|
|
||
| testenv.Run(t, &testenv.Config{ | ||
| RouterConfigJSONTemplate: testenv.ConfigWithPluginsJSONTemplate, | ||
| Plugins: testenv.PluginConfig{ | ||
| Enabled: true, | ||
| Path: "../../router/plugins", | ||
| }, | ||
| RouterOptions: []core.Option{ | ||
| core.WithGRPCPluginDialOptions(grpc.WithUnaryInterceptor(captureInterceptor(&captured))), | ||
| core.WithHeaderRules(config.HeaderRules{ | ||
| All: &config.GlobalHeaderRule{ | ||
| Request: []*config.RequestHeaderRule{ | ||
| // Propagate each header explicitly so the test is | ||
| // independent of any default propagation behaviour. | ||
| {Operation: config.HeaderRuleOperationPropagate, Named: "Authorization"}, | ||
| {Operation: config.HeaderRuleOperationPropagate, Named: "Cookie"}, | ||
| {Operation: config.HeaderRuleOperationPropagate, Named: "Traceparent"}, | ||
| {Operation: config.HeaderRuleOperationPropagate, Named: "Tracestate"}, | ||
| {Operation: config.HeaderRuleOperationPropagate, Named: "Accept-Language"}, | ||
| }, | ||
| }, | ||
| }), | ||
| }, | ||
| }, func(t *testing.T, xEnv *testenv.Environment) { | ||
| xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{ | ||
| Query: `query { projects { id name } }`, | ||
| Header: http.Header{ | ||
| "Authorization": []string{"Bearer eyJhbGciOiJSUzI1NiJ9"}, | ||
| "Cookie": []string{"session=abc123; theme=dark"}, | ||
| "Traceparent": []string{"00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"}, | ||
| "Tracestate": []string{"rojo=00f067aa0ba902b7"}, | ||
| "Accept-Language": []string{"de-DE,de;q=0.9,en;q=0.8"}, | ||
| }, | ||
| }) | ||
|
|
||
| require.Equal(t, []string{"Bearer eyJhbGciOiJSUzI1NiJ9"}, captured.Get("authorization")) | ||
| require.Equal(t, []string{"session=abc123; theme=dark"}, captured.Get("cookie")) | ||
| require.Equal(t, []string{"00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"}, captured.Get("traceparent")) | ||
| require.Equal(t, []string{"rojo=00f067aa0ba902b7"}, captured.Get("tracestate")) | ||
| require.Equal(t, []string{"de-DE,de;q=0.9,en;q=0.8"}, captured.Get("accept-language")) | ||
| }) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Cover the telemetry-enabled regression path here.
The bug this PR fixes only reproduces when telemetry injects its own outgoing metadata, but every subtest in this new suite runs without telemetry enabled. As written, these assertions still pass against the pre-fix overwrite path. Please turn telemetry on in at least one case so the regression is actually exercised.
🤖 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 `@router-tests/protocol/router_plugin_test.go` around lines 624 - 956, Enable
telemetry for at least one subtest so the telemetry-injection regression path is
exercised: pick a subtest (e.g., the "unsafe headers are absent from metadata"
case) and modify the testenv.Config passed to testenv.Run to turn telemetry on
(for example by adding Telemetry: testenv.TelemetryConfig{Enabled: true} or the
equivalent flag in your test harness), keeping the rest of the RouterOptions
(captureInterceptor, core.WithHeaderRules) unchanged so captureInterceptor,
testenv.Run and testenv.Config still validate that telemetry-injected outgoing
metadata does not overwrite/escape header propagation rules.
SkArchon
requested changes
May 21, 2026
|
|
||
| } | ||
|
|
||
| func TestRouterPluginWithHeaderForwarding(t *testing.T) { |
Contributor
There was a problem hiding this comment.
Could you add a test where we pass in header metadata, but also want to do a distributed trace. I think I wrote the func TestVerifyTelemetryForRouterPluginRequests(t *testing.T) { test for this
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
2 participants
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.
This PR fixes an issue that header propagation was not working for router-plugins if telemetry was enabled. Instead the metadata was overwritten by the plugin-client invoke logic.
Summary by CodeRabbit
Release Notes
New Features
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.