From 694e3cb8009f3e3f4daf15b4bbaebccaa51e697a Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 21 Jul 2026 00:12:37 +0900 Subject: [PATCH] feat(rcon): provision per-server RCON so the console, player list and permissions work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A server created through the panel never had RCON. CreateServer built a MinecraftServerSpec without a Rcon block at all, so the field took its zero value and every downstream consumer read Enabled=false. Nothing failed loudly: the operator skips the probe when RCON is off and marks the server Ready on pod readiness alone, so the panel showed "运行中" for a server the control plane could not talk to. Everything that rides the write channel (spec §8 写=RCON) was dead — the online-player list returned nothing because Status.Players is only ever sampled by the probe, and console writes answered 503 ErrConsoleUnavailable because internal/api/console.go refuses when Enabled is false. The whole RCON machinery already existed — builders gate the service port, container port, preStop save-and-stop hook and the RCON_* env on Spec.Rcon, the reconciler probes and reports, console.go dials, the NetworkPolicy opens 25575 to {api, operator}. The only thing missing was that nobody ever turned it on or created a password. This wires the three layers that were absent. Provisioning lives in the operator, not in felis-api. felis-api holds secrets:get and not create, and giving it create solely to mint a password it immediately stops caring about (console.go re-reads the Secret at command time) would widen the API's powers for nothing. The operator already reads every Secret in the namespace, so adding create there grants no read it did not have. It also makes provisioning declarative: a Secret deleted by hand comes back on the next pass, a controller reference garbage-collects it with the server so no delete path has to remember it, and a server that predates RCON only needs spec.rcon filled in for the password to appear. The name comes from naming.RconSecretName so felis-api, `felis setup` and the operator cannot drift apart on it. RCON is enabled per system service rather than by default, because enabling it on a backend that serves no RCON listener is destructive rather than merely useless: the operator gates readiness on the probe, so such a server never leaves Starting and is eventually marked Failed. The login limbo is exactly that backend (LOOHP/Limbo has no RCON) and it is the front door, so it stays off; the lobby runs Paper and is administered through the panel like any other server, so it is on. Paper only reads RCON settings from server.properties, so the operator's injected RCON_PASSWORD did nothing on its own — felis-lobby's entrypoint now writes the three keys on every boot. Rewriting them each time makes the copy in the world volume derived state rather than the source of truth, so an owner who edits them through the panel's file editor cannot lock the control plane out of their own server. Without a password it sets enable-rcon=false and warns rather than refusing to start: unlike the forwarding secret, a missing RCON password degrades the server rather than making it unsafe. That password landing in server.properties is a §286 exposure (RCON 密码绝不下发 前端), since server.properties is readable through the file editor. It is redacted on read rather than the file being denied outright the way config/paper-global.yml is: the forwarding secret is cluster-wide material that merely happens to sit in the volume, whereas server.properties is the single most-edited config an owner has, and hiding one line should not cost them MOTD, difficulty and view-distance. The write path is deliberately left alone — the boot-time rewrite restores the real value, which is what makes redacting rather than denying safe here. Also guards idle auto-stop on Rcon.Enabled. Status.Players is only meaningful when the probe ran; with RCON off it keeps its zero value, which that branch would have read as "empty" and used to stop a server full of people. AutoStopEnabled is not currently settable through any path, so this is a latent footgun rather than a live bug, but it is one line and the alternative is discovering it in production. Checks: the operator provisions a missing Secret with a 32-hex-char password and a controller reference, and does not rotate an existing one; idle auto-stop stays inert without RCON; the editor redacts rcon.password from the world root's server.properties while leaving the rest of the file (and a plugin's own nested copy) intact; login has RCON off and lobby has it on with the shared secret name; CreateServer sets the block. That last one departs from K8sCluster being integration-tested against a live cluster: this defect was a struct literal missing a field, it shipped, and a fake client is enough to pin a struct literal. Existing servers are NOT migrated by this change — CreateServer only covers new ones and ensureSystemServers is create-if-absent, so a `felis setup` re-run will not touch an existing lobby. A deployed install additionally needs the felis-lobby image rebuilt and re-imported for the entrypoint change, and its pods recreated, before the RCON keys reach server.properties. --- cmd/felis/systemservers.go | 36 ++++++++++- cmd/felis/systemservers_test.go | 37 +++++++++++ deploy/lobby/entrypoint.sh | 27 ++++++++ internal/api/k8scluster.go | 18 ++++++ internal/api/k8scluster_test.go | 65 +++++++++++++++++++ internal/fileedit/exec.go | 46 ++++++++++++- internal/fileedit/exec_test.go | 48 ++++++++++++++ internal/naming/naming.go | 15 +++++ internal/operator/reconciler.go | 97 +++++++++++++++++++++++++++- internal/operator/reconciler_test.go | 70 ++++++++++++++++++++ internal/platform/rbac.go | 8 ++- 11 files changed, 461 insertions(+), 6 deletions(-) create mode 100644 internal/api/k8scluster_test.go diff --git a/cmd/felis/systemservers.go b/cmd/felis/systemservers.go index 276e63e..b001135 100644 --- a/cmd/felis/systemservers.go +++ b/cmd/felis/systemservers.go @@ -48,6 +48,13 @@ type systemServerSpec struct { storage string // world PVC size fallbackServer string // "" = none (refuse when down); never the lobby healthHTTPPort int32 // > 0 → gate readiness on an HTTP health endpoint + // rcon opts a system service into the RCON write channel. It is per-service and + // NOT a default, because enabling it on a backend that runs no RCON listener is + // actively destructive rather than merely useless: the operator gates readiness + // on the probe, so the server would never leave Starting and would eventually be + // marked Failed. The login limbo is exactly that case (LOOHP/Limbo has no RCON), + // and it is the front door — taking it down locks everyone out. + rcon bool // env are extra plain (non-secret) environment variables baked into the pod. // System-service configuration derived from the deployment (the internal API // URL, root domain, lobby name) rides here; secrets never do — the service @@ -55,6 +62,24 @@ type systemServerSpec struct { env []v1alpha1.EnvVar } +// systemRcon renders the RCON block for a system service. The secret name comes +// from naming.RconSecretName — the same convention felis-api writes for user +// servers and the operator provisions against — so a system service is not a +// second, parallel way of doing this. Port is left 0 so the operator's default is +// the only place the number lives. +func systemRcon(in systemServerSpec) v1alpha1.RconSpec { + if !in.rcon { + return v1alpha1.RconSpec{Enabled: false} + } + return v1alpha1.RconSpec{ + Enabled: true, + SecretRef: v1alpha1.SecretKeyRef{ + Name: naming.RconSecretName(in.name), + Key: naming.RconSecretKey, + }, + } +} + // felisLimboHealthPort is the port the felis-limbo readiness plugin serves its // HTTP health endpoint on. The login system service gates pod readiness on it so // "the limbo has finished starting" — not merely "the game socket is bound" — @@ -82,8 +107,10 @@ const ( // - permits reserved names (login/lobby) via ValidateSystemServerName, // - sets DesiredState=Running (the service is up the moment it exists), // - sets ReaperExempt=true and AutostartPolicy=public, -// - leaves RCON disabled (LOOHP/Limbo has none; readiness is gated on pod -// TCP/HTTP health, not an RCON probe — see the operator reconciler). +// - enables RCON only where the image actually serves it (in.rcon): the lobby +// is Paper and needs the write channel like any user server, while the login +// limbo has no RCON listener at all and gates readiness on pod TCP/HTTP +// health instead — see the operator reconciler. func buildSystemServer(in systemServerSpec, namespace string) (*v1alpha1.MinecraftServer, error) { if err := naming.ValidateSystemServerName(in.name); err != nil { return nil, fmt.Errorf("invalid name: %w", err) @@ -141,7 +168,7 @@ func buildSystemServer(in systemServerSpec, namespace string) (*v1alpha1.Minecra // forwarding), backends run offline-mode; the proxy is the one place // online-mode is true (spec §8, §11). OnlineMode: false, - Rcon: v1alpha1.RconSpec{Enabled: false}, + Rcon: systemRcon(in), Storage: v1alpha1.StorageSpec{Size: storageQ.String()}, Resources: corev1.ResourceRequirements{Limits: limits, Requests: requests}, Startup: v1alpha1.StartupSpec{HealthHTTPPort: in.healthHTTPPort}, @@ -192,6 +219,9 @@ func lobbySystemServer(image, namespace string) (*v1alpha1.MinecraftServer, erro memory: "1Gi", storage: "2Gi", fallbackServer: naming.SystemLoginServer, + // Paper serves RCON, and the lobby is administered through the panel like any + // other server — online players, console, permissions all ride this channel. + rcon: true, }, namespace) } diff --git a/cmd/felis/systemservers_test.go b/cmd/felis/systemservers_test.go index 1c23584..b053b25 100644 --- a/cmd/felis/systemservers_test.go +++ b/cmd/felis/systemservers_test.go @@ -439,3 +439,40 @@ func TestEnsureSystemServersSkipsUnsetImage(t *testing.T) { t.Errorf("lobby: skipped = %q, want %q", byName[naming.SystemLobbyServer].skipped, "image not configured") } } + +// TestSystemServerRconPolicy pins which system service gets the RCON write +// channel. This is not a preference: the operator gates readiness on the RCON +// probe, so enabling it on a backend that serves no RCON listener would hold that +// server in Starting until it was marked Failed. The login limbo is exactly that +// backend (LOOHP/Limbo has no RCON) AND it is the front door, so getting this +// backwards locks every player out of the deployment. +func TestSystemServerRconPolicy(t *testing.T) { + login, err := loginSystemServer("reg/limbo:1", "minecraft", "http://api:8081", "mc.example.net", "console.mc.example.net") + if err != nil { + t.Fatalf("loginSystemServer: %v", err) + } + if login.Spec.Rcon.Enabled { + t.Fatal("the login limbo must not enable RCON: it serves no RCON listener, so the " + + "operator's readiness probe would never succeed and the login gate would be marked Failed") + } + + lobby, err := lobbySystemServer("reg/lobby:1", "minecraft") + if err != nil { + t.Fatalf("lobbySystemServer: %v", err) + } + if !lobby.Spec.Rcon.Enabled { + t.Fatal("the lobby runs Paper and is administered through the panel; without RCON its " + + "console, online-player list and permission changes are all unavailable") + } + if got, want := lobby.Spec.Rcon.SecretRef.Name, naming.RconSecretName("lobby"); got != want { + t.Fatalf("lobby rcon secret = %q, want %q — the operator provisions against this name", got, want) + } + if got, want := lobby.Spec.Rcon.SecretRef.Key, naming.RconSecretKey; got != want { + t.Fatalf("lobby rcon secret key = %q, want %q", got, want) + } + // Port stays unset so the operator's DefaultRconPort is the only place the + // number is written down. + if lobby.Spec.Rcon.Port != 0 { + t.Fatalf("lobby rcon port = %d, want 0 (operator default)", lobby.Spec.Rcon.Port) + } +} diff --git a/deploy/lobby/entrypoint.sh b/deploy/lobby/entrypoint.sh index acd3f1b..0641ae4 100644 --- a/deploy/lobby/entrypoint.sh +++ b/deploy/lobby/entrypoint.sh @@ -58,6 +58,33 @@ set_prop() { set_prop server-port "$PORT" set_prop online-mode false +# RCON is the control plane's write channel (spec §8 写=RCON): the operator probes it +# for readiness and the player tally, and felis-api runs console/permission commands over +# it. Paper only reads these three keys from server.properties, so the operator's injected +# RCON_PASSWORD has to be written here to take effect — env alone does nothing. +# +# Rewritten on EVERY boot from the Secret, deliberately. That makes the value in the world +# volume derived state rather than the source of truth: an owner who edits (or clobbers) +# these lines through the panel's file editor cannot lock the control plane out of their +# own server, because the next restart restores the real password. The editor is also kept +# from reading the password back out — see internal/fileedit/exec.go (spec §286: RCON +# 密码绝不下发前端). +# +# No password, no RCON: an empty enable-rcon=true would let anything that reaches the port +# in unauthenticated. Unlike the forwarding secret this is not fatal — a server without the +# write channel still serves players — so it warns and starts rather than refusing. +if [ -n "${RCON_PASSWORD:-}" ]; then + set_prop enable-rcon true + set_prop rcon.port "${RCON_PORT:-25575}" + set_prop rcon.password "$RCON_PASSWORD" + echo "felis-lobby: rcon enabled on port ${RCON_PORT:-25575}" +else + set_prop enable-rcon false + echo "felis-lobby: WARNING — RCON_PASSWORD is empty, so the console, the online-player" >&2 + echo " list and permission changes will be unavailable for this server. The operator" >&2 + echo " injects it from the -rcon Secret when spec.rcon.enabled is true." >&2 +fi + # ponytail: rewritten whole, not merged. Paper loads this file and fills every key it does # not find with the default, then writes the full tree back — so a proxies-only file is a # complete, stable input, and the lobby's other globals are simply always the defaults. diff --git a/internal/api/k8scluster.go b/internal/api/k8scluster.go index bc3e461..ce45d94 100644 --- a/internal/api/k8scluster.go +++ b/internal/api/k8scluster.go @@ -96,6 +96,24 @@ func (k *K8sCluster) CreateServer(ctx context.Context, in CreateServerInput) err FallbackServer: naming.SystemLoginServer, Storage: v1alpha1.StorageSpec{Size: in.StorageSize}, Resources: in.Resources, + // RCON is what makes a server manageable at all: the operator gates + // phase=Running on the probe and samples the player tally from it (spec + // §5), and every write — console commands, the LuckPerms grants behind the + // permissions UI — travels over it (spec §8 写=RCON). Leaving it unset + // produced a server that looked Running, reported nobody online, and + // answered the console with 503; enabling it here is the fix for all + // three. Port stays 0 so the operator applies its own default rather than + // this package pinning a second copy of it. The password is not set (and + // felis-api could not set it — it holds secrets:get, not create): the + // operator mints it into this Secret on first reconcile, and felis-api + // only ever reads it back at command time. + Rcon: v1alpha1.RconSpec{ + Enabled: true, + SecretRef: v1alpha1.SecretKeyRef{ + Name: naming.RconSecretName(in.Name), + Key: naming.RconSecretKey, + }, + }, }, } if err := k.c.Create(ctx, ms); err != nil { diff --git a/internal/api/k8scluster_test.go b/internal/api/k8scluster_test.go new file mode 100644 index 0000000..9259558 --- /dev/null +++ b/internal/api/k8scluster_test.go @@ -0,0 +1,65 @@ +package api + +import ( + "context" + "testing" + + "felis.lolicon.best/internal/apis/felis/v1alpha1" + "felis.lolicon.best/internal/naming" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// K8sCluster is documented as integration-tested against a live cluster rather +// than covered by the hermetic suite, and for most of it that is the right call — +// merge-patch semantics are not worth faking. This one test departs from it +// deliberately: "CreateServer forgot to set spec.rcon" is precisely the kind of +// defect a live-cluster test catches only if someone runs it, and it shipped. It +// produced servers that reported Running, listed nobody online, and answered the +// console with 503, and the cause was a struct literal missing a field. A fake +// client is enough to pin a struct literal. +func TestCreateServerEnablesRcon(t *testing.T) { + scheme := runtime.NewScheme() + if err := v1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("scheme: %v", err) + } + c := fake.NewClientBuilder().WithScheme(scheme).Build() + k := NewK8sCluster(c, "minecraft") + + if err := k.CreateServer(context.Background(), CreateServerInput{ + Name: "survival", + Subdomain: "survival", + DisplayName: "Survival", + Image: "reg/paper:1", + JavaMemory: "2G", + StorageSize: "10Gi", + }); err != nil { + t.Fatalf("CreateServer: %v", err) + } + + var ms v1alpha1.MinecraftServer + if err := c.Get(context.Background(), + types.NamespacedName{Namespace: "minecraft", Name: "survival"}, &ms); err != nil { + t.Fatalf("get created server: %v", err) + } + + if !ms.Spec.Rcon.Enabled { + t.Fatal("spec.rcon.enabled is false: the operator will skip the readiness probe and " + + "never sample a player count, and every console write will fail with 503") + } + // The name is the contract with internal/operator.ensureRconSecret, which + // provisions the password against exactly this key. A mismatch leaves the pod + // stuck on a secretKeyRef that nothing ever creates. + if got, want := ms.Spec.Rcon.SecretRef.Name, naming.RconSecretName("survival"); got != want { + t.Fatalf("rcon secretRef name = %q, want %q", got, want) + } + if got, want := ms.Spec.Rcon.SecretRef.Key, naming.RconSecretKey; got != want { + t.Fatalf("rcon secretRef key = %q, want %q", got, want) + } + // felis-api holds secrets:get, not create — it must never try to mint the + // password itself, and nothing here should carry one. + if ms.Spec.Rcon.Port != 0 { + t.Fatalf("rcon port = %d, want 0 so the operator default is the only copy", ms.Spec.Rcon.Port) + } +} diff --git a/internal/fileedit/exec.go b/internal/fileedit/exec.go index 9a63c4b..cb8ace0 100644 --- a/internal/fileedit/exec.go +++ b/internal/fileedit/exec.go @@ -1,6 +1,7 @@ package fileedit import ( + "bytes" "encoding/json" "errors" "fmt" @@ -265,7 +266,50 @@ func read(r *os.Root, name string) Result { if err != nil { return failure(err, name) } - return Result{Content: b} + return Result{Content: redactSecretProps(name, b)} +} + +// propsPath is the server's main config file, and rconPasswordKey the one line in +// it the editor must not hand back (spec §286: RCON 密码绝不下发前端). +const ( + propsPath = "server.properties" + rconPasswordKey = "rcon.password" + // redactedValue is deliberately not empty: a blank value would read as "RCON has + // no password", which is a very different and much more alarming claim than "you + // are not being shown it". + redactedValue = "" +) + +// redactSecretProps blanks the RCON password when server.properties is read. +// +// Unlike secretConfigPath this is a value redaction rather than a whole-file +// denial, because the file is not platform material that merely happens to sit in +// the volume — it is the single most-edited config a server owner has (MOTD, +// view-distance, difficulty, gamemode), and refusing it outright would cost real +// repair to hide one line. The password is also per-server and garbage-collected +// with it, so unlike the cluster-wide forwarding secret it leaks nothing about +// anyone else's server; it is withheld because §286 draws the line at the frontend +// regardless of blast radius, and because the console already gives an owner every +// capability the password would. +// +// The write path is left alone on purpose, mirroring the reasoning at +// secretConfigPath: felis-lobby's entrypoint rewrites all three rcon keys from the +// injected Secret on every boot, so saving the placeholder back cannot lock the +// control plane out — the next restart restores the real value. That is what makes +// redaction safe here; without the boot-time rewrite this would be a footgun. +func redactSecretProps(name string, content []byte) []byte { + if path.Clean(name) != propsPath { + return content + } + lines := bytes.Split(content, []byte("\n")) + for i, line := range lines { + // TrimSpace before matching: a properties key may be indented, and the + // trailing \r of a CRLF file would otherwise ride along into the value. + if bytes.HasPrefix(bytes.TrimSpace(line), []byte(rconPasswordKey+"=")) { + lines[i] = []byte(rconPasswordKey + "=" + redactedValue) + } + } + return bytes.Join(lines, []byte("\n")) } // write replaces a file's contents. It truncates rather than appends, and it does diff --git a/internal/fileedit/exec_test.go b/internal/fileedit/exec_test.go index c570f20..8a04d13 100644 --- a/internal/fileedit/exec_test.go +++ b/internal/fileedit/exec_test.go @@ -355,3 +355,51 @@ func TestReadRefusesTheForwardingSecret(t *testing.T) { t.Errorf("write code = %q, want success — only the read is denied", res.Code) } } + +// TestReadRedactsRconPassword pins spec §286 (RCON 密码绝不下发前端) on the one +// path that could leak it: server.properties is the file owners edit most, so it +// is readable — but the RCON password in it is the control plane's command +// credential for that server, and the editor must not hand it back. +func TestReadRedactsRconPassword(t *testing.T) { + root := t.TempDir() + props := "motd=hello\nrcon.password=hunter2\nrcon.port=25575\nenable-rcon=true\n" + if err := os.WriteFile(filepath.Join(root, "server.properties"), []byte(props), 0o644); err != nil { + t.Fatalf("write server.properties: %v", err) + } + // A same-named file in a subdirectory must NOT be treated as the real one: the + // redaction keys off the cleaned path, and a plugin is free to keep its own + // server.properties anywhere in the volume. + if err := os.MkdirAll(filepath.Join(root, "plugins"), 0o755); err != nil { + t.Fatalf("mkdir plugins: %v", err) + } + if err := os.WriteFile(filepath.Join(root, "plugins", "server.properties"), []byte("rcon.password=notmine\n"), 0o644); err != nil { + t.Fatalf("write nested server.properties: %v", err) + } + + res, err := Execute(root, OpRead, "server.properties", nil) + if err != nil { + t.Fatalf("Execute: %v", err) + } + got := string(res.Content) + if strings.Contains(got, "hunter2") { + t.Fatalf("read returned the RCON password (spec §286):\n%s", got) + } + if !strings.Contains(got, redactedValue) { + t.Fatalf("read did not mark the password as withheld:\n%s", got) + } + // Redaction must not cost the owner the rest of the file — that is the whole + // reason this is a value redaction and not a whole-file denial. + for _, keep := range []string{"motd=hello", "rcon.port=25575", "enable-rcon=true"} { + if !strings.Contains(got, keep) { + t.Fatalf("redaction dropped %q from the file:\n%s", keep, got) + } + } + + nested, err := Execute(root, OpRead, "plugins/server.properties", nil) + if err != nil { + t.Fatalf("Execute nested: %v", err) + } + if !strings.Contains(string(nested.Content), "notmine") { + t.Fatalf("a nested server.properties was redacted; only the world root's is the real one:\n%s", nested.Content) + } +} diff --git a/internal/naming/naming.go b/internal/naming/naming.go index 7ef4267..e6e60ac 100644 --- a/internal/naming/naming.go +++ b/internal/naming/naming.go @@ -127,6 +127,21 @@ func WorldPVCName(server string) string { return worldVolumeName + "-" + server + "-0" } +// RconSecretKey is the key holding the password inside a server's RCON Secret. +const RconSecretKey = "password" + +// RconSecretName returns the per-server RCON password Secret name. Like +// WorldPVCName this convention is shared rather than duplicated: felis-api writes +// it into spec.rcon.secretRef when creating a server, `felis setup` writes the +// same for the system lobby, and the operator both provisions the Secret under +// that name and reads it back for the readiness probe. One password per server, +// never a shared one — the console grants whoever holds it full command authority +// over that server, so a single cluster-wide value would make every owner an +// operator of everyone else's world. +func RconSecretName(server string) string { + return server + "-rcon" +} + // Hostname composes subdomain.rootDomain after validating the subdomain. func Hostname(subdomain, rootDomain string) (string, error) { if err := ValidateServerName(subdomain); err != nil { diff --git a/internal/operator/reconciler.go b/internal/operator/reconciler.go index 4ba608b..11b9400 100644 --- a/internal/operator/reconciler.go +++ b/internal/operator/reconciler.go @@ -6,6 +6,8 @@ package operator import ( "context" + "crypto/rand" + "encoding/hex" "fmt" "time" @@ -84,6 +86,18 @@ func (r *Reconciler) reconcileRunning(ctx context.Context, server *v1alpha1.Mine return ctrl.Result{}, err } + // Before the StatefulSet, not after: buildEnv wires RCON_PASSWORD as a + // 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 { + r.markStarting(server, "RconSecretUnavailable", err.Error()) + if perr := r.patchStatus(ctx, server); perr != nil { + return ctrl.Result{}, perr + } + return ctrl.Result{RequeueAfter: requeueSecret}, nil + } + desired, err := buildStatefulSet(server, 1) if err != nil { // A malformed spec (e.g. bad storage quantity) is terminal until edited. @@ -151,7 +165,10 @@ func (r *Reconciler) reconcileRunning(ctx context.Context, server *v1alpha1.Mine // player tally is zero, track the empty duration and auto-stop when the // configured timeout expires. The existing RCON probe already supplies // the player count — no extra network cost. - if server.Spec.Idle.AutoStopEnabled && server.Spec.Idle.EmptySecondsBeforeStop > 0 { + // Rcon.Enabled is part of the condition because `players` is only a real tally + // when the probe above ran: with RCON off it keeps its zero value, which this + // branch would read as "empty" and use to stop a server full of people. + if server.Spec.Rcon.Enabled && server.Spec.Idle.AutoStopEnabled && server.Spec.Idle.EmptySecondsBeforeStop > 0 { if players.Online == 0 { if server.Status.EmptySince == nil { t := r.now() @@ -228,6 +245,84 @@ func (r *Reconciler) ensureServices(ctx context.Context, server *v1alpha1.Minecr return endpointAddress, nil } +// 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). +// +// 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 { + if !server.Spec.Rcon.Enabled { + 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") + } + 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 + } + return fmt.Errorf("secret %s exists but has no key %q", ref.Name, ref.Key) + } + if !apierrors.IsNotFound(err) { + return err + } + + password, err := randomRconPassword() + if err != nil { + return err + } + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: ref.Name, + Namespace: server.Namespace, + Labels: map[string]string{v1alpha1.LabelServer: server.Name}, + }, + Type: corev1.SecretTypeOpaque, + // Data, not StringData: StringData is a write-only convenience the API server + // folds into Data, so anything reading the object back — including this + // package's own tests — would see an empty value. Encoding here keeps the + // object self-consistent the moment it is built. + Data: map[string][]byte{ref.Key: []byte(password)}, + } + if err := controllerutil.SetControllerReference(server, secret, r.Scheme); err != nil { + 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. + if apierrors.IsAlreadyExists(err) { + return nil + } + return err + } + return nil +} + +// randomRconPassword returns 16 crypto/rand bytes as hex. Hex, not base64: the +// value is written verbatim into server.properties, whose parser treats the line +// as raw text to end-of-line, and hex avoids every character (=, :, \, whitespace) +// that a properties file or a shell round-trip could interpret. +func randomRconPassword() (string, error) { + var b [16]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("generate rcon password: %w", err) + } + return hex.EncodeToString(b[:]), nil +} + func (r *Reconciler) rconPassword(ctx context.Context, server *v1alpha1.MinecraftServer) (string, error) { ref := server.Spec.Rcon.SecretRef if ref.Name == "" || ref.Key == "" { diff --git a/internal/operator/reconciler_test.go b/internal/operator/reconciler_test.go index bb47d69..fecc6d5 100644 --- a/internal/operator/reconciler_test.go +++ b/internal/operator/reconciler_test.go @@ -640,3 +640,73 @@ func TestReconcileRunning_StartDurationObservedOnce(t *testing.T) { t.Errorf("startRequestedAt = %v after Stop, want nil so the next start re-anchors", s.Status.StartRequestedAt) } } + +// A server whose RCON Secret does not exist yet is the normal case on first +// reconcile — felis-api writes spec.rcon.secretRef but holds secrets:get, not +// create, so the name it points at is a promise the operator has to keep. Before +// this the server sat in Starting/RconSecretUnavailable forever. +func TestReconcileProvisionsMissingRconSecret(t *testing.T) { + r, c := newReconciler(t, fakeProber{}, runningServer()) + reconcile(t, r, "survival") + + var secret corev1.Secret + if err := c.Get(context.Background(), + types.NamespacedName{Namespace: "minecraft", Name: "survival-rcon"}, &secret); err != nil { + t.Fatalf("expected the operator to create the rcon Secret: %v", err) + } + pw := string(secret.Data["password"]) + if pw == "" { + t.Fatal("rcon Secret was created with an empty password") + } + // 16 random bytes as hex. An assertion on the shape, not the value: a password + // short enough to guess is the failure this is guarding, and it cannot be + // checked by comparing against a fixture. + if len(pw) != 32 { + t.Fatalf("password = %d chars, want 32 hex chars", len(pw)) + } + if strings.Trim(pw, "0123456789abcdef") != "" { + t.Fatalf("password %q is not hex; it is written verbatim into server.properties", pw) + } + // The controller reference is what makes Kubernetes delete the Secret with the + // server. Without it every deleted server leaves its password behind. + if len(secret.OwnerReferences) != 1 || secret.OwnerReferences[0].Name != "survival" { + t.Fatalf("owner references = %+v, want one referring to the server", secret.OwnerReferences) + } +} + +// Reconcile runs every few seconds; regenerating the password on each pass would +// leave the running server authenticating with a value the control plane no longer +// has, breaking the console until the pod happened to restart. +func TestReconcileKeepsAnExistingRconPassword(t *testing.T) { + r, c := newReconciler(t, fakeProber{}, runningServer(), rconSecret()) + reconcile(t, r, "survival") + reconcile(t, r, "survival") + + var secret corev1.Secret + if err := c.Get(context.Background(), + types.NamespacedName{Namespace: "minecraft", Name: "survival-rcon"}, &secret); err != nil { + t.Fatalf("get rcon secret: %v", err) + } + if got := string(secret.Data["password"]); got != "hunter2" { + t.Fatalf("password = %q, want the original %q — the operator rotated it", got, "hunter2") + } +} + +// The idle auto-stop reads Status.Players, which is only sampled by the RCON +// probe. With RCON off the tally stays at its zero value, and a bare +// "players == 0" test would read that as an empty server and stop one full of +// people. +func TestIdleAutoStopIsInertWithoutRcon(t *testing.T) { + server := runningServer() + server.Spec.Rcon = v1alpha1.RconSpec{Enabled: false} + server.Spec.Idle = v1alpha1.IdleSpec{AutoStopEnabled: true, EmptySecondsBeforeStop: 1} + + r, c := newReconciler(t, fakeProber{}, server) + reconcile(t, r, "survival") + reconcile(t, r, "survival") + + if got := getServer(t, c, "survival").Spec.DesiredState; got != v1alpha1.DesiredRunning { + t.Fatalf("desiredState = %q, want %q — idle auto-stop fired on an unprobed player count", + got, v1alpha1.DesiredRunning) + } +} diff --git a/internal/platform/rbac.go b/internal/platform/rbac.go index b3cbc38..3cfd9c5 100644 --- a/internal/platform/rbac.go +++ b/internal/platform/rbac.go @@ -136,7 +136,13 @@ func OperatorRole(p Params) *rbacv1.Role { rule([]string{groupFelis}, []string{"minecraftservers/status"}, []string{"update"}), rule([]string{groupApps}, []string{"statefulsets"}, []string{"get", "list", "watch", "create", "update"}), rule([]string{groupCore}, []string{"services"}, []string{"get", "list", "watch", "create", "update"}), - rule([]string{groupCore}, []string{"secrets"}, []string{"get", "list", "watch"}), + // create is here for the per-server RCON password Secret the operator + // provisions on first reconcile (internal/operator.ensureRconSecret). It is a + // smaller grant than it looks: this identity already holds get/list/watch on + // every Secret in this namespace, so being able to add one grants no read it + // did not already have. No update/delete — the password is written once and + // removed by garbage collection through its controller reference. + rule([]string{groupCore}, []string{"secrets"}, []string{"get", "list", "watch", "create"}), }) }