diff --git a/.github/workflows/validate-charts.yml b/.github/workflows/validate-charts.yml index 32d8f8b..4979c78 100644 --- a/.github/workflows/validate-charts.yml +++ b/.github/workflows/validate-charts.yml @@ -263,6 +263,46 @@ jobs: - name: "Check every bump level of the version-bump hook" run: ci/run-bump-level-test.sh + yaml-scalar: + name: "YAML scalars" + runs-on: ubuntu-latest + # The fifth job that ignores the changed-chart matrix, and the fourth that + # runs over *every* chart (Gitea #11), for the same reason as the three + # render tests above: the unquoted + # form arrives by copy into a chart whose own diff nobody reads for it, and + # it is in seven charts right now. + # + # What it asserts: every CronJob schedule, every env[].value and every label + # and annotation value a chart renders is a STRING, which is what Kubernetes + # types all three as. A value rendered without | quote fails in one of two + # ways and the quiet one is the dangerous one: the render aborts with a YAML + # error naming the TEMPLATE while the operator's value appears nowhere in + # it, or - when the content is a legal plain scalar that simply is not a + # string - the render succeeds and the API server rejects the object at + # apply time with "cannot unmarshal bool into Go struct field + # EnvVar.value of type string". + # + # WHY THIS JOB EXISTS AT ALL, rather than the install-test covering it: the + # object that carried the defect is rendered by no fixture. paperless-ngx' + # consume CronJob sits behind smbConnection.enabled, false in the defaults + # and in every committed fixture, so nothing in this repository ever + # produced it - which is how an unquoted schedule shipped for months. The + # overlays under ci/yaml-scalar/ switch such objects on and feed them + # hostile values, and the script fails when an overlay stops reaching its + # object. + # + # No cluster and no Trivy; the whole repository in well under a minute, + # depends on no other job. The YAML is parsed by Helm through ci/yaml2json, + # because the runners have neither pyyaml nor yq and a hand-written parser + # could read nothing and report nothing - see the script's header. + steps: + - name: "Checkout Repository" + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + - name: "Install Helm" + uses: azure/setup-helm@9bc31f4ebc9c6b171d7bfbaa5d006ae7abdb4310 # v5 + - name: "Check every rendered scalar that must be a string" + run: ci/run-yaml-scalar-render-test.sh + trivy-config: name: "Trivy config ${{ matrix.chart }}" needs: detect-changed-charts diff --git a/README.md b/README.md index 79cfaf2..b1f1102 100644 --- a/README.md +++ b/README.md @@ -68,8 +68,8 @@ Every chart has a wiki page of its own — `https://git.jk-dev.de/jlab-cluster/h Note: the registry **accepts a re-upload of an already published version** (HTTP 201) and replaces the stored artifact, so a republish of an existing version is still silent — see #160. That is also what makes a manual `workflow_dispatch` of any `publish-.yml` a harmless way to prove the shared block works. - `change-app-version.yml` — manually dispatched app-version bump; the chart must be listed in its `chart-name` choice list. -- `ci/helm-dependencies.sh ` — not a workflow but the step they all share: fetches a chart's declared dependencies into `charts/`, and returns silently for the charts that declare none (all but three: `ytdlp-web-player`, `stalwart`, `paperless-ngx`). It runs in **seven** places, because every helm command this repo builds on exits 1 while a declared dependency is missing: before lint and package in `validate-charts.yml`, before package in the shared publish block (which is why that block names this script as a prerequisite of its callers — see above), and inside `ci/run-install-test.sh`, `ci/run-quantity-render-test.sh`, `ci/run-selector-match-test.sh`, `ci/run-httproute-match-test.sh` and `ci/run-trivy-config.sh`. See "Chart dependencies" in the style guidelines for what that costs. -- `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 (the same changed-charts matrix, minus the charts listed in `ci/excluded-charts.txt`) 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, or run the all-charts install-test after the merge. The **resource-quantity render test**, the **selector-match render test** and the **HTTPRoute render test** are the three exceptions to that rule: all of them run over every chart on every PR — see below. A fourth job, the **bump-level test**, ignores the matrix for a different reason: it tests no chart at all, but the `postUpgradeTasks` hook that computes their versions. +- `ci/helm-dependencies.sh ` — not a workflow but the step they all share: fetches a chart's declared dependencies into `charts/`, and returns silently for the charts that declare none (all but three: `ytdlp-web-player`, `stalwart`, `paperless-ngx`). It runs in **eight** places, because every helm command this repo builds on exits 1 while a declared dependency is missing: before lint and package in `validate-charts.yml`, before package in the shared publish block (which is why that block names this script as a prerequisite of its callers — see above), and inside `ci/run-install-test.sh`, `ci/run-quantity-render-test.sh`, `ci/run-selector-match-test.sh`, `ci/run-httproute-match-test.sh`, `ci/run-yaml-scalar-render-test.sh` and `ci/run-trivy-config.sh`. See "Chart dependencies" in the style guidelines for what that costs. +- `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 (the same changed-charts matrix, minus the charts listed in `ci/excluded-charts.txt`) 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, or run the all-charts install-test after the merge. The **resource-quantity render test**, the **selector-match render test**, the **HTTPRoute render test** and the **YAML-scalar render test** are the four exceptions to that rule: all of them run over every chart on every PR — see below. A fourth job, the **bump-level test**, ignores the matrix for a different reason: it tests no chart at all, but the `postUpgradeTasks` hook that computes their versions. - `install-test.yml` — reusable workflow (`workflow_call`) holding the install-test **job** itself: the runner-label expression, the pinned `kubectl` with its skew check, `envsubst`, the containerised-job detection, the per-leg k3d cluster and the `freesailarr` VPN credentials. Its one input is a JSON array of chart names; its callers decide *which* charts are tested and whether the job runs at all, never *how*. Two callers: `validate-charts.yml` and `install-test-all-charts.yml`. **It is not copied for the second caller** — a second copy is a second chance to test differently, unnoticed, the same argument that gives `ci/excluded-charts.txt` exactly one parser. **It stays in this repository, and that is a decision, not an oversight** (Gitea #66, decision 4). It is reusable in form but not in substance: it is welded to the `ci/` fixtures, to the `ci/excluded-charts.txt` parser and to this repository's chart matrix. A building block that only one repository can use is not shared, it is merely moved somewhere else — and moving it would buy nothing while putting a tag between us and a file we change often. `publish-helm-chart.yml` was the opposite case: it takes a chart name and a registry owner and knows nothing about this repository, which is exactly why it could go. This note exists so the question is not asked again in half a year. @@ -156,10 +156,11 @@ Neither is a render error, neither fails the install-test, and neither shows up Three things to know before touching it: - **Helm is the YAML parser, through the one-template chart `ci/yaml2json`.** The runners have neither `pyyaml` nor `yq` (measured; the same constraint `ci/run-trivy-config.sh` and `ci/run-quantity-render-test.sh` work under), and a YAML reader written in `awk` was the obvious alternative and the wrong one: its failure mode is a parser that quietly reads nothing, finds no selector, reports no mismatch and leaves the test green for a chart it never looked at. So the rendered manifest stream is handed to that chart as a **values file** and comes back as JSON, which `python3` reads from the standard library. The one textual step is the document split on a line that is exactly `---` in column one, and getting *that* wrong does not produce a false pass — the fragment stops being valid YAML and `helm` exits 1. -- **What is deliberately not checked, named so the gap is not mistaken for coverage.** `podAffinity`/`podAntiAffinity` are out of scope: their semantics are relative to pods that exist, so a term matching nothing is a legitimate configuration rather than a mistake. And a selector expressed **only** through `matchExpressions` is **reported as "not checked", never counted as passing** — evaluating set-based requirements means reimplementing label-selector semantics, which is a second parser by another name. -- **Two anti-vacuum guards, because this test can be green over nothing.** Both objects are rendered only above a replica threshold, and most charts' CI fixtures sit below it. `ci/selector-match/.yaml` is a render-only overlay applied **last**, whose only job is to push a chart over that threshold — and a chart that *has* one and still renders no selector **fails**, since that overlay existing is the whole claim that something was checked. On top of that the run fails if **no selector at all** was checked anywhere. Today `stalwart` is exercised through its overlay (two replicas in both node groups plus an `externalServices.valkey.host`, without which the chart's own "cluster with no in-memory store" abort fires) and `keycloak` through its ordinary install-test fixture, which already runs two replicas. +- **What is deliberately not checked, named so the gap is not mistaken for coverage.** *Preferred* `podAffinity` and `podAntiAffinity` of either strength are out of scope: their semantics are relative to pods that exist, so a term matching nothing is a legitimate configuration rather than a mistake — a preference that cannot be met is dropped, and an anti-affinity over zero pods is satisfied. And a selector expressed **only** through `matchExpressions` is **reported as "not checked", never counted as passing** — evaluating set-based requirements means reimplementing label-selector semantics, which is a second parser by another name. +- **`requiredDuringSchedulingIgnoredDuringExecution` `podAffinity` *is* checked** (Gitea #101), and it is the one affinity case that is not a preference: a required term whose selector matches no pod in the release can never be satisfied, so the pod stays `Pending` for good. It is held against **all** pod templates of the render rather than its own, because selecting a *different* workload's pod is the entire purpose of such a term. First and so far only user: `paperless-ngx`' consume CronJob, pinned onto the node of its StatefulSet pod so Longhorn will mount the shared RWX consume volume — where the silent failure would be a mover that stops moving, one `Pending` pod per schedule, with both templates still rendering perfectly. +- **Two anti-vacuum guards, because this test can be green over nothing.** Both objects are rendered only above a replica threshold, and most charts' CI fixtures sit below it. `ci/selector-match/.yaml` is a render-only overlay applied **last**, whose only job is to push a chart over that threshold — and a chart that *has* one and still renders no selector **fails**, since that overlay existing is the whole claim that something was checked. On top of that the run fails if **no selector at all** was checked anywhere. Today `stalwart` is exercised through its overlay (two replicas in both node groups plus an `externalServices.valkey.host`, without which the chart's own "cluster with no in-memory store" abort fires), `keycloak` through its ordinary install-test fixture, which already runs two replicas, and `paperless-ngx` through `ci/selector-match/paperless-ngx.yaml`, whose only job is to switch `smbConnection` on so the consume CronJob — and with it the required `podAffinity` — renders at all. -Measured: 30 charts, **5 selectors verified in ~14 s**, no cluster. The fail path was exercised rather than assumed — renaming `node-group` to `nodegroup` in a hand-written copy of the PDB selector produced two `FAIL` lines naming both the selector and the labels actually present, and a non-zero exit; the same was done for the spread constraint's selector. +Measured: 30 charts, **6 selectors verified**, no cluster. The fail path was exercised rather than assumed — renaming `node-group` to `nodegroup` in a hand-written copy of the PDB selector produced two `FAIL` lines naming both the selector and the labels actually present, and a non-zero exit; the same was done for the spread constraint's selector. For the `podAffinity` check the same was done by renaming the StatefulSet's `app` label in a throwaway copy of `paperless-ngx` while leaving the CronJob's term alone: one `FAIL` naming the selector, the three pod templates it missed and the labels actually present, and a non-zero exit. ### HTTPRoute render test (all charts, every PR) @@ -199,6 +200,26 @@ What the cases cover: each update type, the `freesailarr` shape (an app minor wh **What it cannot assert, and does not pretend to:** that Renovate passes the update type at all. The hook runs only when the command is allow-listed on the shared instance, which is outside this repository — which is why a *missing* second argument is itself a case here, and why the change that introduced it carries a dry run instead of a claim (see "The version-bump hook"). +### YAML-scalar render test (all charts, every PR) + +`ci/run-yaml-scalar-render-test.sh [chart …]` (no argument = all charts) renders every chart with the same fixtures the install-test uses and checks that **every scalar Kubernetes types as a string really arrived as one**. Introduced with Gitea #11, where a value rendered without `| quote` had been shipping in a chart for months; #112 turned out to be the same defect in the same chart's `env` block. Three assertions, and all three are properties the API itself requires — there is no chart for which the opposite is a legitimate configuration, which is why this test needs no per-chart expectations: + +| Assertion | What a bare render does | Who notices today | +|---|---|---| +| `CronJob.spec.schedule` is a non-empty string | a schedule beginning with `*` (the commonest cron notation there is) or with `@` **aborts the render** — YAML reads the leading character as an alias or a reserved indicator | the operator, eventually: the error names the **template** and their own value appears nowhere in it, and **quoting it in the values file does not help**, because Helm passes the string's content through | +| every `env[].value` of every container and initContainer is a string | `"true"` and `"0755"` are legal plain scalars that are **not strings**: the chart renders perfectly and the API server rejects the object with `cannot unmarshal bool into Go struct field EnvVar.value of type string` | nobody until apply time — which in this repo means the install-test or a cluster, never the render | +| every `metadata.labels` and `metadata.annotations` value is a string | both are typed `map[string]string`, so a unit-less `100` in an upload-size annotation is rejected whole | nobody in the render | + +**Why this job exists next to the render tests that already run:** the object that carried the original defect is rendered by **no fixture at all**. `paperless-ngx`' consume CronJob sits behind `smbConnection.enabled`, which is `false` in the chart defaults and `false` in its install-test fixture — deliberately, since the CI cluster has no SMB CSI driver. So nothing in this repository ever produced that object: not lint, not the other render tests, not the install-test. A test that renders only what the fixtures already render would have been just as blind, which is the whole argument for the overlays below. + +Three things to know before touching it: + +- **Helm is the YAML parser**, through `ci/yaml2json`, for the same reason as the two tests above — the runners have neither `pyyaml` nor `yq`, and a parser written in `awk` fails by reading nothing and reporting nothing. +- **Two anti-vacuum guards.** `ci/yaml-scalar/.yaml` is a render-only overlay applied **last**, whose job is to switch such objects on *and to feed them the hostile values* — a schedule beginning with a star, an `env` value that is a bare `true`. A chart that *has* one and still renders none of the three kinds **fails**, and the run fails if **nothing at all** was asserted anywhere. +- **What is deliberately not checked**, named so the gaps are not mistaken for coverage. A value rendered into a scalar that is **hand-quoted in the template** (`"{{ .Values.x }}"`) is a string in the output by construction, so this test is blind to it — what it can still do is break the YAML when the value itself contains a quote or a backslash, and only `| quote` fixes that. Everything that is **not** a label, an annotation, an `env` value or a schedule: a bare `storage:` or `className:` is just as unquoted, but its value space is DNS labels and quantities, where no legal value is YAML-ambiguous. And **whether a cron schedule is a *valid* schedule** — that is the controller's judgement, and narrowing the schema to a regex was considered and rejected, because the defect was never a wrong schedule but a correct one rendered bare. + +Measured locally: **30 charts, 408 scalars verified, 0 failed**, no cluster. The fail path was exercised rather than assumed, in all three of its shapes, against a throwaway copy of the chart as it stands on `main`: the unquoted schedule aborted the render with `yaml: line 6: did not find expected alphabetic or numeric character` and the test reported it; with only the schedule quoted, the star-valued `env` entry aborted the render on the StatefulSet instead; and with only the bool-looking value left — a render that **succeeds** — the type assertion produced a `FAIL` naming the variable, the container and the exact API-server message. **Seven charts still render their `env` block bare** (`matrix-synapse`, `minecraft-router`, `minecraft-server`, `rallly`, `satisfactory`, `teamspeak-server`, and `paperless-ngx` until #11); the test is green for them only because their fixtures set no hostile `env` value, and the overlay is what makes the difference between covered and merely green. + ### Install-test (PR pipeline stage 2) For every chart a PR changes, `ci/run-install-test.sh ` runs against a throwaway k3d cluster: it applies the stub `OnePasswordItem` CRD plus the chart's fixtures, `helm install`s the chart with its CI values, and waits for all Deployments/StatefulSets to roll out (per-chart timeout). Since probes and security hardening are active on all workloads, "Ready" means the app actually answers. On failure the job dumps pod descriptions, events and container logs. @@ -518,6 +539,7 @@ These guidelines are **binding for every current and future chart** in this repo - Every workload container offers an additive `env` value block, appended to the env entries the chart always sets. - Entries are rendered through `tpl` (so values may contain Helm templating) and support the `value`, `secretKeyRef` and `configMapKeyRef` forms (pattern: `template-chart/templates/xxx.deployment.yaml`). +- **`| quote` on the rendered `value`, and on the `name`/`key` of the ref forms, is part of the pattern and not cosmetic.** An env value is typed `string` by Kubernetes, so a value of `"true"` or `"0755"` rendered bare becomes a YAML bool or integer and the API server rejects the whole manifest with `cannot unmarshal bool into Go struct field EnvVar.value of type string` — after a perfectly green render. A value beginning with `*` is worse in the other direction: it aborts the render with a YAML error naming the template, while the operator's own value appears nowhere in the message, and quoting it in the values file does not help. **The rule is held by `ci/run-yaml-scalar-render-test.sh` on every PR** (Gitea #11/#112), which is also how the count below is known rather than assumed: `template-chart` carries the rule in a comment claiming "every chart in this repo quotes here", and **six still do not** — `matrix-synapse`, `minecraft-router`, `minecraft-server`, `rallly`, `satisfactory`, `teamspeak-server`. They are green only because no fixture of theirs sets a hostile value; bringing a chart to the quoted form is a **major** bump for it, since it changes rendered output for values that currently work. - **Additive means additive: an entry repeating a name the chart already sets does not replace it, and what the pair does depends on when it arrives.** At install both entries reach the pod and the later one — the user's — wins. On an *existing* release the same entry is lost: Kubernetes merges `env` by name, the pair collapses to the chart's own value, and `helm upgrade` still reports success. Measured on `urlaubsverwaltung` (helm 3.19.0, Kubernetes 1.35): `helm get manifest` shows the variable twice while the live Deployment shows it once, carrying the chart's value. The failure mode is silence, so a chart whose `NOTES.txt` tells operators to override one of its own variables has to say that the override only takes at install time — and that a green upgrade proves nothing, the value belongs read back off the Deployment. ### 11. Scaling diff --git a/ci/run-selector-match-test.sh b/ci/run-selector-match-test.sh index 692bb9b..b688a0e 100755 --- a/ci/run-selector-match-test.sh +++ b/ci/run-selector-match-test.sh @@ -52,10 +52,22 @@ # # WHAT IS DELIBERATELY NOT CHECKED # -# - podAffinity / podAntiAffinity. They carry labelSelectors too, but their -# semantics are "relative to pods that exist", and an anti-affinity term -# matching nothing is a legitimate configuration rather than a mistake. -# Out of scope, and named so that the gap is not mistaken for coverage. +# - preferred podAffinity and podAntiAffinity of either strength. They carry +# labelSelectors too, but their semantics are "relative to pods that exist", +# and a term matching nothing is a legitimate configuration rather than a +# mistake: a preference that cannot be met is dropped and an anti-affinity +# over zero pods is satisfied. Out of scope, and named so that the gap is +# not mistaken for coverage. +# +# requiredDuringSchedulingIgnoredDuringExecution podAffinity is NOT in that +# gap and is checked (Gitea #101). Its failure is not a preference quietly +# dropped: a required term whose selector matches no pod in the release can +# never be satisfied, so the pod stays Pending forever. For the workload +# that introduced one here - paperless-ngx' consume CronJob, pinned onto the +# node of its StatefulSet pod so that Longhorn will mount the shared RWX +# volume - that would mean a mover that silently stops moving, with one +# Pending pod per schedule and a green render behind it. The term names a +# label written in ANOTHER template, which is the copy this test holds. # - selectors expressed ONLY through matchExpressions. Evaluating a set-based # requirement means reimplementing label-selector semantics, which is a # second parser by another name. Those are REPORTED as "not checked" and @@ -343,6 +355,55 @@ for doc in pdbs: ", ".join("%s=%s" % kv for kv in sorted(match_labels.items())), len(all_templates), "; ".join(known) or "none")) +# 3. Every REQUIRED podAffinity term, against ALL pod templates of the render +# (Gitea #101). Not against its own template: the whole point of such a term +# is to select a DIFFERENT workload's pod, so the correct question is whether +# anything in this release carries the labels it asks for. Preferred terms and +# anti-affinity of either strength are left alone - see the header. +for where, template in all_templates: + affinity = template.get("spec", {}).get("affinity") + if not isinstance(affinity, dict): + continue + pod_affinity = affinity.get("podAffinity") + if not isinstance(pod_affinity, dict): + continue + terms = pod_affinity.get("requiredDuringSchedulingIgnoredDuringExecution") + if not isinstance(terms, list): + continue + for i, term in enumerate(terms): + if not isinstance(term, dict): + lines.append("FAIL|%s: required podAffinity[%d] at %s is not a mapping" + % (chart, i, where)) + continue + tag = "required podAffinity[%d] on %s at %s" % ( + i, term.get("topologyKey", ""), where) + match_labels, reason = classify(term.get("labelSelector"), tag) + if match_labels is None: + if term.get("labelSelector") is None: + lines.append("FAIL|%s: %s" % (chart, reason)) + else: + lines.append("SKIP|%s: %s" % (chart, reason)) + continue + hits = [w for w, t in all_templates if matches(match_labels, labels_of(t))] + if hits: + lines.append("OK|%s: %s matches %d pod template(s): %s" + % (chart, tag, len(hits), ", ".join(hits))) + else: + known = sorted({"%s=%s" % kv for _, t in all_templates for kv in labels_of(t).items()}) + lines.append("FAIL|%s: %s selects %s, which matches NONE of the %d " + "pod template(s) in the same render. A REQUIRED " + "podAffinity term that matches no pod can never be " + "satisfied, so the pod it is attached to stays Pending " + "forever - for a CronJob that means one Pending pod per " + "schedule and a workload that silently stopped running, " + "with nothing in the render to show for it. The usual " + "cause is a label renamed in the template that carries " + "the selected pod. Labels actually present on the pod " + "templates: %s" + % (chart, tag, + ", ".join("%s=%s" % kv for kv in sorted(match_labels.items())), + len(all_templates), "; ".join(known) or "none")) + for line in lines: print(line) PY diff --git a/ci/run-yaml-scalar-render-test.sh b/ci/run-yaml-scalar-render-test.sh new file mode 100755 index 0000000..b130af2 --- /dev/null +++ b/ci/run-yaml-scalar-render-test.sh @@ -0,0 +1,412 @@ +#!/usr/bin/env bash +# YAML-scalar render test: an operator value that must arrive in the manifest as +# a STRING is held against the manifest it actually produced. +# +# Usage: ci/run-yaml-scalar-render-test.sh [chart ...] (no argument = all charts) +# +# WHAT IT GUARDS AGAINST (Gitea #11, and #112 turned out to be the same genus) +# +# A value is rendered into a YAML scalar without `| quote`. The operator quotes +# it correctly in their values file, Helm passes the string's CONTENT through, +# and the quotes are gone again in the manifest. Two outcomes, and the quiet one +# is the worse one: +# +# 1. LOUD, but misdirected - the content is not a legal plain scalar and the +# render aborts. Measured on paperless-ngx' consume CronJob, whose schedule +# was unquoted: a schedule in the commonest cron notation of all (a star, a +# slash, an interval) made `helm template` exit 1 with +# +# error converting YAML to JSON: yaml: line 6: did not find expected +# alphabetic or numeric character +# +# YAML read the leading "*" as an alias indicator. The message names the +# TEMPLATE and the operator's own value is nowhere in it, so the search +# starts in the wrong place. "@daily" fails the same way. +# 2. SILENT until the API server - the content IS a legal plain scalar, just +# not a string. "true" becomes a bool, "0755" becomes an integer, and the +# chart renders perfectly. Kubernetes then rejects the object with +# "cannot unmarshal number into Go struct field EnvVar.value of type +# string" - at apply time, which for this repository means in the +# install-test or in a cluster, never in the render. +# +# Neither is caught by "does helm template exit 0" alone: the first one is, but +# only if something actually renders the object, and the second one is not +# caught at all. +# +# WHAT IS ASSERTED, and all three are properties Kubernetes itself requires - +# there is no chart for which the opposite is a legitimate configuration, which +# is why this needs no per-chart expectations: +# +# 1. CronJob .spec.schedule is a non-empty string. +# 2. Every env[].value of every container and initContainer is a string. +# 3. Every metadata.labels and metadata.annotations value is a string +# (the API types both as map[string]string). +# +# WHY OVER ALL CHARTS, not the changed ones: the same reason as +# ci/run-quantity-render-test.sh and ci/run-selector-match-test.sh. The env +# block is copied into every chart rather than shared, so the unquoted form +# travels by copy into whatever chart is started from an older one - and that +# chart's own pull request is not where anyone would look for it. Running over +# everything also means there is no list to maintain: adding a chart enrols it. +# +# THE OVERLAYS, AND WHY THIS TEST IS USELESS WITHOUT THEM +# +# The defect above survived in a shipped chart for months for one reason: the +# object that carried it is rendered by NO fixture. paperless-ngx' CronJob sits +# behind smbConnection.enabled, which is false in the chart defaults and in +# every committed fixture, so neither lint, nor the render tests, nor the +# install-test ever produced it. A test that renders only what the fixtures +# already render would have been just as blind. +# +# So ci/yaml-scalar/.yaml is an overlay applied LAST whose job is to +# switch such objects on AND to feed them the hostile values - a schedule +# beginning with a star, an env value that is a bare "true". Two rules follow, +# the same ones ci/run-selector-match-test.sh works under: +# +# 1. A chart that HAS an overlay and still renders none of the three things +# above FAILS. The overlay exists for exactly that, so an overlay that +# stopped reaching its object is a finding and not a shrug. +# 2. If nothing at all was asserted across the whole run, the run fails. A +# pass over zero assertions is not a pass. +# +# WHAT IS DELIBERATELY NOT CHECKED, so the gap is not mistaken for coverage: +# +# - Values rendered into a scalar that is hand-quoted in the template +# ("{{ .Values.x }}"). Those are strings in the output, so this test is +# blind to them by construction; what they can still do is break the YAML +# when the value itself contains a quote or a backslash, and only `| quote` +# fixes that. Reported here rather than silently implied. +# - Everything that is not a label, an annotation, an env value or a schedule. +# A bare `storage:` or `className:` is just as unquoted, but its value space +# is DNS labels and quantities, where no legal value is YAML-ambiguous. The +# three above are where operator free text actually lands. +# - Whether a cron schedule is a VALID schedule. That is the cron +# controller's judgement, and narrowing the schema to a regex was considered +# and rejected: the defect was never a wrong schedule, it was a correct one +# rendered bare. +# +# How the YAML is parsed: with helm, via ci/yaml2json, exactly as in +# ci/run-selector-match-test.sh - the runners have neither pyyaml nor yq, and a +# YAML reader written in awk here would fail by quietly reading nothing and +# staying green. That script's header carries the full argument. +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +CI_DIR="$ROOT/ci" +YAML2JSON="$CI_DIR/yaml2json" +OVERLAY_DIR="$CI_DIR/yaml-scalar" +RELEASE="scalar" + +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="" +CHECKED="" +NOT_EXERCISED="" +TOTAL_ASSERTIONS=0 +SUMMARY_ROWS="" + +for CHART in "${CHARTS[@]}"; do + if [ ! -f "$ROOT/$CHART/Chart.yaml" ]; then + echo "ERROR: '$CHART' is not a chart directory" >&2 + exit 1 + fi + + # A chart with a declared dependency cannot be rendered at all while charts/ + # is empty, and this job runs over EVERY chart on EVERY pull request. + "$ROOT/ci/helm-dependencies.sh" "$ROOT/$CHART" + + VALUES_ARGS=() + VALUES_LABEL="chart defaults" + 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" + 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" + fi + HAS_OVERLAY="" + if [ -f "$OVERLAY_DIR/$CHART.yaml" ]; then + # Last, so it wins over the install-test values. + VALUES_ARGS+=(-f "$OVERLAY_DIR/$CHART.yaml") + VALUES_LABEL="$VALUES_LABEL + ci/yaml-scalar/$CHART.yaml" + HAS_OVERLAY=1 + fi + + echo "::group::$CHART ($VALUES_LABEL)" + + RENDER="$WORK/$CHART.manifest.yaml" + if ! helm template "$RELEASE" "$ROOT/$CHART" \ + ${VALUES_ARGS[@]+"${VALUES_ARGS[@]}"} >"$RENDER" 2>"$WORK/err"; then + sed 's/^/ /' "$WORK/err" + echo "::endgroup::" + echo "::error title=yaml-scalar $CHART::$CHART does not render with these values. If the error is a YAML parse error naming a template line, that IS the finding this test exists for: a value reached a scalar unquoted. Reproduce: helm template $RELEASE $CHART $(printf '%s ' "${VALUES_ARGS[@]+${VALUES_ARGS[@]}}")" + FAILED="$FAILED $CHART" + continue + fi + + # The document split, the only textual step. A wrong split cannot produce a + # false pass: the result stops being parsable and helm exits 1. + DOCS="$WORK/$CHART.docs.yaml" + awk ' + BEGIN { print "docs:"; started = 0 } + /^---[ \t]*$/ { print " -"; started = 1; next } + { + if (!started) { print " -"; started = 1 } + if (length($0) == 0) print ""; else print " " $0 + } + ' "$RENDER" >"$DOCS" + + JSON="$WORK/$CHART.json" + if ! helm template y2j "$YAML2JSON" -f "$DOCS" 2>"$WORK/err" \ + | awk '/^\{/ { print; exit }' >"$JSON"; then + sed 's/^/ /' "$WORK/err" + echo "::endgroup::" + echo "::error title=yaml-scalar $CHART::the rendered manifests of $CHART could not be re-read as YAML by ci/yaml2json. That is a parse failure, not a scalar finding." + FAILED="$FAILED $CHART" + continue + fi + if [ ! -s "$JSON" ]; then + echo "::endgroup::" + echo "::error title=yaml-scalar $CHART::ci/yaml2json produced no JSON for $CHART - the manifests were not re-read, so nothing was checked." + FAILED="$FAILED $CHART" + continue + fi + + # --- The comparison -------------------------------------------------------- + # Standard library only, and it never decides the exit code itself: it writes + # "VERDICT|detail" lines and the loop below decides, so a verdict can never + # be lost to a swallowed status. + set +e + python3 - "$CHART" "$JSON" >"$WORK/$CHART.out" 2>"$WORK/$CHART.pyerr" <<'PY' +import json, sys + +chart, path = sys.argv[1], sys.argv[2] +docs = json.load(open(path)).get("docs") or [] +lines = [] + + +def name_of(doc): + meta = doc.get("metadata") + if isinstance(meta, dict) and meta.get("name"): + return str(meta["name"]) + return "" % doc.get("kind", "object") + + +def typename(value): + return {bool: "bool", int: "int", float: "float", type(None): "null", + dict: "mapping", list: "list"}.get(type(value), type(value).__name__) + + +def pod_templates(node, path="$"): + """Any value of a key named "template" carrying a spec with containers. + Found by walking, so a CronJob's spec.jobTemplate.spec.template needs no + special case and neither does whatever workload kind a chart adds next.""" + out = [] + if isinstance(node, dict): + for key, value in node.items(): + here = "%s.%s" % (path, key) + if (key == "template" and isinstance(value, dict) + and isinstance(value.get("spec"), dict) + and "containers" in value["spec"]): + out.append((here, value)) + out.extend(pod_templates(value, here)) + elif isinstance(node, list): + for i, value in enumerate(node): + out.extend(pod_templates(value, "%s[%d]" % (path, i))) + return out + + +def metadatas(node, path="$"): + """Every metadata mapping anywhere in the render - top level and inside pod + templates both, because labels and annotations exist at both levels.""" + out = [] + if isinstance(node, dict): + for key, value in node.items(): + here = "%s.%s" % (path, key) + if key == "metadata" and isinstance(value, dict): + out.append((here, value)) + out.extend(metadatas(value, here)) + elif isinstance(node, list): + for i, value in enumerate(node): + out.extend(metadatas(value, "%s[%d]" % (path, i))) + return out + + +# --- 1. CronJob schedules --------------------------------------------------- +for doc in docs: + if not isinstance(doc, dict) or doc.get("kind") != "CronJob": + continue + where = "CronJob %s .spec.schedule" % name_of(doc) + spec = doc.get("spec") if isinstance(doc.get("spec"), dict) else {} + schedule = spec.get("schedule", "") + if isinstance(schedule, str) and schedule.strip(): + lines.append("OK|%s: %s is the string %r" % (chart, where, schedule)) + elif schedule == "": + lines.append("FAIL|%s: %s is missing entirely - a CronJob without a " + "schedule is rejected by the API server" % (chart, where)) + else: + lines.append("FAIL|%s: %s rendered as %s (%r), not a string. A schedule " + "is operator free text and has to leave the template " + "through | quote; without it YAML re-reads the content and " + "a schedule beginning with a digit survives by luck while " + "one beginning with * or @ does not" + % (chart, where, typename(schedule), schedule)) + +# --- 2. env values ---------------------------------------------------------- +for where, template in pod_templates({"docs": docs}): + spec = template.get("spec") if isinstance(template.get("spec"), dict) else {} + for kind in ("initContainers", "containers"): + for container in spec.get(kind) or []: + if not isinstance(container, dict): + continue + cname = container.get("name", "") + for entry in container.get("env") or []: + if not isinstance(entry, dict) or "value" not in entry: + continue # valueFrom entries carry no literal to check + value = entry["value"] + tag = "env %s of container %s at %s" % ( + entry.get("name", ""), cname, where) + if isinstance(value, str): + lines.append("OK|%s: %s is a string" % (chart, tag)) + else: + lines.append("FAIL|%s: %s rendered as %s (%r), not a string. " + "Kubernetes types env[].value as a string and " + "the API server rejects the whole manifest with " + "\"cannot unmarshal %s into Go struct field " + "EnvVar.value of type string\" - at apply time, " + "not in the render. The fix is | quote at the " + "render site; template-chart carries the form" + % (chart, tag, typename(value), value, + "number" if not isinstance(value, bool) else "bool")) + +# --- 3. labels and annotations ---------------------------------------------- +for where, meta in metadatas({"docs": docs}): + for field in ("labels", "annotations"): + block = meta.get(field) + if not isinstance(block, dict): + continue + for key, value in block.items(): + tag = "%s[%r] at %s" % (field, key, where) + if isinstance(value, str): + lines.append("OK|%s: %s is a string" % (chart, tag)) + else: + lines.append("FAIL|%s: %s rendered as %s (%r), not a string. " + "Kubernetes types %s as map[string]string, so the " + "API server rejects the object - the render stays " + "green. The fix is | quote at the render site" + % (chart, tag, typename(value), value, field)) + +for line in lines: + print(line) +PY + RC=$? + set -e + if [ "$RC" -gt 1 ]; then + sed 's/^/ /' "$WORK/$CHART.pyerr" + echo "::endgroup::" + echo "::error title=yaml-scalar $CHART::the scalar comparison itself failed for $CHART (python exit $RC). Treated as a failure, never as a pass." + FAILED="$FAILED $CHART" + continue + fi + + CHART_FAIL="" + CHART_OK=0 + while IFS='|' read -r VERDICT DETAIL; do + case "$VERDICT" in + OK) + CHART_OK=$((CHART_OK + 1)) + echo " ok $DETAIL" + ;; + FAIL) + CHART_FAIL=1 + echo " FAIL $DETAIL" + echo "::error title=yaml-scalar $CHART::$DETAIL" + ;; + *) + [ -n "$VERDICT" ] && echo " $VERDICT $DETAIL" + ;; + esac + done <"$WORK/$CHART.out" + echo "::endgroup::" + + TOTAL_ASSERTIONS=$((TOTAL_ASSERTIONS + CHART_OK)) + + if [ -n "$CHART_FAIL" ]; then + FAILED="$FAILED $CHART" + SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | **failed** | $CHART_OK |"$'\n' + echo "$CHART: FAILED" + elif [ "$CHART_OK" -gt 0 ]; then + CHECKED="$CHECKED $CHART" + SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | ok | $CHART_OK |"$'\n' + echo "$CHART: ok ($CHART_OK scalar(s) checked)" + elif [ -n "$HAS_OVERLAY" ]; then + FAILED="$FAILED $CHART" + SUMMARY_ROWS="${SUMMARY_ROWS}| \`$CHART\` | **failed** | 0 |"$'\n' + echo "::error title=yaml-scalar $CHART::$CHART has an overlay at ci/yaml-scalar/$CHART.yaml but its render carried no schedule, no env value and no label or annotation at all, so nothing was checked. That overlay exists only to make such a scalar render; a green tick here would be a tick for a check that ran over nothing." + echo "$CHART: FAILED (overlay present, nothing asserted)" + else + NOT_EXERCISED="$NOT_EXERCISED $CHART" + echo "$CHART: nothing of the three kinds rendered - nothing to check." + fi +done + +# --- 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 "### YAML-scalar render test (Gitea #11)" + echo + echo "Every \`CronJob.spec.schedule\`, every \`env[].value\` and every label and" + echo "annotation value in a chart's render, held against the type Kubernetes requires" + echo "for it. A value rendered without \`| quote\` either aborts the render with an" + echo "error pointing at the template instead of at the value, or arrives as a bool or" + echo "an integer and is rejected by the API server at apply time - never in the render." + echo + echo "| Result | Count | Charts |" + echo "|---|---|---|" + echo "| Scalars checked | $(count "$CHECKED") | $(listing "$CHECKED") |" + echo "| Nothing of the kind rendered | $(count "$NOT_EXERCISED") | $(listing "$NOT_EXERCISED") |" + echo "| Failed | $(count "$FAILED") | $(listing "$FAILED") |" + echo + echo "$TOTAL_ASSERTIONS scalar(s) verified in total." + if [ -n "$SUMMARY_ROWS" ]; then + echo + echo "| Chart | Result | Checked |" + echo "| --- | --- | --- |" + printf '%s' "$SUMMARY_ROWS" + fi +} >>"$SUMMARY" + +echo "===================== yaml scalar render test ======================" +echo "checked: $(count "$CHECKED")${CHECKED:+ -$CHECKED}" +echo "not exercised: $(count "$NOT_EXERCISED")${NOT_EXERCISED:+ -$NOT_EXERCISED}" +echo "failed: $(count "$FAILED")${FAILED:+ -$FAILED}" +echo "scalars verified: $TOTAL_ASSERTIONS" +echo "===================================================================" + +if [ -n "$FAILED" ]; then + echo "ERROR: a value that has to arrive as a string did not, in:$FAILED" >&2 + exit 1 +fi + +if [ "$TOTAL_ASSERTIONS" -eq 0 ]; then + echo "ERROR: not a single scalar was checked in this run. Either no chart renders a schedule, an env value, a label or an annotation any more, or the overlays under ci/yaml-scalar/ no longer reach their objects. A pass over zero assertions is not a pass." >&2 + exit 1 +fi diff --git a/ci/selector-match/paperless-ngx.yaml b/ci/selector-match/paperless-ngx.yaml new file mode 100644 index 0000000..2d5a34e --- /dev/null +++ b/ci/selector-match/paperless-ngx.yaml @@ -0,0 +1,25 @@ +# Selector-match overlay for paperless-ngx (Gitea #101). +# +# Its one job: make the consume CronJob render, so that the required podAffinity +# it carries is actually held against the StatefulSet's pod labels. The template +# sits behind smbConnection.enabled, false in the chart defaults and in +# ci/paperless-ngx/test-values.yaml, so without this overlay the affinity term +# exists in the repository and in no render - and the test would pass over it. +# +# What it guards: the term selects the StatefulSet pod by a label written in +# ANOTHER template (paperless-server.sfs.yaml). Renaming that label leaves both +# files rendering perfectly while the mover can never be scheduled again, one +# Pending pod per schedule, nothing in the render to show for it. +# +# Kept separate from ci/yaml-scalar/paperless-ngx.yaml although both switch SMB +# on: that one feeds the CronJob a hostile SCHEDULE and hostile env values for a +# different test, and folding the two would make each test's failure depend on +# the other's fixture. +smbConnection: + enabled: true + server: "//smb.ci.local/scans" + +secrets: + # Satisfies the required() behind the SMB PersistentVolume's + # nodeStageSecretRef. Nothing is mounted in a render. + smbConnectionSecretRef: op://ci/paperless-smb diff --git a/ci/yaml-scalar/paperless-ngx.yaml b/ci/yaml-scalar/paperless-ngx.yaml new file mode 100644 index 0000000..6ee0a0d --- /dev/null +++ b/ci/yaml-scalar/paperless-ngx.yaml @@ -0,0 +1,53 @@ +# YAML-scalar overlay for paperless-ngx (Gitea #11). +# +# THIS FILE IS THE TEST CASE the issue asked for, and its first job is to make +# an object exist at all. templates/paperless-consume.cronjob.yaml sits behind +# smbConnection.enabled, which is false in the chart defaults and false in +# ci/paperless-ngx/test-values.yaml - deliberately, because the install-test has +# no SMB CSI driver. So before this overlay NOTHING in the repository rendered +# that CronJob: not lint, not the render tests, not the install-test. That is +# the whole reason an unquoted schedule survived in a shipped chart, and it is +# why the fix comes with a render test rather than with a fixture change. +# +# The values below are hostile on purpose. Against the chart before 13.0.0 this +# overlay does not produce a finding - it produces an aborted render, which this +# test reports as the finding it is: +# +# Error: YAML parse error on paperless-ngx/templates/paperless-consume.cronjob.yaml: +# error converting YAML to JSON: yaml: line 6: did not find expected +# alphabetic or numeric character +smbConnection: + enabled: true + server: "//smb.ci.local/scans" + # A schedule beginning with "*" - the commonest cron notation there is, and + # the one that YAML reads as an alias indicator. Written here as a quoted + # string, which is exactly how an operator would write it and exactly what + # does NOT help: Helm passes the content through, and without | quote in the + # template the star stands bare in the manifest again. + cronJob: "*/15 * * * *" + +secrets: + # The CronJob's SMB volume has a required() secret ref behind it; the + # PersistentVolume names it as nodeStageSecretRef. Nothing is mounted in a + # render, so no fixture Secret is needed - only a value that satisfies the + # required(). + smbConnectionSecretRef: op://ci/paperless-smb + +paperless: + # The silent half of the genus, on the chart's additive env block: both of + # these are STRINGS in this file and both are schema-valid, and before 13.0.0 + # the first one aborted the render while the second arrived in the manifest as + # a YAML bool - which the API server rejects at apply time with "cannot + # unmarshal bool into Go struct field EnvVar.value of type string", so the + # render test is the only place it can be caught cheaply. + env: + - name: PAPERLESS_STAR_VALUE + value: "*never-interpreted*" + - name: PAPERLESS_BOOLEAN_LOOKING_VALUE + value: "true" + # The variable the whole env block was asked for (Gitea #112): the log off + # the NFS-backed RWX data volume and onto the /tmp emptyDir this pod already + # mounts. Harmless in a render, and it keeps the worked example honest by + # being the one an operator will actually copy. + - name: PAPERLESS_LOGGING_DIR + value: "/tmp/paperless-log"