Abort the seed and probe scripts on the first failed command

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

api/v1alpha1/bindcluster_types.go

@@ -59,7 +59,8 @@ type BindClusterSpec struct {
 	Replicas int32 `json:"replicas,omitempty"`
 
 	// Image is the BIND9 container image. Must ship named, rndc, nsupdate and
-	// the POSIX tools the operator execs: sh, mkdir, dirname, head, od, tr, mv.
+	// the POSIX tools the operator execs: sh, mkdir, dirname, cat, head, od,
+	// tr, wc, rm, mv.
 	// +kubebuilder:default="internetsystemsconsortium/bind9:9.20"
 	// +optional
 	Image string `json:"image,omitempty"`

config/crd/bases/bind.unkin.net_bindclusters.yaml

@@ -997,7 +997,8 @@ spec:
                 default: internetsystemsconsortium/bind9:9.20
                 description: |-
                   Image is the BIND9 container image. Must ship named, rndc, nsupdate and
-                  the POSIX tools the operator execs: sh, mkdir, dirname, head, od, tr, mv.
+                  the POSIX tools the operator execs: sh, mkdir, dirname, cat, head, od,
+                  tr, wc, rm, mv.
                 type: string
               imagePullPolicy:
                 description: ImagePullPolicy for the BIND container.

config/crd/install.yaml

@@ -1307,7 +1307,8 @@ spec:
                 default: internetsystemsconsortium/bind9:9.20
                 description: |-
                   Image is the BIND9 container image. Must ship named, rndc, nsupdate and
-                  the POSIX tools the operator execs: sh, mkdir, dirname, head, od, tr, mv.
+                  the POSIX tools the operator execs: sh, mkdir, dirname, cat, head, od,
+                  tr, wc, rm, mv.
                 type: string
               imagePullPolicy:
                 description: ImagePullPolicy for the BIND container.

internal/bind/seed.go

@@ -83,14 +83,18 @@ const seedTempSuffix = ".seed-tmp"
 // installs it in that order. Doing all of it in one exec keeps an interrupted
 // seed from leaving the PVC with no zone data, which the next reconcile would
 // read as a fresh install and reseed at serial 1; the rename means a torn write
-// is never visible at path.
+// is never visible at path. shellScript aborts the run at the first failure, so
+// no step can install the skeleton over data an earlier step failed to preserve.
 func seedScript(path string, plan SeedPlan, size int) string {
 	q, tmp := shellQuote(path), shellQuote(path+seedTempSuffix)
 	cmds := []string{
 		fmt.Sprintf("mkdir -p \"$(dirname %s)\"", q),
 		fmt.Sprintf("cat > %s", tmp),
 		// A stdin stream cut mid-transfer gives cat a short file and exit 0.
-		fmt.Sprintf("if [ \"$(wc -c < %s | tr -d ' \\n')\" -ne %d ]; then rm -f %s; exit 1; fi", tmp, size, tmp),
+		fmt.Sprintf("n=$(wc -c < %s | tr -d ' \\n')", tmp),
+		// Compared as strings: an unmeasurable size is empty, not a number, and
+		// a numeric test would exit 2 there and be swallowed by the if.
+		fmt.Sprintf("if [ \"$n\" != '%d' ]; then rm -f %s; exit 1; fi", size, tmp),
 	}
 	if plan.QuarantineZoneFile {
 		cmds = append(cmds, moveAside(path, plan.QuarantineSuffix))
@@ -98,7 +102,7 @@ func seedScript(path string, plan SeedPlan, size int) string {
 	if plan.QuarantineJournal {
 		cmds = append(cmds, moveAside(JournalPath(path), plan.QuarantineSuffix))
 	}
-	return strings.Join(append(cmds, fmt.Sprintf("mv -- %s %s", tmp, q)), "\n")
+	return shellScript(append(cmds, fmt.Sprintf("mv -- %s %s", tmp, q))...)
 }
 
 // AddCatalogMember registers a member zone in a catalog zone by adding the

internal/bind/seed_test.go

@@ -39,12 +39,60 @@ func inFlightZone(t *testing.T) (dir, path string) {
 }
 
 func runSeedScript(t *testing.T, sh, path string, plan SeedPlan, content, stdin string) error {
+	t.Helper()
+	return runSeedScriptWithPath(t, sh, path, plan, content, stdin, "")
+}
+
+func runSeedScriptWithPath(t *testing.T, sh, path string, plan SeedPlan, content, stdin, pathEnv string) error {
 	t.Helper()
 	cmd := exec.Command(sh, "-c", seedScript(path, plan, len(content)))
 	cmd.Stdin = strings.NewReader(stdin)
+	if pathEnv != "" {
+		cmd.Env = append(os.Environ(), "PATH="+pathEnv)
+	}
 	return cmd.Run()
 }
 
+// shimPath puts a stand-in for tool at the front of a PATH, so the generated
+// script meets a failing command where a real image would meet a working one.
+func shimPath(t *testing.T, tool, body string) string {
+	t.Helper()
+	dir := t.TempDir()
+	if err := os.WriteFile(filepath.Join(dir, tool), []byte("#!/bin/sh\n"+body+"\n"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	return dir + string(os.PathListSeparator) + os.Getenv("PATH")
+}
+
+func realTool(t *testing.T, tool string) string {
+	t.Helper()
+	p, err := exec.LookPath(tool)
+	if err != nil {
+		t.Skipf("need %s: %v", tool, err)
+	}
+	return p
+}
+
+// assertZoneUntouched checks the in-flight layout survived a failed run whole:
+// live serial 1 still at path, journal still there, nothing quarantined and no
+// staging file left behind.
+func assertZoneUntouched(t *testing.T, dir, path string) {
+	t.Helper()
+	found := siblings(t, dir)
+	serial, ok := ParseZoneSerial(found[filepath.Base(path)])
+	if !ok || serial != 1 {
+		t.Errorf("zone file was replaced by a failed run: serial = (%d,%v)", serial, ok)
+	}
+	if _, ok := found[filepath.Base(JournalPath(path))]; !ok {
+		t.Error("the journal was lost by a failed run")
+	}
+	for name := range found {
+		if strings.Contains(name, quarantineMarker) {
+			t.Errorf("a failed run must not leave a quarantined file: %s", name)
+		}
+	}
+}
+
 func probeState(t *testing.T, sh, path string) ZoneDiskState {
 	t.Helper()
 	out, err := exec.Command(sh, "-c", zoneStateProbe(path)).Output()
@@ -166,3 +214,66 @@ func keys(m map[string]string) []string {
 	}
 	return out
 }
+
+// A rename that fails leaves live data where it is, so the install must not
+// happen: the skeleton beside a higher-serial journal is the production failure
+// this seed exists to avoid.
+func TestSeedScriptFailedQuarantineAbortsInstall(t *testing.T) {
+	sh := requireShell(t, "wc", "tr", "mv", "rm", "mkdir", "dirname")
+	dir, path := inFlightZone(t)
+	pathEnv := shimPath(t, "mv", `case "$*" in *`+quarantineMarker+`*) exit 1;; esac
+exec `+realTool(t, "mv")+` "$@"`)
+
+	plan := PlanSeed(probeState(t, sh, path))
+	content := renderSeedZone("example.com", "10.0.0.1", plan.Serial)
+	if err := runSeedScriptWithPath(t, sh, path, plan, content, content, pathEnv); err == nil {
+		t.Fatal("a failed quarantine must fail the seed")
+	}
+	assertZoneUntouched(t, dir, path)
+}
+
+// The size guard has to fail closed: an image without wc cannot measure the
+// staged file, and an unverified file must never be installed.
+func TestSeedScriptUnmeasurableStagingAbortsInstall(t *testing.T) {
+	sh := requireShell(t, "wc", "tr", "mv", "rm", "mkdir", "dirname")
+	dir, path := inFlightZone(t)
+	pathEnv := shimPath(t, "wc", "exit 127")
+
+	plan := PlanSeed(probeState(t, sh, path))
+	content := renderSeedZone("example.com", "10.0.0.1", plan.Serial)
+	if err := runSeedScriptWithPath(t, sh, path, plan, content, content, pathEnv); err == nil {
+		t.Fatal("an unmeasurable staging file must fail the seed")
+	}
+	assertZoneUntouched(t, dir, path)
+	if _, ok := siblings(t, dir)[filepath.Base(path)+seedTempSuffix]; ok {
+		t.Error("the staging file should have been cleaned up")
+	}
+}
+
+func TestSeedScriptFailedMkdirAbortsInstall(t *testing.T) {
+	sh := requireShell(t, "wc", "tr", "mv", "rm", "mkdir", "dirname")
+	dir, path := inFlightZone(t)
+	pathEnv := shimPath(t, "mkdir", "exit 1")
+
+	plan := PlanSeed(probeState(t, sh, path))
+	content := renderSeedZone("example.com", "10.0.0.1", plan.Serial)
+	if err := runSeedScriptWithPath(t, sh, path, plan, content, content, pathEnv); err == nil {
+		t.Fatal("a failed mkdir must fail the seed")
+	}
+	assertZoneUntouched(t, dir, path)
+}
+
+// The probe decides whether there is anything to preserve, so a tool it cannot
+// run must be an error rather than a state that reads as "nothing readable".
+func TestZoneStateProbeFailedToolIsAnError(t *testing.T) {
+	sh := requireShell(t, "head", "od", "tr")
+	_, path := inFlightZone(t)
+	pathEnv := shimPath(t, "tr", "exit 127")
+
+	cmd := exec.Command(sh, "-c", zoneStateProbe(path))
+	cmd.Env = append(os.Environ(), "PATH="+pathEnv)
+	out, err := cmd.Output()
+	if err == nil {
+		t.Fatalf("probe reported success without reading the journal header: %q", out)
+	}
+}

internal/bind/zonestate.go

@@ -87,7 +87,9 @@ func PlanSeed(st ZoneDiskState) SeedPlan {
 
 // highestSerial reports the furthest serial on disk. The second result is false
 // when no serial could be read at all: 0 is a legitimate serial, so it cannot
-// double as "nothing found" in RFC 1982 sequence space.
+// double as "nothing found" in RFC 1982 sequence space. RFC 1982 ordering is
+// not total, so the fold is order-dependent once the inputs span more than
+// 2^31; a zone cannot advance that far between reconciles.
 func highestSerial(st ZoneDiskState) (int64, bool) {
 	var h int64
 	var known bool
@@ -250,7 +252,9 @@ const (
 	probeHeadShut   = ">>head"
 	probeOrphanOpen = "orphans<<"
 	probeOrphanShut = ">>orphans"
-	// quarantineMarker joins a preserved file to the serial it was holding.
+	// quarantineMarker joins a preserved file to the serial floor a reseed must
+	// stay above, which highestSerial may take from a sibling rather than from
+	// the renamed file itself.
 	quarantineMarker = ".orphaned-"
 )
 
@@ -258,18 +262,20 @@ const (
 // record in both layouts the operator has to read.
 func zoneStateProbe(path string) string {
 	zone, jnl := shellQuote(path), shellQuote(JournalPath(path))
-	return strings.Join([]string{
+	return shellScript(
 		fmt.Sprintf("printf '%s\\n'", probeBegin),
-		fmt.Sprintf("if [ -f %s ]; then printf 'zonefile=1\\n%s\\n'; head -c 4096 %s || exit 1; printf '\\n%s\\n'; else printf 'zonefile=0\\n'; fi",
+		fmt.Sprintf("if [ -f %s ]; then printf 'zonefile=1\\n%s\\n'; head -c 4096 %s; printf '\\n%s\\n'; else printf 'zonefile=0\\n'; fi",
 			zone, probeHeadOpen, zone, probeHeadShut),
-		fmt.Sprintf("if [ -f %s ]; then printf 'journal=1\\n'; hdr=$(od -An -v -tx1 -N32 %s) || exit 1; printf 'jnl=%%s\\n' \"$(printf '%%s' \"$hdr\" | tr -d ' \\n')\"; else printf 'journal=0\\n'; fi",
+		// The hex is folded in its own assignment so a failing tr aborts the
+		// probe instead of reporting an unreadable journal header.
+		fmt.Sprintf("if [ -f %s ]; then printf 'journal=1\\n'; hdr=$(od -An -v -tx1 -N32 %s); hdr=$(printf '%%s' \"$hdr\" | tr -d ' \\n'); printf 'jnl=%%s\\n' \"$hdr\"; else printf 'journal=0\\n'; fi",
 			jnl, jnl),
 		// The quarantine siblings outlive the files they replaced, so they are
 		// the floor a reseed must stay above after an interrupted transition.
 		fmt.Sprintf("printf '%s\\n'\nfor f in %s* %s*; do if [ -e \"$f\" ]; then printf '%%s\\n' \"$f\"; fi; done\nprintf '%s\\n'",
 			probeOrphanOpen, shellQuote(path+quarantineMarker), shellQuote(JournalPath(path)+quarantineMarker), probeOrphanShut),
 		fmt.Sprintf("printf '%s\\n'", probeEnd),
-	}, "\n")
+	)
 }
 
 // parseZoneDiskState returns false unless the probe output is complete: a
@@ -366,7 +372,7 @@ func (e *Executor) Quarantine(ctx context.Context, namespace, pod, path string,
 	if len(cmds) == 0 {
 		return nil
 	}
-	cmd := []string{"sh", "-c", strings.Join(cmds, "\n")}
+	cmd := []string{"sh", "-c", shellScript(cmds...)}
 	if out, err := e.Exec(ctx, namespace, pod, cmd, ""); err != nil {
 		return fmt.Errorf("quarantine zone files %s: %w (out: %s)", path, err, out)
 	}
@@ -386,3 +392,11 @@ func moveAside(path, suffix string) string {
 func shellQuote(s string) string {
 	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
 }
+
+// shellScript joins commands into a script that stops at the first failure and
+// exits non-zero. A plain script runs every line regardless of the previous
+// one's status, which would let an install proceed over a failed quarantine and
+// still report success to the caller.
+func shellScript(cmds ...string) string {
+	return strings.Join(append([]string{"set -e"}, cmds...), "\n")
+}