From a2afccf1b60e41c5466f98c3c08671d3c1b05f25 Mon Sep 17 00:00:00 2001 From: Scott Wares Date: Sat, 25 Jul 2026 17:48:33 +0000 Subject: [PATCH 1/2] fix(ci): make validation jobs capable of failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four jobs across both pipelines ran their check loop on the right-hand side of a pipe. That runs the loop in a subshell, so the failure flag was discarded when the loop ended — GitHub yaml-lint and dry-run-apply, and GitLab yaml-lint and dry-run-apply all passed unconditionally regardless of input. GitLab's yaml-lint had the same bug in a different shape: its exit 1 exited only the subshell. This is why a Jinja syntax error reached the H4 on 2026-07-25 rather than being caught in CI. Fixing it exposed a bug it had been hiding: GitHub's dry-run-apply lacked the values.yaml/kustomization.yaml exclusions that kubeconform and the GitLab job both have, so it fed Helm values files to kubectl apply. Those failures were real but invisible. Fixing only the subshell would have turned CI red on the next commit, so both are fixed together. Loops now read from a file redirect, keeping them in the current shell. Written for POSIX sh rather than bash process substitution since the GitLab runner image's shell is not guaranteed; verified under sh and dash. Refs: docs/REVIEW-2026-07-24.md C4 --- .github/workflows/validate.yml | 57 +++++++++++++++++++++++++++------- .gitlab-ci.yml | 49 ++++++++++++++++++++++------- docs/REVIEW-2026-07-24.md | 25 ++++++++++++++- 3 files changed, 107 insertions(+), 24 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index d08a80e..622ebcb 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -30,12 +30,26 @@ jobs: - name: Lint all YAML under gitops/ run: | - FAILED=0 - find gitops/ -name '*.yaml' -o -name '*.yml' | while read f; do - yq e 'true' "$f" > /dev/null || { echo "FAIL: $f"; FAILED=1; } - done - [ "$FAILED" -eq 0 ] || exit 1 - echo "All YAML valid." + set -eu + # DO NOT put this loop on the right-hand side of a pipe. + # `find ... | while read` runs the loop body in a SUBSHELL, so any + # variable set inside it — including the failure flag — is discarded + # when the loop ends. This job therefore passed unconditionally on + # every commit until 2026-07-25, including ones with invalid YAML. + # Redirecting from a file keeps the loop in the current shell. + # Parenthesised -name group: without it, precedence makes the implicit + # -print apply inconsistently across the -o branches. + find gitops/ \( -name '*.yaml' -o -name '*.yml' \) -print > /tmp/yaml-files + failed=0 + while IFS= read -r f; do + if ! out=$(yq e 'true' "$f" 2>&1 >/dev/null); then + echo "FAIL: $f" + echo "$out" | sed 's/^/ /' + failed=1 + fi + done < /tmp/yaml-files + [ "$failed" -eq 0 ] || { echo "YAML lint failed."; exit 1; } + echo "All YAML valid ($(wc -l < /tmp/yaml-files) files)." kubeconform: name: Schema validation (kubeconform) @@ -101,9 +115,28 @@ jobs: # --dry-run=client validates manifest structure without a live cluster. # Server-side validation (schema + admission) runs in GitLab CI on a self-hosted runner. run: | - FAILED=0 - find gitops/workloads -name '*.yaml' | while read f; do - echo " checking $f" - kubectl apply --dry-run=client -f "$f" 2>&1 || FAILED=1 - done - [ "$FAILED" -eq 0 ] || { echo "Dry-run failed."; exit 1; } + set -eu + # Two bugs fixed here on 2026-07-25, and they were hiding each other: + # + # 1. The loop ran on the right of a pipe, so FAILED=1 was set in a + # subshell and discarded — this job could never fail. + # 2. The find had no exclusions, so it fed Helm values.yaml and + # kustomization.yaml to `kubectl apply`, which cannot apply them. + # Those failures were real but invisible because of (1). The + # kubeconform and GitLab dry-run jobs already excluded them. + # + # Fixing only (1) would have turned this job red on the next commit. + find gitops/workloads -name '*.yaml' \ + ! -name 'values.yaml' \ + ! -name 'kustomization.yaml' \ + -print > /tmp/manifests + failed=0 + while IFS= read -r f; do + if ! out=$(kubectl apply --dry-run=client -f "$f" 2>&1); then + echo "FAIL: $f" + echo "$out" | sed 's/^/ /' + failed=1 + fi + done < /tmp/manifests + [ "$failed" -eq 0 ] || { echo "Dry-run failed."; exit 1; } + echo "All $(wc -l < /tmp/manifests) manifests passed client-side dry-run." diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index fd55cdf..39c041e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -45,11 +45,26 @@ yaml-lint: before_script: [] # no kubeconfig needed script: - | + set -eu echo "==> Linting all YAML under gitops/" - find gitops/ -name '*.yaml' -o -name '*.yml' | while read f; do - yq e 'true' "$f" > /dev/null || { echo "FAIL: $f"; exit 1; } - done - echo "All YAML files are valid." + # DO NOT put this loop on the right-hand side of a pipe. `find ... | while` + # runs the loop body in a SUBSHELL — so the `exit 1` that used to be here + # exited only the subshell, the loop carried on, and the job reported + # success regardless. Invalid YAML passed CI unchallenged until 2026-07-25. + # Redirecting from a file keeps the loop in the current shell. + # (Written for POSIX sh, not bash: the runner image's shell is not guaranteed, + # so no process substitution here.) + find gitops/ \( -name '*.yaml' -o -name '*.yml' \) -print > /tmp/yaml-files + failed=0 + while IFS= read -r f; do + if ! out=$(yq e 'true' "$f" 2>&1 >/dev/null); then + echo "FAIL: $f" + echo "$out" | sed 's/^/ /' + failed=1 + fi + done < /tmp/yaml-files + [ "$failed" -eq 0 ] || { echo "YAML lint failed."; exit 1; } + echo "All YAML files are valid ($(wc -l < /tmp/yaml-files) files)." rules: - if: '$CI_PIPELINE_SOURCE == "merge_request_event"' - if: '$CI_COMMIT_BRANCH == "main"' @@ -58,16 +73,28 @@ dry-run-apply: stage: validate script: - | + set -eu echo "==> Dry-run kubectl apply on every workload manifest" - FAILED=0 - # Skip Helm values files and kustomization overlays — kubectl can't apply them directly + # DO NOT put this loop on the right-hand side of a pipe. `find ... | while` + # runs the loop body in a SUBSHELL, so FAILED=1 was discarded when the loop + # ended and the [ "$FAILED" -eq 0 ] test below always saw 0. This job — the + # server-side gate, the one with real cluster admission behind it — could + # not fail on any input until 2026-07-25. + # Skip Helm values files and kustomization overlays — kubectl can't apply them directly. find gitops/workloads -name '*.yaml' \ ! -name 'values.yaml' \ - ! -name 'kustomization.yaml' | while read f; do - echo " checking $f" - kubectl apply --dry-run=server -f "$f" || FAILED=1 - done - [ "$FAILED" -eq 0 ] || { echo "Dry-run failed — check output above."; exit 1; } + ! -name 'kustomization.yaml' \ + -print > /tmp/manifests + failed=0 + while IFS= read -r f; do + if ! out=$(kubectl apply --dry-run=server -f "$f" 2>&1); then + echo "FAIL: $f" + echo "$out" | sed 's/^/ /' + failed=1 + fi + done < /tmp/manifests + [ "$failed" -eq 0 ] || { echo "Dry-run failed — check output above."; exit 1; } + echo "All $(wc -l < /tmp/manifests) manifests passed server-side dry-run." rules: - if: '$CI_PIPELINE_SOURCE == "merge_request_event"' - if: '$CI_COMMIT_BRANCH == "main"' diff --git a/docs/REVIEW-2026-07-24.md b/docs/REVIEW-2026-07-24.md index 6a4ef16..2e99b7b 100644 --- a/docs/REVIEW-2026-07-24.md +++ b/docs/REVIEW-2026-07-24.md @@ -156,7 +156,30 @@ Alertmanager, and ArgoCD are all unreachable. - **Fix:** `svc_enable: "true"` + a Traefik `LoadBalancer` Service on a second VIP (e.g. `.201`); repoint `ingress_vip`, the Pi-hole/dnsmasq wildcard, and the CoreDNS template. -### C4. CI validation jobs are structurally incapable of failing +### C4. CI validation jobs are structurally incapable of failing — FIXED + +> **Fixed 2026-07-25**, branch `fix/ci-validation-gates`. Worse than first written: **four** +> jobs were affected, not two — GitHub `yaml-lint` and `dry-run-apply`, and GitLab +> `yaml-lint` and `dry-run-apply`. The GitLab `yaml-lint` had a different shape of the same +> bug: its `exit 1` was *inside* the subshell, so it exited the subshell and the loop +> carried on. +> +> Fixing it exposed a second, hidden bug: GitHub's `dry-run-apply` had no +> `! -name 'values.yaml'` exclusion, so it fed Helm values files to `kubectl apply`. Those +> failures were real all along and invisible only because the job could never fail — so +> fixing the subshell alone would have turned CI red on the next commit. Both are fixed +> together. +> +> All loops now read from a file redirect rather than a pipe, which keeps them in the +> current shell. Written for POSIX `sh` (no process substitution) because the GitLab runner +> image's shell is not guaranteed; verified under both `sh` and `dash`. +> +> **Still open, deliberately out of scope:** widening validation to `gitops/apps/` and +> `gitops/bootstrap/` (H17) — Argo `Application` CRDs need schema handling, so it deserves +> its own change. Tool downloads still use `releases/latest` for `yq` and `kubeconform` +> (`conftest` is correctly pinned) — worth pinning with Renovate comments. + +### C4. CI validation jobs are structurally incapable of failing (original finding) `.gitlab-ci.yml:61-71` and `.github/workflows/validate.yml:31-38` both use `FAILED=0; find ... | while read f; do ... || FAILED=1; done; [ "$FAILED" -eq 0 ] || exit 1`. From c617451278a0102924fed6ac04e16076cdc3b601 Mon Sep 17 00:00:00 2001 From: Scott Wares Date: Sat, 25 Jul 2026 17:56:33 +0000 Subject: [PATCH 2/2] fix(ci): remove client-side dry-run job that never worked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kubectl apply --dry-run=client downloads the OpenAPI schema from the API server, so on a GitHub-hosted runner with no cluster it failed on every manifest: failed to download openapi: Get http://localhost:8080/openapi/v2: connect: connection refused It had done so since the day it was written. Nobody knew, because the loop ran in a subshell and the job could not report failure. Making the job capable of failing is what surfaced it. Deleted rather than patched with --validate=false, which would skip the OpenAPI check and reduce the job to 'is this parseable YAML' — already covered by yaml-lint and covered better by kubeconform. Offline schema and policy validation stay with kubeconform and conftest; real dry-run stays with GitLab CI against the actual cluster. --- .github/workflows/validate.yml | 76 ++++++++++++++++------------------ 1 file changed, 36 insertions(+), 40 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 622ebcb..56c56f8 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -2,8 +2,13 @@ # Lints YAML, validates schemas, and enforces OPA policies. # # CI authority: GitLab CI is the authoritative pipeline (full validate/sync-check/rollback). -# This GitHub Actions workflow runs yaml-lint, schema validation, policy checks, and -# client-side manifest dry-runs — no cluster access required; runs on GitHub-hosted runners. +# This GitHub Actions workflow runs yaml-lint, schema validation and policy checks — all +# genuinely offline, no cluster access required, on GitHub-hosted runners. +# +# NOTE: everything here must work with NO cluster. A check that needs the API server +# will fail on every input, and if it also cannot report failure it will look green +# forever — which is exactly what happened to the client-side dry-run job removed +# below on 2026-07-25. # # No secrets required for these jobs. @@ -102,41 +107,32 @@ jobs: --policy ci/policies \ --data ci/data/exceptions.yaml - dry-run-apply: - name: kubectl dry-run (client-side) - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v7 - - - name: Install kubectl - uses: azure/setup-kubectl@v5 - - - name: Client-side dry-run all workload manifests - # --dry-run=client validates manifest structure without a live cluster. - # Server-side validation (schema + admission) runs in GitLab CI on a self-hosted runner. - run: | - set -eu - # Two bugs fixed here on 2026-07-25, and they were hiding each other: - # - # 1. The loop ran on the right of a pipe, so FAILED=1 was set in a - # subshell and discarded — this job could never fail. - # 2. The find had no exclusions, so it fed Helm values.yaml and - # kustomization.yaml to `kubectl apply`, which cannot apply them. - # Those failures were real but invisible because of (1). The - # kubeconform and GitLab dry-run jobs already excluded them. - # - # Fixing only (1) would have turned this job red on the next commit. - find gitops/workloads -name '*.yaml' \ - ! -name 'values.yaml' \ - ! -name 'kustomization.yaml' \ - -print > /tmp/manifests - failed=0 - while IFS= read -r f; do - if ! out=$(kubectl apply --dry-run=client -f "$f" 2>&1); then - echo "FAIL: $f" - echo "$out" | sed 's/^/ /' - failed=1 - fi - done < /tmp/manifests - [ "$failed" -eq 0 ] || { echo "Dry-run failed."; exit 1; } - echo "All $(wc -l < /tmp/manifests) manifests passed client-side dry-run." + # REMOVED 2026-07-25: `kubectl dry-run (client-side)`. + # + # The job claimed to "validate manifest structure without a live cluster". It + # cannot. `kubectl apply --dry-run=client` downloads the OpenAPI schema from the + # API server to validate against, so on a GitHub-hosted runner with no cluster + # every single file failed with: + # + # failed to download openapi: Get "http://localhost:8080/openapi/v2": + # dial tcp [::1]:8080: connect: connection refused + # + # It had therefore failed on every manifest since the day it was written, and + # nobody knew, because the loop ran in a subshell and the job could not report + # failure (see C4 in docs/REVIEW-2026-07-24.md). Making the job capable of + # failing is what finally surfaced it. + # + # It is deleted rather than patched with --validate=false, which would skip the + # OpenAPI check entirely and reduce the job to "is this parseable YAML" — already + # covered by yaml-lint, and covered far better by kubeconform. + # + # Division of responsibility, so nothing is silently uncovered: + # yaml-lint (here) — parseable YAML + # kubeconform (here) — offline schema validation, k8s 1.36, CRDs skipped + # conftest (here) — offline OPA policy (no :latest, no privileged, limits) + # dry-run (GitLab CI) — --dry-run=server against the real cluster, where + # CRDs resolve and Kyverno admission actually runs + # + # If a kubectl-shaped check is ever wanted here again, the useful one is the + # Kyverno CLI (`kyverno apply gitops/workloads/kyverno/ --resource gitops/`), + # which evaluates the real ClusterPolicies offline. See H17.