diff --git a/api/v1alpha1/bindcluster_types.go b/api/v1alpha1/bindcluster_types.go index ff6b40e..9fc5518 100644 --- a/api/v1alpha1/bindcluster_types.go +++ b/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"` diff --git a/config/crd/bases/bind.unkin.net_bindclusters.yaml b/config/crd/bases/bind.unkin.net_bindclusters.yaml index bec1ab9..759e14e 100644 --- a/config/crd/bases/bind.unkin.net_bindclusters.yaml +++ b/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. diff --git a/config/crd/install.yaml b/config/crd/install.yaml index 429f51a..407d905 100644 --- a/config/crd/install.yaml +++ b/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. diff --git a/internal/bind/seed.go b/internal/bind/seed.go index 2a3ee76..a7a8425 100644 --- a/internal/bind/seed.go +++ b/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 diff --git a/internal/bind/seed_test.go b/internal/bind/seed_test.go index 4e44a9a..71aaa9e 100644 --- a/internal/bind/seed_test.go +++ b/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) + } +} diff --git a/internal/bind/zonestate.go b/internal/bind/zonestate.go index 5465676..dd3cddc 100644 --- a/internal/bind/zonestate.go +++ b/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") +}