Unverified Commit a0064adf authored by Minseong Choi's avatar Minseong Choi 💬
Browse files

fix(setup): stop the login gate sending players to the old console after a re-domain 

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.
parent 80a29ba6

cmd/felis/systemservers.go

+59 −1
Changes for cmd/felis/systemservers.go: 59 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -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.

cmd/felis/systemservers_test.go

+105 −0
Changes for cmd/felis/systemservers_test.go: 105 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -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)

			}

		}

	})

}