From 0f5bd51b0923ca9620407c361ad5e2970f4d17de Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 13:46:56 +0200 Subject: [PATCH] Every setting can be given as an environment variable (closes #128) Each config key can now be set by PIXA_ plus the key in upper case ("." written as "_"), and the port by PORT. A present variable, even an empty one, is read before the config file through the existing typed getters, so every existing check covers it; errors name the key and the variable, never the signing key or metrics password. A variable named in the file's env: section overrides both. An empty string for blocked_networks or trusted_proxies is now an empty list. The image no longer bakes in config.docker.yml or passes --config; its HEALTHCHECK probes ${PORT:-8080}. The config file is looked for under /etc/pixa rather than /etc/pixad. Also covers #99. Model: opus-5-5 --- Dockerfile | 13 +- README.md | 43 ++++- TODO.md | 13 +- config.docker.yml | 11 -- config.example.yml | 9 + internal/config/config.go | 230 +++++++++++++----------- internal/config/env_internal_test.go | 255 +++++++++++++++++++++++++++ 7 files changed, 448 insertions(+), 126 deletions(-) delete mode 100644 config.docker.yml create mode 100644 internal/config/env_internal_test.go diff --git a/Dockerfile b/Dockerfile index bec19d0..96d4a5e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -67,16 +67,17 @@ RUN adduser -D -H -s /sbin/nologin pixad && \ mkdir -p /var/lib/pixa /etc/pixa && \ chown pixad:pixad /var/lib/pixa -# Copy the image config; signing_key comes from PIXA_SIGNING_KEY. -# Mount a file over /etc/pixa/config.yml to override anything else. -COPY config.docker.yml /etc/pixa/config.yml - USER pixad WORKDIR /var/lib/pixa EXPOSE 8080 +# Shell form so the probe follows PORT; a port set only in a mounted +# config file is not seen here. HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \ - CMD wget --spider -q http://localhost:8080/.well-known/healthcheck.json || exit 1 + CMD wget --spider -q "http://localhost:${PORT:-8080}/.well-known/healthcheck.json" || exit 1 -ENTRYPOINT ["/usr/local/bin/pixad", "--config", "/etc/pixa/config.yml"] +# Settings come from PORT and the PIXA_ environment variables; only +# PIXA_SIGNING_KEY is required. A config file mounted at +# /etc/pixa/config.yml is optional and is read when present. +ENTRYPOINT ["/usr/local/bin/pixad"] diff --git a/README.md b/README.md index b167214..4c4d859 100644 --- a/README.md +++ b/README.md @@ -27,12 +27,12 @@ make docker docker run -p 8080:8080 -e PIXA_SIGNING_KEY="$(openssl rand -base64 32)" pixa:latest ``` -A container is configured two ways. The signing key comes from the -`PIXA_SIGNING_KEY` environment variable, which the baked-in config -reads; if it is unset the container exits at startup naming the -variable. Everything else uses built-in defaults, so to change any -other setting mount your own file over `/etc/pixa/config.yml` (see -`config.example.yml` for the full set of keys). +A container takes its settings from environment variables (see +Configuration below for the list). Only `PIXA_SIGNING_KEY` is required; if +it is unset the container exits at startup naming the variable. Everything +else has a built-in default. A config file mounted at `/etc/pixa/config.yml` +is optional: it is read when present, and an environment variable wins over +the same setting in it. ## Rationale @@ -125,7 +125,36 @@ For the same image at quality 40 with fit `contain`, the input ends in ### Configuration -Configured via YAML file (`--config`). Key settings: +Every setting can be given as an environment variable, in a YAML config +file (`--config`), or both. A variable present in the environment wins over +the file, even when it is empty, and the file wins over the built-in +default. The one exception is a variable named in the file's `env:` section: +it is set while the file loads, so it overrides both the environment the +process was started with and the file's own key. A variable's value is +parsed as the same text in the file would be. The three lists take +comma-separated entries, with the spaces around each trimmed; an empty +variable is an empty list. A value that does not parse or is invalid aborts +startup, naming the variable. + +| Variable | Config key | Meaning | +| ------------------------------------ | ------------------------------- | ---------------------------------------------------------------------------- | +| `PIXA_SIGNING_KEY` | `signing_key` | Required: secret for signed and encrypted URLs and login, 32+ characters | +| `PORT` | `port` | Port to listen on; default `8080` | +| `PIXA_STATE_DIR` | `state_dir` | Directory for the database and the disk cache; default `/var/lib/pixa` | +| `PIXA_DB_URL` | `db_url` | SQLite database URL; default `state.sqlite3` in the state directory | +| `PIXA_CACHE_MAX_BYTES` | `cache_max_bytes` | Disk cache limit in bytes; `0` disables it; default 75% of free space | +| `PIXA_ALLOWLIST_HOSTS` | `allowlist_hosts` | Upstream hosts served without a signature | +| `PIXA_BLOCKED_NETWORKS` | `blocked_networks` | CIDR ranges never fetched from, on top of the built-in ones | +| `PIXA_TRUSTED_PROXIES` | `trusted_proxies` | CIDR ranges of proxies whose `X-Forwarded-For` is believed; default RFC 1918 | +| `PIXA_ALLOW_HTTP` | `allow_http` | Allow plain-HTTP upstreams, for testing only; default `false` | +| `PIXA_UPSTREAM_CONNECTIONS_PER_HOST` | `upstream_connections_per_host` | Concurrent connections per upstream host; default `20` | +| `PIXA_METRICS_USERNAME` | `metrics.username` | Username for `/metrics`, which is served only when both are set | +| `PIXA_METRICS_PASSWORD` | `metrics.password` | Password for `/metrics`; set together with the username | +| `PIXA_SENTRY_DSN` | `sentry_dsn` | Sentry DSN for error reporting; empty disables it | +| `PIXA_DEBUG` | `debug` | Debug logging and plain-HTTP local development; default `false` | +| `PIXA_MAINTENANCE_MODE` | `maintenance_mode` | Maintenance flag reported by the health check; default `false` | + +Key settings in more detail: - `access_control_allow_origin` — CORS origin - `allowlist_hosts` — list of allowed upstream hosts diff --git a/TODO.md b/TODO.md index fb5bcd3..4dc5290 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,18 @@ exhaustion # Completed Steps +- 2026-09-28 every setting as an environment variable (closes #128, also + covers #99): each config key can be set by `PIXA_` plus the key in upper + case (`.` written as `_`), and the port by `PORT`; a variable present in + the environment, even empty, wins over the config file, which wins over + the default; the typed getters read the variable first, so every existing + check applies to it and a bad value aborts startup naming the variable; + lists are comma-separated, and an empty variable (or `""` in the file) is + an empty list; the Docker image no longer bakes in `config.docker.yml` or + passes `--config`, and its `HEALTHCHECK` probes `PORT` (default `8080`); + the config file is looked for under `/etc/pixa` and `~/.config/pixa` + instead of the daemon name `pixad`; documented in `README.md` and + `config.example.yml`. - 2026-09-28 quality and fit in the URL signature (closes #60): the signed data is now `host:path:query:width:height:format:expiration:quality:fit`, using `85` and `cover` when the URL has no `q` or `fit`, so one signed @@ -178,7 +190,6 @@ exhaustion - P2: auto format selection (format=auto based on Accept header) - P2: configuration - add all configuration options from README - - environment variable overrides - YAML config file support - P2: operational - optional Sentry error reporting diff --git a/config.docker.yml b/config.docker.yml deleted file mode 100644 index 548b45a..0000000 --- a/config.docker.yml +++ /dev/null @@ -1,11 +0,0 @@ -# Pixa configuration baked into the Docker image. -# -# The signing key is read from the PIXA_SIGNING_KEY environment -# variable; startup aborts naming it when it is unset. Every other key -# is omitted so its default applies. Operators who need more (an -# allowlist, metrics, and so on) mount their own file over -# /etc/pixa/config.yml. - -signing_key: "${ENV:PIXA_SIGNING_KEY}" -state_dir: /var/lib/pixa -port: 8080 diff --git a/config.example.yml b/config.example.yml index 09f415f..8df6a35 100644 --- a/config.example.yml +++ b/config.example.yml @@ -1,4 +1,13 @@ # Pixa Example Configuration +# +# Every key can also be set by an environment variable, which wins over +# this file: PIXA_ plus the key in upper case, with "." written as "_" +# (state_dir is PIXA_STATE_DIR, metrics.username is +# PIXA_METRICS_USERNAME). The one exception is port, which is set by +# PORT. In a variable, a list is comma-separated. A variable named in +# this file's env: section is set while the file loads, so it overrides +# both the environment the process was started with and this file's own +# key. # Server settings port: 8080 diff --git a/internal/config/config.go b/internal/config/config.go index ea15bf7..a2e2037 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -16,7 +16,6 @@ import ( "git.eeqj.de/sneak/smartconfig" "go.uber.org/fx" - "sneak.berlin/go/pixa/internal/globals" "sneak.berlin/go/pixa/internal/logger" ) @@ -92,8 +91,7 @@ var ( type Params struct { fx.In - Globals *globals.Globals - Logger *logger.Logger + Logger *logger.Logger } // Config holds application configuration values. @@ -137,24 +135,27 @@ type Config struct { CacheMaxBytes int64 // cacheMaxBytesExplicit records whether cache_max_bytes was - // explicitly set in the configuration file. Explicit values are - // used exactly as given; only an omitted key gets the computed - // default (and its floor) in resolveCacheMaxBytes. + // explicitly set, in the environment or the configuration file. + // Explicit values are used exactly as given; only an omitted key + // gets the computed default (and its floor) in resolveCacheMaxBytes. cacheMaxBytesExplicit bool } -// New creates a new Config instance by loading configuration from file. +// New creates a new Config instance from the environment and the +// config file. func New(_ fx.Lifecycle, params Params) (*Config, error) { log := params.Logger.Get() - name := params.Globals.Appname - sc, err := loadConfigFile(log, name) + // Look for the config file under the project name (/etc/pixa/, + // ~/.config/pixa/), matching the /var/lib/pixa state directory, + // not under the daemon name pixad. + sc, err := loadConfigFile(log, "pixa") if err != nil { return nil, err } if sc == nil { - log.Info("no config file found, using defaults") + log.Info("no config file found, using environment variables and defaults") } c, err := newFromSmartConfig(sc) @@ -179,22 +180,23 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { return c, nil } -// newFromSmartConfig constructs a Config from a loaded smartconfig -// instance and validates it. A nil sc means no config file was found, -// in which case every option takes its default value. A key that is -// present but unparseable or invalid is an error: defaults apply only -// to omitted keys, never to invalid explicit values. +// newFromSmartConfig constructs a Config from the environment and a +// loaded smartconfig instance, and validates it. A nil sc means no +// config file was found, in which case every option the environment +// does not set takes its default value. A key that is present but +// unparseable or invalid is an error: defaults apply only to omitted +// keys, never to invalid explicit values. func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { if sc != nil { err := validateKnownKeys(sc) if err != nil { return nil, err } + } - err = validateAllowlistHostsValue(sc) - if err != nil { - return nil, err - } + err := validateAllowlistHostsValue(sc) + if err != nil { + return nil, err } blockedNetworks, err := parseCIDRList(sc, keyBlockedNetworks) @@ -238,10 +240,8 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { // The computed default for cache_max_bytes needs a validated // state_dir, so it is resolved later (resolveCacheMaxBytes); here // we only record whether the operator set the key explicitly. - if sc != nil { - if _, present := sc.Get(keyCacheMaxBytes); present { - c.cacheMaxBytesExplicit = true - } + if _, present := lookupValue(sc, keyCacheMaxBytes); present { + c.cacheMaxBytesExplicit = true } // Build DBURL from StateDir if not explicitly set. The derived URL @@ -249,12 +249,9 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { // explicitly empty value. c.DBURL = loader.stringVal(keyDBURL, "") if c.DBURL == "" && loader.err == nil { - if sc != nil { - if _, present := sc.Get(keyDBURL); present { - return nil, fmt.Errorf( - "config key %q: %w; omit the key to derive it from state_dir", - keyDBURL, errValueEmpty) - } + if _, present := lookupValue(sc, keyDBURL); present { + return nil, fmt.Errorf("%s: %w; omit it to derive it from state_dir", + settingName(keyDBURL), errValueEmpty) } c.DBURL = fmt.Sprintf("file:%s/state.sqlite3?_journal_mode=WAL", c.StateDir) @@ -356,6 +353,54 @@ func isKnownConfigKey(key string) bool { return false } +// envVarNames returns, for each configuration key, the environment +// variable that also sets it: PIXA_ plus the key in upper case, with "." +// written as "_", except the port, which REPO_POLICIES.md requires to be +// PORT. metrics is set through its two subkeys; env has no variable. +func envVarNames() map[string]string { + return map[string]string{ //nolint:gosec // G101: variable names, not secrets + keyDebug: "PIXA_DEBUG", + keyMaintenanceMode: "PIXA_MAINTENANCE_MODE", + keyPort: "PORT", + keyStateDir: "PIXA_STATE_DIR", + keySentryDSN: "PIXA_SENTRY_DSN", + keyDBURL: "PIXA_DB_URL", + keyMetricsUsername: "PIXA_METRICS_USERNAME", + keyMetricsPassword: "PIXA_METRICS_PASSWORD", + keySigningKey: "PIXA_SIGNING_KEY", + keyAllowlistHosts: "PIXA_ALLOWLIST_HOSTS", + keyAllowHTTP: "PIXA_ALLOW_HTTP", + keyUpstreamConnectionsPerHost: "PIXA_UPSTREAM_CONNECTIONS_PER_HOST", + keyCacheMaxBytes: "PIXA_CACHE_MAX_BYTES", + keyBlockedNetworks: "PIXA_BLOCKED_NETWORKS", + keyTrustedProxies: "PIXA_TRUSTED_PROXIES", + } +} + +// lookupValue returns the value set for key and whether one is set. The +// key's environment variable wins when it is present, even when empty; +// its value is a string, read exactly as the same text quoted in the +// config file would be. Otherwise the config file's value is used. +func lookupValue(sc *smartconfig.Config, key string) (any, bool) { + value, present := os.LookupEnv(envVarNames()[key]) + if present { + return value, true + } + + if sc == nil { + return nil, false + } + + return sc.Get(key) +} + +// settingName names key in an error message together with its +// environment variable, since either one may have set the value. +func settingName(key string) string { + return fmt.Sprintf("config key %q (environment variable %s)", + key, envVarNames()[key]) +} + // ensureStateDirWritable verifies at startup that StateDir can be // created and written to, so a misconfigured path aborts startup // instead of failing later at first use. @@ -364,28 +409,28 @@ func (c *Config) ensureStateDirWritable() error { err := os.MkdirAll(c.StateDir, stateDirPerms) if err != nil { - return fmt.Errorf("config key %q: cannot create directory %q: %w", - keyStateDir, c.StateDir, err) + return fmt.Errorf("%s: cannot create directory %q: %w", + settingName(keyStateDir), c.StateDir, err) } probe, err := os.CreateTemp(c.StateDir, ".startup-write-probe-*") if err != nil { - return fmt.Errorf("config key %q: directory %q is not writable: %w", - keyStateDir, c.StateDir, err) + return fmt.Errorf("%s: directory %q is not writable: %w", + settingName(keyStateDir), c.StateDir, err) } probePath := probe.Name() err = probe.Close() if err != nil { - return fmt.Errorf("config key %q: cannot close probe file %q: %w", - keyStateDir, probePath, err) + return fmt.Errorf("%s: cannot close probe file %q: %w", + settingName(keyStateDir), probePath, err) } err = os.Remove(probePath) if err != nil { - return fmt.Errorf("config key %q: cannot remove probe file %q: %w", - keyStateDir, probePath, err) + return fmt.Errorf("%s: cannot remove probe file %q: %w", + settingName(keyStateDir), probePath, err) } return nil @@ -396,18 +441,19 @@ func (c *Config) ensureStateDirWritable() error { // key value itself is never echoed in error messages. func (c *Config) validateSigningKey() error { if c.SigningKey == "" { - return fmt.Errorf("config key %q: %w", keySigningKey, errValueRequired) + return fmt.Errorf("%s: %w", settingName(keySigningKey), errValueRequired) } // Minimum key length for security (32 bytes = 256 bits) const minKeyLength = 32 if len(c.SigningKey) < minKeyLength { - return fmt.Errorf("config key %q: %w: must be at least %d characters, got %d", - keySigningKey, errValueTooShort, minKeyLength, len(c.SigningKey)) + return fmt.Errorf("%s: %w: must be at least %d characters, got %d", + settingName(keySigningKey), errValueTooShort, minKeyLength, + len(c.SigningKey)) } if c.SigningKey == placeholderSigningKey { - return fmt.Errorf("config key %q: %w", keySigningKey, errPlaceholderKey) + return fmt.Errorf("%s: %w", settingName(keySigningKey), errPlaceholderKey) } return nil @@ -423,25 +469,25 @@ func (c *Config) validate() error { const maxPort = 65535 if c.Port < 1 || c.Port > maxPort { - return fmt.Errorf("config key %q: value %d is %w 1-%d", - keyPort, c.Port, errPortOutOfRange, maxPort) + return fmt.Errorf("%s: value %d is %w 1-%d", + settingName(keyPort), c.Port, errPortOutOfRange, maxPort) } if c.UpstreamConnectionsPerHost < 1 { - return fmt.Errorf("config key %q: value %d %w", - keyUpstreamConnectionsPerHost, c.UpstreamConnectionsPerHost, - errTooFewConnections) + return fmt.Errorf("%s: value %d %w", + settingName(keyUpstreamConnectionsPerHost), + c.UpstreamConnectionsPerHost, errTooFewConnections) } if c.StateDir == "" { - return fmt.Errorf("config key %q: %w", keyStateDir, errValueEmpty) + return fmt.Errorf("%s: %w", settingName(keyStateDir), errValueEmpty) } // Zero is valid (it disables the disk cache); only negative // values are rejected. No floor applies to explicit values. if c.CacheMaxBytes < 0 { - return fmt.Errorf("config key %q: value %d %w", - keyCacheMaxBytes, c.CacheMaxBytes, errMustNotBeNegative) + return fmt.Errorf("%s: value %d %w", + settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative) } for _, host := range c.AllowlistHosts { @@ -454,14 +500,15 @@ func (c *Config) validate() error { if c.SentryDSN != "" { parsed, err := url.Parse(c.SentryDSN) if err != nil || parsed.Scheme == "" || parsed.Host == "" { - return fmt.Errorf("config key %q: value %q is %w", - keySentryDSN, c.SentryDSN, errNotAValidURL) + return fmt.Errorf("%s: value %q is %w", + settingName(keySentryDSN), c.SentryDSN, errNotAValidURL) } } if (c.MetricsUsername == "") != (c.MetricsPassword == "") { - return fmt.Errorf("config keys %q and %q %w", - keyMetricsUsername, keyMetricsPassword, errMustBeSetTogether) + return fmt.Errorf("%s and %s %w", + settingName(keyMetricsUsername), settingName(keyMetricsPassword), + errMustBeSetTogether) } return nil @@ -476,13 +523,13 @@ func (c *Config) validate() error { // disable URL signing. func validateAllowlistHost(host string) error { if strings.Contains(host, "://") || strings.ContainsAny(host, "/ \t") { - return fmt.Errorf("config key %q: entry %q %w", - keyAllowlistHosts, host, errNotBareHostname) + return fmt.Errorf("%s: entry %q %w", + settingName(keyAllowlistHosts), host, errNotBareHostname) } if strings.Trim(host, ".") == "" { - return fmt.Errorf("config key %q: entry %q %w", - keyAllowlistHosts, host, errNoHostnameLabels) + return fmt.Errorf("%s: entry %q %w", + settingName(keyAllowlistHosts), host, errNoHostnameLabels) } return nil @@ -598,11 +645,7 @@ func (l *strictLoader) boolVal(key string, defaultVal bool) bool { // is omitted. A present value that is not a string, or is explicitly // null, is an error. func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) { - if sc == nil { - return defaultVal, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return defaultVal, nil } @@ -624,11 +667,7 @@ func getString(sc *smartconfig.Config, key, defaultVal string) (string, error) { // omitted. A present value that is not a whole number, or is explicitly // null, is an error; fractional values are never truncated. func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { - if sc == nil { - return defaultVal, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return defaultVal, nil } @@ -652,8 +691,8 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { case string: parsed, err := strconv.Atoi(strings.TrimSpace(val)) if err != nil { - return 0, fmt.Errorf("config key %q: value %q is %w", - key, val, errNotAnInteger) + return 0, fmt.Errorf("%s: value %q is %w", + settingName(key), val, errNotAnInteger) } return parsed, nil @@ -668,11 +707,7 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { // is explicitly null, is an error; fractional values are never // truncated and out-of-range values are never clamped. func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, error) { - if sc == nil { - return defaultVal, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return defaultVal, nil } @@ -703,8 +738,8 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro case string: parsed, err := strconv.ParseInt(strings.TrimSpace(val), 10, 64) if err != nil { - return 0, fmt.Errorf("config key %q: value %q is %w", - key, val, errNotAnInteger) + return 0, fmt.Errorf("%s: value %q is %w", + settingName(key), val, errNotAnInteger) } return parsed, nil @@ -719,11 +754,7 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro // string), or is explicitly null, is an error; numbers are not accepted // as booleans. func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) { - if sc == nil { - return defaultVal, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return defaultVal, nil } @@ -738,8 +769,8 @@ func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) case string: parsed, err := strconv.ParseBool(strings.TrimSpace(val)) if err != nil { - return false, fmt.Errorf("config key %q: value %q is %w", - key, val, errNotABoolean) + return false, fmt.Errorf("%s: value %q is %w", + settingName(key), val, errNotABoolean) } return parsed, nil @@ -755,7 +786,7 @@ func getBool(sc *smartconfig.Config, key string, defaultVal bool) (bool, error) // (or a comma-separated string), a non-string entry, or an empty entry // is an error, never silently skipped. func validateAllowlistHostsValue(sc *smartconfig.Config) error { - raw, ok := sc.Get(keyAllowlistHosts) + raw, ok := lookupValue(sc, keyAllowlistHosts) if !ok { return nil } @@ -785,8 +816,8 @@ func validateAllowlistHostsValue(sc *smartconfig.Config) error { for part := range strings.SplitSeq(val, ",") { if strings.TrimSpace(part) == "" { - return fmt.Errorf("config key %q: value %q %w", - keyAllowlistHosts, val, errEmptyEntry) + return fmt.Errorf("%s: value %q %w", + settingName(keyAllowlistHosts), val, errEmptyEntry) } } default: @@ -802,11 +833,7 @@ func validateAllowlistHostsValue(sc *smartconfig.Config) error { // comma-separated string (backwards compatibility). Malformed entries // are rejected beforehand by validateAllowlistHostsValue. func getStringSlice(sc *smartconfig.Config) []string { - if sc == nil { - return nil - } - - val, ok := sc.Get(keyAllowlistHosts) + val, ok := lookupValue(sc, keyAllowlistHosts) if !ok || val == nil { return nil } @@ -866,11 +893,7 @@ func defaultTrustedProxies() []netip.Prefix { // aborts startup naming the key and the offending value; the default // (an empty list) applies only to an omitted key. func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) { - if sc == nil { - return nil, nil - } - - raw, ok := sc.Get(key) + raw, ok := lookupValue(sc, key) if !ok { return nil, nil } @@ -889,8 +912,8 @@ func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) { for _, entry := range entries { prefix, err := netip.ParsePrefix(entry) if err != nil { - return nil, fmt.Errorf("config key %q: value %q is %w", - key, entry, errNotAValidCIDR) + return nil, fmt.Errorf("%s: value %q is %w", + settingName(key), entry, errNotAValidCIDR) } prefixes = append(prefixes, prefix) @@ -901,7 +924,8 @@ func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) { // cidrListEntries extracts the raw entries of the named CIDR-list key as // trimmed, non-empty strings, from either a YAML list of strings or a -// comma-separated string. Any other shape is a configuration error. +// comma-separated string; an empty string is an empty list, as for +// allowlist_hosts. Any other shape is a configuration error. func cidrListEntries(raw any, key string) ([]string, error) { switch val := raw.(type) { case []any: @@ -926,11 +950,15 @@ func cidrListEntries(raw any, key string) ([]string, error) { case string: entries := make([]string, 0) + if strings.TrimSpace(val) == "" { + return entries, nil + } + for part := range strings.SplitSeq(val, ",") { trimmed := strings.TrimSpace(part) if trimmed == "" { - return nil, fmt.Errorf("config key %q: value %q %w", - key, val, errEmptyEntry) + return nil, fmt.Errorf("%s: value %q %w", + settingName(key), val, errEmptyEntry) } entries = append(entries, trimmed) diff --git a/internal/config/env_internal_test.go b/internal/config/env_internal_test.go new file mode 100644 index 0000000..ea1e361 --- /dev/null +++ b/internal/config/env_internal_test.go @@ -0,0 +1,255 @@ +package config + +import ( + "net/netip" + "os" + "reflect" + "slices" + "strings" + "testing" +) + +// TestMain unsets PORT and every PIXA_ environment variable before the +// tests run, so each test sees only the variables it sets itself, not +// whatever the shell running the tests exports. +func TestMain(m *testing.M) { + for _, entry := range os.Environ() { + name, _, _ := strings.Cut(entry, "=") + if name != "PORT" && !strings.HasPrefix(name, "PIXA_") { + continue + } + + err := os.Unsetenv(name) + if err != nil { + panic(err) + } + } + + m.Run() +} + +// wantStartupError fails the test unless err is a startup error that +// mentions every one of wants. +func wantStartupError(t *testing.T, err error, wants ...string) { + t.Helper() + + if err == nil { + t.Fatalf("want a startup error mentioning %q, got none", wants) + } + + t.Logf("got expected error: %v", err) + + for _, want := range wants { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err.Error(), want) + } + } +} + +// TestEnvironmentSetsEveryKey sets every key from its environment +// variable, with no config file at all: PORT for the port, and PIXA_ +// plus the key in upper case, "." written as "_", for every other key. +func TestEnvironmentSetsEveryKey(t *testing.T) { + t.Setenv("PIXA_DEBUG", "true") + t.Setenv("PIXA_MAINTENANCE_MODE", "1") + t.Setenv("PORT", "9090") + t.Setenv("PIXA_STATE_DIR", "/srv/pixa-env") + t.Setenv("PIXA_SENTRY_DSN", "https://abc123@sentry.example.com/42") + t.Setenv("PIXA_DB_URL", "file:/srv/pixa-env/other.sqlite3") + t.Setenv("PIXA_METRICS_USERNAME", "metricsuser") + t.Setenv("PIXA_METRICS_PASSWORD", "metricspass") + t.Setenv("PIXA_SIGNING_KEY", validTestSigningKey) + t.Setenv("PIXA_ALLOWLIST_HOSTS", "s3.sneak.cloud,.example.com") + t.Setenv("PIXA_ALLOW_HTTP", "true") + t.Setenv("PIXA_UPSTREAM_CONNECTIONS_PER_HOST", "5") + t.Setenv("PIXA_CACHE_MAX_BYTES", "1024") + t.Setenv("PIXA_BLOCKED_NETWORKS", "203.0.113.0/24") + t.Setenv("PIXA_TRUSTED_PROXIES", "192.0.2.0/24") + + c, err := newFromSmartConfig(nil) + if err != nil { + t.Fatalf("configuration from the environment alone should load: %v", err) + } + + want := Config{ + Debug: true, + MaintenanceMode: true, + Port: 9090, + StateDir: "/srv/pixa-env", + SentryDSN: "https://abc123@sentry.example.com/42", + DBURL: "file:/srv/pixa-env/other.sqlite3", + MetricsUsername: "metricsuser", + MetricsPassword: "metricspass", + SigningKey: validTestSigningKey, + AllowlistHosts: []string{testHostS3, ".example.com"}, + AllowHTTP: true, + UpstreamConnectionsPerHost: 5, + CacheMaxBytes: 1024, + cacheMaxBytesExplicit: true, + BlockedNetworks: []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")}, + TrustedProxies: []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}, + } + + if !reflect.DeepEqual(*c, want) { + t.Errorf("config from the environment =\n%+v\nwant\n%+v", *c, want) + } +} + +// TestPortFromEnvironmentOverridesConfigFile checks that PORT wins over +// the port in the config file. +func TestPortFromEnvironmentOverridesConfigFile(t *testing.T) { + t.Setenv("PORT", "9090") + + c, err := configFromYAML(t, signingKeyLine+"port: 8080\n") + if err != nil { + t.Fatalf("PORT=9090 with port 8080 in the file should load: %v", err) + } + + if c.Port != 9090 { + t.Errorf("Port = %d, want 9090 from PORT, not 8080 from the file", c.Port) + } +} + +// TestInvalidPortFromEnvironmentAbortsStartup checks that a PORT that is +// not a number, or is outside the port range, aborts startup naming PORT +// and the value, even though the file's port is valid. +func TestInvalidPortFromEnvironmentAbortsStartup(t *testing.T) { + t.Setenv("PORT", "banana") + + _, err := configFromYAML(t, signingKeyLine+"port: 8080\n") + wantStartupError(t, err, "PORT", "banana") + + t.Setenv("PORT", "70000") + + _, err = configFromYAML(t, signingKeyLine+"port: 8080\n") + wantStartupError(t, err, "PORT", "70000") +} + +// TestListFromEnvironmentReplacesConfigFileList checks that a list +// variable replaces the file's list, split on commas with the spaces +// around each entry trimmed. +func TestListFromEnvironmentReplacesConfigFileList(t *testing.T) { + t.Setenv("PIXA_ALLOWLIST_HOSTS", " cdn.example.com , .example.org ") + + c, err := configFromYAML(t, signingKeyLine+ + "allowlist_hosts:\n - s3.sneak.cloud\n - sneak.berlin\n") + if err != nil { + t.Fatalf("PIXA_ALLOWLIST_HOSTS should load: %v", err) + } + + want := []string{"cdn.example.com", ".example.org"} + if !slices.Equal(c.AllowlistHosts, want) { + t.Errorf("AllowlistHosts = %v, want %v from PIXA_ALLOWLIST_HOSTS", + c.AllowlistHosts, want) + } +} + +// TestInvalidBlockedNetworksFromEnvironmentAbortsStartup checks that an +// invalid CIDR, or an empty entry, in PIXA_BLOCKED_NETWORKS aborts +// startup naming the variable, as the same list in the file does. +func TestInvalidBlockedNetworksFromEnvironmentAbortsStartup(t *testing.T) { + t.Setenv("PIXA_BLOCKED_NETWORKS", "203.0.113.0/24,not-a-cidr") + + _, err := configFromYAML(t, signingKeyLine) + wantStartupError(t, err, "PIXA_BLOCKED_NETWORKS", "not-a-cidr") + + t.Setenv("PIXA_BLOCKED_NETWORKS", "203.0.113.0/24,,198.51.100.0/24") + + _, err = configFromYAML(t, signingKeyLine) + wantStartupError(t, err, "PIXA_BLOCKED_NETWORKS") +} + +// TestInvalidDebugFromEnvironmentAbortsStartup checks that a PIXA_DEBUG +// that strconv.ParseBool rejects aborts startup instead of defaulting. +func TestInvalidDebugFromEnvironmentAbortsStartup(t *testing.T) { + t.Setenv("PIXA_DEBUG", "maybe") + + _, err := configFromYAML(t, signingKeyLine) + wantStartupError(t, err, "PIXA_DEBUG", "maybe") +} + +// TestConfigFileAloneBehavesAsBefore checks that with no variables set +// (TestMain unsets them) the config file's values are used and omitted +// keys take their defaults. +func TestConfigFileAloneBehavesAsBefore(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine+"port: 9191\n") + if err != nil { + t.Fatalf("config file should load: %v", err) + } + + if c.Port != 9191 { + t.Errorf("Port = %d, want 9191 from the file", c.Port) + } + + if c.StateDir != DefaultStateDir { + t.Errorf("StateDir = %q, want default %q", c.StateDir, DefaultStateDir) + } + + if !slices.Equal(c.TrustedProxies, defaultTrustedProxies()) { + t.Errorf("TrustedProxies = %v, want default %v", + c.TrustedProxies, defaultTrustedProxies()) + } +} + +// TestEmptyTrustedProxiesFromEnvironmentTrustsNoOne checks that an empty +// PIXA_TRUSTED_PROXIES is an empty list, like [] in the file: it trusts +// no proxy instead of taking the default ranges. +func TestEmptyTrustedProxiesFromEnvironmentTrustsNoOne(t *testing.T) { + t.Setenv("PIXA_TRUSTED_PROXIES", "") + + c, err := configFromYAML(t, signingKeyLine) + if err != nil { + t.Fatalf("empty PIXA_TRUSTED_PROXIES should load: %v", err) + } + + if len(c.TrustedProxies) != 0 { + t.Errorf("TrustedProxies = %v, want none", c.TrustedProxies) + } +} + +// TestEmptyVariableDoesNotFallBackToConfigFile checks that a variable +// that is present but empty is a set value: an empty PIXA_STATE_DIR +// aborts startup like state_dir: "" in the file, instead of falling +// through to the file's state_dir. +func TestEmptyVariableDoesNotFallBackToConfigFile(t *testing.T) { + t.Setenv("PIXA_STATE_DIR", "") + + _, err := configFromYAML(t, signingKeyLine+"state_dir: /srv/pixa-file\n") + wantStartupError(t, err, "PIXA_STATE_DIR") +} + +// TestMissingSigningKeyNamesItsVariable checks that with no config file +// and no PIXA_SIGNING_KEY, startup aborts naming the variable, which is +// how a container started without it reports the problem. +func TestMissingSigningKeyNamesItsVariable(t *testing.T) { + t.Parallel() + + _, err := newFromSmartConfig(nil) + wantStartupError(t, err, "PIXA_SIGNING_KEY") +} + +// TestSecretsFromEnvironmentAreNotPrinted checks that errors about the +// signing key and the metrics password name their variables but never +// print their values. +func TestSecretsFromEnvironmentAreNotPrinted(t *testing.T) { + t.Setenv("PIXA_SIGNING_KEY", "short-signing-secret") + + _, err := newFromSmartConfig(nil) + wantStartupError(t, err, "PIXA_SIGNING_KEY") + + if strings.Contains(err.Error(), "short-signing-secret") { + t.Errorf("error %q prints the signing key", err.Error()) + } + + t.Setenv("PIXA_SIGNING_KEY", validTestSigningKey) + t.Setenv("PIXA_METRICS_PASSWORD", "metrics-password-secret") + + _, err = newFromSmartConfig(nil) + wantStartupError(t, err, "PIXA_METRICS_PASSWORD") + + if strings.Contains(err.Error(), "metrics-password-secret") { + t.Errorf("error %q prints the metrics password", err.Error()) + } +}