ALPN-based h2 upstream dispatch + fix silently-dropped JS-wrapper config fields#24
ALPN-based h2 upstream dispatch + fix silently-dropped JS-wrapper config fields#24kriszyp wants to merge 3 commits into
Conversation
…s in the JS wrapper Routes can now mark UDS upstreams with protocol: 'h2' (e.g. Harper's -h2.sock mirror, discovered via its 'protocol: h2' UDS metadata): connections that negotiate h2 in ALPN are forwarded there, everything else to the unmarked upstreams. Config invariants: h2 upstreams require a non-XFF source-address mode (header injection would corrupt h2 frames) and at least one default upstream; http2=false with h2 upstreams (unreachable) and http2=true with no h2 upstream (default upstream must speak h2) both warn. Also fixes toJsRoute/toJsUpstream silently dropping fields the Rust side supports: route http2 (ALPN advertisement, dead since e72b220) and upstream pid/tid (CPU-aware balancer tiebreaker, dead through the JS API) never reached the addon. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- xForwardedFor is now rejected for ANY h2-capable route (http2=true too, not just split-h2 destinations): an h2 client's preface would otherwise get an XFF header spliced into it. Runtime guard as well: suspended-route resolutions supply their own source mode, so h2-negotiated connections skip XFF injection rather than corrupt the preface. - h2-marked upstreams on a passthrough route are a config error (symphony never sees negotiated ALPN without terminating). - De-raced the PROXY-precedes-preface test: assert on the accumulated stream, not the first recv (header and pumped bytes are separate writes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces ALPN-based upstream dispatch, allowing HTTP/2 connections to be routed to specific h2-marked UDS upstreams while other traffic goes to default HTTP/1.x upstreams. It includes configuration validation to prevent invalid setups (such as combining HTTP/2 with X-Forwarded-For headers or using h2 upstreams without TLS termination) and adds comprehensive integration tests. The review feedback highlights three key improvements: forwarding the protocol field in the TypeScript wrapper to ensure TCP protocol validation is reachable, using safe slice access (.first()) instead of direct indexing to avoid potential panics, and replacing eprintln! with tracing::warn! to align with the application's logging framework.
… rejection, un-hardcode server.spec version The server.spec version fix also un-reds symphony's test CI, broken on main since the 0.5.0 version sync hardcoded-expectation mismatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Completes the HTTP/2 story on the symphony side (pairs with HarperFast/harper#1707, Harper's cleartext-h2 UDS mirror): symphony stays a byte-pump — no HTTP parsing — and gains protocol-aware upstream dispatch.
protocol: 'h2'(matching theprotocol: h2line in Harper's-h2.sockUDS metadata yaml).build_destinationspartitions a route's upstreams into the default (h1) destination + an optionaldestination_h2; after the TLS handshake, connections that negotiatedh2in ALPN are pumped to the h2 destination, everything else to the default.http2: trueor h2-marked upstreams) rejectssourceAddressHeader: 'xForwardedFor'— header injection would splice into the middle of the h2 connection preface; PROXY protocol rides before the preface and works for both protocols. h2 upstreams on a passthrough route are also rejected (no ALPN visibility without terminating). All-h2-upstream routes are rejected (h1 clients need somewhere to go). Runtime backstop: suspended-route resolutions supply their own source mode, so h2-negotiated connections skip XFF injection instead of corrupting the preface.toJsRoute/toJsUpstreamin the TS wrapper were silently dropping fields the Rust side supports — routehttp2(ALPN advertisement, dead since e72b220) and upstreampid/tid(the CPU-aware balancer tiebreaker) never reached the addon. Verified no deployed consumer setshttp2: true(host-manager/central-manager grepped), so re-arming it has no blast radius, but reviewers should know the flag was inert until now.Where to look
src/router.rsbuild_destinations— the upstream partition + invariants.src/proxy_conn.rs— the post-handshake ALPN selection + runtime XFF guard.ts/proxy.tsis two mapping lines but is the reason thehttp2option now does something.Notes
__test__/h2-dispatch.spec.ts(dispatch h2/h1 with PROXY-precedes-preface byte-order assertions, plus the four config-rejection cases). Full suite 38/39 — the one failure (server.spec.js"writes a status file with pid and version", 0.5.0 vs 0.4.0) is pre-existing on main from the version-sync commit.cargo clippypre-existing errors on main (cargo clippy --all-targets fails on main (10 pre-existing errors under deny(clippy::all), clippy 1.94) #21) unchanged by this diff.http2: truealone.Drafted by an LLM (Claude Fable 5).