refactor(config): derive the owned koanf defaults from normalize - #1116
Merged
Conversation
loadDefaults wrote ~55 keys by hand while the normalize phase independently filled ten of them -- app.startup.timeout, the eight cache.redis.* keys, and keystore.secretminlength. The two literals were kept in agreement by hand and pinned by a drift test, which is a rule a future key has to remember rather than a mechanism that holds. loadDefaults is now koanfOnlyDefaults() merged with derivedDefaults(), which normalizes a zero Config, flattens it through the same koanf tags the loader unmarshals with, and picks an explicit allowlist. Durations render via .String() and a *int is dereferenced, so a koanf getter sees the type it saw when these were literals. One mechanism PER KEY, not one mechanism: keys that must FAIL on zero -- app.name, server.port, server.timeout.*, log.level -- stay hand-written, because deriving them would silently turn a required key into a defaulted one. Full derivation is barred for two more reasons: it would hand debug.allowedips' fail-closed posture (ADR-049) to whatever normalize fills, and it would preload the database identity keys, disarming the koanf-presence check ADR-051's delivered-empty rule reads. Tests are the design doc's decision 18, plus a one-shot equivalence pin carrying the pre-change literal values so derivation is proven to relocate the defaults rather than move them: the two maps are disjoint, every allowlisted key is actually filled by normalize, and every allowlisted value is mode-invariant -- which is what bars the manager pool sizes, where multi-tenant reads zero as unlimited. The old drift test is replaced; it now compares derivation against itself. koanf/maps becomes a direct dependency (go.mod), and the design doc's owned-set snapshot is corrected: it predates keystore.secretminlength. No behavior change, so no ADR and no migration atom. Closes #1023
renderDefault returned nil for a nil pointer, which would have written a nil default into the koanf map -- for keystore.secretminlength that is the secret-length floor disappearing quietly. It now names the key and returns an error, and the branch normalize is supposed to make unreachable is pinned by a test rather than left as code that only looks safe. Test scaffolding for the derived-defaults suite is shared through one helper instead of being repeated per test.
Security pass on the derivation. Three things it corrected, two of them my own claims: The nil-pointer guard defended the harmless direction. A nil written into the defaults map still decodes to nil, so a tri-state survives it; it is a DEREFERENCED false that erases the absent-vs-explicit-false distinction ADR-046 and ADR-048 read. cache.critical and server.logroutes were derivable, so deriving one at a moment when normalize filled it would have written a concrete false and flipped the posture. Both are now denied outright, and the comment says which direction actually bites. The denylist ran over the allowlist only. A hand-written literal under database. or multitenant.tenants. breaks ADR-051's presence check exactly as a derived value would -- it would read as configured and abort every database-free deployment. The preload rule now runs over the MERGED map, so neither door is open. Derivation-denied and preload-denied are separate lists: debug.* literals are legitimate and stay, since ADR-049 reads the decoded struct rather than koanf, which is also the mechanism my earlier comment got wrong. normalize rejecting a zero Config is a framework defect, not a deployment one, so it no longer surfaces a section key an operator would go edit in config that has not been loaded yet. Also: pointers render through rather than one level deep, prefix matching is case-insensitive, the tenant subtree is asserted alongside the other two ADR-051 sections, and mergeDefaults is extracted so its two rules are tested directly instead of sitting as branches nothing can reach today.
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 27 minutes Limit details: You’ve used the included review currently available. Your 105 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Comment |
CodeRabbit (mirror#7). The test asserted assert.NotContains over a map with prefix strings, which compares KEYS -- no key can equal "database.", so the loop passed for any map content, and a hand-written "database.host" would have sailed through the test whose name claims to reject it. It now builds the real merged map and runs matchesPrefix over every key in it. mergeDefaults refuses a denied key itself, so require.NoError is the assertion that bites; the loop names the offender when it does. Renamed to match what it checks, and the debug.allowedips positive assertion moved onto the merged map too.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
loadDefaultswrote ~55 keys by hand whilenormalizeindependently filled ten of them, agreement kept by hand. NowkoanfOnlyDefaults()merged withderivedDefaults(), which normalizes a zeroConfig, flattens it through the loader's own koanf tags, and picks an explicit allowlist. One mechanism per key: keys that must FAIL on zero stay hand-written, and full derivation is barred — it would handdebug's fail-closed posture to normalize and preload the database identity keys, disarming ADR-051.Impact
None. Values, types and provider ordering are unchanged, proven by an equivalence pin carrying the pre-change literals. No ADR, no migration atom.
Verification
Four gates hold at load, not in review: a derived key must differ from the un-normalized zero (one allowlisted before its fill reaches normalize would otherwise derive
"0s"), may not collide with a hand-written key, may not name a posture tri-state, and no key from either map may fall under a preload-denied prefix.Closes #1023