v0.111.0 — E-2c: the backup-target drive can no longer be ejected
A regression guard on a configuration that is live right now. E-1 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 existing eject role gate PASSED it. 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, with no alarm and
the box still reporting a configured tier. Found by E-2 Phase 0, not by a
failure.
handleDiskEject and handleDiskDecommission now call refuseIfBackupTarget AFTER
the role gate and refuse with 409, naming the storage and the remedy -- the op
is ordered, not forbidden: reassign the target first.
NOT a role reclassification, which is the obvious fix and the wrong one: making
RoleForStorage return RoleBackup would refuse every legitimate eject of the
customer's own data drive, because on both demo boxes that drive IS the target.
That trades a silent failure for a permanent obstruction.
backupTargetAt resolves through the agent's own storage view, never the caller's
claim, and fails OPEN -- safe because it sits behind the role gate, which fails
SAFE on the same error.
Red-proofed both ways, mutations verified to land first:
- removing the eject guard -> "eject of the backup-target drive SUCCEEDED (200)"
- the over-correction (any backup-content dir storage is the target)
-> the gate blocks /mnt/spare, failing TestEjectStillAllowedOnANonTargetDrive
Harness note: normalizeBackupTiers DROPS tiers with a nil Service and falls back
to the legacy empty-TargetID tier -- an earlier version of this test exercised
nothing and reported the production bug as if the fix had failed.
Green gate: build + vet + test rc=0 (29 packages), run separately from this commit.
This commit is contained in:
@@ -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())
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user