From 62576f6fc65e53f1c534799473fc64258d05a693 Mon Sep 17 00:00:00 2001 From: clawbot Date: Mon, 24 Aug 2026 03:38:44 +0200 Subject: [PATCH] Bind the app port deliberately and document the proxy deployment (closes #268) (closes #226) --- Dockerfile | 12 + README.md | 271 +++++++++++++++++++- internal/config/config.go | 104 +++++++- internal/config/env_test.go | 280 ++++++++++++++++++--- internal/config/export_test.go | 10 + internal/server/bind_address_test.go | 336 +++++++++++++++++++++++++ internal/server/early_shutdown_test.go | 91 +++++++ internal/server/export_test.go | 20 +- internal/server/http.go | 39 ++- internal/server/listen_failure_test.go | 26 +- internal/server/routes.go | 2 +- internal/server/server.go | 19 +- 12 files changed, 1154 insertions(+), 56 deletions(-) create mode 100644 internal/server/bind_address_test.go create mode 100644 internal/server/early_shutdown_test.go diff --git a/Dockerfile b/Dockerfile index cfecdf6..bb47620 100644 --- a/Dockerfile +++ b/Dockerfile @@ -109,6 +109,18 @@ USER webhooker EXPOSE 8080 +# The binary defaults BIND_ADDRESS to 127.0.0.1, which is right for a +# bare host: the cleartext listener serves the admin UI and the +# unauthenticated receiver, so it must not appear on every interface +# of a machine that configured nothing. A container is the other case. +# Its network namespace is already the isolation boundary, so binding +# every address inside it exposes nothing; what decides exposure is +# the publish flag, and `-p 127.0.0.1:8080:8080` is the operator's +# control there. Shipping the image on loopback would buy no security +# and would make the process unreachable through its own published +# port. +ENV BIND_ADDRESS=0.0.0.0 + HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ CMD wget --no-verbose --tries=1 --spider http://localhost:8080/.well-known/healthcheck || exit 1 diff --git a/README.md b/README.md index 33589ce..f0660ec 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,7 @@ TTY detection, and security headers are always applied. | ----------------------- | ----------------------------------- | -------- | | `WEBHOOKER_ENVIRONMENT` | `dev` or `prod` | `dev` | | `PORT` | HTTP listen port | `8080` | +| `BIND_ADDRESS` | IP address the HTTP listener binds. Loopback by default, so the cleartext listener is not published on every interface. The Docker image ships `0.0.0.0` instead. See [Bind address](#bind-address) | `127.0.0.1` (image: `0.0.0.0`) | | `DATA_DIR` | Directory for all SQLite databases | `/var/lib/webhooker` | | `DEBUG` | Enable debug logging | `false` | | `MAINTENANCE_MODE` | Report `maintenanceMode: true` in the healthcheck JSON. It does not change how any request is served — no maintenance page exists | `false` | @@ -240,6 +241,68 @@ A set but unparseable value aborts startup. When the list is non-empty webhooker logs it at startup, blocks and all, so the hole is visible in the log of any deployment that has one. +#### Bind address + +`BIND_ADDRESS` is the IP address the HTTP listener binds. The binary +defaults to `127.0.0.1`, so a bare webhooker is reachable only from the +host it runs on. The Docker image ships `ENV BIND_ADDRESS=0.0.0.0` +instead — see below for why the two differ. + +That listener speaks **cleartext**, and it serves both the admin UI and +the unauthenticated webhook receiver. webhooker terminates no TLS +itself; a production deployment puts a reverse proxy in front of it +(see +[Deployment behind a reverse proxy](#deployment-behind-a-reverse-proxy)), +and the proxy reaches it over loopback. A default that bound every +interface would leave that cleartext port answering the internet +alongside the proxy — the admin login form and the receiver, in the +clear, on a port nobody chose to publish. Reaching webhooker from +another host is therefore something you configure, not something you +get by default. + +**In a container the answer is `0.0.0.0`, which is why the image ships +that.** A container's network namespace is already the boundary the +loopback default is reaching for: nothing outside the container gets to +`0.0.0.0:8080` because of the namespace, whatever the process bound. +Exposure is decided at the publish flag instead — `-p +127.0.0.1:8080:8080` rather than `-p 8080:8080` — which is the +operator's to choose and is what +[Running with Docker](#running-with-docker) shows. A loopback bind +inside a container buys nothing and makes the process unreachable +through its own published port. + +The value must be an IP address literal: + +- `127.0.0.1` — loopback only (the binary's default). Use this with a + reverse proxy on the same host. +- `0.0.0.0` — every IPv4 address. The image's default; on a bare host, + only behind a firewall on the port. +- `::` — every address, IPv6 and (on Linux, with the default + `net.ipv6.bindv6only=0`) IPv4 as well. +- A specific address such as `10.0.0.5` — that interface only. + +An **empty** value is treated as unset, as everywhere else here, and +takes the default. In a container that matters: `BIND_ADDRESS=` throws +away the image's `0.0.0.0` and falls back to the binary's +`127.0.0.1`, which is the one quiet failure this setting has — see +[Running with Docker](#running-with-docker). + +Hostnames are **not** accepted. `localhost` aborts startup rather than +being resolved: which of `127.0.0.1` and `::1` it means differs by +host, a name can resolve to several addresses of which only one could +be bound, and the answer can change under a running process. A value +carrying a port (`127.0.0.1:8080`) is likewise rejected — the port is +`PORT`'s business. Any unparseable value aborts startup; see +[Invalid values abort startup](#invalid-values-abort-startup). + +An address that parses but is not assigned to this host — say +`10.0.0.5` on a machine that has no such interface — is a valid +literal, so it reaches the listener and fails there. The process logs +the bind error and exits non-zero rather than staying up with nothing +listening. The effective value is in the `bindAddress` field of the +startup log line, which is the way to check what a running deployment +actually bound. + #### Metrics credentials `METRICS_USERNAME` and `METRICS_PASSWORD` are set together or not at @@ -397,8 +460,11 @@ additionally be a number in the range 1–65535, `RECEIVER_RATE_LIMIT` must be at least 1, `RETENTION_SWEEP_INTERVAL` must be greater than zero (it is a ticker period, so `0s` or a negative value would crash the reaper after -startup), and every entry in `TRUSTED_PROXIES` and -`ALLOWED_EGRESS_CIDRS` must be a CIDR block or a bare IP address. +startup), every entry in `TRUSTED_PROXIES` and +`ALLOWED_EGRESS_CIDRS` must be a CIDR block or a bare IP address, and +`BIND_ADDRESS` must be an IP address literal — `localhost`, +`127.0.0.1:8080` and `10.0.0.0/8` are each rejected rather than +resolved, split, or narrowed to something they do not say. `SESSION_IDLE_TIMEOUT` is the exception: a non-positive value there means idle expiry is disabled, not invalid. @@ -543,12 +609,54 @@ decision: ```bash docker run -d \ - -p 8080:8080 \ + -p 127.0.0.1:8080:8080 \ -v /path/to/data:/var/lib/webhooker \ -e WEBHOOKER_ENVIRONMENT=prod \ + -e BIND_ADDRESS=0.0.0.0 \ webhooker:latest ``` +**The image and the bare binary default `BIND_ADDRESS` differently, on +purpose.** The binary defaults to `127.0.0.1`; the image ships +`ENV BIND_ADDRESS=0.0.0.0`, so the `-e BIND_ADDRESS=0.0.0.0` above is +belt-and-braces and the command works without it. + +The two cases are not the same question. On a bare host, `0.0.0.0` +puts the cleartext admin UI and the unauthenticated receiver on every +interface of the machine, which is what the loopback default exists to +prevent. In a container, the network namespace is already that +boundary: nothing outside reaches `0.0.0.0:8080` because of the +namespace, not because of the bind. What decides exposure there is the +**publish flag**, and that is the line to get right. + +So publish to `127.0.0.1:8080` rather than `8080`. A bare +`-p 8080:8080` opens the port on every interface of the host — through +firewall rules too, since Docker's forwarding rules are inserted ahead +of most host firewalls. Publish to the host address your reverse proxy +connects from, and nothing wider. + +**An empty `BIND_ADDRESS` is treated as unset**, like every other +variable here, so `-e BIND_ADDRESS=` does not mean "keep the image +default" — it discards the image's `0.0.0.0` and falls back to the +_binary's_ `127.0.0.1`. In a container that is the failure below, and +nothing in the logs names the variable. A templated Compose file or a +`.env` line with an empty value is the usual way in. Either set a +literal or leave the variable out entirely. + +Overriding `BIND_ADDRESS` to a loopback address in a container — by +that route or deliberately — makes the container unreachable from +outside its namespace even with `-p`. The published port answers +nothing, and the health check fails too: it requests +`http://localhost:8080`, `localhost` resolves to `::1` first, and a +`127.0.0.1` bind is not listening there. The container then goes +`unhealthy` about **65 seconds** after start — from `HEALTHCHECK +--start-period=5s --interval=30s --retries=3`, so failing probes at +5s, 35s and 65s, and `unhealthy` on the third. (Docker's probe cadence +during the start period has changed between versions; re-derive from +those three values rather than trusting the figure. Measured at 65s on +Docker 29.7.2.) A container `unhealthy` with `connection refused` in +its health log, or a published port that resets connections, is this. + The container runs as a non-root user (`webhooker`, UID 1000), exposes port 8080, and includes a health check against `/.well-known/healthcheck`. The `/var/lib/webhooker` volume holds all @@ -558,6 +666,151 @@ databases written by `database` targets (`archive-{uuid}.db`). Mount this as a persistent volume to preserve data across container restarts. +## Deployment behind a reverse proxy + +webhooker terminates no TLS of its own. It serves plaintext HTTP and +expects a reverse proxy in front of it, which is the deployment it is +built for: the proxy holds the certificate, and webhooker binds +loopback where only the proxy can reach it. + +Five things have to be right. Each one is silent when it is wrong — +the service comes up, serves pages, and is broken in a way nothing +reports. + +1. **Bind or firewall the app port.** The binary binds `127.0.0.1` by + default, so the cleartext listener is not published beside the + proxy. The image binds `0.0.0.0` inside its own network namespace + and relies on the publish address instead — + `-p 127.0.0.1:8080:8080`. Either way the port must reach the proxy + and nothing else; widen it only with a firewall or a publish + address in front of it. A cleartext port answering the internet + serves the admin login form and the unauthenticated receiver with + no TLS at all, and the proxy in front of it changes nothing about + that. +2. **Set `WEBHOOKER_ENVIRONMENT=prod`, and make sure the proxy sends + `X-Forwarded-Proto`.** These are two requirements, not one. The + environment setting decides CORS and nothing else: the default + `dev` answers every origin with `Access-Control-Allow-Origin: *` + (without credentials), which a server-rendered production + deployment has no use for. Cookie `Secure` and the strict + Origin/Referer mode are **not** tied to it — they are decided per + request from the transport, which behind a proxy means the + `X-Forwarded-Proto` header. The block below sets it; without it + every request is read as plaintext and cookies ship without + `Secure`. See [Configuration](#configuration). +3. **Set `TRUSTED_PROXIES` to the proxy's address.** Unset, every rate + limiter keys on the connecting peer, which behind a proxy is the + proxy on every request: all clients collapse into one global bucket + per limit and the receiver's per-IP limits become service-wide + ceilings. See [Trusted proxies](#trusted-proxies). List the proxy + and nothing else. +4. **Send `Host` as `$http_host`, not `$host`.** `$host` strips the + port. webhooker's Origin/Referer check compares against the host it + was given, so on any port other than 443 `$host` makes every form + POST — including login — fail with `403 origin invalid`, with + nothing in the error naming the cause. +5. **Keep the proxy's access log.** webhooker's own access log records + the peer address, which behind a proxy is always the proxy. The + proxy's log is the only record of which client sent what. nginx's + default `combined` format already logs `$remote_addr`; do not + replace it with one that drops the client address, and retain those + logs as long as you would want to answer a question about traffic. + +### nginx + +Complete server block. Replace the `server_name` and the two +certificate paths. + +```nginx +server { + listen 443 ssl; + listen [::]:443 ssl; + http2 on; # nginx 1.25.1+; older: listen 443 ssl http2; + + server_name webhooker.example.com; + + ssl_certificate /etc/ssl/certs/webhooker.example.com.crt; + ssl_certificate_key /etc/ssl/private/webhooker.example.com.key; + ssl_protocols TLSv1.2 TLSv1.3; + + # webhooker caps form POST bodies at 1 MB. nginx's default happens + # to match, so leaving this out breaks nothing today — but if you + # ever raise webhooker's cap, this is the limit you will still be + # hitting, and the rejection is nginx's HTML page rather than + # webhooker's message. + client_max_body_size 1m; + + # $remote_addr is the client. webhooker's own log records this + # proxy and nothing else, so this file is the only place the + # client's address is written down. + access_log /var/log/nginx/webhooker.access.log combined; + + location / { + # A literal address, not localhost: with BIND_ADDRESS at its + # 127.0.0.1 default, a localhost that resolves to ::1 first + # gets connection refused. + proxy_pass http://127.0.0.1:8080; + + # $http_host, NOT $host. $host drops the port and every form + # POST fails with 403 origin invalid on any port but 443. + proxy_set_header Host $http_host; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + + # Above webhooker's own 60s request timeout, so its 503 + # reaches the client instead of nginx cutting the connection + # first and answering 504. + proxy_read_timeout 70s; + } +} + +server { + listen 80; + listen [::]:80; + server_name webhooker.example.com; + return 308 https://$host$request_uri; +} +``` + +`X-Forwarded-For` must be **appended** to, which +`$proxy_add_x_forwarded_for` does. webhooker reads no other forwarded +client header: `X-Real-IP` and `True-Client-IP` are ignored from every +peer, so setting them has no effect. See +[Trusted proxies](#trusted-proxies) for how the chain is walked. + +`X-Forwarded-Proto: https` is what tells webhooker the request arrived +over TLS, which decides the `Secure` flag on both the session and CSRF +cookies and the strict Origin/Referer mode. Without it, requests are +treated as plaintext and the cookies ship without `Secure`. Unlike +`X-Forwarded-For`, this header is read from any peer and is not gated +by `TRUSTED_PROXIES`, so the proxy must overwrite whatever a client +sent — `$scheme` above does. + +With that block, webhooker's environment is: + +```sh +WEBHOOKER_ENVIRONMENT=prod +BIND_ADDRESS=127.0.0.1 # the default; stated here to be explicit +TRUSTED_PROXIES=127.0.0.1 +``` + +If nginx runs on another host, `BIND_ADDRESS` becomes the address it +connects to, `TRUSTED_PROXIES` becomes nginx's address, and the port +must be firewalled to that address — the traffic between them is +cleartext. + +### HSTS is always sent, and is not configurable + +Every response carries +`Strict-Transport-Security: max-age=63072000; includeSubDomains; preload`. +Two years, every subdomain, and a `preload` token. There is no setting +that changes or suppresses it. + +This is worth knowing before the first request reaches a browser: a +client that sees it once will refuse plaintext HTTP to that hostname — +and to every subdomain of it — for two years, whatever else is served +there. Terminate TLS on a hostname you are prepared to keep on HTTPS. + ## Backup, Restore, and Upgrades ### What to back up @@ -696,6 +949,18 @@ Upgrade procedure: `curl -s http://host:8080/.well-known/healthcheck` reports the version it was stamped with (see [Version stamping](#version-stamping)). +**Upgrading past the introduction of `BIND_ADDRESS`:** earlier versions +always bound every interface. **Container deployments are unaffected** +— the image ships `ENV BIND_ADDRESS=0.0.0.0`, so a `docker run` or +Compose service that worked before still works with nothing changed. + +A **bare binary** is the case that changes: the listener now binds +`127.0.0.1` unless `BIND_ADDRESS` says otherwise, so a deployment that +relied on reaching it from another host becomes unreachable until it +sets the address the proxy connects to. Check the `bindAddress` field +of the startup log to see what a running process bound. See +[Bind address](#bind-address). + **Downgrade is unsupported.** Once a newer binary has migrated the files there is no way to move them back. `AutoMigrate` is additive — it adds tables, columns and indexes and never drops or rewrites them — so an diff --git a/internal/config/config.go b/internal/config/config.go index 9c886a1..b732848 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -33,6 +33,34 @@ const ( // defaultPort is the default HTTP listen port. defaultPort = 8080 + // defaultBindAddress is the interface the plaintext HTTP + // listener claims when BIND_ADDRESS is unset. + // + // Loopback, because the listener speaks cleartext and serves + // both the admin UI and the unauthenticated receiver: a + // wildcard default publishes them on every interface of every + // host that never configured anything, which is the failure + // this default exists to prevent. Reaching webhooker from off + // the host is then a deliberate act — a reverse proxy in front + // of it, or an explicit BIND_ADDRESS. + // + // This is the binary's default only. The Dockerfile ships + // ENV BIND_ADDRESS=0.0.0.0, so a container deployment needs + // nothing set and is unaffected by this constant. The two + // differ because they answer different questions: a container's + // network namespace is already the boundary this default is + // reaching for, so binding every address inside it exposes + // nothing, and what decides exposure there is the publish flag + // (-p 127.0.0.1:8080:8080). A loopback bind inside a container + // buys no security and makes the process unreachable through + // its own published port. + // + // The split is expressed as two explicit defaults rather than + // container auto-detection, because a heuristic that guesses + // wrong opens the cleartext port exactly where nobody is + // looking. + defaultBindAddress = "127.0.0.1" + // defaultRetentionSweepInterval is how often the retention // reaper deletes events older than each webhook's RetentionDays. defaultRetentionSweepInterval = time.Hour @@ -75,6 +103,10 @@ var ErrInvalidPort = errors.New("invalid port") // nor a bare IP address. var ErrInvalidCIDR = errors.New("invalid CIDR") +// ErrInvalidBindAddress is returned when BIND_ADDRESS is set to +// something that is not an IP address literal. +var ErrInvalidBindAddress = errors.New("invalid bind address") + // ErrIncompleteMetricsAuth is returned when exactly one of // METRICS_USERNAME and METRICS_PASSWORD carries a value. Neither // fallback is acceptable: serving /metrics on the username alone @@ -105,6 +137,13 @@ type Config struct { Port int SentryDSN string + // BindAddress is the IP address the plaintext HTTP listener + // binds, as an address literal. It defaults to + // defaultBindAddress and is never empty: an empty string would + // mean the wildcard to net.Listen, which is the opposite of the + // default this ships. + BindAddress string + // RetentionSweepInterval is how often the retention reaper runs. // Always positive: it becomes a time.NewTicker period. RetentionSweepInterval time.Duration @@ -387,6 +426,42 @@ func envPrefixList(key string) ([]netip.Prefix, error) { return prefixes, nil } +// envBindAddress returns the value of the named environment variable +// parsed as an IP address literal. An unset (or empty, or +// whitespace-only) value yields defaultValue. +// +// Only literals are accepted: no hostname is resolved, so `localhost` +// is an error rather than a DNS lookup at startup whose answer could +// be either loopback family, could change under the process, and +// could return several addresses of which only one would be bound. A +// value with a port in it (`127.0.0.1:8080`) is likewise an error — +// the port is PORT's business, and silently accepting it would bind +// something other than what was asked for. +// +// A set value that is not a literal is a hard error naming the key +// and the bad value, so startup fails loudly rather than falling back +// to a default the operator plainly did not want. A literal that is +// not an address of this host parses here and fails at listen time +// instead, which ends the process non-zero. +func envBindAddress(key, defaultValue string) (string, error) { + v := strings.TrimSpace(os.Getenv(key)) + if v == "" { + return defaultValue, nil + } + + addr, err := netip.ParseAddr(v) + if err != nil { + return "", fmt.Errorf( + "%w: %s: %q must be an IP address literal such as "+ + "127.0.0.1, 0.0.0.0 or ::, not a hostname and not "+ + "host:port: %w", + ErrInvalidBindAddress, key, v, err, + ) + } + + return addr.String(), nil +} + // resolveMetricsAuth reads the /metrics basic-auth credentials and // rejects a half-set pair, naming both variables either way. The // error carries neither value: the password is a secret. @@ -431,6 +506,27 @@ func resolveEnvironment() (string, error) { return environment, nil } +// resolveListener reads the two variables that describe the HTTP +// listener: which port it claims and which address it claims it on. +// They are read together because neither is meaningful alone, and +// because a validation failure in either has to abort startup before +// anything binds. +func resolveListener() (int, string, error) { + port, err := envPort("PORT", defaultPort) + if err != nil { + return 0, "", err + } + + bindAddress, err := envBindAddress( + "BIND_ADDRESS", defaultBindAddress, + ) + if err != nil { + return 0, "", err + } + + return port, bindAddress, nil +} + // loadFromEnv builds a Config from the environment. Every value that // needs parsing fails loudly when it is set but unparseable: the // documented defaults apply only to variables that are unset (or @@ -442,7 +538,7 @@ func loadFromEnv() (*Config, error) { return nil, err } - port, err := envPort("PORT", defaultPort) + port, bindAddress, err := resolveListener() if err != nil { return nil, err } @@ -506,6 +602,7 @@ func loadFromEnv() (*Config, error) { MetricsUsername: metricsUsername, MetricsPassword: metricsPassword, Port: port, + BindAddress: bindAddress, SentryDSN: envString("SENTRY_DSN"), RetentionSweepInterval: retentionSweepInterval, SessionIdleTimeout: sessionIdleTimeout, @@ -625,6 +722,11 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { log.Info("Configuration loaded", "environment", s.Environment, "port", s.Port, + // Logged because which interfaces the cleartext listener + // answers on is not otherwise observable from inside a + // container, and it decides whether anything but the local + // host can reach the admin UI. + "bindAddress", s.BindAddress, "debug", s.Debug, "maintenanceMode", s.MaintenanceMode, "dataDir", s.DataDir, diff --git a/internal/config/env_test.go b/internal/config/env_test.go index a127abe..bc67167 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -21,6 +21,22 @@ const ( envKeyPort = "PORT" envKeyDebug = "DEBUG" envKeyMaintenanceMode = "MAINTENANCE_MODE" + envKeyBindAddress = "BIND_ADDRESS" +) + +// Sample BIND_ADDRESS values used by the tables below. +const ( + // bindAddressDefault is the shipped default. It is asserted + // against the package's own constant in + // TestNewUsesDefaultsWhenUnset, so the two cannot drift. + bindAddressDefault = "127.0.0.1" + + // bindAddressWildcard is the value a container deployment sets. + bindAddressWildcard = "0.0.0.0" + + // bindAddressSample is an arbitrary specific address, standing + // for "one interface of several". + bindAddressSample = "10.1.2.3" ) // envBoolCase is one row of the envBool table. @@ -291,6 +307,160 @@ func TestEnvPort(t *testing.T) { } } +// TestEnvBindAddress covers BIND_ADDRESS parsing. +// +// Only IP address literals are accepted. Every rejection below is a +// value an operator plausibly writes — a hostname, a host:port, a +// CIDR block — and each has to abort startup rather than fall back to +// the default, because falling back would bind an address other than +// the one asked for and, in the wildcard-default case this setting +// exists to end, publish cleartext on every interface. +func TestEnvBindAddress(t *testing.T) { + for _, tt := range envBindAddressCases() { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.set { + t.Setenv(testEnvKey, tt.value) + } else { + require.NoError(t, os.Unsetenv(testEnvKey)) + } + + got, err := config.EnvBindAddressForTest( + testEnvKey, bindAddressDefault, + ) + + if tt.expectError { + require.Error(t, err) + require.ErrorIs(t, err, config.ErrInvalidBindAddress) + assert.Contains(t, err.Error(), testEnvKey) + assert.Contains(t, err.Error(), tt.value) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +// envBindAddressCase is one row of the envBindAddress table. +type envBindAddressCase struct { + name string + set bool + value string + expectError bool + expected string +} + +// envBindAddressCases is the envBindAddress table, kept out of the +// test body so the test itself stays readable. +func envBindAddressCases() []envBindAddressCase { + return append( + envBindAddressAcceptedCases(), + envBindAddressRejectedCases()..., + ) +} + +// envBindAddressAcceptedCases are the values that parse: the three +// spellings of "unset" that take the default, and the literals. +func envBindAddressAcceptedCases() []envBindAddressCase { + return []envBindAddressCase{ + { + name: "unset returns the default", + expected: bindAddressDefault, + }, + { + name: "empty returns the default", + set: true, + value: "", + expected: bindAddressDefault, + }, + { + name: "whitespace returns the default", + set: true, + value: " ", + expected: bindAddressDefault, + }, + { + name: "ipv4 wildcard is parsed", + set: true, + value: bindAddressWildcard, + expected: bindAddressWildcard, + }, + { + name: "ipv4 literal is parsed", + set: true, + value: bindAddressSample, + expected: bindAddressSample, + }, + { + name: "surrounding whitespace is trimmed", + set: true, + value: " " + bindAddressSample + " ", + expected: bindAddressSample, + }, + { + name: "ipv6 wildcard is parsed", + set: true, + value: "::", + expected: "::", + }, + { + name: "ipv6 literal is parsed", + set: true, + value: "2001:db8::5", + expected: "2001:db8::5", + }, + } +} + +// envBindAddressRejectedCases are the values that abort startup. +// Each is something an operator plausibly writes, and none may fall +// back to the default: the default is loopback, so a silent fallback +// would bind somewhere other than what was asked for. +func envBindAddressRejectedCases() []envBindAddressCase { + return []envBindAddressCase{ + { + name: "garbage is rejected", + set: true, + value: "not-an-address", + expectError: true, + }, + { + name: "hostname is rejected", + set: true, + value: "localhost", + expectError: true, + }, + { + name: "unresolvable hostname is rejected", + set: true, + value: "no-such-host.invalid", + expectError: true, + }, + { + name: "host and port is rejected", + set: true, + value: bindAddressDefault + ":8080", + expectError: true, + }, + { + name: "bracketed ipv6 is rejected", + set: true, + value: "[::1]", + expectError: true, + }, + { + name: "CIDR block is rejected", + set: true, + value: "10.0.0.0/8", + expectError: true, + }, + } +} + // buildConfig constructs a Config through fx exactly as the // application does, returning the config and any construction error. func buildConfig(t *testing.T) (*config.Config, error) { @@ -312,13 +482,45 @@ func buildConfig(t *testing.T) (*config.Config, error) { } func TestNewRejectsBadEnvValues(t *testing.T) { - tests := []struct { - name string - key string - value string - expectError bool - check func(t *testing.T, cfg *config.Config) - }{ + for _, tt := range badEnvValueCases() { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + t.Setenv(tt.key, tt.value) + + cfg, err := buildConfig(t) + + if tt.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.key) + assert.Contains(t, err.Error(), tt.value) + + return + } + + require.NoError(t, err) + require.NotNil(t, cfg) + tt.check(t, cfg) + }) + } +} + +// badEnvValueCase is one row of the config.New table: a variable, the +// value it is set to, and either the assertion that startup fails +// naming both, or a check on the Config that resulted. +type badEnvValueCase struct { + name string + key string + value string + expectError bool + check func(t *testing.T, cfg *config.Config) +} + +// badEnvValueCases is the config.New table, kept out of the test body +// so the test itself stays readable. +func badEnvValueCases() []badEnvValueCase { + return []badEnvValueCase{ { name: "valid PORT is used", key: envKeyPort, @@ -361,29 +563,35 @@ func TestNewRejectsBadEnvValues(t *testing.T) { value: "sometimes", expectError: true, }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Cannot use t.Parallel() here because t.Setenv - // is incompatible with parallel subtests. - t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") - t.Setenv(tt.key, tt.value) - - cfg, err := buildConfig(t) - - if tt.expectError { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.key) - assert.Contains(t, err.Error(), tt.value) - - return - } - - require.NoError(t, err) - require.NotNil(t, cfg) - tt.check(t, cfg) - }) + { + name: "valid BIND_ADDRESS is used", + key: envKeyBindAddress, + value: bindAddressWildcard, + check: func(t *testing.T, cfg *config.Config) { + t.Helper() + assert.Equal( + t, bindAddressWildcard, cfg.BindAddress, + ) + }, + }, + { + name: "unparseable BIND_ADDRESS aborts startup", + key: envKeyBindAddress, + value: "not-an-address", + expectError: true, + }, + { + name: "hostname BIND_ADDRESS aborts startup", + key: envKeyBindAddress, + value: "localhost", + expectError: true, + }, + { + name: "BIND_ADDRESS with a port aborts startup", + key: envKeyBindAddress, + value: bindAddressDefault + ":8080", + expectError: true, + }, } } @@ -395,6 +603,7 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) { for _, key := range []string{ envKeyPort, envKeyDebug, envKeyMaintenanceMode, + envKeyBindAddress, } { require.NoError(t, os.Unsetenv(key)) } @@ -406,4 +615,15 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) { assert.Equal(t, 8080, cfg.Port) assert.False(t, cfg.Debug) assert.False(t, cfg.MaintenanceMode) + + // Loopback, not the wildcard: the default must not publish the + // cleartext admin UI and the unauthenticated receiver on every + // interface of a host that configured nothing. The value is read + // from the package rather than repeated, so the README's + // documented default and the compiled-in one are pinned to the + // same constant. + assert.Equal( + t, config.DefaultBindAddressForTest, cfg.BindAddress, + ) + assert.Equal(t, bindAddressDefault, cfg.BindAddress) } diff --git a/internal/config/export_test.go b/internal/config/export_test.go index 710e680..f952bd2 100644 --- a/internal/config/export_test.go +++ b/internal/config/export_test.go @@ -50,3 +50,13 @@ func EnvPositiveIntForTest(key string, defaultValue int) (int, error) { func EnvPortForTest(key string, defaultValue int) (int, error) { return envPort(key, defaultValue) } + +// EnvBindAddressForTest exposes envBindAddress. +func EnvBindAddressForTest(key, defaultValue string) (string, error) { + return envBindAddress(key, defaultValue) +} + +// DefaultBindAddressForTest exposes the compiled-in BIND_ADDRESS +// default, so a test pins the documented value rather than repeating +// a literal that could drift from it. +const DefaultBindAddressForTest = defaultBindAddress diff --git a/internal/server/bind_address_test.go b/internal/server/bind_address_test.go new file mode 100644 index 0000000..9329180 --- /dev/null +++ b/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) +} diff --git a/internal/server/early_shutdown_test.go b/internal/server/early_shutdown_test.go new file mode 100644 index 0000000..8895394 --- /dev/null +++ b/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)) +} diff --git a/internal/server/export_test.go b/internal/server/export_test.go index d5b05be..701c110 100644 --- a/internal/server/export_test.go +++ b/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) diff --git a/internal/server/http.go b/internal/server/http.go index 5413dd9..040197f 100644 --- a/internal/server/http.go +++ b/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) { diff --git a/internal/server/listen_failure_test.go b/internal/server/listen_failure_test.go index 6b71185..3508626 100644 --- a/internal/server/listen_failure_test.go +++ b/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), diff --git a/internal/server/routes.go b/internal/server/routes.go index ed4dc82..05872f8 100644 --- a/internal/server/routes.go +++ b/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, }) diff --git a/internal/server/server.go b/internal/server/server.go index 721df5b..71a46ec 100644 --- a/internal/server/server.go +++ b/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) } }