Unverified Commit 90ccbfed authored by Lemon-miaow's avatar Lemon-miaow
Browse files

fix(restore): replace a finished Job so retries enqueue; replicate felis-config 

An E2E audit on a live install found that a FAILED restore held its
deterministic Job name for the rest of the 10-minute TTL, so the next
restore answered 202 'restoring' while nothing ran (ErrAlreadyExists was
treated as success unconditionally). K8sJobs now inspects the colliding
Job: in-flight still coalesces, finished (succeeded or failed) is
deleted and replaced. The minecraft-namespace Role gains jobs:get/delete
for exactly that replacement.

The same audit found the backup Job mounts the felis-config Secret but
the installer only provisions it in the control namespace, so every
backup Job stranded on FailedMount. felis setup now replicates it into
the minecraft namespace beside the service-token and forwarding
secrets.
parent fd0794d0

cmd/felis/setup.go

+10 −5
Changes for cmd/felis/setup.go: 10 added lines, 5 removed lines.
Original line number Diff line number Diff line
@@ -225,16 +225,21 @@ func provisionSystemServers(ctx context.Context, cfg *config.Config, out io.Writ 
	// renamed it must replicate the Secret by hand.

	controlNS := platform.DefaultControlNamespace

	apiBaseURL := platform.InternalAPIBaseURL(controlNS)

	// Both Secrets must land in the minecraft namespace before the pods that mount

	// them are created: the service token (login authenticates to felis-api with it)

	// and the Velocity forwarding secret (every backend verifies the proxy's signed

	// handshake with it — without it the login gate would derive an OFFLINE UUID and

	// the Owner would bind the wrong Minecraft identity).

	// These Secrets must land in the minecraft namespace before the pods that

	// mount them are created: the service token (login authenticates to felis-api

	// with it), the Velocity forwarding secret (every backend verifies the proxy's

	// signed handshake with it — without it the login gate would derive an OFFLINE

	// UUID and the Owner would bind the wrong Minecraft identity), and felis-config

	// (the on-demand BACKUP Job runs in the minecraft namespace and mounts it to

	// self-record its world_backups row; without the replica the Job's volume

	// mount fails and every backup request strands in the cluster).

	secretOutcomes := []systemServerOutcome{

		ensureSecretReplica(ctx, cl, controlNS, cfg.K8s.Namespace,

			naming.ServiceTokenSecretName, naming.ServiceTokenSecretKey, "service-token"),

		ensureSecretReplica(ctx, cl, controlNS, cfg.K8s.Namespace,

			naming.ForwardingSecretName, naming.ForwardingSecretKey, "forwarding-secret"),

		ensureSecretReplica(ctx, cl, controlNS, cfg.K8s.Namespace,

			"felis-config", "felis.toml", "config"),

	}

	outcomes := ensureSystemServers(ctx, cl, cfg.K8s.Namespace, cfg.Velocity.LoginImage, cfg.Velocity.LobbyImage, apiBaseURL, cfg.Server.RootDomain, defaultPanelHostname(cfg.Server.RootDomain, cfg.Auth.PanelHostname))

	outcomes = append(secretOutcomes, outcomes...)

internal/platform/rbac.go

+8 −6
Changes for internal/platform/rbac.go: 8 added lines, 6 removed lines.
Original line number Diff line number Diff line
@@ -74,11 +74,13 @@ func ControlPlaneRBAC(p Params) RBAC { 
// APIMinecraftRole grants felis-api exactly what it does in the minecraft

// namespace: drive MinecraftServer specs (internal/api.k8scluster — get/list/

// create/patch, never status), read RCON passwords for console writes

// (internal/api.console — secrets:get), create the restore Job

// (internal/restore — jobs:create), 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.

// (internal/api.console — secrets:get), manage the restore Job under its

// deterministic name (internal/restore — jobs:create, plus get/delete so a

// 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.

//

// 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
@@ -90,7 +92,7 @@ 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"}),

		rule([]string{groupBatch}, []string{"jobs"}, []string{"create"}),

		rule([]string{groupBatch}, []string{"jobs"}, []string{"create", "get", "delete"}),

		// Read-side console (spec §8 读=pods/log follow): list pods to find the

		// server's running pod, then read its log subresource. Two separate rules so

		// the verbs stay tight — list on pods, get on pods/log, and nothing else.

internal/platform/rbac_test.go

+4 −2
Changes for internal/platform/rbac_test.go: 4 added lines, 2 removed lines.
Original line number Diff line number Diff line
@@ -73,8 +73,10 @@ func TestAPIRole_CreatesJobsInBothNamespaces(t *testing.T) { 
	if mc.Namespace != "minecraft" {

		t.Errorf("felis-api minecraft Role namespace = %q, want minecraft", mc.Namespace)

	}

	if !hasRule(mc, "batch", "jobs", "create") {

		t.Error("felis-api (minecraft) must have batch/jobs:create for the restore Job")

	for _, v := range []string{"create", "get", "delete"} {

		if !hasRule(mc, "batch", "jobs", v) {

			t.Errorf("felis-api (minecraft) must have batch/jobs:%s for the restore-Job lifecycle", v)

		}

	}



	build := roleByName(t, rbac.Roles, "felis-api-builds")

internal/restore/k8sjobs.go

+63 −7
Changes for internal/restore/k8sjobs.go: 63 added lines, 7 removed lines.
Original line number Diff line number Diff line
@@ -3,16 +3,19 @@ package restore 
import (

	"context"



	batchv1 "k8s.io/api/batch/v1"

	apierrors "k8s.io/apimachinery/pkg/api/errors"

	"k8s.io/apimachinery/pkg/types"

	"sigs.k8s.io/controller-runtime/pkg/client"

)



// K8sJobs is the production Jobs backed by a controller-runtime client (spec

// §7, §16). It creates the world-restore Job — nothing more: the restore Job is

// one-shot and self-cleaning (ttlSecondsAfterFinished), so there is no phase or

// cancel seam, and thus no config to hold (unlike build.K8sJobs, which needs the

// namespace to read and delete its Job). Every restore parameter arrives in the

// JobParams the Restorer builds from its own (defaulted) Config. The

// §7, §16). It creates the world-restore Job and, when a previous run's finished

// Job still holds the deterministic name, replaces it (see CreateRestoreJob).

// There is no other phase or cancel seam: the Job is one-shot and self-cleaning

// (ttlSecondsAfterFinished), so unlike build.K8sJobs there is no namespace to

// hold — every restore parameter arrives in the JobParams the Restorer builds

// from its own (defaulted) Config. The

// cluster-bootstrap objects (the weak felis-restore SA) are installed once by

// the deployment manifests (spec §21), not per restore, so this binding never

// creates them. It is integration-tested against a live cluster, not the
@@ -29,13 +32,52 @@ func NewK8sJobs(c client.Client) *K8sJobs { 


// CreateRestoreJob renders and applies the restore Job. Its name is a

// deterministic function of the server (RestoreJobName), so a concurrent restore

// of the same server collides on Create; that collision is mapped to

// ErrAlreadyExists, which the Restorer treats as success (idempotent enqueue).

// of the same server collides on Create. The collision is answered by the state

// of the Job already holding the name:

//

//   - still running (or not yet started): ErrAlreadyExists, which the Restorer

//     treats as success — the idempotent coalesce.

//   - finished (succeeded OR failed): the finished Job is deleted and replaced,

//     so the caller's retry enqueues for real. Without this, the deterministic

//     name plus the ten-minute TTL would swallow the retry — most importantly

//     the retry after a FAILED restore, which must not have to wait out the TTL

//     (an E2E audit found exactly that: a retry answered 202 "restoring" while

//     nothing ran).

func (k *K8sJobs) CreateRestoreJob(ctx context.Context, p JobParams) error {

	job, err := RestoreJob(p)

	if err != nil {

		return err

	}

	createErr := k.c.Create(ctx, job)

	if createErr == nil {

		return nil

	}

	if !apierrors.IsAlreadyExists(createErr) {

		return createErr

	}



	var existing batchv1.Job

	getErr := k.c.Get(ctx, types.NamespacedName{Namespace: job.Namespace, Name: job.Name}, &existing)

	if apierrors.IsNotFound(getErr) {

		// The name freed itself (TTL cleanup raced us); one retry.

		return k.recreate(ctx, job)

	}

	if getErr != nil {

		return getErr

	}

	if !restoreJobFinished(&existing) {

		return ErrAlreadyExists

	}

	if deleteErr := k.c.Delete(ctx, &existing); deleteErr != nil && !apierrors.IsNotFound(deleteErr) {

		return deleteErr

	}

	return k.recreate(ctx, job)

}



// recreate retries Create once after a finished Job released the name. A

// collision that survives means a concurrent restore re-created first, so the

// idempotent answer applies again.

func (k *K8sJobs) recreate(ctx context.Context, job *batchv1.Job) error {

	if err := k.c.Create(ctx, job); err != nil {

		if apierrors.IsAlreadyExists(err) {

			return ErrAlreadyExists
@@ -44,3 +86,17 @@ func (k *K8sJobs) CreateRestoreJob(ctx context.Context, p JobParams) error { 
	}

	return nil

}



// restoreJobFinished reports whether the Job has reached a terminal state. A Job

// that is merely created-but-not-started (no active pods yet, no completions)

// counts as in flight, not finished, so a duplicate enqueue during startup still

// coalesces.

func restoreJobFinished(job *batchv1.Job) bool {

	if job.Status.Active > 0 {

		return false

	}

	if job.Status.CompletionTime != nil {

		return true

	}

	return job.Status.Succeeded > 0 || job.Status.Failed > 0

}

internal/restore/k8sjobs_test.go

0 → 100644
+40 −0
Changes for internal/restore/k8sjobs_test.go: 40 added lines, 0 removed lines.
Original line number Diff line number Diff line 
package restore



import (

	"testing"

	"time"



	batchv1 "k8s.io/api/batch/v1"

	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

)



// restoreJobFinished decides whether a name collision is a genuine in-flight

// coalesce (ErrAlreadyExists) or a finished Job whose deterministic name must be

// replaced so a retry enqueues for real. Regression: a FAILED restore used to

// absorb every retry for the rest of its ten-minute TTL — the API answered 202

// "restoring" while nothing ran (found by an E2E audit against a live cluster).

func TestRestoreJobFinished(t *testing.T) {

	completion := metav1.NewTime(time.Now())

	cases := []struct {

		name string

		job  batchv1.Job

		want bool

	}{

		{"running", batchv1.Job{Status: batchv1.JobStatus{Active: 1}}, false},

		{"created-not-started", batchv1.Job{}, false},

		{"succeeded", batchv1.Job{Status: batchv1.JobStatus{

			Succeeded: 1, CompletionTime: &completion,

			Conditions: []batchv1.JobCondition{{Type: batchv1.JobComplete, Status: "True"}},

		}}, true},

		{"failed", batchv1.Job{Status: batchv1.JobStatus{

			Failed:     1,

			Conditions: []batchv1.JobCondition{{Type: batchv1.JobFailed, Status: "True"}},

		}}, true},

		{"failed-and-some-active", batchv1.Job{Status: batchv1.JobStatus{Active: 1, Failed: 1}}, false},

	}

	for _, tc := range cases {

		if got := restoreJobFinished(&tc.job); got != tc.want {

			t.Errorf("%s: restoreJobFinished = %v, want %v", tc.name, got, tc.want)

		}

	}

}