From b9f7db69015b2550f18ce6bbc2c5f7498e27572f Mon Sep 17 00:00:00 2001 From: clawbot Date: Mon, 24 Aug 2026 04:25:29 +0200 Subject: [PATCH] Fail loudly on an unparseable SENTRY_DSN and a malformed .env (closes #283) --- README.md | 28 ++++- cmd/webhooker/dotenv_test.go | 107 +++++++++++++++++ cmd/webhooker/main.go | 23 +++- cmd/webhooker/main_test.go | 2 +- internal/config/config.go | 116 +++++++++++++++++- internal/config/dotenv_test.go | 158 +++++++++++++++++++++++++ internal/config/env_test.go | 105 ++++++++++++---- internal/config/export_test.go | 12 ++ internal/config/sentry_test.go | 141 ++++++++++++++++++++++ internal/server/http.go | 2 +- internal/server/listen_failure_test.go | 2 +- internal/server/sentry_failure_test.go | 99 ++++++++++++++++ internal/server/server.go | 83 +++++++++---- 13 files changed, 815 insertions(+), 63 deletions(-) create mode 100644 cmd/webhooker/dotenv_test.go create mode 100644 internal/config/dotenv_test.go create mode 100644 internal/config/sentry_test.go create mode 100644 internal/server/sentry_failure_test.go diff --git a/README.md b/README.md index 4690f62..d1646da 100644 --- a/README.md +++ b/README.md @@ -71,8 +71,18 @@ make clean # Remove bin/ ### Configuration All configuration is via environment variables. For local development, -you can place variables in a `.env` file in the project root (loaded -automatically via `godotenv/autoload`). +you can place variables in a `.env` file in the process working +directory, read once at startup before anything else looks at the +environment. + +The file is optional and having none is the normal case for a +deployment. A file that is there but cannot be parsed aborts startup +with a message naming it, because a single malformed line makes none +of the file apply: every variable in it silently reverts to its +default, which is exactly the failure [Invalid values abort +startup](#invalid-values-abort-startup) exists to prevent, for all of +them at once. A variable already present in the real environment wins +over the file's value for the same name. The environment is selected by setting `WEBHOOKER_ENVIRONMENT` to `dev` or `prod` (default: `dev`). The setting controls exactly one behavior: @@ -125,7 +135,7 @@ TTY detection, and security headers are always applied. | `MAINTENANCE_MODE` | Report `maintenanceMode: true` in the healthcheck JSON. It does not change how any request is served — no maintenance page exists | `false` | | `METRICS_USERNAME` | Basic auth username for `/metrics`. Must be set together with `METRICS_PASSWORD`; one without the other fails startup | `""` | | `METRICS_PASSWORD` | Basic auth password for `/metrics`. Must be set together with `METRICS_USERNAME`; one without the other fails startup | `""` | -| `SENTRY_DSN` | Sentry error reporting DSN | `""` | +| `SENTRY_DSN` | Sentry error reporting DSN. Unset leaves error reporting off; a value the Sentry SDK cannot parse fails startup rather than serving with reporting silently off | `""` | | `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` | | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` | @@ -464,10 +474,20 @@ 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. +resolved, split, or narrowed to something they do not say — and +`SENTRY_DSN` must parse as a Sentry DSN. `SESSION_IDLE_TIMEOUT` is the exception: a non-positive value there means idle expiry is disabled, not invalid. +`SENTRY_DSN` is checked with the Sentry SDK's own parser, the same call +the SDK makes on the DSN it is later handed, so what configuration +accepts is exactly what will initialise. A typo in it is the one +configuration mistake nothing downstream can ever notice — the variable +is still set, so every later signal reports error reporting as on while +no report is being sent — which is why it aborts rather than starting +with reporting off. Leaving it unset is not a mistake and not affected: +error reporting is simply off and startup is normal. + Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`, `true`, `True`, `0`, `f`, `F`, `FALSE`, `false`, `False` — and nothing diff --git a/cmd/webhooker/dotenv_test.go b/cmd/webhooker/dotenv_test.go new file mode 100644 index 0000000..7a579c8 --- /dev/null +++ b/cmd/webhooker/dotenv_test.go @@ -0,0 +1,107 @@ +package main + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/config" +) + +// dotEnvKey is a throwaway variable name these tests write and read, +// so they cannot disturb real configuration. +const dotEnvKey = "WEBHOOKER_TEST_DISPATCH_VALUE" + +// writeDotEnvInWorkingDir puts contents in a .env file in a fresh +// temporary directory and moves the process there. +// +// The callers are deliberately not parallel and must stay that way: +// t.Chdir moves the whole process. Go releases parallel tests only +// after every sequential test in the package has finished, so nothing +// else runs while these do. +func writeDotEnvInWorkingDir(t *testing.T, contents string) { + t.Helper() + + dir := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(dir, config.DotEnvPath), + []byte(contents), 0o600, + )) + t.Chdir(dir) +} + +// TestDispatch_MalformedDotEnvRefuses pins the second half of the +// defect. godotenv applies nothing at all when a file will not parse, +// so one mistyped line used to revert every variable in it to its +// default and start the server anyway, with no log line naming the +// file. The refusal has to arrive before any subcommand runs, which +// is why `help` — the one subcommand that touches nothing — is still +// refused here. +// +//nolint:paralleltest // t.Chdir moves the whole process. +func TestDispatch_MalformedDotEnvRefuses(t *testing.T) { + writeDotEnvInWorkingDir(t, "PORT 19615\n") + + var stdout, stderr bytes.Buffer + + code := dispatch( + []string{helpCommand}, strings.NewReader(""), &stdout, &stderr, + ) + + require.Equal(t, 1, code, "a broken .env must exit non-zero") + assert.Contains( + t, stderr.String(), config.DotEnvPath, + "the refusal must name the file", + ) + assert.Empty( + t, stdout.String(), + "the subcommand must not have run", + ) +} + +// TestDispatch_LoadsDotEnvBeforeSubcommands pins the ordering the +// godotenv/autoload import used to provide for free. It ran in an +// init(), so .env was in the environment before anything read it — +// including config.DataDir, which both the DATA_DIR lock and resetpw +// call outside the fx graph. Loading any later would let a .env that +// sets DATA_DIR lock one directory while the config opened databases +// in another. +func TestDispatch_LoadsDotEnvBeforeSubcommands(t *testing.T) { + t.Setenv(dotEnvKey, "placeholder") + require.NoError(t, os.Unsetenv(dotEnvKey)) + + writeDotEnvInWorkingDir(t, dotEnvKey+"=from-dot-env\n") + + var stdout, stderr bytes.Buffer + + code := dispatch( + []string{helpCommand}, strings.NewReader(""), &stdout, &stderr, + ) + + require.Equal(t, 0, code) + assert.Equal( + t, "from-dot-env", os.Getenv(dotEnvKey), + "the file must be applied before the subcommand runs", + ) +} + +// TestDispatch_MissingDotEnvIsFine pins the case most deployments are +// in: no .env at all, which must stay a normal start. +// +//nolint:paralleltest // t.Chdir moves the whole process. +func TestDispatch_MissingDotEnvIsFine(t *testing.T) { + t.Chdir(t.TempDir()) + + var stdout, stderr bytes.Buffer + + code := dispatch( + []string{helpCommand}, strings.NewReader(""), &stdout, &stderr, + ) + + require.Equal(t, 0, code) + assert.Empty(t, stderr.String()) +} diff --git a/cmd/webhooker/main.go b/cmd/webhooker/main.go index 7bea37a..0da8fa0 100644 --- a/cmd/webhooker/main.go +++ b/cmd/webhooker/main.go @@ -54,6 +54,11 @@ const stopTimeout = 5 * time.Second // caller can tell "called wrong" from "declined". const exitUsage = 2 +// helpCommand is the subcommand that prints usage. The flag spellings +// beside it in the switch are aliases; this is the name the usage text +// documents and the one tests invoke. +const helpCommand = "help" + // Build-time variables set via -ldflags. // //nolint:gochecknoglobals // Build-time variables injected by the linker. @@ -75,11 +80,27 @@ func main() { // every existing deployment invoke; that path is unchanged, including // where the DATA_DIR lock is taken relative to building the fx graph // and how fx propagates a non-zero exit itself. +// +// The optional .env file is read here, before any subcommand and so +// before anything reads the environment — config.DataDir, which both +// the DATA_DIR lock and resetpw call outside the fx graph, above all. +// It used to be read from an init() in internal/config, which put it +// earlier still but threw the error away: a single malformed line +// applied none of the file and said nothing about it. A file that is +// not there stays fine, since .env is optional and most deployments +// do not have one. func dispatch( args []string, stdin io.Reader, stdout, stderr io.Writer, ) int { + err := config.LoadDotEnv() + if err != nil { + _, _ = fmt.Fprintf(stderr, "%s: %v\n", appname, err) + + return 1 + } + if len(args) == 0 { return run(stderr) } @@ -87,7 +108,7 @@ func dispatch( switch args[0] { case resetpw.Name: return resetpw.Run(args[1:], stdin, stdout, stderr) - case "help", "-h", "-help", "--help": + case helpCommand, "-h", "-help", "--help": usage(stdout) return 0 diff --git a/cmd/webhooker/main_test.go b/cmd/webhooker/main_test.go index 878ee24..98ca68e 100644 --- a/cmd/webhooker/main_test.go +++ b/cmd/webhooker/main_test.go @@ -121,7 +121,7 @@ func TestDispatch_Help(t *testing.T) { var stdout, stderr bytes.Buffer code := dispatch( - []string{"help"}, strings.NewReader(""), &stdout, &stderr, + []string{helpCommand}, strings.NewReader(""), &stdout, &stderr, ) require.Equal(t, 0, code) diff --git a/internal/config/config.go b/internal/config/config.go index b732848..6a7a628 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ package config import ( "errors" "fmt" + "io/fs" "log/slog" "net/netip" "os" @@ -11,13 +12,11 @@ import ( "strings" "time" + "github.com/getsentry/sentry-go" + "github.com/joho/godotenv" "go.uber.org/fx" "sneak.berlin/go/webhooker/internal/globals" "sneak.berlin/go/webhooker/internal/logger" - - // Populates the environment from a ./.env file automatically for - // development configuration. Kept in one place only (here). - _ "github.com/joho/godotenv/autoload" ) const ( @@ -84,6 +83,12 @@ const ( // IPv6 prefix spends on the ::ffff:0:0/96 wrapper, so a /104 // covers the same addresses as an IPv4 /8. mappedV4Offset = 96 + + // DotEnvPath is the optional file of KEY=value lines read into the + // environment at startup, relative to the process working + // directory. Exported so that documentation and tests name the + // same path the loader opens. + DotEnvPath = ".env" ) // ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT @@ -107,6 +112,15 @@ var ErrInvalidCIDR = errors.New("invalid CIDR") // something that is not an IP address literal. var ErrInvalidBindAddress = errors.New("invalid bind address") +// ErrInvalidSentryDSN is returned when SENTRY_DSN is set to something +// the Sentry SDK cannot parse as a DSN. +var ErrInvalidSentryDSN = errors.New("invalid Sentry DSN") + +// ErrDotEnvUnreadable is returned when the optional .env file exists +// but cannot be read or parsed. A file that is not there is not an +// error; a file that is there and broken is. +var ErrDotEnvUnreadable = errors.New("unreadable .env file") + // 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 @@ -212,12 +226,62 @@ func (c *Config) MetricsAuthEnabled() bool { return c.MetricsUsername != "" && c.MetricsPassword != "" } +// SentryEnabled reports whether error reporting is shipped to Sentry. +// It is the only answer to that question in the codebase: the SDK +// initialisation, the sentryhttp middleware registration and the +// startup log's sentryEnabled field all read this one method, so the +// log cannot report reporting as on while nothing is sending. +// +// A non-empty DSN is enough because loadFromEnv already parsed it with +// the SDK's own parser and refused to build a Config around one the +// SDK would reject, and because initialising the SDK with a DSN that +// parsed and failed anyway aborts the process rather than leaving this +// true and the client absent. +func (c *Config) SentryEnabled() bool { + return c.SentryDSN != "" +} + // envString returns the value of the named environment variable, // or an empty string if not set. func envString(key string) string { return os.Getenv(key) } +// LoadDotEnv reads DotEnvPath into the environment when that file is +// present, and reports a file that is present but broken. +// +// It has to run before anything reads the environment, so that every +// reader agrees on what the environment holds — the DATA_DIR lock +// taken before the fx graph exists as much as loadFromEnv itself. A +// variable already set in the real environment wins: godotenv never +// overwrites one. +// +// A missing file is not an error. It is a development convenience and +// most deployments set the environment directly. +// +// Any other failure is. godotenv parses the whole file before setting +// anything, so a single malformed line applies none of it: every +// variable in the file silently reverts to its default, which defeats +// the fail-loud guarantee for all of them at once. +func LoadDotEnv() error { + return loadDotEnvFile(DotEnvPath) +} + +// loadDotEnvFile is LoadDotEnv over a named file, so tests can point +// at a temporary one instead of the process working directory. +func loadDotEnvFile(path string) error { + err := godotenv.Load(path) + if err == nil || errors.Is(err, fs.ErrNotExist) { + return nil + } + + return fmt.Errorf( + "%w: %s: %w; nothing in it was applied, so fix the file or "+ + "remove it", + ErrDotEnvUnreadable, path, err, + ) +} + // DataDir resolves DATA_DIR, applying DefaultDataDir when it is unset // or empty. It is exported so that entry points which must act on the // data directory before the fx graph exists — taking the exclusive @@ -462,6 +526,41 @@ func envBindAddress(key, defaultValue string) (string, error) { return addr.String(), nil } +// envSentryDSN returns the value of the named environment variable +// checked as a Sentry DSN. An unset (or empty, or whitespace-only) +// value yields "", which means error reporting stays off — the common +// case, and a normal start. +// +// A set value is parsed with sentry.NewDsn, which is the call +// sentry.Init makes on the DSN it is handed, so what passes here is +// exactly what the SDK will accept later and the two cannot disagree. +// Reproducing the check by hand instead would cost this package its +// dependency on the SDK — already a module dependency, already linked +// into the binary — in exchange for a second definition of "valid DSN" +// free to drift from the one that decides. +// +// A set value that does not parse is a hard error naming the key, so +// startup fails loudly. Losing error reporting is the failure this +// variable exists to prevent, and a typo in a DSN is silent forever: +// nothing later in the process can notice that reports are going +// nowhere. The bad value is quoted because it is a URL to a public +// endpoint carrying a public key, not a secret. +func envSentryDSN(key string) (string, error) { + v := strings.TrimSpace(os.Getenv(key)) + if v == "" { + return "", nil + } + + _, err := sentry.NewDsn(v) + if err != nil { + return "", fmt.Errorf( + "%w: %s: %q: %w", ErrInvalidSentryDSN, key, v, err, + ) + } + + return v, 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. @@ -594,6 +693,11 @@ func loadFromEnv() (*Config, error) { return nil, err } + sentryDSN, err := envSentryDSN("SENTRY_DSN") + if err != nil { + return nil, err + } + return &Config{ DataDir: DataDir(), Debug: debug, @@ -603,7 +707,7 @@ func loadFromEnv() (*Config, error) { MetricsPassword: metricsPassword, Port: port, BindAddress: bindAddress, - SentryDSN: envString("SENTRY_DSN"), + SentryDSN: sentryDSN, RetentionSweepInterval: retentionSweepInterval, SessionIdleTimeout: sessionIdleTimeout, ReceiverRateLimit: receiverRateLimit, @@ -738,7 +842,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { "receiverRateLimit", s.ReceiverRateLimit, "trustedProxies", len(s.TrustedProxies), "allowedEgressCIDRs", len(s.AllowedEgressCIDRs), - "hasSentryDSN", s.SentryDSN != "", + "sentryEnabled", s.SentryEnabled(), "hasMetricsAuth", s.MetricsAuthEnabled(), ) diff --git a/internal/config/dotenv_test.go b/internal/config/dotenv_test.go new file mode 100644 index 0000000..952bb32 --- /dev/null +++ b/internal/config/dotenv_test.go @@ -0,0 +1,158 @@ +package config_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/config" +) + +// dotEnvKey is a throwaway variable name the .env tests write and +// read, so they cannot disturb real configuration. +const dotEnvKey = "WEBHOOKER_TEST_DOTENV_VALUE" + +// malformedDotEnv is a file godotenv cannot parse. The first line is +// the realistic typo — a space where the `=` belongs — and the rest +// make sure nothing downstream treats the file as salvageable line by +// line. +const malformedDotEnv = "PORT 19615\n" + + "this is not = valid ! syntax\n" + + "\"unclosed\n" + +// unsetDotEnvKey makes dotEnvKey genuinely absent for the duration of +// the test and restores it afterwards. t.Setenv registers the restore; +// the Unsetenv that follows is what the test actually needs, because a +// variable set to the empty string is still present in os.Environ and +// godotenv would refuse to overwrite it. +func unsetDotEnvKey(t *testing.T) { + t.Helper() + t.Setenv(dotEnvKey, "placeholder") + require.NoError(t, os.Unsetenv(dotEnvKey)) +} + +// writeDotEnv writes contents to a .env file in a fresh temporary +// directory and returns its path. +func writeDotEnv(t *testing.T, contents string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), config.DotEnvPath) + require.NoError(t, os.WriteFile(path, []byte(contents), 0o600)) + + return path +} + +// TestLoadDotEnv_MissingFileIsFine pins the case most deployments are +// in. The file is optional: it is a development convenience, and a +// deployment that configures the environment directly must start +// normally rather than be refused for a file it was never meant to +// have. +// +//nolint:paralleltest // unsetDotEnvKey uses t.Setenv. +func TestLoadDotEnv_MissingFileIsFine(t *testing.T) { + unsetDotEnvKey(t) + + absent := filepath.Join(t.TempDir(), config.DotEnvPath) + require.NoError(t, config.LoadDotEnvFileForTest(absent)) + + _, present := os.LookupEnv(dotEnvKey) + assert.False(t, present, "nothing may be set from an absent file") +} + +// TestLoadDotEnv_AppliesValues pins that a well-formed file still +// reaches the environment, which is the whole reason the file is read +// at all. +// +//nolint:paralleltest // unsetDotEnvKey uses t.Setenv. +func TestLoadDotEnv_AppliesValues(t *testing.T) { + unsetDotEnvKey(t) + + path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n") + + require.NoError(t, config.LoadDotEnvFileForTest(path)) + assert.Equal(t, "from-dot-env", os.Getenv(dotEnvKey)) +} + +// TestLoadDotEnv_RealEnvironmentWins pins that the file cannot +// override a variable the process was actually started with. A +// deployment that sets DATA_DIR in its unit file must not have it +// silently replaced by a stale .env left in the working directory. +func TestLoadDotEnv_RealEnvironmentWins(t *testing.T) { + t.Setenv(dotEnvKey, "from-environment") + + path := writeDotEnv(t, dotEnvKey+"=from-dot-env\n") + + require.NoError(t, config.LoadDotEnvFileForTest(path)) + assert.Equal(t, "from-environment", os.Getenv(dotEnvKey)) +} + +// TestLoadDotEnv_MalformedFileAborts is the defect this fixes. One bad +// line makes godotenv apply none of the file, so every variable in it +// reverts to its default; the process used to start that way with no +// log line naming the file at all. +// +//nolint:paralleltest // unsetDotEnvKey uses t.Setenv. +func TestLoadDotEnv_MalformedFileAborts(t *testing.T) { + unsetDotEnvKey(t) + + path := writeDotEnv( + t, malformedDotEnv+dotEnvKey+"=from-dot-env\n", + ) + + err := config.LoadDotEnvFileForTest(path) + + require.Error(t, err) + require.ErrorIs(t, err, config.ErrDotEnvUnreadable) + assert.Contains( + t, err.Error(), config.DotEnvPath, + "the failure must name the file it could not read", + ) + + _, present := os.LookupEnv(dotEnvKey) + assert.False( + t, present, + "a rejected file must apply nothing, not part of itself", + ) +} + +// TestLoadDotEnv_UnreadableFileAborts pins that only absence is +// tolerated. A .env that exists but cannot be read is a file the +// operator meant to be applied, so it fails like a malformed one +// rather than being treated as though it were not there. +func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) { + t.Parallel() + + // A directory in the file's place: open succeeds and the read + // fails, which no umask or root-ness can turn back into success + // the way a chmod could. + path := filepath.Join(t.TempDir(), config.DotEnvPath) + require.NoError(t, os.Mkdir(path, 0o750)) + + err := config.LoadDotEnvFileForTest(path) + + require.Error(t, err) + require.ErrorIs(t, err, config.ErrDotEnvUnreadable) +} + +// TestLoadDotEnv_ReadsTheWorkingDirectory pins the path LoadDotEnv +// itself opens, which the tests above bypass. It is relative to the +// process working directory, as it was under godotenv/autoload and as +// the README documents. +// +//nolint:paralleltest // t.Chdir moves the whole process. +func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) { + unsetDotEnvKey(t) + + dir := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(dir, config.DotEnvPath), + []byte(dotEnvKey+"=from-working-directory\n"), + 0o600, + )) + t.Chdir(dir) + + require.NoError(t, config.LoadDotEnv()) + assert.Equal(t, "from-working-directory", os.Getenv(dotEnvKey)) +} diff --git a/internal/config/env_test.go b/internal/config/env_test.go index bc67167..29985c6 100644 --- a/internal/config/env_test.go +++ b/internal/config/env_test.go @@ -518,8 +518,20 @@ type badEnvValueCase struct { } // badEnvValueCases is the config.New table, kept out of the test body -// so the test itself stays readable. +// so the test itself stays readable. It is assembled from per-variable +// groups because one literal covering every variable outgrew the +// function-length budget. func badEnvValueCases() []badEnvValueCase { + cases := listenerEnvValueCases() + cases = append(cases, flagEnvValueCases()...) + cases = append(cases, sentryEnvValueCases()...) + + return cases +} + +// listenerEnvValueCases covers the two variables that describe the +// HTTP listener. +func listenerEnvValueCases() []badEnvValueCase { return []badEnvValueCase{ { name: "valid PORT is used", @@ -542,27 +554,6 @@ func badEnvValueCases() []badEnvValueCase { value: "70000", expectError: true, }, - { - name: "valid DEBUG is used", - key: envKeyDebug, - value: "true", - check: func(t *testing.T, cfg *config.Config) { - t.Helper() - assert.True(t, cfg.Debug) - }, - }, - { - name: "unparseable DEBUG aborts startup", - key: envKeyDebug, - value: "ture", - expectError: true, - }, - { - name: "unparseable MAINTENANCE_MODE aborts startup", - key: envKeyMaintenanceMode, - value: "sometimes", - expectError: true, - }, { name: "valid BIND_ADDRESS is used", key: envKeyBindAddress, @@ -595,6 +586,69 @@ func badEnvValueCases() []badEnvValueCase { } } +// flagEnvValueCases covers the boolean variables. +func flagEnvValueCases() []badEnvValueCase { + return []badEnvValueCase{ + { + name: "valid DEBUG is used", + key: envKeyDebug, + value: "true", + check: func(t *testing.T, cfg *config.Config) { + t.Helper() + assert.True(t, cfg.Debug) + }, + }, + { + name: "unparseable DEBUG aborts startup", + key: envKeyDebug, + value: "ture", + expectError: true, + }, + { + name: "unparseable MAINTENANCE_MODE aborts startup", + key: envKeyMaintenanceMode, + value: "sometimes", + expectError: true, + }, + } +} + +// sentryEnvValueCases covers SENTRY_DSN. The three rejected values are +// the ones measured on the defect: each initialised the SDK with an +// error and left the process serving with error reporting off. +func sentryEnvValueCases() []badEnvValueCase { + return []badEnvValueCase{ + { + name: "valid SENTRY_DSN is used", + key: envKeySentryDSN, + value: validSentryDSN, + check: func(t *testing.T, cfg *config.Config) { + t.Helper() + assert.Equal(t, validSentryDSN, cfg.SentryDSN) + assert.True(t, cfg.SentryEnabled()) + }, + }, + { + name: "unparseable SENTRY_DSN aborts startup", + key: envKeySentryDSN, + value: "not-a-dsn", + expectError: true, + }, + { + name: "SENTRY_DSN that is not a URL aborts startup", + key: envKeySentryDSN, + value: "%%%", + expectError: true, + }, + { + name: "keyless SENTRY_DSN aborts startup", + key: envKeySentryDSN, + value: "https://example.invalid/1", + expectError: true, + }, + } +} + // TestNewUsesDefaultsWhenUnset proves the fail-loud behaviour did not // break the legitimate unset case: absent variables still get their // documented defaults. @@ -603,7 +657,7 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) { for _, key := range []string{ envKeyPort, envKeyDebug, envKeyMaintenanceMode, - envKeyBindAddress, + envKeyBindAddress, envKeySentryDSN, } { require.NoError(t, os.Unsetenv(key)) } @@ -626,4 +680,9 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) { t, config.DefaultBindAddressForTest, cfg.BindAddress, ) assert.Equal(t, bindAddressDefault, cfg.BindAddress) + + // An absent SENTRY_DSN is the common case and must stay a normal + // start with error reporting off, not a refusal. + assert.Empty(t, cfg.SentryDSN) + assert.False(t, cfg.SentryEnabled()) } diff --git a/internal/config/export_test.go b/internal/config/export_test.go index f952bd2..d4d3be8 100644 --- a/internal/config/export_test.go +++ b/internal/config/export_test.go @@ -51,6 +51,18 @@ func EnvPortForTest(key string, defaultValue int) (int, error) { return envPort(key, defaultValue) } +// EnvSentryDSNForTest exposes envSentryDSN. +func EnvSentryDSNForTest(key string) (string, error) { + return envSentryDSN(key) +} + +// LoadDotEnvFileForTest exposes the loader LoadDotEnv runs, over a +// caller-named file rather than the process working directory, so +// each .env state can be covered without moving the test process. +func LoadDotEnvFileForTest(path string) error { + return loadDotEnvFile(path) +} + // EnvBindAddressForTest exposes envBindAddress. func EnvBindAddressForTest(key, defaultValue string) (string, error) { return envBindAddress(key, defaultValue) diff --git a/internal/config/sentry_test.go b/internal/config/sentry_test.go new file mode 100644 index 0000000..0b475bf --- /dev/null +++ b/internal/config/sentry_test.go @@ -0,0 +1,141 @@ +package config_test + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/config" +) + +// envKeySentryDSN is the variable envSentryDSN reads in production. +const envKeySentryDSN = "SENTRY_DSN" + +// validSentryDSN is a syntactically complete DSN. The host is under +// .invalid (RFC 2606), so nothing a test builds around it can reach a +// real Sentry installation. +const validSentryDSN = "https://abc123@sentry.invalid/42" + +// envSentryDSNCase is one row of the envSentryDSN table. +type envSentryDSNCase struct { + name string + set bool + value string + expectError bool + expected string +} + +// envSentryDSNCases is the envSentryDSN table. The three invalid +// values are the ones measured on the defect: each initialised the SDK +// with an error and left the process serving with reporting off. +func envSentryDSNCases() []envSentryDSNCase { + return []envSentryDSNCase{ + { + name: "unset means reporting off", + expected: "", + }, + { + name: "empty means reporting off", + set: true, + value: "", + expected: "", + }, + { + name: "whitespace means reporting off", + set: true, + value: " ", + expected: "", + }, + { + name: "a valid DSN is kept", + set: true, + value: validSentryDSN, + expected: validSentryDSN, + }, + { + name: "surrounding whitespace is trimmed", + set: true, + value: " " + validSentryDSN + "\t", + expected: validSentryDSN, + }, + { + name: "a value that is not a URL is rejected", + set: true, + value: "not-a-dsn", + expectError: true, + }, + { + name: "an unparseable URL is rejected", + set: true, + value: "%%%", + expectError: true, + }, + { + name: "a DSN without a public key is rejected", + set: true, + value: "https://example.invalid/1", + expectError: true, + }, + { + name: "a DSN without a project id is rejected", + set: true, + value: "https://abc123@sentry.invalid/", + expectError: true, + }, + { + name: "a non-HTTP scheme is rejected", + set: true, + value: "ftp://abc123@sentry.invalid/42", + expectError: true, + }, + } +} + +// TestEnvSentryDSN covers the helper directly. What it pins beyond the +// value is the failure shape: a set-but-unparseable DSN names the +// variable and the value, exactly as the other fail-loud helpers do, +// so an operator reads the fix off the message. +func TestEnvSentryDSN(t *testing.T) { + for _, tt := range envSentryDSNCases() { + 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(envKeySentryDSN, tt.value) + } else { + require.NoError(t, os.Unsetenv(envKeySentryDSN)) + } + + got, err := config.EnvSentryDSNForTest(envKeySentryDSN) + + if tt.expectError { + require.Error(t, err) + require.ErrorIs(t, err, config.ErrInvalidSentryDSN) + assert.Contains(t, err.Error(), envKeySentryDSN) + assert.Contains(t, err.Error(), tt.value) + assert.Empty(t, got) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, got) + }) + } +} + +// TestSentryEnabled_TracksTheDSN pins that the one method answering +// "is anything being reported" agrees with the DSN in every state. The +// startup log, the SDK initialisation and the sentryhttp middleware +// all read it, so a log field cannot report reporting as on while +// nothing is sending. +func TestSentryEnabled_TracksTheDSN(t *testing.T) { + t.Parallel() + + assert.False(t, (&config.Config{}).SentryEnabled()) + assert.True( + t, + (&config.Config{SentryDSN: validSentryDSN}).SentryEnabled(), + ) +} diff --git a/internal/server/http.go b/internal/server/http.go index 040197f..1a01999 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -70,7 +70,7 @@ func (s *Server) serveUntilShutdown() { err := s.httpServer.ListenAndServe() if err != nil && !errors.Is(err, http.ErrServerClosed) { s.log.Error("listen error", "error", err) - s.shutdownOnListenFailure() + s.shutdownWithFailure() } } diff --git a/internal/server/listen_failure_test.go b/internal/server/listen_failure_test.go index 3508626..2a2fc6c 100644 --- a/internal/server/listen_failure_test.go +++ b/internal/server/listen_failure_test.go @@ -93,7 +93,7 @@ func requireListenFailureExit(t *testing.T, env *testEnv) { select { case sig := <-app.Wait(): require.Equal( - t, server.ListenFailureExitCode, sig.ExitCode, + t, server.StartupFailureExitCode, sig.ExitCode, "listen failure must exit non-zero", ) case <-time.After(listenFailureDeadline): diff --git a/internal/server/sentry_failure_test.go b/internal/server/sentry_failure_test.go new file mode 100644 index 0000000..8ce41bb --- /dev/null +++ b/internal/server/sentry_failure_test.go @@ -0,0 +1,99 @@ +package server_test + +import ( + "context" + "net" + "strconv" + "testing" + "time" + + "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" +) + +// TestSentryInitFailure_ShutsDownTheApp pins that error reporting +// which is configured and cannot be started ends the application +// instead of serving without it. +// +// The measured defect logged `sentry init failure` and kept running, +// so the deployment served traffic with reporting off while every +// other signal — SENTRY_DSN still set, the startup summary's own +// field — said it was on. Nothing later in the process can notice +// that reports are going nowhere, which is why this exits rather than +// degrades. +// +// The DSN is placed on a hand-built Config, which is the only way to +// reach this branch at all: loadFromEnv now parses SENTRY_DSN with +// sentry.NewDsn, the same call sentry.Init makes, so a DSN that +// survives configuration cannot fail initialisation in the SDK +// version this pins. The branch stays because that is a property of +// the SDK's current implementation rather than of its contract. +func TestSentryInitFailure_ShutsDownTheApp(t *testing.T) { + t.Parallel() + + port := freePort(t) + + env := newTestEnvWithConfig(t, &config.Config{ + DataDir: t.TempDir(), + Environment: config.EnvironmentDev, + BindAddress: loopbackV4, + Port: port, + SentryDSN: "not-a-dsn", + }) + + 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)) + + select { + case sig := <-app.Wait(): + require.Equal( + t, server.StartupFailureExitCode, sig.ExitCode, + "a sentry failure must exit non-zero", + ) + case <-time.After(listenFailureDeadline): + t.Fatal("a sentry failure left the app running") + } + + // The stop sequence still has to complete: the failure must reach + // shutdown through fx rather than around it. + stopCtx, cancelStop := context.WithTimeout( + context.Background(), lifecycleTimeout, + ) + defer cancelStop() + + require.NoError(t, app.Stop(stopCtx)) + + // And it must give up before it listens. A process that bound the + // port and then exited would have accepted requests it could not + // report on, which is the state under test in miniature. + requireBindable(t, port) +} + +// requireBindable asserts that the port is free, which it is only if +// the server under test never claimed it. +func requireBindable(t *testing.T, port int) { + t.Helper() + + var listenCfg net.ListenConfig + + listener, err := listenCfg.Listen( + t.Context(), "tcp", + net.JoinHostPort(loopbackV4, strconv.Itoa(port)), + ) + require.NoError(t, err, "the server bound a port it then gave up") + require.NoError(t, listener.Close()) +} diff --git a/internal/server/server.go b/internal/server/server.go index 71a46ec..86dee1c 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -51,12 +51,13 @@ const ( minSentryFlush = 250 * time.Millisecond ) -// ListenFailureExitCode is the status the process exits with when the -// HTTP listener cannot be established, or dies for a reason other -// than a requested shutdown. It must stay non-zero: systemd -// `Restart=on-failure` and Docker's restart policies key off it, and a -// zero exit would read as a deliberate stop. -const ListenFailureExitCode = 1 +// StartupFailureExitCode is the status the process exits with when +// the serving goroutine gives up: the HTTP listener cannot be +// established or dies for a reason other than a requested shutdown, or +// error reporting is configured and cannot be started. It must stay +// non-zero: systemd `Restart=on-failure` and Docker's restart policies +// key off it, and a zero exit would read as a deliberate stop. +const StartupFailureExitCode = 1 // SentryFlushBudget reports how long the Sentry flush may run when // remaining is the time left on the fx stop context after the HTTP @@ -135,11 +136,25 @@ func New(lc fx.Lifecycle, params ServerParams) (*Server, error) { } // Run configures Sentry and starts serving HTTP requests. +// +// A Sentry failure ends the application instead of listening. It runs +// before the listener rather than after it so that the process never +// binds a port it is about to give up. func (s *Server) Run() { s.configure() // logging before sentry, because sentry logs - s.enableSentry() + err := s.enableSentry() + if err != nil { + s.log.Error( + "SENTRY_DSN is set but error reporting could not be "+ + "started; refusing to serve with it off", + "error", err, + ) + s.shutdownWithFailure() + + return + } s.serve() } @@ -150,11 +165,23 @@ func (s *Server) MaintenanceMode() bool { return s.params.Config.MaintenanceMode } -func (s *Server) enableSentry() { +// enableSentry initialises the Sentry SDK when error reporting is +// configured, and reports the failure when it is configured and cannot +// be initialised. A DSN that is not set is not a failure: reporting +// stays off and the server starts normally. +// +// There is no fallback to running with reporting off. An operator who +// set SENTRY_DSN asked for failures to be visible, and serving traffic +// with reporting quietly off is the one state nothing can ever tell +// them about — the DSN is still set, so every later signal says it is +// on. Config already refused a DSN the SDK cannot parse, which is what +// a typo produces, so reaching this branch means the SDK refused +// something that parsed: not a condition to guess at either. +func (s *Server) enableSentry() error { s.sentryEnabled.Store(false) - if s.params.Config.SentryDSN == "" { - return + if !s.params.Config.SentryEnabled() { + return nil } err := sentry.Init(sentryClientOptions( @@ -166,19 +193,19 @@ func (s *Server) enableSentry() { ), )) if err != nil { - s.log.Error("sentry init failure", "error", err) - // Don't use fatal since we still want the service to run - return + return fmt.Errorf("initialising sentry: %w", err) } s.log.Info("sentry error reporting activated") s.sentryEnabled.Store(true) + + return nil } // serve installs the signal watcher, starts the listener and blocks // until the server's context is cancelled. The process exit status is // fx's to decide — from a signal, or from the code -// shutdownOnListenFailure hands the Shutdowner — so this reports +// shutdownWithFailure hands the Shutdowner — so this reports // nothing back to its caller. func (s *Server) serve() { ctx, cancelFunc := context.WithCancel(context.Background()) @@ -208,20 +235,24 @@ func (s *Server) serve() { // Do not call cleanShutdown() here to avoid double invocation. } -// shutdownOnListenFailure ends the application after the HTTP -// listener failed. The fx OnStart hook returns as soon as the serving -// goroutine is spawned, so nothing downstream of it ever learns that -// the listen failed: fx reports RUNNING and the process sits alive -// with nothing bound, which is invisible to systemd and Docker -// restart policies. Asking the Shutdowner to stop the app with a -// non-zero code is what turns that into a visible failure. +// shutdownWithFailure ends the application non-zero from the serving +// goroutine. It is how anything on that goroutine fails fatally: the +// fx OnStart hook returns as soon as the goroutine is spawned, so +// nothing downstream of it ever learns that the goroutine gave up. fx +// reports RUNNING and the process sits alive having done neither what +// it was asked nor anything visible instead, which systemd and +// Docker restart policies cannot see. Asking the Shutdowner to stop +// the app with a non-zero code is what turns that into a visible +// failure, and it is the whole of "fatal" here — no panic, no +// os.Exit, and every stop hook still runs. // -// The context cancel that follows only unwinds serve()'s own wait. -// The shutdown itself runs through fx's normal stop sequence, so the -// clean-shutdown drain in cleanShutdown is reached unchanged. -func (s *Server) shutdownOnListenFailure() { +// The context cancel that follows only unwinds serve()'s own wait, +// and is skipped before serve has installed one. The shutdown itself +// runs through fx's normal stop sequence, so the clean-shutdown drain +// in cleanShutdown is reached unchanged. +func (s *Server) shutdownWithFailure() { err := s.params.Shutdowner.Shutdown( - fx.ExitCode(ListenFailureExitCode), + fx.ExitCode(StartupFailureExitCode), ) if err != nil { s.log.Error("shutdown request failed", "error", err)