ALPN-based h2 upstream dispatch + fix silently-dropped JS-wrapper config fields#24
Conversation
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.
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Approving — clean ALPN-based h2 dispatch, and the .first() panic-guard and protocol forwarding from the bot round all look good.
One thing I'd like you to confirm rather than block on: the dropped-fields fix also re-arms pid/tid, which were being dropped before this PR — so any UDS consumer whose Harper metadata sets them will now activate the CPU-aware balancer tiebreaker. The PR verifies http2 blast radius but not pid/tid, and that's the field most likely to be set in real deployments. Can you confirm the re-arm is intended/harmless, and ideally add a wrapper-level test asserting pid/tid/protocol survive toJsUpstream? Approving on the assumption this is the intended restore-designed-behavior fix.
Minor: worth a CHANGELOG entry for the new protocol config option (public docs can wait until symphony ships).
sent with Claude Opus 4.8
…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>
… 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>
aee83bc to
5b90794
Compare
…d on main) Rebase of kris/proxy-v2-fingerprint-9 over current main (h2 upstream dispatch #24, JA4/GREASE #20, http2 flag fix #26), squashed. Includes the review fixes: tunnel passthrough gated on the upstream's accept verdict, byte-level header parsing (obs-text safe), multi-field Connection upgrade detection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d on main) (#23) Rebase of kris/proxy-v2-fingerprint-9 over current main (h2 upstream dispatch #24, JA4/GREASE #20, http2 flag fix #26), squashed. Includes the review fixes: tunnel passthrough gated on the upstream's accept verdict, byte-level header parsing (obs-text safe), multi-field Connection upgrade detection. Co-authored-by: Claude Fable 5 <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).