Fail loudly on set-but-unparseable env config values (closes #80)
All checks were successful
check / check (push) Successful in 6m3s
All checks were successful
check / check (push) Successful in 6m3s
The config env helpers silently substituted the documented default whenever a variable was set but could not be parsed, so a typo in an operator-supplied value produced a running daemon with configuration nobody asked for instead of a startup failure. `PORT=eighty` quietly listened on 8080 and `DEBUG=ture` quietly disabled debug logging. Defaults now apply only to variables that are unset or empty. Any variable that is set but unparseable is a hard error that names the key and the offending value and aborts startup through fx. - add `envPositiveInt` with `ErrNonPositiveValue`, copied verbatim from the definition on the unmerged #87 so that rebasing after it lands is a delete-one-copy operation rather than a semantic merge - remove `envInt` entirely; `PORT` is parsed by a new `envPort`, which adds the TCP upper bound (`ErrInvalidPort`, 1..65535) - change `envBool` to return an error and parse with `strconv.ParseBool`, so `yes`, `on`, and typos are rejected rather than silently treated as false; callers are `DEBUG` and `MAINTENANCE_MODE` - move env loading into `loadFromEnv`, with the environment check extracted to `resolveEnvironment` (also as #87 defines it), keeping `New` within the funlen budget `envString` parses nothing and `envDuration` was already fail-loud, so both are unchanged. A repo-wide audit of `os.Getenv`/`os.LookupEnv` found no parse sites outside `internal/config`. Tests cover each helper with a table (unset, valid, set-but-invalid) plus `config.New`-level cases proving a bad `PORT`, `DEBUG`, or `MAINTENANCE_MODE` aborts startup while unset variables still get their defaults. README documents the fail-loud rule and the accepted boolean spellings.
This commit is contained in:
@@ -7,7 +7,6 @@ import (
|
||||
"log/slog"
|
||||
"os"
|
||||
"strconv"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"go.uber.org/fx"
|
||||
@@ -31,12 +30,24 @@ const (
|
||||
// defaultRetentionSweepInterval is how often the retention
|
||||
// reaper deletes events older than each webhook's RetentionDays.
|
||||
defaultRetentionSweepInterval = time.Hour
|
||||
|
||||
// maxPort is the highest valid TCP port number. The lower
|
||||
// bound (at least 1) is enforced by envPositiveInt.
|
||||
maxPort = 65535
|
||||
)
|
||||
|
||||
// ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT
|
||||
// contains an unrecognised value.
|
||||
var ErrInvalidEnvironment = errors.New("invalid environment")
|
||||
|
||||
// ErrNonPositiveValue is returned when an environment variable that
|
||||
// requires a positive integer is set to zero or a negative number.
|
||||
var ErrNonPositiveValue = errors.New("value must be positive")
|
||||
|
||||
// ErrInvalidPort is returned when an environment variable holding a
|
||||
// TCP port number is set above the valid port range.
|
||||
var ErrInvalidPort = errors.New("invalid port")
|
||||
|
||||
//nolint:revive // ConfigParams is a standard fx naming convention.
|
||||
type ConfigParams struct {
|
||||
fx.In
|
||||
@@ -81,27 +92,81 @@ func envString(key string) string {
|
||||
}
|
||||
|
||||
// envBool returns the value of the named environment variable
|
||||
// parsed as a boolean. Returns defaultValue if not set.
|
||||
func envBool(key string, defaultValue bool) bool {
|
||||
if v := os.Getenv(key); v != "" {
|
||||
return strings.EqualFold(v, "true") || v == "1"
|
||||
// parsed as a boolean. Returns defaultValue if not set. If the
|
||||
// variable is set but cannot be parsed, it returns a wrapped error
|
||||
// naming the key and the bad value, so startup fails loudly rather
|
||||
// than silently falling back to the default.
|
||||
//
|
||||
// Parsing is strconv.ParseBool, which accepts 1, t, T, TRUE, true,
|
||||
// True, 0, f, F, FALSE, false and False. Anything else — "yes",
|
||||
// "on", or a typo like "ture" — is an error rather than a silent
|
||||
// false.
|
||||
func envBool(key string, defaultValue bool) (bool, error) {
|
||||
v := os.Getenv(key)
|
||||
if v == "" {
|
||||
return defaultValue, nil
|
||||
}
|
||||
|
||||
return defaultValue
|
||||
b, err := strconv.ParseBool(v)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf(
|
||||
"invalid boolean for %s: %q: %w", key, v, err,
|
||||
)
|
||||
}
|
||||
|
||||
return b, nil
|
||||
}
|
||||
|
||||
// envInt returns the value of the named environment variable
|
||||
// parsed as an integer. Returns defaultValue if not set or
|
||||
// unparseable.
|
||||
func envInt(key string, defaultValue int) int {
|
||||
if v := os.Getenv(key); v != "" {
|
||||
i, err := strconv.Atoi(v)
|
||||
if err == nil {
|
||||
return i
|
||||
}
|
||||
// envPositiveInt returns the value of the named environment variable
|
||||
// parsed as a positive integer. Returns defaultValue if not set. If
|
||||
// the variable is set but cannot be parsed, or parses to less than
|
||||
// one, it returns a wrapped error naming the key and the bad value,
|
||||
// so startup fails loudly rather than silently falling back to the
|
||||
// default.
|
||||
func envPositiveInt(
|
||||
key string,
|
||||
defaultValue int,
|
||||
) (int, error) {
|
||||
v := os.Getenv(key)
|
||||
if v == "" {
|
||||
return defaultValue, nil
|
||||
}
|
||||
|
||||
return defaultValue
|
||||
i, err := strconv.Atoi(v)
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf(
|
||||
"invalid integer for %s: %q: %w", key, v, err,
|
||||
)
|
||||
}
|
||||
|
||||
if i < 1 {
|
||||
return 0, fmt.Errorf(
|
||||
"%w: %s must be at least 1, got %q",
|
||||
ErrNonPositiveValue, key, v,
|
||||
)
|
||||
}
|
||||
|
||||
return i, nil
|
||||
}
|
||||
|
||||
// envPort returns the value of the named environment variable parsed
|
||||
// as a TCP port number. Returns defaultValue if not set. A set value
|
||||
// that is unparseable, below 1, or above maxPort is a hard error
|
||||
// naming the key and the bad value.
|
||||
func envPort(key string, defaultValue int) (int, error) {
|
||||
port, err := envPositiveInt(key, defaultValue)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
}
|
||||
|
||||
if port > maxPort {
|
||||
return 0, fmt.Errorf(
|
||||
"%w: %s must be at most %d, got %d",
|
||||
ErrInvalidPort, key, maxPort, port,
|
||||
)
|
||||
}
|
||||
|
||||
return port, nil
|
||||
}
|
||||
|
||||
// envDuration returns the value of the named environment variable
|
||||
@@ -128,32 +193,52 @@ func envDuration(
|
||||
return d, nil
|
||||
}
|
||||
|
||||
// New creates a Config by reading environment variables.
|
||||
//
|
||||
//nolint:revive // lc parameter is required by fx even if unused.
|
||||
func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
|
||||
log := params.Logger.Get()
|
||||
|
||||
// Determine environment from WEBHOOKER_ENVIRONMENT env var,
|
||||
// default to dev
|
||||
// resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to
|
||||
// dev, and rejects unrecognised values.
|
||||
func resolveEnvironment() (string, error) {
|
||||
environment := os.Getenv("WEBHOOKER_ENVIRONMENT")
|
||||
if environment == "" {
|
||||
environment = EnvironmentDev
|
||||
}
|
||||
|
||||
// Validate environment
|
||||
if environment != EnvironmentDev &&
|
||||
environment != EnvironmentProd {
|
||||
return nil, fmt.Errorf(
|
||||
return "", fmt.Errorf(
|
||||
"%w: WEBHOOKER_ENVIRONMENT must be '%s' or '%s', got '%s'",
|
||||
ErrInvalidEnvironment,
|
||||
EnvironmentDev, EnvironmentProd, environment,
|
||||
)
|
||||
}
|
||||
|
||||
// Parse the retention sweep interval; a set-but-unparseable value
|
||||
// is a hard error so fx aborts startup rather than silently using
|
||||
// the default.
|
||||
return environment, 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
|
||||
// empty), never as a substitute for a value the operator actually
|
||||
// provided.
|
||||
func loadFromEnv() (*Config, error) {
|
||||
environment, err := resolveEnvironment()
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
port, err := envPort("PORT", defaultPort)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
debug, err := envBool("DEBUG", false)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
maintenanceMode, err := envBool("MAINTENANCE_MODE", false)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
retentionSweepInterval, err := envDuration(
|
||||
"RETENTION_SWEEP_INTERVAL",
|
||||
defaultRetentionSweepInterval,
|
||||
@@ -162,21 +247,36 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// Load configuration values from environment variables
|
||||
s := &Config{
|
||||
return &Config{
|
||||
DataDir: envString("DATA_DIR"),
|
||||
Debug: envBool("DEBUG", false),
|
||||
MaintenanceMode: envBool("MAINTENANCE_MODE", false),
|
||||
Debug: debug,
|
||||
MaintenanceMode: maintenanceMode,
|
||||
Environment: environment,
|
||||
MetricsUsername: envString("METRICS_USERNAME"),
|
||||
MetricsPassword: envString("METRICS_PASSWORD"),
|
||||
Port: envInt("PORT", defaultPort),
|
||||
Port: port,
|
||||
SentryDSN: envString("SENTRY_DSN"),
|
||||
RetentionSweepInterval: retentionSweepInterval,
|
||||
log: log,
|
||||
params: ¶ms,
|
||||
}, nil
|
||||
}
|
||||
|
||||
// New creates a Config by reading environment variables.
|
||||
//
|
||||
//nolint:revive // lc parameter is required by fx even if unused.
|
||||
func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
|
||||
log := params.Logger.Get()
|
||||
|
||||
// A set-but-unparseable value anywhere in the environment is a
|
||||
// hard error, so fx aborts startup rather than running with a
|
||||
// silently substituted default.
|
||||
s, err := loadFromEnv()
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
s.log = log
|
||||
s.params = ¶ms
|
||||
|
||||
// Set default DataDir. All SQLite databases (main application
|
||||
// DB and per-webhook event DBs) live here. The same default is
|
||||
// used regardless of environment; override with DATA_DIR if
|
||||
|
||||
Reference in New Issue
Block a user