diff --git a/CHANGELOG.md b/CHANGELOG.md index b2088fc..cb5a43e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,38 @@ +## v0.111.0 — E-2c: the backup-target drive can no longer be ejected out from under the backup (2026-07-29) + +**A regression guard on a configuration that is live right now.** E-1 (2026-07-28) moved each demo +box's whole-guest vzdump target onto its secondary drive, at that drive's own mountpoint. But +`RoleForStorage` types a `local-dir` on a non-system device as **user-data** — so the pre-existing +eject role gate PASSED it, and `POST /disks/eject` on `/mnt/nvme-1tb` (demo-hp) or `/mnt/hdd_1` +(demo-felhom) would have **succeeded silently**, taking the only local whole-guest backup with it. +No alarm, no refusal; the box would keep reporting a configured tier while having lost its +drive-loss protection. Found by E-2's Phase 0, not by a failure. + +`handleDiskEject` and `handleDiskDecommission` now consult `refuseIfBackupTarget` **after** the role +gate and refuse with `409` when the mount backs a configured backup tier, naming the storage and the +remedy — the op is ordered, not forbidden: reassign the target first, then the drive is free. + +**Why this is NOT a role reclassification, which is the obvious fix and the wrong one.** Making +`RoleForStorage` return `RoleBackup` for the target would refuse every legitimate eject of the +customer's own data drive, because on both demo boxes that drive **is** the target (the vzdump +target sits beside `felhom-data` on the same mountpoint). That trades a silent failure for a +permanent obstruction. The gate is therefore separate and narrow, and the role vocabulary is +untouched. + +`backupTargetAt` resolves through the agent's OWN storage view, never the caller's claim, and fails +**OPEN** — safe precisely because it sits behind the role gate, which already fails **SAFE** on the +same error, so an unresolvable mount is refused before it reaches here. + +**Tests + red-proofs.** Eject refused, decommission refused, and — the one that constrains the +design — `TestEjectStillAllowedOnANonTargetDrive` pins that a non-target drive stays ejectable. +Red-proofed both ways: removing the eject guard reproduces `eject of the backup-target drive +SUCCEEDED (200)`, and implementing the over-correction (treat any backup-content dir storage as the +target) fails the non-target test with the gate blocking `/mnt/spare`. + +**Harness note worth keeping:** `normalizeBackupTiers` DROPS any tier with a nil `Service` and falls +back to the legacy tier with an empty `TargetID`. An earlier version of this test therefore exercised +nothing and reported the production bug as if it were the fix failing. + ## v0.110.0 — F-LEAK, third attempt: the fourth root-fenced exception (2026-07-28) **The band-scoped ACL fix (v1.21.0) is durable for exactly ONE use per slot, and the live check caught diff --git a/internal/localapi/backup_target_guard_test.go b/internal/localapi/backup_target_guard_test.go new file mode 100644 index 0000000..f5a987c --- /dev/null +++ b/internal/localapi/backup_target_guard_test.go @@ -0,0 +1,108 @@ +package localapi + +import ( + "context" + "io" + "log/slog" + "net/http" + "strings" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" +) + +// backupTargetServer builds a server whose PRIMARY tier is `felhom-backup`, mounted at /mnt/nvme-1tb +// on its own non-system device — i.e. the exact live shape E-1 created on demo-hp and demo-felhom: +// the drive is simultaneously the enrolled user-data drive AND the whole-guest vzdump target. +func backupTargetServer(t *testing.T) http.Handler { + t.Helper() + sv := fakeStorage{targets: []hub.StorageTarget{ + { + Name: "felhom-backup", Type: hub.StorageTypeLocalDir, State: hub.StorageStateAttached, + Reachable: true, MountPath: "/mnt/nvme-1tb", BackingDevice: "/dev/nvme0n1", + Content: "backup", + }, + { + Name: "spare-drive", Type: hub.StorageTypeLocalDir, State: hub.StorageStateAttached, + Reachable: true, MountPath: "/mnt/spare", BackingDevice: "/dev/sdz1", + Content: "backup", + }, + }} + srv, err := NewServer(Options{ + ListenAddr: "127.0.0.1:0", + Guests: &fakeGuests{}, Backups: &fakeBackups{}, Store: &fakeStore{}, Storage: sv, + Tokens: staticTokens{"A": 8200}, + Disks: &fakeDiskOps{}, DiskGate: &fakeGate{}, HostReader: sysOnSDA(), + // Service is load-bearing: normalizeBackupTiers DROPS any tier with a nil Service and falls + // back to the legacy single tier with an empty TargetID — which silently made an earlier + // version of this test exercise nothing. + BackupTiers: []BackupTier{ + {TargetID: "felhom-backup", Cadence: 24 * time.Hour, Primary: true, Service: &fakeBackups{}}, + {TargetID: "felhom-pbs", Cadence: 168 * time.Hour, Service: &fakeBackups{}}, + }, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatalf("new server: %v", err) + } + srv.baseCtx = context.Background() + srv.now = func() time.Time { return testNow } + return srv.Handler() +} + +// E-2c — ejecting the drive that holds the only local whole-guest backup must be refused. +// +// This is a REGRESSION GUARD on a live configuration, not a hypothetical. E-1 (2026-07-28) moved the +// vzdump target onto each demo box's secondary drive, and `RoleForStorage` types a local-dir on a +// non-system device as user-data — so the pre-existing role gate PASSES it and the customer could +// self-serve eject the drive holding their backups. It would have succeeded silently. +// +// The assertion is on the CONSEQUENCE (the request is refused) plus the remedy being named, because a +// refusal the customer cannot act on just moves the failure. +func TestEjectRefusedOnTheBackupTargetDrive(t *testing.T) { + h := backupTargetServer(t) + rr := do(t, h, http.MethodPost, "/disks/eject", "A", `{"vmid":8200,"where":"/mnt/nvme-1tb"}`) + if rr.Code == http.StatusOK { + t.Fatalf("eject of the backup-target drive SUCCEEDED (%d) — the box would silently lose its "+ + "local drive-loss protection with nothing alarming", rr.Code) + } + body := rr.Body.String() + if !strings.Contains(body, "felhom-backup") { + t.Errorf("refusal must NAME the backup target so the customer knows which role blocks it; got: %s", body) + } + if !strings.Contains(strings.ToLower(body), "reassign") { + t.Errorf("refusal must name the REMEDY (reassign the target first), else it is a dead end; got: %s", body) + } +} + +// Decommission strands the target just as thoroughly as eject — it migrates data off and retires the +// drive. Same gate, asserted separately because it is a different handler and a different caller. +func TestDecommissionRefusedOnTheBackupTargetDrive(t *testing.T) { + h := backupTargetServer(t) + rr := do(t, h, http.MethodPost, "/disks/decommission", "A", `{"vmid":8200,"where":"/mnt/nvme-1tb"}`) + if rr.Code == http.StatusOK { + t.Fatalf("decommission of the backup-target drive SUCCEEDED (%d)", rr.Code) + } + if !strings.Contains(rr.Body.String(), "felhom-backup") { + t.Errorf("refusal must name the backup target; got: %s", rr.Body.String()) + } +} + +// THE OVER-CORRECTION GUARD, and the reason this is a narrow gate instead of a role reclassification. +// +// The tempting fix — make RoleForStorage return RoleBackup for the target — would also refuse every +// OTHER user-data drive op on a box, and on the demo boxes it would refuse the customer's own data +// drive, because that drive IS the target. This pins that a non-target drive stays ejectable: the new +// gate must block exactly one drive, not harden the whole eject path. +// +// It asserts "not blocked BY THIS GATE" rather than "succeeds", because eject has other legitimate +// failure modes in a fake harness; what must never appear is this gate's message. +func TestEjectStillAllowedOnANonTargetDrive(t *testing.T) { + h := backupTargetServer(t) + rr := do(t, h, http.MethodPost, "/disks/eject", "A", `{"vmid":8200,"where":"/mnt/spare"}`) + if strings.Contains(rr.Body.String(), "whole-guest backup target") { + t.Fatalf("the backup-target gate blocked a NON-target drive (/mnt/spare) — over-correction: "+ + "it must block exactly the target, not harden the whole eject path; got: %s", rr.Body.String()) + } +} diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index dd832d8..3293483 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -378,6 +378,11 @@ func (s *Server) handleDiskEject(w http.ResponseWriter, r *http.Request, vmid in writeErr(w, http.StatusForbidden, "mount is system/backup-protected — eject refused (role: "+string(role)+")") return } + // E-2c: the role gate above passes a drive that is BOTH user-data and the vzdump target (E-1 put + // the target on the enrolled drive's own mountpoint). Refuse specifically, naming the remedy. + if s.refuseIfBackupTarget(r.Context(), w, "eject", vmid, req.Where) { + return + } dependents := s.dependentGuests(r.Context(), req.Where) // Record the EJECT intent BEFORE unmounting (the target still resolves to its durable-id) so the // self-heal watchdog leaves it alone — an OFFICIAL eject is the only thing that sets this (P3); an @@ -431,6 +436,11 @@ func (s *Server) handleDiskDecommission(w http.ResponseWriter, r *http.Request, writeErr(w, http.StatusForbidden, "mount is system/backup-protected — decommission refused (role: "+string(role)+")") return } + // E-2c: same narrow gate as eject — decommission migrates data off and retires the drive, which + // would strand the backup target just as thoroughly. + if s.refuseIfBackupTarget(r.Context(), w, "decommission", vmid, req.Where) { + return + } dependents := s.dependentGuests(r.Context(), req.Where) // Resolve the durable-id BEFORE unmounting (it still resolves while mounted) for the bind prune. id := s.durableIDForMount(r.Context(), req.Where) @@ -1041,6 +1051,62 @@ func (s *Server) hostReader() storage.HostReader { return storage.NewProcHostReader() } +// backupTargetAt reports the configured backup TIER whose storage is mounted at `where`, or "" when +// none is. E-2c. +// +// WHY THIS IS NOT A ROLE RECLASSIFICATION. The obvious fix is to make RoleForStorage return +// RoleBackup for the target's storage, and it is wrong here: on both demo boxes the drive that now +// holds the whole-guest archives is ALSO the enrolled user-data drive (E-1 put the vzdump target on +// the drive's own mountpoint, beside felhom-data). Reclassifying it would refuse every legitimate +// eject/decommission of the customer's own data drive — an over-correction that trades one silent +// failure for a permanent obstruction. So this is a SEPARATE, narrower gate that names exactly what +// it protects and leaves the role vocabulary alone. +// +// It resolves through the agent's OWN storage view (never the caller's claim) and fails OPEN — an +// unreadable view returns "" so this gate cannot block on a transient error. That is safe because it +// sits BEHIND the role gate, which already fails SAFE on the same error: an unresolvable mount is +// refused there before it ever reaches this check. +func (s *Server) backupTargetAt(ctx context.Context, where string) string { + if where == "" || s.storage == nil { + return "" + } + targets, err := s.storage.Observe(ctx) + if err != nil { + return "" // fail OPEN — the role gate already fails SAFE on this same error + } + for _, t := range s.tiers { + if t.TargetID == "" { + continue + } + for _, tgt := range targets { + if tgt.Name == t.TargetID && tgt.MountPath == where { + return t.TargetID + } + } + } + return "" +} + +// refuseIfBackupTarget refuses a destructive drive op when `where` backs a configured backup tier, +// and reports whether it did. The message names the storage AND the remedy: the operation is not +// forbidden forever, it is ordered — reassign the backup target first, then the drive is free. +// +// Ejecting the drive that holds the only local whole-guest backup is exactly the silent-degradation +// class this arc has been closing: it succeeds, nothing alarms, and the box quietly loses its +// drive-loss protection while still reporting a configured tier. +func (s *Server) refuseIfBackupTarget(ctx context.Context, w http.ResponseWriter, op string, vmid int, where string) bool { + target := s.backupTargetAt(ctx, where) + if target == "" { + return false + } + s.logger.Warn("local-api: protected — "+op+" refused: the mount backs a configured backup tier", + "vmid", vmid, "where", where, "target", target) + writeErr(w, http.StatusConflict, + "this drive is the whole-guest backup target ("+target+") — "+op+" refused. "+ + "Reassign the backup target to another drive first, or the box loses its local drive-loss protection.") + return true +} + // roleForMountPath resolves the AUTHORITATIVE protection role of the storage mounted at `where`, from // the agent's OWN storage view + host topology (never the caller's claim). It mirrors deviceRole but // keys on the mount path (the eject input). It FAILS SAFE to system (most-protected) on any