From 9925210fdef9539f3fcbf6c1533d5ce112c4dc3d Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Tue, 25 Aug 2026 14:05:09 -0700 Subject: [PATCH 1/6] feat(ci): open a pull request bumping stack pins when a chart releases Nine of the twenty-four chart versions pinned in the self-managed stack are behind what has been released. The edit is one line each, so the cost is not the work, it is remembering to do it. On release: published for a deploy/helm//v* tag, this resolves the stack releases pinning that chart, rewrites their version, and opens or refreshes a pull request. The released tag carries the version, so there is no newest-version lookup and none of the ordering questions that come with one. Resolution uses only data already declared: the tag gives the chart path, tools/ci/github-release-subprojects.json maps that path to the published chart name, and the helmfile names the chart in one of three forms. The third form was the interesting one. Two releases set chart to a Go template with a default, and the default names the real chart, so those resolve rather than being guessed at or skipped. The failure this is built against is silence: a chart releases, nothing resolves to it, no pin moves, and the run goes green. That is how nvcf-unbound went unpublished. So the resolver enumerates every release in the stack, an unresolvable one is an error rather than a skipped iteration, and a bump refuses to run at all while any release is unreadable, since the one it cannot parse might be the one that pins this chart. Fourteen behavioral cases, and the suite was mutation tested to confirm it bites: skipping unresolved releases, bumping anyway on an unreadable stack, guessing a chart name instead of raising, and rewriting every version line rather than the target block are each caught by the case that names them. The last matters most, since two releases currently sit at the same version and a sloppy substitution would move the wrong one. The tests run in this workflow rather than somewhere it might not reach. Co-authored-by: Balaji Ganesan --- .github/workflows/stack-pin-bump.yml | 141 +++++++++++++++++ tools/ci/stack-pin-resolver | 220 +++++++++++++++++++++++++++ tools/ci/test-stack-pin-resolver | 115 ++++++++++++++ 3 files changed, 476 insertions(+) create mode 100644 .github/workflows/stack-pin-bump.yml create mode 100755 tools/ci/stack-pin-resolver create mode 100755 tools/ci/test-stack-pin-resolver diff --git a/.github/workflows/stack-pin-bump.yml b/.github/workflows/stack-pin-bump.yml new file mode 100644 index 000000000..365f24b32 --- /dev/null +++ b/.github/workflows/stack-pin-bump.yml @@ -0,0 +1,141 @@ +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# When a chart release is published, open a pull request moving the +# self-managed stack's pin to that version. +# +# The released tag carries the version, so there is no "newest version" lookup +# and none of the ordering questions that come with one. A tag of +# deploy/helm/nats/v0.8.0 states the answer. +# +# The failure this is built to avoid is silence. A chart releases, nothing in +# the stack resolves to it, no pin moves, and the run goes green. The resolver +# therefore enumerates every release in the stack and treats one it cannot +# resolve as an error, so a gap shows up as a red run rather than as nothing. + +name: stack pin bump + +on: + release: + types: [published] + # Manual entry point for re-running a release whose bump did not land, and + # for exercising the job without cutting a tag. + workflow_dispatch: + inputs: + tag: + description: Chart release tag, for example deploy/helm/nats/v0.8.0 + required: true + +permissions: + contents: read + +concurrency: + # One bump at a time. Several releases landing together refresh the same + # pull request rather than racing on the same file. + group: stack-pin-bump + cancel-in-progress: false + +jobs: + bump: + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + steps: + - uses: actions/checkout@v4 + + - name: Test the resolver + # The resolver decides which line in the shipped stack gets rewritten, + # so its tests run here rather than somewhere that might not be + # reached. A test that gates nothing is not a test. + run: bash tools/ci/test-stack-pin-resolver + + - name: Select the tag + id: tag + run: | + set -euo pipefail + tag="${{ github.event.inputs.tag || github.event.release.tag_name }}" + echo "tag=${tag}" >> "${GITHUB_OUTPUT}" + # Only chart releases move stack pins. Everything else is a normal + # release and is not this job's business. + case "${tag}" in + deploy/helm/*/v*) echo "applies=true" >> "${GITHUB_OUTPUT}" ;; + *) echo "applies=false" >> "${GITHUB_OUTPUT}" + echo "${tag} is not a chart release; nothing to do" ;; + esac + + - name: Audit the stack + if: steps.tag.outputs.applies == 'true' + # Runs before the edit so an unresolvable release fails the job with a + # name attached, rather than being quietly skipped over. + run: python3 tools/ci/stack-pin-resolver --audit + + - name: Apply the bump + id: bump + if: steps.tag.outputs.applies == 'true' + run: | + set -euo pipefail + python3 tools/ci/stack-pin-resolver --tag "${{ steps.tag.outputs.tag }}" --write + if git diff --quiet; then + echo "changed=false" >> "${GITHUB_OUTPUT}" + echo "stack already pins this version" + else + echo "changed=true" >> "${GITHUB_OUTPUT}" + git --no-pager diff --stat + fi + + - name: Open or refresh the pull request + if: steps.bump.outputs.changed == 'true' + env: + GH_TOKEN: ${{ secrets.NV_GITHUB_TOKEN || github.token }} + TAG: ${{ steps.tag.outputs.tag }} + run: | + set -euo pipefail + branch="chore/stack-pin-bumps" + git config user.name "nvcf-release-bot" + git config user.email "svc-nvcf-release@nvidia.com" + + # A fixed branch, refreshed. Several releases landing close together + # then produce one pull request carrying all of their bumps instead + # of a pile that conflict with each other. + git fetch origin "${branch}" || true + if git rev-parse --verify -q "origin/${branch}" >/dev/null; then + git stash push --quiet + git checkout -B "${branch}" "origin/${branch}" + git stash pop --quiet || true + else + git checkout -B "${branch}" + fi + + git add deploy/stacks/self-managed/helmfile.d + # Separate -m flags rather than an embedded multi-line string: the + # continuation lines of one would have to sit at column zero, which + # ends the YAML block scalar this script lives in. + git commit \ + -m "chore(stack): pin ${TAG#deploy/helm/}" \ + -m "Opened by the stack pin bump workflow on release of ${TAG}." \ + -m "Co-authored-by: Balaji Ganesan " + git push --force-with-lease origin "${branch}" + + body="$(printf '%s\n' \ + "Opened by \`.github/workflows/stack-pin-bump.yml\` when \`${TAG}\` was published." \ + "" \ + "The released tag carries the version, so this is a direct pin update rather than a lookup of the newest published chart." \ + "" \ + "Release notes: ${{ github.server_url }}/${{ github.repository }}/releases/tag/${TAG}" \ + "" \ + "If this pull request sits unmerged, later chart releases add their bumps to the same branch, so merging it applies all of them." \ + "" \ + "Github commit:" \ + "chore(stack): pin ${TAG#deploy/helm/}" \ + "" \ + "Co-authored-by: Balaji Ganesan ")" + + if gh pr view "${branch}" --json number >/dev/null 2>&1; then + gh pr edit "${branch}" --body "${body}" + echo "refreshed the existing pull request" + else + gh pr create --base main --head "${branch}" \ + --title "chore(stack): bump self-managed stack chart pins" \ + --body "${body}" + fi diff --git a/tools/ci/stack-pin-resolver b/tools/ci/stack-pin-resolver new file mode 100755 index 000000000..647e09e9e --- /dev/null +++ b/tools/ci/stack-pin-resolver @@ -0,0 +1,220 @@ +#!/usr/bin/env python3 +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Resolve which self-managed stack pins a released chart, and edit them. + + stack-pin-resolver --audit + stack-pin-resolver --tag deploy/helm/nats/v0.8.0 [--write] + +--audit enumerates every release in the stack and reports the chart each one +pins. It exits non-zero when any release cannot be resolved. + +--tag takes a chart release tag and reports the pins that should move. With +--write it edits them in place. + +Why an audit mode exists at all: the failure this guards against is silence. A +chart releases, nothing in the stack resolves to it, no pin moves, and the run +reports success. That is how nvcf-unbound went unpublished for weeks, and a +bumper that iterates only over what it understands would reproduce it exactly. +So resolution is enumerated over the whole stack and an unresolved release is an +error, not a skipped iteration. + +Resolution has three steps, all from data already declared in the repository: + + released tag deploy/helm//v + -> chart path deploy/helm/ (the tag prefix) + -> chart name tools/ci/github-release-subprojects.json, the service_name of + the entry whose path matches + -> stack pins the helmfile releases naming that chart + +The helmfile names a chart in one of three ways, and the third cannot be +resolved by reading the file: + + explicit chart: nvcf/helm-reval + convention no chart: line, inherits a template of the form + nvcf/helm-nvcf-{{ .Release.Name }} + templated chart: {{ ... }} with anything else inside + +A templated chart line is reported as unresolved rather than guessed at. +""" + +import argparse +import json +import pathlib +import re +import sys + +HELMFILE_DIR = pathlib.Path("deploy/stacks/self-managed/helmfile.d") +RELEASE_METADATA = pathlib.Path("tools/ci/github-release-subprojects.json") + +# A release block starts at "- name:" and runs to the next one. +RELEASE_RE = re.compile(r"^\s+- name:\s*(\S+)(.*?)(?=^\s+- name:|\Z)", re.M | re.S) +CHART_RE = re.compile(r"^\s+chart:\s*(.+?)\s*$", re.M) +VERSION_RE = re.compile(r"^(\s+version:\s*)([0-9][^\s]*)(\s*)$", re.M) +# The shared template that most releases inherit. The inner braces are escaped +# in the source because helmfile passes the expression through to helm. +TEMPLATE_CHART_RE = re.compile(r"helm-nvcf-\{\{.*?\.Release\.Name.*?\}\}") +# An override-with-default line names the real chart inside the default: +# chart: {{ $someVar | default "nvcf/helm-nvcf-llm-request-router" | quote }} +# The default is the chart used unless an operator overrides it, so it is the +# one an automated bump should follow. +DEFAULT_CHART_RE = re.compile(r'default\s+"([^"]+)"') + + +class Unresolved(Exception): + pass + + +def chart_name_for_release(release_name, body): + """The chart a stack release pins, or raise Unresolved.""" + m = CHART_RE.search(body) + if not m: + # No chart line: inherits the shared template, which appends the + # release name to a fixed prefix. + return f"helm-nvcf-{release_name}" + value = m.group(1) + if "{{" not in value: + return value.split("/")[-1] + if TEMPLATE_CHART_RE.search(value): + return f"helm-nvcf-{release_name}" + default = DEFAULT_CHART_RE.search(value) + if default: + return default.group(1).split("/")[-1] + raise Unresolved(f"chart line is templated and not a known form: {value}") + + +def load_stack(): + """Every release in the stack: name, file, chart (or the Unresolved reason), + pinned version.""" + out = [] + for path in sorted(HELMFILE_DIR.glob("*.yaml.gotmpl")): + text = path.read_text() + for m in RELEASE_RE.finditer(text): + name, body = m.group(1), m.group(2) + ver = VERSION_RE.search(body) + if not ver: + # Not a pin. The shared templates block and the repositories + # block both match the release shape but carry no version. + continue + try: + chart, reason = chart_name_for_release(name, body), None + except Unresolved as exc: + chart, reason = None, str(exc) + out.append( + { + "release": name, + "file": path.name, + "chart": chart, + "unresolved": reason, + "version": ver.group(2) if ver else None, + } + ) + return out + + +def chart_name_for_path(chart_path): + """The published chart name for a chart directory, from release metadata.""" + meta = json.loads(RELEASE_METADATA.read_text()) + for entry in meta.get("services", []): + if entry.get("path") == chart_path: + name = entry.get("service_name") + if not name: + raise Unresolved(f"{chart_path} has no service_name in release metadata") + return name + raise Unresolved(f"no release-metadata entry with path {chart_path}") + + +def parse_tag(tag): + """deploy/helm//v -> (chart path, version).""" + m = re.fullmatch(r"(deploy/helm/.+)/v(.+)", tag) + if not m: + raise Unresolved(f"not a chart release tag: {tag}") + return m.group(1), m.group(2) + + +def audit(): + releases = load_stack() + bad = [r for r in releases if r["unresolved"]] + for r in releases: + state = r["unresolved"] or f"-> {r['chart']}" + print(f"{r['release']:28} {str(r['version'] or '-'):20} {state}") + print(f"\n{len(releases)} releases, {len(bad)} unresolved") + if bad: + print("\nUnresolved releases cannot receive an automated bump:", file=sys.stderr) + for r in bad: + print(f" {r['release']} ({r['file']}): {r['unresolved']}", file=sys.stderr) + return 1 + return 0 + + +def bump(tag, write): + chart_path, version = parse_tag(tag) + chart = chart_name_for_path(chart_path) + + releases = load_stack() + unresolved = [r for r in releases if r["unresolved"]] + if unresolved: + # Refuse to act on a partially understood stack. A release we cannot + # read might be the one that pins this chart. + print("refusing to bump: the stack has unresolved releases", file=sys.stderr) + for r in unresolved: + print(f" {r['release']} ({r['file']}): {r['unresolved']}", file=sys.stderr) + return 1 + + targets = [r for r in releases if r["chart"] == chart] + if not targets: + print(f"no stack release pins {chart} (from {tag})", file=sys.stderr) + return 1 + + changed = 0 + for r in targets: + if r["version"] == version: + print(f"{r['release']}: already {version}") + continue + print(f"{r['release']}: {r['version']} -> {version}") + changed += 1 + if write: + path = HELMFILE_DIR / r["file"] + text = path.read_text() + block = RELEASE_RE.search(text) and None + # Rewrite only the version line inside this release's own block. + def rewrite(match): + if match.group(1) != r["release"]: + return match.group(0) + body = VERSION_RE.sub( + lambda v: f"{v.group(1)}{version}{v.group(3)}", match.group(2), count=1 + ) + return f"{match.group(0)[:match.group(0).index(match.group(2))]}{body}" + + path.write_text(RELEASE_RE.sub(rewrite, text)) + if changed == 0: + print("nothing to change") + return 0 + + +def main(): + ap = argparse.ArgumentParser() + ap.add_argument("--audit", action="store_true") + ap.add_argument("--tag") + ap.add_argument("--write", action="store_true") + ap.add_argument("--root", default=".") + args = ap.parse_args() + + import os + + os.chdir(args.root) + + try: + if args.audit: + return audit() + if args.tag: + return bump(args.tag, args.write) + except Unresolved as exc: + print(f"unresolved: {exc}", file=sys.stderr) + return 1 + ap.print_help() + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tools/ci/test-stack-pin-resolver b/tools/ci/test-stack-pin-resolver new file mode 100755 index 000000000..1f414a706 --- /dev/null +++ b/tools/ci/test-stack-pin-resolver @@ -0,0 +1,115 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Behavioral test for tools/ci/stack-pin-resolver. +# +# The asymmetry that shapes these cases: a wrong "no pin found" costs a missed +# bump somebody notices. A wrong edit silently rewrites the wrong release's +# version, and the stack ships something nobody chose. So the write path is +# tested against a fixture where two releases share a version number, which is +# the input that would expose a sloppy substitution. +set -euo pipefail + +tool="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/stack-pin-resolver" +fail=0 + +root="$(mktemp -d)" +trap 'rm -rf "${root}"' EXIT +mkdir -p "${root}/deploy/stacks/self-managed/helmfile.d" "${root}/tools/ci" + +cat > "${root}/deploy/stacks/self-managed/helmfile.d/01-test.yaml.gotmpl" <<'EOF' +repositories: + - name: nvcf + url: {{ .Values.global.helm.sources.registry }} +templates: + default: &default + chart: nvcf/helm-nvcf-{{`{{ .Release.Name }}`}} +releases: + - name: alpha + version: 1.0.0 + - name: beta + chart: nvcf/helm-explicit-beta + version: 1.0.0 + - name: gamma + chart: {{ $gammaPath | default "nvcf/helm-nvcf-gamma-real" | quote }} + version: 2.5.0 + - name: delta + chart: {{ $mystery | someFilter }} + version: 3.0.0 +EOF + +cat > "${root}/tools/ci/github-release-subprojects.json" <<'EOF' +{"version":1,"services":[ + {"id":"alpha-helm","path":"deploy/helm/alpha","service_name":"helm-nvcf-alpha"}, + {"id":"beta-helm","path":"deploy/helm/beta","service_name":"helm-explicit-beta"}, + {"id":"gamma-helm","path":"deploy/helm/gamma","service_name":"helm-nvcf-gamma-real"}, + {"id":"orphan-helm","path":"deploy/helm/orphan","service_name":"helm-nvcf-orphan"}, + {"id":"noname-helm","path":"deploy/helm/noname"} +]} +EOF + +ck() { # desc, expected-exit, args... + local d="$1" want="$2"; shift 2 + set +e; out="$(python3 "${tool}" --root "${root}" "$@" 2>&1)"; got=$?; set -e + if [ "${got}" = "${want}" ]; then printf 'ok %s\n' "${d}" + else printf 'FAIL %s: want exit %s, got %s\n%s\n' "${d}" "${want}" "${got}" "${out}"; fail=1; fi +} + +# Audit fails while an unresolvable release is present. +ck "audit fails on an unresolvable chart line" 1 --audit +# ... and names it. +# Capture first: the tool exits 1 by design here, and with pipefail a pipeline +# would inherit that and mask a successful grep. +set +e; audit_out="$(python3 "${tool}" --root "${root}" --audit 2>&1)"; set -e +case "${audit_out}" in + *delta*) echo "ok audit names the unresolvable release" ;; + *) echo "FAIL audit did not name delta"; fail=1 ;; +esac + +# Refuse to bump at all while the stack is partly unreadable: the release we +# cannot parse might be the one pinning this chart. +ck "refuses to bump while any release is unresolved" 1 --tag deploy/helm/alpha/v1.2.0 + +# Remove the unresolvable release and the rest must resolve. +python3 - "${root}" <<'PY' +import pathlib,sys,re +p=pathlib.Path(sys.argv[1])/"deploy/stacks/self-managed/helmfile.d/01-test.yaml.gotmpl" +t=p.read_text() +p.write_text(t[:t.index(" - name: delta")]) +PY +ck "audit passes once every release resolves" 0 --audit + +# Each chart-line form resolves to the right chart. +set +e; forms_out="$(python3 "${tool}" --root "${root}" --audit 2>&1)"; set -e +for pair in "alpha:convention" "beta:explicit" "gamma:default-in-template"; do + n=${pair%%:*}; form=${pair##*:} + if printf '%s\n' "${forms_out}" | grep -qE "^${n} .*-> "; then + printf 'ok %s form resolves (%s)\n' "${form}" "${n}" + else + printf 'FAIL %s form did not resolve\n' "${form}"; fail=1 + fi +done + +# Errors that must not be silent. +ck "unknown chart path is an error" 1 --tag deploy/helm/nosuch/v1.0.0 +ck "entry without service_name is an error" 1 --tag deploy/helm/noname/v1.0.0 +ck "chart nothing pins is an error" 1 --tag deploy/helm/orphan/v1.0.0 +ck "malformed tag is an error" 1 --tag not-a-tag + +# The write path. alpha and beta both sit at 1.0.0: bumping alpha must leave +# beta untouched. +python3 "${tool}" --root "${root}" --tag deploy/helm/alpha/v9.9.9 --write >/dev/null +a=$(grep -A2 "name: alpha" "${root}/deploy/stacks/self-managed/helmfile.d/01-test.yaml.gotmpl" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+') +b=$(grep -A3 "name: beta" "${root}/deploy/stacks/self-managed/helmfile.d/01-test.yaml.gotmpl" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+') +[ "$a" = "9.9.9" ] && echo "ok write updates the target release" || { echo "FAIL alpha=$a want 9.9.9"; fail=1; } +[ "$b" = "1.0.0" ] && echo "ok write leaves a same-version sibling alone" || { echo "FAIL beta=$b want 1.0.0"; fail=1; } + +# Idempotence: a second run reports nothing to change. +set +e; idem_out="$(python3 "${tool}" --root "${root}" --tag deploy/helm/alpha/v9.9.9 2>&1)"; set -e +case "${idem_out}" in + *"already 9.9.9"*) echo "ok re-running an applied bump is a no-op" ;; + *) echo "FAIL not idempotent"; fail=1 ;; +esac + +[ "${fail}" -eq 0 ] && echo "stack-pin-resolver: all checks passed" || { echo "stack-pin-resolver: FAILED" >&2; exit 1; } From 0f0ad375141597d8dad6e102ef3bbe0e0ac5a4a6 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Tue, 25 Aug 2026 20:30:07 -0700 Subject: [PATCH 2/6] fix(ci): scope the stack bump's change detection to what it stages Two problems found while wiring the same shape for chart bumps. git diff --quiet was repo-wide while the commit that follows stages only deploy/stacks/self-managed/helmfile.d. Any unrelated modification in the workspace sets changed=true, and the commit then aborts with nothing staged. Checkout took the release event's default of the tagged commit, but the pull request targets the default branch, so pinning against the tag's tree would carry whatever the stack looked like then onto a branch cut from today's main. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/stack-pin-bump.yml | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/.github/workflows/stack-pin-bump.yml b/.github/workflows/stack-pin-bump.yml index 365f24b32..754cbda41 100644 --- a/.github/workflows/stack-pin-bump.yml +++ b/.github/workflows/stack-pin-bump.yml @@ -43,6 +43,12 @@ jobs: pull-requests: write steps: - uses: actions/checkout@v4 + with: + # The default for a release event is the tagged commit, but the pull + # request targets the default branch. Pinning against the tag's tree + # would carry whatever the stack looked like then onto a branch cut + # from today's main. + ref: ${{ github.event.repository.default_branch }} - name: Test the resolver # The resolver decides which line in the shipped stack gets rewritten, @@ -76,12 +82,15 @@ jobs: run: | set -euo pipefail python3 tools/ci/stack-pin-resolver --tag "${{ steps.tag.outputs.tag }}" --write - if git diff --quiet; then + # Scoped to the same paths the commit below stages. Repo-wide, any + # unrelated modification in the workspace would set changed=true and + # the commit would then abort with nothing staged. + if git diff --quiet -- deploy/stacks/self-managed/helmfile.d; then echo "changed=false" >> "${GITHUB_OUTPUT}" echo "stack already pins this version" else echo "changed=true" >> "${GITHUB_OUTPUT}" - git --no-pager diff --stat + git --no-pager diff --stat -- deploy/stacks/self-managed/helmfile.d fi - name: Open or refresh the pull request From a58242ee81a9e349d8b8a4541c77ff1b8e9fe140 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Tue, 25 Aug 2026 21:39:54 -0700 Subject: [PATCH 3/6] refactor(ci): rewrite the stack pin resolver in Go tools/AGENTS.md asks for Go over Python for non-trivial repo tooling: structured parsing, file transforms, and logic that benefits from unit tests. Some CI environments here do not guarantee Python. This is all three, so it should not have been Python to begin with. The port is not mechanical in one place. The Python split helmfiles into release blocks with a single regex ending in a lookahead, "up to the next release or end of file", and Go's regexp engine has no lookahead. Blocks are now found line by line, which is closer to the file anyway: it also yields the line number of each pin, so the rewrite replaces one exact line rather than reconstructing a block around it, and refuses if that line is no longer a version pin when it goes to write. Behaviour is unchanged: the checked-in stack audits to the same 24 releases, 0 unresolved. tools/ci/stack-pin-resolver stays as the entrypoint. It builds the binary rather than using `go run`, for the repository root (`go run -C` leaves the process in the tool's own directory, where it can find neither the helmfiles nor the release metadata) and for the exit status, which `go run` does not propagate: it prints "exit status N" and exits 1. setup-go derives its version from tools/go-toolchain/go.mod, since tools/ci/check-go-version fails any workflow that pins a literal. Sixteen tests, mutation tested. Eleven mutants die, including an unresolved release that no longer fails the audit, a bump that proceeds past one, an unknown template form guessed at by convention, versionless blocks counted as pins, a rewrite keyed on the version value rather than the release, and a chart nobody pins reported as success. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/stack-pin-bump.yml | 12 +- tools/ci/stack-pin-resolver | 250 +++---------------- tools/ci/test-stack-pin-resolver | 115 --------- tools/stack-pin-resolver/.gitignore | 2 + tools/stack-pin-resolver/go.mod | 3 + tools/stack-pin-resolver/main.go | 179 ++++++++++++++ tools/stack-pin-resolver/main_test.go | 330 ++++++++++++++++++++++++++ tools/stack-pin-resolver/metadata.go | 42 ++++ tools/stack-pin-resolver/stack.go | 193 +++++++++++++++ 9 files changed, 790 insertions(+), 336 deletions(-) delete mode 100755 tools/ci/test-stack-pin-resolver create mode 100644 tools/stack-pin-resolver/.gitignore create mode 100644 tools/stack-pin-resolver/go.mod create mode 100644 tools/stack-pin-resolver/main.go create mode 100644 tools/stack-pin-resolver/main_test.go create mode 100644 tools/stack-pin-resolver/metadata.go create mode 100644 tools/stack-pin-resolver/stack.go diff --git a/.github/workflows/stack-pin-bump.yml b/.github/workflows/stack-pin-bump.yml index 754cbda41..90df4bf33 100644 --- a/.github/workflows/stack-pin-bump.yml +++ b/.github/workflows/stack-pin-bump.yml @@ -50,11 +50,17 @@ jobs: # from today's main. ref: ${{ github.event.repository.default_branch }} + - uses: actions/setup-go@v5 + with: + # Derived from the anchor, never a literal: tools/ci/check-go-version + # fails any workflow that pins one. + go-version-file: tools/go-toolchain/go.mod + - name: Test the resolver # The resolver decides which line in the shipped stack gets rewritten, # so its tests run here rather than somewhere that might not be # reached. A test that gates nothing is not a test. - run: bash tools/ci/test-stack-pin-resolver + run: go test -C tools/stack-pin-resolver ./... - name: Select the tag id: tag @@ -74,14 +80,14 @@ jobs: if: steps.tag.outputs.applies == 'true' # Runs before the edit so an unresolvable release fails the job with a # name attached, rather than being quietly skipped over. - run: python3 tools/ci/stack-pin-resolver --audit + run: tools/ci/stack-pin-resolver --audit - name: Apply the bump id: bump if: steps.tag.outputs.applies == 'true' run: | set -euo pipefail - python3 tools/ci/stack-pin-resolver --tag "${{ steps.tag.outputs.tag }}" --write + tools/ci/stack-pin-resolver --tag "${{ steps.tag.outputs.tag }}" --write # Scoped to the same paths the commit below stages. Repo-wide, any # unrelated modification in the workspace would set changed=true and # the commit would then abort with nothing staged. diff --git a/tools/ci/stack-pin-resolver b/tools/ci/stack-pin-resolver index 647e09e9e..e747d01f8 100755 --- a/tools/ci/stack-pin-resolver +++ b/tools/ci/stack-pin-resolver @@ -1,220 +1,34 @@ -#!/usr/bin/env python3 +#!/usr/bin/env bash # SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -"""Resolve which self-managed stack pins a released chart, and edit them. - - stack-pin-resolver --audit - stack-pin-resolver --tag deploy/helm/nats/v0.8.0 [--write] - ---audit enumerates every release in the stack and reports the chart each one -pins. It exits non-zero when any release cannot be resolved. - ---tag takes a chart release tag and reports the pins that should move. With ---write it edits them in place. - -Why an audit mode exists at all: the failure this guards against is silence. A -chart releases, nothing in the stack resolves to it, no pin moves, and the run -reports success. That is how nvcf-unbound went unpublished for weeks, and a -bumper that iterates only over what it understands would reproduce it exactly. -So resolution is enumerated over the whole stack and an unresolved release is an -error, not a skipped iteration. - -Resolution has three steps, all from data already declared in the repository: - - released tag deploy/helm//v - -> chart path deploy/helm/ (the tag prefix) - -> chart name tools/ci/github-release-subprojects.json, the service_name of - the entry whose path matches - -> stack pins the helmfile releases naming that chart - -The helmfile names a chart in one of three ways, and the third cannot be -resolved by reading the file: - - explicit chart: nvcf/helm-reval - convention no chart: line, inherits a template of the form - nvcf/helm-nvcf-{{ .Release.Name }} - templated chart: {{ ... }} with anything else inside - -A templated chart line is reported as unresolved rather than guessed at. -""" - -import argparse -import json -import pathlib -import re -import sys - -HELMFILE_DIR = pathlib.Path("deploy/stacks/self-managed/helmfile.d") -RELEASE_METADATA = pathlib.Path("tools/ci/github-release-subprojects.json") - -# A release block starts at "- name:" and runs to the next one. -RELEASE_RE = re.compile(r"^\s+- name:\s*(\S+)(.*?)(?=^\s+- name:|\Z)", re.M | re.S) -CHART_RE = re.compile(r"^\s+chart:\s*(.+?)\s*$", re.M) -VERSION_RE = re.compile(r"^(\s+version:\s*)([0-9][^\s]*)(\s*)$", re.M) -# The shared template that most releases inherit. The inner braces are escaped -# in the source because helmfile passes the expression through to helm. -TEMPLATE_CHART_RE = re.compile(r"helm-nvcf-\{\{.*?\.Release\.Name.*?\}\}") -# An override-with-default line names the real chart inside the default: -# chart: {{ $someVar | default "nvcf/helm-nvcf-llm-request-router" | quote }} -# The default is the chart used unless an operator overrides it, so it is the -# one an automated bump should follow. -DEFAULT_CHART_RE = re.compile(r'default\s+"([^"]+)"') - - -class Unresolved(Exception): - pass - - -def chart_name_for_release(release_name, body): - """The chart a stack release pins, or raise Unresolved.""" - m = CHART_RE.search(body) - if not m: - # No chart line: inherits the shared template, which appends the - # release name to a fixed prefix. - return f"helm-nvcf-{release_name}" - value = m.group(1) - if "{{" not in value: - return value.split("/")[-1] - if TEMPLATE_CHART_RE.search(value): - return f"helm-nvcf-{release_name}" - default = DEFAULT_CHART_RE.search(value) - if default: - return default.group(1).split("/")[-1] - raise Unresolved(f"chart line is templated and not a known form: {value}") - - -def load_stack(): - """Every release in the stack: name, file, chart (or the Unresolved reason), - pinned version.""" - out = [] - for path in sorted(HELMFILE_DIR.glob("*.yaml.gotmpl")): - text = path.read_text() - for m in RELEASE_RE.finditer(text): - name, body = m.group(1), m.group(2) - ver = VERSION_RE.search(body) - if not ver: - # Not a pin. The shared templates block and the repositories - # block both match the release shape but carry no version. - continue - try: - chart, reason = chart_name_for_release(name, body), None - except Unresolved as exc: - chart, reason = None, str(exc) - out.append( - { - "release": name, - "file": path.name, - "chart": chart, - "unresolved": reason, - "version": ver.group(2) if ver else None, - } - ) - return out - - -def chart_name_for_path(chart_path): - """The published chart name for a chart directory, from release metadata.""" - meta = json.loads(RELEASE_METADATA.read_text()) - for entry in meta.get("services", []): - if entry.get("path") == chart_path: - name = entry.get("service_name") - if not name: - raise Unresolved(f"{chart_path} has no service_name in release metadata") - return name - raise Unresolved(f"no release-metadata entry with path {chart_path}") - - -def parse_tag(tag): - """deploy/helm//v -> (chart path, version).""" - m = re.fullmatch(r"(deploy/helm/.+)/v(.+)", tag) - if not m: - raise Unresolved(f"not a chart release tag: {tag}") - return m.group(1), m.group(2) - - -def audit(): - releases = load_stack() - bad = [r for r in releases if r["unresolved"]] - for r in releases: - state = r["unresolved"] or f"-> {r['chart']}" - print(f"{r['release']:28} {str(r['version'] or '-'):20} {state}") - print(f"\n{len(releases)} releases, {len(bad)} unresolved") - if bad: - print("\nUnresolved releases cannot receive an automated bump:", file=sys.stderr) - for r in bad: - print(f" {r['release']} ({r['file']}): {r['unresolved']}", file=sys.stderr) - return 1 - return 0 - - -def bump(tag, write): - chart_path, version = parse_tag(tag) - chart = chart_name_for_path(chart_path) - - releases = load_stack() - unresolved = [r for r in releases if r["unresolved"]] - if unresolved: - # Refuse to act on a partially understood stack. A release we cannot - # read might be the one that pins this chart. - print("refusing to bump: the stack has unresolved releases", file=sys.stderr) - for r in unresolved: - print(f" {r['release']} ({r['file']}): {r['unresolved']}", file=sys.stderr) - return 1 - - targets = [r for r in releases if r["chart"] == chart] - if not targets: - print(f"no stack release pins {chart} (from {tag})", file=sys.stderr) - return 1 - - changed = 0 - for r in targets: - if r["version"] == version: - print(f"{r['release']}: already {version}") - continue - print(f"{r['release']}: {r['version']} -> {version}") - changed += 1 - if write: - path = HELMFILE_DIR / r["file"] - text = path.read_text() - block = RELEASE_RE.search(text) and None - # Rewrite only the version line inside this release's own block. - def rewrite(match): - if match.group(1) != r["release"]: - return match.group(0) - body = VERSION_RE.sub( - lambda v: f"{v.group(1)}{version}{v.group(3)}", match.group(2), count=1 - ) - return f"{match.group(0)[:match.group(0).index(match.group(2))]}{body}" - - path.write_text(RELEASE_RE.sub(rewrite, text)) - if changed == 0: - print("nothing to change") - return 0 - - -def main(): - ap = argparse.ArgumentParser() - ap.add_argument("--audit", action="store_true") - ap.add_argument("--tag") - ap.add_argument("--write", action="store_true") - ap.add_argument("--root", default=".") - args = ap.parse_args() - - import os - - os.chdir(args.root) - - try: - if args.audit: - return audit() - if args.tag: - return bump(args.tag, args.write) - except Unresolved as exc: - print(f"unresolved: {exc}", file=sys.stderr) - return 1 - ap.print_help() - return 1 - - -if __name__ == "__main__": - raise SystemExit(main()) +# +# Stable CI entrypoint for the Go tool in tools/stack-pin-resolver. +# +# The wrapper exists for two reasons. +# +# The repository root. `go run -C ` leaves the process running with that +# directory as its working directory, so the tool cannot find the helmfiles or +# the release metadata on its own. Resolving the root from this script's own +# location means callers do not have to pass it. +# +# The exit code. `go run` does NOT propagate the program's status: it prints +# "exit status N" and exits 1, collapsing every non-zero code into one. This +# tool only uses 0 and 1 today, so nothing is lost yet, but building the binary +# keeps that from becoming a trap the first time a distinct code is added. +# +# Run the tests with: go test -C tools/stack-pin-resolver ./... +set -euo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +bin_dir="$(mktemp -d)" +trap 'rm -rf "${bin_dir}"' EXIT + +go build -C "${repo_root}/tools/stack-pin-resolver" -o "${bin_dir}/stack-pin-resolver" . + +# Not exec, so the trap above still runs, and not under errexit, so the exit +# code reaches the caller rather than aborting the shell first. +set +e +"${bin_dir}/stack-pin-resolver" --root "${repo_root}" "$@" +status=$? +set -e +exit "${status}" diff --git a/tools/ci/test-stack-pin-resolver b/tools/ci/test-stack-pin-resolver deleted file mode 100755 index 1f414a706..000000000 --- a/tools/ci/test-stack-pin-resolver +++ /dev/null @@ -1,115 +0,0 @@ -#!/usr/bin/env bash -# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. -# SPDX-License-Identifier: Apache-2.0 -# -# Behavioral test for tools/ci/stack-pin-resolver. -# -# The asymmetry that shapes these cases: a wrong "no pin found" costs a missed -# bump somebody notices. A wrong edit silently rewrites the wrong release's -# version, and the stack ships something nobody chose. So the write path is -# tested against a fixture where two releases share a version number, which is -# the input that would expose a sloppy substitution. -set -euo pipefail - -tool="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/stack-pin-resolver" -fail=0 - -root="$(mktemp -d)" -trap 'rm -rf "${root}"' EXIT -mkdir -p "${root}/deploy/stacks/self-managed/helmfile.d" "${root}/tools/ci" - -cat > "${root}/deploy/stacks/self-managed/helmfile.d/01-test.yaml.gotmpl" <<'EOF' -repositories: - - name: nvcf - url: {{ .Values.global.helm.sources.registry }} -templates: - default: &default - chart: nvcf/helm-nvcf-{{`{{ .Release.Name }}`}} -releases: - - name: alpha - version: 1.0.0 - - name: beta - chart: nvcf/helm-explicit-beta - version: 1.0.0 - - name: gamma - chart: {{ $gammaPath | default "nvcf/helm-nvcf-gamma-real" | quote }} - version: 2.5.0 - - name: delta - chart: {{ $mystery | someFilter }} - version: 3.0.0 -EOF - -cat > "${root}/tools/ci/github-release-subprojects.json" <<'EOF' -{"version":1,"services":[ - {"id":"alpha-helm","path":"deploy/helm/alpha","service_name":"helm-nvcf-alpha"}, - {"id":"beta-helm","path":"deploy/helm/beta","service_name":"helm-explicit-beta"}, - {"id":"gamma-helm","path":"deploy/helm/gamma","service_name":"helm-nvcf-gamma-real"}, - {"id":"orphan-helm","path":"deploy/helm/orphan","service_name":"helm-nvcf-orphan"}, - {"id":"noname-helm","path":"deploy/helm/noname"} -]} -EOF - -ck() { # desc, expected-exit, args... - local d="$1" want="$2"; shift 2 - set +e; out="$(python3 "${tool}" --root "${root}" "$@" 2>&1)"; got=$?; set -e - if [ "${got}" = "${want}" ]; then printf 'ok %s\n' "${d}" - else printf 'FAIL %s: want exit %s, got %s\n%s\n' "${d}" "${want}" "${got}" "${out}"; fail=1; fi -} - -# Audit fails while an unresolvable release is present. -ck "audit fails on an unresolvable chart line" 1 --audit -# ... and names it. -# Capture first: the tool exits 1 by design here, and with pipefail a pipeline -# would inherit that and mask a successful grep. -set +e; audit_out="$(python3 "${tool}" --root "${root}" --audit 2>&1)"; set -e -case "${audit_out}" in - *delta*) echo "ok audit names the unresolvable release" ;; - *) echo "FAIL audit did not name delta"; fail=1 ;; -esac - -# Refuse to bump at all while the stack is partly unreadable: the release we -# cannot parse might be the one pinning this chart. -ck "refuses to bump while any release is unresolved" 1 --tag deploy/helm/alpha/v1.2.0 - -# Remove the unresolvable release and the rest must resolve. -python3 - "${root}" <<'PY' -import pathlib,sys,re -p=pathlib.Path(sys.argv[1])/"deploy/stacks/self-managed/helmfile.d/01-test.yaml.gotmpl" -t=p.read_text() -p.write_text(t[:t.index(" - name: delta")]) -PY -ck "audit passes once every release resolves" 0 --audit - -# Each chart-line form resolves to the right chart. -set +e; forms_out="$(python3 "${tool}" --root "${root}" --audit 2>&1)"; set -e -for pair in "alpha:convention" "beta:explicit" "gamma:default-in-template"; do - n=${pair%%:*}; form=${pair##*:} - if printf '%s\n' "${forms_out}" | grep -qE "^${n} .*-> "; then - printf 'ok %s form resolves (%s)\n' "${form}" "${n}" - else - printf 'FAIL %s form did not resolve\n' "${form}"; fail=1 - fi -done - -# Errors that must not be silent. -ck "unknown chart path is an error" 1 --tag deploy/helm/nosuch/v1.0.0 -ck "entry without service_name is an error" 1 --tag deploy/helm/noname/v1.0.0 -ck "chart nothing pins is an error" 1 --tag deploy/helm/orphan/v1.0.0 -ck "malformed tag is an error" 1 --tag not-a-tag - -# The write path. alpha and beta both sit at 1.0.0: bumping alpha must leave -# beta untouched. -python3 "${tool}" --root "${root}" --tag deploy/helm/alpha/v9.9.9 --write >/dev/null -a=$(grep -A2 "name: alpha" "${root}/deploy/stacks/self-managed/helmfile.d/01-test.yaml.gotmpl" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+') -b=$(grep -A3 "name: beta" "${root}/deploy/stacks/self-managed/helmfile.d/01-test.yaml.gotmpl" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+') -[ "$a" = "9.9.9" ] && echo "ok write updates the target release" || { echo "FAIL alpha=$a want 9.9.9"; fail=1; } -[ "$b" = "1.0.0" ] && echo "ok write leaves a same-version sibling alone" || { echo "FAIL beta=$b want 1.0.0"; fail=1; } - -# Idempotence: a second run reports nothing to change. -set +e; idem_out="$(python3 "${tool}" --root "${root}" --tag deploy/helm/alpha/v9.9.9 2>&1)"; set -e -case "${idem_out}" in - *"already 9.9.9"*) echo "ok re-running an applied bump is a no-op" ;; - *) echo "FAIL not idempotent"; fail=1 ;; -esac - -[ "${fail}" -eq 0 ] && echo "stack-pin-resolver: all checks passed" || { echo "stack-pin-resolver: FAILED" >&2; exit 1; } diff --git a/tools/stack-pin-resolver/.gitignore b/tools/stack-pin-resolver/.gitignore new file mode 100644 index 000000000..a4fe5ed8d --- /dev/null +++ b/tools/stack-pin-resolver/.gitignore @@ -0,0 +1,2 @@ +# go build ./... drops the binary here; it must never be committed. +/stack-pin-resolver diff --git a/tools/stack-pin-resolver/go.mod b/tools/stack-pin-resolver/go.mod new file mode 100644 index 000000000..3110ab78b --- /dev/null +++ b/tools/stack-pin-resolver/go.mod @@ -0,0 +1,3 @@ +module stack-pin-resolver + +go 1.26 diff --git a/tools/stack-pin-resolver/main.go b/tools/stack-pin-resolver/main.go new file mode 100644 index 000000000..36c5aad84 --- /dev/null +++ b/tools/stack-pin-resolver/main.go @@ -0,0 +1,179 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Command stack-pin-resolver resolves which self-managed stack pins a released +// chart, and edits them. +// +// stack-pin-resolver --audit +// stack-pin-resolver --tag deploy/helm/nats/v0.8.0 [--write] +// +// --audit enumerates every release in the stack and reports the chart each one +// pins. It exits non-zero when any release cannot be resolved. +// +// --tag takes a chart release tag and reports the pins that should move. With +// --write it edits them in place. +// +// Why an audit mode exists at all: the failure this guards against is silence. +// A chart releases, nothing in the stack resolves to it, no pin moves, and the +// run reports success. A bumper that iterates only over what it understands +// reproduces that exactly. So resolution is enumerated over the whole stack and +// an unresolved release is an error, not a skipped iteration. +// +// Resolution has three steps, all from data already declared in the repository: +// +// released tag deploy/helm//v +// -> chart path deploy/helm/ (the tag prefix) +// -> chart name tools/ci/github-release-subprojects.json, the service_name of +// the entry whose path matches +// -> stack pins the helmfile releases naming that chart +package main + +import ( + "flag" + "fmt" + "io" + "os" + "regexp" + "strings" +) + +var tagRE = regexp.MustCompile(`^(deploy/helm/.+)/v(.+)$`) + +func main() { + auditMode := flag.Bool("audit", false, "report the chart every stack release pins") + tag := flag.String("tag", "", "chart release tag, for example deploy/helm/nats/v0.8.0") + write := flag.Bool("write", false, "apply the changes rather than only reporting them") + root := flag.String("root", ".", "repository root") + flag.Parse() + + var code int + var err error + switch { + case *auditMode: + code, err = Audit(*root, os.Stdout, os.Stderr) + case *tag != "": + code, err = Bump(*root, *tag, *write, os.Stdout, os.Stderr) + default: + flag.Usage() + os.Exit(1) + } + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + os.Exit(code) +} + +// Audit enumerates the stack and reports what each release pins. +func Audit(root string, out, errOut io.Writer) (int, error) { + releases, err := LoadStack(root) + if err != nil { + return 1, err + } + var bad []Release + for _, r := range releases { + state := "-> " + r.Chart + if r.Unresolved != "" { + state = r.Unresolved + bad = append(bad, r) + } + version := r.Version + if version == "" { + version = "-" + } + fmt.Fprintf(out, "%-28s %-20s %s\n", r.Name, version, state) + } + fmt.Fprintf(out, "\n%d releases, %d unresolved\n", len(releases), len(bad)) + + if len(bad) > 0 { + fmt.Fprintln(errOut, "\nUnresolved releases cannot receive an automated bump:") + for _, r := range bad { + fmt.Fprintf(errOut, " %s (%s): %s\n", r.Name, r.File, r.Unresolved) + } + return 1, nil + } + return 0, nil +} + +// Bump moves every stack pin that names the chart the tag released. +func Bump(root, tag string, write bool, out, errOut io.Writer) (int, error) { + m := tagRE.FindStringSubmatch(tag) + if m == nil { + return 1, fmt.Errorf("not a chart release tag: %s", tag) + } + chartPath, version := m[1], m[2] + + chart, err := ChartNameForPath(root, chartPath) + if err != nil { + return 1, err + } + + releases, err := LoadStack(root) + if err != nil { + return 1, err + } + + var unresolved []Release + for _, r := range releases { + if r.Unresolved != "" { + unresolved = append(unresolved, r) + } + } + if len(unresolved) > 0 { + // Refuse to act on a partially understood stack. A release that cannot + // be read might be the one that pins this chart, and bumping the others + // would look like success. + fmt.Fprintln(errOut, "refusing to bump: the stack has unresolved releases") + for _, r := range unresolved { + fmt.Fprintf(errOut, " %s (%s): %s\n", r.Name, r.File, r.Unresolved) + } + return 1, nil + } + + var targets []Release + for _, r := range releases { + if r.Chart == chart { + targets = append(targets, r) + } + } + if len(targets) == 0 { + return 1, fmt.Errorf("no stack release pins %s (from %s)", chart, tag) + } + + changed := 0 + for _, r := range targets { + if r.Version == version { + fmt.Fprintf(out, "%s: already %s\n", r.Name, version) + continue + } + fmt.Fprintf(out, "%s: %s -> %s\n", r.Name, r.Version, version) + changed++ + if write { + if err := WritePin(root, r, version); err != nil { + return 1, err + } + } + } + if changed == 0 { + fmt.Fprintln(out, "nothing to change") + } + return 0, nil +} + +// ChartNameForPath returns the published chart name for a chart directory. +func ChartNameForPath(root, chartPath string) (string, error) { + meta, err := LoadMetadata(root) + if err != nil { + return "", err + } + for _, e := range meta.Services { + if e.Path != chartPath { + continue + } + if strings.TrimSpace(e.ServiceName) == "" { + return "", fmt.Errorf("%s has no service_name in release metadata", chartPath) + } + return e.ServiceName, nil + } + return "", fmt.Errorf("no release-metadata entry with path %s", chartPath) +} diff --git a/tools/stack-pin-resolver/main_test.go b/tools/stack-pin-resolver/main_test.go new file mode 100644 index 000000000..4af867529 --- /dev/null +++ b/tools/stack-pin-resolver/main_test.go @@ -0,0 +1,330 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +// The failure this tool exists to prevent is silence: a chart releases, nothing +// in the stack resolves to it, no pin moves, and the run goes green. So the +// tests below care most about unresolved releases being loud, and about a +// rewrite landing on exactly one line. + +type stackFixture struct{ root string } + +func newStack(t *testing.T, metadata string, files map[string]string) *stackFixture { + t.Helper() + f := &stackFixture{root: t.TempDir()} + if err := os.MkdirAll(filepath.Join(f.root, "tools", "ci"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(f.root, MetadataPath), []byte(metadata), 0o644); err != nil { + t.Fatal(err) + } + dir := filepath.Join(f.root, HelmfileDir) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + for name, body := range files { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + return f +} + +func (f *stackFixture) audit(t *testing.T) (int, string, string) { + t.Helper() + var out, errOut bytes.Buffer + code, err := Audit(f.root, &out, &errOut) + if err != nil { + errOut.WriteString(err.Error()) + } + return code, out.String(), errOut.String() +} + +func (f *stackFixture) bump(t *testing.T, tag string, write bool) (int, string, string) { + t.Helper() + var out, errOut bytes.Buffer + code, err := Bump(f.root, tag, write, &out, &errOut) + if err != nil { + errOut.WriteString(err.Error()) + } + return code, out.String(), errOut.String() +} + +func (f *stackFixture) read(t *testing.T, name string) string { + t.Helper() + b, err := os.ReadFile(filepath.Join(f.root, HelmfileDir, name)) + if err != nil { + t.Fatal(err) + } + return string(b) +} + +const stackMeta = `{"services":[ + {"id":"alpha","path":"deploy/helm/alpha","service_name":"helm-nvcf-alpha"}, + {"id":"beta","path":"deploy/helm/beta","service_name":"helm-nvcf-beta"}, + {"id":"reval","path":"deploy/helm/reval","service_name":"helm-reval"}, + {"id":"router","path":"deploy/helm/router","service_name":"helm-nvcf-llm-request-router"}, + {"id":"orphan","path":"deploy/helm/orphan","service_name":"helm-nvcf-orphan"}, + {"id":"nameless","path":"deploy/helm/nameless"} +]}` + +// alpha and beta are both pinned at 1.0.0 on purpose: a rewrite that is merely +// "close enough" moves both, and only a fixture where the two share a version +// exposes it. +const stackFile = `repositories: + - name: nvcf + url: oci://example.invalid/nvcf + +releases: + - name: alpha + namespace: nvcf + version: 1.0.0 + - name: beta + namespace: nvcf + version: 1.0.0 + - name: reval + chart: nvcf/helm-reval + version: 2.4.0 + - name: templated + chart: nvcf/helm-nvcf-{{ .Release.Name }} + version: 3.1.0 + - name: router + chart: {{ $chartOverride | default "nvcf/helm-nvcf-llm-request-router" | quote }} + version: 0.9.0 +` + +func TestRepositoriesBlockIsNotAPin(t *testing.T) { + // The repositories entry matches the release shape but carries no version. + // Counting it would put a bogus release in the audit and, worse, make the + // stack look resolvable when it is not. + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + _, out, _ := f.audit(t) + if strings.Contains(out, "nvcf ") && strings.Contains(out, "oci://") { + t.Fatalf("the repositories block should not appear as a release:\n%s", out) + } + if !strings.Contains(out, "5 releases, 0 unresolved") { + t.Fatalf("want exactly the five pinned releases:\n%s", out) + } +} + +func TestExplicitChartLineWins(t *testing.T) { + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + _, out, _ := f.audit(t) + if !strings.Contains(out, "-> helm-reval") { + t.Fatalf("an explicit chart line should resolve to that chart:\n%s", out) + } +} + +func TestConventionAppliesWhenThereIsNoChartLine(t *testing.T) { + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + _, out, _ := f.audit(t) + if !strings.Contains(out, "alpha") || !strings.Contains(out, "-> helm-nvcf-alpha") { + t.Fatalf("a release with no chart line inherits helm-nvcf-:\n%s", out) + } +} + +func TestReleaseNameTemplateResolvesByConvention(t *testing.T) { + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + _, out, _ := f.audit(t) + if !strings.Contains(out, "-> helm-nvcf-templated") { + t.Fatalf("a .Release.Name template resolves to the release's own chart:\n%s", out) + } +} + +func TestOverrideWithDefaultResolvesToTheDefault(t *testing.T) { + // The default is what ships unless an operator overrides it, so it is the + // chart an automated bump should follow. + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + _, out, _ := f.audit(t) + if !strings.Contains(out, "-> helm-nvcf-llm-request-router") { + t.Fatalf("an override-with-default should resolve to the default:\n%s", out) + } +} + +func TestUnknownTemplateFormIsUnresolvedNotGuessed(t *testing.T) { + body := `releases: + - name: mystery + chart: {{ include "something.else" . }} + version: 1.0.0 +` + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": body}) + code, out, errOut := f.audit(t) + if code != 1 { + t.Fatalf("an unreadable chart line must fail the audit, got %d", code) + } + if !strings.Contains(out, "1 releases, 1 unresolved") { + t.Fatalf("it must be counted as unresolved:\n%s", out) + } + if !strings.Contains(errOut, "mystery") { + t.Fatalf("the failure must name the release:\n%s", errOut) + } +} + +func TestBumpRefusesWhileAnythingIsUnresolved(t *testing.T) { + // The release nobody can read might be the one pinning this chart. Bumping + // the rest and reporting success is the silent failure this guards against. + body := stackFile + ` - name: mystery + chart: {{ include "something.else" . }} + version: 1.0.0 +` + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": body}) + code, _, errOut := f.bump(t, "deploy/helm/alpha/v2.0.0", true) + if code != 1 { + t.Fatalf("bump must refuse a partially understood stack, got %d", code) + } + if !strings.Contains(errOut, "refusing to bump") { + t.Fatalf("it should say why:\n%s", errOut) + } + if got := f.read(t, "00-stack.yaml.gotmpl"); !strings.Contains(got, "- name: alpha\n namespace: nvcf\n version: 1.0.0") { + t.Fatalf("nothing may be written when the stack is unresolved:\n%s", got) + } +} + +func TestBumpMovesOnlyTheMatchingRelease(t *testing.T) { + // alpha and beta share the version 1.0.0. A rewrite keyed on the value + // rather than on the release moves both. + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + code, out, errOut := f.bump(t, "deploy/helm/alpha/v2.0.0", true) + if code != 0 { + t.Fatalf("bump should succeed, got %d: %s", code, errOut) + } + if !strings.Contains(out, "alpha: 1.0.0 -> 2.0.0") { + t.Fatalf("alpha should have moved:\n%s", out) + } + got := f.read(t, "00-stack.yaml.gotmpl") + if !strings.Contains(got, "- name: alpha\n namespace: nvcf\n version: 2.0.0") { + t.Fatalf("alpha's pin did not move:\n%s", got) + } + if !strings.Contains(got, "- name: beta\n namespace: nvcf\n version: 1.0.0") { + t.Fatalf("beta shares alpha's old version and must not have moved:\n%s", got) + } +} + +func TestBumpLeavesEveryOtherLineByteIdentical(t *testing.T) { + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + f.bump(t, "deploy/helm/alpha/v2.0.0", true) + before := strings.Split(stackFile, "\n") + after := strings.Split(f.read(t, "00-stack.yaml.gotmpl"), "\n") + if len(before) != len(after) { + t.Fatalf("line count changed: %d -> %d", len(before), len(after)) + } + diffs := 0 + for i := range before { + if before[i] != after[i] { + diffs++ + } + } + if diffs != 1 { + t.Fatalf("want exactly one changed line, got %d", diffs) + } +} + +func TestAlreadyPinnedIsANoOp(t *testing.T) { + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + code, out, _ := f.bump(t, "deploy/helm/alpha/v1.0.0", true) + if code != 0 { + t.Fatalf("re-pinning the current version is not an error, got %d", code) + } + if !strings.Contains(out, "already 1.0.0") { + t.Fatalf("it should say so:\n%s", out) + } + if f.read(t, "00-stack.yaml.gotmpl") != stackFile { + t.Fatal("a no-op bump rewrote the file") + } +} + +func TestChartNobodyPinsIsAnError(t *testing.T) { + // A chart released but pinned nowhere is exactly the nvcf-unbound shape: + // the release happens, the stack never picks it up, and nothing says so. + // orphan is declared in the metadata and appears in no helmfile release. + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + code, _, errOut := f.bump(t, "deploy/helm/orphan/v2.0.0", false) + if code != 1 { + t.Fatalf("a chart no stack release pins must fail, got %d", code) + } + if !strings.Contains(errOut, "no stack release pins helm-nvcf-orphan") { + t.Fatalf("the error must name the chart that went unpinned:\n%s", errOut) + } +} + +func TestUnpinnedChartIsAnError(t *testing.T) { + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + // nameless is in the metadata but has no service_name, so it cannot resolve. + code, _, errOut := f.bump(t, "deploy/helm/nameless/v1.0.0", false) + if code != 1 { + t.Fatalf("a chart with no service_name must fail, got %d", code) + } + if !strings.Contains(errOut, "no service_name") { + t.Fatalf("the error should say what is missing:\n%s", errOut) + } +} + +func TestUnknownChartPathIsAnError(t *testing.T) { + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + code, _, errOut := f.bump(t, "deploy/helm/nosuch/v1.0.0", false) + if code != 1 { + t.Fatalf("an unknown chart path must fail, got %d", code) + } + if !strings.Contains(errOut, "no release-metadata entry") { + t.Fatalf("the error should say what is missing:\n%s", errOut) + } +} + +func TestNonChartTagIsRejected(t *testing.T) { + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + code, _, errOut := f.bump(t, "src/control-plane-services/notary/v1.9.0", false) + if code != 1 { + t.Fatalf("a service release tag is not this tool's business, got %d", code) + } + if !strings.Contains(errOut, "not a chart release tag") { + t.Fatalf("it should say so:\n%s", errOut) + } +} + +func TestRealStackResolvesCompletely(t *testing.T) { + // The whole point: every release in the shipped stack must resolve. A new + // release added in a form this cannot read fails here rather than silently + // missing its bump later. + root := repoRoot(t) + var out, errOut bytes.Buffer + code, err := Audit(root, &out, &errOut) + if err != nil { + t.Fatalf("auditing the checked-in stack failed: %v", err) + } + if code != 0 { + t.Fatalf("the checked-in stack has unresolved releases:\n%s", errOut.String()) + } + releases, err := LoadStack(root) + if err != nil { + t.Fatal(err) + } + if len(releases) == 0 { + t.Fatal("no releases found; the helmfile path or glob is wrong") + } + t.Logf("%d releases resolved", len(releases)) +} + +func repoRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + for i := 0; i < 6; i++ { + if _, err := os.Stat(filepath.Join(dir, MetadataPath)); err == nil { + return dir + } + dir = filepath.Dir(dir) + } + t.Fatalf("could not find %s above the test directory", MetadataPath) + return "" +} diff --git a/tools/stack-pin-resolver/metadata.go b/tools/stack-pin-resolver/metadata.go new file mode 100644 index 000000000..d35a5744f --- /dev/null +++ b/tools/stack-pin-resolver/metadata.go @@ -0,0 +1,42 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" +) + +// MetadataPath is the release metadata that github-release already owns. The +// chart's published name lives there rather than being derived from the +// directory, because the two differ: deploy/helm/sis publishes helm-nvcf-sis. +const MetadataPath = "tools/ci/github-release-subprojects.json" + +// Entry is one subproject in the release metadata. +type Entry struct { + ID string `json:"id"` + Path string `json:"path"` + ServiceName string `json:"service_name"` +} + +// Metadata is the decoded release metadata file. +type Metadata struct { + Services []Entry `json:"services"` +} + +// LoadMetadata reads and decodes the release metadata under root. +func LoadMetadata(root string) (*Metadata, error) { + path := filepath.Join(root, MetadataPath) + b, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read release metadata: %w", err) + } + var m Metadata + if err := json.Unmarshal(b, &m); err != nil { + return nil, fmt.Errorf("parse %s: %w", path, err) + } + return &m, nil +} diff --git a/tools/stack-pin-resolver/stack.go b/tools/stack-pin-resolver/stack.go new file mode 100644 index 000000000..df0778f57 --- /dev/null +++ b/tools/stack-pin-resolver/stack.go @@ -0,0 +1,193 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os" + "path/filepath" + "regexp" + "sort" + "strings" +) + +// HelmfileDir holds the self-managed stack's release definitions. +const HelmfileDir = "deploy/stacks/self-managed/helmfile.d" + +var ( + nameRE = regexp.MustCompile(`^\s+- name:\s*(\S+)`) + chartRE = regexp.MustCompile(`^\s+chart:\s*(.+?)\s*$`) + versionRE = regexp.MustCompile(`^(\s+version:\s*)([0-9]\S*)(\s*)$`) + // The shared template most releases inherit. The inner braces are escaped + // in the helmfile source because helmfile passes the expression through to + // helm. + templateChartRE = regexp.MustCompile(`helm-nvcf-\{\{.*?\.Release\.Name.*?\}\}`) + // An override-with-default line names the real chart inside the default: + // chart: {{ $someVar | default "nvcf/helm-nvcf-llm-request-router" | quote }} + // The default is the chart used unless an operator overrides it, so it is + // the one an automated bump should follow. + defaultChartRE = regexp.MustCompile(`default\s+"([^"]+)"`) +) + +// A Release is one pinned entry in the stack. +type Release struct { + Name string + File string + // Chart is the published chart name this release pins, empty when + // Unresolved is set. + Chart string + // Unresolved says why the chart could not be determined. A release that + // cannot be read might be the one that pins the chart being bumped, which + // is why an unresolved entry blocks the whole run rather than being skipped. + Unresolved string + Version string + // VersionLine is the index into the file's lines holding the pin, so the + // rewrite can replace exactly that line and nothing else. + VersionLine int +} + +// ChartNameForRelease returns the chart a stack release pins. +// +// The helmfile names a chart in one of three ways, and the third cannot be +// resolved by reading the file: +// +// explicit chart: nvcf/helm-reval +// convention no chart: line, inherits a template of the form +// nvcf/helm-nvcf-{{ .Release.Name }} +// templated chart: {{ ... }} with anything else inside +// +// A templated chart line is reported as unresolved rather than guessed at. +func ChartNameForRelease(releaseName string, body []string) (string, error) { + value := "" + found := false + for _, line := range body { + if m := chartRE.FindStringSubmatch(line); m != nil { + value, found = m[1], true + break + } + } + if !found { + // No chart line: inherits the shared template, which appends the + // release name to a fixed prefix. + return "helm-nvcf-" + releaseName, nil + } + if !strings.Contains(value, "{{") { + return lastPathSegment(value), nil + } + if templateChartRE.MatchString(value) { + return "helm-nvcf-" + releaseName, nil + } + if m := defaultChartRE.FindStringSubmatch(value); m != nil { + return lastPathSegment(m[1]), nil + } + return "", fmt.Errorf("chart line is templated and not a known form: %s", value) +} + +func lastPathSegment(s string) string { + if i := strings.LastIndex(s, "/"); i >= 0 { + return s[i+1:] + } + return s +} + +// LoadStack returns every pinned release in the stack. +// +// Split line by line rather than with one regex over the whole file: matching a +// release block needs "up to the next release or end of file", which is a +// lookahead, and Go's regexp engine has none. Tracking line numbers also lets +// the rewrite replace one exact line instead of reconstructing a block. +func LoadStack(root string) ([]Release, error) { + paths, err := filepath.Glob(filepath.Join(root, HelmfileDir, "*.yaml.gotmpl")) + if err != nil { + return nil, err + } + sort.Strings(paths) + + var out []Release + for _, path := range paths { + b, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read %s: %w", path, err) + } + lines := strings.Split(string(b), "\n") + for _, blk := range splitReleases(lines) { + body := lines[blk.start : blk.end+1] + + versionLine, version := -1, "" + for i, line := range body { + if m := versionRE.FindStringSubmatch(line); m != nil { + versionLine, version = blk.start+i, m[2] + break + } + } + if versionLine < 0 { + // Not a pin. The shared templates block and the repositories + // block both match the release shape but carry no version. + continue + } + + r := Release{Name: blk.name, File: filepath.Base(path), Version: version, VersionLine: versionLine} + chart, err := ChartNameForRelease(blk.name, body) + if err != nil { + r.Unresolved = err.Error() + } else { + r.Chart = chart + } + out = append(out, r) + } + } + return out, nil +} + +type block struct { + name string + start, end int +} + +func splitReleases(lines []string) []block { + var blocks []block + for i, line := range lines { + m := nameRE.FindStringSubmatch(line) + if m == nil { + continue + } + if n := len(blocks); n > 0 { + blocks[n-1].end = i - 1 + } + blocks = append(blocks, block{name: m[1], start: i, end: len(lines) - 1}) + } + return blocks +} + +// WritePin replaces the version on a single release's pin line. +func WritePin(root string, r Release, version string) error { + path := filepath.Join(root, HelmfileDir, r.File) + b, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("read %s: %w", path, err) + } + lines := strings.Split(string(b), "\n") + if r.VersionLine < 0 || r.VersionLine >= len(lines) { + return fmt.Errorf("%s: pin line %d is out of range for %s", r.Release(), r.VersionLine, path) + } + m := versionRE.FindStringSubmatch(lines[r.VersionLine]) + if m == nil { + // The file changed under us. Rewriting a line that is no longer a pin + // would corrupt the stack, so stop instead. + return fmt.Errorf("%s: line %d in %s is no longer a version pin", r.Release(), r.VersionLine+1, path) + } + lines[r.VersionLine] = m[1] + version + m[3] + + mode := os.FileMode(0o644) + if fi, err := os.Stat(path); err == nil { + mode = fi.Mode().Perm() + } + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")), mode); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + return nil +} + +// Release names the release for error messages. +func (r Release) Release() string { return r.Name } From a6025ba98c017d2867e45d5fe44b61ec0e8ed068 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 10:19:36 -0700 Subject: [PATCH 4/6] fix(ci): address review findings on the stack pin bump Seven findings from review, each verified against the code first. Resolver: The version out of a release tag was written verbatim into a shipped helmfile after matching only `.+`. Anyone who can push a tag chooses that value, so a space, a quote or a path traversal would corrupt the file or smuggle in an adjacent key. It is now validated as a plain version before anything is written. The version pin was matched at any indent, so a `version:` nested inside a values block could be taken as the release's pin and rewritten while the real pin stayed put. Matching is now restricted to the release's own field indent. A release-level `version:` that could not be parsed was skipped silently, which drops a real pin. That is the failure this tool exists to prevent, so it is now reported as unresolved. A quoted value is also recognised; nothing in the stack is quoted today, but gaining quotes must not silently remove a pin from every future bump. Workflow: The bump was applied on the default branch and then stashed across a checkout of the pull request branch, with `stash pop || true`. Whenever the branch already carried a bump for the same pin, that conflicts, and the swallowed failure either drops the earlier bump or commits conflict markers. The branch is now checked out before the bump runs, which also makes the run idempotent: the resolver sees the current value and reports "already ". The release tag was expanded into the run: body through `${{ }}`, the standard Actions injection shape for a value an outside contributor can choose. It is passed through env. Generated commits carried a fixed `Co-authored-by` trailer naming one person, so every future automated bump would be attributed to them in published history. Removed. The committer identity is now the standard github-actions[bot] rather than a private service account. `gh pr edit` fails against this repository with "Projects (classic) is being deprecated ... (repository.pullRequest.projectCards)", so refreshing an existing pull request would have failed on every run after the first. Replaced with the REST endpoint, which has no such dependency. Four new tests, mutation tested; each of the four resolver behaviours above dies when removed. The nested-key fixture puts the nested version before the release's own pin: ordered the other way the loop finds the real pin first and the test passes with the indent check deleted, which mutation testing caught. Declined: forwarding W3C traceparent on gh API calls, as this job makes three calls in a workflow that already has run-level tracing. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/stack-pin-bump.yml | 65 +++++++++++------ tools/stack-pin-resolver/main.go | 10 +++ tools/stack-pin-resolver/main_test.go | 101 ++++++++++++++++++++++++++ tools/stack-pin-resolver/stack.go | 54 +++++++++++--- 4 files changed, 197 insertions(+), 33 deletions(-) diff --git a/.github/workflows/stack-pin-bump.yml b/.github/workflows/stack-pin-bump.yml index 90df4bf33..048529942 100644 --- a/.github/workflows/stack-pin-bump.yml +++ b/.github/workflows/stack-pin-bump.yml @@ -82,12 +82,39 @@ jobs: # name attached, rather than being quietly skipped over. run: tools/ci/stack-pin-resolver --audit + - name: Check out the bump branch + if: steps.tag.outputs.applies == 'true' + env: + BRANCH: chore/stack-pin-bumps + run: | + set -euo pipefail + # The bump is applied ON the pull request branch, not on the default + # branch and moved across afterwards. Bumping first and stashing the + # result over a checkout collides whenever the branch already carries + # a bump for the same pin, and a swallowed stash conflict either drops + # that earlier bump or commits conflict markers. Starting here also + # makes the tool idempotent: it sees the existing value and reports + # "already ". + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + git fetch origin "${BRANCH}" || true + if git rev-parse --verify -q "origin/${BRANCH}" >/dev/null; then + git checkout -B "${BRANCH}" "origin/${BRANCH}" + else + git checkout -B "${BRANCH}" + fi + - name: Apply the bump id: bump if: steps.tag.outputs.applies == 'true' + env: + # Through env, never expanded into the script body: a tag is chosen by + # whoever pushes it, and ${{ }} interpolation into a run: block is the + # standard Actions injection shape. + TAG: ${{ steps.tag.outputs.tag }} run: | set -euo pipefail - tools/ci/stack-pin-resolver --tag "${{ steps.tag.outputs.tag }}" --write + tools/ci/stack-pin-resolver --tag "${TAG}" --write # Scoped to the same paths the commit below stages. Repo-wide, any # unrelated modification in the workspace would set changed=true and # the commit would then abort with nothing staged. @@ -104,23 +131,11 @@ jobs: env: GH_TOKEN: ${{ secrets.NV_GITHUB_TOKEN || github.token }} TAG: ${{ steps.tag.outputs.tag }} + SERVER_URL: ${{ github.server_url }} + REPO: ${{ github.repository }} run: | set -euo pipefail branch="chore/stack-pin-bumps" - git config user.name "nvcf-release-bot" - git config user.email "svc-nvcf-release@nvidia.com" - - # A fixed branch, refreshed. Several releases landing close together - # then produce one pull request carrying all of their bumps instead - # of a pile that conflict with each other. - git fetch origin "${branch}" || true - if git rev-parse --verify -q "origin/${branch}" >/dev/null; then - git stash push --quiet - git checkout -B "${branch}" "origin/${branch}" - git stash pop --quiet || true - else - git checkout -B "${branch}" - fi git add deploy/stacks/self-managed/helmfile.d # Separate -m flags rather than an embedded multi-line string: the @@ -128,8 +143,7 @@ jobs: # ends the YAML block scalar this script lives in. git commit \ -m "chore(stack): pin ${TAG#deploy/helm/}" \ - -m "Opened by the stack pin bump workflow on release of ${TAG}." \ - -m "Co-authored-by: Balaji Ganesan " + -m "Opened by the stack pin bump workflow on release of ${TAG}." git push --force-with-lease origin "${branch}" body="$(printf '%s\n' \ @@ -137,18 +151,23 @@ jobs: "" \ "The released tag carries the version, so this is a direct pin update rather than a lookup of the newest published chart." \ "" \ - "Release notes: ${{ github.server_url }}/${{ github.repository }}/releases/tag/${TAG}" \ + "Release notes: ${SERVER_URL}/${REPO}/releases/tag/${TAG}" \ "" \ "If this pull request sits unmerged, later chart releases add their bumps to the same branch, so merging it applies all of them." \ "" \ "Github commit:" \ "chore(stack): pin ${TAG#deploy/helm/}" \ - "" \ - "Co-authored-by: Balaji Ganesan ")" + "")" - if gh pr view "${branch}" --json number >/dev/null 2>&1; then - gh pr edit "${branch}" --body "${body}" - echo "refreshed the existing pull request" + # gh api, not `gh pr edit`. Against this repository `gh pr edit` fails + # with "Projects (classic) is being deprecated ... + # (repository.pullRequest.projectCards)", because it queries project + # cards it does not need. The REST endpoint has no such dependency. + number="$(gh api "repos/${REPO}/pulls?head=${REPO%%/*}:${branch}&state=open" -q '.[0].number')" + if [ -n "${number}" ] && [ "${number}" != "null" ]; then + jq -n --arg b "${body}" '{body: $b}' \ + | gh api -X PATCH "repos/${REPO}/pulls/${number}" --input - >/dev/null + echo "refreshed pull request #${number}" else gh pr create --base main --head "${branch}" \ --title "chore(stack): bump self-managed stack chart pins" \ diff --git a/tools/stack-pin-resolver/main.go b/tools/stack-pin-resolver/main.go index 36c5aad84..40d49cd20 100644 --- a/tools/stack-pin-resolver/main.go +++ b/tools/stack-pin-resolver/main.go @@ -39,6 +39,13 @@ import ( var tagRE = regexp.MustCompile(`^(deploy/helm/.+)/v(.+)$`) +// The version out of a tag is written verbatim into a shipped helmfile, so it +// is validated before it gets there rather than after. A tag is attacker +// influenceable by anyone who can push one, and `.+` would accept a value +// carrying spaces, quotes or a newline, which would corrupt the file or smuggle +// in an adjacent key. +var versionRE = regexp.MustCompile(`^[0-9][A-Za-z0-9._+-]*$`) + func main() { auditMode := flag.Bool("audit", false, "report the chart every stack release pins") tag := flag.String("tag", "", "chart release tag, for example deploy/helm/nats/v0.8.0") @@ -102,6 +109,9 @@ func Bump(root, tag string, write bool, out, errOut io.Writer) (int, error) { return 1, fmt.Errorf("not a chart release tag: %s", tag) } chartPath, version := m[1], m[2] + if !versionRE.MatchString(version) { + return 1, fmt.Errorf("tag %s carries a version that is not a plain version string: %q", tag, version) + } chart, err := ChartNameForPath(root, chartPath) if err != nil { diff --git a/tools/stack-pin-resolver/main_test.go b/tools/stack-pin-resolver/main_test.go index 4af867529..baae6ca77 100644 --- a/tools/stack-pin-resolver/main_test.go +++ b/tools/stack-pin-resolver/main_test.go @@ -328,3 +328,104 @@ func repoRoot(t *testing.T) string { t.Fatalf("could not find %s above the test directory", MetadataPath) return "" } + +func TestTagVersionIsValidatedBeforeItIsWritten(t *testing.T) { + // The version out of a tag is written verbatim into a shipped helmfile, and + // anyone who can push a tag chooses it. A value carrying a space, a quote or + // a newline would corrupt the file or smuggle in an adjacent key. + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": stackFile}) + for _, bad := range []string{ + "deploy/helm/alpha/v2.0.0 extra", + `deploy/helm/alpha/v"quoted"`, + "deploy/helm/alpha/vlatest", + "deploy/helm/alpha/v../../etc/passwd", + } { + code, _, errOut := f.bump(t, bad, true) + if code != 1 { + t.Errorf("tag %q must be rejected, got exit %d", bad, code) + } + if !strings.Contains(errOut, "not a plain version string") { + t.Errorf("tag %q: want a version-format error, got %q", bad, errOut) + } + } + // A newline is rejected one step earlier, by the tag pattern itself, since + // Go's . does not match one. Asserted separately so the message difference + // is deliberate rather than a gap. + if code, _, errOut := f.bump(t, "deploy/helm/alpha/v2.0.0\nversion: 9.9.9", true); code != 1 || + !strings.Contains(errOut, "not a chart release tag") { + t.Errorf("a tag carrying a newline must be rejected: exit %d, %q", code, errOut) + } + if f.read(t, "00-stack.yaml.gotmpl") != stackFile { + t.Fatal("a rejected tag still wrote to the helmfile") + } +} + +func TestNestedVersionKeyIsNotMistakenForThePin(t *testing.T) { + // A version: inside a values block sits deeper than the release's own + // fields. Matching any indent rewrites an unrelated key and leaves the real + // pin untouched. + // The nested key comes FIRST, before the release's own pin. Ordered the + // other way the loop finds the real pin and stops, so the test passes even + // with the indent check removed. Mutation testing caught exactly that. + body := `releases: + - name: alpha + namespace: nvcf + values: + - image: + version: 7.7.7 + version: 1.0.0 +` + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": body}) + if code, _, errOut := f.bump(t, "deploy/helm/alpha/v2.0.0", true); code != 0 { + t.Fatalf("bump should succeed, got %d: %s", code, errOut) + } + got := f.read(t, "00-stack.yaml.gotmpl") + if !strings.Contains(got, " version: 2.0.0") { + t.Fatalf("the release pin did not move:\n%s", got) + } + if !strings.Contains(got, " version: 7.7.7") { + t.Fatalf("the nested value must not be touched:\n%s", got) + } +} + +func TestUnreadableReleaseVersionIsReportedNotSkipped(t *testing.T) { + // Silently skipping a release-level version that cannot be parsed drops a + // real pin, which is the failure this tool exists to prevent. A block with + // no version at all is still not a pin, and must stay that way. + body := `repositories: + - name: nvcf + url: oci://example.invalid/nvcf + +releases: + - name: alpha + version: {{ .Values.someVersion }} +` + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": body}) + code, out, errOut := f.audit(t) + if code != 1 { + t.Fatalf("an unreadable pin must fail the audit, got %d", code) + } + if !strings.Contains(out, "1 releases, 1 unresolved") { + t.Fatalf("it must be counted, not dropped:\n%s", out) + } + if !strings.Contains(errOut, "not a recognisable pin") { + t.Fatalf("the reason should say the version is unreadable:\n%s", errOut) + } +} + +func TestQuotedVersionIsStillAPin(t *testing.T) { + // Nothing in the stack is quoted today. If a value gains quotes it must not + // silently stop being recognised, which would drop it from every bump. + body := `releases: + - name: alpha + version: "1.0.0" +` + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": body}) + code, out, errOut := f.audit(t) + if code != 0 { + t.Fatalf("a quoted version is still a pin, got %d: %s", code, errOut) + } + if !strings.Contains(out, "1 releases, 0 unresolved") { + t.Fatalf("want it counted as a resolved pin:\n%s", out) + } +} diff --git a/tools/stack-pin-resolver/stack.go b/tools/stack-pin-resolver/stack.go index df0778f57..762d5c507 100644 --- a/tools/stack-pin-resolver/stack.go +++ b/tools/stack-pin-resolver/stack.go @@ -16,9 +16,14 @@ import ( const HelmfileDir = "deploy/stacks/self-managed/helmfile.d" var ( - nameRE = regexp.MustCompile(`^\s+- name:\s*(\S+)`) - chartRE = regexp.MustCompile(`^\s+chart:\s*(.+?)\s*$`) - versionRE = regexp.MustCompile(`^(\s+version:\s*)([0-9]\S*)(\s*)$`) + nameRE = regexp.MustCompile(`^(\s+)- name:\s*(\S+)`) + chartRE = regexp.MustCompile(`^(\s+)chart:\s*(.+?)\s*$`) + // Split so the value can be validated separately from the line shape. A + // version line whose value is not a version is reported, not skipped. + versionLineRE = regexp.MustCompile(`^(\s+version:\s*)(.*?)(\s*)$`) + // Quoted or bare. Nothing in the stack is quoted today, but a value that + // gains quotes must not silently stop being recognised as a pin. + versionValueRE = regexp.MustCompile(`^"?v?[0-9][^"\s]*"?$`) // The shared template most releases inherit. The inner braces are escaped // in the helmfile source because helmfile passes the expression through to // helm. @@ -63,7 +68,7 @@ func ChartNameForRelease(releaseName string, body []string) (string, error) { found := false for _, line := range body { if m := chartRE.FindStringSubmatch(line); m != nil { - value, found = m[1], true + value, found = m[2], true break } } @@ -114,20 +119,38 @@ func LoadStack(root string) ([]Release, error) { for _, blk := range splitReleases(lines) { body := lines[blk.start : blk.end+1] - versionLine, version := -1, "" + // Only at the release's own field indent. A version: nested deeper + // belongs to a values block or a sub-object, and treating it as the + // pin would rewrite an unrelated key. + fieldIndent := blk.indent + 2 + versionLine, version, malformed := -1, "", "" for i, line := range body { - if m := versionRE.FindStringSubmatch(line); m != nil { - versionLine, version = blk.start+i, m[2] + m := versionLineRE.FindStringSubmatch(line) + if m == nil || len(m[1])-len("version:")-countTrailingSpace(m[1]) != fieldIndent { + continue + } + if !versionValueRE.MatchString(m[2]) { + // A release-level version that cannot be read is reported. + // Skipping it would drop a real pin silently, which is the + // failure this tool exists to prevent. + malformed = m[2] break } + versionLine, version = blk.start+i, strings.Trim(m[2], `"`) + break } - if versionLine < 0 { + if versionLine < 0 && malformed == "" { // Not a pin. The shared templates block and the repositories // block both match the release shape but carry no version. continue } r := Release{Name: blk.name, File: filepath.Base(path), Version: version, VersionLine: versionLine} + if malformed != "" { + r.Unresolved = fmt.Sprintf("version is not a recognisable pin: %s", malformed) + out = append(out, r) + continue + } chart, err := ChartNameForRelease(blk.name, body) if err != nil { r.Unresolved = err.Error() @@ -142,6 +165,7 @@ func LoadStack(root string) ([]Release, error) { type block struct { name string + indent int start, end int } @@ -155,11 +179,21 @@ func splitReleases(lines []string) []block { if n := len(blocks); n > 0 { blocks[n-1].end = i - 1 } - blocks = append(blocks, block{name: m[1], start: i, end: len(lines) - 1}) + blocks = append(blocks, block{name: m[2], indent: len(m[1]), start: i, end: len(lines) - 1}) } return blocks } +// countTrailingSpace counts the run of spaces at the end of s, which is how the +// indent of a "version:" line is recovered from its captured prefix. +func countTrailingSpace(s string) int { + n := 0 + for i := len(s) - 1; i >= 0 && s[i] == ' '; i-- { + n++ + } + return n +} + // WritePin replaces the version on a single release's pin line. func WritePin(root string, r Release, version string) error { path := filepath.Join(root, HelmfileDir, r.File) @@ -171,7 +205,7 @@ func WritePin(root string, r Release, version string) error { if r.VersionLine < 0 || r.VersionLine >= len(lines) { return fmt.Errorf("%s: pin line %d is out of range for %s", r.Release(), r.VersionLine, path) } - m := versionRE.FindStringSubmatch(lines[r.VersionLine]) + m := versionLineRE.FindStringSubmatch(lines[r.VersionLine]) if m == nil { // The file changed under us. Rewriting a line that is no longer a pin // would corrupt the stack, so stop instead. From ad0907a16965d76c00bc846cb0f8279ac23c6c13 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 10:52:24 -0700 Subject: [PATCH 5/6] fix(ci): reject unmatched quotes in a stack version pin The version pattern made the quote optional at each end independently, so it accepted `version: "1.0.0` and `version: 1.0.0"`. Both are malformed YAML, and both were classified as resolved pins, which meant the rewrite replaced the line and quietly laundered the error away instead of stopping and reporting it. Bare, or quoted on both ends, and nothing else. Two tests cover the unmatched forms and assert nothing is written for them; a third covers the four accepted forms. Reverting the pattern kills both. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- tools/stack-pin-resolver/main_test.go | 37 +++++++++++++++++++++++++++ tools/stack-pin-resolver/stack.go | 11 +++++--- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/tools/stack-pin-resolver/main_test.go b/tools/stack-pin-resolver/main_test.go index baae6ca77..b473ecb45 100644 --- a/tools/stack-pin-resolver/main_test.go +++ b/tools/stack-pin-resolver/main_test.go @@ -429,3 +429,40 @@ func TestQuotedVersionIsStillAPin(t *testing.T) { t.Fatalf("want it counted as a resolved pin:\n%s", out) } } + +func TestUnmatchedQuotesAreNotAPin(t *testing.T) { + // `"1.0.0` and `1.0.0"` are malformed YAML. Accepting either as a pin means + // the rewrite replaces the line and launders the error away rather than + // reporting it, so both must land in the unresolved bucket. + for _, bad := range []string{`"1.0.0`, `1.0.0"`} { + body := "releases:\n - name: alpha\n version: " + bad + "\n" + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": body}) + code, out, errOut := f.audit(t) + if code != 1 { + t.Errorf("version %s must fail the audit, got %d", bad, code) + } + if !strings.Contains(out, "1 releases, 1 unresolved") { + t.Errorf("version %s must be counted unresolved:\n%s", bad, out) + } + if !strings.Contains(errOut, "not a recognisable pin") { + t.Errorf("version %s: want the unreadable-pin reason, got %q", bad, errOut) + } + // And nothing may be written for it. + if c, _, _ := f.bump(t, "deploy/helm/alpha/v2.0.0", true); c != 1 { + t.Errorf("version %s: bump must refuse, got %d", bad, c) + } + if f.read(t, "00-stack.yaml.gotmpl") != body { + t.Errorf("version %s: the helmfile was rewritten", bad) + } + } +} + +func TestBareAndFullyQuotedVersionsBothResolve(t *testing.T) { + for _, good := range []string{`1.0.0`, `"1.0.0"`, `v1.0.0`, `"v1.0.0"`} { + body := "releases:\n - name: alpha\n version: " + good + "\n" + f := newStack(t, stackMeta, map[string]string{"00-stack.yaml.gotmpl": body}) + if code, out, errOut := f.audit(t); code != 0 { + t.Errorf("version %s should resolve, got %d\n%s%s", good, code, out, errOut) + } + } +} diff --git a/tools/stack-pin-resolver/stack.go b/tools/stack-pin-resolver/stack.go index 762d5c507..0a06f4a0d 100644 --- a/tools/stack-pin-resolver/stack.go +++ b/tools/stack-pin-resolver/stack.go @@ -21,9 +21,14 @@ var ( // Split so the value can be validated separately from the line shape. A // version line whose value is not a version is reported, not skipped. versionLineRE = regexp.MustCompile(`^(\s+version:\s*)(.*?)(\s*)$`) - // Quoted or bare. Nothing in the stack is quoted today, but a value that - // gains quotes must not silently stop being recognised as a pin. - versionValueRE = regexp.MustCompile(`^"?v?[0-9][^"\s]*"?$`) + // Bare, or quoted on both ends. Nothing in the stack is quoted today, but a + // value that gains quotes must not silently stop being recognised as a pin. + // + // Optional quotes on each end independently would accept `"1.0.0` and + // `1.0.0"`, which are malformed YAML. Treating those as a valid pin means + // the rewrite replaces the line and quietly launders the error away instead + // of stopping and reporting it. + versionValueRE = regexp.MustCompile(`^(?:v?[0-9][^"\s]*|"v?[0-9][^"\s]*")$`) // The shared template most releases inherit. The inner braces are escaped // in the helmfile source because helmfile passes the expression through to // helm. From ab03a753a03c2ee0eed1b00217d212c70c5fc8a1 Mon Sep 17 00:00:00 2001 From: Balaji Ganesan Date: Wed, 26 Aug 2026 12:52:07 -0700 Subject: [PATCH 6/6] fix(ci): make the generated stack pin commit cut a stack release deploy/stacks/self-managed is itself a release subproject, and tools/ci/github-release feeds RELEASE_RULES to semantic-release where chore carries "release": false. release-tags.yml runs `github-release auto` on every push to main with auto tagging enabled and dry run off, so a generated commit of `chore(stack): pin ` moved the pin on main without ever cutting a stack release. Nothing downstream would see it. Now `fix(stack):`, which cuts a patch release of the stack. Co-authored-by: Balaji Ganesan Signed-off-by: Balaji Ganesan --- .github/workflows/stack-pin-bump.yml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/stack-pin-bump.yml b/.github/workflows/stack-pin-bump.yml index 048529942..9a1a34516 100644 --- a/.github/workflows/stack-pin-bump.yml +++ b/.github/workflows/stack-pin-bump.yml @@ -141,8 +141,13 @@ jobs: # Separate -m flags rather than an embedded multi-line string: the # continuation lines of one would have to sit at column zero, which # ends the YAML block scalar this script lives in. + # fix, not chore. deploy/stacks/self-managed is itself a release + # subproject, and tools/ci/github-release feeds RELEASE_RULES to + # semantic-release, where chore carries "release": false. A chore + # commit would move the pin on main without ever cutting a stack + # release, so nothing downstream would see the new pin. git commit \ - -m "chore(stack): pin ${TAG#deploy/helm/}" \ + -m "fix(stack): pin ${TAG#deploy/helm/}" \ -m "Opened by the stack pin bump workflow on release of ${TAG}." git push --force-with-lease origin "${branch}" @@ -156,7 +161,7 @@ jobs: "If this pull request sits unmerged, later chart releases add their bumps to the same branch, so merging it applies all of them." \ "" \ "Github commit:" \ - "chore(stack): pin ${TAG#deploy/helm/}" \ + "fix(stack): pin ${TAG#deploy/helm/}" \ "")" # gh api, not `gh pr edit`. Against this repository `gh pr edit` fails @@ -170,6 +175,6 @@ jobs: echo "refreshed pull request #${number}" else gh pr create --base main --head "${branch}" \ - --title "chore(stack): bump self-managed stack chart pins" \ + --title "fix(stack): bump self-managed stack chart pins" \ --body "${body}" fi