Bind the app port deliberately and document the proxy deployment (closes #268) (closes #226)

internal/server/bind_address_test.go

@@ -0,0 +1,336 @@
+package server_test
+
+import (
+	"context"
+	"net"
+	"net/http"
+	"strconv"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/fx"
+	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/globals"
+	"sneak.berlin/go/webhooker/internal/server"
+)
+
+const (
+	// loopbackV4 is the shipped BIND_ADDRESS default.
+	loopbackV4 = "127.0.0.1"
+
+	// wildcardV4 is the value a container deployment must set,
+	// where a loopback-bound process is unreachable from outside
+	// its network namespace even with a published port.
+	wildcardV4 = "0.0.0.0"
+
+	// unavailableAddr is a TEST-NET-1 address (RFC 5737). It is a
+	// well-formed literal that no host is assigned, so binding it
+	// fails with EADDRNOTAVAIL rather than succeeding somewhere
+	// unexpected.
+	unavailableAddr = "192.0.2.1"
+
+	// listenReadyTimeout bounds the wait for the listener to accept
+	// connections. The bind itself is immediate; this only covers
+	// goroutine scheduling.
+	listenReadyTimeout = 3 * time.Second
+
+	// listenPollInterval is how often the readiness wait retries.
+	listenPollInterval = 10 * time.Millisecond
+
+	// dialTimeout bounds a single connection attempt in these
+	// tests. Everything dialled here is on this host, so a dial
+	// that is not answered immediately is a failure, not slowness.
+	dialTimeout = time.Second
+)
+
+// freePort returns a TCP port that is free on every local address at
+// the moment it returns, by taking one on the wildcard and releasing
+// it. The window between release and re-bind is the standard one
+// every "pick a free port" helper carries.
+func freePort(t *testing.T) int {
+	t.Helper()
+
+	var listenCfg net.ListenConfig
+
+	l, err := listenCfg.Listen(t.Context(), "tcp", "0.0.0.0:0")
+	require.NoError(t, err)
+
+	addr, ok := l.Addr().(*net.TCPAddr)
+	require.True(t, ok, "listener is not TCP")
+	require.NoError(t, l.Close())
+
+	return addr.Port
+}
+
+// otherLocalAddr returns a local IPv4 address that is not
+// loopbackV4, or skips the test when the host has none.
+//
+// The bind-address tests need a second address of this host to stand
+// in for "another interface": what a wildcard bind claims and a
+// loopback bind does not. 127.0.0.2 is that address on Linux, where
+// the whole 127.0.0.0/8 is local; elsewhere an interface address is
+// used instead. Each candidate is proven bindable before it is
+// returned, so a host that offers neither skips rather than fails on
+// something that was never about the code under test.
+func otherLocalAddr(t *testing.T) string {
+	t.Helper()
+
+	candidates := []string{"127.0.0.2"}
+
+	ifaceAddrs, err := net.InterfaceAddrs()
+	require.NoError(t, err)
+
+	for _, a := range ifaceAddrs {
+		ipNet, ok := a.(*net.IPNet)
+		if !ok {
+			continue
+		}
+
+		ip4 := ipNet.IP.To4()
+		if ip4 == nil || ip4.String() == loopbackV4 {
+			continue
+		}
+
+		candidates = append(candidates, ip4.String())
+	}
+
+	var listenCfg net.ListenConfig
+
+	for _, candidate := range candidates {
+		l, listenErr := listenCfg.Listen(
+			t.Context(), "tcp", net.JoinHostPort(candidate, "0"),
+		)
+		if listenErr != nil {
+			continue
+		}
+
+		require.NoError(t, l.Close())
+
+		return candidate
+	}
+
+	t.Skip("host has no second local IPv4 address to bind")
+
+	return ""
+}
+
+// startBoundServer starts the wired app with the given bind address
+// on a free port and returns that port. The app is stopped on
+// cleanup.
+func startBoundServer(t *testing.T, bindAddress string) int {
+	t.Helper()
+
+	port := freePort(t)
+
+	env := newTestEnv(t)
+	env.cfg.BindAddress = bindAddress
+	env.cfg.Port = port
+
+	app := fx.New(
+		fx.NopLogger,
+		fx.Supply(env.log, env.cfg, env.mw, env.hnd),
+		fx.Provide(globals.New, server.New),
+		fx.Invoke(func(*server.Server) {}),
+	)
+
+	startCtx, cancelStart := context.WithTimeout(
+		context.Background(), lifecycleTimeout,
+	)
+	defer cancelStart()
+
+	require.NoError(t, app.Start(startCtx))
+
+	t.Cleanup(func() {
+		stopCtx, cancelStop := context.WithTimeout(
+			context.Background(), lifecycleTimeout,
+		)
+		defer cancelStop()
+
+		require.NoError(t, app.Stop(stopCtx))
+	})
+
+	return port
+}
+
+// dialable reports whether a TCP connection to addr succeeds.
+func dialable(ctx context.Context, addr string) bool {
+	dialer := net.Dialer{Timeout: dialTimeout}
+
+	conn, err := dialer.DialContext(ctx, "tcp", addr)
+	if err != nil {
+		return false
+	}
+
+	_ = conn.Close()
+
+	return true
+}
+
+// requireDialable waits for addr to accept connections, failing the
+// test if it never does.
+func requireDialable(t *testing.T, addr string) {
+	t.Helper()
+
+	deadline := time.Now().Add(listenReadyTimeout)
+	for time.Now().Before(deadline) {
+		if dialable(t.Context(), addr) {
+			return
+		}
+
+		time.Sleep(listenPollInterval)
+	}
+
+	t.Fatalf("nothing accepted connections on %s", addr)
+}
+
+// TestListenAddr pins how BindAddress and Port are rendered into the
+// listen address.
+//
+// The defect this covers was a bare fmt.Sprintf(":%d", port), which
+// binds every interface with no way to say otherwise. The IPv6 rows
+// are here because an unbracketed IPv6 host would produce an address
+// net.Listen rejects, turning a valid configuration into a startup
+// failure.
+func TestListenAddr(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		bindAddress string
+		port        int
+		expected    string
+	}{
+		{
+			name:        "loopback default",
+			bindAddress: loopbackV4,
+			port:        8080,
+			expected:    "127.0.0.1:8080",
+		},
+		{
+			name:        "ipv4 wildcard",
+			bindAddress: wildcardV4,
+			port:        8080,
+			expected:    "0.0.0.0:8080",
+		},
+		{
+			name:        "ipv6 wildcard is bracketed",
+			bindAddress: "::",
+			port:        8080,
+			expected:    "[::]:8080",
+		},
+		{
+			name:        "ipv6 literal is bracketed",
+			bindAddress: "2001:db8::5",
+			port:        9001,
+			expected:    "[2001:db8::5]:9001",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			assert.Equal(t, tt.expected, server.ListenAddrForTest(
+				&config.Config{
+					BindAddress: tt.bindAddress,
+					Port:        tt.port,
+				},
+			))
+		})
+	}
+}
+
+// TestBindAddress_LoopbackIsNotOnOtherAddresses proves the fix end to
+// end: with BIND_ADDRESS at its loopback default, the cleartext
+// listener answers on loopback and has not claimed any other address
+// of this host.
+//
+// The second address is proven free by binding it on the same port
+// while the server runs. That is the assertion that fails against the
+// old wildcard bind — a wildcard listener owns the port on every
+// address, so this bind would return EADDRINUSE. Dialling from
+// another machine is what the operator cares about, and this is the
+// in-process form of it: the socket the remote host would connect to
+// does not exist.
+func TestBindAddress_LoopbackIsNotOnOtherAddresses(t *testing.T) {
+	t.Parallel()
+
+	other := otherLocalAddr(t)
+	port := startBoundServer(t, loopbackV4)
+
+	// Positive control: the service really is up and serving.
+	requireDialable(t, net.JoinHostPort(loopbackV4, strconv.Itoa(port)))
+
+	var listenCfg net.ListenConfig
+
+	l, err := listenCfg.Listen(
+		t.Context(), "tcp",
+		net.JoinHostPort(other, strconv.Itoa(port)),
+	)
+	require.NoError(
+		t, err,
+		"port %d on %s is taken while bound to %s: the listener "+
+			"claimed more than its configured address",
+		port, other, loopbackV4,
+	)
+
+	require.NoError(t, l.Close())
+}
+
+// TestBindAddress_WildcardReachesOtherAddresses is the counterpart:
+// the value a container deployment sets does reach the addresses the
+// default withholds. Without this, a loopback-only bind would pass
+// the test above by never listening at all.
+func TestBindAddress_WildcardReachesOtherAddresses(t *testing.T) {
+	t.Parallel()
+
+	other := otherLocalAddr(t)
+	port := startBoundServer(t, wildcardV4)
+
+	requireDialable(t, net.JoinHostPort(other, strconv.Itoa(port)))
+}
+
+// TestBindAddress_ServesRequestsOnConfiguredAddress proves the bound
+// listener serves the application rather than merely accepting TCP,
+// so a bind address that is honoured cannot be mistaken for one that
+// is honoured and broken.
+func TestBindAddress_ServesRequestsOnConfiguredAddress(t *testing.T) {
+	t.Parallel()
+
+	port := startBoundServer(t, loopbackV4)
+	addr := net.JoinHostPort(loopbackV4, strconv.Itoa(port))
+	requireDialable(t, addr)
+
+	req, err := http.NewRequestWithContext(
+		t.Context(), http.MethodGet,
+		"http://"+addr+"/.well-known/healthcheck", nil,
+	)
+	require.NoError(t, err)
+
+	client := &http.Client{Timeout: dialTimeout}
+
+	resp, err := client.Do(req)
+	require.NoError(t, err)
+
+	defer func() { _ = resp.Body.Close() }()
+
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+}
+
+// TestBindAddress_UnavailableAddressShutsDownTheApp covers the half
+// of the fail-loud rule that configuration parsing cannot reach. A
+// syntactically valid address that is not assigned to this host
+// parses fine and fails at bind time, after fx has already reported
+// RUNNING. It must end the process non-zero rather than leave it
+// alive with nothing listening.
+func TestBindAddress_UnavailableAddressShutsDownTheApp(t *testing.T) {
+	t.Parallel()
+
+	env := newTestEnv(t)
+	env.cfg.BindAddress = unavailableAddr
+	env.cfg.Port = freePort(t)
+
+	requireListenFailureExit(t, env)
+}

