From bea80c8715b81d3565937963f9fef43fe9cf1538 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Fri, 11 Sep 2026 14:29:08 +0200 Subject: [PATCH] fix(std): restore the h2c Upgrade handshake the stdlib migration dropped celeris#440 moved this engine off x/net/http2/h2c and onto http.Server.Protocols + SetUnencryptedHTTP2. That covers prior-knowledge h2c. It does NOT cover the RFC 7540 3.2 HTTP/1.1 Upgrade handshake: net/http implements the upgrade path internally (serveHTTP2Conn takes an UpgradeRequest and Settings) but exposes no way for a caller to reach it, and the hook it does expose is reserved for x/net/http2 itself. So the migration silently removed a capability rather than relocating it. epoll and io_uring implement the handshake themselves via Config.EnableH2Upgrade and were untouched. Three engines, two behaviours, on a project whose release bar is that they agree. My commit message on #440 said "nothing depended on it: the spec suite's upgrade tests already skip std, and the benchmark harness only ever advertises H2CUpgrade for the io_uring column". Both clauses are true and the conclusion was wrong. test/spec/h2c_upgrade_test.go skips std, and its comment says why -- "std engine uses x/net/http2/h2c middleware; its upgrade path is separate" -- so the coverage had been DELEGATED to the middleware. Removing the middleware removed the coverage with it. I checked the two places that skip this engine and not the one that exercises it: the cluster nightly, which drove 3,597 upgrade preambles at kitchen_sink/std on both arches and got back not one 101. Restores h2c.NewHandler, which serves both shapes -- client preface and upgrade -- and falls through to the wrapped handler for plain HTTP/1.1. The deprecation stands; a deprecated package that does the job beats a supported one that silently does less. The stale nolint text from before #440 ("We pin Go 1.26.3") does not come back: the new comment says the actual reason, which is that no stdlib equivalent exists. Adds the case that was missing. engine/std/h2c_upgrade_test.go asserts the 101 on the engine that owns the behaviour, plus that an H2C listener still answers plain HTTP/1.1 -- the half most likely to break while fixing the first. Reinstating exactly what #440 shipped fails it: h2c upgrade got "HTTP/1.1 200 OK", want a 101 Switching Protocols engine/std and test/conformance pass. --- engine/std/engine.go | 55 ++++++++------ engine/std/h2c_upgrade_test.go | 134 +++++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+), 23 deletions(-) create mode 100644 engine/std/h2c_upgrade_test.go diff --git a/engine/std/engine.go b/engine/std/engine.go index 5546e73e..d5864a26 100644 --- a/engine/std/engine.go +++ b/engine/std/engine.go @@ -15,6 +15,17 @@ import ( "github.com/goceleris/celeris/engine" "github.com/goceleris/celeris/protocol/h2/stream" "github.com/goceleris/celeris/resource" + + "golang.org/x/net/http2" + //nolint:staticcheck // SA1019: h2c is deprecated, and its replacement + // (http.Server.Protocols + SetUnencryptedHTTP2) does NOT cover the + // RFC 7540 3.2 Upgrade handshake -- net/http implements the upgrade path + // internally but exposes no way for a caller to reach it. celeris#440 + // migrated to the stdlib and silently dropped h2c Upgrade from this + // engine; the epoll and io_uring engines still honour it, and the + // nightly caught the asymmetry. Until the stdlib exposes an equivalent, + // this package is the only way to keep the three engines agreeing. + "golang.org/x/net/http2/h2c" ) // Engine wraps net/http.Server to implement the engine.Engine interface. @@ -55,9 +66,29 @@ func New(cfg resource.Config, handler stream.Handler) (*Engine, error) { bridge := &Bridge{engine: e, handler: handler} + // h2c through the deprecated handler rather than http.Protocols, + // because it is the only one of the two that performs the RFC 7540 3.2 + // Upgrade handshake. See the import comment: dropping it took the + // std engine out of step with epoll and io_uring, which both honour + // Config.EnableH2Upgrade, and made the validation harness's h2c-churn + // slice vacuous on std -- 3,597 upgrade preambles, not one 101. + // + // h2c.NewHandler serves BOTH shapes: a client that opens with the + // HTTP/2 preface (prior knowledge) and one that asks to upgrade from + // HTTP/1.1. Plain HTTP/1.1 requests fall through to the wrapped + // handler, so an H2C listener still serves H1. + var httpHandler http.Handler = bridge + if cfg.Protocol == engine.H2C || cfg.Protocol == engine.Auto { + h2s := &http2.Server{ + MaxConcurrentStreams: cfg.MaxConcurrentStreams, + MaxReadFrameSize: cfg.MaxFrameSize, + } + httpHandler = h2c.NewHandler(bridge, h2s) //nolint:staticcheck // SA1019: see the import comment -- no stdlib equivalent covers Upgrade. + } + e.server = &http.Server{ Addr: cfg.Addr, - Handler: bridge, + Handler: httpHandler, ReadTimeout: cfg.ReadTimeout, ReadHeaderTimeout: cfg.ReadHeaderTimeout, WriteTimeout: cfg.WriteTimeout, @@ -67,28 +98,6 @@ func New(cfg resource.Config, handler stream.Handler) (*Engine, error) { BaseContext: func(net.Listener) context.Context { return e.baseCtx }, } - if cfg.Protocol == engine.H2C || cfg.Protocol == engine.Auto { - // Cleartext HTTP/2 through net/http's own HTTP/2 server, selected by - // Protocols.SetUnencryptedHTTP2. This replaces x/net/http2/h2c, which - // is deprecated (celeris#440). HTTP/1.1 stays enabled alongside it so - // a plain H1 request on an H2C listener is still served, matching what - // h2c.NewHandler did by falling through to the wrapped handler. - // - // Scope: this covers prior-knowledge h2c (client preface on a fresh - // connection). It does NOT cover the RFC 7540 3.2 HTTP/1.1 Upgrade - // handshake, which RFC 9113 removed from the specification and - // net/http does not implement. The io_uring and epoll engines still - // honour Config.EnableH2Upgrade; the std engine no longer does. - p := new(http.Protocols) - p.SetHTTP1(true) - p.SetUnencryptedHTTP2(true) - e.server.Protocols = p - e.server.HTTP2 = &http.HTTP2Config{ - MaxConcurrentStreams: int(cfg.MaxConcurrentStreams), - MaxReadFrameSize: int(cfg.MaxFrameSize), - } - } - return e, nil } diff --git a/engine/std/h2c_upgrade_test.go b/engine/std/h2c_upgrade_test.go new file mode 100644 index 00000000..6b7561b0 --- /dev/null +++ b/engine/std/h2c_upgrade_test.go @@ -0,0 +1,134 @@ +package std + +import ( + "bufio" + "context" + "encoding/base64" + "fmt" + "net" + "strings" + "testing" + "time" + + "github.com/goceleris/celeris/engine" + "github.com/goceleris/celeris/resource" +) + +// TestStdEngineAnswersH2CUpgradeWith101 is the regression guard for the hole +// celeris#440 fell through. +// +// That change moved this engine off x/net/http2/h2c and onto +// http.Server.Protocols. The stdlib serves prior-knowledge h2c but performs +// no RFC 7540 §3.2 Upgrade handshake, so the engine silently stopped +// answering upgrade requests -- while epoll and io_uring, which implement +// the handshake themselves via Config.EnableH2Upgrade, carried on. Three +// engines, two behaviours. +// +// Nothing in this repo caught it. test/spec/h2c_upgrade_test.go skips std +// for every upgrade case, and its comment says why: "std engine uses +// x/net/http2/h2c middleware; its upgrade path is separate. These tests +// target the custom engines." The coverage had been delegated to the +// middleware, so removing the middleware removed the coverage with it. The +// cluster nightly found it instead -- 3,597 upgrade preambles against +// kitchen_sink/std on both architectures and not one 101. +// +// This test is that missing case, on the engine that actually owns the +// behaviour. +func TestStdEngineAnswersH2CUpgradeWith101(t *testing.T) { + addr := startH2CEngine(t) + + c, err := net.DialTimeout("tcp", addr, 5*time.Second) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer func() { _ = c.Close() }() + _ = c.SetDeadline(time.Now().Add(10 * time.Second)) + + // RFC 7540 §3.2: an HTTP/1.1 request carrying Upgrade: h2c plus a + // base64url SETTINGS payload. An empty SETTINGS frame is valid. + settings := base64.RawURLEncoding.EncodeToString(nil) + req := fmt.Sprintf("GET / HTTP/1.1\r\nHost: %s\r\n"+ + "Connection: Upgrade, HTTP2-Settings\r\n"+ + "Upgrade: h2c\r\n"+ + "HTTP2-Settings: %s\r\n\r\n", addr, settings) + if _, err := c.Write([]byte(req)); err != nil { + t.Fatalf("write upgrade request: %v", err) + } + + line, err := bufio.NewReader(c).ReadString('\n') + if err != nil { + t.Fatalf("read status line: %v", err) + } + if !strings.HasPrefix(line, "HTTP/1.1 101") { + t.Fatalf("h2c upgrade got %q, want a 101 Switching Protocols.\n"+ + "The std engine is not performing the RFC 7540 §3.2 handshake, so it "+ + "disagrees with epoll and io_uring, which both honour Config.EnableH2Upgrade.", + strings.TrimSpace(line)) + } +} + +// TestStdEngineStillServesPlainHTTP1OnAnH2CListener guards the other half of +// the contract, which is easy to break while fixing the first: an H2C +// listener must still answer an ordinary HTTP/1.1 request. h2c.NewHandler +// gets this right by falling through to the wrapped handler, and a +// hand-rolled upgrade path is exactly where it would be lost. +func TestStdEngineStillServesPlainHTTP1OnAnH2CListener(t *testing.T) { + addr := startH2CEngine(t) + + c, err := net.DialTimeout("tcp", addr, 5*time.Second) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer func() { _ = c.Close() }() + _ = c.SetDeadline(time.Now().Add(10 * time.Second)) + + if _, err := c.Write([]byte("GET / HTTP/1.1\r\nHost: " + addr + "\r\nConnection: close\r\n\r\n")); err != nil { + t.Fatalf("write: %v", err) + } + line, err := bufio.NewReader(c).ReadString('\n') + if err != nil { + t.Fatalf("read status line: %v", err) + } + if !strings.HasPrefix(line, "HTTP/1.1 2") { + t.Fatalf("plain HTTP/1.1 on an H2C listener got %q, want a 2xx", strings.TrimSpace(line)) + } +} + +// startH2CEngine brings up an H2C std engine on a free port and returns its +// address, shutting it down when the test ends. +func startH2CEngine(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + addr := ln.Addr().String() + if err := ln.Close(); err != nil { + t.Fatalf("close probe listener: %v", err) + } + + e, err := New(resource.Config{ + Addr: addr, + Engine: engine.Std, + Protocol: engine.H2C, + }, &echoHandler{}) + if err != nil { + t.Fatalf("New: %v", err) + } + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { _ = e.Listen(ctx) }() + + // Wait for the listener rather than sleeping a fixed amount. + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + c, derr := net.DialTimeout("tcp", addr, 200*time.Millisecond) + if derr == nil { + _ = c.Close() + return addr + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("engine did not accept on %s within the deadline", addr) + return "" +}