From 474a73d52811b76178fc52209412d1174f8daf28 Mon Sep 17 00:00:00 2001 From: Gonzalo Rojas Date: Thu, 27 Aug 2026 13:43:29 -0300 Subject: [PATCH] ci: reject new moving version defaults, and document what to pin check-version-pinning.sh rejects a new moving default, a repository URL pinned to a branch, or a helm_release with no version. Without it the pinning erodes on its own: someone adds default = "latest" in six months and nothing notices. It runs in pre-commit and again as a step in the terraform-lint workflow -- folded into the existing job rather than given its own, because it needs the same changed-file list that job already computes and adding it there costs no new check. That mirrors what the workflow already does for tflint, whose comment calls itself the CI backstop for anyone committing with --no-verify. Findings are keyed by path plus variable name rather than line number so the baseline survives files moving, and the patterns use POSIX classes rather than \s, which neither POSIX awk nor GNU grep supports and which would have made the check silently useless on the runner. It found three violations a manual sweep had missed. Fifteen findings are baselined. Ten are the scopes and service-spec repositories, which can be pinned without a breaking change and are deferred for their own review. The other five are fixed by the separate "require an explicit version" PR, whose base does not contain this file and so cannot remove the entries itself. VERSIONS.md lists every version to pin with its current value, and records the one trap worth knowing before bumping an image by hand: k8s-traffic-manager publishes a v2.0.2 built five months before 1.8.0, so the higher version number is the older build from a line that was not continued. Nothing here bumps a version automatically. Doing that would put the drift back in documentation form and contradict the rule of pinning what you already run. The root README pointed its Usage example at v6.11.0, eight releases behind this line, because the generator rewrites every module README's source ref but never touches the root one. --- .github/workflows/tflint-unused.yml | 16 +++- .pre-commit-config.yaml | 7 ++ README.md | 7 +- VERSIONS.md | 134 +++++++++++++++++++++++++++ scripts/check-version-pinning.sh | 81 ++++++++++++++++ scripts/version-pinning-baseline.txt | 26 ++++++ 6 files changed, 268 insertions(+), 3 deletions(-) create mode 100644 VERSIONS.md create mode 100755 scripts/check-version-pinning.sh create mode 100644 scripts/version-pinning-baseline.txt diff --git a/.github/workflows/tflint-unused.yml b/.github/workflows/tflint-unused.yml index b2be4f8f5..11c7e1c4e 100644 --- a/.github/workflows/tflint-unused.yml +++ b/.github/workflows/tflint-unused.yml @@ -1,4 +1,4 @@ -name: tflint-unused-declarations +name: terraform-lint on: pull_request: @@ -7,7 +7,7 @@ on: jobs: check: - name: Check for unused declarations + name: Lint changed Terraform runs-on: ubuntu-24.04 steps: - name: Checkout repository @@ -52,3 +52,15 @@ jobs: echo "::error::tflint found unused declarations in one or more changed modules (see groups above)." exit 1 fi + + # CI backstop for the local pre-commit hook. + - name: Check version pinning + if: steps.changed.outputs.dirs != '' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + files="$(git diff --name-only "$BASE_SHA" "$HEAD_SHA" -- '*.tf' || true)" + [ -z "$files" ] && exit 0 + # shellcheck disable=SC2086 + ./scripts/check-version-pinning.sh $files diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 3a7551db7..246c08c4d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -23,6 +23,13 @@ repos: require_serial: true exclude: '\.terraform/|\.terragrunt-cache/' + - id: check-version-pinning + name: check version pinning + entry: scripts/check-version-pinning.sh + language: script + files: '\.tf$' + exclude: '\.terraform/|\.terragrunt-cache/' + - id: block-superpowers-files name: block superpowers files entry: >- diff --git a/README.md b/README.md index eea697ad0..6ed387a9b 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ Reference any module via its Git source, pinned to a release tag: ```hcl module "vpc" { - source = "git::https://github.com/nullplatform/tofu-modules.git//infrastructure/aws/vpc?ref=v6.11.0" + source = "git::https://github.com/nullplatform/tofu-modules.git//infrastructure/aws/vpc?ref=v6.19.1" # module inputs ... } @@ -46,6 +46,11 @@ tofu apply See the [latest releases](https://github.com/nullplatform/tofu-modules/releases) for available versions. +Several modules require you to pin a chart version, an image tag or a git ref, with no +default. **[VERSIONS.md](VERSIONS.md) lists every one of them with its current value and a +ready-to-paste block** — read it before your first apply rather than hunting the numbers down +one variable at a time. + ## Versioning Releases follow [Semantic Versioning](https://semver.org/) and are automated via [release-please](https://github.com/googleapis/release-please). See [CHANGELOG.md](CHANGELOG.md) for the full release history. diff --git a/VERSIONS.md b/VERSIONS.md new file mode 100644 index 000000000..b2352a36a --- /dev/null +++ b/VERSIONS.md @@ -0,0 +1,134 @@ +# Pinned versions + +Every version these modules deploy — Helm charts, container images, and the git refs the agent +clones — is listed here with the value to pin. + +For the version of *these modules*, see the +[releases](https://github.com/nullplatform/tofu-modules/releases). + +## Why it matters + +`latest` and branch names resolve at deploy time, not at apply time. A pod restart can pull a +different build with no change on your side and no diff to review. Every default below names +a specific release, so an upgrade is something someone decides. + +## What to pin + +Verified 2026-08-27. + +| Component | Current | Variable | Module | +| --- | --- | --- | --- | +| `nullplatform-base` chart | `2.44.0` | `nullplatform_base_helm_version` | `nullplatform/base` | +| `nullplatform-agent` chart | `2.37.0` | `nullplatform_agent_helm_version` | `nullplatform/agent` | +| `cert-manager` chart | `v1.21.1` | `cert_manager_version` | `infrastructure/commons/cert_manager` | +| `prometheus` chart | `29.27.0` | `prometheus_version` | `infrastructure/commons/prometheus` | +| `k8s-logs-controller` | `1.6.0` | `logging_controller_image_tag` | `nullplatform/base` | +| `controlplane-agent` | `0.9.2` | `control_plane_agent_image_tag` | `nullplatform/base` | +| `k8s-traffic-manager` | `1.8.0` | `agent_traffic_manager_tag` | `nullplatform/agent` | +| traffic manager (provider config) | `1.8.0` | `traffic_manager_version` | `container_orchestration/eks` | +| `scopes` repository | `v1.15.1` | `agent_repos_scope` | `nullplatform/agent` | + +**Read your cluster before copying these.** The rule is to pin what you are already running, +so the change stays functionally inert. Four of these were previously unpinnable and resolved +at deploy time, so what you run may not match the table: `cert_manager_version`, +`prometheus_version`, `logging_controller_image_tag`, and `traffic_manager_version` on eks. + +## Ready to paste + +```hcl +module "base" { + nullplatform_base_helm_version = "2.44.0" + logging_controller_image_tag = "1.6.0" + control_plane_agent_image_tag = "0.9.2" +} + +module "agent" { + nullplatform_agent_helm_version = "2.37.0" + image_tag = "0.9.2" + agent_repos_scope_tag = "v1.15.1" + agent_traffic_manager_tag = "1.8.0" + + agent_repos_extra = [ + "https://github.com/nullplatform/scopes-lambda.git#v0.3.1", + "https://github.com/nullplatform/scopes-static-files.git#v0.4.0", + ] +} + +# eks, aks and gke all take this +module "container_orchestration" { + traffic_manager_version = "1.8.0" +} + +module "cert_manager" { + cert_manager_version = "v1.21.1" +} + +module "prometheus" { + prometheus_version = "29.27.0" +} + +module "service_definition" { + # No value listed: repository_org and repository_name are configurable, so which spec + # repository you read is your choice and so is its ref. + repository_branch = "..." +} +``` + +To find what an install is actually running before changing anything: + +```bash +helm -n nullplatform-tools get values nullplatform-base +helm -n list -o json | jq -r '.[] | "\(.name)\t\(.chart)"' +kubectl -n nullplatform-tools get deploy \ + -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.spec.template.spec.containers[*].image}{"\n"}{end}' +``` + +The traffic manager image is assembled from `agent_traffic_manager_tag` and published to the +agent as `TRAFFIC_CONTAINER_IMAGE`. `extra_envs` still takes precedence over it, so a digest +or a mirrored registry path can be passed the way it was before the tag was exposed. + +## Caveats + +**The scopes ref steps back.** `agent_repos_scope` used to point at `scopes.git#main`, and +`main` has moved past `v1.15.1`. Pinning the tag is deliberate — it is the ref named in the +migration request — but it is not the same tree the branch tip pointed at. + +**cert-manager and prometheus were not pinnable at all.** `cert_manager_version` existed but +was never wired to its `helm_release`, and `prometheus` had no version argument, so both +tracked whatever their chart repository served. + +**Not everything is covered yet.** The scopes and service-spec repositories are read through +eleven other paths, in `scope_definition`, `scope_definition_agent_association`, +`parameter_storage_definition` and `service_definition`, and those still default to a moving +branch. Pinning `agent_repos_scope` does not cover them: the agent clones the ref while the +definition modules read the branch. They are listed in +`scripts/version-pinning-baseline.txt` with the reason. + +**A name cannot prove immutability.** The checks below reject `latest`, `main`, `master` and +`HEAD`. A tag called `beta` or a branch called `develop` passes. Nothing distinguishes a +mutable ref from a fixed one by name alone. + +## Keeping this current + +There is no automation that bumps these numbers, on purpose. Bumping a documented version to +whatever is newest would put the drift back in documentation form, and it contradicts the rule +above about pinning what you already run. When a new version ships, someone decides and edits +this table. + +What is automated is the opposite direction: `scripts/check-version-pinning.sh` rejects a *new* +moving default, a repository URL pinned to a branch, or a `helm_release` with no `version`. It +runs in pre-commit and again as a step in the `terraform-lint` workflow, so skipping the local +hook does not skip the check. Deliberately deferred violations live in +`scripts/version-pinning-baseline.txt` with the reason; that file should only ever shrink. + +One trap worth knowing before bumping an image by hand: **`k8s-traffic-manager` publishes a +`v2.0.2` built 2026-02-09 while `1.8.0` was built 2026-07-29**, and `k8s-logs-controller` a +`v2.0.1` from that same February against a `1.6.0` from August. The higher version number is +the older build, from a line that was not continued. Compare build dates, not version numbers: + +```bash +tag=1.8.0; repo=nullplatform/k8s-traffic-manager +t=$(curl -s "https://public.ecr.aws/token/?scope=repository:$repo:pull" | jq -r .token) +curl -s -H "Authorization: Bearer $t" "https://public.ecr.aws/v2/$repo/tags/list" | jq -r '.tags[]' +``` + diff --git a/scripts/check-version-pinning.sh b/scripts/check-version-pinning.sh new file mode 100755 index 000000000..4c77edce9 --- /dev/null +++ b/scripts/check-version-pinning.sh @@ -0,0 +1,81 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Rejects a new moving version reference. See VERSIONS.md. +# +# Usage: check-version-pinning.sh ... + +BASELINE="$(dirname "$0")/version-pinning-baseline.txt" +FAILED=0 + +baselined() { + [ -f "$BASELINE" ] || return 1 + grep -qxF "$1" <(grep -v '^[[:space:]]*#' "$BASELINE" | grep -v '^[[:space:]]*$') 2>/dev/null +} + +for file in "$@"; do + [ -f "$file" ] || continue + case "$file" in + */.terraform/*|*/.terragrunt-cache/*) continue ;; + *.tf) ;; + *) continue ;; + esac + + while IFS='|' read -r key message; do + [ -n "$key" ] || continue + if baselined "$key"; then + continue + fi + printf ' %s\n %s\n' "$key" "$message" >&2 + FAILED=1 + done < <( + awk -v F="$file" ' + function emit(key, msg) { print F ":" key "|" msg } + + /^variable[[:space:]]+"/ { + match($0, /"[^"]+"/) + vname = substr($0, RSTART + 1, RLENGTH - 2) + invar = 1; next + } + invar && /^}/ { invar = 0; vname = ""; next } + + invar && $0 ~ /^[[:space:]]*default[[:space:]]*=[[:space:]]*"(latest|main|master|HEAD)"[[:space:]]*$/ { + emit(vname, "defaults to a moving reference. Pin it, or drop the default so the caller has to pin it.") + next + } + invar && $0 ~ /^[[:space:]]*default[[:space:]]*=[[:space:]]*".*#(latest|main|master|HEAD)"[[:space:]]*$/ { + emit(vname, "default pins a git ref to a moving branch. Expose the ref as its own variable without a default.") + next + } + invar && $0 ~ /^[[:space:]]*default[[:space:]]*=[[:space:]]*".*\/refs\/heads"?[[:space:]]*$/ { + emit(vname, "default hardcodes refs/heads, so a caller cannot pin to a tag without also rewriting the URL. Expose the ref namespace as a variable.") + next + } + + /^resource[[:space:]]+"helm_release"[[:space:]]+"/ { + n = $0 + sub(/^resource[[:space:]]+"helm_release"[[:space:]]+"/, "", n) + sub(/".*$/, "", n) + rname = n; inres = 1; hasver = 0; next + } + inres && $0 ~ /^[[:space:]]+version[[:space:]]*=/ { hasver = 1; next } + inres && /^}/ { + if (!hasver) { + emit("helm_release." rname, "has no version argument, so Helm resolves whatever the chart repository serves at apply time with no diff to review. Add version = var..") + } + inres = 0; next + } + ' "$file" + ) +done + +if [ "$FAILED" -ne 0 ]; then + { + echo "" + echo "Version pinning check failed." + echo "Fix the finding, or -- if the fix is deliberately deferred -- add the key to" + echo "scripts/version-pinning-baseline.txt with a comment saying why and where it is tracked." + } >&2 +fi + +exit $FAILED diff --git a/scripts/version-pinning-baseline.txt b/scripts/version-pinning-baseline.txt new file mode 100644 index 000000000..92cb25f64 --- /dev/null +++ b/scripts/version-pinning-baseline.txt @@ -0,0 +1,26 @@ +# Deliberately deferred version-pinning violations. Each group says why. +# This file should only ever shrink. +# +# Format: : + +# The scopes and service-spec repositories, read through ten paths across three +# modules, all defaulting to a moving branch. Fixable without a breaking change; +# deferred for its own review. +nullplatform/parameter_storage_definition/variables.tf:repository_parameter_storage_spec +nullplatform/parameter_storage_definition/variables.tf:repository_parameter_storage_spec_branch +nullplatform/scope_definition/variables.tf:repository_action_templates +nullplatform/scope_definition/variables.tf:repository_action_templates_branch +nullplatform/scope_definition/variables.tf:repository_scope_template +nullplatform/scope_definition/variables.tf:repository_scope_template_branch +nullplatform/scope_definition/variables.tf:repository_service_spec +nullplatform/scope_definition/variables.tf:repository_service_spec_branch +nullplatform/scope_definition_agent_association/variables.tf:repository_notification_channel +nullplatform/scope_definition_agent_association/variables.tf:repository_notification_channel_branch + +# Fixed by the "require an explicit version" PR, whose base does not contain this +# file. Remove these when it merges. +infrastructure/commons/cert_manager/main.tf:helm_release.cert_manager +infrastructure/commons/prometheus/main.tf:helm_release.prometheus +nullplatform/agent/variables.tf:agent_repos_scope +nullplatform/container_orchestration/eks/variables.tf:traffic_manager_version +nullplatform/service_definition/variables.tf:repository_branch