feat(router): add ConnectRPC protocol support for gRPC subgraphs#2665
Closed
fengyuwusong wants to merge 4 commits intowundergraph:mainfrom
Closed
feat(router): add ConnectRPC protocol support for gRPC subgraphs#2665fengyuwusong wants to merge 4 commits intowundergraph:mainfrom
fengyuwusong wants to merge 4 commits intowundergraph:mainfrom
Conversation
Add a `grpc_protocol` configuration section that allows users to choose between native gRPC and ConnectRPC on a per-subgraph basis. Connect subgraphs communicate over HTTP/1.1 with either Protobuf or JSON encoding, bypassing the need for HTTP/2 end-to-end. Key changes: - New `GRPCProtocolConfig` in config with global defaults and per-subgraph overrides for protocol and encoding - New `grpcprotocol` package with config validation, resolution, and transport builder - Factory resolver checks Connect transports before gRPC connector - Connect subgraphs skip native gRPC provider registration Depends on: wundergraph/graphql-go-tools#1453 Closes: wundergraph#2664 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fengyuwusong
requested review from
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
Contributor
WalkthroughAdds ConnectRPC transport support for gRPC subgraphs: new GRPC protocol config types and validation, Connect transport builder and tests, per‑subgraph HTTP client wiring in graph server, plumbing of connectTransports through executor/factory resolver to prefer Connect transports when configured. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. 📝 Coding Plan
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 |
![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: 3
🤖 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/graph_server.go`:
- Around line 1661-1667: The Disabled flag in toGRPCConfiguration is incorrectly
computed from config.Plugin presence alone; update toGRPCConfiguration (in
router/core/factoryresolver.go) to consider the effective protocol (use the same
connectTransports mapping used in graph_server.go) before setting Disabled so
that subgraphs with an effective Connect transport are not marked Disabled when
plugins are off. Concretely: determine if the subgraph is using Connect (e.g.,
check if connectTransports[sg.Name] exists or otherwise derive the protocol) and
only apply the Plugin != nil && !pluginsEnabled check for native gRPC subgraphs;
leave Disabled false for connect-mode subgraphs even if config.Plugin is
non-nil. Ensure you reference the same connectTransports key lookup logic as
used around connectTransports and sg.Name in graph_server.go so behavior is
consistent.
- Around line 1248-1254: newGraphServer currently passes a nil
grpcProtocolConfig into grpcprotocol.BuildConnectTransports so ConnectRPC never
activates and the per-subgraph/default http.Clients are not wired into each
RPCTransport; update newGraphServer to use the embedded Config.GRPCProtocol (or
s.GRPCProtocol) so the protocol decision is based on Config.GRPCProtocol, and
move the call to build RPCTransport instances to the place where
per-subgraph/default http.Clients already exist (the code path that constructs
http.Clients in factoryresolver.go) so grpcprotocol.BuildConnectTransports
produces RPCTransport objects that have the actual HTTPClient baked in and can
honor router timeout/retry/circuit-breaker clients.
In `@router/pkg/grpcprotocol/config.go`:
- Around line 17-36: The Validate function in grpcprotocol (Validate) is never
called so invalid grpc_protocol/default/default_encoding/subgraph
protocol/encoding values bypass checks; either call grpcprotocol.Validate(cfg)
from the config post-unmarshal flow inside LoadConfig (ensure you avoid an
import cycle by calling via a package that already imports grpcprotocol or add a
small validation bridge), or add enum constraints for default, default_encoding
and each subgraph.protocol/subgraph.encoding in config.schema.json so the JSON
schema enforces allowed values (matching ProtocolGRPC, ProtocolConnectRPC,
EncodingProto, EncodingJSON); reference Validate, LoadConfig, ResolveProtocol
and ResolveEncoding when choosing the fix to ensure validations run before those
resolvers apply defaults.
🪄 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: c63d9638-5e36-4b3e-aa09-21e8bc979371
📒 Files selected for processing (8)
router/core/executor.gorouter/core/factoryresolver.gorouter/core/graph_server.gorouter/pkg/config/config.gorouter/pkg/grpcprotocol/config.gorouter/pkg/grpcprotocol/config_test.gorouter/pkg/grpcprotocol/transport_builder.gorouter/pkg/grpcprotocol/transport_builder_test.go
- Add grpcProtocol field to Config struct and WithGRPCProtocol option so the GRPCProtocol config from the YAML config is propagated into the graphServer (was always nil before, disabling ConnectRPC) - Call grpcprotocol.Validate on startup to reject invalid protocol/ encoding values early - Pass properly-configured per-subgraph and default HTTP clients to BuildConnectTransports instead of nil, ensuring TLS and timeouts are respected for Connect subgraphs - Wire WithGRPCProtocol(config.GRPCProtocol) in optionsFromResources - Confirm Connect path in ResolveGraphqlFactory bypasses Disabled check, so the toGRPCConfiguration disabled flag is not an issue for Connect subgraphs (issue 2 is not a real bug) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
router/core/router_config.go (1)
147-147: Consider adding usage tracking forgrpcProtocol.Other protocol features like
mcp,connectRPC, andpluginshave corresponding entries in theUsage()method for telemetry. For consistency and observability, consider adding a usage entry for this new gRPC protocol configuration.♻️ Suggested addition to Usage() method
Add something like this near the other protocol usage entries (around line 337):
usage["grpc_protocol_enabled"] = c.grpcProtocol != nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/router_config.go` at line 147, The Usage() method is missing telemetry for grpcProtocol; update the Usage() implementation (the Usage() function on the same config type that already reports mcp, connectRPC and plugins) to add an entry such as "grpc_protocol_enabled" set to c.grpcProtocol != nil so the gRPC protocol config is tracked alongside the other protocol flags; place this new usage line near the existing protocol usage entries to keep consistency.
🤖 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/graph_server.go`:
- Around line 1253-1277: The current code builds connectSubgraphHTTPClients only
from s.subgraphTransportOptions.SubgraphMap so subgraphs that only have
per-subgraph TLS configured in s.subgraphTransports are missed; update the logic
after the existing loop to iterate s.subgraphTransports and for any subgraph key
not already in connectSubgraphHTTPClients create an *http.Client using the
transport from s.subgraphTransports (falling back to s.baseTransport if nil) and
the appropriate timeout (e.g., connectDefaultHTTPClient.Timeout or a sensible
default) so BuildConnectTransports sees per-subgraph TLS-aware clients rather
than always using connectDefaultHTTPClient.
In `@router/go.mod`:
- Line 200: Remove or comment out the local replace directive that points to the
absolute developer path for github.com/wundergraph/graphql-go-tools/v2 (the line
using "replace github.com/wundergraph/graphql-go-tools/v2 =>
/Users/.../graphql-go-tools/v2"); either comment it out to match the pattern
used in router-tests/go.mod (e.g. "// github.com/wundergraph/graphql-go-tools/v2
=> ../../graphql-go-tools/v2") or delete the replace entirely, and once
wundergraph/graphql-go-tools#1453 is merged and a release exists, update the
require directive to reference the published version instead of a local path.
---
Nitpick comments:
In `@router/core/router_config.go`:
- Line 147: The Usage() method is missing telemetry for grpcProtocol; update the
Usage() implementation (the Usage() function on the same config type that
already reports mcp, connectRPC and plugins) to add an entry such as
"grpc_protocol_enabled" set to c.grpcProtocol != nil so the gRPC protocol config
is tracked alongside the other protocol flags; place this new usage line near
the existing protocol usage entries to keep consistency.
🪄 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: 0bb94dab-9d26-4b79-b222-22997815b340
📒 Files selected for processing (5)
router/core/graph_server.gorouter/core/router.gorouter/core/router_config.gorouter/core/supervisor_instance.gorouter/go.mod
…irective - Include subgraphs with per-subgraph TLS (but no traffic shaping overrides) in Connect HTTP client map to avoid losing TLS config - Comment out local replace directive for graphql-go-tools/v2; will be updated to published version once graphql-go-tools#1453 merges Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…o-tools - Include subgraphs with per-subgraph TLS (but no traffic shaping overrides) in Connect HTTP client map to avoid losing TLS config - Replace local path replace directive with fork pseudo-version pointing to fengyuwusong/graphql-go-tools commit 12c891d9 (will be updated to released version once wundergraph#1453 merges) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/core/graph_server.go (1)
2105-2125: Consider using overridden routing URLs for Connect transports.This function collects
sg.RoutingUrldirectly from the config subgraphs. However, routing URLs can be overridden viaoverrideRoutingURLConfigurationoroverridesConfig(seeconfigureSubgraphOverwritesstarting at line 1997).If a user overrides the routing URL for a gRPC subgraph, the Connect transport may still use the original URL instead of the override.
Consider calling
collectGRPCSubgraphURLsafterconfigureSubgraphOverwriteshas processed the overrides, or passing the processed subgraphs list instead ofopts.ConfigSubgraphs:- connectTransports := grpcprotocol.BuildConnectTransports( - s.grpcProtocolConfig, - collectGRPCSubgraphURLs(opts.EngineConfig, opts.ConfigSubgraphs), - connectSubgraphHTTPClients, - connectDefaultHTTPClient, - ) + // Use processed subgraphs to respect routing URL overrides + grpcSubgraphURLs := map[string]string{} + for _, sg := range subgraphs { + // Check if this subgraph has gRPC config + for _, dsConfig := range opts.EngineConfig.DatasourceConfigurations { + if dsConfig.Id == sg.Id && dsConfig.GetCustomGraphql().GetGrpc() != nil { + grpcSubgraphURLs[sg.Name] = sg.UrlString + break + } + } + } + connectTransports := grpcprotocol.BuildConnectTransports( + s.grpcProtocolConfig, + grpcSubgraphURLs, + connectSubgraphHTTPClients, + connectDefaultHTTPClient, + )Alternatively, refactor
collectGRPCSubgraphURLsto accept the processed[]Subgraphslice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/graph_server.go` around lines 2105 - 2125, collectGRPCSubgraphURLs currently reads sg.RoutingUrl from the original config and can miss user overrides applied by configureSubgraphOverwrites/overrideRoutingURLConfiguration/overridesConfig; fix by either calling collectGRPCSubgraphURLs only after configureSubgraphOverwrites has been applied (so it receives the processed configSubgraphs) or refactor collectGRPCSubgraphURLs to take the post-processed []Subgraph slice (or a function that resolves the effective routing URL) and use that value when building the map for gRPC subgraphs so Connect transports respect overrides.
🤖 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/go.mod`:
- Around line 200-201: The replace directive referencing
github.com/fengyuwusong/graphql-go-tools/v2 with pseudo-version
v2.0.0-20260319034538-12c891d918df points to a non-existent commit; update the
replace to a resolvable reference by either replacing the pseudo-version with a
valid commit hash that exists in that fork, or use an available tag/branch on
github.com/fengyuwusong/graphql-go-tools/v2, or remove the replace and revert
back to the upstream module github.com/wundergraph/graphql-go-tools/v2 once PR
`#1453` is merged — edit the replace line accordingly so Go can resolve the
dependency.
---
Nitpick comments:
In `@router/core/graph_server.go`:
- Around line 2105-2125: collectGRPCSubgraphURLs currently reads sg.RoutingUrl
from the original config and can miss user overrides applied by
configureSubgraphOverwrites/overrideRoutingURLConfiguration/overridesConfig; fix
by either calling collectGRPCSubgraphURLs only after configureSubgraphOverwrites
has been applied (so it receives the processed configSubgraphs) or refactor
collectGRPCSubgraphURLs to take the post-processed []Subgraph slice (or a
function that resolves the effective routing URL) and use that value when
building the map for gRPC subgraphs so Connect transports respect overrides.
🪄 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: 892f5f84-93b0-457e-b6ac-d5990ed3e8c9
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
router/core/graph_server.gorouter/go.mod
Contributor
|
Hi @fengyuwusong , This PR isn’t mergeable under our standards, as it lacks valuable unit and integration tests. The changes are also difficult to review due to limited context and supporting structure. Using AI to assist with code is absolutely fine, but we expect contributions to meet certain quality and clarity standards. To help clarify this, our CTO recently introduced a guideline that we hope you and the broader community will find useful: https://www.human-oss.dev/ Additionally, when working on new features, we recommend opening an issue first to gather feedback before starting implementation. I’m going to close this PR for now, but I hope this doesn’t discourage you—we really value contributions and would be happy to see a revised submission in the future. |
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.
Motivation and Context
The Cosmo Router currently only supports native gRPC (HTTP/2 + Protobuf) for communicating with gRPC-based subgraphs. This creates friction in environments where HTTP/2 pass-through is difficult (load balancers, CDNs) or where teams use the ConnectRPC framework.
This PR adds a
grpc_protocolconfiguration section that allows choosing between native gRPC and ConnectRPC on a per-subgraph basis.Related Issue
Closes #2664
Depends on: wundergraph/graphql-go-tools#1453 (RPCTransport interface + Connect transport implementation)
Changes
New config type (
router/pkg/config/config.go)GRPCProtocolConfigwith global defaults and per-subgraph overrides for protocol (grpc/connectrpc) and encoding (proto/json)New package (
router/pkg/grpcprotocol/)config.go— validation, protocol/encoding resolution with fallback chaintransport_builder.go— buildsRPCTransportinstances for Connect-configured subgraphsRouter core integration
factoryresolver.go— checks Connect transports before gRPC connector (priority: Connect > gRPC > HTTP)executor.go— passesconnectTransportsto factory resolvergraph_server.go— collects gRPC subgraph URLs, builds Connect transports, skips native gRPC registration for Connect subgraphsConfiguration example
Checklist
Performance Evaluation
Connect transport adds minimal overhead compared to native gRPC — one HTTP round-trip per unary call with identical Protobuf serialization cost. JSON encoding is slightly slower but provides better debuggability. Fully backward compatible: nil config = no behavior change.
Summary by CodeRabbit
New Features
Tests