fix(ingress): support WebSocket over HTTP/2 (replace gorilla/websocket)#1117
fix(ingress): support WebSocket over HTTP/2 (replace gorilla/websocket)#1117Pangjiping wants to merge 8 commits into
Conversation
…ocket with coder/websocket Replace the archived gorilla/websocket with the actively-maintained coder/websocket library and add HTTP/2 Extended CONNECT (RFC 8441) support so WebSocket upgrades work when the ingress is fronted by an L7 proxy that advertises h2, or when clients connect via h2 directly. Key changes: - Replace gorilla/websocket with github.com/coder/websocket v1.8.15 - Fix isWebSocketRequest to use case-insensitive token matching for Connection header (handles "Connection: keep-alive, Upgrade" from L7 proxies) - Add HTTP/2 Extended CONNECT detection (RFC 8441) for h2 WebSocket - Add serveH2Tunnel for raw bidirectional h2-to-h1 WebSocket tunneling using ResponseController.EnableFullDuplex - Enable h2c (unencrypted HTTP/2) by default on the ingress server - Set GODEBUG=http2xconnect=1 in Helm deployment template to enable Go's Extended CONNECT support Closes opensandbox-group#1105 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pangjiping
requested review from
Generalwin,
Spground,
fengcone,
hittyt,
kevinlynx and
ninan-nn
as code owners
- Suppress errcheck on deferred CloseNow calls (fire-and-forget cleanup) - Close response body from websocket.Dial in h2 tunnel path (bodyclose) - Discard io.Copy return values in h2 tunnel (errcheck) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c45038160
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Strip h2 pseudo-headers (e.g. :protocol) before backend dial - Fix h2 tunnel: use raw TCP + manual WS handshake instead of websocket.NetConn to avoid double-framing WebSocket frames - Remove 32 KiB message cap: call SetReadLimit(-1) on both conns to match gorilla's unlimited default - Pass client subprotocols via DialOptions.Subprotocols and strip Sec-WebSocket-Protocol from forwarded headers to avoid conflicts - Preserve Set-Cookie headers from backend handshake response - Add 45s backend handshake timeout (matching gorilla's default) - Forward close codes/reasons instead of always sending 1000 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89857ac1c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Skip Host header in extraHeaders to avoid duplicate Host in raw handshake request - Read full HTTP response until \r\n\r\n instead of single Read call that may return partial headers - Flush ResponseWriter after each chunk via copyFlush helper so small WebSocket frames reach h2 clients without buffering delay - Close backend connection when h2 client disconnects to unblock the read goroutine and prevent handler/connection leaks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f9996f22a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if len(resp) > 4096 { | ||
| return fmt.Errorf("handshake response too large") |
There was a problem hiding this comment.
In the h2 tunnel path, backend upgrade responses with more than 4 KiB of headers now fail with handshake response too large. This can happen for authenticated WebSocket apps that set several cookies or other handshake headers, and it is a regression from the HTTP/1 path/previous dialer that parse normal HTTP response headers with a much larger budget. Parse the response with http.ReadResponse or use a limit comparable to the server/client header limits instead of 4096 bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6b1fb2c. Raised limit from 4KiB to 16KiB.
… and EOF cleanup - Forward client subprotocols in raw WebSocket handshake for h2 path - Wrap backend connection with TLS for wss/https schemes - Return actual backend status code (401/403/etc) instead of always 502 - Close r.Body after backend EOF to unblock client→backend goroutine Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 713f21d967
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Backend scheme is always ws:// (ingress has no TLS termination), so the wss/https TLS wrapping code was dead. Remove it along with the :443 default port branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45f13d6bed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| http.Error(rw, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| rw.WriteHeader(http.StatusOK) |
There was a problem hiding this comment.
When an h2 Extended CONNECT client requests a WebSocket subprotocol and the backend accepts one, this success branch sends the client a bare 200 without the backend's Sec-WebSocket-Protocol response header. Fresh evidence: clientSubprotocols are now forwarded into rawWebSocketHandshake, but the helper still discards successful response headers before this WriteHeader, so clients/libraries that require protocols such as graphql-ws see no negotiated protocol even though the backend selected one.
Useful? React with 👍 / 👎.
|
|
||
| var buf strings.Builder | ||
| buf.WriteString("GET " + path + " HTTP/1.1\r\n") | ||
| buf.WriteString("Host: " + target.Host + "\r\n") |
There was a problem hiding this comment.
For h2 Extended CONNECT through NewWebSocketProxy where the target URL is an internal address but the forwarded Host identifies the backend virtual host, this raw handshake always sends target.Host and then skips the Host from extraHeaders. That regresses the previous Host-preserving behavior for virtual-hosted WebSocket backends; use the forwarded Host when present instead of unconditionally writing the dial target.
Useful? React with 👍 / 👎.
| connBackend, resp, err := websocket.Dial(dialCtx, backendURL.String(), &websocket.DialOptions{ | ||
| HTTPHeader: requestHeader, | ||
| Subprotocols: clientSubprotocols, | ||
| }) |
There was a problem hiding this comment.
In the HTTP/1 path, this websocket.Dial uses coder/websocket's default http.Client, so backend 3xx handshake responses are followed inside ingress instead of being returned to the client as the old gorilla dialer did via resp. When a sandbox WebSocket endpoint redirects to /login or another host, the client now sees the final non-101 response or gets connected to the redirected target rather than receiving the backend redirect; pass an HTTPClient whose CheckRedirect returns http.ErrUseLastResponse.
Useful? React with 👍 / 👎.
| backendAddr += ":80" | ||
| } | ||
|
|
||
| backendConn, err := net.DialTimeout("tcp", backendAddr, backendHandshakeTimeout) |
There was a problem hiding this comment.
For h2 Extended CONNECT requests where the client disconnects while the backend address is blackholed or the handshake stalls, this path uses net.DialTimeout and later deadlines without r.Context(), so the handler goroutine can stay around for the full 45s timeout after there is no client left. The HTTP/1 path already ties the backend dial to the request context; use a net.Dialer.DialContext and abort the raw handshake when the request is canceled.
Useful? React with 👍 / 👎.
- rawWebSocketHandshake returns parsed response headers; validate Upgrade/Connection before tunneling (fixes handshake validation) - Forward Set-Cookie from backend handshake in h2 path - Raise handshake response size limit from 4KiB to 16KiB - Half-close backend write side (CloseWrite) instead of full Close so backend can finish sending after client disconnects - Pass Host to DialOptions.Host in h1 path for virtual-hosted backends Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b1fb2c090
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for { | ||
| n, err := src.Read(buf) | ||
| if n > 0 { | ||
| _, _ = dst.Write(buf[:n]) |
There was a problem hiding this comment.
When an h2 WebSocket client resets or disconnects while the backend keeps the socket open, dst.Write can start returning errors, but this loop ignores them and continues reading from the backend. In that case the handler never reaches the cleanup below until the backend eventually closes, leaving the backend connection and handler goroutine pinned for streaming or idle WebSocket backends; break/close the tunnel on write failure.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
gorilla/websocketwith actively-maintainedgithub.com/coder/websocketv1.8.15isWebSocketRequestto use case-insensitive token matching — handles L7 proxies sendingConnection: keep-alive, UpgradeserveH2Tunnel)http.ProtocolsGODEBUG=http2xconnect=1in Helm ingress-gateway deployment templateCloses #1105
Test plan
go build ./...passesgo vet ./...passesgo test ./pkg/proxy/... -run TestIsWebSocketRequest— token matching, case-insensitive, Extended CONNECT detectiongo test ./pkg/proxy/... -run Test_WebSocketProxy— header mode + URI mode echo round-tripGODEBUG=http2xconnect=1, verify direct h2 Extended CONNECT WebSocket tunnel🤖 Generated with Claude Code