feat(router): add ConnectRPC transport for gRPC subgraphs#2808
Draft
fengyuwusong wants to merge 13 commits intowundergraph:mainfrom
Draft
feat(router): add ConnectRPC transport for gRPC subgraphs#2808fengyuwusong wants to merge 13 commits intowundergraph:mainfrom
fengyuwusong wants to merge 13 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>
- 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>
…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>
…tions Address feedback that the new gRPC transport-protocol configuration is not discoverable through documentation alone. The renames make field intent explicit and align tags with the rest of router/pkg/config: - type GRPCProtocolConfig -> GRPCProtocolConfiguration - type SubgraphGRPCProtocolConfig -> GRPCProtocolSubgraph - field Default -> DefaultProtocol (yaml: default_protocol) - add envDefault/env tags on default_protocol and default_encoding - add envPrefix:"GRPC_PROTOCOL_" on the top-level grpc_protocol entry Validation error keys are updated accordingly (grpc_protocol.default -> grpc_protocol.default_protocol).
Add the grpc_protocol object (default_protocol, default_encoding, and per-subgraph overrides) to router/pkg/config/config.schema.json so that editor and CI validators recognize the new configuration. Update testdata snapshots to reflect the new field on the Config struct.
Add a "gRPC Subgraph Protocol" section to the router configuration reference covering default_protocol, default_encoding, and per-subgraph overrides with environment variable / YAML mappings. Add a brief "ConnectRPC Support" section to the gRPC concepts page that explains when ConnectRPC is useful (no end-to-end HTTP/2) and links into the configuration reference.
Migrate the projects standalone subgraph to a `buf generate`-based code generation pipeline that produces three things from the same .proto: - Message structs (protoc-gen-go) - gRPC server/client (protoc-gen-go-grpc) - ConnectRPC handler/client (protoc-gen-connect-go) The Connect handler natively serves Connect, gRPC, and gRPC-Web on the same HTTP endpoint, so this lets the demo subgraph be reached over either transport from the router based on the new `grpc_protocol` configuration block. Notable changes: - Add buf.yaml and buf.gen.yaml. The Connect plugin needs Mservice.proto= to point at .../generated because the proto declares the bare module path as its go_package. - Regenerate service.pb.go and service_grpc.pb.go through buf so the whole subgraph stays consistent with one generator pipeline. - Add `make projects-buf-generate` target in demo/Makefile. - Replace the standalone gRPC server in cmd/service/main.go with a ConnectRPC handler running over H2C on :4011. The handler accepts Connect, gRPC, and gRPC-Web traffic, so existing gRPC clients keep working unchanged. - Add ProjectsConnectService (src/service/service_connect.go), a thin adapter that forwards every Connect call to the existing *ProjectsService gRPC implementation. Generated mechanically from the gRPC method signatures so it covers all 48 RPCs in one shot.
Add a new connectrpc_subgraph_test.go that drives the demo projects subgraph through the router using the ConnectRPC protocol. The tests mirror the four categories called out in the gRPC subgraph review: plain queries, mutations, federation entity lookups, and resolver- triggered field RPCs, plus protocol/encoding overrides via the `grpc_protocol` configuration block. To make the test possible: - testenv gains an `EnableConnectRPC` flag that swaps the projects *grpc.Server for a ConnectRPC handler running over H2C. Existing EnableGRPC behaviour is unchanged when the flag is left false, so the 78 existing TestGRPCSubgraph cases keep passing as a regression check. - grpcprotocol.BuildConnectTransports now normalises the routing URL before handing it to the Connect HTTP client. URLs declared with the gRPC name resolver scheme (dns:///host:port) or as a bare host:port - the formats produced by the testenv replacements and config templates - are turned into http://host:port; existing http(s):// URLs pass through.
The Cosmo Connect gRPC Services overview page now points readers at the new `grpc_protocol` configuration block so they discover from the entry page that a gRPC subgraph can be reached over either native gRPC or ConnectRPC. The configuration reference and the gRPC concepts page hold the detailed schema and examples.
The routing URL validator on createFederatedSubgraph, updateSubgraph
and publishFederatedSubgraph rejected anything that did not match a
gRPC name resolver scheme (dns://, unix://, vsock://, ipv4://, ipv6://).
That blocks ConnectRPC subgraphs, which serve over plain HTTP/1.1 and
need an http(s):// routing URL even though their subgraph type is
GRPC_SERVICE.
Add isValidGrpcSubgraphRoutingURL that accepts either form:
- any URL recognised by the existing isValidGrpcNamingScheme
(preserves the historical strict gRPC validation), or
- any well-formed http:// or https:// URL.
The router decides at request time which transport to use via the new
`grpc_protocol` configuration block, so the control plane does not
need to commit to one or the other at registration time. The error
message is updated to mention both options.
Three integration tests previously asserted that an http:// or https:// routing URL was rejected for GRPC_SERVICE subgraphs. Now that the control plane accepts those URLs (so ConnectRPC subgraphs can register their HTTP endpoint directly), invert the assertions: - createFederatedSubgraph with http(s) -> EnumStatusCode.OK - updateSubgraph with http(s) -> EnumStatusCode.OK - publishFederatedSubgraph with http(s) -> EnumStatusCode.OK Test names updated accordingly. The companion "Should allow ... with gRPC naming scheme" tests remain unchanged so the gRPC path keeps the same coverage.
…me handling The helper that turns a federated routing URL into a base URL the ConnectRPC HTTP client can dial only handled `http(s)://`, `dns:///` and the bare `host:port` form. Routing URLs registered with the colon-only forms (`dns:host:port`, `ipv4:host:port`, etc.) ended up as `http://dns:host:port`, which would never resolve. Rewrite the helper to recognise the full set of gRPC name resolver schemes accepted by the control plane (dns, ipv4, ipv6, vsock, unix, unix-abstract, passthrough) and add a table-driven test TestNormalizeConnectBaseURL covering pass-through, the three dns shapes, ipv4, unix, and the empty input edge case.
New examples/router-grpc-connect/ mirrors the existing router-simple
layout (config.yaml + graph.yaml + start.sh + README) but is wired up
to the demo projects gRPC subgraph reached over ConnectRPC.
The example covers the three pieces a user needs to put together to use
the new transport:
- Router config with `grpc_protocol.default_protocol: connectrpc` plus
a per-subgraph override snippet for mixed graphs.
- Federation graph.yaml with an http:// routing URL for the gRPC
subgraph and references to its proto/mapping/SDL files.
- start.sh that boots the standalone projects subgraph (Connect handler
over H2C on :4011), composes the supergraph, and starts the router.
The README also shows the matching `wgc grpc-service create
--routing-url http://...` invocation now that the control plane accepts
http(s) URLs for GRPC_SERVICE subgraphs.
Contributor
WalkthroughThis PR introduces ConnectRPC protocol support to the Cosmo Router, enabling gRPC subgraphs to communicate via HTTP/1.1 with Protobuf or JSON encoding in addition to native gRPC. Changes include validation updates for HTTP(S) routing URLs, configuration schema additions, transport builder implementation, test infrastructure, documentation, and a complete example demonstrating ConnectRPC integration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 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 |
Author
5 tasks
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demo/pkg/subgraphs/projects/go.mod (1)
33-39:⚠️ Potential issue | 🟠 MajorPin
go.opentelemetry.io/otel/sdktov1.40.0or newer in this module.Line 38 has
go.opentelemetry.io/otel/sdk v1.28.0, which is vulnerable to GHSA-9h8m-3fm2-qjrq. Add an explicit version constraint to at leastv1.40.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/projects/go.mod` around lines 33 - 39, Update the go.mod entry for the OpenTelemetry SDK by pinning the module go.opentelemetry.io/otel/sdk to at least v1.40.0 (replace the current v1.28.0 with v1.40.0 or newer) so the vulnerable version is not used; locate the go.mod dependency line mentioning go.opentelemetry.io/otel/sdk and change its version token accordingly, then run `go mod tidy` to reconcile dependencies.
🧹 Nitpick comments (3)
examples/router-grpc-connect/start.sh (1)
2-2: ⚡ Quick winUse stricter shell mode for safer failures.
set -emisses unset-variable and pipeline failure cases. Using strict mode makes this bootstrap script more predictable in local/dev environments.Proposed change
-set -e +set -euo pipefail🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/router-grpc-connect/start.sh` at line 2, Replace the loose "set -e" in start.sh with stricter shell options: enable -euo pipefail and set a safe IFS (e.g., IFS=$'\n\t') so the script fails on unset variables and on pipeline errors; update the top of the script where "set -e" appears to use these stricter options to make bootstrap behavior deterministic in local/dev runs.router-tests/testenv/testenv.go (1)
641-645: ⚡ Quick win
EnableConnectRPCcurrently bypasses the existing project interceptor hook.The gRPC branch preserves
cfg.Subgraphs.Projects.GRPCInterceptor, but the ConnectRPC branch ignores it entirely. That makesEnableConnectRPCsilently change server behavior for interceptor-driven tests. Please either thread the hook through the Connect setup or fail fast when both options are set.Possible fail-fast guard until Connect-side interception is supported
if cfg.EnableGRPC { if cfg.EnableConnectRPC { + if cfg.Subgraphs.Projects.GRPCInterceptor != nil { + return nil, errors.New("projects GRPCInterceptor is not supported with EnableConnectRPC") + } connectHTTPServer, endpoint = makeSafeConnectRPCServer(t) } else { projectServer, endpoint = makeSafeGRPCServer(t, &projects.ProjectsService_ServiceDesc, &service.ProjectsService{}, cfg.Subgraphs.Projects.GRPCInterceptor) } }Also applies to: 1094-1098
🤖 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 641 - 645, The ConnectRPC branch ignores the project-level gRPC interceptor (cfg.Subgraphs.Projects.GRPCInterceptor) while the gRPC branch preserves it; update makeSafeConnectRPCServer or the setup logic so the Projects.GRPCInterceptor is either passed into the Connect server creation (thread the hook through makeSafeConnectRPCServer and use it when constructing the Connect handlers for ProjectsService) or add a fail-fast check where cfg.EnableConnectRPC is true and cfg.Subgraphs.Projects.GRPCInterceptor is non-nil (return an explicit error or t.Fatalf) to prevent silent behavioral changes; adjust the same logic where projectServer/connectHTTPServer and endpoint are assigned (the code around makeSafeConnectRPCServer and makeSafeGRPCServer) and apply the same fix to the other occurrence referenced.router-tests/protocol/connectrpc_subgraph_test.go (1)
22-181: ⚡ Quick winThese parallel subtests are still sharing mutable demo state.
Lines 58-60 already call out that
updateProjectStatuscan change data seen by sibling cases. Dropping thestatusassertion hides one symptom, but the suite is still order-dependent as long as every subtest runs witht.Parallel()against the same in-memory store. Please serialize this suite or give each environment an isolated projects store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router-tests/protocol/connectrpc_subgraph_test.go` around lines 22 - 181, The subtests in TestConnectRPCSubgraph run in parallel while sharing a mutable in-memory demo store (so updateProjectStatus can mutate state seen by siblings); fix by either removing the per-subtest t.Parallel() calls (serialize the subtests) or ensure each test gets an isolated projects store by changing the testenv.Run invocation to supply a fresh store per run via testenv.Config (so each testenv.Run creates an independent environment rather than sharing the demo state); update references around TestConnectRPCSubgraph, the t.Parallel() calls, testenv.Run and testenv.Config (or the code path that seeds the in-memory store) accordingly so updateProjectStatus no longer causes cross-test interference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo/pkg/subgraphs/projects/cmd/service/main.go`:
- Around line 37-46: The recoveryInterceptor currently logs and then returns raw
panic details to clients; change it so the deferred panic handler still logs the
recovered value (e.g., keep log.Printf("Recovered from panic: %v", r) or expand
to include stack trace) but replace the client-facing error produced by
connect.NewError/connect.CodeInternal with a generic message like "internal
server error" (do not include r or stack info). Update any other places in the
same interceptor (and similar handlers around the defer/err assignment) that
build client responses (e.g., calls to connect.NewError or status.Errorf) to use
the generic message while preserving detailed diagnostics only in server logs.
In `@docs-website/router/configuration.mdx`:
- Around line 2439-2456: The YAML example uses the same top-level key
grpc_protocol twice which is invalid/misleading; split the example into two
separate code blocks (or a CodeGroup of two snippets) so each example has its
own version: one block showing a single global grpc_protocol with
default_protocol: connectrpc and default_encoding: proto, and a second block
showing a different grpc_protocol example with default_protocol: grpc and a
subgraphs map (employees/products) — update the snippet surrounding the
grpc_protocol occurrences (search for grpc_protocol, default_protocol,
default_encoding, subgraphs) to implement the two distinct examples instead of
duplicate keys.
In `@docs-website/router/gRPC/concepts.mdx`:
- Around line 252-256: Replace the em dash in the sentence containing "is not
possible — for example when a proxy or load balancer in front of the subgraph
does not forward gRPC." with a period (or otherwise restructure into two
sentences) so it reads like: "...is not possible. For example, when a proxy or
load balancer in front of the subgraph does not forward gRPC." Update the MDX
paragraph that explains ConnectRPC accordingly to comply with the MDX docs
guideline to avoid em dashes.
In `@examples/router-grpc-connect/graph.yaml`:
- Line 20: The mappingFile value in graph.yaml currently points to a
non-existent path; update the mappingFile entry so it references the actual
relative location of mapping.json in the repository (replace the current
'../../demo/pkg/subgraphs/projects/mapping.json' value with the correct relative
path to the real mapping.json file) so the example config loads successfully;
look for the mapping.json file in the demo/pkg/subgraphs or demo directory tree
and correct the mappingFile key accordingly.
In `@router/pkg/grpcprotocol/transport_builder.go`:
- Around line 64-99: normalizeConnectBaseURL currently maps non-TCP schemes
(unix, unix-abstract, vsock, passthrough) into an http:// URL but the HTTP
client/transport still dials TCP, causing runtime failures; update the code path
that handles ConnectRPC to either reject these non-TCP schemes early or add a
proper custom dialer for them. Specifically, in the code that selects/uses
normalizeConnectBaseURL and constructs the *http.Client / http.Transport for
ConnectRPC, detect schemes returned by normalizeConnectBaseURL (or inspect the
original routingURL prefixes such as "unix:", "unix-abstract:", "vsock:") and
(a) return a validation error when the selected protocol is connectrpc and the
scheme is not a TCP/HTTP-compatible one, or (b) implement and attach a custom
DialContext/Dial function on the http.Transport that understands unix domain
sockets and vsock and performs the correct dial for "unix:" / "unix-abstract:" /
"vsock:" endpoints; keep references to normalizeConnectBaseURL, the
http.Transport construction, and any transport builder function so reviewers can
locate the change.
---
Outside diff comments:
In `@demo/pkg/subgraphs/projects/go.mod`:
- Around line 33-39: Update the go.mod entry for the OpenTelemetry SDK by
pinning the module go.opentelemetry.io/otel/sdk to at least v1.40.0 (replace the
current v1.28.0 with v1.40.0 or newer) so the vulnerable version is not used;
locate the go.mod dependency line mentioning go.opentelemetry.io/otel/sdk and
change its version token accordingly, then run `go mod tidy` to reconcile
dependencies.
---
Nitpick comments:
In `@examples/router-grpc-connect/start.sh`:
- Line 2: Replace the loose "set -e" in start.sh with stricter shell options:
enable -euo pipefail and set a safe IFS (e.g., IFS=$'\n\t') so the script fails
on unset variables and on pipeline errors; update the top of the script where
"set -e" appears to use these stricter options to make bootstrap behavior
deterministic in local/dev runs.
In `@router-tests/protocol/connectrpc_subgraph_test.go`:
- Around line 22-181: The subtests in TestConnectRPCSubgraph run in parallel
while sharing a mutable in-memory demo store (so updateProjectStatus can mutate
state seen by siblings); fix by either removing the per-subtest t.Parallel()
calls (serialize the subtests) or ensure each test gets an isolated projects
store by changing the testenv.Run invocation to supply a fresh store per run via
testenv.Config (so each testenv.Run creates an independent environment rather
than sharing the demo state); update references around TestConnectRPCSubgraph,
the t.Parallel() calls, testenv.Run and testenv.Config (or the code path that
seeds the in-memory store) accordingly so updateProjectStatus no longer causes
cross-test interference.
In `@router-tests/testenv/testenv.go`:
- Around line 641-645: The ConnectRPC branch ignores the project-level gRPC
interceptor (cfg.Subgraphs.Projects.GRPCInterceptor) while the gRPC branch
preserves it; update makeSafeConnectRPCServer or the setup logic so the
Projects.GRPCInterceptor is either passed into the Connect server creation
(thread the hook through makeSafeConnectRPCServer and use it when constructing
the Connect handlers for ProjectsService) or add a fail-fast check where
cfg.EnableConnectRPC is true and cfg.Subgraphs.Projects.GRPCInterceptor is
non-nil (return an explicit error or t.Fatalf) to prevent silent behavioral
changes; adjust the same logic where projectServer/connectHTTPServer and
endpoint are assigned (the code around makeSafeConnectRPCServer and
makeSafeGRPCServer) and apply the same fix to the other occurrence referenced.
🪄 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: 4d81d5e6-a69d-416d-a287-4d05e7184298
⛔ Files ignored due to path filters (4)
demo/pkg/subgraphs/projects/generated/projectsconnect/service.connect.gois excluded by!**/generated/**demo/pkg/subgraphs/projects/generated/service.pb.gois excluded by!**/*.pb.go,!**/generated/**demo/pkg/subgraphs/projects/generated/service_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**demo/pkg/subgraphs/projects/go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
controlplane/src/core/bufservices/subgraph/createFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/updateSubgraph.tscontrolplane/src/core/util.test.tscontrolplane/src/core/util.tscontrolplane/test/subgraph/create-subgraph.test.tscontrolplane/test/subgraph/publish-subgraph.test.tscontrolplane/test/subgraph/update-subgraph.test.tsdemo/Makefiledemo/pkg/subgraphs/projects/buf.gen.yamldemo/pkg/subgraphs/projects/buf.yamldemo/pkg/subgraphs/projects/cmd/service/main.godemo/pkg/subgraphs/projects/go.moddemo/pkg/subgraphs/projects/src/service/service_connect.godocs-website/connect/grpc-services.mdxdocs-website/router/configuration.mdxdocs-website/router/gRPC/concepts.mdxexamples/router-grpc-connect/README.mdexamples/router-grpc-connect/config.yamlexamples/router-grpc-connect/graph.yamlexamples/router-grpc-connect/start.shrouter-tests/protocol/connectrpc_subgraph_test.gorouter-tests/testenv/testenv.gorouter/core/executor.gorouter/core/factoryresolver.gorouter/core/graph_server.gorouter/core/router.gorouter/core/router_config.gorouter/core/supervisor_instance.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/grpcprotocol/config.gorouter/pkg/grpcprotocol/config_test.gorouter/pkg/grpcprotocol/transport_builder.gorouter/pkg/grpcprotocol/transport_builder_test.go
Comment on lines
+37
to
+46
| // recoveryInterceptor catches panics from RPC handlers so that one bad | ||
| // request cannot bring down the whole server. | ||
| func recoveryInterceptor() connect.UnaryInterceptorFunc { | ||
| return func(next connect.UnaryFunc) connect.UnaryFunc { | ||
| return func(ctx context.Context, req connect.AnyRequest) (resp connect.AnyResponse, err error) { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| log.Printf("Recovered from panic: %v", r) | ||
| err = connect.NewError(connect.CodeInternal, status.Errorf(codes.Internal, "panic: %v", r)) | ||
| } |
Contributor
There was a problem hiding this comment.
Do not expose panic/internal error details to clients.
Line 45 and Line 79 include raw panic/error values in client-facing responses. That leaks internals. Return a generic internal error message and keep specifics in logs.
Suggested hardening patch
func recoveryInterceptor() connect.UnaryInterceptorFunc {
@@
defer func() {
if r := recover(); r != nil {
log.Printf("Recovered from panic: %v", r)
- err = connect.NewError(connect.CodeInternal, status.Errorf(codes.Internal, "panic: %v", r))
+ err = connect.NewError(connect.CodeInternal, status.Error(codes.Internal, "internal server error"))
}
}()
@@
func errorInterceptor() connect.UnaryInterceptorFunc {
@@
if err != nil {
var connErr *connect.Error
if !errors.As(err, &connErr) {
if _, ok := status.FromError(err); !ok {
- err = connect.NewError(connect.CodeInternal, status.Errorf(codes.Internal, "internal server error: %v", err))
+ err = connect.NewError(connect.CodeInternal, status.Error(codes.Internal, "internal server error"))
}
}
}Also applies to: 69-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/pkg/subgraphs/projects/cmd/service/main.go` around lines 37 - 46, The
recoveryInterceptor currently logs and then returns raw panic details to
clients; change it so the deferred panic handler still logs the recovered value
(e.g., keep log.Printf("Recovered from panic: %v", r) or expand to include stack
trace) but replace the client-facing error produced by
connect.NewError/connect.CodeInternal with a generic message like "internal
server error" (do not include r or stack info). Update any other places in the
same interceptor (and similar handlers around the defer/err assignment) that
build client responses (e.g., calls to connect.NewError or status.Errorf) to use
the generic message while preserving detailed diagnostics only in server logs.
Comment on lines
+2439
to
+2456
| ```yaml config.yaml | ||
| version: "1" | ||
|
|
||
| # Switch all gRPC subgraphs to ConnectRPC over HTTP/1.1 | ||
| grpc_protocol: | ||
| default_protocol: connectrpc | ||
| default_encoding: proto | ||
|
|
||
| # Or mix protocols on a per-subgraph basis | ||
| grpc_protocol: | ||
| default_protocol: grpc | ||
| subgraphs: | ||
| employees: | ||
| protocol: connectrpc | ||
| encoding: json | ||
| products: | ||
| protocol: grpc | ||
| ``` |
Contributor
There was a problem hiding this comment.
Avoid duplicate grpc_protocol keys in one YAML example.
This snippet defines grpc_protocol twice in the same document. In YAML, the second key overrides the first, so copy-paste behavior is misleading.
Proposed change
-```yaml config.yaml
-version: "1"
-
-# Switch all gRPC subgraphs to ConnectRPC over HTTP/1.1
-grpc_protocol:
- default_protocol: connectrpc
- default_encoding: proto
-
-# Or mix protocols on a per-subgraph basis
-grpc_protocol:
- default_protocol: grpc
- subgraphs:
- employees:
- protocol: connectrpc
- encoding: json
- products:
- protocol: grpc
-```
+<CodeGroup>
+ ```yaml All gRPC subgraphs use ConnectRPC
+ version: "1"
+ grpc_protocol:
+ default_protocol: connectrpc
+ default_encoding: proto
+ ```
+
+ ```yaml Mixed per-subgraph protocol
+ version: "1"
+ grpc_protocol:
+ default_protocol: grpc
+ subgraphs:
+ employees:
+ protocol: connectrpc
+ encoding: json
+ products:
+ protocol: grpc
+ ```
+</CodeGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs-website/router/configuration.mdx` around lines 2439 - 2456, The YAML
example uses the same top-level key grpc_protocol twice which is
invalid/misleading; split the example into two separate code blocks (or a
CodeGroup of two snippets) so each example has its own version: one block
showing a single global grpc_protocol with default_protocol: connectrpc and
default_encoding: proto, and a second block showing a different grpc_protocol
example with default_protocol: grpc and a subgraphs map (employees/products) —
update the snippet surrounding the grpc_protocol occurrences (search for
grpc_protocol, default_protocol, default_encoding, subgraphs) to implement the
two distinct examples instead of duplicate keys.
Comment on lines
+252
to
+256
| The router can talk to gRPC subgraphs over either native gRPC (HTTP/2 + | ||
| Protobuf) or [ConnectRPC](https://connectrpc.com/) (HTTP/1.1 with Protobuf | ||
| or JSON). ConnectRPC is useful when end-to-end HTTP/2 to the subgraph is | ||
| not possible — for example when a proxy or load balancer in front of the | ||
| subgraph does not forward gRPC. |
Contributor
There was a problem hiding this comment.
Replace the em dash in this paragraph.
Line 255 uses an em dash, which conflicts with the docs writing rules for MDX files.
Proposed change
-The router can talk to gRPC subgraphs over either native gRPC (HTTP/2 +
-Protobuf) or [ConnectRPC](https://connectrpc.com/) (HTTP/1.1 with Protobuf
-or JSON). ConnectRPC is useful when end-to-end HTTP/2 to the subgraph is
-not possible — for example when a proxy or load balancer in front of the
-subgraph does not forward gRPC.
+The router can talk to gRPC subgraphs over either native gRPC (HTTP/2 +
+Protobuf) or [ConnectRPC](https://connectrpc.com/) (HTTP/1.1 with Protobuf
+or JSON). ConnectRPC is useful when end-to-end HTTP/2 to the subgraph is
+not possible. For example, a proxy or load balancer in front of the
+subgraph may not forward gRPC.As per coding guidelines: "Avoid em dashes. Use periods or restructure the sentence instead."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The router can talk to gRPC subgraphs over either native gRPC (HTTP/2 + | |
| Protobuf) or [ConnectRPC](https://connectrpc.com/) (HTTP/1.1 with Protobuf | |
| or JSON). ConnectRPC is useful when end-to-end HTTP/2 to the subgraph is | |
| not possible — for example when a proxy or load balancer in front of the | |
| subgraph does not forward gRPC. | |
| The router can talk to gRPC subgraphs over either native gRPC (HTTP/2 + | |
| Protobuf) or [ConnectRPC](https://connectrpc.com/) (HTTP/1.1 with Protobuf | |
| or JSON). ConnectRPC is useful when end-to-end HTTP/2 to the subgraph is | |
| not possible. For example, a proxy or load balancer in front of the | |
| subgraph may not forward gRPC. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs-website/router/gRPC/concepts.mdx` around lines 252 - 256, Replace the em
dash in the sentence containing "is not possible — for example when a proxy or
load balancer in front of the subgraph does not forward gRPC." with a period (or
otherwise restructure into two sentences) so it reads like: "...is not possible.
For example, when a proxy or load balancer in front of the subgraph does not
forward gRPC." Update the MDX paragraph that explains ConnectRPC accordingly to
comply with the MDX docs guideline to avoid em dashes.
| file: ../../demo/pkg/subgraphs/projects/src/schema.graphql | ||
| grpc: | ||
| protoFile: ../../demo/pkg/subgraphs/projects/generated/service.proto | ||
| mappingFile: ../../demo/pkg/subgraphs/projects/mapping.json |
Contributor
There was a problem hiding this comment.
Fix mappingFile path in the example config
Line 20 points to a non-existent file path for mapping.json; this example will fail as-is.
Suggested fix
- mappingFile: ../../demo/pkg/subgraphs/projects/mapping.json
+ mappingFile: ../../demo/pkg/subgraphs/projects/generated/mapping.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mappingFile: ../../demo/pkg/subgraphs/projects/mapping.json | |
| mappingFile: ../../demo/pkg/subgraphs/projects/generated/mapping.json |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/router-grpc-connect/graph.yaml` at line 20, The mappingFile value in
graph.yaml currently points to a non-existent path; update the mappingFile entry
so it references the actual relative location of mapping.json in the repository
(replace the current '../../demo/pkg/subgraphs/projects/mapping.json' value with
the correct relative path to the real mapping.json file) so the example config
loads successfully; look for the mapping.json file in the demo/pkg/subgraphs or
demo directory tree and correct the mappingFile key accordingly.
Comment on lines
+64
to
+99
| // The set of supported source schemes mirrors isValidGrpcNamingScheme on | ||
| // the control plane (dns, ipv4, ipv6, vsock, unix, unix-abstract, | ||
| // passthrough). For schemes that wrap an authority component | ||
| // (`scheme://authority/endpoint`) the authority is dropped because Connect | ||
| // targets the endpoint directly. | ||
| func normalizeConnectBaseURL(routingURL string) string { | ||
| if routingURL == "" { | ||
| return routingURL | ||
| } | ||
| if strings.HasPrefix(routingURL, "http://") || strings.HasPrefix(routingURL, "https://") { | ||
| return routingURL | ||
| } | ||
|
|
||
| // Order matters: longer prefixes must be checked before their shorter | ||
| // substrings (`unix-abstract:` before `unix:`). | ||
| grpcSchemes := []string{"unix-abstract:", "passthrough:", "dns:", "ipv4:", "ipv6:", "vsock:", "unix:"} | ||
| for _, prefix := range grpcSchemes { | ||
| if !strings.HasPrefix(routingURL, prefix) { | ||
| continue | ||
| } | ||
| rest := routingURL[len(prefix):] | ||
| // `scheme://authority/endpoint` and `scheme:///endpoint` both end up | ||
| // with a leading `//`; strip the authority and the path separator so | ||
| // only the endpoint survives. | ||
| if strings.HasPrefix(rest, "//") { | ||
| after := rest[2:] | ||
| if i := strings.Index(after, "/"); i >= 0 { | ||
| rest = after[i+1:] | ||
| } else { | ||
| rest = after | ||
| } | ||
| } | ||
| return "http://" + rest | ||
| } | ||
| return "http://" + routingURL | ||
| } |
Contributor
There was a problem hiding this comment.
Reject non-TCP resolver schemes for ConnectRPC.
normalizeConnectBaseURL rewrites unix:, unix-abstract:, and vsock: into plain http://... URLs, but the transport you build here is still a normal *http.Client. For example, unix:///tmp/grpc.sock becomes http://tmp/grpc.sock, which http.Transport will try to dial as TCP host tmp, not as a Unix socket. That means ConnectRPC subgraphs registered with these currently-advertised routing schemes will fail at runtime. Please either fail validation for non-HTTP/TCP schemes when connectrpc is selected, or add a custom dialer that actually supports those transports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@router/pkg/grpcprotocol/transport_builder.go` around lines 64 - 99,
normalizeConnectBaseURL currently maps non-TCP schemes (unix, unix-abstract,
vsock, passthrough) into an http:// URL but the HTTP client/transport still
dials TCP, causing runtime failures; update the code path that handles
ConnectRPC to either reject these non-TCP schemes early or add a proper custom
dialer for them. Specifically, in the code that selects/uses
normalizeConnectBaseURL and constructs the *http.Client / http.Transport for
ConnectRPC, detect schemes returned by normalizeConnectBaseURL (or inspect the
original routingURL prefixes such as "unix:", "unix-abstract:", "vsock:") and
(a) return a validation error when the selected protocol is connectrpc and the
scheme is not a TCP/HTTP-compatible one, or (b) implement and attach a custom
DialContext/Dial function on the http.Transport that understands unix domain
sockets and vsock and performs the correct dial for "unix:" / "unix-abstract:" /
"vsock:" endpoints; keep references to normalizeConnectBaseURL, the
http.Transport construction, and any transport builder function so reviewers can
locate the change.
fengyuwusong
changed the title
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
1 participant
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.
@coderabbitai summary
Summary
This PR adds first-class support for serving gRPC subgraphs over the ConnectRPC protocol (HTTP/1.1, Protobuf or JSON) in addition to the existing native-gRPC (HTTP/2) path. The transport is selected per subgraph through a new
grpc_protocolconfiguration block on the router, so a federated graph can mix and match transports without changing the underlying subgraph implementation.ConnectRPC is the right transport whenever end-to-end HTTP/2 to the subgraph is unavailable — for example when a corporate proxy or load balancer in front of the subgraph does not forward gRPC. A
connect-gohandler natively serves Connect, gRPC, and gRPC-Web on the same endpoint, so this change is purely additive on the subgraph side.This supersedes #2665, which proposed the same feature without the meeting follow-ups (config naming, JSON schema, docs, demo & test coverage, control plane acceptance of
http(s)://routing URLs, ConnectRPC example).Related
grpctest).What's changed
Router
grpc_protocolconfiguration block on the router config:router/pkg/grpcprotocol: validation, per-subgraph protocol/encoding resolution, and Connect-transport construction (BuildConnectTransports).router/core/factoryresolverskips the gRPC connector for ConnectRPC subgraphs and threads the matchingRPCTransportinto the data source throughgraph_server.dns:///host:port,dns:host:port, plainhost:port, and other gRPC name resolver schemes are converted tohttp://host:portfor the Connect HTTP client.GRPCProtocolConfig→GRPCProtocolConfigurationDefault→DefaultProtocolSubgraphGRPCProtocolConfig→GRPCProtocolSubgraphenvDefault/env/envPrefix:"GRPC_PROTOCOL_"tagsControl plane
isValidGrpcSubgraphRoutingURLaccepts both gRPC name resolver schemes andhttp(s)://URLs forGRPC_SERVICEsubgraphs. The router decides the actual transport at request time, so the control plane no longer needs to commit to one or the other at registration. Applied tocreateFederatedSubgraph,updateSubgraph, andpublishFederatedSubgraph.http(s)rejection are flipped to assert acceptance, and a new unit-test block forisValidGrpcSubgraphRoutingURLcovers the http/https/case-insensitive/empty/delegation paths.Demo
demo/pkg/subgraphs/projectsmigrates tobuf generatewithprotoc-gen-go,protoc-gen-go-grpc, andprotoc-gen-connect-go.cmd/service/main.goexposes the projects subgraph through a Connect handler running over H2C on:4011, which serves Connect, gRPC, and gRPC-Web from the same endpoint.ProjectsConnectServiceadapter forwards every Connect call to the existing gRPC implementation (one method per RPC, 48 in total).Documentation
docs-website/router/configuration.mdx: new "gRPC Subgraph Protocol" reference with environment-variable / YAML mapping table.docs-website/router/gRPC/concepts.mdx: new "ConnectRPC Support" section.docs-website/connect/grpc-services.mdx: short Transport Protocols paragraph linking back to the configuration reference.Examples
examples/router-grpc-connect/: a minimal example mirroringrouter-simple/that boots the projects subgraph and points the router athttp://localhost:4011withgrpc_protocol.default_protocol: connectrpc.Testing
router/pkg/grpcprotocol: 29 unit tests (config validate / resolve / build transports /normalizeConnectBaseURL).router-tests/protocol/connectrpc_subgraph_test.go: 8 e2e cases covering queries, mutations, federation entity lookups, resolver-triggered field RPCs, plus per-subgraph protocol overrides and JSON encoding.router-tests/protocol/grpc_subgraph_test.go: 78 existing cases pass — the newEnableConnectRPCtestenv flag is opt-in, the gRPC path is unchanged.router/pkg/config: testdata snapshots updated for the newGRPCProtocolfield.controlplane/test/subgraph/{create,update,publish}-subgraph.test.ts: flipped assertions to acceptance forhttp(s)URLs.Migration / backward compatibility
grpc_protocolis omitted by default; existing graphs continue to use native gRPC with no behavioural change.Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.