From 508a1c02dab9b984a7e763131b186cca2ced2ed0 Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Wed, 23 Sep 2026 07:49:00 +0800 Subject: [PATCH] fix(api): refuse backup/restore before a missing world volume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A server whose world PVC does not exist yet (never started) or no longer exists (the world was already reaped) accepted the backup/restore POST, answered 202, and the Job sat Pending on the missing claim until its deadline with nothing recorded anywhere — a silent no-op from the operator's seat. The live drill on the reaped `resolvecheck` world reproduced exactly that. Both handlers now read the world PVC (Cluster.WorldVolumeExists, over the same naming.WorldPVCName the Jobs mount) and answer a specific 409 no_world_volume with "start it once to create it, then retry". The felis-api Role gains the matching get-only PVC grant — the first live run surfaced the missing RBAC as a 403 behind a 500, so the fix ships with it. Live (auditfix38): resolvecheck -> 409 no_world_volume on both faces; test-one (which has a world) still backs up through the new gate end to end. --- internal/api/api_test.go | 10 ++++++- internal/api/cluster.go | 6 ++++ internal/api/handlers_backup_now_test.go | 30 ++++++++++++++++++++ internal/api/handlers_backups.go | 35 ++++++++++++++++++++++++ internal/api/handlers_backups_test.go | 15 ++++++++++ internal/api/k8scluster.go | 16 +++++++++++ internal/platform/rbac.go | 10 +++++-- internal/platform/rbac_test.go | 8 ++++++ 8 files changed, 127 insertions(+), 3 deletions(-) diff --git a/internal/api/api_test.go b/internal/api/api_test.go index cf5fa99..fa2a7c5 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -1451,6 +1451,7 @@ type fakeCluster struct { desired map[string]v1alpha1.DesiredState created map[string]CreateServerInput // name -> the validated input it was created from patched map[string]ServerSpecPatch // name -> the validated spec patch it received + noWorld map[string]bool // server names modeled WITHOUT a world volume (never started / reaped) createErr error pingErr error } @@ -1458,7 +1459,7 @@ type fakeCluster struct { func newFakeCluster() *fakeCluster { return &fakeCluster{byName: map[string]*ServerInfo{}, bySub: map[string]*ServerInfo{}, desired: map[string]v1alpha1.DesiredState{}, created: map[string]CreateServerInput{}, - patched: map[string]ServerSpecPatch{}} + patched: map[string]ServerSpecPatch{}, noWorld: map[string]bool{}} } func (c *fakeCluster) GetServer(_ context.Context, n string) (*ServerInfo, error) { if s, ok := c.byName[n]; ok { @@ -1474,6 +1475,13 @@ func (c *fakeCluster) GetBySubdomain(_ context.Context, s string) (*ServerInfo, } func (c *fakeCluster) ListServers(_ context.Context) ([]ServerInfo, error) { return c.list, nil } func (c *fakeCluster) Ping(_ context.Context) error { return c.pingErr } + +// WorldVolumeExists models the world PVC: present unless the test named the +// server in noWorld (never started / already reaped). +func (c *fakeCluster) WorldVolumeExists(_ context.Context, n string) (bool, error) { + return !c.noWorld[n], nil +} + func (c *fakeCluster) SetDesiredState(_ context.Context, n string, s v1alpha1.DesiredState) error { c.desired[n] = s return nil diff --git a/internal/api/cluster.go b/internal/api/cluster.go index d1d88c2..d8b6f9e 100644 --- a/internal/api/cluster.go +++ b/internal/api/cluster.go @@ -81,6 +81,12 @@ type Cluster interface { // GetServer reads one MinecraftServer's lifecycle view, or ErrNotFound. GetServer(ctx context.Context, name string) (*ServerInfo, error) + // WorldVolumeExists reports whether the server's world PVC exists in the + // server namespace. A server that never started — or whose world the + // retention reaper already archived and deleted — has no claim, and a + // backup/restore Job would hang Pending on the missing volume with nothing + // ever recorded, so both handlers refuse those up front. + WorldVolumeExists(ctx context.Context, name string) (bool, error) // GetBySubdomain finds the MinecraftServer whose spec.subdomain matches, or // ErrNotFound. GetBySubdomain(ctx context.Context, subdomain string) (*ServerInfo, error) diff --git a/internal/api/handlers_backup_now_test.go b/internal/api/handlers_backup_now_test.go index 42af752..d641856 100644 --- a/internal/api/handlers_backup_now_test.go +++ b/internal/api/handlers_backup_now_test.go @@ -134,6 +134,22 @@ func TestBackupNow(t *testing.T) { } }) + t.Run("no world volume -> 409 no_world_volume, no backup", func(t *testing.T) { + // A never-started (or reaped) server has no world PVC: the Job would hang + // Pending on the missing claim with nothing recorded, so the gate must + // refuse before the backuper is reached. + api, _, cl, backuper := mk() + cl.noWorld["survival"] = true + api.External = staticExternal{p: owner} + w := do(api.ExternalHandler(), "POST", path, "", nil) + if w.Code != http.StatusConflict || decodeErr(t, w) != "no_world_volume" { + t.Fatalf("code = %d body %s", w.Code, w.Body.String()) + } + if backuper.calls != 0 { + t.Fatal("a world-less server must not reach the backuper") + } + }) + t.Run("nil Backuper -> 503 backup_unavailable", func(t *testing.T) { api, _, _, _ := mk() api.Backuper = nil @@ -240,6 +256,20 @@ func TestInternalBackup(t *testing.T) { } }) + t.Run("no world volume -> 409 no_world_volume, no backup", func(t *testing.T) { + // The break-glass face shares enqueueBackup, so the world-volume gate must + // hold here too — this is the face the TUI's Sync picker drives. + api, _, cl, backuper := mk() + cl.noWorld["survival"] = true + w := do(api.InternalHandler(), "POST", path, "", jsonHeader) + if w.Code != http.StatusConflict || decodeErr(t, w) != "no_world_volume" { + t.Fatalf("code = %d body %s", w.Code, w.Body.String()) + } + if backuper.calls != 0 { + t.Fatal("a world-less server must not reach the backuper") + } + }) + t.Run("nil Backuper -> 503 backup_unavailable", func(t *testing.T) { api, _, _, _ := mk() api.Backuper = nil diff --git a/internal/api/handlers_backups.go b/internal/api/handlers_backups.go index bd4b877..247bd2e 100644 --- a/internal/api/handlers_backups.go +++ b/internal/api/handlers_backups.go @@ -9,6 +9,16 @@ import ( "felis.lolicon.best/internal/naming" ) +// errNoWorldVolume is the shared 409 for backup and restore when the server's +// world PVC does not exist: the Job would only hang Pending on the missing +// claim — invisible to the caller and to the backups list — so the handlers +// refuse up front. Starting the server once (which creates the claim via the +// StatefulSet volumeClaimTemplate) unlocks both ops. +func errNoWorldVolume() error { + return newError(http.StatusConflict, "no_world_volume", + "this server has no world volume yet — start it once to create it, then retry") +} + // handleListBackups lists the world backups visible to the caller (spec §7 GET // /api/v1/backups; world_backups in §22). It is app-tier: an admin sees every // present backup; a regular user sees only the backups of worlds they formerly @@ -154,6 +164,19 @@ func (a *API) handleRestoreBackup(w http.ResponseWriter, r *http.Request) { return } + // World-volume gate: the restore Job mounts the world PVC read-write to unpack + // the archive into it, so a missing claim means a Pod stuck Pending — a 202 + // "restoring" with nothing ever written. Same refusal as the backup face + // (shared errNoWorldVolume): the operator starts the server once to create the + // claim, then restores into it. + if exists, err := a.Cluster.WorldVolumeExists(r.Context(), name); err != nil { + writeError(w, r, err) + return + } else if !exists { + writeError(w, r, errNoWorldVolume()) + return + } + // Restorer is optional: when unwired the endpoint reports 503 rather than // panicking, so the authorization boundary above is exercised even before the // restore-Job executor is wired (see Restorer). @@ -285,6 +308,18 @@ func (a *API) enqueueBackup(w http.ResponseWriter, r *http.Request, name string, return } + // World-volume gate: the Job mounts the world PVC by claim name, and a missing + // claim would leave its Pod Pending — a 202 "backing_up" with nothing ever + // recorded anywhere. A never-started or already-reaped server is refused with + // the same specificity as the stopped gate. + if exists, err := a.Cluster.WorldVolumeExists(r.Context(), name); err != nil { + writeError(w, r, err) + return + } else if !exists { + writeError(w, r, errNoWorldVolume()) + return + } + // Backuper is optional: when unwired the endpoint reports 503 rather than // panicking, so the authorization boundary above is exercised even before the // backup-Job executor is wired (see Backuper). diff --git a/internal/api/handlers_backups_test.go b/internal/api/handlers_backups_test.go index 6f99edf..63d85e8 100644 --- a/internal/api/handlers_backups_test.go +++ b/internal/api/handlers_backups_test.go @@ -269,6 +269,21 @@ func TestRestoreBackup(t *testing.T) { } }) + t.Run("no world volume -> 409 no_world_volume, no restore", func(t *testing.T) { + // Restoring into a missing world PVC would leave the Job Pending on the + // missing claim — a 202 "restoring" that never writes anything. + api, _, cl, restorer := mk() + cl.noWorld["survival"] = true + api.External = staticExternal{p: owner} + w := do(api.ExternalHandler(), "POST", path, "", nil) + if w.Code != http.StatusConflict || decodeErr(t, w) != "no_world_volume" { + t.Fatalf("code = %d body %s", w.Code, w.Body.String()) + } + if restorer.calls != 0 { + t.Fatal("a world-less server must not reach the restorer") + } + }) + t.Run("nil Restorer -> 503 restore_unavailable", func(t *testing.T) { api, _, _, _ := mk() api.Restorer = nil diff --git a/internal/api/k8scluster.go b/internal/api/k8scluster.go index ce45d94..d306a3a 100644 --- a/internal/api/k8scluster.go +++ b/internal/api/k8scluster.go @@ -43,6 +43,22 @@ func (k *K8sCluster) GetServer(ctx context.Context, name string) (*ServerInfo, e return serverInfo(&ms), nil } +// WorldVolumeExists reads the world PVC the operator's StatefulSet +// volumeClaimTemplate creates (naming.WorldPVCName — the same name the backup +// and restore Jobs mount), so existence here is exactly existence at Job mount +// time. NotFound is (false, nil): the caller refuses with a specific 409. +func (k *K8sCluster) WorldVolumeExists(ctx context.Context, name string) (bool, error) { + var pvc corev1.PersistentVolumeClaim + err := k.c.Get(ctx, types.NamespacedName{Namespace: k.namespace, Name: naming.WorldPVCName(name)}, &pvc) + if apierrors.IsNotFound(err) { + return false, nil + } + if err != nil { + return false, err + } + return true, nil +} + func (k *K8sCluster) GetBySubdomain(ctx context.Context, subdomain string) (*ServerInfo, error) { var list v1alpha1.MinecraftServerList if err := k.c.List(ctx, &list, client.InNamespace(k.namespace)); err != nil { diff --git a/internal/platform/rbac.go b/internal/platform/rbac.go index a915e62..0d4292e 100644 --- a/internal/platform/rbac.go +++ b/internal/platform/rbac.go @@ -84,8 +84,11 @@ func ControlPlaneRBAC(p Params) RBAC { // FINISHED Job whose name still blocks a retry can be replaced), and stream the // live console for the read side (internal/api.logstream — pods:list to find // the server's running pod, then pods/log:get to follow it; spec §8 读=pods/log -// follow). felis-api uses a DIRECT client, so it needs no list/watch beyond the -// explicit List calls. +// follow). It also Gets the world PVC before backup/restore +// (internal/api.k8scluster.WorldVolumeExists) so a never-started or reaped +// world is refused up front instead of leaving a Job Pending on a missing +// claim. felis-api uses a DIRECT client, so it needs no list/watch beyond the +// explicit List calls — and the PVC grant is get-only, mirroring that. // // The read-side grant is deliberately minimal: pods:list + pods/log:get, NOT // pods:get — the streamer lists pods by the server label then reads the chosen @@ -97,6 +100,9 @@ func APIMinecraftRole(p Params) *rbacv1.Role { return role(p.MinecraftNamespace, "felis-api", ComponentAPI, []rbacv1.PolicyRule{ rule([]string{groupFelis}, []string{"minecraftservers"}, []string{"get", "list", "create", "patch"}), rule([]string{groupCore}, []string{"secrets"}, []string{"get"}), + // get-only: WorldVolumeExists does a single direct Get of the world PVC; + // nothing in felis-api lists or deletes PVCs. + rule([]string{groupCore}, []string{"persistentvolumeclaims"}, []string{"get"}), // list backs GET /servers/{name}/jobs — the async status outlet reads the // backup/restore Jobs back by the server label (read-only). rule([]string{groupBatch}, []string{"jobs"}, []string{"create", "get", "delete", "list"}), diff --git a/internal/platform/rbac_test.go b/internal/platform/rbac_test.go index f78ce36..a91ca58 100644 --- a/internal/platform/rbac_test.go +++ b/internal/platform/rbac_test.go @@ -121,6 +121,14 @@ func TestAPIRole_MinecraftPowersExact(t *testing.T) { if !hasRule(mc, groupCore, "secrets", "get") { t.Error("felis-api must read RCON secrets (secrets:get) for console writes") } + // WorldVolumeExists (backup/restore pre-gate) does a single direct PVC Get; + // nothing in felis-api lists or deletes claims. + if !hasRule(mc, groupCore, "persistentvolumeclaims", "get") { + t.Error("felis-api must get the world PVC (persistentvolumeclaims:get) for the backup/restore world-volume gate") + } + if hasRule(mc, groupCore, "persistentvolumeclaims", "list") || hasRule(mc, groupCore, "persistentvolumeclaims", "delete") { + t.Error("felis-api must NOT list or delete PVCs (the gate is a single direct Get)") + } // Read-side console (spec §8 读=pods/log follow): list pods to find the // running pod, then read its log subresource — and nothing wider. if !hasRule(mc, groupCore, "pods", "list") {