From 148e15a1f6dda5abca9a87aa27f9ff64a9cf7fce Mon Sep 17 00:00:00 2001 From: jklein Date: Sat, 29 Aug 2026 01:25:48 +0200 Subject: [PATCH] ci: guard resource quantities against exponential notation Adds ci/run-quantity-render-test.sh and a validate-charts job that runs it over every chart on every PR. The bug behind #157 has a loud and a silent half. Loud: a bare integer request of 1048576 is reformatted by printf "%v" to "1.048576e+06", the helper rejects the suffix it produced itself and the render aborts. Silent: with a fractional limitFactor the *product* is reformatted after the suffix check, the render succeeds, and the manifest carries memory: "1.4999985e+06" - rejected by nothing until the API server sees it at apply time. So the test renders each chart twice and, for the silent case, searches the rendered text rather than the exit code. It runs over all charts, not the changed ones: the resources helper is copied into every chart's _helpers.tpl, so the regression arrives by copy, in a PR whose diff shows a new chart and not the bug inside it. The resources paths come from values.schema.json (JSON, so python3 reads it without pyyaml), which also means a new chart is enrolled automatically and there is no list to maintain. Charts that pin every computable limit in their fallback defaults never reach the helper (gotenberg, rss-bridge). They are reported as "not exercised" instead of counted as passing. --- .github/workflows/validate-charts.yml | 23 ++ README.md | 20 +- ci/run-quantity-render-test.sh | 291 ++++++++++++++++++++++++++ 3 files changed, 333 insertions(+), 1 deletion(-) create mode 100755 ci/run-quantity-render-test.sh diff --git a/.github/workflows/validate-charts.yml b/.github/workflows/validate-charts.yml index 4a358f1..b12ee03 100644 --- a/.github/workflows/validate-charts.yml +++ b/.github/workflows/validate-charts.yml @@ -89,6 +89,29 @@ jobs: echo "Version bump OK: $BASE_VERSION -> $NEW_VERSION" fi + quantity-render: + name: "Resource quantities" + runs-on: ubuntu-latest + # The one job in this workflow that deliberately ignores the changed-chart + # matrix and runs over *every* chart (issue #157). The bug it guards + # against - resource quantities formatted with printf "%v", where float64 + # flips to exponential notation above ~1e6 - lives in the resources helper, + # and that helper is copied into every chart's templates/_helpers.tpl, + # because charts here have no dependencies. So it comes back by copy: a new + # chart started from an older one, or from template-chart before the fix. + # In that PR the chart is new and its copy of the bug is in no diff anyone + # reads. Running over everything also means there is no list to maintain - + # adding a chart enrolls it. Two renders per chart, no cluster, a few + # seconds in total; it depends on no other job and finishes long before + # build-chart, so it adds no wall-clock time to the pipeline. + steps: + - name: "Checkout Repository" + uses: actions/checkout@v7 + - name: "Install Helm" + uses: azure/setup-helm@v5 + - name: "Render every chart with bare integer quantities" + run: ci/run-quantity-render-test.sh + trivy-config: name: "Trivy config ${{ matrix.chart }}" needs: detect-changed-charts diff --git a/README.md b/README.md index 8e5c2d0..0216865 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,25 @@ Collection of some self-made helm-charts. Each top-level directory is a standalo - `publish-.yml` — one thin workflow per chart; triggers on pushes to `main` touching `/**` (or manually) and calls the reusable workflow below. **A push to a chart directory on `main` publishes that chart**, so the chart version must be bumped in the same change. - `publish-helm-chart.yml` — reusable workflow: `helm package`, then `helm push` to the private JK-Registry. Registry credentials come from 1Password Connect (`OP_CONNECT_TOKEN` secret, items under `op://GitHub-Actions/`). The loaded values are read via **`steps..outputs.*`**, mapped into each step's own `env:` block and used as quoted shell variables — never interpolated into the command line. `load-secrets-action` **v5 no longer exports them as environment variables** for later steps the way v4 did, so after the v5 bump every `${{ env.JK_CR_* }}` reference resolved to an empty string while the loading step still logged `Populating variable` and stayed green. Each value is additionally guarded with `:?`, because an empty one used to collapse into a missing argument rather than an error: `-u` consumed the following `--password-stdin`, helm fell back to an interactive prompt and the job died with `inappropriate ioctl for device` — a terminal error for what was really an unset variable. - `change-app-version.yml` — manually dispatched app-version bump; the chart must be listed in its `chart-name` choice list. -- `validate-charts.yml` — PR pipeline: detects which charts a PR touches and, for exactly those, runs `helm lint --strict`, `helm package`, and a chart-version bump check (`template-chart` is exempt from the version check). PRs without chart changes skip the build matrix. After the build stage, an **install-test** job (same changed-charts matrix) installs each changed chart into an ephemeral [k3d](https://k3d.io) cluster on the runner and requires every workload to become Ready — see below. Both matrices follow the changed chart directories, so a PR that only changes the harness under `ci/` exercises nothing by itself; the Trivy scan has a fixed-sample fallback for that case, the install-test has none — bump the affected chart's patch version to pull it into the matrix. +- `validate-charts.yml` — PR pipeline: detects which charts a PR touches and, for exactly those, runs `helm lint --strict`, `helm package`, and a chart-version bump check (`template-chart` is exempt from the version check). PRs without chart changes skip the build matrix. After the build stage, an **install-test** job (same changed-charts matrix) installs each changed chart into an ephemeral [k3d](https://k3d.io) cluster on the runner and requires every workload to become Ready — see below. Both matrices follow the changed chart directories, so a PR that only changes the harness under `ci/` exercises nothing by itself; the Trivy scan has a fixed-sample fallback for that case, the install-test has none — bump the affected chart's patch version to pull it into the matrix. The **resource-quantity render test** is the exception to that rule: it runs over every chart on every PR — see below. + +### Resource-quantity render test (all charts, every PR) + +`ci/run-quantity-render-test.sh [chart …]` (no argument = all charts) renders every chart twice with the fixtures the install-test and the Trivy scan already use, injecting a bare integer memory request at **every** `resources` block the chart's `values.schema.json` declares: + +| Case | Injected | Expected limit | Catches | +|---|---|---|---| +| `bare-int` | `requests.memory: 1048576`, `limitFactor: 2` | `2097152` | render aborts | +| `fraction` | `requests.memory: 999999`, `limitFactor: 1.5` | `1499998.5` | quantity in exponential notation | + +The two cases exist because the bug behind #157 — quantities formatted with `printf "%v"`, where the `float64` the null-safe fallback helpers produce flips to exponential notation from about `1e6` — has a loud and a silent half. Loud: the reformatted quantity carries the suffix `e+06`, the helper rejects it, `helm template` exits non-zero. Silent: the *product* of a fractional `limitFactor` and a bare number is reformatted **after** the suffix check, so the render succeeds and the manifest carries `memory: "1.4999985e+06"` — rejected by nothing until the API server sees it at apply time. A check on the exit code alone therefore proves only half the case; the second case searches the **rendered text** for a resource quantity in exponential notation. `999999` stays below `1e6` on purpose, so the fraction case isolates the second formatting site instead of tripping over the first. + +Why all charts rather than the changed ones: charts here have no dependencies, so the resources helper is copied into every chart's `templates/_helpers.tpl`. The regression arrives by copy — a new chart started from an older chart, or from `template-chart` before the fix — and in that PR the copied bug is in no diff anyone reads. Running over everything also removes the list nobody would maintain: adding a chart enrolls it automatically. Two renders per chart, no cluster, a few seconds for the whole repo; the job depends on nothing else and finishes well inside the `build-chart` runtime. + +Two details worth knowing before touching it: + +- **The resources paths come from `values.schema.json`, not `values.yaml`** — it is JSON, so `python3` reads it without pyyaml (same runner constraint as `ci/run-trivy-config.sh`), and every chart-owned key must appear there anyway. A block counts when it is `#/definitions/resources` or carries `limitFactor`/`requests` itself, which is what keeps `minecraft-server`'s JVM-sizing `minecraft.resources` out. A chart whose templates render a resources block while its schema declares none **fails** the test with exactly that message: the schema is then out of sync with the chart. +- **A chart that never reaches the helper is reported, not counted as passing.** `gotenberg` sets every computable limit explicitly in its fallback defaults, and since a `null` never deletes a default there, no override can free them — nothing is multiplied, so nothing is proven. Those charts appear under "helper not reached" in the job summary rather than as a green tick for a code path that was never entered. ### Install-test (PR pipeline stage 2) diff --git a/ci/run-quantity-render-test.sh b/ci/run-quantity-render-test.sh new file mode 100755 index 0000000..3e7038f --- /dev/null +++ b/ci/run-quantity-render-test.sh @@ -0,0 +1,291 @@ +#!/usr/bin/env bash +# Resource-quantity render test: renders every chart with values that hit both +# failure modes of issue #157 and checks the *output*, not just the exit code. +# +# Usage: ci/run-quantity-render-test.sh [chart ...] (no argument = all charts) +# +# What went wrong (#157): the resources helper formatted numbers with +# printf "%v". The null-safe fallback helpers pass values through fromYaml, +# which parses every number as float64, and Go's "%v" switches float64 to +# exponential notation from about 1e6 upwards. A bare byte count like +# 1048576 became "1.048576e+06". That produced two different failures, and only +# one of them is loud: +# +# 1. LOUD - the quantity itself is reformatted before it is parsed, the +# helper sees the suffix "e+06" and aborts the render with +# `unsupported resource quantity suffix "e+06"`. A broken render stops the +# pipeline by itself. +# 2. SILENT - the *product* of a fractional limitFactor and a bare number is +# reformatted after the suffix check. The render succeeds and the manifest +# looks fine; it carries `memory: "1.4999985e+06"`, which no suffix check +# ever sees and which only the API server rejects, at apply time. This one +# was reachable in all eleven charts checked with ordinary, schema-valid +# values, because limitFactor is typed "number", not "integer". +# +# So the loud case is covered by "does helm template exit 0", and the silent +# case is not - it needs the rendered text to be searched for a quantity in +# exponential notation. Both are checked here, per chart. +# +# Why over *all* charts instead of the changed ones: charts in this repo have +# no dependencies, so the helper is copied into every chart's +# templates/_helpers.tpl. The regression this guards against arrives by copy - +# a new chart started from an older chart, or from template-chart before the +# fix. That chart is new to the repo, but its copy of the bug is not something +# any diff of *this* PR would point at. The test is cheap enough (two renders +# per chart, no cluster) to simply run over everything, and adding a chart +# enrolls it automatically: there is no list to maintain here. +# +# Which values: the same committed fixtures the install-test and the Trivy scan +# use, ci//test-values.yaml (or ci/trivy/values/.yaml, or the +# chart defaults) - they exist, they are maintained, and they satisfy the +# required() values every chart has. On top of them this script sets, at every +# resources block the chart's values schema declares: +# +# case "bare-int": requests.memory = 1048576, limitFactor = 2 -> limit 2097152 +# case "fraction": requests.memory = 999999, limitFactor = 1.5 -> limit 1499998.5 +# +# 999999 stays below 1e6 on purpose: it keeps the quantity itself out of +# exponential range, so the "fraction" case isolates the second, silent +# formatting site instead of tripping over the first one. +# +# Where the resources blocks come from: values.schema.json, not values.yaml. +# It is JSON (python3 without pyyaml can read it, same constraint as +# ci/run-trivy-config.sh), and every chart-owned key must appear in it anyway - +# the schemas carry additionalProperties: false on every chart-owned level. A +# chart whose templates use the resources helper but whose schema declares no +# resources block fails the test with that as the message: the schema is then +# out of sync with the chart, which is a finding of its own. +# +# Charts that never reach the helper: a chart can set every computable limit +# explicitly (gotenberg and rss-bridge do - their fallback defaults pin the +# memory and ephemeral-storage limits, and in that helper a null never deletes a +# default, so no override can free them). Nothing is multiplied there, so +# nothing can be proven there either. Those charts are reported as "not +# exercised" rather than counted as passing - a green tick for a code path that +# was never entered is the kind of signal this repo removes rather than adds. +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +CI_DIR="$ROOT/ci" +RELEASE="quantity" + +# Probe values and the limits they must produce. Kept together because the +# expected products are what tells "the helper ran and was correct" apart from +# "the helper never ran" - see the reachability check below. +BARE_INT_REQUEST=1048576 +BARE_INT_FACTOR=2 +BARE_INT_LIMIT=2097152 +FRACTION_REQUEST=999999 +FRACTION_FACTOR=1.5 +FRACTION_LIMIT=1499998.5 + +# A resource quantity written in exponential notation. Matched on the resource +# keys only, so the line in the failure message is the offending one and not +# some unrelated number that happens to look similar. +EXP_RE='^[[:space:]]*(cpu|memory|ephemeral-storage|storage):[[:space:]]*"?[0-9]+(\.[0-9]+)?[eE][-+]?[0-9]+[A-Za-z]*"?[[:space:]]*$' + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +# --- Which charts ----------------------------------------------------------- +CHARTS=() +if [ "$#" -gt 0 ]; then + CHARTS=("$@") +else + for DIR in "$ROOT"/*/; do + [ -f "$DIR/Chart.yaml" ] || continue + CHARTS+=("$(basename "$DIR")") + done +fi +if [ "${#CHARTS[@]}" -eq 0 ]; then + echo "ERROR: no chart directories found under $ROOT" >&2 + exit 1 +fi + +FAILED="" +EXERCISED="" +NOT_EXERCISED="" + +for CHART in "${CHARTS[@]}"; do + if [ ! -f "$ROOT/$CHART/Chart.yaml" ]; then + echo "ERROR: '$CHART' is not a chart directory" >&2 + exit 1 + fi + SCHEMA="$ROOT/$CHART/values.schema.json" + if [ ! -f "$SCHEMA" ]; then + echo "::error title=quantity-render $CHART::$CHART has no values.schema.json - every chart in this repo ships one (README, \"Values that must not lie\"), and this test reads the resources paths from it." + FAILED="$FAILED $CHART" + continue + fi + + # --- 1. Find every resources block the schema declares --------------------- + # A block counts when it is the shared #/definitions/resources or carries + # limitFactor / requests itself. That distinction matters: minecraft-server + # has a "minecraft.resources" block that is JVM/storage configuration and has + # nothing to do with container resources. + PATHS="$(python3 - "$SCHEMA" <<'PY' +import json, sys + +schema = json.load(open(sys.argv[1])) +found = [] + +def is_resources(node): + if not isinstance(node, dict): + return False + if node.get("$ref") == "#/definitions/resources": + return True + props = node.get("properties") + return isinstance(props, dict) and ("limitFactor" in props or "requests" in props) + +def walk(node, path): + if not isinstance(node, dict): + return + props = node.get("properties") + if not isinstance(props, dict): + return + for key, value in props.items(): + here = path + [key] + if key == "resources" and is_resources(value): + found.append(".".join(here)) + continue + walk(value, here) + +walk(schema, []) +print("\n".join(found)) +PY +)" + + if [ -z "$PATHS" ]; then + # No resources block in the schema. Fine for a chart that has none - but a + # chart whose templates call the helper and whose schema hides the block + # would slip through this test unnoticed, so that combination is an error. + if grep -REq 'include[[:space:]]+"[^"]*\.(effective)?[Rr]esources"' "$ROOT/$CHART/templates"; then + echo "::error title=quantity-render $CHART::$CHART renders a resources block in its templates, but values.schema.json declares none - the schema is out of sync with the chart, and this test cannot reach the resource helper." + FAILED="$FAILED $CHART" + else + echo "$CHART: no resources block in the values schema - nothing to render." + NOT_EXERCISED="$NOT_EXERCISED $CHART" + fi + continue + fi + + # --- 2. Values, same precedence as ci/run-trivy-config.sh ------------------ + VALUES_ARGS=() + VALUES_LABEL="chart defaults" + REPRO_VALUES="" + if [ -f "$CI_DIR/$CHART/test-values.yaml" ]; then + VALUES_ARGS=(-f "$CI_DIR/$CHART/test-values.yaml") + VALUES_LABEL="ci/$CHART/test-values.yaml" + REPRO_VALUES="-f ci/$CHART/test-values.yaml" + elif [ -f "$CI_DIR/trivy/values/$CHART.yaml" ]; then + VALUES_ARGS=(-f "$CI_DIR/trivy/values/$CHART.yaml") + VALUES_LABEL="ci/trivy/values/$CHART.yaml" + REPRO_VALUES="-f ci/trivy/values/$CHART.yaml" + fi + + SET_BARE_INT=() + SET_FRACTION=() + for P in $PATHS; do + SET_BARE_INT+=(--set-json "$P.limitFactor=$BARE_INT_FACTOR" --set-json "$P.requests.memory=$BARE_INT_REQUEST") + SET_FRACTION+=(--set-json "$P.limitFactor=$FRACTION_FACTOR" --set-json "$P.requests.memory=$FRACTION_REQUEST") + done + + echo "::group::$CHART ($VALUES_LABEL; $(echo "$PATHS" | wc -l | tr -d ' ') resources block(s))" + echo "$PATHS" | sed 's/^/ /' + + CHART_FAILED="" + CHART_HITS=0 + + # --- 3. Both cases --------------------------------------------------------- + # Case order matters only for readability; both run either way, so a chart + # broken in both places reports both. + for CASE in bare-int fraction; do + OUT="$WORK/$CHART.$CASE.yaml" + ERR="$WORK/$CHART.$CASE.err" + if [ "$CASE" = "bare-int" ]; then + SET_ARGS=("${SET_BARE_INT[@]}") + DESC="bare integer request ($BARE_INT_REQUEST) x limitFactor $BARE_INT_FACTOR" + MARKER="$BARE_INT_LIMIT" + else + SET_ARGS=("${SET_FRACTION[@]}") + DESC="bare integer request ($FRACTION_REQUEST) x fractional limitFactor $FRACTION_FACTOR" + MARKER="$FRACTION_LIMIT" + fi + + # The reproduce line is logged rather than folded into the annotation: with + # ten resources blocks (freesailarr) it is several hundred characters, and + # an annotation that long is unreadable in the GitHub UI. + REPRO="helm template t $CHART $REPRO_VALUES ${SET_ARGS[*]}" + + if ! helm template "$RELEASE" "$ROOT/$CHART" \ + ${VALUES_ARGS[@]+"${VALUES_ARGS[@]}"} "${SET_ARGS[@]}" >"$OUT" 2>"$ERR"; then + echo "--- $CASE: helm stderr ---" + sed 's/^/ /' "$ERR" + echo "--- reproduce (from the repo root) ---" + echo " $REPRO" + echo "::error title=quantity-render $CHART ($CASE)::$CHART aborts rendering on a $DESC - the loud half of issue #157: the quantity is formatted with printf \"%v\", float64 flips to exponential notation above ~1e6, and the helper rejects the suffix it produced itself. Fix: run numbers through the chart's formatNumber helper (reference: template-chart/templates/_helpers.tpl). Reproduce command in the job log." + CHART_FAILED=1 + continue + fi + + if grep -nE "$EXP_RE" "$OUT" >"$WORK/hits" 2>/dev/null; then + echo "--- $CASE: quantities in exponential notation ---" + sed 's/^/ /' "$WORK/hits" + echo "--- reproduce (from the repo root) ---" + echo " $REPRO" + echo "::error title=quantity-render $CHART ($CASE)::$CHART renders resource quantities in exponential notation on a $DESC - $(wc -l <"$WORK/hits" | tr -d ' ') occurrence(s), first: '$(head -1 "$WORK/hits" | sed 's/^[0-9]*:[[:space:]]*//')'. This is the silent half of issue #157: the render succeeds, the manifest looks valid, and only the API server rejects it at apply time. Fix: run numbers through the chart's formatNumber helper (reference: template-chart/templates/_helpers.tpl). Reproduce command in the job log." + CHART_FAILED=1 + continue + fi + + if grep -qE "^[[:space:]]*memory:[[:space:]]*\"?$MARKER\"?[[:space:]]*$" "$OUT"; then + CHART_HITS=$((CHART_HITS + 1)) + echo " $CASE: computed limit memory: $MARKER - helper exercised, no exponential notation." + else + echo " $CASE: renders clean, but no computed limit appeared (expected memory: $MARKER)." + fi + done + echo "::endgroup::" + + if [ -n "$CHART_FAILED" ]; then + FAILED="$FAILED $CHART" + echo "$CHART: FAILED" + elif [ "$CHART_HITS" -eq 2 ]; then + EXERCISED="$EXERCISED $CHART" + echo "$CHART: ok (both cases exercised the resource helper)" + else + # Renders are clean but the helper was not reached - see the header comment + # on gotenberg. Reported, never counted as proof. + NOT_EXERCISED="$NOT_EXERCISED $CHART" + echo "$CHART: renders clean, but $((2 - CHART_HITS)) of 2 cases never reached the resource helper (all computable limits set explicitly) - nothing proven for this chart." + fi +done + +# --- 4. Report --------------------------------------------------------------- +count() { set -- $1; echo "$#"; } +listing() { set -- $1; [ "$#" -eq 0 ] && { echo "–"; return; }; printf '`%s`' "$1"; shift; for C in "$@"; do printf ', `%s`' "$C"; done; echo; } +SUMMARY="${GITHUB_STEP_SUMMARY:-/dev/null}" +{ + echo "### Resource-quantity render test (issue #157)" + echo + echo "Every chart rendered twice - once with a bare integer memory request, once with a" + echo "bare integer request and a fractional \`limitFactor\` - and the output searched for" + echo "resource quantities in exponential notation." + echo + echo "| Result | Count | Charts |" + echo "|---|---|---|" + echo "| Helper exercised, quantities plain | $(count "$EXERCISED") | $(listing "$EXERCISED") |" + echo "| Renders clean, helper not reached | $(count "$NOT_EXERCISED") | $(listing "$NOT_EXERCISED") |" + echo "| Failed | $(count "$FAILED") | $(listing "$FAILED") |" +} >>"$SUMMARY" + +echo "===================== quantity render test =====================" +echo "exercised: $(count "$EXERCISED")${EXERCISED:+ -$EXERCISED}" +echo "not exercised: $(count "$NOT_EXERCISED")${NOT_EXERCISED:+ -$NOT_EXERCISED}" +echo "failed: $(count "$FAILED")${FAILED:+ -$FAILED}" +echo "================================================================" + +if [ -n "$FAILED" ]; then + echo "ERROR: resource quantities render wrong in:$FAILED" >&2 + exit 1 +fi