From 7dc0ad7902ab1fc3621fb4d546a8bbe95cbdc842 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 28 Sep 2026 09:32:50 +0000 Subject: [PATCH 1/3] test: every setting from its environment variable (closes #128) Tests, written before the change, for the PIXA_ variables and PORT: every key set from the environment with no config file, PORT over the file's port, a list variable replacing the file's list, an empty variable as a set value, invalid values aborting startup naming the variable, and the signing key and metrics password never printed. All fail until the change lands, except the check that a config file alone behaves as before. TestMain unsets PORT and every PIXA_ variable so the shell running the tests cannot change their result. Model: opus-5-5 --- internal/config/env_internal_test.go | 255 +++++++++++++++++++++++++++ 1 file changed, 255 insertions(+) create mode 100644 internal/config/env_internal_test.go 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()) + } +} -- 2.54.0 From a7947da6f1f28e5f61b389b67bf005c9da8d264d Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 28 Sep 2026 09:42:23 +0000 Subject: [PATCH 2/3] Take every setting from PIXA_ variables and PORT (closes #128) Each config key can now be set by PIXA_ plus the key in upper case ("." written as "_"), and the port by PORT. One list pairs keys with variables; the typed getters read a present variable, even an empty one, before the config file, so every existing check covers it, and errors a variable can reach name both the key and the variable. An empty string for blocked_networks or trusted_proxies is now an empty list, as for allowlist_hosts. The image no longer bakes in config.docker.yml or passes --config, and its HEALTHCHECK probes ${PORT:-8080}; the config file is looked for under /etc/pixa rather than /etc/pixad, so a file mounted at /etc/pixa/config.yml is still read. Model: opus-5-5 --- Dockerfile | 13 ++- README.md | 40 +++++-- TODO.md | 13 ++- config.docker.yml | 11 -- config.example.yml | 6 + internal/config/config.go | 230 +++++++++++++++++++++----------------- 6 files changed, 187 insertions(+), 126 deletions(-) delete mode 100644 config.docker.yml 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..3f300b4 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,33 @@ 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. 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..3edac9e 100644 --- a/config.example.yml +++ b/config.example.yml @@ -1,4 +1,10 @@ # 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. # 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) -- 2.54.0 From 9b49a9d5b39361964a6654549f3169592d355358 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 28 Sep 2026 10:32:05 +0000 Subject: [PATCH 3/3] Say that a config file's env: section overrides the environment (closes #128) A variable named in the config file's env: section is set while the file loads, so it overrides both the environment the process was started with and the file's own key. README.md and the config.example.yml header said only that a variable wins over the file; they now state this exception where the precedence is given. Model: opus-5-5 --- README.md | 11 +++++++---- config.example.yml | 5 ++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 3f300b4..4c4d859 100644 --- a/README.md +++ b/README.md @@ -128,10 +128,13 @@ For the same image at quality 40 with fit `contain`, the input ends in 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. 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. +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 | | ------------------------------------ | ------------------------------- | ---------------------------------------------------------------------------- | diff --git a/config.example.yml b/config.example.yml index 3edac9e..8df6a35 100644 --- a/config.example.yml +++ b/config.example.yml @@ -4,7 +4,10 @@ # 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. +# 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 -- 2.54.0