Inspect zone file and journal before seeding a zone #20

Merged

benvin merged 7 commits from benvin/zone-seed-journal-safe into main 2026-09-19 23:42:03 +10:00

Conversation 4 Commits 7 Files Changed 16

+1398 -25

unkin-agent commented 2026-09-19 22:33:57 +10:00

Member

Copy Link

Copy Source

Seeding a zone overwrote its database file at serial 1 unconditionally. With a BIND journal present the skeleton fell outside the journal's serial range, so named failed the load with "out of range"; where it did load, secondaries holding a higher serial refused the transfer under RFC 1982.

• probe the SOA serial, the journal header and any quarantine siblings before seeding
• write a skeleton only when nothing is left to preserve, always above the highest serial seen
• rename unusable files aside rather than overwriting them
• stage, verify and install in a single exec so an interrupted seed cannot regress the serial
• abort the generated scripts at the first failed command, and fail the size guard closed
• route zone, catalog and policy seeds through EnsureSeedZone

Seeding a zone overwrote its database file at serial 1 unconditionally. With a BIND journal present the skeleton fell outside the journal's serial range, so named failed the load with "out of range"; where it did load, secondaries holding a higher serial refused the transfer under RFC 1982. - probe the SOA serial, the journal header and any quarantine siblings before seeding - write a skeleton only when nothing is left to preserve, always above the highest serial seen - rename unusable files aside rather than overwriting them - stage, verify and install in a single exec so an interrupted seed cannot regress the serial - abort the generated scripts at the first failed command, and fail the size guard closed - route zone, catalog and policy seeds through EnsureSeedZone

unkin-agent added 1 commit 2026-09-19 22:33:57 +10:00

Inspect zone file and journal before seeding a zone ... f1d47c8ff7

Fixes #19

unkin-agent commented 2026-09-19 22:45:32 +10:00

Author

Member

Copy Link

Copy Source

• internal/controller/bindpolicy_controller.go:70, internal/controller/bindcatalogzone_controller.go:57 — still call WriteSeedZone(..., 1) with no PlanSeed, on a type primary; allow-update zone under the same zones/ directory, so the exact bug this PR fixes still bites RPZ and catalog zones → route both through ZoneDiskState/PlanSeed/Quarantine, or the new WriteSeedZone contract ("callers must clear it with PlanSeed first") is violated by 2 of its 3 callers.
• internal/bind/zonestate.go:162 parseZoneDiskState — empty or truncated probe output parses to {ZoneFile:false, Journal:false}, which PlanSeed turns into WriteSeed at serial 1 and WriteSeedZone then clobbers whatever is on the PVC. The probe exits 0 whatever happens, so a degraded exec is indistinguishable from an empty directory → emit a terminating sentinel line from the probe and return an error when it is missing, instead of defaulting to the seed-and-clobber state.
• internal/bind/zonestate.go:43 — the !st.ZoneFile branch ignores JournalOK: a journal present but unreadable (no od in the image, short read, EACCES, unknown format) is moved aside and the zone reseeded at serial 1 with its real serial unknown. parseZoneDiskState("zonefile=0\njournal=1\njnl=\n") yields {WriteSeed:true Serial:1 QuarantineJournal:true QuarantineSuffix:".orphaned-0"} → return SeedPlan{} when st.Journal && !st.JournalOK.
• internal/bind/zonestate.go:73 highestSerial — seeds h with 0 and then compares it in RFC 1982 space, so 0 is treated as a serial. With no zone file and JournalEnd >= 2^31, serialLT(0, JournalEnd) is false, h stays 0 and the seed serial drops to 1. Measured: JournalEnd=2147483648 → Serial=1, serialLT(2147483648, 1) == false, i.e. a serial regression, and suffix .orphaned-0 → take JournalEnd directly when there is no usable zone serial rather than folding it through a zero sentinel.
• internal/bind/zonestate_test.go:217 TestPlanSeedSerialWrap — passes for the wrong reason: highestSerial returns 0 here, so nextSerial's next == 0 branch is never executed and the wrap is untested. The test asserts Serial == 1 but never that the seed is newer than JournalEnd → assert serialLT(st.JournalEnd, p.Serial) and it will fail as written.
• nit: internal/bind/zonestate.go:154,219 and internal/bind/seed.go:60 — paths are interpolated into single quotes with no escaping, and api/v1alpha1/bindzone_types.go:51 puts no kubebuilder:validation:Pattern on zoneName, so a name containing ' yields arbitrary shell — now including mv → validate zoneName at the CRD, or pass paths as sh -c '...' _ "$1" positional args.
• nit: internal/bind/zonestate.go:219 moveAside — plain mv overwrites an existing <path>.orphaned-<serial>, so a second incident computing the same suffix replaces the copy that preserved the original data → mv -n, or make the suffix unique.
• nit: internal/bind/zonestate.go:136 — prefix-matching ;BIND LOG V9 accepts any future V9.x; BIND memcmps the full 16 bytes against ";BIND LOG V9\n" / ";BIND LOG V9.2\n" (lib/dns/journal.c) → match both exactly so a format that moves the begin/end offsets is not silently misparsed.
• nit: internal/bind/zonestate.go:154 — the probe adds od and head -c to the BIND image requirements; api/v1alpha1/bindcluster_types.go:61 still documents only "named, rndc and nsupdate".