internal/server/early_shutdown_test.go

@@ -0,0 +1,91 @@
+package server_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.uber.org/fx"
+	"sneak.berlin/go/webhooker/internal/globals"
+	"sneak.berlin/go/webhooker/internal/server"
+)
+
+// earlyStopIterations is how many start/stop cycles the race test
+// runs. The window it aims at is the gap between the OnStart hook
+// returning and the serving goroutine reaching its first field
+// access, which is microseconds wide. The race detector reports an
+// unsynchronised pair whenever it observes one, but it has to observe
+// one, so a single cycle can miss purely on scheduling. Repetition
+// makes the observation reliable; the collaborators are built once,
+// so the cycles themselves are cheap.
+const earlyStopIterations = 25
+
+// TestEarlyShutdown_NoPanicAndNoRace stops the application
+// immediately after starting it, before the serving goroutine has
+// necessarily run at all.
+//
+// Two defects live in that window. The OnStart hook returns as soon
+// as it has spawned the serving goroutine, so fx runs the stop
+// sequence against a Server whose serving goroutine may not have
+// executed a single line. cleanShutdown called Shutdown on an
+// httpServer that goroutine was supposed to assign, which was a nil
+// dereference on an early SIGTERM; and it read httpServer and
+// sentryEnabled with nothing ordering those reads against the
+// goroutine's writes, which is a data race that only surfaces once
+// something both starts and stops the server. Nothing did before this
+// test: the listen-failure test never binds, and the router tests
+// bypass the lifecycle entirely.
+//
+// httpServer is now built in New, on the constructing goroutine, so
+// it is written before any hook exists and can never be nil.
+// sentryEnabled is atomic. This test is what catches either one
+// coming back — under -race, which is how the suite runs.
+func TestEarlyShutdown_NoPanicAndNoRace(t *testing.T) {
+	t.Parallel()
+
+	// Built once: the collaborators are not what is under test, and
+	// standing up a database per iteration would make repetition too
+	// expensive to be worth having.
+	env := newTestEnv(t)
+	env.cfg.BindAddress = loopbackV4
+
+	for range earlyStopIterations {
+		requireStartStopIsClean(t, env)
+	}
+}
+
+// requireStartStopIsClean runs one start/stop cycle with no wait in
+// between, failing the test if either half errors.
+//
+// Each cycle gets a fresh fx app, so the Server under test is
+// constructed anew every time — that construction is where the
+// httpServer write now happens, and reusing one Server would test it
+// only once.
+func requireStartStopIsClean(t *testing.T, env *testEnv) {
+	t.Helper()
+
+	env.cfg.Port = freePort(t)
+
+	app := fx.New(
+		fx.NopLogger,
+		fx.Supply(env.log, env.cfg, env.mw, env.hnd),
+		fx.Provide(globals.New, server.New),
+		fx.Invoke(func(*server.Server) {}),
+	)
+
+	startCtx, cancelStart := context.WithTimeout(
+		context.Background(), lifecycleTimeout,
+	)
+	defer cancelStart()
+
+	require.NoError(t, app.Start(startCtx))
+
+	// No sleep and no readiness wait: stopping while the serving
+	// goroutine is still in flight is the whole point.
+	stopCtx, cancelStop := context.WithTimeout(
+		context.Background(), lifecycleTimeout,
+	)
+	defer cancelStop()
+
+	require.NoError(t, app.Stop(stopCtx))
+}

