Skip to content

bug(config): an explicit false/0 in config.yaml is replaced by the field's env-default #631

Description

@EricAndrechek

Finding (measured): the boot config loader turns an explicit false, 0 or "" in config.yaml back into the field's env-default. With otel.enabled: true and otel.traces.enabled: false in the file, config.Load returns otel.traces.enabled = true. An operator who turns a signal off in YAML gets it back on, and nothing reports it. The same value set through the WH_* env var works.

Repro (measured, main @ 93d8019, cleanenv v1.5.0)

I copied the non-test files of internal/config into a scratch module that uses the repo's go.mod, then called the real config.Load on this file:

settings:
  dir: ./settings
otel:
  enabled: true
  traces:  { enabled: false, sample_rate: 0 }
  metrics: { enabled: false }
  logs:    { enabled: false, sample_rate: 0 }
cache:
  l1_max_cost: 0
server:
  shutdown_timeout: 0
prometheus:
  path: ""
data_dir: ""

With no WH_* env set:

otel.enabled=true traces.enabled=true traces.sample_rate=1 metrics.enabled=true logs.enabled=true logs.sample_rate=1
cache.l1_max_cost=67108864 server.shutdown_timeout=10 prometheus.path="/metrics" data_dir="./data"

otel.enabled: true was read, because its default is false. Every other value that the YAML set to its zero value came back as the default.

With WH_OTEL_TRACES_ENABLED=false WH_OTEL_TRACES_SAMPLE_RATE=0 WH_CACHE_L1_MAX_COST=0 WH_SERVER_SHUTDOWN_TIMEOUT=0 on the same file:

otel.enabled=true traces.enabled=false traces.sample_rate=0 metrics.enabled=true logs.enabled=true logs.sample_rate=1
cache.l1_max_cost=0 server.shutdown_timeout=0 prometheus.path="/metrics" data_dir="./data"

The env vars are honoured, but the fields that only the YAML set still come back as defaults.

Mechanism (inferred from cleanenv v1.5.0's source, and consistent with the measurement above): ReadConfig decodes the YAML first and then runs readEnvVars. For each field with no env var set, it applies env-default if the field is still at its zero value. At that point it cannot tell "the YAML set this to false/0/""" apart from "the YAML did not set this".

Affected fields: every env-default that is not a zero value

field default would an operator want the zero value?
otel.traces.enabled true Yes. Metrics and logs without traces is a normal setup. Measured: silently re-enabled.
otel.metrics.enabled true Yes. For example, when Prometheus scraping is the metrics path and OTLP carries only traces and logs. Measured: silently re-enabled.
otel.logs.enabled true Yes. For example, when stdout is already scraped by Loki. Measured: silently re-enabled.
otel.traces.sample_rate 1.0 Yes. 0 is a valid rate, and Validate accepts [0,1]. Measured: becomes 1.0, so everything is sampled. That is the opposite of what the operator asked for, and it costs money.
otel.logs.sample_rate 1.0 Yes, for the same reason. It drops DEBUG/INFO from OTLP export. Measured: becomes 1.0.
server.shutdown_timeout 10 Plausible. Validate allows 0, which means no drain. Measured: becomes 10.
cache.l1_max_cost 67108864 Unlikely. 0 is not a usable ristretto size. Measured: silently becomes 64 MiB instead of failing.
prometheus.path /metrics No. An empty path is not meaningful. Only the silent rewrite matters here.
data_dir ./data No. Empty would be a misconfiguration. Only the silent rewrite matters here.
server.port 8080 No. 0 fails Validate.

Fields whose default is already the zero value (otel.enabled, prometheus.enabled, prometheus.port, clickhouse.max_total_conns) are not affected.

Impact (inferred): the worst cases are the three OTel signal toggles and the two sample rates. An operator who turns telemetry down in YAML gets full-rate export, with extra egress and backend cost. No error or warning appears, and config.yaml says the opposite of what is running. This will also affect every future field that gets a non-zero env-default. PR #630 has already had to work around it for cache.redis.compress_min_bytes: -1 means "never compress" because a 0 in the file reads as unset. Each new knob would need a similar workaround.

Fix direction

Any of these would work. The first is the smallest change:

  1. Apply defaults before the decode, and drop env-default. Build the Config with its defaults in Go, decode the YAML over it, and then apply WH_* overrides. A key the YAML sets, including to false/0, wins over the default. This also removes the need for PR feat(config): cache.backend=redis selects the shared cache #630's -1 sentinel.
  2. A pre-pass that records which keys the YAML set. rejectUnknownKeys already walks the YAML with yaml.v3. Collect the set paths there, and after cleanenv runs, restore the zero value for any key the file set explicitly.
  3. Pointer fields (*bool, *float64) for the affected knobs, so nil can be told apart from an explicit zero. This is more invasive and spreads nil checks to every consumer.

Whichever fix is chosen, add a regression test that loads a YAML with otel.traces.enabled: false and otel.traces.sample_rate: 0 through Load, and asserts the loaded values rather than a hand-built struct.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions