From fdcf1a3c61706579bc2ffc15f1b6cee1152f5439 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 26 Oct 2026 20:49:38 +0000 Subject: [PATCH] Load every setting from one table in internal/config (closes #320) Config.settings lists each setting once: its key, its environment variable and the function that loads it. Most rows load through field, whose arguments give the type, the default and the check. db_url and cache_max_bytes keep their own load functions. Loading, the unknown-key and unknown-variable checks and the names in error messages all read the table. allowlist_hosts now loads through parseHostList, as referer_blocklist does. isKnownConfigKey, envVarNames, strictLoader, validate, validateAllowlistHostsValue and getStringSlice are gone, with the getStringSlice tests. The key constants move to the tests, their only users. A new test checks that README.md and configs/config.example.yml name the keys of the table. Model: opus-5-5 --- TODO.md | 23 + internal/config/config.go | 927 +++++++----------- internal/config/config_internal_test.go | 85 -- .../config/config_validation_internal_test.go | 30 +- .../documented_settings_internal_test.go | 108 ++ internal/config/env_internal_test.go | 7 +- 6 files changed, 507 insertions(+), 673 deletions(-) create mode 100644 internal/config/documented_settings_internal_test.go diff --git a/TODO.md b/TODO.md index 39e0f30..74f269d 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,29 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-26 one table in `internal/config` lists every setting once (closes + #320). `Config.settings` gives each setting's key, its environment variable + and the function that loads it. + - Most rows load through `field`, whose arguments give the setting's type, + its default and its check. + - `db_url` and `cache_max_bytes` have their own load functions. The default + of `db_url` comes from `state_dir`, and `cache_max_bytes` records whether + it is set. + - Loading, the unknown-key check, the unknown-variable check and the names + in error messages all read the table. + - `allowlist_hosts` loads as `referer_blocklist` does, through + `parseHostList`. + - The metrics username and password are checked together once the table has + loaded. + - When a config holds more than one invalid value, the error names the one + that comes first in the table. + - A test checks that `README.md` and `configs/config.example.yml` name the + keys of the table. + - Removed: + - the key constants + - `isKnownConfigKey`, `envVarNames`, `strictLoader` and `validate` + - `validateAllowlistHostsValue` and `getStringSlice`, with the three + tests of `getStringSlice` - 2026-10-26 a fetched image's format is detected from its bytes once, as an `imagerequest.Format` (closes #319). The upstream `Content-Type` check in the fetcher stays as the filter before downloading. diff --git a/internal/config/config.go b/internal/config/config.go index 3f484b7..2f433c7 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -76,34 +76,6 @@ func DefaultCacheMaxBytes(freeBytes uint64, heldBytes int64) int64 { return max(int64(computed), DefaultCacheMaxBytesFloor) } -// Configuration key names. -const ( - keyDebug = "debug" - keyMaintenanceMode = "maintenance_mode" - keyPort = "port" - keyStateDir = "state_dir" - keySentryDSN = "sentry_dsn" - keyDBURL = "db_url" - keyMetrics = "metrics" - keyMetricsUsername = "metrics.username" - keyMetricsPassword = "metrics.password" - keySigningKey = "signing_key" - keyAllowlistHosts = "allowlist_hosts" - keyRefererBlocklist = "referer_blocklist" - keyAllowHTTP = "allow_http" - keyUpstreamConnectionsPerHost = "upstream_connections_per_host" - keyUpstreamConnections = "upstream_connections" - keyMaxConcurrentProcessing = "max_concurrent_processing" - keyCacheMaxBytes = "cache_max_bytes" - keyBlockedNetworks = "blocked_networks" - keyTrustedProxies = "trusted_proxies" - keyAccessControlAllowOrigin = "access_control_allow_origin" - keyUpstreamFetchTimeout = "upstream_fetch_timeout" - keyUpstreamMaxResponseSize = "upstream_max_response_size" - keyDownstreamTimeout = "downstream_timeout" - keyImageRequestsPerMinute = "image_requests_per_minute" -) - // placeholderSigningKey is the dummy signing_key shipped in // configs/config.example.yml. It is 45 characters, so it passes the length // check, but it is public in this repository and must be rejected at @@ -123,7 +95,7 @@ var ( errNotAStringList = errors.New("not a list of strings") errNotAValidCIDR = errors.New("not a valid CIDR network") errMappedTooShort = errors.New("in the IPv4-mapped form but shorter than /96") - errNotAMetricsMap = errors.New("not a map of metrics settings") + errNotAMap = errors.New("not a map") errEmptyListEntry = errors.New("list contains an empty entry") errEmptyEntry = errors.New("contains an empty entry") errNotAValidURL = errors.New("not a valid URL") @@ -284,7 +256,7 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { // 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 +// config file was found, in which case every setting 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. @@ -294,93 +266,171 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { return nil, err } - err = validateAllowlistHostsValue(sc) - if err != nil { - return nil, err - } + c := &Config{} - blockedNetworks, err := parseCIDRList(sc, keyBlockedNetworks) - if err != nil { - return nil, err - } - - trustedProxies, err := parseCIDRList(sc, keyTrustedProxies) - if err != nil { - return nil, err - } - - // parseCIDRList returns a nil slice only when the key is absent; an - // explicitly empty list ([]) comes back non-nil and empty. An omitted - // key takes the RFC 1918 default, while an explicit empty list is left - // as trust-nothing. - if trustedProxies == nil { - trustedProxies = defaultTrustedProxies() - } - - loader := &strictLoader{sc: sc} - c := &Config{ - Debug: loader.boolVal(keyDebug, false), - MaintenanceMode: loader.boolVal(keyMaintenanceMode, false), - Port: loader.intVal(keyPort, DefaultPort), - StateDir: loader.stringVal(keyStateDir, DefaultStateDir), - SentryDSN: loader.stringVal(keySentryDSN, ""), - MetricsUsername: loader.stringVal(keyMetricsUsername, ""), - MetricsPassword: loader.stringVal(keyMetricsPassword, ""), - SigningKey: loader.stringVal(keySigningKey, ""), - AllowlistHosts: getStringSlice(sc), - AllowHTTP: loader.boolVal(keyAllowHTTP, false), - UpstreamConnectionsPerHost: loader.intVal( - keyUpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost), - UpstreamConnections: loader.intVal( - keyUpstreamConnections, DefaultUpstreamConnections), - MaxConcurrentProcessing: loader.intVal( - keyMaxConcurrentProcessing, DefaultMaxConcurrentProcessing()), - UpstreamFetchTimeout: loader.durationVal( - keyUpstreamFetchTimeout, DefaultUpstreamFetchTimeout), - UpstreamMaxResponseSize: loader.int64Val( - keyUpstreamMaxResponseSize, DefaultUpstreamMaxResponseSize), - AccessControlAllowOrigin: loader.stringVal( - keyAccessControlAllowOrigin, DefaultAccessControlAllowOrigin), - DownstreamTimeout: loader.durationVal( - keyDownstreamTimeout, DefaultDownstreamTimeout), - ImageRequestsPerMinute: loader.intVal( - keyImageRequestsPerMinute, DefaultImageRequestsPerMinute), - CacheMaxBytes: loader.int64Val(keyCacheMaxBytes, 0), - BlockedNetworks: blockedNetworks, - TrustedProxies: trustedProxies, - RefererBlocklist: loader.hostListVal(keyRefererBlocklist), - } - - // The default for an omitted cache_max_bytes is worked out when - // pixad builds the cache; here we only record whether the operator - // set the key explicitly. - _, c.CacheMaxBytesExplicit = lookupValue(sc, keyCacheMaxBytes) - - // Build DBURL from StateDir if not explicitly set. The derived URL - // is a default: it applies only when db_url is omitted, never to an - // explicitly empty value. - c.DBURL = loader.stringVal(keyDBURL, "") - if c.DBURL == "" && loader.err == nil { - if _, present := lookupValue(sc, keyDBURL); present { - return nil, fmt.Errorf("%s: %w; omit it to derive it from state_dir", - settingName(keyDBURL), errValueEmpty) + for _, s := range c.settings() { + err := s.load(lookupValue(sc, s)) + if err != nil { + return nil, err } - - c.DBURL = defaultDBURL(c.StateDir) } - if loader.err != nil { - return nil, loader.err - } - - err = c.validate() - if err != nil { - return nil, err + // The metrics username and password are checked together, once both + // have loaded. + if (c.MetricsUsername == "") != (c.MetricsPassword == "") { + return nil, fmt.Errorf("%s and %s %w", + settingName("metrics.username"), settingName("metrics.password"), + errMustBeSetTogether) } return c, nil } +// A setting is one row of the settings table. key is its key in the +// config file, and envVar is the environment variable that also sets it. +// load reads the value set for it into its Config field. +type setting struct { + key string + envVar string + load func(v value) error +} + +// A value is what the environment or the config file sets for one +// setting. present is false when neither sets it. +type value struct { + key string + raw any + present bool +} + +// settings returns the settings table: every setting pixa reads, once +// each, in the order they load into c. Most rows fill their Config field +// with field, whose arguments give the setting's type, its default and +// its check. Code that needs only the keys and the variables reads the +// table of a new Config. +func (c *Config) settings() []setting { + return []setting{ + {"signing_key", "PIXA_SIGNING_KEY", + field(&c.SigningKey, getString, "", validateSigningKey)}, + {"port", "PORT", field(&c.Port, getInt, DefaultPort, validatePort)}, + {"state_dir", "PIXA_STATE_DIR", + field(&c.StateDir, getString, DefaultStateDir, validateNotEmpty)}, + {"db_url", "PIXA_DB_URL", c.loadDBURL}, + {"cache_max_bytes", "PIXA_CACHE_MAX_BYTES", c.loadCacheMaxBytes}, + {"allowlist_hosts", "PIXA_ALLOWLIST_HOSTS", + field(&c.AllowlistHosts, parseHostList, nil, nil)}, + {"referer_blocklist", "PIXA_REFERER_BLOCKLIST", + field(&c.RefererBlocklist, parseHostList, nil, nil)}, + {"blocked_networks", "PIXA_BLOCKED_NETWORKS", + field(&c.BlockedNetworks, parseCIDRList, nil, nil)}, + {"trusted_proxies", "PIXA_TRUSTED_PROXIES", + field(&c.TrustedProxies, parseCIDRList, defaultTrustedProxies(), nil)}, + {"allow_http", "PIXA_ALLOW_HTTP", field(&c.AllowHTTP, getBool, false, nil)}, + {"upstream_connections_per_host", "PIXA_UPSTREAM_CONNECTIONS_PER_HOST", + field(&c.UpstreamConnectionsPerHost, getInt, + DefaultUpstreamConnectionsPerHost, validateAtLeastOne)}, + {"upstream_connections", "PIXA_UPSTREAM_CONNECTIONS", + field(&c.UpstreamConnections, getInt, + DefaultUpstreamConnections, validateAtLeastOne)}, + {"max_concurrent_processing", "PIXA_MAX_CONCURRENT_PROCESSING", + field(&c.MaxConcurrentProcessing, getInt, + DefaultMaxConcurrentProcessing(), validateAtLeastOne)}, + {"image_requests_per_minute", "PIXA_IMAGE_REQUESTS_PER_MINUTE", + field(&c.ImageRequestsPerMinute, getInt, + DefaultImageRequestsPerMinute, validateNotNegative)}, + {"upstream_fetch_timeout", "PIXA_UPSTREAM_FETCH_TIMEOUT", + field(&c.UpstreamFetchTimeout, getDuration, + DefaultUpstreamFetchTimeout, nil)}, + {"upstream_max_response_size", "PIXA_UPSTREAM_MAX_RESPONSE_SIZE", + field(&c.UpstreamMaxResponseSize, getInt64, + DefaultUpstreamMaxResponseSize, validateUpstreamMaxResponseSize)}, + {"downstream_timeout", "PIXA_DOWNSTREAM_TIMEOUT", + field(&c.DownstreamTimeout, getDuration, + DefaultDownstreamTimeout, nil)}, + {"access_control_allow_origin", "PIXA_ACCESS_CONTROL_ALLOW_ORIGIN", + field(&c.AccessControlAllowOrigin, getString, + DefaultAccessControlAllowOrigin, validateAccessControlAllowOrigin)}, + {"metrics.username", "PIXA_METRICS_USERNAME", + field(&c.MetricsUsername, getString, "", nil)}, + {"metrics.password", "PIXA_METRICS_PASSWORD", + field(&c.MetricsPassword, getString, "", nil)}, + {"sentry_dsn", "PIXA_SENTRY_DSN", + field(&c.SentryDSN, getString, "", validateSentryDSN)}, + {"debug", "PIXA_DEBUG", field(&c.Debug, getBool, false, nil)}, + {"maintenance_mode", "PIXA_MAINTENANCE_MODE", + field(&c.MaintenanceMode, getBool, false, nil)}, + } +} + +// field returns the load function of a setting that fills *dst. read +// reads a value that is set, and so gives the setting's type. A setting +// that is not set takes defaultValue. The value, a default too, must +// then pass check, unless check is nil. +func field[T any]( + dst *T, + read func(v value) (T, error), + defaultValue T, + check func(key string, val T) error, +) func(v value) error { + return func(v value) error { + val := defaultValue + + if v.present { + var err error + + val, err = read(v) + if err != nil { + return err + } + } + + if check != nil { + err := check(v.key, val) + if err != nil { + return err + } + } + + *dst = val + + return nil + } +} + +// loadDBURL loads db_url. An omitted db_url is derived from state_dir, +// so db_url comes after state_dir in the table. An empty db_url is +// refused, since defaults apply only to omitted keys. +func (c *Config) loadDBURL(v value) error { + if !v.present { + c.DBURL = defaultDBURL(c.StateDir) + + return nil + } + + dbURL, err := getString(v) + if err != nil { + return err + } + + if dbURL == "" { + return fmt.Errorf("%s: %w; omit it to derive it from state_dir", + settingName(v.key), errValueEmpty) + } + + c.DBURL = dbURL + + return nil +} + +// loadCacheMaxBytes loads cache_max_bytes, and records whether it is set. +// For an omitted cache_max_bytes pixad works out DefaultCacheMaxBytes +// when it builds the cache. +func (c *Config) loadCacheMaxBytes(v value) error { + c.CacheMaxBytesExplicit = v.present + + return field(&c.CacheMaxBytes, getInt64, 0, validateNotNegative)(v) +} + // defaultDBURL returns the db_url used when db_url is omitted: the file // state.sqlite3 in stateDir, in WAL mode, which the driver sets only // through a _pragma parameter. String() percent-encodes any %, ? or # in @@ -409,50 +459,65 @@ func defaultDBURL(stateDir string) string { // application does not understand, so typos fail at startup instead of // being silently ignored, and rejects keys that are explicitly set to // null: a null is a SET value, never an omission, so it must not -// silently take the default. The env section is permitted because -// smartconfig consumes it for environment variable injection. A nil sc -// means no config file, which has no keys to check. +// silently take the default. A setting whose key has a dot, such as +// metrics.username, is written inside the map of the key's first part. +// The env section is permitted because smartconfig consumes it for +// environment variable injection. A nil sc means no config file, which +// has no keys to check. func validateKnownKeys(sc *smartconfig.Config) error { if sc == nil { return nil } + topLevelKeys := map[string]bool{"env": true} + mapKeys := map[string]bool{} + keysInMaps := map[string]bool{} + + for _, s := range new(Config).settings() { + mapKey, _, inMap := strings.Cut(s.key, ".") + topLevelKeys[mapKey] = true + + if inMap { + mapKeys[mapKey] = true + keysInMaps[s.key] = true + } + } + var unknown, nullKeys []string - for key, value := range sc.Data() { - if !isKnownConfigKey(key) { + for key, val := range sc.Data() { + switch { + case !topLevelKeys[key]: unknown = append(unknown, key) - - continue - } - - if value == nil { + case val == nil: nullKeys = append(nullKeys, key) - - continue - } - - if key == keyMetrics { - metricsMap, ok := value.(map[string]any) + case mapKeys[key]: + settingsMap, ok := val.(map[string]any) if !ok { - return fmt.Errorf("config key %q: value %v is %w", - keyMetrics, value, errNotAMetricsMap) + return fmt.Errorf("config key %q: value %v is %w of %s settings", + key, val, errNotAMap, key) } - for subkey, subvalue := range metricsMap { - if subkey != "username" && subkey != "password" { - unknown = append(unknown, keyMetrics+"."+subkey) + for subkey, subvalue := range settingsMap { + fullKey := key + "." + subkey - continue - } - - if subvalue == nil { - nullKeys = append(nullKeys, keyMetrics+"."+subkey) + switch { + case !keysInMaps[fullKey]: + unknown = append(unknown, fullKey) + case subvalue == nil: + nullKeys = append(nullKeys, fullKey) } } } } + return unknownOrNullKeysError(unknown, nullKeys) +} + +// unknownOrNullKeysError reports the unknown keys validateKnownKeys +// found, or else the keys set to null. It returns nil when there are +// neither. +func unknownOrNullKeysError(unknown, nullKeys []string) error { if len(unknown) > 0 { sort.Strings(unknown) @@ -480,55 +545,6 @@ func errNullConfigValue(key string) error { return fmt.Errorf("config key %q: %w", key, errValueNull) } -// isKnownConfigKey reports whether key is a permitted top-level -// configuration key. -func isKnownConfigKey(key string) bool { - switch key { - case keyDebug, keyMaintenanceMode, keyPort, keyStateDir, keySentryDSN, - keyDBURL, keyMetrics, keySigningKey, keyAllowlistHosts, keyAllowHTTP, - keyUpstreamConnectionsPerHost, keyUpstreamConnections, - keyMaxConcurrentProcessing, keyCacheMaxBytes, keyBlockedNetworks, - keyTrustedProxies, keyAccessControlAllowOrigin, keyUpstreamFetchTimeout, - keyUpstreamMaxResponseSize, keyDownstreamTimeout, keyRefererBlocklist, - keyImageRequestsPerMinute, "env": - return true - } - - 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", - keyRefererBlocklist: "PIXA_REFERER_BLOCKLIST", - keyAllowHTTP: "PIXA_ALLOW_HTTP", - keyUpstreamConnectionsPerHost: "PIXA_UPSTREAM_CONNECTIONS_PER_HOST", - keyUpstreamConnections: "PIXA_UPSTREAM_CONNECTIONS", - keyMaxConcurrentProcessing: "PIXA_MAX_CONCURRENT_PROCESSING", - keyCacheMaxBytes: "PIXA_CACHE_MAX_BYTES", - keyBlockedNetworks: "PIXA_BLOCKED_NETWORKS", - keyTrustedProxies: "PIXA_TRUSTED_PROXIES", - keyAccessControlAllowOrigin: "PIXA_ACCESS_CONTROL_ALLOW_ORIGIN", - keyUpstreamFetchTimeout: "PIXA_UPSTREAM_FETCH_TIMEOUT", - keyUpstreamMaxResponseSize: "PIXA_UPSTREAM_MAX_RESPONSE_SIZE", - keyDownstreamTimeout: "PIXA_DOWNSTREAM_TIMEOUT", - keyImageRequestsPerMinute: "PIXA_IMAGE_REQUESTS_PER_MINUTE", - } -} - // validateKnownEnvVars rejects environment variables whose names start // with PIXA_ but that are neither a setting's variable nor // PIXA_CONFIG_PATH, so a misspelled variable fails at startup instead of @@ -538,8 +554,8 @@ func envVarNames() map[string]string { func validateKnownEnvVars() error { known := map[string]bool{"PIXA_CONFIG_PATH": true} - for _, name := range envVarNames() { - known[name] = true + for _, s := range new(Config).settings() { + known[s.envVar] = true } var unknown []string @@ -566,28 +582,37 @@ func validateKnownEnvVars() error { return nil } -// 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]) +// lookupValue returns the value set for s. Its 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, s setting) value { + envValue, present := os.LookupEnv(s.envVar) if present { - return value, true + return value{key: s.key, raw: envValue, present: true} } if sc == nil { - return nil, false + return value{key: s.key} } - return sc.Get(key) + fileValue, present := sc.Get(s.key) + + return value{key: s.key, raw: fileValue, present: present} } -// settingName names key in an error message together with its -// environment variable, since either one may have set the value. +// settingName names the setting 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]) + var envVar string + + for _, s := range new(Config).settings() { + if s.key == key { + envVar = s.envVar + } + } + + return fmt.Sprintf("config key %q (environment variable %s)", key, envVar) } // ensureStateDirWritable verifies at startup that StateDir can be @@ -596,16 +621,18 @@ func settingName(key string) string { func (c *Config) ensureStateDirWritable() error { const stateDirPerms = 0o750 + name := settingName("state_dir") + err := os.MkdirAll(c.StateDir, stateDirPerms) if err != nil { return fmt.Errorf("%s: cannot create directory %q: %w", - settingName(keyStateDir), c.StateDir, err) + name, c.StateDir, err) } probe, err := os.CreateTemp(c.StateDir, ".startup-write-probe-*") if err != nil { return fmt.Errorf("%s: directory %q is not writable: %w", - settingName(keyStateDir), c.StateDir, err) + name, c.StateDir, err) } probePath := probe.Name() @@ -613,13 +640,13 @@ func (c *Config) ensureStateDirWritable() error { err = probe.Close() if err != nil { return fmt.Errorf("%s: cannot close probe file %q: %w", - settingName(keyStateDir), probePath, err) + name, probePath, err) } err = os.Remove(probePath) if err != nil { return fmt.Errorf("%s: cannot remove probe file %q: %w", - settingName(keyStateDir), probePath, err) + name, probePath, err) } return nil @@ -628,101 +655,79 @@ func (c *Config) ensureStateDirWritable() error { // validateSigningKey checks that the signing key is present, long // enough, and not the public placeholder from configs/config.example.yml. // The key value itself is never echoed in error messages. -func (c *Config) validateSigningKey() error { - if c.SigningKey == "" { - return fmt.Errorf("%s: %w", settingName(keySigningKey), errValueRequired) +func validateSigningKey(key, signingKey string) error { + if signingKey == "" { + return fmt.Errorf("%s: %w", settingName(key), errValueRequired) } // Minimum key length for security (32 bytes = 256 bits) const minKeyLength = 32 - if len(c.SigningKey) < minKeyLength { + if len(signingKey) < minKeyLength { return fmt.Errorf("%s: %w: must be at least %d characters, got %d", - settingName(keySigningKey), errValueTooShort, minKeyLength, - len(c.SigningKey)) + settingName(key), errValueTooShort, minKeyLength, len(signingKey)) } - if c.SigningKey == placeholderSigningKey { - return fmt.Errorf("%s: %w", settingName(keySigningKey), errPlaceholderKey) + if signingKey == placeholderSigningKey { + return fmt.Errorf("%s: %w", settingName(key), errPlaceholderKey) } return nil } -// validate checks that all required configuration values are set and -// that every value is within its valid range. -func (c *Config) validate() error { - err := c.validateSigningKey() - if err != nil { - return err - } - +// validatePort checks that a port is from 1 to 65535. +func validatePort(key string, port int) error { const maxPort = 65535 - if c.Port < 1 || c.Port > maxPort { + if port < 1 || port > maxPort { return fmt.Errorf("%s: value %d is %w 1-%d", - settingName(keyPort), c.Port, errPortOutOfRange, maxPort) + settingName(key), port, errPortOutOfRange, maxPort) } - err = c.validateConcurrencyLimits() - if err != nil { - return err + return nil +} + +// validateNotEmpty checks that a value is not empty. +func validateNotEmpty(key, val string) error { + if val == "" { + return fmt.Errorf("%s: %w", settingName(key), errValueEmpty) } - if c.StateDir == "" { - return fmt.Errorf("%s: %w", settingName(keyStateDir), errValueEmpty) - } + return nil +} - // Zero is valid (it disables the disk cache); only negative - // values are rejected. No floor applies to explicit values. - if c.CacheMaxBytes < 0 { +// validateAtLeastOne checks that a limit, such as the most images +// processed at once, is at least 1. +func validateAtLeastOne(key string, limit int) error { + if limit < 1 { return fmt.Errorf("%s: value %d %w", - settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative) + settingName(key), limit, errMustBeAtLeastOne) } - // Zero is valid (it turns the limit off); only negative values are - // rejected. - if c.ImageRequestsPerMinute < 0 { + return nil +} + +// validateNotNegative checks that a number is not negative. Zero is valid +// for the two settings that use it: it turns the disk cache or the limit +// on image requests off. +func validateNotNegative[T int | int64](key string, number T) error { + if number < 0 { return fmt.Errorf("%s: value %d %w", - settingName(keyImageRequestsPerMinute), c.ImageRequestsPerMinute, - errMustNotBeNegative) + settingName(key), number, errMustNotBeNegative) } - err = c.validateUpstreamMaxResponseSize() - if err != nil { - return err - } - - for _, host := range c.AllowlistHosts { - err := validateHostPattern(keyAllowlistHosts, host) - if err != nil { - return err - } - } - - err = c.validateSentryDSN() - if err != nil { - return err - } - - if (c.MetricsUsername == "") != (c.MetricsPassword == "") { - return fmt.Errorf("%s and %s %w", - settingName(keyMetricsUsername), settingName(keyMetricsPassword), - errMustBeSetTogether) - } - - return c.validateAccessControlAllowOrigin() + return nil } // validateSentryDSN checks that sentry_dsn, when set, is a URL with a scheme // and a host. -func (c *Config) validateSentryDSN() error { - if c.SentryDSN == "" { +func validateSentryDSN(key, dsn string) error { + if dsn == "" { return nil } - parsed, err := url.Parse(c.SentryDSN) + parsed, err := url.Parse(dsn) if err != nil || parsed.Scheme == "" || parsed.Host == "" { return fmt.Errorf("%s: value %q is %w", - settingName(keySentryDSN), c.SentryDSN, errNotAValidURL) + settingName(key), dsn, errNotAValidURL) } return nil @@ -732,13 +737,11 @@ func (c *Config) validateSentryDSN() error { // is from 1 byte to 1 GiB. An upstream response is read whole into // memory, and the image processor reads one byte past this limit, which // must not overflow. -func (c *Config) validateUpstreamMaxResponseSize() error { +func validateUpstreamMaxResponseSize(key string, size int64) error { const maxUpstreamMaxResponseSize = 1 << 30 // 1 GiB - if c.UpstreamMaxResponseSize < 1 || - c.UpstreamMaxResponseSize > maxUpstreamMaxResponseSize { + if size < 1 || size > maxUpstreamMaxResponseSize { return fmt.Errorf("%s: value %d is %w 1-%d", - settingName(keyUpstreamMaxResponseSize), c.UpstreamMaxResponseSize, - errSizeOutOfRange, maxUpstreamMaxResponseSize) + settingName(key), size, errSizeOutOfRange, maxUpstreamMaxResponseSize) } return nil @@ -747,14 +750,13 @@ func (c *Config) validateUpstreamMaxResponseSize() error { // validateAccessControlAllowOrigin accepts "*" or an origin exactly as a browser // sends it: http or https, an IP address as netip writes it or a lowercase name // with a letter in its last part, and an optional port 1-65535, not the default. -func (c *Config) validateAccessControlAllowOrigin() error { - origin := c.AccessControlAllowOrigin +func validateAccessControlAllowOrigin(key, origin string) error { if origin == "*" { return nil } errOrigin := fmt.Errorf("%s: value %q is %w", - settingName(keyAccessControlAllowOrigin), origin, errNotAnOrigin) + settingName(key), origin, errNotAnOrigin) parsed, err := url.Parse(origin) if err != nil { @@ -804,30 +806,6 @@ func (c *Config) validateAccessControlAllowOrigin() error { return nil } -// validateConcurrencyLimits checks that the two upstream connection limits -// and the image processing limit are at least 1. -func (c *Config) validateConcurrencyLimits() error { - if c.UpstreamConnectionsPerHost < 1 { - return fmt.Errorf("%s: value %d %w", - settingName(keyUpstreamConnectionsPerHost), - c.UpstreamConnectionsPerHost, errMustBeAtLeastOne) - } - - if c.UpstreamConnections < 1 { - return fmt.Errorf("%s: value %d %w", - settingName(keyUpstreamConnections), - c.UpstreamConnections, errMustBeAtLeastOne) - } - - if c.MaxConcurrentProcessing < 1 { - return fmt.Errorf("%s: value %d %w", - settingName(keyMaxConcurrentProcessing), - c.MaxConcurrentProcessing, errMustBeAtLeastOne) - } - - return nil -} - // hostNamePattern matches a host name: letters, digits, hyphens, underscores // and dots, optionally after one leading dot. var hostNamePattern = regexp.MustCompile(`^\.?[A-Za-z0-9_-][A-Za-z0-9_.-]*$`) @@ -903,128 +881,31 @@ func loadConfigFile(log *slog.Logger, appName string) (*smartconfig.Config, erro return nil, nil //nolint:nilnil // nil config is valid (use defaults) } -// strictLoader accumulates the first error encountered while reading -// typed values out of a smartconfig instance, so Config construction -// can stay a single struct literal. -type strictLoader struct { - sc *smartconfig.Config - err error -} - -func (l *strictLoader) stringVal(key, defaultVal string) string { - if l.err != nil { - return "" +// getString returns the string v holds. A value that is not a string, or +// is explicitly null, is an error. +func getString(v value) (string, error) { + if v.raw == nil { + return "", errNullConfigValue(v.key) } - val, err := getString(l.sc, key, defaultVal) - if err != nil { - l.err = err - } - - return val -} - -func (l *strictLoader) intVal(key string, defaultVal int) int { - if l.err != nil { - return 0 - } - - val, err := getInt(l.sc, key, defaultVal) - if err != nil { - l.err = err - } - - return val -} - -func (l *strictLoader) int64Val(key string, defaultVal int64) int64 { - if l.err != nil { - return 0 - } - - val, err := getInt64(l.sc, key, defaultVal) - if err != nil { - l.err = err - } - - return val -} - -func (l *strictLoader) durationVal(key string, defaultVal time.Duration) time.Duration { - if l.err != nil { - return 0 - } - - val, err := getDuration(l.sc, key, defaultVal) - if err != nil { - l.err = err - } - - return val -} - -func (l *strictLoader) boolVal(key string, defaultVal bool) bool { - if l.err != nil { - return false - } - - val, err := getBool(l.sc, key, defaultVal) - if err != nil { - l.err = err - } - - return val -} - -func (l *strictLoader) hostListVal(key string) []string { - if l.err != nil { - return nil - } - - val, err := parseHostList(l.sc, key) - if err != nil { - l.err = err - } - - return val -} - -// getString returns the string value for key, or defaultVal if the key -// 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) { - raw, ok := lookupValue(sc, key) - if !ok { - return defaultVal, nil - } - - if raw == nil { - return "", errNullConfigValue(key) - } - - str, ok := raw.(string) + str, ok := v.raw.(string) if !ok { return "", fmt.Errorf("config key %q: value %v (%T) is %w", - key, raw, raw, errNotAString) + v.key, v.raw, v.raw, errNotAString) } return str, nil } -// getInt returns the integer value for key, or defaultVal if the key is -// 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) { - raw, ok := lookupValue(sc, key) - if !ok { - return defaultVal, nil +// getInt returns the integer v holds. A value that is not a whole +// number, or is explicitly null, is an error; fractional values are +// never truncated. +func getInt(v value) (int, error) { + if v.raw == nil { + return 0, errNullConfigValue(v.key) } - if raw == nil { - return 0, errNullConfigValue(key) - } - - switch val := raw.(type) { + switch val := v.raw.(type) { case int: return val, nil case int64: @@ -1032,7 +913,7 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { case float64: if val != math.Trunc(val) { return 0, fmt.Errorf("config key %q: value %v is %w", - key, val, errNotAnInteger) + v.key, val, errNotAnInteger) } return int(val), nil @@ -1040,31 +921,25 @@ func getInt(sc *smartconfig.Config, key string, defaultVal int) (int, error) { parsed, err := strconv.Atoi(strings.TrimSpace(val)) if err != nil { return 0, fmt.Errorf("%s: value %q is %w", - settingName(key), val, errNotAnInteger) + settingName(v.key), val, errNotAnInteger) } return parsed, nil default: return 0, fmt.Errorf("config key %q: value %v (%T) is %w", - key, raw, raw, errNotAnInteger) + v.key, v.raw, v.raw, errNotAnInteger) } } -// getInt64 returns the 64-bit integer value for key, or defaultVal if -// the key is omitted. A present value that is not a whole number, or -// 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) { - raw, ok := lookupValue(sc, key) - if !ok { - return defaultVal, nil +// getInt64 returns the 64-bit integer v holds. A value that is not a +// whole number, or is explicitly null, is an error; fractional values +// are never truncated and out-of-range values are never clamped. +func getInt64(v value) (int64, error) { + if v.raw == nil { + return 0, errNullConfigValue(v.key) } - if raw == nil { - return 0, errNullConfigValue(key) - } - - switch val := raw.(type) { + switch val := v.raw.(type) { case int: return int64(val), nil case int64: @@ -1072,14 +947,14 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro case uint64: if val > math.MaxInt64 { return 0, fmt.Errorf("config key %q: value %d %w", - key, val, errOverflowsInt64) + v.key, val, errOverflowsInt64) } return int64(val), nil case float64: if val != math.Trunc(val) { return 0, fmt.Errorf("config key %q: value %v is %w", - key, val, errNotAnInteger) + v.key, val, errNotAnInteger) } return int64(val), nil @@ -1087,174 +962,70 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro parsed, err := strconv.ParseInt(strings.TrimSpace(val), 10, 64) if err != nil { return 0, fmt.Errorf("%s: value %q is %w", - settingName(key), val, errNotAnInteger) + settingName(v.key), val, errNotAnInteger) } return parsed, nil default: return 0, fmt.Errorf("config key %q: value %v (%T) is %w", - key, raw, raw, errNotAnInteger) + v.key, v.raw, v.raw, errNotAnInteger) } } -// getDuration returns the duration value for key, or defaultVal if the -// key is omitted. A present value must be a positive Go duration string -// such as "30s" or "2m", read with time.ParseDuration; a bare number has -// no unit and is an error, as is an explicit null. -func getDuration( - sc *smartconfig.Config, key string, defaultVal time.Duration, -) (time.Duration, error) { - raw, ok := lookupValue(sc, key) - if !ok { - return defaultVal, nil +// getDuration returns the duration v holds. The value must be a positive +// Go duration string such as "30s" or "2m", read with +// time.ParseDuration; a bare number has no unit and is an error, as is +// an explicit null. +func getDuration(v value) (time.Duration, error) { + if v.raw == nil { + return 0, errNullConfigValue(v.key) } - if raw == nil { - return 0, errNullConfigValue(key) - } - - str, ok := raw.(string) + str, ok := v.raw.(string) if !ok { return 0, fmt.Errorf("config key %q: value %v (%T) is %w", - key, raw, raw, errNotADuration) + v.key, v.raw, v.raw, errNotADuration) } parsed, err := time.ParseDuration(strings.TrimSpace(str)) if err != nil { return 0, fmt.Errorf("%s: value %q is %w", - settingName(key), str, errNotADuration) + settingName(v.key), str, errNotADuration) } if parsed <= 0 { return 0, fmt.Errorf("%s: value %q %w", - settingName(key), str, errMustBePositive) + settingName(v.key), str, errMustBePositive) } return parsed, nil } -// getBool returns the boolean value for key, or defaultVal if the key -// is omitted. A present value that is not a boolean (or a ParseBool-able -// 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) { - raw, ok := lookupValue(sc, key) - if !ok { - return defaultVal, nil +// getBool returns the boolean v holds. A value that is not a boolean (or +// a ParseBool-able string), or is explicitly null, is an error; numbers +// are not accepted as booleans. +func getBool(v value) (bool, error) { + if v.raw == nil { + return false, errNullConfigValue(v.key) } - if raw == nil { - return false, errNullConfigValue(key) - } - - switch val := raw.(type) { + switch val := v.raw.(type) { case bool: return val, nil case string: parsed, err := strconv.ParseBool(strings.TrimSpace(val)) if err != nil { return false, fmt.Errorf("%s: value %q is %w", - settingName(key), val, errNotABoolean) + settingName(v.key), val, errNotABoolean) } return parsed, nil default: return false, fmt.Errorf("config key %q: value %v (%T) is %w", - key, raw, raw, errNotABoolean) + v.key, v.raw, v.raw, errNotABoolean) } } -// validateAllowlistHostsValue checks the raw shape of the -// allowlist_hosts value before the lenient extraction in getStringSlice -// runs: an explicitly null value, a value that is not a list of strings -// (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 := lookupValue(sc, keyAllowlistHosts) - if !ok { - return nil - } - - if raw == nil { - return errNullConfigValue(keyAllowlistHosts) - } - - switch val := raw.(type) { - case []any: - for _, item := range val { - str, ok := item.(string) - if !ok { - return fmt.Errorf("config key %q: list entry %v (%T) is %w", - keyAllowlistHosts, item, item, errNotAString) - } - - if strings.TrimSpace(str) == "" { - return fmt.Errorf("config key %q: %w", - keyAllowlistHosts, errEmptyListEntry) - } - } - case string: - if strings.TrimSpace(val) == "" { - return nil - } - - for part := range strings.SplitSeq(val, ",") { - if strings.TrimSpace(part) == "" { - return fmt.Errorf("%s: value %q %w", - settingName(keyAllowlistHosts), val, errEmptyEntry) - } - } - default: - return fmt.Errorf("config key %q: value %v (%T) is %w", - keyAllowlistHosts, raw, raw, errNotAStringList) - } - - return nil -} - -// getStringSlice returns the allowlist_hosts list of strings, or nil if -// the key is omitted. It accepts a YAML list of strings or a -// comma-separated string (backwards compatibility). Malformed entries -// are rejected beforehand by validateAllowlistHostsValue. -func getStringSlice(sc *smartconfig.Config) []string { - val, ok := lookupValue(sc, keyAllowlistHosts) - if !ok || val == nil { - return nil - } - - // Handle YAML list format - if slice, ok := val.([]any); ok { - result := make([]string, 0, len(slice)) - for _, item := range slice { - if str, ok := item.(string); ok { - trimmed := strings.TrimSpace(str) - if trimmed != "" { - result = append(result, trimmed) - } - } - } - - return result - } - - // Fall back to comma-separated string for backwards compatibility - if str, ok := val.(string); ok && str != "" { - parts := strings.Split(str, ",") - result := make([]string, 0, len(parts)) - - for _, part := range parts { - trimmed := strings.TrimSpace(part) - if trimmed != "" { - result = append(result, trimmed) - } - } - - return result - } - - return nil -} - // defaultTrustedProxies returns the trusted_proxies default: the three RFC // 1918 private ranges. pixa is always deployed behind a TLS-terminating // reverse proxy, which in practice sits on a private network, so its @@ -1270,12 +1041,10 @@ func defaultTrustedProxies() []netip.Prefix { } } -// parseCIDRList parses the value of the named config key into CIDR -// prefixes, or returns nil if the key is omitted. It accepts a YAML list -// of strings or a comma-separated string. An explicitly null value, a -// wrong type, an empty entry, a non-string entry, or an unparseable CIDR -// aborts startup naming the key and the offending value; the default -// (an empty list) applies only to an omitted key. +// parseCIDRList parses the list v holds into CIDR prefixes. It accepts a +// YAML list of strings or a comma-separated string. An explicitly null +// value, a wrong type, an empty entry, a non-string entry, or an +// unparseable CIDR aborts startup naming the key and the offending value. // // An entry written in the IPv4-mapped form (::ffff:a.b.c.d) is kept as the // IPv4 range it covers: ::ffff:10.0.0.0/104 is kept as 10.0.0.0/8. Both @@ -1283,17 +1052,12 @@ func defaultTrustedProxies() []netip.Prefix { // address as the IPv4 address it holds, so the entry as written would match // nothing. Such an entry shorter than /96 covers more than the IPv4 // addresses and aborts startup. -func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) { - raw, ok := lookupValue(sc, key) - if !ok { - return nil, nil +func parseCIDRList(v value) ([]netip.Prefix, error) { + if v.raw == nil { + return nil, errNullConfigValue(v.key) } - if raw == nil { - return nil, errNullConfigValue(key) - } - - entries, err := listEntries(raw, key) + entries, err := listEntries(v.raw, v.key) if err != nil { return nil, err } @@ -1308,12 +1072,12 @@ func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) { prefix, err := netip.ParsePrefix(entry) if err != nil { return nil, fmt.Errorf("%s: value %q is %w", - settingName(key), entry, errNotAValidCIDR) + settingName(v.key), entry, errNotAValidCIDR) } if prefix.Addr().Is4In6() && prefix.Bits() < mappedPrefixBits { return nil, fmt.Errorf("%s: value %q is %w", - settingName(key), entry, errMappedTooShort) + settingName(v.key), entry, errMappedTooShort) } if prefix.Addr().Is4In6() { @@ -1327,28 +1091,23 @@ func parseCIDRList(sc *smartconfig.Config, key string) ([]netip.Prefix, error) { return prefixes, nil } -// parseHostList parses the value of the named config key into host patterns, -// or returns nil if the key is omitted. It accepts a YAML list of strings or a -// comma-separated string. An explicitly null value, a wrong type, an empty -// entry, a non-string entry, or an entry validateHostPattern rejects aborts -// startup naming the key and the offending value. -func parseHostList(sc *smartconfig.Config, key string) ([]string, error) { - raw, ok := lookupValue(sc, key) - if !ok { - return nil, nil +// parseHostList parses the list v holds into host patterns. It accepts a +// YAML list of strings or a comma-separated string. An explicitly null +// value, a wrong type, an empty entry, a non-string entry, or an entry +// validateHostPattern rejects aborts startup naming the key and the +// offending value. +func parseHostList(v value) ([]string, error) { + if v.raw == nil { + return nil, errNullConfigValue(v.key) } - if raw == nil { - return nil, errNullConfigValue(key) - } - - entries, err := listEntries(raw, key) + entries, err := listEntries(v.raw, v.key) if err != nil { return nil, err } for _, entry := range entries { - err := validateHostPattern(key, entry) + err := validateHostPattern(v.key, entry) if err != nil { return nil, err } @@ -1359,8 +1118,8 @@ func parseHostList(sc *smartconfig.Config, key string) ([]string, error) { // listEntries extracts the raw entries of the named list key as trimmed, // non-empty strings, from either a YAML list of strings or a comma-separated -// string; an empty string is an empty list, as for allowlist_hosts. Any other -// shape is a configuration error. +// string; an empty string is an empty list. Any other shape is a +// configuration error. func listEntries(raw any, key string) ([]string, error) { switch val := raw.(type) { case []any: diff --git a/internal/config/config_internal_test.go b/internal/config/config_internal_test.go index b655b35..bae94c7 100644 --- a/internal/config/config_internal_test.go +++ b/internal/config/config_internal_test.go @@ -1,97 +1,12 @@ package config import ( - "os" "path/filepath" "testing" "git.eeqj.de/sneak/smartconfig" ) -// writeTestConfig writes yamlContent to a temp config file and returns -// the file path. -func writeTestConfig(t *testing.T, yamlContent string) string { - t.Helper() - - tmpDir := t.TempDir() - configPath := filepath.Join(tmpDir, "config.yml") - - err := os.WriteFile(configPath, []byte(yamlContent), 0o600) - if err != nil { - t.Fatalf("failed to write test config: %v", err) - } - - return configPath -} - -// checkAllowlistHosts loads the config at configPath and asserts that -// getStringSlice returns the three expected hosts. -func checkAllowlistHosts(t *testing.T, configPath string) { - t.Helper() - - sc, err := loadTestConfig(configPath) - if err != nil { - t.Fatalf("failed to load config: %v", err) - } - - hosts := getStringSlice(sc) - - if len(hosts) != 3 { - t.Errorf("expected 3 hosts, got %d: %v", len(hosts), hosts) - } - - expected := []string{"static.sneak.cloud", "sneak.berlin", testHostS3} - for i, want := range expected { - if i >= len(hosts) { - t.Errorf("missing host at index %d: want %q", i, want) - - continue - } - - if hosts[i] != want { - t.Errorf("host[%d] = %q, want %q", i, hosts[i], want) - } - } -} - -func TestGetStringSlice_YAMLList(t *testing.T) { - t.Parallel() - - yamlContent := ` -allowlist_hosts: - - static.sneak.cloud - - sneak.berlin - - s3.sneak.cloud -` - - checkAllowlistHosts(t, writeTestConfig(t, yamlContent)) -} - -func TestGetStringSlice_CommaSeparated(t *testing.T) { - t.Parallel() - - // Backwards compatibility with comma-separated string values. - yamlContent := `allowlist_hosts: "static.sneak.cloud, sneak.berlin, s3.sneak.cloud"` - - checkAllowlistHosts(t, writeTestConfig(t, yamlContent)) -} - -func TestGetStringSlice_Empty(t *testing.T) { - t.Parallel() - - configPath := writeTestConfig(t, `port: 8080`) - - sc, err := loadTestConfig(configPath) - if err != nil { - t.Fatalf("failed to load config: %v", err) - } - - hosts := getStringSlice(sc) - if len(hosts) != 0 { - t.Errorf("expected nil or empty slice, got %v", hosts) - } -} - // TestExampleConfigAllowlistsNoHost loads configs/config.example.yml // with a real signing key, as Getting Started in README.md has it copied // and edited, and checks that it allowlists no host: every image on an diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index 1324050..1aaac63 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -15,8 +15,36 @@ import ( _ "modernc.org/sqlite" // SQLite driver registration ) +// The config keys the tests name in the errors they expect. +const ( + keyDebug = "debug" + keyMaintenanceMode = "maintenance_mode" + keyPort = "port" + keyStateDir = "state_dir" + keySentryDSN = "sentry_dsn" + keyDBURL = "db_url" + keyMetrics = "metrics" + keyMetricsUsername = "metrics.username" + keyMetricsPassword = "metrics.password" + keySigningKey = "signing_key" + keyAllowlistHosts = "allowlist_hosts" + keyRefererBlocklist = "referer_blocklist" + keyAllowHTTP = "allow_http" + keyUpstreamConnectionsPerHost = "upstream_connections_per_host" + keyUpstreamConnections = "upstream_connections" + keyMaxConcurrentProcessing = "max_concurrent_processing" + keyCacheMaxBytes = "cache_max_bytes" + keyBlockedNetworks = "blocked_networks" + keyTrustedProxies = "trusted_proxies" + keyAccessControlAllowOrigin = "access_control_allow_origin" + keyUpstreamFetchTimeout = "upstream_fetch_timeout" + keyUpstreamMaxResponseSize = "upstream_max_response_size" + keyDownstreamTimeout = "downstream_timeout" + keyImageRequestsPerMinute = "image_requests_per_minute" +) + // validTestSigningKey is a 32-character signing key that satisfies the -// minimum length requirement in validate(). +// minimum length requirement in validateSigningKey. const validTestSigningKey = "0123456789abcdef0123456789abcdef" // signingKeyLine is a valid signing_key config line used as the base of diff --git a/internal/config/documented_settings_internal_test.go b/internal/config/documented_settings_internal_test.go new file mode 100644 index 0000000..b19e4ce --- /dev/null +++ b/internal/config/documented_settings_internal_test.go @@ -0,0 +1,108 @@ +package config + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// TestReadmeNamesEverySetting checks that the Configuration section of +// README.md names every key of the settings table in backticks. It also +// checks that every name in backticks there written like a key with an +// underscore, such as state_dir, is a key of the table. The other keys, +// such as port and metrics.username, are checked only the first way. +func TestReadmeNamesEverySetting(t *testing.T) { + t.Parallel() + + readme, err := os.ReadFile(filepath.Join("..", "..", "README.md")) + if err != nil { + t.Fatalf("failed to read README.md: %v", err) + } + + _, section, found := strings.Cut(string(readme), "\n### Configuration\n") + if !found { + t.Fatal("README.md has no Configuration section") + } + + // The section ends at the next heading of its level or above. + section, _, _ = strings.Cut(section, "\n## ") + section, _, _ = strings.Cut(section, "\n### ") + + named := map[string]bool{} + + for _, match := range regexp.MustCompile("`([^`]+)`"). + FindAllStringSubmatch(section, -1) { + named[match[1]] = true + } + + for _, s := range new(Config).settings() { + if !named[s.key] { + t.Errorf("README.md's Configuration section does not name %s", s.key) + } + + delete(named, s.key) + } + + keyWithUnderscore := regexp.MustCompile(`^[a-z]+(_[a-z]+)+$`) + + for name := range named { + if keyWithUnderscore.MatchString(name) { + t.Errorf("README.md's Configuration section names %s, "+ + "which is not a setting", name) + } + } +} + +// TestExampleConfigNamesEverySetting checks that +// configs/config.example.yml names exactly the keys of the settings +// table, each one set or commented out with "# ". A key line holds the +// key, a colon and at most a one-word value, which tells it from a +// comment line that starts with a word and a colon. An indented key is +// inside the map of the key above it, as username is inside metrics. +func TestExampleConfigNamesEverySetting(t *testing.T) { + t.Parallel() + + const examplePath = "configs/config.example.yml" + + example, err := os.ReadFile(filepath.Join("..", "..", examplePath)) + if err != nil { + t.Fatalf("failed to read %s: %v", examplePath, err) + } + + keyLine := regexp.MustCompile(`(?m)^(?:# )?( *)([a-z_]+):(?: \S+)?$`) + named := map[string]bool{} + + var mapKey string + + for _, match := range keyLine.FindAllStringSubmatch(string(example), -1) { + indent, key := match[1], match[2] + if indent == "" { + mapKey = key + named[key] = true + + continue + } + + // The key holding the map is not itself a setting. + delete(named, mapKey) + + named[mapKey+"."+key] = true + } + + // The env section sets environment variables; it is not a setting. + delete(named, "env") + + for _, s := range new(Config).settings() { + if !named[s.key] { + t.Errorf("%s does not name %s", examplePath, s.key) + } + + delete(named, s.key) + } + + for key := range named { + t.Errorf("%s names %s, which is not a setting", examplePath, key) + } +} diff --git a/internal/config/env_internal_test.go b/internal/config/env_internal_test.go index 4b23302..435c825 100644 --- a/internal/config/env_internal_test.go +++ b/internal/config/env_internal_test.go @@ -138,7 +138,8 @@ func TestPixaPortAbortsStartupPointingToPort(t *testing.T) { // TestSettingVariablesAndConfigPathAreAccepted checks that every // setting's variable and PIXA_CONFIG_PATH pass the check for unknown -// PIXA_ variables. TestEnvironmentSetsEveryKey pins the names in the list. +// PIXA_ variables. TestEnvironmentSetsEveryKey pins the names in the +// settings table. func TestSettingVariablesAndConfigPathAreAccepted(t *testing.T) { // A config file's env section loaded by another test can leave a // PIXA_ variable set for the whole process, so every one is unset @@ -159,8 +160,8 @@ func TestSettingVariablesAndConfigPathAreAccepted(t *testing.T) { t.Setenv("PIXA_CONFIG_PATH", "/etc/pixa/config.yml") - for _, name := range envVarNames() { - t.Setenv(name, "") + for _, s := range new(Config).settings() { + t.Setenv(s.envVar, "") } err := validateKnownEnvVars()