From 72fdda3bbf767996d7c93ac7aa15d97594b188e3 Mon Sep 17 00:00:00 2001 From: Justin Klein Date: Tue, 22 Sep 2026 20:08:54 +0000 Subject: [PATCH] satisfactory: server.storage stays the documented exception (#39) Walks back the part of the previous commit that described storage as null-safe. It is not: an erased size still renders a PVC without one and fails at apply time, which is the decision values.schema.json has always documented. Overturning it belonged in its own issue, #39, not in the null-safety bugfix - so the chart page now marks it as an exception and names where the trade is argued, including the part that is easy to miss: the difference only matters on a new install, because an existing release cannot shrink its PVC either. The erased-key table stays and gains a "now" column, so the one key that did not change is visible rather than implied. Style guide 2 and the lessons entry lose the claim that the exception did not hold and gain the rule that replaced it: an exception is allowed, an unmarked one is not, and a bugfix that settles an open design question has stopped being a bugfix. --- Chart-Style-Guide.md | 4 +++- Lessons-Learned.md | 6 ++++-- satisfactory.md | 24 +++++++++++++++--------- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/Chart-Style-Guide.md b/Chart-Style-Guide.md index ac59ec4..439ad30 100644 --- a/Chart-Style-Guide.md +++ b/Chart-Style-Guide.md @@ -140,7 +140,9 @@ The `defaultedDict` helper defines the opposite semantics — an empty block mea Do **not** wrap these blocks in `{{- if }}`: a guard does not render a visible `null`, it removes the probe entirely — the same bug with no evidence left. -**The rule is about the shape, not about those three block kinds.** It applies to every chart-owned block a template dereferences, plain app-config blocks included. `satisfactory` shipped 7.0.0 with `game` null-safe and its neighbours `dev` and `server` not, and half-safe is worse than unsafe: whoever sees `game=null` fall back assumes `server=null` does too. The single erased key is the quieter half — a nulled `server.gamePort` rendered `port:` and `nodePort:` empty into the Service, a nulled `server.storage` a PVC asking for no size, and nothing rejects either until the API server does. A block deliberately left un-defaulted must say so in the schema **and** survive the check that erasing its *parent* still renders something usable; `satisfactory.storage` carried such a note and it did not hold. +**The rule is about the shape, not about those three block kinds.** It applies to every chart-owned block a template dereferences, plain app-config blocks included. `satisfactory` shipped 7.0.0 with `game` null-safe and its neighbours `dev` and `server` not, and half-safe is worse than unsafe: whoever sees `game=null` fall back assumes `server=null` does too. The single erased key is the quieter half — a nulled `server.gamePort` rendered `port:` and `nodePort:` empty into the Service and an empty `containerPort` into the StatefulSet, and nothing rejects that until the API server does. + +**An exception is allowed; an unmarked one is not.** A key may be left un-defaulted on purpose — `satisfactory.storage` is, so that an erased size fails at apply time instead of being guessed by the chart — but then the schema, `values.yaml` and the chart's wiki page all name it as an exception and point at the issue where the trade is argued. That is what separates intent from oversight for the next reader. And do **not** settle such an exception inside an unrelated bugfix: overturning a documented decision there is a behaviour change smuggled into a patch, and it costs the patch the property that makes it reviewable — that its output is byte-identical for every valid input. **And `helm template` does not render `NOTES.txt`.** A value read there stays broken after a fix that looks green in every render, until a real `helm install` — and for a chart excluded from the install-test, no stage renders the notes at all. `helm install --dry-run=client` is no way around it: it still wants a reachable API server (measured on helm v3.22.0). Cluster-free method: in a throwaway copy of the chart, wrap the verbatim `NOTES.txt` body in a `{{- define }}` and `include` it into a scratch ConfigMap, which `helm template` *does* render — same engine, same root context, only the sink differs. diff --git a/Lessons-Learned.md b/Lessons-Learned.md index 5289b6e..bb00041 100644 --- a/Lessons-Learned.md +++ b/Lessons-Learned.md @@ -44,11 +44,13 @@ Wrapping the block in `{{- if }}` is the same bug with the evidence removed — ## The half-protected chart, and the template no pipeline renders -The fix above was applied per block, and `satisfactory` reached 7.0.0 with `game` null-safe and its two neighbours not. That is worse than none of them being safe: an operator who watches `--set game=null` fall back cleanly concludes the chart handles erased blocks, and `--set server=null` then aborts the render. The quieter half was the single erased key — a nulled `server.gamePort` rendered a Service with `port:` and `nodePort:` empty, a nulled `server.storage` a PVC asking for no size. Nothing rejects either until the API server does, at apply time. +The fix above was applied per block, and `satisfactory` reached 7.0.0 with `game` null-safe and its two neighbours not. That is worse than none of them being safe: an operator who watches `--set game=null` fall back cleanly concludes the chart handles erased blocks, and `--set server=null` then aborts the render. The quieter half was the single erased key — a nulled `server.gamePort` rendered a Service with `port:` and `nodePort:` empty and a StatefulSet with an empty `containerPort`. Nothing rejects that until the API server does, at apply time. The second finding is the one that generalises. `server` was read in **four** files, and the fourth was `templates/NOTES.txt` — which **`helm template` does not render**. A fix confined to the manifests would have passed every local render, passed lint, passed the pipeline, and still aborted on the first real `helm install`. For a chart excluded from the install-test, as this one is, no stage renders the notes at all: it is template code the whole pipeline is blind to. `helm install --dry-run=client` is not the workaround — it still wants a reachable API server. -**Now:** every chart-owned block a template dereferences goes through the fallback helper, not just the three kinds named above; a block deliberately left un-defaulted says so in the schema *and* has to survive the check that erasing its **parent** still renders something usable. And when a values change touches a chart's notes, the notes are rendered explicitly, without a cluster: wrap the verbatim `NOTES.txt` body in a `{{- define }}` in a throwaway copy of the chart and `include` it into a scratch ConfigMap — same engine, same root context, and `helm template` renders that. +**Now:** every chart-owned block a template dereferences goes through the fallback helper, not just the three kinds named above. A key deliberately left un-defaulted is *marked* as the exception it is — in the schema, in `values.yaml` and on the chart page — with a pointer to the issue where the trade is argued; `satisfactory.storage` is exactly that, and it was left standing rather than overturned in passing while the bug next to it was fixed. And when a values change touches a chart's notes, the notes are rendered explicitly, without a cluster: wrap the verbatim `NOTES.txt` body in a `{{- define }}` in a throwaway copy of the chart and `include` it into a scratch ConfigMap — same engine, same root context, and `helm template` renders that. + +The third lesson is about the fix, not the bug: **a bugfix that also settles an open design question is no longer a bugfix.** The tempting move here was to default `storage` along with everything else — it is one line, it removes a visible inconsistency, and the argument for it is good. It also would have been the only change in the release that altered rendered output, against a documented decision someone else had reasoned out. It went to its own issue instead, and the patch kept the property that made it cheap to review: byte-identical output for every valid input. ## The strict schema that had to meet reality first diff --git a/satisfactory.md b/satisfactory.md index 1ae2dc3..470218f 100644 --- a/satisfactory.md +++ b/satisfactory.md @@ -84,20 +84,26 @@ The whole block may be erased (`null`) — the chart then falls back to exactly | `server.replicas` | int | `1` | Replica count; `0` is allowed. More than one does not work — see [One replica](#one-replica-one-volume-one-port). | | `server.gamePort` | int | `30777` | Game traffic (UDP) **and** the server's HTTPS API (TCP), as `containerPort` and as a pinned `nodePort`. Also passed to the image as `SERVERGAMEPORT`, so the port a player types is the port the server believes it runs on. | | `server.messagingPort` | int | `30888` | Messaging port (TCP), `SERVERMESSAGINGPORT`, likewise pinned as a `nodePort`. | -| `server.storage` | string | `20Gi` | Size of the data PVC — game files *and* saves. Since **7.0.1** an erased value falls back to this default; up to 7.0.0 it was deliberately left un-defaulted and rendered a PVC asking for no size at all. | +| `server.storage` | string | `20Gi` | Size of the data PVC — game files *and* saves. **The one key of this block that is not null-safe, on purpose:** an erased value yields an invalid PVC rather than a silently degraded workload. See [the storage exception](#the-storage-exception-is-deliberate). | | `server.resources.limitFactor` | number | `2` | Memory and ephemeral-storage limits are computed as factor × request. **`2`, not the repo-wide `3`, on purpose** — that is what holds the memory limit at the 8Gi this chart has always published ([style guide §12.4](Chart-Style-Guide#12-values-that-must-not-lie)). | | `server.resources.requests` | object | `memory: 4Gi`, `cpu: 1`, `ephemeral-storage: 5Gi` | | | `server.resources.limits` | object | unset | An explicit limit always wins. A CPU limit is only rendered when set here — CPU is compressible, so an unlimited container is throttled rather than killed. | -The block, and every single key in it, may be erased (`null`) and falls back to exactly these defaults. That is new in **7.0.1** and it closed two different failures. The loud one: `--set server=null` aborted the render at `.Values.server.replicas`. The quiet one, which is the reason to care — a *single* nulled key rendered without complaint into a manifest nothing rejects until the API server sees it: +The block may be erased (`null`) and falls back to these defaults — **except `storage`**, see below. That is new in **7.0.1** and it closed two different failures. The loud one: `--set server=null` aborted the render at `.Values.server.replicas`. The quiet one, which is the reason to care — a *single* nulled key rendered without complaint into a manifest nothing rejects until the API server sees it: -| Erased key | What 7.0.0 rendered | -|---|---| -| `server.gamePort` / `server.messagingPort` | Service with `port:` and `nodePort:` empty, StatefulSet with an empty `containerPort` | -| `server.storage` | PVC requesting no size | -| `server.replicas` | fell back to `1` already (the replicas helper was null-safe) | +| Erased key | What 7.0.0 rendered | Now | +|---|---|---| +| `server.gamePort` / `server.messagingPort` | Service with `port:` and `nodePort:` empty, StatefulSet with an empty `containerPort` | falls back to `30777` / `30888` | +| `server.storage` | PVC requesting no size | **unchanged — still a PVC without a size, deliberately** | +| `server.replicas` | fell back to `1` already (the replicas helper was null-safe) | unchanged | -`server.resources` is the one key with no default of its own here — it is passed through to the resources fallback, which carries `limitFactor: 2` and the requests. Keeping that in one place is deliberate: a second copy in the `server` fallback would be a third place for the numbers to drift apart. +`server.resources` is the one key with no default of its own in the fallback — it is passed through to the resources fallback, which carries `limitFactor: 2` and the requests. Keeping that in one place is deliberate: a second copy in the `server` fallback would be a third place for the numbers to drift apart. + +#### The storage exception is deliberate + +`server.storage` is the single key of this block that does **not** fall back. An erased size — including the one you get from `server: null` — renders a PVC requesting no storage, and the install fails when it is applied. That is the documented intent: *an erased value yields an invalid PVC rather than a silently degraded workload.* It was left standing when everything around it was made null-safe in 7.0.1, rather than quietly overturned inside a bugfix. + +Whether it is still the right trade is argued in **issue #39**, not here. The argument against it: every other block in this chart now falls back, so the one that does not is exactly the kind of inconsistency 7.0.1 set out to remove. The argument for it: a volume size guessed by the chart is a worse thing to discover late than a failed apply. Worth knowing either way — **the difference only matters on a new install.** For an existing release a fallback to `20Gi` would fail too, because a PVC cannot shrink. ### `env` @@ -120,7 +126,7 @@ The block, and every single key in it, may be erased (`null`) and falls back to | `securityContext.pod` | object | non-root `1000:1000`, `fsGroup: 1000`, `fsGroupChangePolicy: OnRootMismatch`, `seccompProfile: RuntimeDefault` | Passed into the pod spec. The uid is pinned because the image supports rootless operation **only** as its own `steam` user; its entrypoint rejects any other non-root uid/gid. `OnRootMismatch` is deliberate ([style guide §12.4](Chart-Style-Guide#12-values-that-must-not-lie)) — see [The uid is pinned](#the-uid-is-pinned-and-the-chown-is-skipped). | | `securityContext.container` | object | `readOnlyRootFilesystem: true`, all caps dropped, no privilege escalation, non-root | Works because the chart mounts emptyDirs at `/tmp` and over the steam user's `~/.steam`, `~/.config` and `~/.local`; everything else is written to the PVC at `/config`. | -Probe, `securityContext`, `game` and `resources` blocks may be erased (`null`) — the chart then falls back to exactly these defaults instead of rendering an unhardened, unprobed or unaccounted container. Since **7.0.1** that holds for **every** chart-owned block, `dev` and `server` included, so no block in this chart is half-safe any more. +Probe, `securityContext`, `game` and `resources` blocks may be erased (`null`) — the chart then falls back to exactly these defaults instead of rendering an unhardened, unprobed or unaccounted container. Since **7.0.1** that holds for **every** chart-owned block, `dev` and `server` included, so no erased block aborts the render any more. The one key that stays un-defaulted is [`server.storage`](#the-storage-exception-is-deliberate), and it is marked as an exception rather than left to look like an oversight. ## Operating notes