- internal/controller/bindpolicy_controller.go:70, internal/controller/bindcatalogzone_controller.go:57 — still call `WriteSeedZone(..., 1)` with no `PlanSeed`, on a `type primary; allow-update` zone under the same `zones/` directory, so the exact bug this PR fixes still bites RPZ and catalog zones → route both through `ZoneDiskState`/`PlanSeed`/`Quarantine`, or the new `WriteSeedZone` contract ("callers must clear it with PlanSeed first") is violated by 2 of its 3 callers. - internal/bind/zonestate.go:162 `parseZoneDiskState` — empty or truncated probe output parses to `{ZoneFile:false, Journal:false}`, which `PlanSeed` turns into `WriteSeed` at serial 1 and `WriteSeedZone` then clobbers whatever is on the PVC. The probe exits 0 whatever happens, so a degraded exec is indistinguishable from an empty directory → emit a terminating sentinel line from the probe and return an error when it is missing, instead of defaulting to the seed-and-clobber state. - internal/bind/zonestate.go:43 — the `!st.ZoneFile` branch ignores `JournalOK`: a journal present but unreadable (no `od` in the image, short read, EACCES, unknown format) is moved aside and the zone reseeded at serial 1 with its real serial unknown. `parseZoneDiskState("zonefile=0\njournal=1\njnl=\n")` yields `{WriteSeed:true Serial:1 QuarantineJournal:true QuarantineSuffix:".orphaned-0"}` → return `SeedPlan{}` when `st.Journal && !st.JournalOK`. - internal/bind/zonestate.go:73 `highestSerial` — seeds `h` with 0 and then compares it in RFC 1982 space, so 0 is treated as a serial. With no zone file and `JournalEnd >= 2^31`, `serialLT(0, JournalEnd)` is false, `h` stays 0 and the seed serial drops to 1. Measured: `JournalEnd=2147483648` → `Serial=1`, `serialLT(2147483648, 1) == false`, i.e. a serial regression, and suffix `.orphaned-0` → take `JournalEnd` directly when there is no usable zone serial rather than folding it through a zero sentinel. - internal/bind/zonestate_test.go:217 `TestPlanSeedSerialWrap` — passes for the wrong reason: `highestSerial` returns 0 here, so `nextSerial`'s `next == 0` branch is never executed and the wrap is untested. The test asserts `Serial == 1` but never that the seed is newer than `JournalEnd` → assert `serialLT(st.JournalEnd, p.Serial)` and it will fail as written. - nit: internal/bind/zonestate.go:154,219 and internal/bind/seed.go:60 — paths are interpolated into single quotes with no escaping, and `api/v1alpha1/bindzone_types.go:51` puts no `kubebuilder:validation:Pattern` on `zoneName`, so a name containing `'` yields arbitrary shell — now including `mv` → validate `zoneName` at the CRD, or pass paths as `sh -c '...' _ "$1"` positional args. - nit: internal/bind/zonestate.go:219 `moveAside` — plain `mv` overwrites an existing `<path>.orphaned-<serial>`, so a second incident computing the same suffix replaces the copy that preserved the original data → `mv -n`, or make the suffix unique. - nit: internal/bind/zonestate.go:136 — prefix-matching `;BIND LOG V9` accepts any future `V9.x`; BIND memcmps the full 16 bytes against `";BIND LOG V9\n"` / `";BIND LOG V9.2\n"` (lib/dns/journal.c) → match both exactly so a format that moves the begin/end offsets is not silently misparsed. - nit: internal/bind/zonestate.go:154 — the probe adds `od` and `head -c` to the BIND image requirements; `api/v1alpha1/bindcluster_types.go:61` still documents only "named, rndc and nsupdate".

unkin-agent added 2 commits 2026-09-19 22:55:34 +10:00

Route every zone seed through a fail-closed journal check da679285f0

Restrict zone names to DNS label characters 6b5e465093

unkin-agent added 1 commit 2026-09-19 22:56:51 +10:00

Unexport the unguarded seed write 0065268372

unkin-agent commented 2026-09-19 23:05:21 +10:00

Author

Member

Copy Link

Copy Source

internal/bind/seed.go:68-74 — Quarantine and writeSeedZone are two separate execs, so a failure of the second leaves the PVC with no zone file and no journal. The next reconcile probes a clean directory and seeds at serial 1, below the serial the quarantined journal recorded (and below what secondaries already hold) — the serial floor only survives in the .orphaned-<n> filename, which nothing reads. Reachable on any transient exec error/pod eviction between the two calls, and it hits the exact production case the PR fixes (quarantine both, reseed at 17, write fails, reseed at 1). → do the rename and the seed write in one sh -c (seed content on stdin), or recover the floor from *.orphaned-*.

internal/bind/seed.go:82, internal/bind/zonestate.go:60-62 — cat > path truncates in place, so an interrupted seed leaves a 0-byte/partial zone file; PlanSeed then returns the empty plan forever for ZoneFile=true, ZoneSerialOK=false (verified: probe on an empty file yields {ZoneFile:true ZoneSerialOK:false}, ok=true, plan {}). AddZone fails every reconcile and nothing repairs it without a manual exec. → cat > path.tmp && mv -- path.tmp path.

nit: internal/controller/bindcatalogzone_controller.go:57, internal/controller/bindpolicy_controller.go:70 — preserving the on-disk file changes semantics for these two. Catalog members and RPZ rules are add-only via nsupdate with no pruning reconcile; the old seed-at-1 was the only thing that dropped stale member PTRs / stale CNAME rules when named forgot the zone. After this change they persist indefinitely. Decide explicitly whether these two should preserve or reset.

nit: internal/controller/bindzone_controller.go:82 + rndc.go:54 — DelZone is plain rndc delzone, leaving db+jnl on the PVC. Delete-then-recreate of a BindZone now resurrects the previous incarnation's records instead of starting from a skeleton.

nit: internal/bind/zonestate.go:161-164 — journalFormats is pinned to V9/V9.2. A future BIND journal magic makes every journal unjudgeable and hard-blocks any zone whose file is missing; at minimum log the unrecognised magic in the Blocked message.

internal/bind/seed.go:68-74 — `Quarantine` and `writeSeedZone` are two separate execs, so a failure of the second leaves the PVC with no zone file and no journal. The next reconcile probes a clean directory and seeds at serial **1**, below the serial the quarantined journal recorded (and below what secondaries already hold) — the serial floor only survives in the `.orphaned-<n>` filename, which nothing reads. Reachable on any transient exec error/pod eviction between the two calls, and it hits the exact production case the PR fixes (quarantine both, reseed at 17, write fails, reseed at 1). → do the rename and the seed write in one `sh -c` (seed content on stdin), or recover the floor from `*.orphaned-*`. internal/bind/seed.go:82, internal/bind/zonestate.go:60-62 — `cat > path` truncates in place, so an interrupted seed leaves a 0-byte/partial zone file; `PlanSeed` then returns the empty plan forever for `ZoneFile=true, ZoneSerialOK=false` (verified: probe on an empty file yields `{ZoneFile:true ZoneSerialOK:false}`, `ok=true`, plan `{}`). `AddZone` fails every reconcile and nothing repairs it without a manual exec. → `cat > path.tmp && mv -- path.tmp path`. nit: internal/controller/bindcatalogzone_controller.go:57, internal/controller/bindpolicy_controller.go:70 — preserving the on-disk file changes semantics for these two. Catalog members and RPZ rules are add-only via nsupdate with no pruning reconcile; the old seed-at-1 was the only thing that dropped stale member PTRs / stale CNAME rules when named forgot the zone. After this change they persist indefinitely. Decide explicitly whether these two should preserve or reset. nit: internal/controller/bindzone_controller.go:82 + rndc.go:54 — `DelZone` is plain `rndc delzone`, leaving db+jnl on the PVC. Delete-then-recreate of a BindZone now resurrects the previous incarnation's records instead of starting from a skeleton. nit: internal/bind/zonestate.go:161-164 — `journalFormats` is pinned to V9/V9.2. A future BIND journal magic makes every journal unjudgeable and hard-blocks any zone whose file is missing; at minimum log the unrecognised magic in the `Blocked` message.

unkin-agent added 2 commits 2026-09-19 23:14:32 +10:00

Stage and install the seed zone file in one exec ... 6730b8bcb2

Quarantine and write were separate round-trips, so a failure between them
left the PVC with no zone data and the next reconcile reseeded at serial 1.
The write also truncated the destination in place, leaving a torn file that
PlanSeed refuses to touch.

Read quarantine evidence back when planning a seed 949662a334

unkin-agent commented 2026-09-19 23:25:35 +10:00

Author

Member

Copy Link

Copy Source

• internal/bind/seed.go:87-101 — seedScript joins its steps with \n and never sets set -e, so a failed step does not abort. If either moveAside rename fails, the final mv still installs the skeleton over the still-live zone file and the script exits 0, so EnsureSeedZone reports success. Reproduced with a failing mv: the zone file ends up as the skeleton with the old .jnl still beside it — the exact "out of range" state this PR exists to prevent. → prepend set -e (or chain the steps with &&); the explicit rm -f; exit 1 guard still works under -e.
• internal/bind/seed.go:93 — the short-read guard fails open. When the command substitution yields nothing (wc not in the image, or $tmp missing because mkdir/cat failed), [ "" -ne <size> ] exits 2, the if takes the false branch, and the script proceeds to quarantine and install. Reproduced with a wc that exits 127: a 4-byte stdin was installed as the zone file, script exit 0. set -e does not fix this — the test is an if condition. → capture first and fail closed, e.g. sz=$(wc -c < tmp) || { rm -f tmp; exit 1; } plus a string compare [ "$sz" != "<size>" ].
• nit: internal/bind/zonestate.go:253 — "quarantineMarker joins a preserved file to the serial it was holding" is no longer true: quarantineSuffix uses highestSerial, which now folds in OrphanSerial and JournalEnd, so the number can exceed the renamed file's own serial. → reword to "the high-water serial observed when it was preserved".
• nit: internal/bind/zonestate.go:91 — highestSerial folds three inputs with pairwise serialLT, which is not a total order; if any two of zone serial / journal end / orphan floor are ~2^31 apart the fold can settle on a value that nextSerial does not put strictly after all three. Unreachable with realistic serials, but nothing documents or rejects the case. → either note the assumption or reject a spread > 2^31 as Blocked.

- `internal/bind/seed.go:87-101` — `seedScript` joins its steps with `\n` and never sets `set -e`, so a failed step does not abort. If either `moveAside` rename fails, the final `mv` still installs the skeleton over the still-live zone file and the script exits 0, so `EnsureSeedZone` reports success. Reproduced with a failing `mv`: the zone file ends up as the skeleton with the old `.jnl` still beside it — the exact "out of range" state this PR exists to prevent. → prepend `set -e` (or chain the steps with `&&`); the explicit `rm -f; exit 1` guard still works under `-e`. - `internal/bind/seed.go:93` — the short-read guard fails open. When the command substitution yields nothing (`wc` not in the image, or `$tmp` missing because `mkdir`/`cat` failed), `[ "" -ne <size> ]` exits 2, the `if` takes the false branch, and the script proceeds to quarantine and install. Reproduced with a `wc` that exits 127: a 4-byte stdin was installed as the zone file, script exit 0. `set -e` does not fix this — the test is an `if` condition. → capture first and fail closed, e.g. `sz=$(wc -c < tmp) || { rm -f tmp; exit 1; }` plus a string compare `[ "$sz" != "<size>" ]`. - nit: `internal/bind/zonestate.go:253` — "quarantineMarker joins a preserved file to the serial it was holding" is no longer true: `quarantineSuffix` uses `highestSerial`, which now folds in `OrphanSerial` and `JournalEnd`, so the number can exceed the renamed file's own serial. → reword to "the high-water serial observed when it was preserved". - nit: `internal/bind/zonestate.go:91` — `highestSerial` folds three inputs with pairwise `serialLT`, which is not a total order; if any two of zone serial / journal end / orphan floor are ~2^31 apart the fold can settle on a value that `nextSerial` does not put strictly after all three. Unreachable with realistic serials, but nothing documents or rejects the case. → either note the assumption or reject a spread > 2^31 as `Blocked`.

unkin-agent added 1 commit 2026-09-19 23:31:15 +10:00

Abort the seed and probe scripts on the first failed command ... d1dd5040d5

Guard the staged size as a string so an unmeasurable file fails closed.

benvin merged commit 03df5d56b4 into main 2026-09-19 23:42:03 +10:00

benvin deleted branch benvin/zone-seed-journal-safe 2026-09-19 23:42:03 +10:00

unkin-agent commented 2026-09-19 23:43:20 +10:00

Author

Member

Copy Link

Copy Source

• nit: internal/bind/zonestate.go:65 — a zone file whose SOA will not parse (0-byte, truncated, unknown layout) returns an empty plan, so EnsureSeedZone reports success and the zone then wedges at AddZone with an opaque BIND error → return Blocked with that reason, as the unreadable-journal case already does.
• nit: internal/bind/zonestate.go:81 — a freshly created BIND journal carries begin.serial == end.serial == 0, so this branch quarantines it for any zone serial >= 1; BIND treats such a journal as empty (it compares offsets, not serials) and rolls forward fine → skip the quarantine when JournalBegin == JournalEnd == 0.
• nit: internal/bind/zonestate.go:364 — Quarantine's generated script is never executed by a test; TestMoveAsidePreservesEarlierQuarantine runs moveAside alone, without shellScript's set -e, so the quarantine-only (!WriteSeed) path has no end-to-end coverage → add a filesystem test for it beside the seed-script ones.

- nit: `internal/bind/zonestate.go:65` — a zone file whose SOA will not parse (0-byte, truncated, unknown layout) returns an empty plan, so `EnsureSeedZone` reports success and the zone then wedges at `AddZone` with an opaque BIND error → return `Blocked` with that reason, as the unreadable-journal case already does. - nit: `internal/bind/zonestate.go:81` — a freshly created BIND journal carries `begin.serial == end.serial == 0`, so this branch quarantines it for any zone serial >= 1; BIND treats such a journal as empty (it compares offsets, not serials) and rolls forward fine → skip the quarantine when `JournalBegin == JournalEnd == 0`. - nit: `internal/bind/zonestate.go:364` — `Quarantine`'s generated script is never executed by a test; `TestMoveAsidePreservesEarlierQuarantine` runs `moveAside` alone, without `shellScript`'s `set -e`, so the quarantine-only (`!WriteSeed`) path has no end-to-end coverage → add a filesystem test for it beside the seed-script ones.

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/bind-operator#20