From 71e1664c36464cb64fa9ed7be6449777d847736c Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 12:58:46 +0900 Subject: [PATCH 01/10] build: pin line endings to LF so the embedded bootstrap.sh ships without CRs The repository had no .gitattributes. With core.autocrlf=true a Windows checkout handed deploy/bootstrap.sh 2374 CRs, and bootstrap_asset.go embeds that file from the working tree verbatim, so a dev-built felis piped a CRLF script into `bash -s` on the target host. CI builds on Linux, which is why released binaries were clean and only local builds carried it. eol=lf is global rather than scoped to *.sh because go:embed reaches further than the installer: deploy/*/Dockerfile, deploy/*/entrypoint.sh, plugins/*/src, the migrations and internal/panel/static are all compiled in and read on Linux. *.bat is the one exception, for the gradle wrappers' Windows launchers. Renormalizing the index touched exactly one tracked file, cmd/felis/version.go, and only its line endings: `git diff --cached --ignore-cr-at-eol` reports nothing outside .gitattributes itself. TestBootstrapPinsViaBlockConnectionsOff used to strip \r\n before asserting, with a comment stating that the repository pinned no eol attribute. That is no longer true, and the stripping hid the regression this commit prevents. It now asserts the absence of CRs, so losing the attribute reports itself as line endings rather than as a missing serverside-blockconnections pin. Closes #5 --- .gitattributes | 8 +++ bootstrap_asset_test.go | 12 +++- cmd/felis/version.go | 132 ++++++++++++++++++++-------------------- 3 files changed, 83 insertions(+), 69 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..b315b5c --- /dev/null +++ b/.gitattributes @@ -0,0 +1,8 @@ +# Everything in this repository is consumed on Linux: deploy/bootstrap.sh is embedded +# verbatim by bootstrap_asset.go and piped into `bash -s`, the Dockerfiles and entrypoints +# are read inside the images, and the operator ships YAML. Without eol=lf a Windows +# checkout under core.autocrlf=true hands all of them CRs. +* text=auto eol=lf + +# The gradle wrappers' Windows launchers are the one thing that wants CRLF. +*.bat text eol=crlf diff --git a/bootstrap_asset_test.go b/bootstrap_asset_test.go index b54c803..78914f8 100644 --- a/bootstrap_asset_test.go +++ b/bootstrap_asset_test.go @@ -73,9 +73,15 @@ func TestLobbyLuckPermsWiringIsConsistent(t *testing.T) { // default it can inherit — and nothing else in the install would notice it missing. The failure // surfaces only when a legacy player joins, on a host that installed cleanly. func TestBootstrapPinsViaBlockConnectionsOff(t *testing.T) { - // go:embed takes the working tree verbatim, and this repository pins no eol attribute, so - // a Windows checkout embeds CRLF. Only the assertion spanning a line break below cares. - script := strings.ReplaceAll(BootstrapScript(), "\r\n", "\n") + // go:embed takes the working tree verbatim, so the eol attribute is what keeps a Windows + // checkout from compiling CRs into the installer. Assert it rather than normalizing them + // away: the only assertion that would otherwise notice is the one spanning a line break + // below, and it would report a missing pin instead of the line endings. + script := BootstrapScript() + if strings.Contains(script, "\r\n") { + t.Fatal("embedded bootstrap.sh has CRLF line endings; .gitattributes pins *.sh to LF " + + "and this script is piped into `bash -s` on a Linux host") + } const key = "serverside-blockconnections" if !strings.Contains(script, key+": false") { diff --git a/cmd/felis/version.go b/cmd/felis/version.go index f3e983c..7654327 100644 --- a/cmd/felis/version.go +++ b/cmd/felis/version.go @@ -1,66 +1,66 @@ -package main - -import ( - "fmt" - "io" - "runtime" - "runtime/debug" -) - -// version is the build stamp injected at link time via -// -// -ldflags "-X main.version=v1.2.3" (release channel: the tag verbatim) -// -ldflags "-X main.version=v1.2.3+g1a2b3c4" (dev channel: tag + build metadata) -// -// deploy/bootstrap.sh computes it per install channel (FELIS_VERSION_BOOTSTRAP): -// the release channel stamps the resolved tag verbatim (v1.2.3), the dev channel -// stamps "+g". It stays "dev" for an un-stamped local -// `go build`, where ReadBuildInfo below still surfaces the vcs revision. -// -// NOT `git describe`, for two reasons that both bite. Its "--g" form -// puts the distance in the PRERELEASE field, which sorts BELOW the bare tag, so a -// dev build ahead of v1.2.3 would compare as older than v1.2.3 and `felis update` -// would propose "upgrading" onto the release it already contains — hence "+", which -// is build metadata and ignored for ordering. And bootstrap's primary clone is -// --depth 1, which carries no tags, so describe would fall back to a bare SHA that -// updates.Parse rejects outright. -var version = "dev" - -// cmdVersion prints the build stamp. It takes no flags and never touches the -// cluster, so it is safe to run as any user (unlike setup/breakGlass). -func cmdVersion(args []string, stdout, stderr io.Writer) int { - fmt.Fprintf(stdout, "felis %s\n", resolvedVersion()) - fmt.Fprintf(stdout, " go: %s %s/%s\n", runtime.Version(), runtime.GOOS, runtime.GOARCH) - if rev, ok := vcsRevision(); ok { - fmt.Fprintf(stdout, " revision: %s\n", rev) - } - return 0 -} - -// resolvedVersion prefers the ldflag stamp, then the module version recorded by -// `go install`, and only reports "unknown" when neither is present. -func resolvedVersion() string { - if version != "" { - return version - } - if bi, ok := debug.ReadBuildInfo(); ok && bi.Main.Version != "" { - return bi.Main.Version - } - return "unknown" -} - -// vcsRevision returns the git commit the binary was built from when the build -// carried VCS stamping (local `go build` in a checkout; the docker build strips -// .git, so there the ldflag version carries the identity instead). -func vcsRevision() (string, bool) { - bi, ok := debug.ReadBuildInfo() - if !ok { - return "", false - } - for _, s := range bi.Settings { - if s.Key == "vcs.revision" && s.Value != "" { - return s.Value, true - } - } - return "", false -} +package main + +import ( + "fmt" + "io" + "runtime" + "runtime/debug" +) + +// version is the build stamp injected at link time via +// +// -ldflags "-X main.version=v1.2.3" (release channel: the tag verbatim) +// -ldflags "-X main.version=v1.2.3+g1a2b3c4" (dev channel: tag + build metadata) +// +// deploy/bootstrap.sh computes it per install channel (FELIS_VERSION_BOOTSTRAP): +// the release channel stamps the resolved tag verbatim (v1.2.3), the dev channel +// stamps "+g". It stays "dev" for an un-stamped local +// `go build`, where ReadBuildInfo below still surfaces the vcs revision. +// +// NOT `git describe`, for two reasons that both bite. Its "--g" form +// puts the distance in the PRERELEASE field, which sorts BELOW the bare tag, so a +// dev build ahead of v1.2.3 would compare as older than v1.2.3 and `felis update` +// would propose "upgrading" onto the release it already contains — hence "+", which +// is build metadata and ignored for ordering. And bootstrap's primary clone is +// --depth 1, which carries no tags, so describe would fall back to a bare SHA that +// updates.Parse rejects outright. +var version = "dev" + +// cmdVersion prints the build stamp. It takes no flags and never touches the +// cluster, so it is safe to run as any user (unlike setup/breakGlass). +func cmdVersion(args []string, stdout, stderr io.Writer) int { + fmt.Fprintf(stdout, "felis %s\n", resolvedVersion()) + fmt.Fprintf(stdout, " go: %s %s/%s\n", runtime.Version(), runtime.GOOS, runtime.GOARCH) + if rev, ok := vcsRevision(); ok { + fmt.Fprintf(stdout, " revision: %s\n", rev) + } + return 0 +} + +// resolvedVersion prefers the ldflag stamp, then the module version recorded by +// `go install`, and only reports "unknown" when neither is present. +func resolvedVersion() string { + if version != "" { + return version + } + if bi, ok := debug.ReadBuildInfo(); ok && bi.Main.Version != "" { + return bi.Main.Version + } + return "unknown" +} + +// vcsRevision returns the git commit the binary was built from when the build +// carried VCS stamping (local `go build` in a checkout; the docker build strips +// .git, so there the ldflag version carries the identity instead). +func vcsRevision() (string, bool) { + bi, ok := debug.ReadBuildInfo() + if !ok { + return "", false + } + for _, s := range bi.Settings { + if s.Key == "vcs.revision" && s.Value != "" { + return s.Value, true + } + } + return "", false +} -- 2.54.0 From 3af5cc360c28e9adbfba461dd4f216bbe967a9f2 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 13:49:07 +0900 Subject: [PATCH 02/10] docs(troubleshooting): correct four fields the runbook documents as inert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sections 11 and 12 told the operator that idle auto-stop, both startup budgets, and the player tally are read by nobody. All four are read, and §1 repeated the same claim in its strongest form: "the operator has no start timeout ... loops forever". spec.idle.autoStopEnabled reconciler.go:175 spec.idle.emptySecondsBeforeStop reconciler.go:175 spec.startup.timeoutSeconds reconciler.go:479, called at :126 spec.startup.readinessTimeoutSeconds reconciler.go:490, called at :157 status.players.online reconciler.go:413 (markRunningReady) The repository already contained the disproof: TestReconcileRunning_StartupTimeoutConvertsToFailed and TestReconcileRunning_ReadinessTimeoutConvertsToFailed both assert the escalation §1 said does not exist. The test §1 cited, TestReconcileRunning_RconProbeFailureStaysStarting, only asserts that a single failed probe does not flap the phase; that was read as "forever". §11 was the costly one, because it misdiagnosed a configuration problem as a missing feature. Idle auto-stop and the player tally both hang off spec.rcon.enabled -- the tally is a by-product of the RCON readiness probe (prober.go:63 runs `list`), and reconciler.go:175 carries the RCON condition explicitly so a never-sampled zero cannot stop a server full of people. Following the old text, an operator whose RCON was never enabled would conclude the feature was unwritten and stop. The section now opens with the jsonpath that reads spec.rcon.enabled. Also separated the prober's fixed 5s dial timeout (prober.go:45) from spec.startup.readinessTimeoutSeconds, which §1c conflated: the former bounds one probe, the latter is a deadline for the whole start measured from status.startRequestedAt. spec.storage.retainOnDelete is the one field still genuinely inert, so the [INERT] legend and §12 stay -- §12 now records the condition each read field depends on instead of claiming none of them are read. --- docs/troubleshooting.md | 89 +++++++++++++++++++++++++++-------------- 1 file changed, 60 insertions(+), 29 deletions(-) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 6f63538..cbf19b9 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -20,8 +20,8 @@ graded for how far the in-repo Go test suite proves the behaviour: containerd, Postgres, or the network, not by Felis Go code; you will see it in `kubectl describe` / pod logs, never in `MinecraftServer.status`. - **[INERT]** — the configuration field exists in the CRD but no controller - reads it. Tuning it does nothing. These are the highest-value traps and are - collected in §10. + reads it. Tuning it does nothing. §12 lists the one field this still applies + to, alongside the fields that *are* read and the condition each depends on. The operator never invents the parent domain; routing identity is `spec.subdomain` under the deployment zone. Examples below use @@ -32,14 +32,21 @@ concrete host. ## 1. Server is stuck in `Starting` and never becomes `Running` -`MinecraftServer.status.phase` stays `Starting`. The single most important fact: -**the operator has no start timeout.** A start that never succeeds loops in -`Starting`, requeued every 5s, **forever** — it is never auto-escalated to -`Failed`. [GO-TESTED: `TestReconcileRunning_RconProbeFailureStaysStarting` -asserts the phase stays `Starting`.] Do **not** reach for -`spec.startup.timeoutSeconds` / `spec.startup.readinessTimeoutSeconds` — both are -[INERT] (§10). A hung start must be diagnosed from pod state, not from -`MinecraftServer.status`. +`MinecraftServer.status.phase` stays `Starting`. A start that never succeeds is +requeued every 5s until one of the two startup budgets expires, then escalated +to `Failed` — `StartupTimeout` if the pod never passed TCP readiness, +`ReadinessTimeout` if the RCON probe never succeeded. Both default to **300s** +when `spec.startup.timeoutSeconds` / `spec.startup.readinessTimeoutSeconds` are +unset or `0`, and both are measured from `status.startRequestedAt`. +[GO-TESTED: `TestReconcileRunning_StartupTimeoutConvertsToFailed`, +`TestReconcileRunning_ReadinessTimeoutConvertsToFailed`.] + +So `Starting` seen *once* is normal and +`TestReconcileRunning_RconProbeFailureStaysStarting` asserts exactly that — a +single failed probe must not flap the phase. `Starting` seen for longer than the +budget means the reconcile loop is not running at all; check the operator's own +logs before tuning anything. Either way the underlying cause is diagnosed from +pod state, not from `MinecraftServer.status`. First, read the condition reason: @@ -111,8 +118,10 @@ message is the verbatim dial error: port yet, RCON is disabled in `server.properties`, or `spec.rcon.port` (default 25575) is wrong. [INTEGRATION-ONLY for the live handshake.] -The per-probe timeout is a fixed 5s in code — it is **not** derived from -`spec.startup.readinessTimeoutSeconds` ([INERT]). +The per-probe timeout is a fixed 5s in code (`prober.go:45`, shortened further if +the reconcile context has a nearer deadline). It is **not** derived from +`spec.startup.readinessTimeoutSeconds`, which is the deadline for the whole +start, not for one probe — see §12. --- @@ -463,14 +472,35 @@ store paths are [INTEGRATION-ONLY]. ## 11. Idle auto-stop never fires; player count always shows 0 -**Idle auto-stop is entirely unimplemented in the operator.** `spec.idle.*` -([INERT], §12) is read by no controller, and no idle controller is registered. -An empty `Running` server stays `Running`. +Both are implemented, and both hang off the same switch: **`spec.rcon.enabled`**. +Check it first. -Relatedly, `status.players.online` is **permanently 0**: the only writer of -`status.players` zeroes it on stop, and the RCON prober only dials + closes — it -never runs `list`. Any panel reading `status.players.online` will always show -empty. Do not build alerting on it, and do not expect "empty server" automation. +```sh +kubectl get minecraftserver -o jsonpath='{.spec.rcon.enabled}' +``` + +The player tally is a by-product of the RCON readiness probe — `prober.go:63` +runs `list` on the same connection that just authenticated, and `parseListReply` +extracts the tally from `There are (\d+) of a max of (\d+) players online`. With +RCON disabled the probe never runs, `players` keeps its zero value, and +`markRunningReady` (`reconciler.go:413`) writes that zero into +`status.players.online`. So a permanent 0 means "never sampled", not "nobody +online". + +Idle auto-stop (`reconciler.go:175`) reads that same tally, which is why it +carries the RCON condition explicitly: + +```go +if server.Spec.Rcon.Enabled && server.Spec.Idle.AutoStopEnabled && server.Spec.Idle.EmptySecondsBeforeStop > 0 { +``` + +The comment above it says why: with RCON off the zero tally "would read as +'empty' and use to stop a server full of people". So the guard is deliberate — +enabling `spec.idle.*` without RCON is a no-op by design, not a missing feature. + +Both fields set and still nothing happens? Then the probe is failing rather than +disabled: the server would be stuck in `Starting` with `RconNotReachable` +(`reconciler.go:156`), which is §1's symptom, not this one. Note the reaper's `last_active_at` (§10) is a *different* subsystem (Postgres business layer, bumped by join events) — it keeps worlds alive against the @@ -478,22 +508,23 @@ reaper, but it does **not** auto-stop empty running servers. --- -## 12. Inert configuration fields (highest-value traps) +## 12. A configuration field seems to be ignored -These CRD fields exist and validate, but **no controller reads them.** Setting -them has **no effect**. Verified by grep: each appears only in the type -definition and its deepcopy, never in a controller. +One CRD field exists and validates but is read by no controller; the rest of +this table records fields that *are* read, together with the condition that +decides whether setting them does anything. | Field | What you might expect | Reality | |---|---|---| -| `spec.startup.timeoutSeconds` | Start budget before `Failed` | **[INERT]** — no Starting→Failed timeout exists; a hung start loops forever (§1) | -| `spec.startup.readinessTimeoutSeconds` | First-probe budget | **[INERT]** — probe timeout is a fixed 5s in code | -| `spec.idle.autoStopEnabled` | Auto-stop empty servers | **[INERT]** — idle auto-stop unimplemented (§11) | -| `spec.idle.emptySecondsBeforeStop` | Empty grace period | **[INERT]** | +| `spec.startup.timeoutSeconds` | Start budget before `Failed` | Read by `startupTimedOut` (`reconciler.go:479`), called at `:126`. `0` or unset falls back to **300s**, then `markFailed("StartupTimeout")` | +| `spec.startup.readinessTimeoutSeconds` | First-probe budget | Read by `readinessTimedOut` (`reconciler.go:490`), called at `:157`. `0` or unset falls back to **300s**, then `markFailed("ReadinessTimeout")`. Not to be confused with the prober's own 5s dial timeout (`prober.go:45`) | +| `spec.idle.autoStopEnabled` | Auto-stop empty servers | Read at `reconciler.go:175` — but gated on `spec.rcon.enabled`, since the player tally comes from the RCON probe (§11) | +| `spec.idle.emptySecondsBeforeStop` | Empty grace period | Same branch. Must be `> 0`; the guard treats `0` as "off", not "stop immediately" | | `spec.storage.retainOnDelete` | Keep/drop PVC on delete | **[INERT]** — world PVCs **always** survive server deletion; only the reaper ever deletes a world PVC (§13) | -If a runbook tells someone to "tune `startup.timeoutSeconds`" for a hung start, -it is wrong — use `kubectl describe pod` (§1a) instead. +Both startup budgets are measured from the same `status.startRequestedAt`, so +`readinessTimeoutSeconds` is not a budget *after* pod readiness — it is a +deadline for the whole start, applied on the RCON-probe branch. --- -- 2.54.0 From 82a1275fcf629565be2384c14695a4aacf3247ee Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 14:33:37 +0900 Subject: [PATCH 03/10] docs(plugins): say which plugin jars an install actually produces The module table listed felis-fabric, felis-forge and felis-neoforge next to the two jars a finished install really has, with nothing distinguishing them. Neither deploy/bootstrap.sh nor the embed set in bootstrap_asset.go builds a loader mod, so someone reading the table expected three jars that are not there after setup and had no way to tell from this file. The mods do build -- the wrapper commands under Building work -- they are just never installed for you, which is what the new column says. Two further disagreements with the code, in the same table: limbo/ was missing entirely. It is embedded, built by bootstrap.sh and running on the login gate, so the one module the table omitted was a shipped one. It is also the only module that reaches the account-link endpoint without a command: it mints the code on join for anyone unlinked and holds them until they redeem it, so the opening claim that every module except the lobby ships /link named the wrong exception. velocity was listed as felis-velocity-0.2.0.jar; plugins/velocity/build.gradle:6 says 0.1.0, as does every other module. Nothing breaks on this because bootstrap.sh globs felis-velocity-*.jar and installs it under a fixed name, but the version in the table was not a version anything produces. The Gradle table and the JDK note now carry limbo's Java-21 toolchain, which it needs for the same reason paper does and for a different cause: LOOHP/Limbo releases are class-file major 65, so the compiler JDK must be able to read them. It still emits release 17 bytecode. --- plugins/README.md | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/plugins/README.md b/plugins/README.md index 905cfd5..291dc82 100644 --- a/plugins/README.md +++ b/plugins/README.md @@ -1,12 +1,16 @@ # Felis server-side plugins -These are the in-cluster and edge plugins for Felis. Every module **except the -lobby** ships the in-game first leg of the §10 account-link flow: a player who is already online +These are the in-cluster and edge plugins for Felis. The Velocity proxy and the three loader +mods ship the in-game first leg of the §10 account-link flow: a player who is already online (so Mojang has verified their UUID) runs `/link`; the plugin asks felis-api to mint a one-time code for that UUID and shows it in chat. The player then enters the code on the web console → **Account** page (the second leg), which binds the code to their logged-in account. The web side is already built. +The **limbo** module reaches the same felis-api endpoint without a command: it is the login +gate, so it mints the code on join for anyone not yet linked and holds them until they redeem +it. The **paper** lobby ships neither — see below. + The **Velocity** module additionally carries the §11 domain-autostart routing loop — recognizing each server's subdomain, registering backends dynamically, waking a sleeping target and holding the player until it is ready. It is a full @@ -19,14 +23,22 @@ The **Paper** module is different in kind: it is the §12 lobby UI face. It ship to Velocity, which is the only side that ever talks to felis-api. See **[Lobby menu](#lobby-menu-§12)** below. -| Module | Platform | Target | Jar | -| ------------------ | --------------------------- | ----------------------------------- | ---------------------------- | -| `velocity/` | Velocity proxy plugin | velocity-api 3.3.0-SNAPSHOT | `felis-velocity-0.2.0.jar` | -| `fabric/` | Fabric server mod | MC 1.20.1 / fabric-loader 0.16.x | `felis-fabric-0.1.0.jar` | -| `forge/` | Forge server mod | MC 1.20.1 / Forge 47.3.0 | `felis-forge-0.1.0.jar` | -| `neoforge/` | NeoForge server mod | MC 1.20.4 / NeoForge 20.4.251 | `felis-neoforge-0.1.0.jar` | -| `paper/` | Paper server plugin (lobby) | paper-api 1.21.4-R0.1-SNAPSHOT | `felis-paper-0.1.0.jar` | -| `shared/` | *(not built on its own)* | — | source compiled into each | +| Module | Platform | Target | Jar | Built by the installer | +| ------------------ | --------------------------- | ----------------------------------- | ---------------------------- | ---------------------- | +| `velocity/` | Velocity proxy plugin | velocity-api 3.3.0-SNAPSHOT | `felis-velocity-0.1.0.jar` | yes | +| `limbo/` | LOOHP/Limbo plugin (login) | Limbo API / Java 17 bytecode | `felis-limbo-0.1.0.jar` | yes | +| `paper/` | Paper server plugin (lobby) | paper-api 1.21.4-R0.1-SNAPSHOT | `felis-paper-0.1.0.jar` | yes | +| `fabric/` | Fabric server mod | MC 1.20.1 / fabric-loader 0.16.x | `felis-fabric-0.1.0.jar` | no | +| `forge/` | Forge server mod | MC 1.20.1 / Forge 47.3.0 | `felis-forge-0.1.0.jar` | no | +| `neoforge/` | NeoForge server mod | MC 1.20.4 / NeoForge 20.4.251 | `felis-neoforge-0.1.0.jar` | no | +| `shared/` | *(not built on its own)* | — | source compiled into each | source only | + +"Built by the installer" is what `deploy/bootstrap.sh` produces, and it is the same set +`bootstrap_asset.go` embeds into the felis binary for the TUI install path, which has no source +checkout to build from. **The three loader mods are not in that set** — a finished install has +no `felis-fabric`/`felis-forge`/`felis-neoforge` jar anywhere. They build from this checkout with +the commands under [Building](#building) and are deployed by hand; the account-link flow they +carry works, but nothing installs them for you. ## Architecture @@ -176,6 +188,7 @@ preference): | Module | Gradle | Why | | ----------- | ----------- | --------------------------------------------------------------- | | `velocity` | 9.5.1 (system) | plain `java` plugin — no loader Gradle plugin | +| `limbo` | 9.5.1 (system), **JDK 21 toolchain** | plain `java` plugin; current LOOHP/Limbo releases ship class-file major 65, so the compiler JDK must be ≥ 21 to read them. It emits `release 17` bytecode, so the jar still loads on any Limbo running Java 17+ | | `fabric` | 8.8 (wrapper) | loom 1.7.4 uses `Problems.forNamespace`, removed in Gradle 9 | | `forge` | 8.8 (wrapper) | ForgeGradle 6 is Gradle-8-only | | `neoforge` | 8.14 (wrapper) | NeoGradle 7.1.38 requires Gradle API ≥ 8.14 | @@ -185,18 +198,20 @@ preference): # Velocity — system Gradle is fine gradle -p plugins/velocity build -# Paper — system Gradle too, but it compiles on a Java-21 toolchain (see table) +# Paper and limbo — system Gradle too, but both compile on a Java-21 toolchain (see table) gradle -p plugins/paper build +gradle -p plugins/limbo build -# Fabric / Forge / NeoForge — use the per-module wrapper +# Fabric / Forge / NeoForge — use the per-module wrapper. Nothing installs these; the jar you +# want is the one this produces. plugins/fabric/gradlew -p plugins/fabric build plugins/forge/gradlew -p plugins/forge build plugins/neoforge/gradlew -p plugins/neoforge build ``` -Requires JDK 17 — **except `paper`, which needs a Java-21 toolchain available to -Gradle** (paper-api 1.21.4 is a Java-21 artifact; the rest of the suite is Java -17). The first build of each mod downloads and remaps/decompiles Minecraft, so it +Requires JDK 17 — **except `paper` and `limbo`, which need a Java-21 toolchain available to +Gradle** (paper-api 1.21.4 is a Java-21 artifact and the Limbo API is compiled to major 65; the +rest of the suite is Java 17). The first build of each mod downloads and remaps/decompiles Minecraft, so it takes a few minutes; subsequent builds are fast. Jars land in each module's `build/libs/`. -- 2.54.0 From 417769407f3fd4f83ba72125024458a9ce8d363e Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 17:27:00 +0900 Subject: [PATCH 04/10] ci: run the checks on push and pull request release.yml was the only workflow and it fires on v* tags, so `go vet` and `go test` first met a change once that change was already on the release path, where the only remedy is another tag. The panel suite ran nowhere at all: a release goes through the Dockerfile and the Dockerfile runs `npm run build`, never `npm test`. 111 assertions across 8 files existed and nothing outside a developer's checkout ever executed them. Both jobs are green as of this commit, checked before writing it rather than after: go vet and go test ./... (24 packages, 0 failures, on Linux), npm test (8 files, 111 tests) and npm run typecheck. A gate that lands red is a gate everyone learns to ignore. The panel's Node version is read out of the Dockerfile instead of repeated here. `FROM node:` is the only place the tree declares it -- no .nvmrc, no engines field -- so a copy in this file would keep testing 22 the first time the image moved. That is the class of drift this workflow exists to catch, not to introduce. The step fails loudly if the FROM line stops matching. Tags are excluded from the push trigger. A v* push already runs release.yml, which repeats the Go job, and this is a private repository billed for both. --- .github/workflows/ci.yml | 68 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..5253371 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,68 @@ +# Runs the checks on every push and pull request. +# +# release.yml already runs `go vet` and `go test`, but only once a vX.Y.Z tag exists — by then +# a red change is on the release path and the only remedy is a new tag. This is the same gate +# moved to where it can still stop something, plus the panel suite, which nothing ran at all: +# the release goes through the Dockerfile, and the Dockerfile runs `npm run build`, never +# `npm test`. +# +# Tags are excluded from the push trigger. A vX.Y.Z push fires release.yml, which repeats the +# Go job itself; running both would spend a private repository's Actions minutes twice for one +# answer. +name: ci + +on: + push: + branches: ['**'] + pull_request: + +permissions: + contents: read + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +jobs: + go: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + + - run: go vet ./... + - run: go test ./... + + panel: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + # The Dockerfile's `FROM node:` is the only place the panel's Node version is + # declared — there is no .nvmrc and no engines field. Reading it here rather than + # repeating the number keeps CI testing the version a release is actually built on; + # a second copy would drift silently the first time the image is bumped. + - name: Read the panel's Node version from the Dockerfile + id: node + run: | + version="$(sed -n 's/^FROM.*node:\([0-9][0-9]*\)-.*/\1/p' Dockerfile | head -1)" + [ -n "$version" ] || { echo "Dockerfile has no 'FROM ... node:' line"; exit 1; } + echo "version=${version}" >> "$GITHUB_OUTPUT" + + - uses: actions/setup-node@v4 + with: + node-version: ${{ steps.node.outputs.version }} + cache: npm + cache-dependency-path: panel/package-lock.json + + - run: npm ci + working-directory: panel + + - run: npm test + working-directory: panel + + - run: npm run typecheck + working-directory: panel -- 2.54.0 From 23792d6251f261d9f92a515248bc2e8d94b907f7 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 17:27:01 +0900 Subject: [PATCH 05/10] fix(crd): remove spec.storage.retainOnDelete rather than leave it inert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The field validated, shipped in the CRD, and reached no controller. The world PVC survives deletion unconditionally -- it is a StatefulSet VolumeClaimTemplate, StatefulSet deletion does not cascade to template PVCs, and no finalizer exists anywhere in the operator. So setting it true described what already happened, and setting it false did nothing at all. False is the worse half: it reads as a request to delete a world, and was silently ignored. This departs from spec v4.1 §5, which asks for "删除:finalizer 清 Service/STS/ConfigMap,PVC 按 retainOnDelete". Neither half was ever built. Restoring that line means adding a finalizer whose other listed duties -- Service, StatefulSet, ConfigMap -- ownerReference GC already performs, so the only work it would newly do is delete worlds, on a path that does not pass the reaper's verified-backup check. The reaper is the one thing in the system allowed to destroy a world and it earns that by proving a backup first. A second door without that check is not an improvement. The spec is a frozen versioned document, so it is left alone and the departure is recorded in troubleshooting.md §13, beside the behaviour it explains. §12 loses its inert row and its opening sentence, which existed to introduce this one field: every field in that table is now read by a controller. Deployed installs need nothing. A CR still carrying retainOnDelete keeps working, because a v1 CRD prunes unknown keys on the next write and the behaviour the field claimed to control was never conditional. go build, go vet and go test ./... pass on Linux with zero failures; the CRD still parses and storage keeps size and storageClassName. --- .../felis.lolicon.best_minecraftservers.yaml | 4 --- docs/troubleshooting.md | 27 +++++++++++++------ .../felis/v1alpha1/minecraftserver_types.go | 2 -- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/deploy/crd/felis.lolicon.best_minecraftservers.yaml b/deploy/crd/felis.lolicon.best_minecraftservers.yaml index e51642b..f58883f 100644 --- a/deploy/crd/felis.lolicon.best_minecraftservers.yaml +++ b/deploy/crd/felis.lolicon.best_minecraftservers.yaml @@ -269,10 +269,6 @@ spec: storage: description: Storage configures the world PVC. properties: - retainOnDelete: - description: RetainOnDelete keeps the PVC when the MinecraftServer - is deleted. - type: boolean size: description: Size is the requested PVC capacity (e.g. "10Gi"). type: string diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index cbf19b9..c9fe44a 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -510,9 +510,8 @@ reaper, but it does **not** auto-stop empty running servers. ## 12. A configuration field seems to be ignored -One CRD field exists and validates but is read by no controller; the rest of -this table records fields that *are* read, together with the condition that -decides whether setting them does anything. +Every field below is read by a controller. What varies is the condition that +decides whether setting it does anything. | Field | What you might expect | Reality | |---|---|---| @@ -520,7 +519,6 @@ decides whether setting them does anything. | `spec.startup.readinessTimeoutSeconds` | First-probe budget | Read by `readinessTimedOut` (`reconciler.go:490`), called at `:157`. `0` or unset falls back to **300s**, then `markFailed("ReadinessTimeout")`. Not to be confused with the prober's own 5s dial timeout (`prober.go:45`) | | `spec.idle.autoStopEnabled` | Auto-stop empty servers | Read at `reconciler.go:175` — but gated on `spec.rcon.enabled`, since the player tally comes from the RCON probe (§11) | | `spec.idle.emptySecondsBeforeStop` | Empty grace period | Same branch. Must be `> 0`; the guard treats `0` as "off", not "stop immediately" | -| `spec.storage.retainOnDelete` | Keep/drop PVC on delete | **[INERT]** — world PVCs **always** survive server deletion; only the reaper ever deletes a world PVC (§13) | Both startup budgets are measured from the same `status.startRequestedAt`, so `readinessTimeoutSeconds` is not a budget *after* pod readiness — it is a @@ -534,16 +532,29 @@ This is expected. The world PVC is a StatefulSet `VolumeClaimTemplate`. There is **no `persistentVolumeClaimRetentionPolicy` and no finalizer** anywhere in the operator. Deleting the `MinecraftServer` garbage-collects the StatefulSet, but StatefulSet deletion does **not** cascade to its template PVCs, and nothing else -cleans them up. So the world PVC **always survives** server deletion, regardless -of `spec.storage.retainOnDelete` ([INERT], §12). The **only** code that deletes a -world PVC is the reaper, and only after a verified backup (§10). To reclaim a -world PVC manually: +cleans them up. So the world PVC **always survives** server deletion. The +**only** code that deletes a world PVC is the reaper, and only after a verified +backup (§10). To reclaim a world PVC manually: ``` kubectl get pvc -l app.kubernetes.io/name= kubectl delete pvc # irreversible — the world is gone ``` +`spec.storage.retainOnDelete` sat in the CRD and reached no controller. Spec +v4.1 §5 asks for it — 「删除:finalizer 清 Service/STS/ConfigMap,PVC 按 +`retainOnDelete`」 — and neither half was ever built: there is no finalizer, and +nothing read the field. It was removed rather than implemented, which is a +deliberate departure from that line, recorded here because the spec is a frozen +document and still says otherwise. + +The reasoning is that implementing it buys a second path that deletes a world — +one that skips the reaper's verified-backup check — in order to restore a +finalizer whose other listed duties (Service, StatefulSet, ConfigMap) +ownerReference GC already performs. A CR still carrying the field keeps working: +the API server prunes the unknown key on its next write, and nothing above +changes, because retention was never conditional in the first place. + --- ## 14. Metrics for diagnosis (spec §23) diff --git a/internal/apis/felis/v1alpha1/minecraftserver_types.go b/internal/apis/felis/v1alpha1/minecraftserver_types.go index d175428..0c80348 100644 --- a/internal/apis/felis/v1alpha1/minecraftserver_types.go +++ b/internal/apis/felis/v1alpha1/minecraftserver_types.go @@ -192,8 +192,6 @@ type StorageSpec struct { Size string `json:"size,omitempty"` // StorageClassName selects the StorageClass; empty uses the default. StorageClassName string `json:"storageClassName,omitempty"` - // RetainOnDelete keeps the PVC when the MinecraftServer is deleted. - RetainOnDelete bool `json:"retainOnDelete,omitempty"` } // LifecycleSpec tunes graceful shutdown (spec §7). The operator injects a -- 2.54.0 From afdbfac7a8ffe2e15a85ea836ba6586853575bf4 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 17:27:01 +0900 Subject: [PATCH 06/10] docs: index the deferred integration seams and correct two stale markers INTEGRATION-ONLY and KNOWN-LIMITATION are grep-able, but the grep answers the wrong question. Thirty-four Go sites share the two markers and they carry four different meanings: "declared, nothing implements it" reads exactly like "implemented, only its I/O is unreachable from here", and neither reads differently from a limitation that was accepted on purpose and is not coming back. docs/deferred-seams.md sorts them, following the bucketed shape internal/updater/doc.go already uses for its own package rather than starting a second convention. Sorting them turned up two markers that had outlived the condition they describe. config.go called the modpack upload transport a deferred integration after both backends had shipped -- LocalContextStore and S3ContextStore, selected in cmd/felis by the shape of user_uploads_context, with the uploads PVC mounted and the felis-uploads-s3 Secret rendered. What is still deferred is the far end: Kaniko reading that context from inside the build Pod. updater/doc.go listed the `felis update` CLI and the off-cluster Velocity jar read under REMAINING INTEGRATION. Both exist -- cmd/felis/update.go, and gatherer_host.go, which answers Velocity from the installed jar's manifest and felis-api from the running binary's build stamp. The two nil seams that bullet also names are real, but they belong to the in-cluster gatherer only, so the bullet now says which caller has what and which is still empty. The index also records the collision that makes a naive grep misleading: docs/troubleshooting.md uses [INTEGRATION-ONLY] for something else, defined in its own opening at :19 -- the symptom is produced by the kubelet, kaniko or a live handshake, so it cannot be reproduced from the repository. Those twelve marks say where a failure comes from, not that something is unbuilt, and are excluded. Both code changes are comments. Every file:line the index cites was checked against the line it points at. --- docs/deferred-seams.md | 113 ++++++++++++++++++++++++++++++++++++++ internal/config/config.go | 9 ++- internal/updater/doc.go | 18 +++--- 3 files changed, 130 insertions(+), 10 deletions(-) create mode 100644 docs/deferred-seams.md diff --git a/docs/deferred-seams.md b/docs/deferred-seams.md new file mode 100644 index 0000000..7c53a6b --- /dev/null +++ b/docs/deferred-seams.md @@ -0,0 +1,113 @@ +# Deferred integration seams + +`INTEGRATION-ONLY` and `KNOWN-LIMITATION` are grep-able markers in the Go source. +This file is the index of what each one currently means, so that reading the +unfinished face of the system does not require re-deriving it from 34 comment +sites. + +It exists for two reasons. The markers do not all mean the same thing — four +distinct states share them, and "declared, nothing implements it" reads exactly +like "implemented, but its I/O cannot be exercised from this repo". And a marker +outlives the condition it describes: two of them were stale when this index was +first assembled, both claiming as future work something that had already shipped. + +The pattern here is the one `internal/updater/doc.go` already uses for its own +package — state the verification boundary in buckets, so a green test suite is not +mistaken for a finished integration. This file is the same idea across the whole +tree. + +## The marker collides with a different vocabulary in troubleshooting.md + +`docs/troubleshooting.md` uses `[INTEGRATION-ONLY]` for something else, defined in +its own opening at `:19`: the symptom is produced by the kubelet, kaniko or a live +handshake, so it cannot be reproduced from the repository. Those twelve marks say +where a failure comes from. They are not unfinished work and are not indexed below. +A grep across `*.md` and `*.go` returns both sets; only the Go ones are seams. + +## Declared, nothing implements it + +- `internal/updates/seams.go:32` — `Notifier`. `internal/mail` sends OTP over SMTP, + but nothing adapts it to this interface and no in-game channel exists. `felis + update` passes nil deliberately: a human typing the command is the notification. +- `internal/updates/seams.go:43` — `Applier`. Nothing applies an update anywhere. A + nil applier is not silent — `Run` records `errNoApplier` against every planned + apply, so a mis-scheduled apply is loud rather than lost. +- `internal/updater/gatherer_integration.go:22` — the two current-version seams + `NewSysGatherer` leaves nil, for the in-cluster path: the k8s read of the + control-plane Deployment image, and the Velocity jar inspection. Both are answered + on the host path (see "Built" below), so this gap is specific to a caller that has + a cluster client instead of the node. +- `internal/api/handlers_updates.go:21,28` — the maintenance window persists and the + API serves it, but the in-cluster CronJob that would hand a real window to a runner + does not exist. `felis update` runs with a zero window, under which every + `Scheduled` component degrades to a notify, so no path can currently claim an + apply is under way. +- `internal/submit/blobstore.go:40` — the uploads PVC is mounted into felis-api but + not into the Kaniko build Pod, so a submitted context is durable at the derived + location without yet being readable by the build that consumes it. + +## Built; only its I/O is unverifiable from this repo + +Code exists and is unit-tested against fakes. What is missing is a host, a cluster +or a real upstream account to run it against — not an implementation. + +- `cmd/felis/tui_edge_apply.go:246,274,295` — the `nft` edge fence, its idempotent + teardown, and the cloudflared invocation. +- `internal/cfsetup/runner.go:18` and `internal/cfsetup/cfsetup.go:329` — the real + Cloudflare Tunnel and Access API calls; `internal/cfsetup/cfsetup_test.go:11` + drives the whole flow through a fake. +- `internal/api/console.go:39`, `internal/api/logstream.go:236`, + `internal/api/logstream.go:306`, `internal/fileedit/k8sjobs.go:45` — each needs a + live cluster (RCON, `pods/log` follow, a Job). +- `internal/api/handlers_access.go:170,490` — parsing real vanilla and LuckPerms + command output. + +## Deliberately accepted, not scheduled to close + +These are decisions, not backlog. Each names the condition under which it would be +worth revisiting. + +- `internal/api/pgrepo.go:281` — the quota check and `ClaimServer` are two statements + (audit #4 TOCTOU). Closeable only against a real Postgres. +- `internal/api/api.go:671` — `cooldownLimiter` is process-local, so across N api + replicas a caller could draw up to N OTP codes per window. The intra-replica burst + is closed; cross-replica bounding needs a shared store, out of scope for a + single-replica install. +- `internal/submit/submit.go:436` and `internal/submit/submit_test.go:351` — a + post-CAS `Approve` + failure leaves a row indistinguishable from the benign case, so `Approve` returns a + distinct error naming the running build rather than allowing a blind re-drive that + would double-push. The alternative ordering is worse. +- `cmd/felis/tui_edge_apply.go:246`, second marker on the same site — the fence + targets nftables. On a firewalld host a reload can flush the standalone table; + firewalld-native coordination is not handled. A missing `nft` binary fails loud + rather than leaving the port open. +- `internal/api/handlers_account.go:169` — the reclaim "start fresh vs inherit" + choice is CODE-ONLY on the Java/Velocity side; the link-status endpoint reports + link completion only and does not surface it. + +## Wired since the marker was written + +- `internal/api/handlers_email_otp.go:54,223,227` and `internal/api/api.go:84` — + SMTP shipped on + 2026-07-20 (`internal/mail`, wired at `cmd/felis/api.go:264`). The nil-`Mailer` + branch that logs the code server-side is a runtime fallback for an install with no + `[smtp]` section, not an unbuilt feature. The comments are accurate; the reading + "Felis cannot send mail" is not. +- `internal/config/config.go:117` — was stale. It described the upload transport as a + deferred integration after both backends had shipped (`LocalContextStore`, + `S3ContextStore`, selected in `cmd/felis/api.go` by the shape of the configured + base). Corrected in the change that added this file; what remains deferred is only + Kaniko's read, indexed above. +- `internal/updater/doc.go:44` — was stale. Its REMAINING INTEGRATION bullet listed + the `felis update` CLI and the off-cluster Velocity jar read, both of which exist + (`cmd/felis/update.go`, `internal/updater/gatherer_host.go`). Corrected in the same + change; the two nil seams it also names are real and remain above. + +## Recorded outside the code + +- `deploy/limbo/README.md:139` — no NetworkPolicy locks the minecraft-namespace + egress or the control-namespace ingress today, which is why the login pod reaches + `felis-api-internal:8081`. This is a conditional obligation rather than a seam: if + a future deployment adds either lock, it must also open that path. Spec v4.1 §21 + asks for those policies; `cmd/felis/manifests.go` renders the game-port one. diff --git a/internal/config/config.go b/internal/config/config.go index cd8daaa..d3769ef 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -118,9 +118,12 @@ type RegistryConfig struct { // modpack's Kaniko build context is pinned. It belongs to the §16 build // subsystem's input domain (the build-context store), introduced by the // user-directed modpack approval lane (see internal/submit package doc). The - // lane derives {UserUploadsContext}/{submissionID}/context.tar.gz; the upload - // transport that places the blob there is a separate, deferred integration - // (INTEGRATION-ONLY). It is kept distinct from [archive] on purpose — a world + // lane derives {UserUploadsContext}/{submissionID}/context.tar.gz; both transports + // that place the blob there now ship (submit.LocalContextStore for a local path, + // submit.S3ContextStore for an s3:// base, selected in cmd/felis by the shape of + // this value). What stays deferred is the far end — Kaniko reading that context + // from inside the build Pod (INTEGRATION-ONLY, see submit/blobstore.go). It is + // kept distinct from [archive] on purpose — a world // archive (§19 WorldArchiver) and a build context (§16) are different artifacts // with different lifecycles, so the two must not share a store binding. UserUploadsContext string `toml:"user_uploads_context"` diff --git a/internal/updater/doc.go b/internal/updater/doc.go index 1452d3d..df4db98 100644 --- a/internal/updater/doc.go +++ b/internal/updater/doc.go @@ -41,11 +41,15 @@ // read and a CLI read agree instead of the image masquerading as a prerelease. The // CLI seam (execRunner) is wired for real; only its exec I/O is un-verified here. // -// - REMAINING INTEGRATION (genuinely I/O-bound — needs a node/cluster/mailbox): the -// two current-version PRODUCING seams the gatherer still lacks — the k8s read of the -// control-plane Deployment's image (felis-api) and the off-cluster Velocity jar -// inspection, both left nil so those components surface a gather error rather than a -// wrong version — plus the concrete Notifier (SMTP + in-game) and Applier -// (control-plane image bump, cloudflared swap), the `felis update` CLI + CronJob -// entry point, and the runtime append of the live Pinned Minecraft fleet. +// - REMAINING INTEGRATION (genuinely I/O-bound — needs a cluster/mailbox): the two +// current-version PRODUCING seams NewSysGatherer still leaves nil — the k8s read of +// the control-plane Deployment's image (felis-api) and the Velocity jar inspection — +// so an IN-CLUSTER caller surfaces a gather error rather than a wrong version. The +// ON-HOST caller has both: NewHostGatherer (gatherer_host.go) answers felis-api from +// the running binary's build stamp and Velocity from the installed jar's manifest, +// and that is what the built `felis update` CLI runs on. Still absent: the concrete +// Notifier (SMTP + in-game) and Applier (control-plane image bump, cloudflared swap) +// — the CLI passes nil for both on purpose, so it reports and never applies — the +// in-cluster CronJob entry point, and the runtime append of the live Pinned +// Minecraft fleet. package updater -- 2.54.0 From 4e5a809dad9ed5295f379ee6801a29d8bcb664df Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 17:27:02 +0900 Subject: [PATCH 07/10] docs(nano): drop the promise of a `felis setup --nano` that should not exist nano.go's package comment told the reader that `felis setup --nano` installs the multiplexer as a service. No such flag has ever existed -- `felis setup` defines only -config and -dev -- so anyone following the comment gets "flag provided but not defined: -nano" and exit 2. Adding the flag was the obvious reading, and it is the wrong one. setup does not install anything selectively: it re-images the host by running the full bootstrap TUI, and no install-mode parameter is threaded anywhere -- nothing in Go reads or writes FELIS_INSTALL_MODE, which is a shell variable bootstrap.sh consumes on its own. So `--nano` could only mean one of two things. Re-run the installer in nano mode, which is what pointing at the installer already does. Or convert a provisioned full host into a nano one, which means tearing down k3s, Postgres and the proxy -- an uninstall, not a flag. setup's --dev already settled this shape once. It looked like a channel selector, silently installed release, and the fix was to refuse it and name the installer rather than pretend to choose. The same answer applies here, so the comment now names the real entry point -- the installer's `[2] Felis-nano` prompt, or FELIS_INSTALL_MODE=nano -- and records why there is no flag, so the next reader does not reopen it. No refusing --nano flag is added: --dev exists because it used to be a silent no-op that people passed, and nothing has ever accepted --nano, so flag's own "not defined" error is already the correct and clearer failure. --- cmd/felis/nano.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd/felis/nano.go b/cmd/felis/nano.go index c01c3c5..a1c7a76 100644 --- a/cmd/felis/nano.go +++ b/cmd/felis/nano.go @@ -13,8 +13,16 @@ package main // off-loopback with -listen; see the flag below for why that is an explicit opt-in. // // This is the no-database delivery of the identical brain `felis api` mounts through its -// route table (internal/api.HasJoinedHandler). `felis setup --nano` / the bootstrap nano -// choice install this as the runtime service; here it just serves. +// route table (internal/api.HasJoinedHandler). The bootstrap installer's nano choice — its +// `[2] Felis-nano` prompt, or FELIS_INSTALL_MODE=nano — installs this as the +// felis-nano.service unit; here it just serves. +// +// There is deliberately no `felis setup --nano`. setup re-images the host through the full +// bootstrap TUI and carries no install-mode parameter anywhere (nothing in Go reads or sets +// FELIS_INSTALL_MODE), so such a flag would either re-run the installer — which is what +// pointing at the installer already does — or tear a full install down into a nano one, +// which is an uninstall, not a flag. This is the same reasoning that makes setup's --dev +// refuse and name the installer rather than pretend to choose a channel. import ( "context" -- 2.54.0 From 4f5014d033cf3d607ba5a1f814111dd750dd760b Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 17:27:02 +0900 Subject: [PATCH 08/10] feat(bootstrap): make the legacy-forwarding backend list overridable Which backends receive their forwarded identity through the handshake address -- rather than proxy-wide modern forwarding -- was the literal string "legacy18", assigned inside write_velocity_service. Standing up a second protocol-47 backend therefore meant editing this script, on every host, and remembering to. It is now FELIS_LEGACY_FORWARDING_SERVERS, defaulting to legacy18, declared beside FELIS_NANO_LISTEN and documented in the Tunables block like every other knob. The default is unchanged, so an existing install re-runs to the same systemd unit it already has. This is deliberately only half of what the list should eventually do. It is a JVM system property, read once when Velocity starts, so it is fixed for the life of the proxy process and a change still needs a restart -- an environment variable is as far as a startup property can be pushed. Having the list follow the MinecraftServer CRs is a larger change than it looks: the forwarding decision is made by the fork's patch to Velocity core, not by the Felis plugin, so core would have to read state the plugin owns and refreshes. The plugin already maintains a dynamic backend registry, which is where that state would come from, but the bridge from core to it does not exist. The comment at the assignment now says so instead of leaving "the upgrade path is to have the operator render this list from the MinecraftServer CRs" as though it were a small step. The -D is now double-quoted in ExecStart. The fork trims each element -- it parses the property as `split(",")` into a Set, mapped through String::trim with empties filtered -- so it accepts "legacy18, legacy112", but systemd splits ExecStart on whitespace before java sees it. Unquoted, that spelling handed java a stray "legacy112" argument and the unit failed to start; documenting the knob as comma-separated without quoting it would have shipped that as a footgun. `bash -n` passes; the default resolves to legacy18, an override to the value given, and a value containing a space renders inside a single quoted ExecStart item. --- deploy/bootstrap.sh | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/deploy/bootstrap.sh b/deploy/bootstrap.sh index 9bd4a50..7334325 100644 --- a/deploy/bootstrap.sh +++ b/deploy/bootstrap.sh @@ -30,6 +30,10 @@ # FELIS_INSTALL_MODE full|nano — skip the prompt (default: ask on a tty, else full) # FELIS_NANO_LISTEN listen addr for `felis nano` (default: 127.0.0.1:8081 — loopback # only; set a private-network IP to serve an off-host proxy) +# FELIS_LEGACY_FORWARDING_SERVERS comma-separated backends that receive their identity +# through the handshake address instead of modern forwarding +# (default: legacy18). Read once at Velocity start, so changing it +# means re-running this script and restarting the proxy. # FELIS_GO_VERSION Go toolchain used to build the nano binary (default: 1.26.4) # FELIS_REPO_URL git URL to build from (raw script mode only) # FELIS_VERSION_BOOTSTRAP release|dev — which version to install (default: release). @@ -95,6 +99,11 @@ INSTALL_MODE="${FELIS_INSTALL_MODE:-}" # own players stop getting in. Same-host Velocity reaches 127.0.0.1 fine; a proxy on # another machine must opt in explicitly with FELIS_NANO_LISTEN=:8081. FELIS_NANO_LISTEN="${FELIS_NANO_LISTEN:-127.0.0.1:8081}" +# Backends that take their forwarded identity through the handshake address instead of +# proxy-wide modern forwarding. See write_velocity_service for why a protocol-47 backend +# needs this. Overridable because adding a second 1.8 backend otherwise means editing this +# script; it is still a restart-time list, not one that follows the CRs. +FELIS_LEGACY_FORWARDING_SERVERS="${FELIS_LEGACY_FORWARDING_SERVERS:-legacy18}" FELIS_GO_VERSION="${FELIS_GO_VERSION:-1.26.4}" PKG_LOCK_TIMEOUT="${PKG_LOCK_TIMEOUT:-${APT_LOCK_TIMEOUT:-900}}" APT_LOCK_TIMEOUT="${APT_LOCK_TIMEOUT:-$PKG_LOCK_TIMEOUT}" @@ -1591,9 +1600,20 @@ install_velocity_service() { # behind ViaVersion, which strips modern forwarding's login-plugin-message when it down-translates # the proxy->backend pipeline to protocol 47; only the handshake field survives Via. The Felis # fork reads this list from -Dfelis.legacy-forwarding.servers and forwards those servers legacy; - # every other backend keeps modern+secret untouched. v1 hardcodes the one legacy backend; the - # upgrade path is to have the operator render this list from the MinecraftServer CRs. - local legacy_forwarding_servers="legacy18" + # every other backend keeps modern+secret untouched. + # + # The list is a JVM system property, so it is fixed for the life of the proxy process and a + # change needs a Velocity restart. FELIS_LEGACY_FORWARDING_SERVERS makes that reachable + # without editing this script, which is as far as a startup property can go. Having it follow + # the MinecraftServer CRs instead is a larger change: the forwarding decision lives in the + # fork's patch to Velocity core, not in the Felis plugin, so core would need to read state the + # plugin owns and refreshes. + # + # The -D below is double-quoted in ExecStart on purpose. The fork trims each element, so it + # accepts "legacy18, legacy112", but systemd splits ExecStart on whitespace before java ever + # sees it -- unquoted, that spelling would hand java a stray "legacy112" argument and the unit + # would not start. Quoting keeps the whole property one argv item. + local legacy_forwarding_servers="${FELIS_LEGACY_FORWARDING_SERVERS}" cat > "$VELOCITY_SERVICE" < Date: Tue, 28 Jul 2026 18:01:39 +0900 Subject: [PATCH 09/10] feat(bootstrap): refuse to install an unverified Velocity fork jar FELIS_VELOCITY_FORK_JAR replaces the proxy every player connects through, and the only thing checked about it was that the path pointed at a readable file. A truncated copy, a stale build left at the same path, or the two-patch jar where the three-patch one was meant all installed silently. It now requires FELIS_VELOCITY_FORK_JAR_SHA256 and refuses on a mismatch, hashing stdin rather than the path for the reason install_via_plugins already documents: sha256sum escapes its output line for a filename carrying a backslash or newline, and the leading "\" that adds fails every comparison. The absent-digest refusal prints the jar's actual hash, so the first run after a deliberate rebuild is one copy-paste rather than an investigation. The comparison ignores case and internal spaces. The fork is built on a developer machine, which is usually Windows, and nothing there prints a digest the way sha256sum does: Get-FileHash returns uppercase and certutil has shipped the bytes space-separated. Comparing raw would refuse two of the three spellings of the correct answer and word the refusal as tampering. No digest is hardcoded, which is the half of the request this does not deliver. The fork is built from Felis-Legacy and has never been reproduced on a second machine, so a constant here would pin one machine's output rather than the fork. The comment that previously asserted the build "is not byte-reproducible" is gone too -- it was stated more confidently than the evidence supports. The fork jars on disk carry Gradle's constant 1980-02-01 entry timestamps, so the usual reason a jar differs between builds is already absent; that is not proof it reproduces, and neither claim should sit in the script unmeasured. This is deliberately not a supply-chain signature and the comment says so: an operator who can write the jar can write the digest. What it buys is that a path stops being an identity, and that every later re-run re-checks the same build. Scope: the fork jar only. The else branch still curls stock Velocity from PaperMC with no verification at all, and that is the branch a default install takes. The digest is already in hand there -- Fill v3 returns checksums.sha256 and its download URL is content-addressed on that same value -- and papermc_latest_jar discards it. Left alone rather than widened into this change. deploy/bootstrap_test.sh covers the gate's two refusals, its happy path, and the two Windows digest spellings. Each case extracts the block under test out of bootstrap.sh with awk and runs it with die/log stubbed, rather than transcribing it -- a transcribed copy passes forever after someone edits the original. The extraction is length-bounded: awk runs an unmatched end pattern to EOF, which would quietly feed the rest of bootstrap.sh to the shell under test. bootstrap.sh itself cannot run here; it wants root, a package manager and k3s. A `shell` CI job runs that plus a syntax check over every tracked script. The syntax step dispatches on each file's shebang instead of running `sh -n` across the board. The blanket form looks fine and is a false green: on a developer machine `sh` is usually bash and accepts everything, while the runner's `sh` is dash. Verified against the real thing rather than an approximation -- inside ubuntu:24.04, where /bin/sh is /usr/bin/dash, the dispatching loop passes all six scripts and the blanket loop dies at bootstrap.sh:191 on the first of its 14 arrays. Refs: Felis-Legacy #19 --- .github/workflows/ci.yml | 23 +++++++++++++ deploy/bootstrap.sh | 42 +++++++++++++++++++++--- deploy/bootstrap_test.sh | 70 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 deploy/bootstrap_test.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5253371..2a718ff 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,6 +36,29 @@ jobs: - run: go vet ./... - run: go test ./... + shell: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + # bootstrap.sh is the only thing that ever runs on a fresh host, and nothing here can + # run it — it wants root, a package manager and k3s. Syntax plus the extracted-block + # checks in bootstrap_test.sh is the coverage that is reachable without a machine. + # Each file is parsed by the interpreter its own shebang names. A blanket `sh -n` is + # wrong and not obviously so: on a developer machine `sh` is usually bash and passes + # everything, while the runner's `sh` is dash and rejects bootstrap.sh at the first of + # its arrays. Honouring the shebang is what makes local and CI agree. + - name: Check shell syntax + run: | + for f in $(git ls-files '*.sh'); do + case "$(head -1 "$f")" in + *bash) bash -n "$f" || exit 1 ;; + *) sh -n "$f" || exit 1 ;; + esac + done + + - run: sh deploy/bootstrap_test.sh + panel: runs-on: ubuntu-latest steps: diff --git a/deploy/bootstrap.sh b/deploy/bootstrap.sh index 7334325..adc3ec1 100644 --- a/deploy/bootstrap.sh +++ b/deploy/bootstrap.sh @@ -34,6 +34,10 @@ # through the handshake address instead of modern forwarding # (default: legacy18). Read once at Velocity start, so changing it # means re-running this script and restarting the proxy. +# FELIS_VELOCITY_FORK_JAR path to a Felis-Legacy Velocity fork build to install as the +# proxy instead of the stock download (default: unset, stock). +# FELIS_VELOCITY_FORK_JAR_SHA256 expected sha256 of that jar. REQUIRED whenever the jar +# above is set; the install refuses on a mismatch. # FELIS_GO_VERSION Go toolchain used to build the nano binary (default: 1.26.4) # FELIS_REPO_URL git URL to build from (raw script mode only) # FELIS_VERSION_BOOTSTRAP release|dev — which version to install (default: release). @@ -132,9 +136,20 @@ FELIS_VELOCITY_VERSION="${FELIS_VELOCITY_VERSION:-3.5.1}" # # Opt-in because it is unmeasured where it counts: FL-008's probe runs offline-mode # against a stub, and this jar would carry every real Mojang session on the server. -# The build lives in Felis-Legacy and is not byte-reproducible, so there is no digest -# to pin here — the jar is trusted because that probe certified the build. FELIS_VELOCITY_FORK_JAR="${FELIS_VELOCITY_FORK_JAR:-}" +# Expected sha256 of that jar, REQUIRED whenever it is set. Case and internal spaces are +# ignored, so whatever sha256sum, Get-FileHash or certutil printed can be pasted as-is. +# No digest is hardcoded here: +# the build lives in Felis-Legacy and has never been reproduced on a second machine, so +# any constant this script carried would pin one machine's output rather than the fork. +# +# So this is not a supply-chain signature and does not pretend to be one — an operator +# who can write the jar can write this value too. What it does buy: a path is not an +# identity, and every re-run of this script re-checks it. A truncated copy, a stale build +# left at the same path, or the two-patch jar where the three-patch one was meant all +# change the digest and stop the install. Naming the digest once is what turns "whatever +# is at that path today" into one specific build. +FELIS_VELOCITY_FORK_JAR_SHA256="${FELIS_VELOCITY_FORK_JAR_SHA256:-}" # Temurin 25: Velocity 3.5 needs 21+, and 25 is also what a future Velocity 4 requires, # so the runtime does not have to move again when the pin does. Distro JDK packaging is # a lottery across four package managers — a tarball is one code path everywhere (same @@ -1471,12 +1486,31 @@ install_jre() { install_velocity() { install_jre - local url tmp + local url tmp have want prepare_velocity_layout if [ -n "$FELIS_VELOCITY_FORK_JAR" ]; then [ -f "$FELIS_VELOCITY_FORK_JAR" ] \ || die "FELIS_VELOCITY_FORK_JAR is not a readable file: ${FELIS_VELOCITY_FORK_JAR}" - log "installing the Felis-Legacy Velocity fork from ${FELIS_VELOCITY_FORK_JAR}" + # Hash stdin, never the path — same reason as install_via_plugins: sha256sum escapes its + # output line for a filename carrying a backslash or a newline, and the leading "\" that + # adds would fail every comparison below. + have="$(sha256sum <"$FELIS_VELOCITY_FORK_JAR" | cut -d' ' -f1)" + # Refuse rather than warn. This jar is the proxy every player connects through, and a + # warning in an install log is not a gate. The digest is printed so the first run after + # a deliberate rebuild is one copy-paste, not an investigation. + [ -n "$FELIS_VELOCITY_FORK_JAR_SHA256" ] || die \ + "FELIS_VELOCITY_FORK_JAR_SHA256 is required whenever FELIS_VELOCITY_FORK_JAR is set. + The jar at that path hashes to ${have}. + Check that against the build you meant to install, then re-run with + FELIS_VELOCITY_FORK_JAR_SHA256=${have}" + # Normalise the operator's digest before comparing. sha256sum prints lowercase, but the + # build host is often Windows, where Get-FileHash prints uppercase and certutil has + # shipped both with and without spaces between the bytes. All three name the same jar, + # so comparing raw would refuse two of the three spellings and word it as tampering. + want="$(printf '%s' "$FELIS_VELOCITY_FORK_JAR_SHA256" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + [ "$have" = "$want" ] || die \ + "FELIS_VELOCITY_FORK_JAR checksum mismatch: got ${have}, expected ${want}" + log "installing the Felis-Legacy Velocity fork from ${FELIS_VELOCITY_FORK_JAR} (sha256 ${have})" atomic_install_file "$FELIS_VELOCITY_FORK_JAR" "${VELOCITY_DIR}/velocity.jar" 0644 root root else log "resolving the newest Velocity ${FELIS_VELOCITY_VERSION} build" diff --git a/deploy/bootstrap_test.sh b/deploy/bootstrap_test.sh new file mode 100644 index 0000000..ac06448 --- /dev/null +++ b/deploy/bootstrap_test.sh @@ -0,0 +1,70 @@ +#!/bin/sh +# Checks for deploy/bootstrap.sh. Run it as: sh deploy/bootstrap_test.sh +# +# The script it tests cannot be run here — it wants root, a package manager, k3s and the +# network — so each case extracts the block it is about out of bootstrap.sh verbatim and runs +# that with die/log stubbed. Extraction rather than a transcribed copy is the point: a copy +# passes forever after someone edits the original. +set -u + +BS="${1:-$(dirname "$0")/bootstrap.sh}" +[ -f "$BS" ] || { echo "no such script: $BS"; exit 1; } +fails=0 + +expect() { # label needle haystack + case "$3" in + *"$2"*) echo "PASS $1" ;; + *) echo "FAIL $1: expected <$2> in:"; echo "$3"; fails=$((fails + 1)) ;; + esac +} + +# --- the FELIS_VELOCITY_FORK_JAR digest gate ------------------------------------------- +# This jar becomes the proxy every player connects through, so the interesting cases are the +# two refusals, not the happy path. + +gate="$(awk '/have="\$\(sha256sum <"\$FELIS_VELOCITY_FORK_JAR"/,/^ log "installing the Felis-Legacy/' "$BS")" +[ -n "$gate" ] || { echo "FAIL: no fork-jar digest gate found in $BS"; exit 1; } +# awk runs an unmatched end pattern to EOF, which would quietly pipe the rest of bootstrap.sh +# into the `sh -c` below. The emptiness check above only catches a broken start pattern. +[ "$(printf '%s\n' "$gate" | wc -l)" -lt 40 ] \ + || { echo "FAIL: the extracted block is not the gate -- did its last line move?"; exit 1; } + +jar="$(mktemp)" +trap 'rm -f "$jar"' EXIT +printf 'stand-in for a fork build\n' > "$jar" +want="$(sha256sum <"$jar" | cut -d' ' -f1)" + +run_gate() { # digest + FELIS_VELOCITY_FORK_JAR="$jar" FELIS_VELOCITY_FORK_JAR_SHA256="$1" sh -c ' + die() { printf "DIE: %s\n" "$*"; exit 1; } + log() { printf "LOG: %s\n" "$*"; } + '"$gate" +} + +out="$(run_gate '')" +expect "fork jar without a digest is refused" "DIE: FELIS_VELOCITY_FORK_JAR_SHA256 is required" "$out" +expect "the refusal names the jar's real digest" "$want" "$out" + +out="$(run_gate 'deadbeef')" +expect "a mismatched digest is refused" "checksum mismatch: got ${want}, expected deadbeef" "$out" + +out="$(run_gate "$want")" +expect "the matching digest installs" "LOG: installing the Felis-Legacy Velocity fork" "$out" +expect "the install line records the digest" "(sha256 ${want})" "$out" + +# The build host is usually Windows, where nothing prints the digest the way sha256sum does: +# Get-FileHash returns uppercase and certutil has shipped the bytes space-separated. Feeding +# the gate its own output can never catch that -- these two cases are the operator's paste. +out="$(run_gate "$(printf '%s' "$want" | tr 'a-z' 'A-Z')")" +expect "an uppercase digest is the same digest" "LOG: installing the Felis-Legacy Velocity fork" "$out" + +out="$(run_gate "$(printf '%s' "$want" | sed 's/../& /g')")" +expect "a space-separated digest is the same digest" "LOG: installing the Felis-Legacy Velocity fork" "$out" + +# --------------------------------------------------------------------------------------- +if [ "$fails" -eq 0 ]; then + echo "ALL PASS" +else + echo "$fails FAILED" +fi +exit "$fails" -- 2.54.0 From 503240db7be29afcab44bc97be5e5940b052f65b Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 28 Jul 2026 18:20:40 +0900 Subject: [PATCH 10/10] ci: stop running the whole suite twice on every pull-request push The header of this file argues that release.yml must not be repeated here, because a private repository is billed twice for one answer. The push trigger it shipped with then did exactly that: `branches: ['**']` plus `pull_request` means a branch with an open PR runs everything once for refs/heads/ and once for refs/pull/N/merge. The concurrency group is keyed on github.ref, which differs between the two, so neither cancels the other. Visible on this branch's own checks: go 3m14s and go 3m3s, shell 7s and 7s, panel 25s and 24s. Limiting the push trigger to main keeps both gates that matter -- a PR is still checked before merge, main is still checked after -- and drops only the duplicate. The one case that loses coverage is a branch pushed with no PR open, where nothing has asked for the answer yet. Verified by parsing the result with the repository's own sigs.k8s.io/yaml: triggers are {"pull_request":null,"push":{"branches":["main"]}} and the three jobs go/panel/shell are intact. Worth recording for the next person who parses a workflow: YAML 1.1 reads the bare key `on` as the boolean true, so it arrives as the string "true" after the YAML-to-JSON conversion, and a struct tag of `json:"on"` silently matches nothing. GitHub's own parser does not have this problem; a local check of the triggers does. Refs #7 --- .github/workflows/ci.yml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2a718ff..e2ddc1f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,4 +1,4 @@ -# Runs the checks on every push and pull request. +# Runs the checks on pull requests and on main. # # release.yml already runs `go vet` and `go test`, but only once a vX.Y.Z tag exists — by then # a red change is on the release path and the only remedy is a new tag. This is the same gate @@ -6,14 +6,17 @@ # the release goes through the Dockerfile, and the Dockerfile runs `npm run build`, never # `npm test`. # -# Tags are excluded from the push trigger. A vX.Y.Z push fires release.yml, which repeats the -# Go job itself; running both would spend a private repository's Actions minutes twice for one -# answer. +# The push trigger is limited to main rather than every branch, for the reason release.yml is +# not repeated here: a branch with an open PR would otherwise run the whole suite twice per +# push, once for refs/heads/ and once for refs/pull/N/merge. Those are different +# concurrency groups, so neither cancels the other, and this repository is private and billed +# for both. A branch with no PR open yet is the one case that loses coverage, and opening the +# PR is what asks for the answer. name: ci on: push: - branches: ['**'] + branches: [main] pull_request: permissions: -- 2.54.0