Support gRPC proxy phase3#39
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a per-route gRPC-Web bridge, allowing the gateway to support application/grpc-web[-text] clients by translating them to the standard gRPC wire format for upstreams. The implementation includes updated request classification for both HTTP/1.1 and HTTP/2, a new GrpcWebBridge for outbound translation, and a GrpcWebInboundBodyStream decorator for decoding text-mode requests. Review feedback highlights several performance improvement opportunities, particularly regarding the reduction of redundant string allocations and copies during data translation and header normalization. Suggestions were also made to enable zero-copy passthrough for binary mode traffic to minimize overhead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65b68e6876
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
LGTM |
Support gRPC-Web bridge (Phase 3 — final phase)
Summary
Phase 3 of the gRPC proxy feature ships the gRPC-Web bridge: a per-route serialization shim that lets HTTP/1.1 and HTTP/2 clients carrying
application/grpc-weborapplication/grpc-web-text(with optional+suffix) reach existing gRPC upstream pools without a dedicated sidecar proxy. The bridge translates wire bytes to and from canonical gRPC, while every Phase 1+2 surface (Trailers-Only synthesis,grpc-timeoutdeadlines, OTelrpc.*emission, trailer-status retry, breaker classification) applies unchanged becauseis_grpc_web_ = truealways forcesis_grpc_ = trueat the classifier.This is the final phase of the gRPC proxying feature — Phases 1 and 2 are already merged on
main(PR #37 + PR #38).What ships
proxy.grpc_web.enabledfield (restart-only). Rejected at boot whenproxy.protocol = "rest". Live onprotocol∈ {grpc,auto}.IsGrpcWebMediaTypeadmitsapplication/grpc-web[-text]with optional+proto/+json/ vendor suffix; rejects spelling-adjacent neighbours (application/grpc-websocket,application/grpc-web-extras). H1 hook fires fromHttpConnectionHandler::SetHeadersCompleteCallback— gRPC-Web-only (rawapplication/grpcon H1 stays unclassified per PROTOCOL-HTTP2 §3.2). H2 classifier extension reuses the existing dispatch site.GrpcWebInboundBodyStreamdecorator with the Rev 4 F3 Read matrix (neverOK + bytes_read == 0; explicit WOULD_BLOCK / END_OF_STREAM / ABORTED states). Buffered text-mode: in-place base64 decode inProxyTransaction::Start()BEFORE dispatch. Binary mode: transparent passthrough. Outboundcontent-typerewritten toapplication/grpc; client-sentte: trailersstripped (force-re-injected by the existingclient_te_trailers_ || is_grpc_fold).0x80 + 4-byte BE length + lowercase ASCII header lines + ': ' separator + CRLF between lines, no trailing CRLF after the final line. Emitted as DATA on the streaming path (OnResponseCompletegRPC-Web fork) and appended toresponse_body_on the buffered path (BuildClientResponsegRPC-Web fork). When upstream omits trailers, the bridge synthesisesgrpc-statusfromMapHttpToGrpcStatus(response_status_code)and writes it to the observability snapshot.RewriteTrailersOnlyForGrpcWebhoisted intoHttpServer::FinalizeIfSnapshot(H1 wrap-ownership) + explicit per-callsite calls at every H2 emission site (Http2Session::DispatchStreamRequest{,Streaming}, H2 sync dispatch callback inSetupHttp2Handlers, H2 async-resume submit lambda, H2 async-handler completion callback). All paths idempotent viaIsTrailersOnly()+IsGrpcWebRewritten()defense-in-depth flags.PumpH1StreamingBody_ABORTED, H2 streaming data-source ABORTED, buffered cap-overrun, andMaybeRetrydenial all callReleaseBreakerAdmissionNeutral()before terminal error. Wire status maps toRESULT_PARSE_ERROR → INTERNAL(Rev 1 user decision: no new RESULT_* codes).H2StreamingAbortCallbackextended from(int code, const std::string& msg)to(int code, const std::string& msg, bool breaker_neutral).ProxyTransaction::MakeDeferredErrorCallback's closure consumes the flag and callsReleaseBreakerAdmissionNeutral()beforeOnError.HttpResponse::ClearPreservedContentLength()helper.OnBodyChunkafterTranslateOutboundData, and buffered terminal afterFlushAndBuildTrailerFrame. Overrun emits a deterministicMakeGrpcWebErrorResponseTrailers-Only INTERNAL response with snapshot-write-before-delivery.Architecture
Four extension axes against the Phase 1+2 baseline:
IsGrpcWebMediaTypestrict parser +MaybeClassifyGrpcWebOnH1H1 hook + H2ClassifyRequestextension. Both flipsis_grpc_web_ANDis_grpc_=true.GrpcWebBridge— per-request shim owned byProxyTransaction::grpc_web_bridge_(unique_ptr). Holds binary/text mode + 3-byte outbound residue buffer ++suffixfor response content-type echo. Reset across retries via newGrpcWebBridge::Reset().GrpcWebInboundBodyStream— consumer-side decode decorator wrapping the parser's body stream. All 13 BodyStream virtuals forwarded; text-mode Read decodes on demand; SnapshotForSubmit clamps residue-only tobytes_queued ≥ 1.RewriteTrailersOnlyForGrpcWebhoisted intoFinalizeIfSnapshotfor H1, explicit per-site calls for H2, both wraps run BEFORE wire submit. Audit table covers 9 H2 callsites.Locked operator decisions (per the design plan Rev 7):
content-typedrives bridge mode; HTTPAcceptparsed for log/observability but ignored — documented compatibility limitation).protocol ∈ {grpc, auto};restrejected at boot.grpc-encoding/grpc-accept-encodingpass through verbatim (the bridge is a wire-format shim, not a message-compression transformer).Files changed
24 files, +1112 / -60 lines.
New files (6):
include/grpc/grpc_web_bridge.h+server/grpc_web_bridge.cc— bridge class,IsGrpcWebMediaType,MaybeClassifyGrpcWebOnH1,RewriteTrailersOnlyForGrpcWeb,MakeGrpcWebErrorResponse,ComputeClientFacingContentType,IsGrpcWebBridgeDecodeFailureReason,BuildTrailerFrame,FlushAndBuildTrailerFrame.include/upstream/grpc_web_inbound_body_stream.h+server/grpc_web_inbound_body_stream.cc— consumer-side decode decorator.test/grpc_web_test.h— 67 unit tests.test/grpc_web_edge_test.h— 24 edge / race / leak / perf tests.Modified files (18): http_request.h (gRPC-Web fields), http_response.h (new helpers +
grpc_web_rewritten_flag), route_options.h (grpc_web_enabled), server_config.h (GrpcWebConfig), upstream_callbacks.h (3-arg callback), upstream_h2_stream.h (streaming_abort_breaker_neutralfield), proxy_transaction.h (bridge + flags), util/base64.{h,cc} (strictDecodeStandard+ INT_MAX guard), config_loader.cc, http_connection_handler.cc, http_server.cc, http2_session.cc, upstream_h2_connection.cc, proxy_transaction.cc, grpc_synthesis.{h,cc}, header_rewriter.cc, plus 4 test fixtures (h2_upstream_test.h, grpc_test.h, grpc_proxy_test.h, run_test.cc).Build / CI:
Makefile(new sources +test_grpc_web/test_grpc_web_edgetargets),.github/workflows/ci.yml(new suites inbuild-linux-tsan-rest+build-macos),.github/workflows/weekly-valgrind.yml(memory-safety subset).Docs:
docs/grpc.md(operator-facing gRPC-Web section + compatibility limitations).Test coverage
1826 / 1826 tests passing across two consecutive stability runs. Baseline was 1728; Phase 3 adds 98 tests:
test/grpc_web_test.h— bridge class, decoder, classifier, helpers, decorator Read matrix.test/grpc_web_edge_test.h— concurrent multi-stream gRPC-Web on a single H2 connection, client disconnect mid-decode, 10K-iteration leak guards, perf upper bounds onTranslateOutboundData/BuildTrailerFrame/IsGrpcWebMediaType.test/grpc_proxy_test.h—TestGW1_TrailersOnlyRewriteOnGrpcWebRoute(H2 Trailers-Only synthesis → in-stream trailer-frame on the wire) andTestGW2_BufferedTextDecodesInboundBody(regression guard for the bridge-construction-order bug caught in code review).Review trail
The PR went through extensive iterative review before merge:
gateway-code-reviewer)/code-review xhigh(5 finder angles + Phase-3 sweep)Key bugs caught and fixed during review:
Start().ResetForRetryAttemptdid not clear bridge residue; text-mode retries would have prepended stale 1-2 byte residue to attempt-2 output (corrupted gRPC frame). Fixed by addingGrpcWebBridge::Reset().rewritten_headers_["content-length"]afterDecodeBufferedTextBody.response_trailers_(non-gRPC upstream or misbehaving gRPC) produced a 5-byte trailer-frame with NO grpc-status. Fixed by synthesising grpc-status fromMapHttpToGrpcStatus(response_status_code)when trailers are empty.obs_snapshotpopulated;SynthesizeMiddlewareRejectsilently skipped the snapshot write, exportingrpc.response.status_code = __missing__for every async-handler 4xx/5xx on gRPC routes. Fixed by capturingobs_snap_localinto the synthetic request.Compatibility notes
proxy.grpc_web.enabled(default false). All Phase 1+2 behaviour forapplication/grpctraffic is unchanged.H2StreamingAbortCallbacksignature change (std::functionalias ininclude/upstream/upstream_callbacks.h) is the only public-interface change — extended from 2-arg to 3-arg. Production callers (ProxyTransaction::MakeDeferredErrorCallback) updated in this PR. Any downstream sink overridingMakeDeferredErrorCallbackmust update the lambda signature.docs/grpc.md:Acceptis parsed for log but does not override response encoding.grpc-encoding/grpc-accept-encodingpass through verbatim; the bridge is not a per-message compression transformer.