From 56f3abdb36d6f25ca5e9c61736cbc57e8604679e Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Wed, 23 Sep 2026 04:20:49 +0800 Subject: [PATCH] fix(operator): heal a deleted RCON Secret instead of locking the server out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting the per-server RCON Secret used to leave a running pod authenticating with the lost password while the operator re-minted a fresh one and probed with it: the RCON gate failed forever (live: 96s+ of RconNotReachable, headed for ReadinessTimeout) and nothing re-triggered a pod restart — the server only came back when the pod was deleted by hand. Two changes pair up: - Owns(&corev1.Secret{}) so the deletion is noticed at all (a quiet Running server emits no other events; the Secret is controller-owned, so the watch maps it back to the CR). - The pod template now carries a fingerprint of the current password (RconSecretAnnotation). Re-creation changes the fingerprint, the StatefulSet rolls, and the new pod picks the new password up; while the Secret is untouched the value is stable so no spurious rolls. Unit tests pin stability across reconciles and the change-on-recreation roll. --- internal/operator/reconciler.go | 81 ++++++++++++++++++++-------- internal/operator/reconciler_test.go | 63 ++++++++++++++++++++++ 2 files changed, 122 insertions(+), 22 deletions(-) diff --git a/internal/operator/reconciler.go b/internal/operator/reconciler.go index 1ad001e..81a96e0 100644 --- a/internal/operator/reconciler.go +++ b/internal/operator/reconciler.go @@ -7,6 +7,7 @@ package operator import ( "context" "crypto/rand" + "crypto/sha256" "encoding/hex" "fmt" "time" @@ -40,6 +41,21 @@ const ( defaultReadinessTimeoutSec = 300 ) +// RconSecretAnnotation stamps the pod template with a fingerprint of the +// current RCON password. If the Secret is ever lost (its deletion destroys the +// password) and re-provisioned, the fingerprint changes and the StatefulSet +// rolls the pod onto the new password. Without it the running pod keeps +// authenticating with the old value while the operator probes with the new +// one, and the RCON gate fails until someone restarts the pod by hand. +const RconSecretAnnotation = "felis.lolicon.best/rcon-secret" + +// rconStamp fingerprints an RCON password for RconSecretAnnotation. 64 bits of +// SHA-256: enough to never confuse two passwords, short enough to read. +func rconStamp(password []byte) string { + sum := sha256.Sum256(password) + return hex.EncodeToString(sum[:8]) +} + // Reconciler reconciles a MinecraftServer with its managed children. type Reconciler struct { client.Client @@ -69,6 +85,10 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { For(&v1alpha1.MinecraftServer{}). Owns(&appsv1.StatefulSet{}). Owns(&corev1.Service{}). + // Owns the Secrets too: the per-server RCON password is managed here, + // and a watch is what lets a deleted Secret be noticed at all (a quiet + // Running server otherwise produces no events). + Owns(&corev1.Secret{}). Complete(r) } @@ -100,7 +120,8 @@ func (r *Reconciler) reconcileRunning(ctx context.Context, server *v1alpha1.Mine // secretKeyRef, so a pod created ahead of its Secret never starts — it sits in // CreateContainerConfigError, which reads like a broken image rather than a // missing key. - if err := r.ensureRconSecret(ctx, server); err != nil { + passwordStamp, err := r.ensureRconSecret(ctx, server) + if err != nil { r.markStarting(server, "RconSecretUnavailable", err.Error()) if perr := r.patchStatus(ctx, server); perr != nil { return ctrl.Result{}, perr @@ -114,6 +135,12 @@ func (r *Reconciler) reconcileRunning(ctx context.Context, server *v1alpha1.Mine r.markFailed(server, "InvalidSpec", err.Error()) return ctrl.Result{}, r.patchStatus(ctx, server) } + if passwordStamp != "" { + if desired.Spec.Template.Annotations == nil { + desired.Spec.Template.Annotations = map[string]string{} + } + desired.Spec.Template.Annotations[RconSecretAnnotation] = passwordStamp + } if err := controllerutil.SetControllerReference(server, desired, r.Scheme); err != nil { return ctrl.Result{}, err } @@ -281,41 +308,44 @@ func (r *Reconciler) ensureServices(ctx context.Context, server *v1alpha1.Minecr // ensureRconSecret creates the per-server RCON password Secret named by // spec.rcon.secretRef the first time a server with RCON enabled reconciles, and // leaves it alone afterwards. Provisioning lives here rather than in felis-api's -// CreateServer for three reasons: it is declarative (a server whose Secret was -// deleted heals on the next pass instead of staying permanently unreachable), the -// controller reference makes Kubernetes garbage-collect the Secret with the server -// so no delete path has to remember it, and it backfills — a server created before -// RCON existed only needs spec.rcon filled in, and the password appears without -// anyone handling it. felis-api never mints the password and never needs to: it -// reads the Secret at command time (internal/api/console.go rconPassword). +// CreateServer for three reasons: it is declarative (a deleted Secret is +// re-minted on the next pass), the controller reference makes Kubernetes +// garbage-collect the Secret with the server so no delete path has to remember +// it, and it backfills — a server created before RCON existed only needs +// spec.rcon filled in, and the password appears without anyone handling it. +// felis-api never mints the password and never needs to: it reads the Secret at +// command time (internal/api/console.go rconPassword). // // The password is 32 hex chars from crypto/rand. It is generated once and never // rotated here: rewriting it would leave the running server authenticating with // the old value until its pod restarts, so rotation belongs to an explicit -// operation, not to a reconcile that runs every few seconds. -func (r *Reconciler) ensureRconSecret(ctx context.Context, server *v1alpha1.MinecraftServer) error { +// operation, not to a reconcile that runs every few seconds. Re-creation after +// a deletion is the one case where a fresh password must reach the pod — the +// caller stamps the returned fingerprint onto the pod template, so the +// StatefulSet rolls exactly when the password underneath it changes. +func (r *Reconciler) ensureRconSecret(ctx context.Context, server *v1alpha1.MinecraftServer) (string, error) { if !server.Spec.Rcon.Enabled { - return nil + return "", nil } ref := server.Spec.Rcon.SecretRef if ref.Name == "" || ref.Key == "" { - return fmt.Errorf("rcon.secretRef.name and .key are required when rcon is enabled") + return "", fmt.Errorf("rcon.secretRef.name and .key are required when rcon is enabled") } var existing corev1.Secret err := r.Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: ref.Name}, &existing) if err == nil { - if _, ok := existing.Data[ref.Key]; ok { - return nil + if b, ok := existing.Data[ref.Key]; ok { + return rconStamp(b), nil } - return fmt.Errorf("secret %s exists but has no key %q", ref.Name, ref.Key) + return "", fmt.Errorf("secret %s exists but has no key %q", ref.Name, ref.Key) } if !apierrors.IsNotFound(err) { - return err + return "", err } password, err := randomRconPassword() if err != nil { - return err + return "", err } secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -331,17 +361,24 @@ func (r *Reconciler) ensureRconSecret(ctx context.Context, server *v1alpha1.Mine Data: map[string][]byte{ref.Key: []byte(password)}, } if err := controllerutil.SetControllerReference(server, secret, r.Scheme); err != nil { - return err + return "", err } if err := r.Create(ctx, secret); err != nil { // Another reconcile (or a racing replica) won: that Secret is as good as - // this one, so treat the collision as success rather than thrashing. + // this one, but the stamp must match what the next pass will read, so + // fetch the winner — a stale cache here just requeues (the caller's + // RconSecretUnavailable path retries in 10s) instead of stamping a + // value that would flip on the next reconcile and roll the pod twice. if apierrors.IsAlreadyExists(err) { - return nil + var winner corev1.Secret + if gerr := r.Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: ref.Name}, &winner); gerr != nil { + return "", gerr + } + return rconStamp(winner.Data[ref.Key]), nil } - return err + return "", err } - return nil + return rconStamp([]byte(password)), nil } // randomRconPassword returns 16 crypto/rand bytes as hex. Hex, not base64: the diff --git a/internal/operator/reconciler_test.go b/internal/operator/reconciler_test.go index caedb52..f8c2de4 100644 --- a/internal/operator/reconciler_test.go +++ b/internal/operator/reconciler_test.go @@ -487,6 +487,69 @@ func TestNoIdleRequeueWhenDisabled(t *testing.T) { } } +// TestRconStamp_StableAcrossReconciles pins that the pod template stamp does +// not drift while the Secret is untouched — a drifting value would roll the +// pod on every reconcile. +func TestRconStamp_StableAcrossReconciles(t *testing.T) { + r, c := newReconciler(t, fakeProber{players: operator.PlayerCount{Online: 0, Max: 20}}, runningServer(), rconSecret()) + + reconcile(t, r, "survival") + markPodReady(t, c, "survival") + reconcile(t, r, "survival") + + first := stsTemplateStamp(t, c, "survival") + if first == "" { + t.Fatal("pod template must carry the RCON secret stamp") + } + reconcile(t, r, "survival") + if second := stsTemplateStamp(t, c, "survival"); second != first { + t.Fatalf("stamp drifted from %q to %q across reconciles", first, second) + } +} + +// TestRconSecretRecreation_RollsTemplate is the regression for the live lockup: +// deleting the Secret re-mints a different password, and the pod must be +// rolled onto it — otherwise the old pod keeps authenticating with the lost +// password and the RCON gate fails forever. +func TestRconSecretRecreation_RollsTemplate(t *testing.T) { + r, c := newReconciler(t, fakeProber{players: operator.PlayerCount{Online: 0, Max: 20}}, runningServer(), rconSecret()) + + reconcile(t, r, "survival") + markPodReady(t, c, "survival") + reconcile(t, r, "survival") + before := stsTemplateStamp(t, c, "survival") + if before == "" { + t.Fatal("pod template must carry the RCON secret stamp") + } + + var sec corev1.Secret + if err := c.Get(context.Background(), types.NamespacedName{Namespace: "minecraft", Name: "survival-rcon"}, &sec); err != nil { + t.Fatalf("get secret: %v", err) + } + if err := c.Delete(context.Background(), &sec); err != nil { + t.Fatalf("delete secret: %v", err) + } + + reconcile(t, r, "survival") + + after := stsTemplateStamp(t, c, "survival") + if after == "" { + t.Fatal("pod template must carry the RCON secret stamp after re-creation") + } + if after == before { + t.Fatalf("stamp %q unchanged after the Secret was re-created — the pod would keep the lost password", before) + } +} + +func stsTemplateStamp(t *testing.T, c client.Client, name string) string { + t.Helper() + var sts appsv1.StatefulSet + if err := c.Get(context.Background(), types.NamespacedName{Namespace: "minecraft", Name: name}, &sts); err != nil { + t.Fatalf("get sts: %v", err) + } + return sts.Spec.Template.Annotations[operator.RconSecretAnnotation] +} + // TestIdleAutoStop_SkipsWhenDisabled verifies that a Running empty server does // NOT get an EmptySince timestamp when AutoStopEnabled is false. func TestIdleAutoStop_SkipsWhenDisabled(t *testing.T) {