Skip to content

config: empty string into *bool decodes as non-nil false, defeating tri-states #1110

Description

@gaborage

CACHE_CRITICAL= — a set-but-empty environment variable, an empty secretKeyRef, an envsubst over an unset variable — decodes as a non-nil pointer to false, which turns the cache readiness probe non-critical. /ready then answers 200 through a full Redis outage, and no replica drains from rotation. ADR-046 made the strict posture the default precisely so this cannot happen by omission; an empty string flips it open, silently.

Why the decoder does this

koanf keeps a set-but-empty key, and mapstructure's WeaklyTypedInput fills a bool from "" as false. Because the target is *bool, the result is not the zero value the tri-state reads as "unset" — it is a non-nil pointer, which every downstream reader treats as "the operator chose false":

  • CacheConfig.Critical *bool (config/types.go) — documented "nil = strict, the shipped default" (ADR-046). Config.IsCacheCritical() returns false.
  • PoolKeepAliveConfig.Enabled *bool — a default-true knob that silently turns off.
  • ServerConfig.LogRoutes *bool — same shape, lower stakes.

This is the same defect ADR-074 closed for numeric keys, where KEYSTORE_SECRETMINLENGTH= decoded as *0 and disabled the secret-length floor. The numeric guard deliberately excludes bool: an empty string bound to a bool is not a parse failure the way it is for an int, so rejecting it is a policy choice about what "delivered empty" means for a flag rather than a straightforward decode error — and ADR-074's issue scoped non-numeric targets out explicitly rather than deciding it in passing.

The framework already disagrees with itself here, which is the clearest argument that this needs a decision: InjectInto's convertToBool (config/converters.go) rejects an empty string for a bool target, while the Load decode path accepts it as false. Two public seams, two answers, same input.

What a fix would mirror

configdecode.EmptyStringToNumericGuardHookFunc is the template: a decode hook running ahead of the weak coercion, rejecting the empty (or whitespace-only) string with an error naming the key, wired into every decoder seam (buildDecoderConfig, unmarshalDecoderConfig, migration.decodeSecretConfig, and the CLI's tenantDecoderConfig). Extending it to bool targets is a small change to the predicate; the behaviour change is what needs deciding, since FOO= currently meaning false may be load-bearing in someone's manifest.

Whatever is decided, it is a behaviour change for delivered-empty bools and wants an ADR plus a wiki/migrations.md atom, as ADR-074 / C60.15 did for numerics.

Reproduction

Set CACHE_CRITICAL= (empty) with a cache configured, stop Redis, and poll /ready: 200, with the cache probe reporting unhealthy but non-critical. Unset the variable and the same outage answers 503.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/configAffects the config packagebugSomething isn't workingready-for-agentFully specified, ready for an AFK agent

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions