diff --git a/TODO.md b/TODO.md index e3fee17..10c50d2 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,9 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-09-28: the server timeout test now drives `Run` and checks the + `http.Server` it serves carries the timeouts; corrected the `ReadTimeout` + note in that test (closes #120). - 2026-09-28: upaas deploy readiness — runtime image runs as unprivileged `dnswatcher`, Docker `HEALTHCHECK`, startup fails when the data directory is not writable, README "Running under upaas" (closes #147). diff --git a/internal/server/export_test.go b/internal/server/export_test.go index 8df05f2..5a1bec7 100644 --- a/internal/server/export_test.go +++ b/internal/server/export_test.go @@ -5,15 +5,20 @@ import ( "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 + +// SetListenPort overrides the port Run binds. A test uses it to hand +// Run an unbindable port so ListenAndServe fails immediately and Run +// returns after storing its http.Server. +func SetListenPort(s *Server, port int) { + s.port = port +} + +// HTTPServerOf returns the http.Server that Run built and stored, so a +// test can inspect the timeouts the running server actually carries. +func HTTPServerOf(s *Server) *http.Server { + return s.httpServer +} diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 9909f7d..6cd036c 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1,112 +1,130 @@ package server_test import ( - "net/http" "testing" + "github.com/spf13/viper" + "go.uber.org/fx" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/handlers" + "sneak.berlin/go/dnswatcher/internal/healthcheck" + "sneak.berlin/go/dnswatcher/internal/logger" + "sneak.berlin/go/dnswatcher/internal/middleware" + "sneak.berlin/go/dnswatcher/internal/notify" "sneak.berlin/go/dnswatcher/internal/server" + "sneak.berlin/go/dnswatcher/internal/state" ) -// 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) - }, +// buildServer wires a *server.Server exactly as cmd/dnswatcher does, +// minus the watcher/resolver subtree that would touch live DNS. fx +// builds the object graph but the lifecycle is never started, so no +// OnStart hook runs and nothing listens or resolves. The caller must +// first configure viper (config.New reads it), which is also why the +// caller cannot run in parallel. +func buildServer(t *testing.T) *server.Server { + t.Helper() + + var srv *server.Server + + app := fx.New( + fx.NopLogger, + fx.Provide( + globals.New, + logger.New, + config.New, + state.New, + healthcheck.New, + notify.New, + middleware.New, + handlers.New, + server.New, + ), + fx.Populate(&srv), ) -} -// 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, - ) + err := app.Err() + if err != nil { + t.Fatalf("building server graph: %v", err) } - if srv.ReadHeaderTimeout <= 0 { + return srv +} + +// TestRunWiresSocketTimeouts pins that the http.Server the running +// server actually serves — the one Run builds and hands to +// ListenAndServe — carries every socket-level timeout, plus the two +// relationships the values must satisfy. +// +// Run is driven to completion with an unbindable port: it builds and +// stores s.httpServer, then ListenAndServe fails at once and Run +// returns without ever listening. The assertions run in the same +// goroutine after Run returns, so reading s.httpServer is free of any +// data race. Nothing here measures elapsed time. +// +// ReadTimeout must be at least ReadHeaderTimeout. net/http reads the +// headers under ReadHeaderTimeout, then sets the read deadline for the +// rest of the request to ReadTimeout, counted from when it started +// reading the request. If ReadTimeout were smaller, a request whose +// headers arrived after ReadTimeout but within ReadHeaderTimeout would +// get a read deadline that had already passed, so reading its body +// would fail at once. +func TestRunWiresSocketTimeouts(t *testing.T) { + // Sets an env var and touches viper global state, so like the + // config tests it cannot use t.Parallel. + viper.Reset() + t.Setenv("DNSWATCHER_TARGETS", "example.com") + + srv := buildServer(t) + server.SetListenPort(srv, -1) + + srv.Run() + + hs := server.HTTPServerOf(srv) + if hs == nil { + t.Fatal("Run did not build an http.Server") + } + + if hs.ReadTimeout <= 0 { + t.Errorf("ReadTimeout must be non-zero, got %v", hs.ReadTimeout) + } + + if hs.ReadHeaderTimeout <= 0 { t.Errorf( "ReadHeaderTimeout must be non-zero, got %v", - srv.ReadHeaderTimeout, + hs.ReadHeaderTimeout, ) } - if srv.WriteTimeout <= 0 { - t.Errorf( - "WriteTimeout must be non-zero, got %v", - srv.WriteTimeout, - ) + if hs.WriteTimeout <= 0 { + t.Errorf("WriteTimeout must be non-zero, got %v", hs.WriteTimeout) } - if srv.IdleTimeout <= 0 { - t.Errorf( - "IdleTimeout must be non-zero, got %v", - srv.IdleTimeout, - ) + if hs.IdleTimeout <= 0 { + t.Errorf("IdleTimeout must be non-zero, got %v", hs.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 { + if hs.WriteTimeout <= server.RequestTimeout { t.Errorf( "WriteTimeout (%v) must exceed handler budget (%v)", - srv.WriteTimeout, + hs.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 { + if hs.ReadTimeout < hs.ReadHeaderTimeout { t.Errorf( "ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)", - srv.ReadTimeout, - srv.ReadHeaderTimeout, + hs.ReadTimeout, + hs.ReadHeaderTimeout, + ) + } + + if hs.Handler != srv { + t.Errorf( + "Run wired handler %T, want the *server.Server", + hs.Handler, ) } } - -// 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") - } -}