From 8bfa5255686beca73b461265799e3c76432ca569 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Tue, 28 Apr 2026 15:13:49 -0300 Subject: [PATCH] test: cover imagemgr + dmsnap helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both packages had zero tests before this change. The helpers in them are pure (imagemgr) or scripted-runner-friendly (dmsnap), so they're cheap to pin and worth catching regressions on. imagemgr/paths_test.go: * DebianBasePackages returns a defensive copy (mutating the result can't poison subsequent calls — important because hashPackages digests this list). * BuildMetadataPackages stays in lockstep with DebianBasePackages. * hashPackages is order-sensitive and includes a trailing newline in its canonical join (regression guard for any future "sort the list before hashing" temptation that would invalidate every on-disk hash). * StageOptionalArtifactPath returns "" for empty/whitespace input and joins by name otherwise. * WritePackagesMetadata writes .packages.sha256 with the expected hash, no-ops on empty rootfs path or empty package list. * DebianBasePackages contains the small critical-package floor (ca-certificates, curl, git) so a future apt-list trim can't silently drop them. dmsnap/dmsnap_test.go: * Create runs losetup base, losetup cow, blockdev getsz, dmsetup create in that order, with a snapshot table referencing the loops in (base, cow) order — a swap would corrupt every VM. * Create's failure path unwinds with losetup -d on cow then base. * Cleanup tears down dmsetup before losetup (otherwise dmsetup sees EBUSY against vanished backing devices). * Cleanup falls back to DMDev when DMName is empty. * Cleanup tolerates "No such device" on losetup -d (idempotent re-run after a partial cleanup). * Cleanup surfaces non-missing losetup errors (the tolerance is narrow on purpose). * Remove returns nil on a missing target and surfaces non-retryable errors immediately. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/daemon/dmsnap/dmsnap_test.go | 288 +++++++++++++++++++++++++ internal/daemon/imagemgr/paths_test.go | 169 +++++++++++++++ 2 files changed, 457 insertions(+) create mode 100644 internal/daemon/dmsnap/dmsnap_test.go create mode 100644 internal/daemon/imagemgr/paths_test.go diff --git a/internal/daemon/dmsnap/dmsnap_test.go b/internal/daemon/dmsnap/dmsnap_test.go new file mode 100644 index 0000000..f179f2b --- /dev/null +++ b/internal/daemon/dmsnap/dmsnap_test.go @@ -0,0 +1,288 @@ +package dmsnap + +import ( + "context" + "errors" + "strings" + "testing" +) + +// scriptedRunner records every RunSudo call's argv and plays back a +// scripted sequence of (out, err) responses. Going past the script is +// a fatal error so an unexpected extra call shows up clearly. Mirrors +// the pattern used by internal/daemon/fcproc/fcproc_test.go but stays +// local to dmsnap (this is a leaf package). +type scriptedRunner struct { + t *testing.T + scripts []scriptedReply + calls [][]string +} + +type scriptedReply struct { + out []byte + err error +} + +func (r *scriptedRunner) RunSudo(_ context.Context, args ...string) ([]byte, error) { + r.t.Helper() + r.calls = append(r.calls, append([]string(nil), args...)) + if len(r.scripts) == 0 { + r.t.Fatalf("unexpected RunSudo call %d: %v", len(r.calls), args) + } + step := r.scripts[0] + r.scripts = r.scripts[1:] + return step.out, step.err +} + +func argsContain(args []string, want ...string) bool { + if len(args) < len(want) { + return false + } + for i, w := range want { + if args[i] != w { + return false + } + } + return true +} + +// TestCreateOrdersOpsAndPopulatesHandles pins the four-step setup +// sequence Create runs in: losetup base (read-only), losetup cow, +// blockdev getsz, dmsetup create with a snapshot table. If the order +// drifts the helper would build dm targets backed by the wrong +// device, which silently corrupts every VM that uses the snapshot. +func TestCreateOrdersOpsAndPopulatesHandles(t *testing.T) { + runner := &scriptedRunner{ + t: t, + scripts: []scriptedReply{ + {out: []byte("/dev/loop0\n")}, // losetup -f --show --read-only rootfs + {out: []byte("/dev/loop1\n")}, // losetup -f --show cow + {out: []byte("16384\n")}, // blockdev --getsz /dev/loop0 + {}, // dmsetup create + }, + } + + handles, err := Create(context.Background(), runner, "/state/rootfs.ext4", "/state/cow.img", "fc-rootfs-test") + if err != nil { + t.Fatalf("Create: %v", err) + } + + if len(runner.calls) != 4 { + t.Fatalf("got %d RunSudo calls, want 4", len(runner.calls)) + } + if !argsContain(runner.calls[0], "losetup", "-f", "--show", "--read-only", "/state/rootfs.ext4") { + t.Fatalf("call 0 = %v, want read-only losetup of rootfs", runner.calls[0]) + } + if !argsContain(runner.calls[1], "losetup", "-f", "--show", "/state/cow.img") { + t.Fatalf("call 1 = %v, want losetup of cow", runner.calls[1]) + } + if !argsContain(runner.calls[2], "blockdev", "--getsz", "/dev/loop0") { + t.Fatalf("call 2 = %v, want blockdev getsz on base loop", runner.calls[2]) + } + if !argsContain(runner.calls[3], "dmsetup", "create", "fc-rootfs-test") { + t.Fatalf("call 3 = %v, want dmsetup create of dm name", runner.calls[3]) + } + // The snapshot table must reference the base + cow loops in that + // order. Pin it so a future refactor can't accidentally swap them + // (which would make the COW the read-only side and corrupt every + // write). + tableArg := runner.calls[3][len(runner.calls[3])-1] + if !strings.Contains(tableArg, "snapshot /dev/loop0 /dev/loop1") { + t.Fatalf("dmsetup table = %q, want 'snapshot /dev/loop0 /dev/loop1'", tableArg) + } + + if handles.BaseLoop != "/dev/loop0" || handles.COWLoop != "/dev/loop1" { + t.Fatalf("loops = %+v, want base=loop0 cow=loop1", handles) + } + if handles.DMName != "fc-rootfs-test" || handles.DMDev != "/dev/mapper/fc-rootfs-test" { + t.Fatalf("dm names = %+v, want fc-rootfs-test", handles) + } +} + +// TestCreateFailureRunsCleanup verifies that a partial setup is +// unwound on failure: if dmsetup create fails after both loops are +// attached, Create must release them via losetup -d before returning. +// Without this the host accumulates orphan loop devices on every +// failed VM start. +func TestCreateFailureRunsCleanup(t *testing.T) { + dmCreateErr := errors.New("dmsetup table refused") + runner := &scriptedRunner{ + t: t, + scripts: []scriptedReply{ + {out: []byte("/dev/loop0\n")}, // losetup base + {out: []byte("/dev/loop1\n")}, // losetup cow + {out: []byte("16384\n")}, // blockdev getsz + {err: dmCreateErr}, // dmsetup create fails + {}, // cleanup: losetup -d /dev/loop1 + {}, // cleanup: losetup -d /dev/loop0 + }, + } + + _, err := Create(context.Background(), runner, "/state/rootfs.ext4", "/state/cow.img", "fc-rootfs-test") + if !errors.Is(err, dmCreateErr) { + t.Fatalf("Create error = %v, want dmsetup error to bubble", err) + } + if len(runner.calls) != 6 { + t.Fatalf("got %d RunSudo calls, want 6 (4 setup + 2 cleanup)", len(runner.calls)) + } + // Cleanup order: cow first, then base, mirroring stack unwind. + if !argsContain(runner.calls[4], "losetup", "-d", "/dev/loop1") { + t.Fatalf("call 4 = %v, want losetup -d on cow loop", runner.calls[4]) + } + if !argsContain(runner.calls[5], "losetup", "-d", "/dev/loop0") { + t.Fatalf("call 5 = %v, want losetup -d on base loop", runner.calls[5]) + } +} + +// TestCleanupOrdersDmsetupBeforeLosetup pins the destruction order: +// the dm target must come down BEFORE the loops it sits on are +// detached, otherwise dmsetup remove sees EBUSY because the target's +// backing devices vanished mid-flight. +func TestCleanupOrdersDmsetupBeforeLosetup(t *testing.T) { + runner := &scriptedRunner{ + t: t, + scripts: []scriptedReply{ + {}, // dmsetup remove fc-rootfs-test + {}, // losetup -d cow + {}, // losetup -d base + }, + } + + handles := Handles{ + BaseLoop: "/dev/loop0", + COWLoop: "/dev/loop1", + DMName: "fc-rootfs-test", + DMDev: "/dev/mapper/fc-rootfs-test", + } + if err := Cleanup(context.Background(), runner, handles); err != nil { + t.Fatalf("Cleanup: %v", err) + } + if len(runner.calls) != 3 { + t.Fatalf("got %d RunSudo calls, want 3", len(runner.calls)) + } + if !argsContain(runner.calls[0], "dmsetup", "remove", "fc-rootfs-test") { + t.Fatalf("call 0 = %v, want dmsetup remove first", runner.calls[0]) + } + if !argsContain(runner.calls[1], "losetup", "-d", "/dev/loop1") { + t.Fatalf("call 1 = %v, want cow loop detach second", runner.calls[1]) + } + if !argsContain(runner.calls[2], "losetup", "-d", "/dev/loop0") { + t.Fatalf("call 2 = %v, want base loop detach last", runner.calls[2]) + } +} + +// TestCleanupFallsBackToDMDevWhenNameEmpty covers the "we only know +// the /dev/mapper path" branch — Remove accepts either form, and +// Cleanup picks DMDev when DMName isn't recorded (older state files +// only stored the path). +func TestCleanupFallsBackToDMDevWhenNameEmpty(t *testing.T) { + runner := &scriptedRunner{ + t: t, + scripts: []scriptedReply{ + {}, // dmsetup remove /dev/mapper/fc-rootfs-test + {}, // losetup -d cow + {}, // losetup -d base + }, + } + handles := Handles{ + BaseLoop: "/dev/loop0", + COWLoop: "/dev/loop1", + DMDev: "/dev/mapper/fc-rootfs-test", + // DMName intentionally empty. + } + if err := Cleanup(context.Background(), runner, handles); err != nil { + t.Fatalf("Cleanup: %v", err) + } + if !argsContain(runner.calls[0], "dmsetup", "remove", "/dev/mapper/fc-rootfs-test") { + t.Fatalf("call 0 = %v, want dmsetup remove of DMDev path", runner.calls[0]) + } +} + +// TestCleanupTolerantOfMissingLoops pins the idempotency contract: +// running cleanup against handles whose loops are already detached +// (e.g. a daemon crash mid-cleanup, then a second pass) returns nil +// rather than failing. dmsnap.isMissing recognises kernel/losetup's +// "No such device" wording. +func TestCleanupTolerantOfMissingLoops(t *testing.T) { + missing := errors.New("losetup: /dev/loop1: No such device or address") + runner := &scriptedRunner{ + t: t, + scripts: []scriptedReply{ + {}, // dmsetup remove ok + {err: missing}, // losetup -d cow: already gone + {err: missing}, // losetup -d base: already gone + }, + } + handles := Handles{ + BaseLoop: "/dev/loop0", + COWLoop: "/dev/loop1", + DMName: "fc-rootfs-test", + } + if err := Cleanup(context.Background(), runner, handles); err != nil { + t.Fatalf("Cleanup: %v, want nil for already-gone loops", err) + } +} + +// TestCleanupSurfacesUnexpectedLoopErrors confirms that NON-missing +// errors do bubble up — the idempotency guard is narrow on purpose, +// so an EBUSY or permission error from losetup actually fails the +// cleanup. +func TestCleanupSurfacesUnexpectedLoopErrors(t *testing.T) { + wedged := errors.New("losetup: /dev/loop1: device is busy") + runner := &scriptedRunner{ + t: t, + scripts: []scriptedReply{ + {}, + {err: wedged}, + {}, + }, + } + handles := Handles{ + BaseLoop: "/dev/loop0", + COWLoop: "/dev/loop1", + DMName: "fc-rootfs-test", + } + err := Cleanup(context.Background(), runner, handles) + if !errors.Is(err, wedged) { + t.Fatalf("Cleanup error = %v, want busy error to bubble", err) + } +} + +// TestRemoveReturnsNilOnMissingTarget mirrors the loop-cleanup +// idempotency guard: an absent dm target is the desired end state, so +// Remove returns nil without retrying. +func TestRemoveReturnsNilOnMissingTarget(t *testing.T) { + missing := errors.New("dmsetup: target not found") + runner := &scriptedRunner{ + t: t, + scripts: []scriptedReply{ + {err: missing}, + }, + } + if err := Remove(context.Background(), runner, "fc-rootfs-test"); err != nil { + t.Fatalf("Remove: %v, want nil for missing target", err) + } + if len(runner.calls) != 1 { + t.Fatalf("got %d RunSudo calls, want 1 (missing should not retry)", len(runner.calls)) + } +} + +// TestRemoveBubblesNonRetryableErrors covers the third Remove branch: +// errors that aren't busy and aren't missing must surface immediately +// so the daemon can record the failure and clean up by other means. +func TestRemoveBubblesNonRetryableErrors(t *testing.T) { + denied := errors.New("dmsetup: permission denied") + runner := &scriptedRunner{ + t: t, + scripts: []scriptedReply{ + {err: denied}, + }, + } + err := Remove(context.Background(), runner, "fc-rootfs-test") + if !errors.Is(err, denied) { + t.Fatalf("Remove error = %v, want permission error to bubble", err) + } + if len(runner.calls) != 1 { + t.Fatalf("got %d RunSudo calls, want 1 (permission error should not retry)", len(runner.calls)) + } +} diff --git a/internal/daemon/imagemgr/paths_test.go b/internal/daemon/imagemgr/paths_test.go new file mode 100644 index 0000000..668eb8a --- /dev/null +++ b/internal/daemon/imagemgr/paths_test.go @@ -0,0 +1,169 @@ +package imagemgr + +import ( + "crypto/sha256" + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestDebianBasePackagesReturnsCopy pins the contract that mutating the +// slice returned by DebianBasePackages() can't poison subsequent calls. +// hashPackages digests this list, so a caller that sorts or appends in +// place would silently change every image's package metadata. +func TestDebianBasePackagesReturnsCopy(t *testing.T) { + t.Parallel() + first := DebianBasePackages() + original := append([]string(nil), first...) + if len(first) == 0 { + t.Fatal("DebianBasePackages returned empty slice") + } + first[0] = "tampered" + second := DebianBasePackages() + if second[0] == "tampered" { + t.Fatalf("DebianBasePackages leaks internal state; second[0] = %q after first[0] mutation", second[0]) + } + for i := range original { + if second[i] != original[i] { + t.Fatalf("DebianBasePackages drifted at %d: got %q, want %q", i, second[i], original[i]) + } + } +} + +// TestBuildMetadataPackagesMatchesDebianBase confirms the metadata +// packages used for image-drift detection are the same set we apply +// during build. If these diverge the hash recorded next to a rootfs +// stops matching the actual installed package set. +func TestBuildMetadataPackagesMatchesDebianBase(t *testing.T) { + t.Parallel() + build := BuildMetadataPackages() + debian := DebianBasePackages() + if len(build) != len(debian) { + t.Fatalf("BuildMetadataPackages len = %d, DebianBasePackages len = %d", len(build), len(debian)) + } + for i := range build { + if build[i] != debian[i] { + t.Fatalf("BuildMetadataPackages[%d] = %q, want %q", i, build[i], debian[i]) + } + } +} + +func TestHashPackagesStableForSameInput(t *testing.T) { + t.Parallel() + pkgs := []string{"git", "make", "vim"} + first := hashPackages(pkgs) + second := hashPackages(append([]string(nil), pkgs...)) + if first != second { + t.Fatalf("hashPackages drifted between identical calls: %q vs %q", first, second) + } + // Sanity: hash differs when input differs. + if first == hashPackages([]string{"git", "make"}) { + t.Fatal("hashPackages collapsed two distinct inputs to the same hash") + } + // Verify the format is hex sha256 of "git\nmake\nvim\n" — pin the + // concrete digest so a future refactor that changes joining (e.g. + // drops the trailing newline) trips this test. + want := fmt.Sprintf("%x", sha256.Sum256([]byte("git\nmake\nvim\n"))) + if first != want { + t.Fatalf("hashPackages format drifted: got %q, want %q", first, want) + } +} + +func TestStageOptionalArtifactPathEmptyStaysEmpty(t *testing.T) { + t.Parallel() + if got := StageOptionalArtifactPath("/tmp/artifacts", "", "initrd.img"); got != "" { + t.Fatalf("StageOptionalArtifactPath(empty staged) = %q, want empty", got) + } + if got := StageOptionalArtifactPath("/tmp/artifacts", " ", "initrd.img"); got != "" { + t.Fatalf("StageOptionalArtifactPath(whitespace staged) = %q, want empty", got) + } +} + +func TestStageOptionalArtifactPathJoinsName(t *testing.T) { + t.Parallel() + got := StageOptionalArtifactPath("/tmp/artifacts", "/host/path/initrd.img", "initrd.img") + want := filepath.Join("/tmp/artifacts", "initrd.img") + if got != want { + t.Fatalf("StageOptionalArtifactPath = %q, want %q", got, want) + } +} + +func TestWritePackagesMetadataWritesHashFile(t *testing.T) { + t.Parallel() + dir := t.TempDir() + rootfs := filepath.Join(dir, "rootfs.ext4") + if err := os.WriteFile(rootfs, []byte("rootfs"), 0o644); err != nil { + t.Fatalf("write rootfs: %v", err) + } + pkgs := []string{"git", "vim"} + if err := WritePackagesMetadata(rootfs, pkgs); err != nil { + t.Fatalf("WritePackagesMetadata: %v", err) + } + got, err := os.ReadFile(rootfs + ".packages.sha256") + if err != nil { + t.Fatalf("read metadata: %v", err) + } + want := hashPackages(pkgs) + "\n" + if string(got) != want { + t.Fatalf("metadata content = %q, want %q", got, want) + } +} + +func TestWritePackagesMetadataNoOpOnEmptyInputs(t *testing.T) { + t.Parallel() + dir := t.TempDir() + rootfs := filepath.Join(dir, "rootfs.ext4") + if err := os.WriteFile(rootfs, []byte("rootfs"), 0o644); err != nil { + t.Fatalf("write rootfs: %v", err) + } + + // Empty package list is the "managed-image build skipped apt" case. + if err := WritePackagesMetadata(rootfs, nil); err != nil { + t.Fatalf("WritePackagesMetadata(nil packages): %v", err) + } + if _, err := os.Stat(rootfs + ".packages.sha256"); !os.IsNotExist(err) { + t.Fatalf("metadata file was created for empty packages; err = %v", err) + } + + // Empty rootfs path is a no-op too — callers pass "" when they + // haven't decided where to write yet. + if err := WritePackagesMetadata("", []string{"git"}); err != nil { + t.Fatalf("WritePackagesMetadata(empty rootfs): %v", err) + } +} + +// TestHashPackagesIgnoresOrder confirms the canonical join is +// strict-order-sensitive: callers must keep the ordering they want the +// hash to digest. Pin this so a future "convenience" sort doesn't +// silently invalidate every recorded image hash on disk. +func TestHashPackagesOrderSensitive(t *testing.T) { + t.Parallel() + a := hashPackages([]string{"git", "make"}) + b := hashPackages([]string{"make", "git"}) + if a == b { + t.Fatal("hashPackages collapsed two orderings to the same hash; metadata-on-disk would be ambiguous") + } + // Trailing newlines must be normalised by the joiner, not the + // caller. If callers had to remember to add their own, every + // historical hash on disk would be a footgun. + withTrailing := hashPackages([]string{"git", "make", ""}) + if withTrailing == a { + t.Fatalf("hashPackages tolerated an empty trailing element silently; got %q == %q", withTrailing, a) + } +} + +// TestDebianBasePackagesContainsCriticalEntries pins the small core of +// packages every managed image must have. Stops a future refactor +// from dropping (say) ca-certificates without the owner noticing — a +// rebuilt image without it can't talk to TLS endpoints. +func TestDebianBasePackagesContainsCriticalEntries(t *testing.T) { + t.Parallel() + pkgs := strings.Join(DebianBasePackages(), " ") + for _, must := range []string{"ca-certificates", "curl", "git"} { + if !strings.Contains(pkgs, must) { + t.Errorf("DebianBasePackages missing critical entry %q; got %q", must, pkgs) + } + } +}