From a0064adfdd456eed5846d86e43563deff8d01d99 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 21 Jul 2026 00:47:04 +0900 Subject: [PATCH] fix(setup): stop the login gate sending players to the old console after a re-domain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changing root_domain updated felis.toml and the panel, but the login gate kept pointing players at the hostname it was created with. FELIS_ROOT_DOMAIN and FELIS_PANEL_HOSTNAME are baked into the login MinecraftServer at provisioning time, FelisLimboPlugin reads them to build the link an unauthenticated player is told to open, and ensureSystemServers is create-if-absent — so nothing in the install ever rewrote them. On the demo host the CR still carried console.159.223.32.51.nip.io hours after the domain had moved to mc.flyemoji.network: every joining player was handed a link that bypasses the tunnel, hits the node directly and trips a certificate warning, on the one screen someone with no account is guaranteed to see. setup now converges these values on an existing system service instead of skipping it. Create-if-absent stays the rule for everything else, and the comment on it is still true — an operator's edits to a system service must survive a re-run. These three names are the exception because they are not the operator's to own: they are a copy of config that is wrong the moment config changes, and there is no other writer who could notice. The convergence is deliberately narrow. Only a name already present with a different value is rewritten, so env the operator added by hand is untouched and the rest of the spec — image, memory, storage — is not read at all. A derived name that is absent from the live object is left absent rather than added back: a deliberate removal and drift look identical from here, and re-adding it would mean fighting the operator on every run. The outcome string reports the refresh so a setup run does not silently rewrite the front door. This closes one surface of a re-domain, not the whole of it. The write-once panel certificate at deploy/bootstrap.sh keeps its old SANs, and so do the Velocity config and the forwarding material; the warning in bootstrap.sh that says so is still accurate. What changes is that the surface players actually walk through now catches up when setup is re-run. Checks: a login gate built with the old domain converges onto the new one and says so; an env var the operator added and a hand-raised javaMemory both survive that same run; and an install whose config already matches reports no refresh, so a routine setup does not read like a re-domain. The middle one is the one worth having — converging config must not turn into a licence to clobber the edits create-if-absent exists to protect. --- cmd/felis/systemservers.go | 60 +++++++++++++++++- cmd/felis/systemservers_test.go | 105 ++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 1 deletion(-) diff --git a/cmd/felis/systemservers.go b/cmd/felis/systemservers.go index b001135..53235d8 100644 --- a/cmd/felis/systemservers.go +++ b/cmd/felis/systemservers.go @@ -308,7 +308,16 @@ func ensureSystemServers(ctx context.Context, cl client.Client, namespace, login }) continue } - outcomes = append(outcomes, systemServerOutcome{name: p.name, available: true, skipped: "already exists"}) + refreshed, err := refreshDerivedEnv(ctx, cl, &existing, ms) + if err != nil { + outcomes = append(outcomes, systemServerOutcome{name: p.name, err: err}) + continue + } + skipped := "already exists" + if refreshed { + skipped = "already exists; refreshed the console hostnames it points players at" + } + outcomes = append(outcomes, systemServerOutcome{name: p.name, available: true, skipped: skipped}) continue } if !apierrors.IsNotFound(getErr) { @@ -344,6 +353,55 @@ func ensureSystemServers(ctx context.Context, cl client.Client, namespace, login return outcomes } +// derivedSystemEnv are the system-server env vars whose values setup computes from +// config rather than inventing. They are the exception to create-if-absent, and the +// exception is narrow on purpose. +// +// Everything else on an existing system service is left alone so an operator's edits +// survive a re-run — but these are not the operator's to own, they are a copy of +// config that goes stale the moment config changes. That is not hypothetical: after +// a root-domain change the login gate keeps handing every joining player a console +// link built from the OLD domain, which is the one screen an unauthenticated player +// is guaranteed to see. Nothing else in the install rewrites them, so a re-run of +// setup is the only chance they get to catch up. +var derivedSystemEnv = map[string]bool{ + envAPIBaseURL: true, + envRootDomain: true, + envPanelHostname: true, +} + +// refreshDerivedEnv converges the config-derived env of an existing system server +// onto what setup just computed, and reports whether anything actually changed. +// +// It only ever overwrites a name that is already present with a different value, and +// only for the names above: env the operator added by hand is untouched, and a name +// missing from the live object is left missing rather than added back, since a +// deliberate removal is indistinguishable from drift and re-adding it would fight the +// operator every run. +func refreshDerivedEnv(ctx context.Context, cl client.Client, existing, desired *v1alpha1.MinecraftServer) (bool, error) { + want := make(map[string]string, len(derivedSystemEnv)) + for _, e := range desired.Spec.Env { + if derivedSystemEnv[e.Name] { + want[e.Name] = e.Value + } + } + + changed := false + for i, e := range existing.Spec.Env { + if v, ok := want[e.Name]; ok && v != e.Value { + existing.Spec.Env[i].Value = v + changed = true + } + } + if !changed { + return false, nil + } + if err := cl.Update(ctx, existing); err != nil { + return false, fmt.Errorf("refresh %s env: %w", existing.Name, err) + } + return true, nil +} + // The login gate is a hard prerequisite of the Owner bind, so setup waits for it // rather than racing it. The ceiling covers a cold image pull on a fresh node; // the poll is fast enough that a warm start feels immediate. diff --git a/cmd/felis/systemservers_test.go b/cmd/felis/systemservers_test.go index b053b25..190bd13 100644 --- a/cmd/felis/systemservers_test.go +++ b/cmd/felis/systemservers_test.go @@ -476,3 +476,108 @@ func TestSystemServerRconPolicy(t *testing.T) { t.Fatalf("lobby rcon port = %d, want 0 (operator default)", lobby.Spec.Rcon.Port) } } + +// A root-domain change leaves the login gate handing every joining player a console +// link built from the old domain — the one screen an unauthenticated player is +// guaranteed to see. setup is the only thing that ever rewrites that env, and it is +// create-if-absent, so before this it could not. The interesting half of the test is +// the second one: converging config must not become a licence to clobber the operator +// edits create-if-absent exists to protect. +func TestEnsureSystemServersRefreshesDerivedEnv(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("scheme: %v", err) + } + if err := v1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("scheme: %v", err) + } + + // The login gate as a pre-re-domain install left it, plus one env nobody but a + // human would have added and a memory bump off the built-in default. + stale := func() *v1alpha1.MinecraftServer { + ms, err := loginSystemServer("felis-limbo:demo", "minecraft", + "http://old.internal:8081", "159.223.32.51.nip.io", "console.159.223.32.51.nip.io") + if err != nil { + t.Fatalf("build stale login server: %v", err) + } + ms.Spec.Env = append(ms.Spec.Env, v1alpha1.EnvVar{Name: "OPERATOR_TUNING", Value: "keep-me"}) + ms.Spec.JavaMemory = "768Mi" + return ms + } + + run := func(cl client.Client) []systemServerOutcome { + return ensureSystemServers(ctx, cl, "minecraft", "felis-limbo:demo", "felis-lobby:demo", + "http://felis-api-internal.felis.svc.cluster.local:8081", + "mc.flyemoji.network", "console.mc.flyemoji.network") + } + + envOf := func(t *testing.T, cl client.Client) map[string]string { + t.Helper() + var ms v1alpha1.MinecraftServer + if err := cl.Get(ctx, client.ObjectKey{Namespace: "minecraft", Name: naming.SystemLoginServer}, &ms); err != nil { + t.Fatalf("get login server: %v", err) + } + got := make(map[string]string, len(ms.Spec.Env)) + for _, e := range ms.Spec.Env { + got[e.Name] = e.Value + } + return got + } + + t.Run("converges the console hostnames after a re-domain", func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(stale()).Build() + for _, out := range run(cl) { + if out.err != nil { + t.Fatalf("%s: %v", out.name, out.err) + } + if out.name == naming.SystemLoginServer && !strings.Contains(out.skipped, "refreshed") { + t.Errorf("login outcome = %+v, want the refresh reported so setup's output "+ + "does not read as if nothing happened", out) + } + } + env := envOf(t, cl) + if env[envPanelHostname] != "console.mc.flyemoji.network" { + t.Errorf("%s = %q — players are still being sent to the old console", + envPanelHostname, env[envPanelHostname]) + } + if env[envRootDomain] != "mc.flyemoji.network" { + t.Errorf("%s = %q, want the new root domain", envRootDomain, env[envRootDomain]) + } + }) + + t.Run("leaves operator edits alone", func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(stale()).Build() + run(cl) + + env := envOf(t, cl) + if env["OPERATOR_TUNING"] != "keep-me" { + t.Error("an env var the operator added by hand was dropped; create-if-absent " + + "exists so hand edits survive a re-run, and only config-derived names are ours") + } + var ms v1alpha1.MinecraftServer + if err := cl.Get(ctx, client.ObjectKey{Namespace: "minecraft", Name: naming.SystemLoginServer}, &ms); err != nil { + t.Fatalf("get login server: %v", err) + } + if ms.Spec.JavaMemory != "768Mi" { + t.Errorf("javaMemory = %q, want 768Mi — only env is converged, never the "+ + "rest of the spec", ms.Spec.JavaMemory) + } + }) + + t.Run("reports no refresh when config already matches", func(t *testing.T) { + fresh, err := loginSystemServer("felis-limbo:demo", "minecraft", + "http://felis-api-internal.felis.svc.cluster.local:8081", + "mc.flyemoji.network", "console.mc.flyemoji.network") + if err != nil { + t.Fatalf("build fresh login server: %v", err) + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(fresh).Build() + for _, out := range run(cl) { + if out.name == naming.SystemLoginServer && strings.Contains(out.skipped, "refreshed") { + t.Errorf("outcome = %+v — an unchanged install must not claim it rewrote "+ + "anything, or every setup run looks like a re-domain", out) + } + } + }) +}