From ca0b169a4e3c4a91f802e69256e9ba9391e2f422 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 7 Jul 2026 18:42:19 +0200 Subject: [PATCH] feat(reconcile): re-assert pool membership after restore-over-existing (campaign-2 R2, v0.74.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pool membership is what lets the pool-scoped token reach a guest; pct restore --pool sets it only at CREATE, so a restore over an existing VMID drops the guest from the felhom pool and 403s the next restore-test/DR on VM.Audit. This empty-pool state is the true root cause of the campaign's "R1" (bind-mount restore failing was a symptom — restore-test's existing bind neutralization never ran without config-read). Add Client.PoolAddVMID (PUT /pools, additive+idempotent, Pool.Allocate) and call it in bring-up after liveness when spec.Pool!="" — warn-not-fail on a hiccup (liveness wins). B3 scratch-teardown 403 diagnosed as a cascade (restoretest already passes Pool). Role/ACL untouched. Tests + red-proof. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6 --- CHANGELOG.md | 26 ++++++++ REUSE.md | 1 + internal/proxmox/mutate.go | 31 ++++++++++ internal/proxmox/pool_test.go | 96 ++++++++++++++++++++++++++++++ internal/reconcile/bringup.go | 16 +++++ internal/reconcile/bringup_test.go | 88 +++++++++++++++++++++++++++ internal/reconcile/engine_test.go | 17 ++++++ internal/reconcile/state.go | 3 + 8 files changed, 278 insertions(+) create mode 100644 internal/proxmox/pool_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e75ce0..9a3dca6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,29 @@ +## v0.74.0 — pool membership re-asserted after restore-over-existing (campaign-2 R2) (2026-07-07) + +Closes campaign-2 finding **R2** (`felhom.eu/documentation/tests/CAMPAIGN-2-2026-07-07.md`). Pool +membership is what lets the pool-scoped `FelhomAgentGuest` token reach a guest (the grant applies only +to pool MEMBERS). `pct restore --pool` sets membership at CREATE, but a restore **over an existing +VMID** (the host-loss/finale path) never re-applies it — and no code re-added a guest to the pool — so +every destroy-restore silently dropped membership, and the NEXT `--selftest=restore-test`/DR 403'd on +`VM.Audit`/`VM.Allocate`. That empty-pool state is the true root cause of the campaign's "R1" (the +bind-mount restore failure was a symptom: with no config-read, restore-test's *existing, correct* +bind-mount neutralization never ran). + +- **`Client.PoolAddVMID(ctx, pool, vmid)`** (`internal/proxmox/mutate.go`): `PUT /pools/{pool}` + `vms={vmid}` — PVE-additive (merge, not replace), idempotent (already-member swallowed), needs + `Pool.Allocate` (the token has it). Sync — no UPID. +- **bring-up re-asserts** (`internal/reconcile/bringup.go`): after liveness is proven, if `spec.Pool + != ""`, call `PoolAddVMID`. A pool-add hiccup is surfaced as a LOUD warning + `res.StartWarnings` + but must NOT flip a healthy running guest's verdict (membership matters for the NEXT op). +- **B3 (scratch teardown 403) — diagnosed, no code:** `restoretest.go` already passes + `Pool: DefaultPool` for scratch restores, so the campaign's `VM.Allocate` teardown 403 was a + CASCADE of the bind-mount restore failing (half-built guest outside any pool), not an independent + gap. A re-run on the healed pool confirms. +- Tests: `PoolAddVMID` PUT shape + idempotency + real-error-surfaces + validation (`pool_test.go`); + bring-up re-asserts when `Pool!=""` (red-proof: pre-fix no-call → FAIL, demonstrated), no-pool→no-call, + pool-add failure warns-but-passes (liveness wins, guest kept). Role/ACL untouched — the fault was + membership, not privileges. + ## v0.73.0 — F2 mount-role fallback: enrolled user-data drives are ejectable/decommissionable again (2026-07-06) Closes campaign finding **F2** (`felhom.eu/documentation/audits/CAMPAIGN-nomercy-2026-07-06.md` + diff --git a/REUSE.md b/REUSE.md index 7778c2b..3218ab6 100644 --- a/REUSE.md +++ b/REUSE.md @@ -79,6 +79,7 @@ | `Client.WaitTask` | internal/proxmox/task.go | `WaitTask(ctx, upid, opts) (TaskStatus, error)` | asserting EVERY mutating op | POST 200 ≠ success; authz can fail at task exec; `AllowWarnings` opt-in | | `Client.Pool` | internal/proxmox/query.go | `Pool(ctx, name) (PoolInfo, error)` | felhom-pool membership (the ownership registry, A1) | Needs `Pool.Audit` at `/pool/` (host-install v1.9.0+); `Pool.Allocate` does NOT satisfy the read; members can be storages (type `storage`, vmid 0) — filter them | | `Client` mutate wrappers (`RestoreLXC/Vzdump/DestroyLXC/Snapshot/Rollback/SetConfig/ResizeLXC/Start/Stop`) | internal/proxmox/mutate.go | return `(upid, error)` | all API mutations | Async → always pair with WaitTask; route via gate/queue, not ad-hoc | +| `Client.PoolAddVMID` | internal/proxmox/mutate.go | `PoolAddVMID(ctx, pool, vmid) error` | re-assert pool membership after a restore-over-existing (campaign-2 R2) | SYNC (no UPID, don't WaitTask); PVE `PUT /pools` is additive (merge, not replace) — `delete=1` removes; idempotent (already-member swallowed); needs `Pool.Allocate` at `/pool/`. `pct restore --pool` sets membership only at CREATE — a restore over an existing vmid drops it, so bring-up re-asserts post-restore | | `TLSConfig.build` / `normalizeFingerprint` | internal/proxmox/tls.go | `build() (*tls.Config, error)` | PVE leaf-cert SHA-256 pinning | No insecure default | | `pinnedTLS` | internal/pbs/pin.go | `pinnedTLS(fingerprint) (*tls.Config, error)` | PBS leaf pinning | Same model as PVE; 64-hex fingerprint normalized | | `hub.Client.Report` | internal/hub/client.go | `Report(ctx, *HostReport) (*ControlEnvelope, error)` | the heartbeat | Typed `TransportError`/`HTTPError`, never contain the bearer token | diff --git a/internal/proxmox/mutate.go b/internal/proxmox/mutate.go index 5b67071..5888dca 100644 --- a/internal/proxmox/mutate.go +++ b/internal/proxmox/mutate.go @@ -6,6 +6,7 @@ import ( "net/http" "net/url" "strconv" + "strings" ) // Async mutating operations. Each is API-token-covered (the FelhomAgent role) and @@ -185,6 +186,36 @@ func (c *Client) SetConfig(ctx context.Context, vmid int, params map[string]stri return c.dataString(ctx, http.MethodPut, path, v) } +// PoolAddVMID adds a guest to a PVE pool via PUT /pools/{pool} with vms={vmid}. +// +// Membership is what makes a pool-scoped token (FelhomAgentGuest @ /pool/) reach a guest: the +// grant applies only to pool MEMBERS. `pct restore --pool` sets membership at CREATE, but a restore +// OVER AN EXISTING VMID (the P9/host-loss finale) does NOT re-apply it — so a destroy-restore +// silently drops the guest from the pool and 403s the NEXT restore-test/DR (campaign-2 R2). This +// re-asserts it after such a restore. +// +// PVE semantics: `PUT /pools/{poolid}` with `vms` is ADDITIVE (a merge) — `delete=1` is required to +// REMOVE, so passing a single vmid adds it without disturbing existing members. Adding a guest that +// is already a member is treated as a no-op success (idempotent): PVE reports "already" in the error +// body, which we swallow. Requires Pool.Allocate at /pool/ (the token has it). +func (c *Client) PoolAddVMID(ctx context.Context, pool string, vmid int) error { + if pool == "" || vmid == 0 { + return fmt.Errorf("proxmox: PoolAddVMID needs pool and vmid") + } + v := url.Values{} + v.Set("vms", strconv.Itoa(vmid)) + path := "/pools/" + url.PathEscape(pool) + _, err := c.dataString(ctx, http.MethodPut, path, v) + if err != nil { + // Idempotent: a guest already in the pool is success, not a failure. + if ae, ok := err.(*APIError); ok && strings.Contains(strings.ToLower(ae.Body), "already") { + return nil + } + return err + } + return nil +} + // ResizeLXC grows a guest volume via PUT /nodes/{node}/lxc/{vmid}/resize // (token-covered: VM.Config.Disk + Datastore.AllocateSpace). Returns the UPID. // diff --git a/internal/proxmox/pool_test.go b/internal/proxmox/pool_test.go new file mode 100644 index 0000000..18c206f --- /dev/null +++ b/internal/proxmox/pool_test.go @@ -0,0 +1,96 @@ +package proxmox + +import ( + "context" + "io" + "net/http" + "testing" +) + +// PoolAddVMID issues PUT /pools/{pool} with vms={vmid} and treats an already-member error as success. +func TestPoolAddVMID_PUTShape(t *testing.T) { + var gotMethod, gotPath, gotVMS string + d := &mockDoer{fn: func(r *http.Request) (*http.Response, error) { + gotMethod = r.Method + gotPath = r.URL.Path + body, _ := io.ReadAll(r.Body) // the client encodes url.Values into the request body + for _, kv := range splitAmp(string(body)) { + if k, v, ok := cut(kv, "="); ok && k == "vms" { + gotVMS = v + } + } + return jsonResp(http.StatusOK, `{"data":null}`), nil + }} + c := newTestClient(d) + if err := c.PoolAddVMID(context.Background(), "felhom", 9201); err != nil { + t.Fatalf("PoolAddVMID: %v", err) + } + if gotMethod != http.MethodPut { + t.Errorf("method = %q, want PUT", gotMethod) + } + if gotPath != "/api2/json/pools/felhom" { + t.Errorf("path = %q, want /api2/json/pools/felhom", gotPath) + } + if gotVMS != "9201" { + t.Errorf("vms param = %q, want 9201", gotVMS) + } +} + +// Idempotent: an "already in pool" error from PVE is swallowed as success. +func TestPoolAddVMID_IdempotentOnAlreadyMember(t *testing.T) { + d := &mockDoer{fn: func(_ *http.Request) (*http.Response, error) { + return jsonResp(http.StatusInternalServerError, `{"data":null,"errors":{"vms":"VM 9201 is already in pool 'felhom'"}}`), nil + }} + c := newTestClient(d) + if err := c.PoolAddVMID(context.Background(), "felhom", 9201); err != nil { + t.Fatalf("already-member should be idempotent success, got: %v", err) + } +} + +// A real failure (e.g. a 403 with no "already") is surfaced, not swallowed. +func TestPoolAddVMID_RealErrorSurfaces(t *testing.T) { + d := &mockDoer{fn: func(_ *http.Request) (*http.Response, error) { + return jsonResp(http.StatusForbidden, `Permission check failed (/pool/felhom, Pool.Allocate)`), nil + }} + c := newTestClient(d) + if err := c.PoolAddVMID(context.Background(), "felhom", 9201); err == nil { + t.Fatal("expected a real 403 to surface, got nil") + } +} + +func TestPoolAddVMID_Validation(t *testing.T) { + c := newTestClient(&mockDoer{fn: func(_ *http.Request) (*http.Response, error) { + t.Fatal("should not issue a request on bad input") + return nil, nil + }}) + if err := c.PoolAddVMID(context.Background(), "", 9201); err == nil { + t.Error("empty pool should error") + } + if err := c.PoolAddVMID(context.Background(), "felhom", 0); err == nil { + t.Error("zero vmid should error") + } +} + +// tiny helpers (avoid pulling strings.Split into an assertion path that could mask a bug) +func splitAmp(s string) []string { + var out []string + cur := "" + for _, r := range s { + if r == '&' { + out = append(out, cur) + cur = "" + continue + } + cur += string(r) + } + return append(out, cur) +} + +func cut(s, sep string) (string, string, bool) { + for i := 0; i+len(sep) <= len(s); i++ { + if s[i:i+len(sep)] == sep { + return s[:i], s[i+len(sep):], true + } + } + return s, "", false +} diff --git a/internal/reconcile/bringup.go b/internal/reconcile/bringup.go index 40593d6..92d2224 100644 --- a/internal/reconcile/bringup.go +++ b/internal/reconcile/bringup.go @@ -344,6 +344,22 @@ func (e *Engine) runBringUp(ctx context.Context, spec BringUpSpec, res *BringUpR return } + // 6b. Re-assert pool membership (campaign-2 R2). `pct restore --pool` sets membership only at + // CREATE; a restore OVER AN EXISTING VMID (host-loss finale) does not re-apply it, silently + // dropping the guest from the pool and 403-ing the NEXT restore-test/DR. Idempotent on a + // fresh-VMID restore that already got membership. This runs AFTER liveness is proven: a + // pool-add hiccup is surfaced LOUD as a warning but must NOT flip a healthy, running guest's + // verdict to fail (membership matters for the next op, not this guest's boot). + if spec.Pool != "" { + if err := e.api.PoolAddVMID(ctx, spec.Pool, spec.VMID); err != nil { + e.logger.Error("bring-up: pool membership re-assert FAILED (next restore-test/DR may 403); guest is healthy", + "vmid", spec.VMID, "pool", spec.Pool, "err", err) + res.StartWarnings = append(res.StartWarnings, fmt.Sprintf("pool re-assert failed (pool=%s): %v", spec.Pool, err)) + } else { + e.logger.Info("bring-up: pool membership re-asserted", "vmid", spec.VMID, "pool", spec.Pool) + } + } + // 7. Success — KEEP the guest; mark the owning entry terminal so Recover ignores it. res.Pass = true res.Verified = "boot+running" diff --git a/internal/reconcile/bringup_test.go b/internal/reconcile/bringup_test.go index b925c39..a0decdc 100644 --- a/internal/reconcile/bringup_test.go +++ b/internal/reconcile/bringup_test.go @@ -629,3 +629,91 @@ func lockBackoffFast(t *testing.T) { configLockBackoff = time.Millisecond t.Cleanup(func() { configLockBackoff = prev }) } + +// --- Part B (campaign-2 R2): pool re-assertion after bring-up ------------------------------------ + +// poolAddsFor returns the recorded PoolAddVMID calls for a vmid. +func poolAddsFor(api *fakeAPI, vmid int) []poolAddCall { + var out []poolAddCall + for _, c := range api.poolAdds { + if c.vmid == vmid { + out = append(out, c) + } + } + return out +} + +// A bring-up with a Pool set MUST re-assert pool membership after the restore (so a +// restore-over-existing that dropped membership is healed for the NEXT restore-test/DR). +// COMPANION RED-PROOF: on the pre-fix code (no PoolAddVMID call) poolAddsFor is empty → this FAILS. +func TestRunBringUp_ReassertsPoolMembership(t *testing.T) { + const vmid = 8100 + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeDRGuestLoss, Archive: "local:backup/cust.tar.zst", VMID: vmid, + RestoreStorage: "local-lvm", Pool: "felhom", + }) + if res.Err != nil || !res.Pass { + t.Fatalf("bring-up must pass, got %+v", res) + } + adds := poolAddsFor(api, vmid) + if len(adds) != 1 || adds[0].pool != "felhom" { + t.Fatalf("pool membership not re-asserted: poolAdds=%+v", api.poolAdds) + } +} + +// No Pool → no PoolAddVMID call (a broad-token restore needs no pool). +func TestRunBringUp_NoPool_NoPoolAdd(t *testing.T) { + const vmid = 8101 + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "local:backup/golden.tar.zst", VMID: vmid, + RestoreStorage: "local-lvm", Hostname: "h", + }) + if res.Err != nil || !res.Pass { + t.Fatalf("bring-up must pass, got %+v", res) + } + if len(api.poolAdds) != 0 { + t.Fatalf("no Pool set → PoolAddVMID must not be called: %+v", api.poolAdds) + } +} + +// A pool-add FAILURE must NOT flip a healthy, running guest's verdict — it surfaces as a LOUD warning +// (membership matters for the NEXT op, not this guest's boot). Liveness wins. +func TestRunBringUp_PoolAddFailure_WarnsButPasses(t *testing.T) { + const vmid = 8102 + api := &fakeAPI{ + cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}, + poolAddErr: errors.New("proxmox: PUT /pools/felhom -> HTTP 500: transient"), + } + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeDRGuestLoss, Archive: "local:backup/cust.tar.zst", VMID: vmid, + RestoreStorage: "local-lvm", Pool: "felhom", + }) + if res.Err != nil || !res.Pass { + t.Fatalf("a pool-add hiccup must NOT fail a healthy guest, got %+v", res) + } + // the guest is KEPT (not torn down) despite the pool-add failure. + if len(api.destroys) != 0 { + t.Fatalf("pool-add failure must not trigger teardown: %+v", api.destroys) + } + // the failure is surfaced as a warning. + found := false + for _, w := range res.StartWarnings { + if strings.Contains(w, "pool re-assert failed") { + found = true + } + } + if !found { + t.Fatalf("pool-add failure must surface as a warning: %+v", res.StartWarnings) + } +} diff --git a/internal/reconcile/engine_test.go b/internal/reconcile/engine_test.go index b6f3e12..235e318 100644 --- a/internal/reconcile/engine_test.go +++ b/internal/reconcile/engine_test.go @@ -49,6 +49,23 @@ type fakeAPI struct { waits []string waitOpts []proxmox.WaitOptions // parallel to waits: the options each WaitTask was called with listErr error + + // poolAdds records (pool, vmid) for each PoolAddVMID; poolAddErr backs the failure path. + poolAdds []poolAddCall + poolAddErr error +} + +type poolAddCall struct { + pool string + vmid int +} + +func (f *fakeAPI) PoolAddVMID(_ context.Context, pool string, vmid int) error { + f.mu.Lock() + f.poolAdds = append(f.poolAdds, poolAddCall{pool: pool, vmid: vmid}) + err := f.poolAddErr + f.mu.Unlock() + return err } type resizeCall struct { diff --git a/internal/reconcile/state.go b/internal/reconcile/state.go index 46118d4..7b7059b 100644 --- a/internal/reconcile/state.go +++ b/internal/reconcile/state.go @@ -172,6 +172,9 @@ type GuestAPI interface { ResizeLXC(ctx context.Context, vmid int, disk, size string) (string, error) // RestoreLXC restores an archive into a (fresh) vmid — the create path (slice 6). Async → UPID. RestoreLXC(ctx context.Context, opts proxmox.RestoreLXCOptions) (string, error) + // PoolAddVMID re-asserts pool membership after a restore-over-existing (campaign-2 R2). Sync (no + // UPID); idempotent. Membership is what lets the pool-scoped token reach the guest next time. + PoolAddVMID(ctx context.Context, pool string, vmid int) error // DestroyLXC destroys a guest — the scratch-teardown primitive (slice 6). Async → UPID. // Destructive-class; the engine only ever issues it for an agent-tagged scratch guest // (benign by provenance) via the gate.