internal/server/export_test.go

@@ -55,6 +55,16 @@ func NewRouterForTest(
 	return s.router
 }
 
+// ListenAddrForTest exposes the address the HTTP listener binds for
+// a given Config, so the rendering of host and port — IPv6
+// bracketing above all — can be pinned without standing up a
+// listener.
+func ListenAddrForTest(cfg *config.Config) string {
+	s := &Server{params: ServerParams{Config: cfg}}
+
+	return s.listenAddr()
+}
+
 // ProbePattern is the route NewRouterWithProbeForTest adds to the
 // production route tree.
 const ProbePattern = "/probe"
@@ -80,12 +90,12 @@ func NewRouterWithProbeForTest(
 	probe http.HandlerFunc,
 ) http.Handler {
 	s := &Server{
-		log:           log,
-		mw:            mw,
-		h:             h,
-		params:        ServerParams{Config: cfg},
-		sentryEnabled: sentryEnabled,
+		log:    log,
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
 	}
+	s.sentryEnabled.Store(sentryEnabled)
 	s.SetupRoutes()
 	s.router.Handle(ProbePattern, probe)
 

internal/server/http.go

@@ -2,8 +2,9 @@ package server
 
 import (
 	"errors"
-	"fmt"
+	"net"
 	"net/http"
+	"strconv"
 	"time"
 )
 
@@ -24,21 +25,47 @@ const (
 	httpMaxHeaderBytes = 1 << 20
 )
 
-func (s *Server) serveUntilShutdown() {
-	listenAddr := fmt.Sprintf(":%d", s.params.Config.Port)
-	s.httpServer = &http.Server{
-		Addr:           listenAddr,
+// listenAddr renders the address the HTTP listener binds.
+//
+// The host half is always present: an empty host would be the
+// wildcard, and the whole point of BIND_ADDRESS is that binding every
+// interface is a choice the operator makes rather than one the
+// process makes for them. Config guarantees a literal, so
+// JoinHostPort's bracketing is enough to make IPv6 well formed.
+func (s *Server) listenAddr() string {
+	return net.JoinHostPort(
+		s.params.Config.BindAddress,
+		strconv.Itoa(s.params.Config.Port),
+	)
+}
+
+// newHTTPServer builds the HTTP server for this Server's
+// configuration.
+//
+// It is called from New, on the constructing goroutine, rather than
+// from the serving goroutine that used to assign s.httpServer
+// directly. Two goroutines reach that field — the serving goroutine
+// and the fx stop hook, which calls Shutdown on it — with nothing
+// ordering them. Constructing it during New puts the write before
+// every hook fx will later run, which both removes the race and rules
+// out the nil dereference a stop that arrived before the serving
+// goroutine had run would have caused.
+func (s *Server) newHTTPServer() *http.Server {
+	return &http.Server{
+		Addr:           s.listenAddr(),
 		ReadTimeout:    httpReadTimeout,
 		WriteTimeout:   httpWriteTimeout,
 		MaxHeaderBytes: httpMaxHeaderBytes,
 		Handler:        s,
 	}
+}
 
+func (s *Server) serveUntilShutdown() {
 	// add routes
 	// this does any necessary setup in each handler
 	s.SetupRoutes()
 
-	s.log.Info("http begin listen", "listenaddr", listenAddr)
+	s.log.Info("http begin listen", "listenaddr", s.httpServer.Addr)
 
 	err := s.httpServer.ListenAndServe()
 	if err != nil && !errors.Is(err, http.ErrServerClosed) {

internal/server/listen_failure_test.go

@@ -36,9 +36,9 @@ const lifecycleTimeout = 15 * time.Second
 //
 // The port is occupied by a listener this test holds open, on a
 // kernel-chosen port, so the failure is the real EADDRINUSE the
-// operator hits when a second instance starts. Loopback is enough to
-// collide with the server's wildcard bind: a listening socket on a
-// specific address blocks the wildcard from claiming the same port.
+// operator hits when a second instance starts. The server is pointed
+// at the same loopback address, so the collision is a direct one on
+// the exact address it asks the kernel for.
 func TestListenFailure_ShutsDownTheApp(t *testing.T) {
 	t.Parallel()
 
@@ -55,11 +55,27 @@ func TestListenFailure_ShutsDownTheApp(t *testing.T) {
 	require.True(t, ok, "listener is not TCP")
 
 	// The collaborators come from the wired graph rather than stubs,
-	// so the Server under test is the one that ships. Only the port
-	// is test-specific.
+	// so the Server under test is the one that ships. Only the
+	// listen address is test-specific.
 	env := newTestEnv(t)
+	env.cfg.BindAddress = loopbackV4
 	env.cfg.Port = addr.Port
 
+	requireListenFailureExit(t, env)
+}
+
+// requireListenFailureExit starts the wired app over env and asserts
+// that it gives up on its own with the listen-failure status, then
+// completes its stop sequence.
+//
+// Two different listen failures share it — a port already in use and
+// an address that is not on this host — because what has to hold for
+// both is the same: the failure is discovered after fx has already
+// reported RUNNING, so the only thing that can turn it into a visible
+// exit is the shutdown path under test.
+func requireListenFailureExit(t *testing.T, env *testEnv) {
+	t.Helper()
+
 	app := fx.New(
 		fx.NopLogger,
 		fx.Supply(env.log, env.cfg, env.mw, env.hnd),

internal/server/routes.go

@@ -81,7 +81,7 @@ func (s *Server) setupGlobalMiddleware() {
 	// Sentry error reporting (if SENTRY_DSN is set). Repanic is
 	// true so panics still bubble up to the Recoverer middleware
 	// registered immediately above.
-	if s.sentryEnabled {
+	if s.sentryEnabled.Load() {
 		sentryHandler := sentryhttp.New(sentryhttp.Options{
 			Repanic: true,
 		})

internal/server/server.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"os"
 	"os/signal"
+	"sync/atomic"
 	"syscall"
 	"time"
 
@@ -88,8 +89,15 @@ type ServerParams struct {
 // Server is the main HTTP server that wires up routes and manages
 // graceful shutdown.
 type Server struct {
-	startupTime   time.Time
-	sentryEnabled bool
+	startupTime time.Time
+
+	// sentryEnabled is written by the serving goroutine, in
+	// enableSentry, and read by the fx stop hook in cleanShutdown.
+	// Nothing orders those two: the OnStart hook returns as soon as
+	// the goroutine is spawned, so a stop can be running while
+	// enableSentry is still deciding. It is atomic to supply the
+	// edge the goroutines do not.
+	sentryEnabled atomic.Bool
 	log           *slog.Logger
 	cancelFunc    context.CancelFunc
 	httpServer    *http.Server
@@ -107,6 +115,7 @@ func New(lc fx.Lifecycle, params ServerParams) (*Server, error) {
 	s.mw = params.Middleware
 	s.h = params.Handlers
 	s.log = params.Logger.Get()
+	s.httpServer = s.newHTTPServer()
 
 	lc.Append(fx.Hook{
 		OnStart: func(_ context.Context) error {
@@ -142,7 +151,7 @@ func (s *Server) MaintenanceMode() bool {
 }
 
 func (s *Server) enableSentry() {
-	s.sentryEnabled = false
+	s.sentryEnabled.Store(false)
 
 	if s.params.Config.SentryDSN == "" {
 		return
@@ -163,7 +172,7 @@ func (s *Server) enableSentry() {
 	}
 
 	s.log.Info("sentry error reporting activated")
-	s.sentryEnabled = true
+	s.sentryEnabled.Store(true)
 }
 
 // serve installs the signal watcher, starts the listener and blocks
@@ -242,7 +251,7 @@ func (s *Server) cleanShutdown(ctx context.Context) {
 
 	s.cleanupForExit()
 
-	if s.sentryEnabled {
+	if s.sentryEnabled.Load() {
 		s.flushSentry(ctx)
 	}
 }