Secondary zones are immutable: MUST-NOT-MODIFY invariant for tdns-auth#331
Open
johanix wants to merge 17 commits into
Open
Secondary zones are immutable: MUST-NOT-MODIFY invariant for tdns-auth#331johanix wants to merge 17 commits into
johanix wants to merge 17 commits into
Conversation
Rev 1's central corollary does not survive contact with the code and is withdrawn: "every other origination path is option-gated, so forcing the options off neutralizes them through the existing runtime gates". The existing gates sit at CALL SITES, and the applier they feed accepts anything flagged InternalUpdate regardless of options (zone_updater.go:147) -- which every ops_* publisher sets. New/corrected mutation vectors: - 4d DSYNC publication (SetupZoneSync -> PublishDsyncRRs) is gated on delegation-sync-parent alone, no allow-updates check, and unlike CDS is not a no-op without local DNSKEYs. Rev 1's delsync carve-out missed it. - 10 the InternalUpdate applier bypass (structural). - 5b resignWorkingSetSOAIfSigned has no role gate, unlike SetupZoneSigning, and runs on every publish including the refresh path. - 6b catalog authoring IS reachable via the API handlers; rev 1 said it was not (it only considered the CreateCatalogZone path). Five fixes now, not three: Fix D adds a fail-closed gate in the ZoneUpdater applier (the enforcement point), Fix E adds the missing re-sign role gate. Fix B becomes the operator-facing visibility layer rather than the enforcement mechanism. Fix A widens to six persist/unixtime sites. Fix C widens to both zone handlers, the catalog handler, policy-reset, and now refuses freeze/thaw (whose rev-1 rationale was wrong: OptFrozen gates DDNS only, and its guards set resp.Error without returning). delegation-sync-parent joins the turn-off list (now four options), with the KeyState EDNS(0) consequence recorded as intended rather than incidental. New sections: config model (outbound_soa_serial is server-global and so cannot be role-scoped at config level -- it must become per-zone/per- template), parse ordering and the persisted-config round-trip, diagnostics (per-primary serial visibility in zone desc), migration (severity is per-serial-mode; the default keep mode already resets at every restart), and the forced-transfer contract. Residuals resolved: the JWK/A-AAAA/TLSA/SVCB/URI publishers have zero callers in tdns; the bumpch bump path is dead code. Still PROPOSED, not implemented. Open items in section 12.
…em closures Closes open items 2, 3 and 5 after a full OptOnlineSigning consumer survey (~40 sites): - online-signing joins the turn-off list (now five options). On tdns-auth the only keys are local ones, and the option unlocks -- with no role check anywhere -- whole-zone re-signing via the ResignQ path (key_state_worker -> triggerResign -> resignNow -> SignZone(force), zero role checks in key_state_worker.go/resigner.go/sign.go), standby key minting, per-refresh DNSKEY injection (CollectDynamicRRs), the per-publish SOA re-sign, BOGUS ephemeral signing of denial NSECs, and SERVFAIL of all responses when upstream is unsigned. Two new audit vectors recorded (5c, 5d). - Fix E alone would make things worse: gating the SOA re-sign while the option stays on trips ZoneTransferOut's fail-closed signed-zone check, silently refusing every outbound transfer. No coherent "inert" state exists; off is the only correct one. Fix E stays as defence in depth. - The legitimate form -- signing at the edge with DISTRIBUTED keys -- is the tdns-nm/tdns-es project and will most likely be a new app with its own AppType, which the 1.1 app-scoping already accommodates untouched. Recorded as a second worked example in 1.1; nothing here forecloses it. - Enforcement framing refined: Fix D backstops only the UpdateQ class of publishers; for the direct-under-zd.mu class (SignZone, PublishDnskeyRRs, CollectDynamicRRs) the option normalizer IS the enforcement. - Item 3: stale persisted serials are cleared (not retained). - Item 5: freeze/thaw returns land as a separate commit in the same PR. Remaining open: item 1 (per-zone outbound_soa_serial -- leaning toward inclusion, pending invasiveness scoping) and item 4 (pause-refresh, parked for discussion).
…ed, scope estimate Closes the last decidable open item and finalizes the doc for implementation: - Item 1 CLOSED: per-zone outbound_soa_serial is FOLDED INTO this PR (own commit pair, first). Invasiveness scoping showed ~120-180 lines + tests, not the feared doubling: ExpandTemplate generically gap-fills new ZoneConf fields (template propagation is free; an explicit "keep" under a "persist" template wins since the string is non-zero), the six read sites are lines Fix A edits anyway, and the OutgoingSerials table is already keyed per zone. Deliberate non-feature: no API knob in zone add/modify -- templates carry outbound-serial policy. - Section 5 rewritten around the decided state (the stale "scope TBD" two-part framing replaced): part 1 is the schema change itself, part 2 the suppression-regardless-of-source rule, with the explicit-vs-inherited warning split spelled out. - Consistency fixes from a fresh top-to-bottom read: header Base updated to 0536be8 (stale forward-merge note removed); rev 2.2 entry added to the section 0 change log; Fix B gains a note that the normalizer's duty covers the OutboundSoaSerial string field as well as ZoneOptions; section 10 gains the per-zone-mode test bullets (zone-wins-over- template, gap-fill, explicit keep under persist template, global fallback, reload flip, dynamic-zone round-trip, normalizer warning, per-zone independence). - NEW section 14: implementation scope estimate. Per-item file/LOC table; totals ~450-550 production LOC over ~14 files, ~1,200-1,600 test LOC in 7-9 files, ~100-200 existing-test adjustments; net ~+2,000/-200 over ~20-24 files in 9-10 commits (same order as the outbound-IXFR PR #328). Uncertainty drivers: the as-configured vs effective options mechanism, the DoTransfer probe extraction, and Fix D's validation (testbed wall-clock, not diff size). Remaining open: only item 4 (pause-refresh on a secondary), parked for a separate discussion and explicitly not part of this PR. The design is ready for implementation.
outbound_soa_serial was server-global (dnsengine.outbound_soa_serial ->
kdb.OutboundSoaSerial), read directly by every consumer. That makes it
impossible to express a per-zone serial policy -- and, in particular,
impossible to say "a secondary must not persist its serial" without
breaking co-hosted primaries on the same server.
Add zones.<z>.outbound_soa_serial (keep|unixtime|persist). Empty means
"inherit the server-global setting", which stays the default, so every
existing config keeps byte-identical semantics.
The intended granularity is per TEMPLATE: setting it on a template gives
a whole class of zones one serial policy. That needs no template code --
ExpandTemplate gap-fills any new ZoneConf field generically. A zone that
sets the value explicitly wins over its template, including an explicit
"keep" under a "persist" template, because a non-empty string counts as
set by the gap-fill.
- ZoneConf/ZoneData/ZoneRefresher gain the field; validated with
omitempty,oneof=keep unixtime persist.
- New zd.EffectiveOutboundSoaSerial() resolves zone -> global -> keep.
All six former direct-read sites now go through it (publish, refresh
replacement, both refresh-engine blocks, nextOutboundSerial).
- Refresher merge sites: the first-population and fresh-ZoneData paths
copy unconditionally; the reload/merge path gates on zr.ConfigUpdate
rather than on non-emptiness, since empty is MEANINGFUL here ("inherit
global") -- a config edit that removes the per-zone value must clear
it, while a bare NOTIFY refresher must not wipe a configured one.
- ModifyDynamicZone carries the value forward across the ZoneData
replacement, same reasoning as the existing Notify/ACL carry-forward:
there is deliberately no API knob for this, so without it a TSIG-only
modify would silently reset the zone to the global default.
- Dynamic-zone round-trip: zoneDataToZoneConf serializes the field, and
the LoadDynamicZoneFiles / dynamic-primary refreshers carry it.
- applyOutboundSoaSerial now creates the OutgoingSerials table
unconditionally: zones are parsed after it runs, so it can no longer
know whether some individual zone will be in persist mode.
No behaviour change for any existing config: with no per-zone value set
anywhere, every zone resolves to the global mode exactly as before.
Builds clean (go build ./..., go vet ./...), gofmt clean.
…-trip Pins the behaviour added in the previous commit: - tier resolution: explicit per-zone value wins, empty inherits the server-global, neither set defaults to "keep"; nil KeyDB does not panic (several init and unit-test paths build a ZoneData without one). - nextOutboundSerial reads the EFFECTIVE mode: per-zone unixtime jumps to a timestamp under a global keep, and an explicit per-zone keep stays on prev+1 under a global unixtime. - template interaction: a template value fills a silent zone, an explicit zone value wins. The "explicit keep under a persist template" case is pinned deliberately -- ExpandTemplate's gap-fill treats non-zero as "already set", so this only works because the field is a string; a bool would silently lose the override. - dynamic-zone round-trip: zoneDataToZoneConf serializes the field, and an unset mode serializes as EMPTY rather than a materialized "keep", so a zone that merely inherits the global is not pinned the first time its config is rewritten. Full v2 suite green.
…ix A) applyRefreshReplacementLocked bumped CurrentSerial unconditionally on every non-first load. That is the one mutation vector in the audit that was neither role- nor option-gated: even a zero-option secondary drifted +1 per refresh. The failure it caused is real. A signed zone distributed by two masters downstream of one signer -- BIND9 (no bump) and tdns-auth (+1 per refresh) -- advertises two different serials for identical content. Edge nodes NOTIFY'd by both always see the higher tdns serial and so always fetch from tdns, silently collapsing a deliberately redundant pair. A tdns-auth secondary that did not originate its content now mirrors new_zd.IncomingSerial verbatim. Primaries, inline-signing secondaries (the sanctioned exception -- they add their own RRSIGs, so their content and serial legitimately diverge) and every non-auth app are unchanged. Introduces the shared predicate zoneMayOriginateContent(zd) in a new zone_origination.go. It deliberately returns true for every app type other than tdns-auth, so the app-scope guarantee lives in ONE place and no future gate built on it can forget it: v2/ is a shared library and tdns-mpcombiner (which edits zones as a secondary by design), tdns-mpagent and tdns-agent must keep their own rules when they next bump their pin. MUST-NOT-MODIFY is absolute, not keep-mode-only, so the outbound-serial machinery is suppressed for a mirroring secondary at every site: the two SaveOutgoingSerial writes, both refresh-engine restore/unixtime blocks, and nextOutboundSerial's unixtime branch. Any serial persisted before the zone became a mirror is now DELETED (new KeyDB.DeleteOutgoingSerial) rather than left inert -- otherwise an inflated legacy value sits in the DB waiting for a future change to the restore path to resurrect it. Migration: a secondary previously running persist/unixtime (or a pre-fix build) steps backwards once, and its own downstreams will then refuse to transfer until upstream climbs past the old value (RFC 1982; BIND/NSD behave the same and a NOTIFY with a lower serial is ignored). That step is logged at ERROR naming both serials and the remedy, so it surfaces as an actionable message rather than as mysteriously stale data. In the default keep mode there is no new event: a restart already reset the serial to upstream's, and this merely stops the re-inflation afterwards. Tests are mutation-verified: TestSecondaryMirrorsUpstreamSerial fails on the pre-fix code (CurrentSerial = 6 instead of the mirrored 40) and passes with it. Also covered: the backwards step, primary unchanged, inline-signing secondary unchanged, unixtime suppression, the predicate itself, and the §1.1 app-scope guarantee via AppTypeAgent. Full v2 suite green; build, vet and gofmt clean.
resignWorkingSetSOAIfSigned gated only on online-signing/inline-signing,
with no role check -- unlike SetupZoneSigning, which has one ("a
non-primary signs only with inline-signing"). It runs inside
publishWorkingSetLocked, so it fires on EVERY publish including the
refresh path.
Consequence on a tdns-auth secondary carrying `online-signing`: the
upstream SOA is re-signed with locally generated keys (EnsureActiveDnssecKeys
mints them if absent) -- signatures from a key that is not in the zone's
published DNSKEY RRset, i.e. BOGUS to every validator downstream.
Gate it on zoneMayOriginateContent, the same predicate as Fix A. The
option is additionally normalized off for such a zone; this is the
defence in depth behind that, and it is what keeps the invariant intact
if the option is ever re-enabled through some path the normalizer does
not cover.
Build, vet, gofmt clean; full v2 suite green.
Both the freeze and thaw cases set resp.Error and an ErrorMsg on a failed precondition but never returned. Execution fell through to zd.SetOption(OptFrozen, ...) and then overwrote resp.Msg with a success string, so a caller received Error: true together with "Zone X is now frozen" -- and the zone's frozen state changed anyway, despite the handler having just declared the operation a no-op. Add the missing returns. Safe: the handler's deferred JSON encoder still writes the response on an early return, and that is the pattern every other case in the switch already uses (see "bump"). Behaviour change on the primary path too, and intentional: freeze on a primary without allow-updates previously froze it while reporting an error, and now genuinely refuses. That matches the message it was already returning. No idempotency regression -- the "already frozen" case likewise already returned Error: true, so any caller checking the flag was seeing it. Separate commit ahead of the secondary-role refusal, which depends on these guards actually returning to be a refusal at all.
…Fix D) The per-command checks accept ZONE-UPDATE when allow-updates is set OR when the request carries InternalUpdate -- and every ops_* publisher sets InternalUpdate (jwk, a_aaaa, dsync, key, uri, cds, tlsa, csync, svcb), as do ksk_rollover_ds_notify and signal_republish. allow-updates is therefore a CALL-SITE convention, not an applier gate: any publisher that does not check the option before enqueuing walks straight through. That is how DSYNC publication slipped past the original audit -- it is reached from SetupZoneSync rather than delegation_sync.go, so it never met the option check the other delsync publishers happen to make. Add one gate at the head of the updater loop, right after the zone is resolved: a tdns-auth zone that may not originate content never has zone content applied, whatever flags the request carries. Scoped by the new updaterCmdMutatesZoneContent to ZONE-UPDATE and CHILD-UPDATE; TRUSTSTORE-UPDATE passes through untouched (it writes the keystore, not zone content, and gating it would break SIG(0) key management on secondaries). Logged at ERROR as an invariant violation, matching the existing precedent for the child-updates case: once the origination options are normalized off nothing should reach this gate, so a hit means a code path bypassed the option system. Deliberately not written to the zone error registry, so it cannot collide with the operator-facing warning. SCOPE NOTE recorded in the tests: this gate is currently defence in depth for the ZONE-UPDATE path specifically, because the ZoneType==Secondary branch of that path calls kdb.ApplyZoneUpdateToDB, which is a `return nil` placeholder -- a ZONE-UPDATE against a secondary is already inert for that unrelated reason. The gate forecloses the vector before that placeholder is implemented. CHILD-UPDATE, by contrast, reaches a real DelegationBackend.ApplyChildUpdate today. Full v2 suite green under -race; build, vet, gofmt clean.
…tness finding Status moves to IN IMPLEMENTATION with a per-item progress table: the per-zone outbound_soa_serial pair, Fix A, Fix E, the freeze/thaw returns and Fix D are landed; Fix B, Fix C, the diagnostics, the forced-transfer contract and the startup warning remain. Also records two things learned while implementing: - the shared predicate folds the AppTypeAuth test into itself, so the §1.1 guarantee is stated once rather than at every call site (with the consequence that gate tests must set Globals.App.Type explicitly); - a correction to the audit's severity: ApplyZoneUpdateToDB is a placeholder and is the whole ZoneType==Secondary branch of the ZONE-UPDATE apply path, so every ZONE-UPDATE-borne vector on a secondary (DDNS, transport signals, DSYNC, the ops_*/InternalUpdate family) is currently LATENT rather than live. The fixes stand; the vectors go live the moment that placeholder is implemented, which is the argument for Fix D being structural.
…Fix B)
Five options let a zone originate content, and none is legitimate on a
tdns-auth secondary that is not inline-signing: allow-updates,
allow-child-updates, add-transport-signal, delegation-sync-parent and
online-signing. An explicit per-zone outbound_soa_serial of
persist/unixtime is the same class of misconfiguration -- it would
rewrite a serial that belongs to upstream -- so it is handled here too,
even though it is a string rather than a ZoneOption.
normalizeOptionsForRole strips them, returns the effective set plus what
it suppressed, and builds one operator-facing message naming the reason.
For online-signing the message also points at inline-signing, which is
almost certainly what was meant.
ConfigWarning, not ConfigError. ConfigError is in serviceImpactingErrors
("a NOTIFY/UPDATE/query handler should refuse with SERVFAIL"), so using
it would eventually take a merely-misconfigured secondary out of service,
and would park it permanently in the error column of zone list. The zone
is serving correctly; only some of its config is being ignored.
Ordering matters and is load-bearing: the static-config call site sits
between parseZoneOptions and activateUpdatePolicy. activateUpdatePolicy
returns a HARD error -- quarantining the zone -- when allow-child-updates
is set without a delegationbackend, so normalizing after it would replace
the promised soft warning with an outage. Running before it also means
the delegation-sync setup block sees delegation-sync-parent already
false, so SetupZoneSync never registers for a secondary and the DSYNC
vector is closed at parse time with no extra wiring.
The other call sites are the refresh-engine merge points, which is where
every dynamic/API ingress converges. Options and the serial mode are
normalized together there, since assigning either separately afterwards
would overwrite a normalized value with the raw one; their update rules
still differ (options replace only if provided; the serial mode is gated
on ConfigUpdate because empty is meaningful). Normalization runs against
the EFFECTIVE zone type, preferring the refresher's when it carries one,
so a reload that flips Primary->Secondary does not normalize against the
stale role.
As-configured vs effective: zd.SuppressedOptions records what was
stripped, and zoneDataToZoneConf now serializes Options u SuppressedOptions
via asConfiguredOptions(). The dynamic config file is REGENERATED from
live state on every successful refresh, so writing the effective set
would permanently delete the operator's allow-updates from their own
config -- after which the warning clears and the misconfiguration becomes
invisible, silently "fixed" by destroying the evidence. ModifyDynamicZone
carries the record across its ZoneData replacement for the same reason.
Full v2 suite green under -race; build, vet, gofmt clean.
…e decisions Fix B lands as 6c6b295. Two implementation decisions worth carrying in the doc rather than only in the commit: - the as-configured/effective split (the one genuinely open design choice in §6) resolved as SuppressedOptions: record only what was stripped and serialize the union, rather than duplicating the whole options map. Minimal surface, self-healing across reloads. - normalization runs against the EFFECTIVE zone type, and normalizes options and the serial mode together, so a Primary->Secondary flip on reload does not normalize against the stale role and neither value can be overwritten with its raw form afterwards.
…x C) Seven /zone commands and two /zone/dsync commands make the server originate content -- they write into the zone or advance its serial -- and none had a role check: bump, sign-zone, resign-zone, policy-set, change-policy, policy-reset, generate-nsec, publish-dsync-rrset, unpublish-dsync-rrset. Refuse them on a tdns-auth secondary. Gated once ahead of each handler's switch rather than a line per case, so a command added to originationAPICommands is covered automatically. Deliberately NOT a blanket secondary-refusal. write-zone stays allowed: it depends only on OptDirty/force, touches no options, and dumping a transferred zone to disk is a legitimate secondary operation. Read-only commands are untouched, as is reload -- re-pulling from upstream is the opposite of originating. freeze/thaw are refused at their own cases rather than through the set, because ORDER matters: the role check has to fire before the allow-updates precondition, or a secondary operator is told to enable an option the normalizer immediately strips again. Named the true reason instead. (OptFrozen gates DDNS and nothing else -- it does not pause refresh -- so on a secondary, where allow-updates is always normalized off, it is inert and refusing it costs nothing real.) Catalog authoring is gated inside regenerateCatalogZone, which covers all four handlers that reach it (add/delete zone, add/delete group) and any added later. The original audit claimed this path was unreachable for a secondary because it only considered CreateCatalogZone; the API reaches it directly, so a secondary catalog zone could be rewritten and republished through it. Catalog CONSUMPTION is untouched: auto-create and auto-delete act on MEMBER zones, never on the catalog zone, and are the entire point of RFC 9432. The refusal wording matches the option normalizer's warning and the applier gate's log line, so all three name the same reason. Full v2 suite green under -race; build, vet, gofmt clean.
FetchFromUpstream discarded a completed transfer whose serial matched ours, unconditionally. So `tdns-cli zone reload --force` on an already-current zone was a silent no-op that still reported success -- force did not mean force. That is load-bearing rather than cosmetic. Under strict serial passthrough a forced retransfer is the ONLY remedy for a downstream wedged behind a secondary whose serial stepped backwards, and the only escape hatch from a zone holding corrupt data under a current serial. Thread `force` into FetchFromUpstream (it did not take it) and exempt the unchanged-serial discard. The neighbouring lower-serial case already worked, but only incidentally: DoTransfer returns do_transfer=false WITHOUT an error for a lower upstream serial, and Refresh's `do_transfer || force` lets the forced path through anyway. Nothing pinned that, and tightening the comparison -- or turning it into an error -- would have broken the migration remedy silently, silently because the forced transfer would still report success while changing nothing. The decision is now a named predicate, shouldDiscardUnchangedTransfer, with a table test covering equal, ahead and behind serials in both modes; removing the force exemption fails it. Full v2 suite green; build, vet, gofmt clean.
… secondaries (§5) The option normalizer warns per zone about an EXPLICIT per-zone outbound_soa_serial of persist/unixtime on a secondary. A secondary that merely INHERITS a global one gets no such warning, deliberately: that would be noise on every secondary of a server whose primaries legitimately use persist. But without anything, the suppression is invisible — the operator set a server-wide policy and some zones quietly do not follow it. Log it once per parse instead, naming the affected zones and the reason. Scoped to tdns-auth, like every other gate, and skips zones carrying an explicit per-zone value since those already drew the per-zone warning.
The condition that motivated this whole invariant -- two masters serving one zone at different serials, with edge nodes silently always fetching from the higher one -- was not visible from tdns at all. Add it. `zone list -v` now shows our outbound serial (what we advertise to downstreams) next to our inbound one (what we last received from upstream), plus the effective outbound serial mode and which tier it came from (zone / global / default). Free: both serials are already on ZoneData, no network. On a correct secondary they are equal; a gap is exactly the drift being prevented. `zone desc` additionally probes EVERY configured primary and lists what each answered, flagging any that disagrees with our inbound serial. That is deliberately not DoTransfer: DoTransfer probes to DECIDE and short-circuits on the first usable answer, because one good primary is enough to transfer from. Here the disagreement IS the signal, so ProbeUpstreamSerials keeps every answer -- including failures, since an unreachable master is itself diagnostic. Read-only, makes no transfer decision, and costs one query per primary, so it is single-zone only and stays off the bulk listing. The probe uses the same verified channel the transfer would (per-peer TLS config and TSIG), so an XoT peer is not silently probed in plaintext and a TSIG mismatch surfaces here rather than at transfer time. Both new base-block lines are conditional on having something to show, which also keeps the `zone list -v` golden byte-identical (TestVerboseListZone_GoldenParity passes unchanged -- its fixture has zero serials). Full v2 suite green under -race, cli suite green, all four cmdv2 binaries build.
Fix C (b4b0744), the forced-transfer contract (5db9c4a), the global outbound-serial startup warning (fbff7ed) and the §7 diagnostics (4275b29) complete the §14.1 table. Status moves to IMPLEMENTED, with the two things still outstanding stated explicitly: review, and the live foffe testbed validation Fix D calls for — that gate changes a path which today accepts every internal update unconditionally, and unit tests cannot cover it the way the testbed can.
Contributor
📝 WalkthroughWalkthroughThe change adds tdns-auth origination gates for secondary zones, introduces per-zone outbound SOA serial policies, preserves those policies through refresh and dynamic-zone persistence, mirrors upstream serials for non-originating secondaries, supports forced transfers, and exposes serial diagnostics through zone APIs and CLI output. ChangesOrigination policy and option normalization
Serial resolution and secondary refresh behavior
Configuration and dynamic-zone propagation
Serial probing and CLI diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant APIzone
participant ZoneData
participant Upstream
CLI->>APIzone: request zone desc
APIzone->>ZoneData: populate serial details
ZoneData->>Upstream: probe SOA serial
Upstream-->>ZoneData: serial or probe error
ZoneData-->>CLI: render upstream serial diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v2/dynamic_zones.go (1)
1108-1146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize operator-supplied options before applying the modified zone
ModifyDynamicZoneonly normalizes the old options whenin.Options == nil. Whenin.Optionsis supplied,newZd.Optionscan contain origination options for a non-originating secondary, whilenewZd.SuppressedOptionsstays stale and noConfigWarningis produced. RunnormalizeOptionsForRole/applyOptionNormalizationon the submitted options before constructingnewZd, and updateOptions,OutboundSoaSerial, andSuppressedOptionswith the returned effective state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/dynamic_zones.go` around lines 1108 - 1146, Update ModifyDynamicZone to normalize submitted in.Options through normalizeOptionsForRole and applyOptionNormalization before constructing newZd, not only when options are nil. Use the returned effective state to set newZd.Options, OutboundSoaSerial, and SuppressedOptions, ensuring invalid origination options for non-originating secondaries are suppressed and ConfigWarning is produced.v2/dynamic_primary.go (1)
382-403: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPer-zone
OutboundSoaSerialis silently dropped for API/template-provisioned dynamic primaries.
zr.OutboundSoaSerialis set here, butprovisionDynamicPrimarypre-setszd.ZoneType: Primaryon the registeredZoneData(lines 382-403) before enqueueingzr. RefreshEngine's pre-registered-stub config-merge block — the only place that copieszr.OutboundSoaSerialontozd.OutboundSoaSerialviaapplyOptionNormalization— is gated byif zd.ZoneType == 0, which isfalsehere from the start. So the merge block never runs and this new field never reaches the liveZoneData; the zone silently falls back to the server-global default regardless of the template'soutbound_soa_serial.Set it directly on the
zdstruct literal (matching how every other field here bypasses the async merge), rather than relying on thezr-only path.🐛 Proposed fix
zd := &ZoneData{ - ZoneName: name, - ZoneType: Primary, + ZoneName: name, + ZoneType: Primary, + OutboundSoaSerial: spec.Zconf.OutboundSoaSerial, ZoneStore: MapZone,Also applies to: 451-471
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/dynamic_primary.go` around lines 382 - 403, Set the per-zone OutboundSoaSerial value directly in the ZoneData struct literals created by provisionDynamicPrimary, including both provisioning paths, using the value from the corresponding zone configuration/template. Do not rely on the RefreshEngine pre-registered-stub merge guarded by zd.ZoneType == 0, since these literals initialize ZoneType as Primary and bypass that path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/parseconfig.go`:
- Around line 1198-1241: The warning currently inspects asynchronously populated
zone registry fields, causing stale or false secondary classifications. Move
suppressed-zone collection into the ParseZones loop, using the already resolved
zonetype, options, and zconf.OutboundSoaSerial values; then pass the collected
names or equivalent resolved data to warnGlobalOutboundSerialSuppressed,
preserving global-mode, auth-type, explicit per-zone, and sorting behavior.
In `@v2/refreshengine.go`:
- Around line 837-845: Move the mirroring-secondary outgoing-serial cleanup from
initialLoadZone into publishWorkingSetLocked so it runs on every working-set
publish, including live role transitions. When a zone is non-originating and has
a KeyDB, call the existing DeleteOutgoingSerial logic; preserve the current
upstream serial behavior and ensure later persist-mode transitions cannot
resurrect the stale value.
In `@v2/zone_serial_probe.go`:
- Around line 90-98: Consolidate outbound SOA serial precedence into a shared
helper used by both EffectiveOutboundSoaSerial and
ZoneData.outboundSoaSerialSource. Remove the duplicated zone→global→default
resolution from outboundSoaSerialSource while preserving its source labels and
ensuring both methods use the same tier ordering.
- Around line 31-86: Update ZoneData.ProbeUpstreamSerials to accept a
context.Context parameter and use c.ExchangeContext for each upstream probe
instead of c.Exchange. Thread the zone API handler’s request context through the
ProbeUpstreamSerials call, preserving the existing per-upstream result handling.
---
Outside diff comments:
In `@v2/dynamic_primary.go`:
- Around line 382-403: Set the per-zone OutboundSoaSerial value directly in the
ZoneData struct literals created by provisionDynamicPrimary, including both
provisioning paths, using the value from the corresponding zone
configuration/template. Do not rely on the RefreshEngine pre-registered-stub
merge guarded by zd.ZoneType == 0, since these literals initialize ZoneType as
Primary and bypass that path.
In `@v2/dynamic_zones.go`:
- Around line 1108-1146: Update ModifyDynamicZone to normalize submitted
in.Options through normalizeOptionsForRole and applyOptionNormalization before
constructing newZd, not only when options are nil. Use the returned effective
state to set newZd.Options, OutboundSoaSerial, and SuppressedOptions, ensuring
invalid origination options for non-originating secondaries are suppressed and
ConfigWarning is produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 343e3ebf-da76-4e0e-9da3-e563282fcbea
⛔ Files ignored due to path filters (1)
docs/2026-07-25-secondary-zones-immutable.mdis excluded by none and included by none
📒 Files selected for processing (24)
v2/apihandler_catalog.gov2/apihandler_zone.gov2/cli/zone_cmds.gov2/cli/zone_serial_display_test.gov2/db_outgoing_serial.gov2/dynamic_primary.gov2/dynamic_zones.gov2/forced_transfer_test.gov2/outbound_soa_serial_test.gov2/parseconfig.gov2/refreshengine.gov2/secondary_serial_mirror_test.gov2/structs.gov2/transfer_fallback_test.gov2/xot_pkix_test.gov2/zone_mutation.gov2/zone_option_normalize.gov2/zone_option_normalize_test.gov2/zone_origination.gov2/zone_origination_api_test.gov2/zone_serial_probe.gov2/zone_updater.gov2/zone_updater_origination_test.gov2/zone_utils.go
Comment on lines
+1198
to
+1241
| warnGlobalOutboundSerialSuppressed(conf) | ||
|
|
||
| lgConfig.Debug("ParseZones complete") | ||
| return all_zones, broken_zones, nil | ||
| } | ||
|
|
||
| // warnGlobalOutboundSerialSuppressed tells the operator, once per parse, that a | ||
| // server-wide outbound_soa_serial of persist/unixtime is being ignored for the | ||
| // tdns-auth secondaries on this server. | ||
| // | ||
| // The option normalizer warns per zone about an EXPLICIT per-zone mode, but a | ||
| // secondary that merely inherits a global one gets no per-zone warning — that | ||
| // would be noise on every secondary of a server whose primaries legitimately | ||
| // use persist. Without this, though, the suppression would be entirely | ||
| // invisible: the operator set a server-wide policy and some zones quietly do | ||
| // not follow it. Name them once instead. | ||
| func warnGlobalOutboundSerialSuppressed(conf *Config) { | ||
| mode := strings.TrimSpace(strings.ToLower(conf.DnsEngine.OutboundSoaSerial)) | ||
| if mode != OutboundSoaSerialPersist && mode != OutboundSoaSerialUnixtime { | ||
| return | ||
| } | ||
| if Globals.App.Type != AppTypeAuth { | ||
| return | ||
| } | ||
|
|
||
| var suppressed []string | ||
| for zname, zd := range Zones.Items() { | ||
| // Only zones inheriting the global are interesting here; an explicit | ||
| // per-zone value already drew the normalizer's per-zone warning. | ||
| if zd == nil || zd.OutboundSoaSerial != "" { | ||
| continue | ||
| } | ||
| if !zoneMayOriginateContent(zd) { | ||
| suppressed = append(suppressed, zname) | ||
| } | ||
| } | ||
| if len(suppressed) == 0 { | ||
| return | ||
| } | ||
| sort.Strings(suppressed) | ||
| lgConfig.Warn("global outbound_soa_serial is suppressed for secondary zones", | ||
| "mode", mode, "count", len(suppressed), "zones", suppressed, | ||
| "reason", "a secondary must serve the serial it received from upstream, unmodified") | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Suppression warning reads zd.ZoneType/zd.OutboundSoaSerial before RefreshEngine has populated them — false positives on cold start.
zd.ZoneType (and zd.OutboundSoaSerial for a first-time zone) are only ever assigned inside RefreshEngine, asynchronously, after it consumes the ZoneRefresher sent over conf.Internal.RefreshZoneCh. warnGlobalOutboundSerialSuppressed runs synchronously right after the ParseZones loop, with no guarantee RefreshEngine has processed any of those messages yet.
On a cold start, every freshly-registered zd has ZoneType == 0, so zoneMayOriginateContent(zd) (zd.ZoneType == Primary || zd.Options[OptInlineSigning]) evaluates false for essentially every zone without inline-signing — including legitimate PRIMARY zones — so they'd be spuriously listed as "suppressed secondaries" whenever the global mode is persist/unixtime. On reload it reads the previous parse's values rather than the ones just computed in this pass.
Consider computing the suppressed set inline during the ParseZones loop itself (you already have zonetype/options/zconf.OutboundSoaSerial resolved there), rather than a second pass over the async-populated registry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/parseconfig.go` around lines 1198 - 1241, The warning currently inspects
asynchronously populated zone registry fields, causing stale or false secondary
classifications. Move suppressed-zone collection into the ParseZones loop, using
the already resolved zonetype, options, and zconf.OutboundSoaSerial values; then
pass the collected names or equivalent resolved data to
warnGlobalOutboundSerialSuppressed, preserving global-mode, auth-type, explicit
per-zone, and sorting behavior.
Comment on lines
+837
to
+845
| // Apply outbound_soa_serial mode after upstream refresh — | ||
| // but never for a mirroring secondary (MUST-NOT-MODIFY; | ||
| // applyRefreshReplacementLocked already set the serial | ||
| // to upstream's and nothing may move it off that). | ||
| if zd.KeyDB != nil && !zoneMayOriginateContent(zd) { | ||
| // no-op: serial belongs to upstream | ||
| } else if zd.KeyDB != nil { | ||
| serialChanged := false | ||
| switch zd.KeyDB.OutboundSoaSerial { | ||
| switch zd.EffectiveOutboundSoaSerial() { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Stale outgoing serial is never cleared on a live role transition (only at first load).
initialLoadZone (lines 119-128) clears a mirroring secondary's persisted outgoing serial, but this ticker-refresh branch is a pure no-op. A zone that becomes non-originating via a live config reload (no restart) keeps whatever serial was persisted before the transition; if it later flips back to may-originate under persist mode, LoadOutgoingSerial could resurrect that stale value — the exact resurrection scenario DeleteOutgoingSerial's doc comment warns about.
Consider moving the clear-on-mirror logic into publishWorkingSetLocked (zone_mutation.go), which runs on every publish path, instead of only in initialLoadZone.
🐛 Proposed fix (mirrors the initialLoadZone branch)
if zd.KeyDB != nil && !zoneMayOriginateContent(zd) {
- // no-op: serial belongs to upstream
+ // Clear any serial persisted before this zone became a
+ // mirror; see the matching clear in initialLoadZone.
+ if err := zd.KeyDB.DeleteOutgoingSerial(zone); err != nil {
+ lgEngine.Warn("failed to clear persisted outgoing serial for mirroring secondary",
+ "zone", zone, "err", err)
+ }
} else if zd.KeyDB != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Apply outbound_soa_serial mode after upstream refresh — | |
| // but never for a mirroring secondary (MUST-NOT-MODIFY; | |
| // applyRefreshReplacementLocked already set the serial | |
| // to upstream's and nothing may move it off that). | |
| if zd.KeyDB != nil && !zoneMayOriginateContent(zd) { | |
| // no-op: serial belongs to upstream | |
| } else if zd.KeyDB != nil { | |
| serialChanged := false | |
| switch zd.KeyDB.OutboundSoaSerial { | |
| switch zd.EffectiveOutboundSoaSerial() { | |
| // Apply outbound_soa_serial mode after upstream refresh — | |
| // but never for a mirroring secondary (MUST-NOT-MODIFY; | |
| // applyRefreshReplacementLocked already set the serial | |
| // to upstream's and nothing may move it off that). | |
| if zd.KeyDB != nil && !zoneMayOriginateContent(zd) { | |
| // Clear any serial persisted before this zone became a | |
| // mirror; see the matching clear in initialLoadZone. | |
| if err := zd.KeyDB.DeleteOutgoingSerial(zone); err != nil { | |
| lgEngine.Warn("failed to clear persisted outgoing serial for mirroring secondary", | |
| "zone", zone, "err", err) | |
| } | |
| } else if zd.KeyDB != nil { | |
| serialChanged := false | |
| switch zd.EffectiveOutboundSoaSerial() { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/refreshengine.go` around lines 837 - 845, Move the mirroring-secondary
outgoing-serial cleanup from initialLoadZone into publishWorkingSetLocked so it
runs on every working-set publish, including live role transitions. When a zone
is non-originating and has a KeyDB, call the existing DeleteOutgoingSerial
logic; preserve the current upstream serial behavior and ensure later
persist-mode transitions cannot resurrect the stale value.
Comment on lines
+31
to
+86
| func (zd *ZoneData) ProbeUpstreamSerials(conf *Config) []UpstreamSerial { | ||
| if zd == nil || len(zd.Upstreams) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| out := make([]UpstreamSerial, 0, len(zd.Upstreams)) | ||
| for _, up := range zd.Upstreams { | ||
| res := UpstreamSerial{Addr: up.Addr} | ||
|
|
||
| upstream := up.Addr | ||
| if _, _, err := net.SplitHostPort(upstream); err != nil { | ||
| upstream = net.JoinHostPort(upstream, defaultPortForPeer(up)) | ||
| } | ||
|
|
||
| m := new(dns.Msg) | ||
| m.SetQuestion(zd.ZoneName, dns.TypeSOA) | ||
| c := new(dns.Client) | ||
|
|
||
| // Probe over the same verified channel the transfer itself would use, | ||
| // so an XoT peer is not silently probed in plaintext and a TSIG | ||
| // mismatch surfaces here rather than at transfer time. | ||
| if tlsCfg, terr := conf.ClientTLSConfigForPeer(up); terr != nil { | ||
| res.Err = fmt.Sprintf("TLS setup failed: %v", terr) | ||
| out = append(out, res) | ||
| continue | ||
| } else if tlsCfg != nil { | ||
| c.Net = "tcp-tls" | ||
| c.TLSConfig = tlsCfg | ||
| } | ||
| provider, serr := SignForPeer(m, up.Key, conf) | ||
| if serr != nil { | ||
| res.Err = fmt.Sprintf("TSIG sign setup failed: %v", serr) | ||
| out = append(out, res) | ||
| continue | ||
| } | ||
| c.TsigProvider = provider // nil for NOKEY => plain exchange | ||
|
|
||
| r, _, err := c.Exchange(m, upstream) | ||
| switch { | ||
| case err != nil: | ||
| res.Err = err.Error() | ||
| case r.MsgHdr.Rcode != dns.RcodeSuccess: | ||
| res.Err = dns.RcodeToString[r.MsgHdr.Rcode] | ||
| case len(r.Answer) == 0: | ||
| res.Err = "NOERROR but empty answer section" | ||
| default: | ||
| if soa, ok := r.Answer[0].(*dns.SOA); ok { | ||
| res.Serial = soa.Serial | ||
| } else { | ||
| res.Err = "first answer is not a SOA" | ||
| } | ||
| } | ||
| out = append(out, res) | ||
| } | ||
| return out | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)zone_serial_probe\.go$|apihandler_zone\.go$|zone_utils\.go$|go\.mod$' || true
echo
echo "zone_serial_probe outline:"
ast-grep outline v2/zone_serial_probe.go --view expanded || true
echo
echo "Relevant v2/zone_serial_probe.go:"
cat -n v2/zone_serial_probe.go
echo
echo "Search ProbeUpstreamSerials usages:"
rg -n "ProbeUpstreamSerials|outboundSoaSerialSource|EffectiveOutboundSoaSerial" . -g '*.go' || true
echo
echo "Relevant apihandler_zone context around ProbeUpstreamSerials:"
rg -n -C 20 "ProbeUpstreamSerials" v2/apihandler_zone.go || true
echo
echo "Go module dns version:"
if [ -f go.mod ]; then rg -n "miekg/dns|zone_serial" go.mod go.sum || true; fiRepository: johanix/tdns
Length of output: 10183
🌐 Web query:
miekg dns Client ExchangeContext documentation timeout DialTimeout nettcp net-tls
💡 Result:
In the miekg/dns Go package, ExchangeContext is the recommended method for performing synchronous DNS queries while respecting context deadlines [1][2]. Client Timeouts and Context Deadlines When using ExchangeContext, the library attempts to honor both the provided context's deadline and any configured timeouts on the dns.Client instance (such as Timeout, DialTimeout, ReadTimeout, or WriteTimeout) [1][3]. If both a context deadline and a client timeout are configured, the earliest of the two takes effect [1]. Specifically: 1. Dialing: The context is used to control the dial phase via DialContext [1]. 2. Read/Write: While the library attempts to respect deadlines during IO operations, some versions have had known limitations regarding how they honor context cancellation versus configured timeouts [3][4]. For example, a context deadline might be used to set a connection deadline, but the context itself might not always be monitored directly during the read/write process, which can lead to situations where error types like context.DeadlineExceeded or context.Canceled are not returned as expected [4]. Client Fields - Timeout: A cumulative timeout for dial, write, and read operations [1][5]. When non-zero, it overrides individual DialTimeout, ReadTimeout, and WriteTimeout settings [1]. - DialTimeout: A specific timeout for the dial phase, defaulting to 2 seconds if not otherwise specified [1][5]. - Dialer: Allows for advanced control using a net.Dialer, which can be used to set timeouts, local addresses, and other network-level configurations [6][1][5]. - Net: Determines the transport protocol (e.g., "udp", "tcp", "tcp-tls") [1][5]. To use DNS over TLS, set the Net field to "tcp-tls" and provide a valid *tls.Config in the TLSConfig field [5]. Best Practices - To implement precise timeouts, prefer using a context with a deadline passed to ExchangeContext [1][2]. - For advanced network customization (such as binding to a specific interface or setting granular dialer timeouts), use the Client.Dialer field [6]. - Be aware that in some complex scenarios involving network jitter or underlying net.Conn behavior, the effective timeout might be slightly influenced by both the client's internal timers and the context deadline [4].
Citations:
- 1: https://github.com/miekg/dns/blob/master/client.go
- 2: https://github.com/miekg/dns/blob/b69c3a2007f8e943787a62f73ae1b5e09e143ff0/client.go
- 3: client timeouts vs deadline context miekg/dns#1398
- 4: canceling deadline context not honored miekg/dns#1399
- 5: https://github.com/miekg/dns/blob/v1.1.72/client.go
- 6: https://github.com/miekg/dns/blob/master/doc.go
🌐 Web query:
miekg dns Client Exchange implementation dial read write timeout defaults v1.1.114
💡 Result:
In the miekg/dns library, the dns.Client implementation handles timeouts for dial, read, and write operations using a combination of specific fields and a default constant [1]. As of version 1.1.114, the behavior remains consistent with established implementation patterns [1][2]. Default Timeout Values: The library uses a hardcoded default, dnsTimeout, which is set to 2 seconds [1][3]. This value is used if the specific timeout fields are not configured by the user [1]. Timeout Configuration Fields: 1. DialTimeout: Defaults to 2 seconds [1]. 2. ReadTimeout: Defaults to 2 seconds [1]. 3. WriteTimeout: Defaults to 2 seconds [1]. 4. Timeout (Cumulative): This is an optional cumulative timeout for dial, write, and read operations [1]. It defaults to 0 (disabled) [1]. When set to a non-zero value, it overrides the individual DialTimeout, ReadTimeout, and WriteTimeout settings [1]. Implementation Logic: The library determines the active timeout for each operation using helper methods (e.g., dialTimeout, readTimeout, writeTimeout) [1]. These methods follow this priority logic [1]: 1. If the cumulative Client.Timeout is set (non-zero), it is used as the timeout for the operation [1]. 2. If Client.Timeout is 0, the specific field (e.g., Client.DialTimeout) is checked [1]. If that is non-zero, it is used [1]. 3. If neither is set, the default dnsTimeout (2s) is returned [1]. Additional Context: - Users can override these defaults using a custom net.Dialer provided to the Client or by using context-aware methods like ExchangeContext, which honors context deadlines [1][4][5]. - The cumulative Timeout field is designed to simplify timeout management but does not automatically supersede context deadlines [1][5].
Citations:
- 1: https://github.com/miekg/dns/blob/master/client.go
- 2: https://github.com/miekg/dns/blob/b69c3a2007f8e943787a62f73ae1b5e09e143ff0/client.go
- 3: https://github.com/miekg/dns/blob/v1.1.72/client.go
- 4: https://github.com/miekg/dns/blob/master/doc.go
- 5: client timeouts vs deadline context miekg/dns#1398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate zone handler context:"
rg -n -C 8 "populateZoneDescDetail|func .*zone .*desc|zone desc|zoneDesc|DescribeZone" v2 -g '*.go' || true
echo
echo "v2/apihandler_zone.go top/context:"
sed -n '1,80p' v2/apihandler_zone.go | cat -n
echo
echo "apihandler_zone around function:"
sed -n '400,445p' v2/apihandler_zone.go | cat -n
echo
echo "v2/go.mod dns dependency:"
rg -n "miekg/dns" v2/go.mod || trueRepository: johanix/tdns
Length of output: 50368
Thread the request context through ProbeUpstreamSerials.
probeUpstreamSerials(&Conf) is called synchronously from the zone API handler (apihandler_zone.go:439), but it has no context parameter and uses c.Exchange, so a multi-primary zone desc can exceed the HTTP context deadline and block the handler for len(zd.Upstreams) × per-upstream timeout. Accept [context.Context] as the first parameter and call c.ExchangeContext for each upstream.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/zone_serial_probe.go` around lines 31 - 86, Update
ZoneData.ProbeUpstreamSerials to accept a context.Context parameter and use
c.ExchangeContext for each upstream probe instead of c.Exchange. Thread the zone
API handler’s request context through the ProbeUpstreamSerials call, preserving
the existing per-upstream result handling.
Source: Coding guidelines
Comment on lines
+90
to
+98
| func (zd *ZoneData) outboundSoaSerialSource() string { | ||
| if zd.OutboundSoaSerial != "" { | ||
| return "zone" // per-zone setting, possibly inherited from its template | ||
| } | ||
| if zd.KeyDB != nil && zd.KeyDB.OutboundSoaSerial != "" { | ||
| return "global" // dnsengine.outbound_soa_serial | ||
| } | ||
| return "default" | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate tier-resolution logic with EffectiveOutboundSoaSerial.
outboundSoaSerialSource re-implements the same zone→global→default precedence chain already defined in EffectiveOutboundSoaSerial (v2/zone_utils.go:718-726). If the precedence logic ever changes in one place (e.g., a new per-template tier), the other risks silently diverging, causing zone desc to show a "source" that doesn't match the actual effective value.
♻️ Suggested consolidation
-func (zd *ZoneData) outboundSoaSerialSource() string {
- if zd.OutboundSoaSerial != "" {
- return "zone"
- }
- if zd.KeyDB != nil && zd.KeyDB.OutboundSoaSerial != "" {
- return "global"
- }
- return "default"
-}
+// EffectiveOutboundSoaSerialWithSource returns both the effective value and
+// which tier supplied it, used by both refresh logic and `zone desc`.
+func (zd *ZoneData) EffectiveOutboundSoaSerialWithSource() (string, string) {
+ if zd.OutboundSoaSerial != "" {
+ return zd.OutboundSoaSerial, "zone"
+ }
+ if zd.KeyDB != nil && zd.KeyDB.OutboundSoaSerial != "" {
+ return zd.KeyDB.OutboundSoaSerial, "global"
+ }
+ return OutboundSoaSerialKeep, "default"
+}EffectiveOutboundSoaSerial and outboundSoaSerialSource can then both delegate to this single helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/zone_serial_probe.go` around lines 90 - 98, Consolidate outbound SOA
serial precedence into a shared helper used by both EffectiveOutboundSoaSerial
and ZoneData.outboundSoaSerialSource. Remove the duplicated zone→global→default
resolution from outboundSoaSerialSource while preserving its source labels and
ensuring both methods use the same tier ordering.
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.
Implements
docs/2026-07-25-secondary-zones-immutable.md(rev 2.2, on this branch).The invariant
A secondary serves a copy of a zone whose authoritative source is upstream, so what it serves — content and SOA serial — must match what it received. That is what makes multiple secondaries of one primary interchangeable. The gating question is not "is this zone signed" but "did we originate this content?"
One predicate drives everything:
zoneMayOriginateContent(zd) = Primary || inline-signing, inv2/zone_origination.go. It deliberately returns true for every app type other than tdns-auth, so the app-scope guarantee is stated once and no gate built on it can forget it —v2/is a shared library and tdns-mpcombiner (which edits zones as a secondary by design), tdns-mpagent and tdns-agent must keep their own rules when they next bump their pin.The failure this fixes
A signed zone distributed by two masters downstream of one signer — BIND9 (no bump) and tdns-auth (+1 per refresh) — advertised two different serials for identical content. Edge nodes NOTIFY'd by both always saw the higher tdns serial and so always fetched from tdns, silently collapsing a deliberately redundant pair.
What changed
c5dc33a095374aoutbound_soa_serial. Was server-global, so "a secondary must not persist its serial" could not be expressed without breaking co-hosted primaries. Now per-zone (in practice per template —ExpandTemplategap-fills it for free), empty = inherit the global. Every existing config keeps byte-identical semantics.a030ab4applyRefreshReplacementLockedbumped unconditionally: the one vector that was neither role- nor option-gated. A mirroring secondary now takes upstream's serial verbatim, with persist/unixtime suppressed at all six sites and any stale persisted row cleared. Mutation-verified.1fa3f36online-signingsecondary re-signed the upstream SOA with locally minted keys — bogus to every downstream validator.82b54f5returns. Pre-existing bug: the guards setresp.Errorbut fell through, changing state anyway and returning an error and a success message. Separate commit because the role refusal depends on them.477ea49allow-updatesis a call-site convention, not an applier gate: everyops_*publisher setsInternalUpdate, which bypasses it. One gate at the ZoneUpdater head, scoped toZONE-UPDATE/CHILD-UPDATE(TRUSTSTORE-UPDATEpasses — it writes the keystore, not zone content).6c6b295ConfigWarning(notConfigError, which is service-impacting). Ordering is load-bearing: it runs beforeactivateUpdatePolicy, which would otherwise hard-quarantine a secondary withallow-child-updatesand no backend instead of warning it.b4b0744regenerateCatalogZone, covering all four entry points).write-zonestays allowed; catalog consumption is untouched — it provisions other zones and is the point of RFC 9432.5db9c4azone reload --forceon an already-current zone was a silent no-op — force did not mean force. It is the only remedy for a downstream wedged behind a backwards serial step. Mutation-verified.fbff7ed4275b29zone list -vshows outbound vs inbound serial;zone descprobes every primary and flags disagreement. The split-brain above was previously invisible from tdns.Review notes
Two audit corrections worth knowing.
ApplyZoneUpdateToDBis areturn nilplaceholder and is the entireZoneType == Secondarybranch of the ZONE-UPDATE apply path — so every ZONE-UPDATE-borne vector on a secondary (DDNS, transport signals, DSYNC, the wholeops_*/InternalUpdate family) is currently latent, not live. The fixes stand; they foreclose the vectors before that placeholder is implemented, which is the argument for Fix D being structural. And catalog authoring is reachable via the API — rev 1 claimed otherwise, having only consideredCreateCatalogZone.Test honesty. Two tests initially passed with their fix removed, because they asserted on state the path doesn't touch. Fixed by extracting the real decision into named predicates (
updaterCmdMutatesZoneContent,shouldDiscardUnchangedTransfer) and testing those. Fix A and §9 are mutation-verified end to end; Fix D's end-to-end test is labelled a smoke test in its own comment, for the placeholder reason above.The one open design choice, the as-configured vs effective options split, resolved as
SuppressedOptions: record only what was stripped and serialize the union. The dynamic config file is regenerated from live state on every refresh, so writing the effective set would have permanently deleted the operator'sallow-updatesfrom their own config — after which the warning clears and the misconfiguration becomes invisible.Migration
Severity is per serial mode. In the default
keepmode there is no new event — a restart already reset the serial to upstream's, and this merely stops the re-inflation. Onlypersist/unixtimedeployments see a one-time backwards step, and only where the secondary is itself a master for someone. That step is logged at ERROR naming both serials and the remedy (tdns-cli zone reload --forceon each downstream;rndc retransferfor BIND).Verification
go vetclean ·go test -racegreen (v2 + cli) · gofmt clean · all fourcmdv2binaries build · every commit compiles and is GPG-signed.Not yet done: the live foffe testbed validation Fix D calls for. That gate changes a path which today accepts every internal update unconditionally, and unit tests cannot cover it the way the testbed can.
Prerequisite for inbound IXFR — the serial-space contract IXFR-in depends on only makes sense once the secondary serial mirrors upstream.
Summary by CodeRabbit
New Features
Bug Fixes