diff --git a/README.md b/README.md index 83f9226..eb75df4 100644 --- a/README.md +++ b/README.md @@ -182,6 +182,27 @@ dnswatcher exposes a lightweight HTTP API for operational visibility: | `GET /api/v1/status` | Current monitoring state | | `GET /metrics` | Prometheus metrics (optional) | +#### Server timeouts + +The HTTP server sets all four socket-level timeouts. These are compile-time +constants in `internal/server/server.go`, not configurable via environment +variables. + +| Timeout | Value | Purpose | +|---------------------|-------|-----------------------------------------------| +| `ReadHeaderTimeout` | 10s | Bounds the request header read (slowloris) | +| `ReadTimeout` | 15s | Bounds the whole request read, headers + body | +| `WriteTimeout` | 75s | Bounds handler execution plus response flush | +| `IdleTimeout` | 120s | Reaps idle keep-alive connections | + +These are distinct from the 60s per-request handler budget applied by +`chimw.Timeout` in `internal/server/routes.go`, which cancels the request +context but does not touch the socket. `WriteTimeout` is deliberately +larger than that budget: the write deadline is armed once request headers +are read, so a smaller value would sever the connection before a handler +using its full budget could respond. `IdleTimeout` exceeds common +Prometheus scrape intervals so the scraper reuses its connection. + --- ## Architecture diff --git a/TODO.md b/TODO.md index bc4c519..60d780f 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,14 @@ confirm make check still passes. # Completed Steps +- 2026-08-09: `http.Server` now sets all four socket-level timeouts + (`ReadTimeout` 15s, `ReadHeaderTimeout` 10s, `WriteTimeout` 75s, + `IdleTimeout` 120s) as named constants in `internal/server/server.go`, + closing the slowloris / unreaped-keep-alive exposure required by + `REPO_POLICIES.md` before 1.0; `WriteTimeout` is deliberately greater + than the 60s `chimw.Timeout` handler budget so that budget stays + reachable, and tests in `internal/server` pin both the non-zero + values and that relationship (#99) - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the org-standard v2-schema config used across the org's repos diff --git a/internal/server/export_test.go b/internal/server/export_test.go new file mode 100644 index 0000000..8df05f2 --- /dev/null +++ b/internal/server/export_test.go @@ -0,0 +1,19 @@ +package server + +import ( + "net/http" + "time" +) + +// NewHTTPServer exports newHTTPServer for testing. +func NewHTTPServer( + listenAddr string, + handler http.Handler, +) *http.Server { + return newHTTPServer(listenAddr, handler) +} + +// RequestTimeout exports the handler execution budget applied by +// chimw.Timeout in SetupRoutes, so tests can assert the relationship +// between it and the server's WriteTimeout. +const RequestTimeout time.Duration = requestTimeout diff --git a/internal/server/server.go b/internal/server/server.go index 4cb30a2..f9385eb 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -33,8 +33,52 @@ type Params struct { // shutdownTimeout is how long to wait for graceful shutdown. const shutdownTimeout = 30 * time.Second -// readHeaderTimeout is the max duration for reading request headers. -const readHeaderTimeout = 10 * time.Second +// Socket-level timeouts for the HTTP server. +// +// These bound time spent on the connection itself and are a distinct +// control from the per-request handler budget enforced by +// chimw.Timeout(requestTimeout) in routes.go: that one cancels the +// request context after requestTimeout but never touches the socket, +// so without the values below a peer can hold a connection open +// forever (slowloris, unreaped keep-alives). +// +// The one hard constraint between the two controls is +// writeTimeout > requestTimeout. net/http arms the write deadline +// once the request headers have been read, so on a plaintext +// connection it covers handler execution AND the response flush. If +// writeTimeout were <= requestTimeout the server would sever the +// connection before a handler that legitimately consumed its full +// budget could emit anything, making the 60s budget unreachable in +// practice. The margin between them is the response-flush allowance. +// +// The only clients of this service are browsers loading the dashboard +// and a Prometheus scraper; the values are sized for those. +const ( + // readHeaderTimeout is the max duration for reading request + // headers. + readHeaderTimeout = 10 * time.Second + + // readTimeout bounds reading the entire request, headers plus + // body. Every route here is a GET with no body, so this only + // ever needs to cover headers; the extra 5s over + // readHeaderTimeout is slack, not a real allowance, and keeps a + // body dribbled one byte at a time from holding the read side + // open indefinitely. + readTimeout = 15 * time.Second + + // writeTimeout must exceed the requestTimeout handler budget + // (60s) per the note above. The 15s difference is the allowance + // for flushing a completed response to a slow client. + writeTimeout = 75 * time.Second + + // idleTimeout reaps keep-alive connections between requests. It + // is deliberately longer than the common Prometheus scrape + // intervals (15s/30s/60s) so the scraper reuses its connection + // rather than reconnecting every cycle, while a browser tab + // left open on the dashboard stops occupying a connection + // within two minutes of going quiet. + idleTimeout = 120 * time.Second +) // Server is the HTTP server. type Server struct { @@ -76,16 +120,29 @@ func New( return srv, nil } +// newHTTPServer builds the listening http.Server with every +// socket-level timeout set. All four are set deliberately: a zero +// value in net/http means "no limit", not "some default". +func newHTTPServer( + listenAddr string, + handler http.Handler, +) *http.Server { + return &http.Server{ + Addr: listenAddr, + Handler: handler, + ReadTimeout: readTimeout, + ReadHeaderTimeout: readHeaderTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, + } +} + // Run starts the HTTP server. func (s *Server) Run() { s.SetupRoutes() listenAddr := fmt.Sprintf(":%d", s.port) - s.httpServer = &http.Server{ - Addr: listenAddr, - Handler: s, - ReadHeaderTimeout: readHeaderTimeout, - } + s.httpServer = newHTTPServer(listenAddr, s) s.log.Info("http server starting", "addr", listenAddr) diff --git a/internal/server/server_test.go b/internal/server/server_test.go new file mode 100644 index 0000000..9909f7d --- /dev/null +++ b/internal/server/server_test.go @@ -0,0 +1,112 @@ +package server_test + +import ( + "net/http" + "testing" + + "sneak.berlin/go/dnswatcher/internal/server" +) + +// noopHandler stands in for the router; newHTTPServer only stores it. +func noopHandler() http.Handler { + return http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + ) +} + +// TestHTTPServerTimeoutsAreSet asserts that every socket-level +// timeout is configured. A zero value in net/http means "no limit", +// so a refactor that silently drops one of these reintroduces the +// slowloris / unreaped-keep-alive exposure this guards against. +// +// The assertions are on the configured field values only; nothing +// here measures elapsed time, so the test cannot flake on timing. +func TestHTTPServerTimeoutsAreSet(t *testing.T) { + t.Parallel() + + srv := server.NewHTTPServer(":8080", noopHandler()) + + if srv.ReadTimeout <= 0 { + t.Errorf( + "ReadTimeout must be non-zero, got %v", + srv.ReadTimeout, + ) + } + + if srv.ReadHeaderTimeout <= 0 { + t.Errorf( + "ReadHeaderTimeout must be non-zero, got %v", + srv.ReadHeaderTimeout, + ) + } + + if srv.WriteTimeout <= 0 { + t.Errorf( + "WriteTimeout must be non-zero, got %v", + srv.WriteTimeout, + ) + } + + if srv.IdleTimeout <= 0 { + t.Errorf( + "IdleTimeout must be non-zero, got %v", + srv.IdleTimeout, + ) + } +} + +// TestWriteTimeoutExceedsHandlerBudget pins the one relationship the +// values must satisfy. net/http arms the write deadline once request +// headers are read, so it covers handler execution plus the response +// flush. If WriteTimeout were not greater than the chimw.Timeout +// handler budget, the connection would be severed before a handler +// that used its full budget could respond, making that budget +// unreachable. +func TestWriteTimeoutExceedsHandlerBudget(t *testing.T) { + t.Parallel() + + srv := server.NewHTTPServer(":8080", noopHandler()) + + if srv.WriteTimeout <= server.RequestTimeout { + t.Errorf( + "WriteTimeout (%v) must exceed handler budget (%v)", + srv.WriteTimeout, + server.RequestTimeout, + ) + } +} + +// TestReadTimeoutCoversHeaderTimeout asserts the read deadline for +// the whole request is at least as long as the header-only deadline; +// a smaller ReadTimeout would make ReadHeaderTimeout unreachable. +func TestReadTimeoutCoversHeaderTimeout(t *testing.T) { + t.Parallel() + + srv := server.NewHTTPServer(":8080", noopHandler()) + + if srv.ReadTimeout < srv.ReadHeaderTimeout { + t.Errorf( + "ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)", + srv.ReadTimeout, + srv.ReadHeaderTimeout, + ) + } +} + +// TestHTTPServerAddrAndHandler covers the rest of the constructor so +// a future edit cannot drop the listen address or the handler. +func TestHTTPServerAddrAndHandler(t *testing.T) { + t.Parallel() + + srv := server.NewHTTPServer(":9999", noopHandler()) + + if srv.Addr != ":9999" { + t.Errorf("Addr = %q, want %q", srv.Addr, ":9999") + } + + if srv.Handler == nil { + t.Error("Handler must not be nil") + } +}