From 6a9aed5d3c29ff5d7eb488ee5e05b05e9773dcf9 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Fri, 18 Sep 2026 10:59:48 -0700 Subject: [PATCH 1/5] ci(stack): check migrations against the proposed stack version bump The upgrade contract is that a customer installs each stack major version in order, derived from the version numbers alone so nothing has to publish a catalog or maintain a floor per release. What that rule needs in exchange is that a major boundary is actually cut whenever a candidate contains something a customer cannot safely skip past. That is not decidable by eye. A stack-pin diff is a list of chart versions moving, and nothing in "1.5.3 -> 2.0.0" says a migration landed. In a monorepo the evidence is computable instead, which also means it cannot be forgotten the way a declaration written in a pull request months earlier can. Additive migrations never qualify: golang-migrate applies the ordered set per keyspace, so a cluster many versions behind still arrives at the right schema. Destructive migrations and deleted migration files do, because a drop is unrecoverable without a restore and a deleted migration is how a compatibility bridge stops shipping. Comments are stripped before classifying. nvcf_api/03_init_tables.up.sql documents a "high-churn write/delete workload" above a CREATE TABLE, and a classifier reading raw text calls that destructive. Run over the current corpus the classifier finds exactly the four real drops and leaves the other 38 files additive. The baseline is the stack's last release tag rather than the pinned migrations image, because the stack does not pin that image yet (#1976). Refs #1975 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/build-test.yml | 37 +++++ tools/ci/check-stack-upgrade-policy | 33 +++++ tools/ci/github-release-subprojects.json | 1 + tools/stack-upgrade-policy/.gitignore | 2 + tools/stack-upgrade-policy/classify.go | 57 ++++++++ tools/stack-upgrade-policy/classify_test.go | 46 ++++++ tools/stack-upgrade-policy/config.go | 54 +++++++ tools/stack-upgrade-policy/config_test.go | 74 ++++++++++ tools/stack-upgrade-policy/decide.go | 112 +++++++++++++++ tools/stack-upgrade-policy/decide_test.go | 84 +++++++++++ tools/stack-upgrade-policy/evidence.go | 88 ++++++++++++ tools/stack-upgrade-policy/evidence_test.go | 143 +++++++++++++++++++ tools/stack-upgrade-policy/go.mod | 3 + tools/stack-upgrade-policy/main.go | 150 ++++++++++++++++++++ tools/stack-upgrade-policy/main_test.go | 147 +++++++++++++++++++ 15 files changed, 1031 insertions(+) create mode 100755 tools/ci/check-stack-upgrade-policy create mode 100644 tools/stack-upgrade-policy/.gitignore create mode 100644 tools/stack-upgrade-policy/classify.go create mode 100644 tools/stack-upgrade-policy/classify_test.go create mode 100644 tools/stack-upgrade-policy/config.go create mode 100644 tools/stack-upgrade-policy/config_test.go create mode 100644 tools/stack-upgrade-policy/decide.go create mode 100644 tools/stack-upgrade-policy/decide_test.go create mode 100644 tools/stack-upgrade-policy/evidence.go create mode 100644 tools/stack-upgrade-policy/evidence_test.go create mode 100644 tools/stack-upgrade-policy/go.mod create mode 100644 tools/stack-upgrade-policy/main.go create mode 100644 tools/stack-upgrade-policy/main_test.go diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 447735dee..48c23e90d 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -187,6 +187,43 @@ jobs: - name: Build, vet, and test the Go modules under tools/ run: tools/ci/check-go-tools + stack-upgrade-policy: + name: stack upgrade policy + # Only a pull request has a baseline to measure a proposed bump against. On + # a push to main the candidate and the baseline are the same commit. + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + stack: + - nvcf-self-managed-stack + - nvcf-compute-plane-stack + - nvcf-observability-stack + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + # The check diffs against the stack's last published release tag, so + # it needs the tags and the history behind them. + fetch-depth: 0 + fetch-tags: true + + - uses: actions/setup-go@v5 + with: + go-version-file: tools/go-toolchain/go.mod + + # Interpolations go through env, never into the script body: a ref name + # expanded inline is a shell injection. + - name: Check migrations against the proposed version bump + env: + BASE_REF: ${{ github.base_ref }} + STACK: ${{ matrix.stack }} + run: | + bump="$(tools/ci/release-bump-type for-branch "origin/${BASE_REF}")" + echo "proposed bump: ${bump}" + tools/ci/check-stack-upgrade-policy --stack "${STACK}" --bump "${bump}" + github-release-helper: name: GitHub release helper runs-on: ubuntu-latest diff --git a/tools/ci/check-stack-upgrade-policy b/tools/ci/check-stack-upgrade-policy new file mode 100755 index 000000000..4c50b0af9 --- /dev/null +++ b/tools/ci/check-stack-upgrade-policy @@ -0,0 +1,33 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Stable CI entrypoint for the Go tool in tools/stack-upgrade-policy. +# +# The wrapper exists for the same two reasons tools/ci/chart-service-edge does. +# +# The repository root. `go run -C ` leaves the process running with that +# directory as its working directory, so the tool cannot find the release +# metadata or the git history 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. This tool distinguishes 1 (policy violation or +# error) from 2 (bad invocation), so collapsing them would be a trap. +# +# Run the tests with: go test -C tools/stack-upgrade-policy ./... +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-upgrade-policy" -o "${bin_dir}/stack-upgrade-policy" . + +# 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-upgrade-policy" --root "${repo_root}" "$@" +status=$? +set -e +exit "${status}" diff --git a/tools/ci/github-release-subprojects.json b/tools/ci/github-release-subprojects.json index 19a647bf2..1e88d7fe9 100644 --- a/tools/ci/github-release-subprojects.json +++ b/tools/ci/github-release-subprojects.json @@ -42,6 +42,7 @@ { "id": "nvcf-self-managed-stack", "path": "deploy/stacks/self-managed", + "migration_paths": ["migrations/cassandra", "migrations/openbao"], "service_name": "nvcf-self-managed-stack", "tag_format": "deploy/stacks/self-managed/v${version}", "version_file": "VERSION", diff --git a/tools/stack-upgrade-policy/.gitignore b/tools/stack-upgrade-policy/.gitignore new file mode 100644 index 000000000..920c6a71a --- /dev/null +++ b/tools/stack-upgrade-policy/.gitignore @@ -0,0 +1,2 @@ +# go build ./... drops the binary here; it must never be committed. +/stack-upgrade-policy diff --git a/tools/stack-upgrade-policy/classify.go b/tools/stack-upgrade-policy/classify.go new file mode 100644 index 000000000..d29a2fd29 --- /dev/null +++ b/tools/stack-upgrade-policy/classify.go @@ -0,0 +1,57 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "regexp" + "strings" +) + +// Class is what a migration does to data that already exists. +type Class int + +const ( + // Additive migrations only add schema. A cluster that skipped every stack + // version between two points still arrives at the right schema, because + // golang-migrate applies the ordered set regardless of how far behind the + // cluster was. + Additive Class = iota + // Destructive migrations remove a table, type, or column, or delete rows. + // Skipping versions is still safe for the schema itself, but the drop is + // unrecoverable without a restore, and it constrains deployment order: + // the migration must land before the services that read what it removes. + Destructive +) + +func (c Class) String() string { + if c == Destructive { + return "Destructive" + } + return "Additive" +} + +var destructive = regexp.MustCompile(`(?i)\b(DROP|TRUNCATE|DELETE)\b`) + +// Classify reports what a CQL migration does. Comments are stripped first: +// migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql documents a +// "high-churn write/delete workload" above a CREATE TABLE, and a classifier +// that reads raw text calls that destructive. +func Classify(sql string) Class { + if destructive.MatchString(stripComments(sql)) { + return Destructive + } + return Additive +} + +func stripComments(sql string) string { + var b strings.Builder + for line := range strings.SplitSeq(sql, "\n") { + if i := strings.Index(line, "--"); i >= 0 { + line = line[:i] + } + b.WriteString(line) + b.WriteByte('\n') + } + return b.String() +} diff --git a/tools/stack-upgrade-policy/classify_test.go b/tools/stack-upgrade-policy/classify_test.go new file mode 100644 index 000000000..a842c05ea --- /dev/null +++ b/tools/stack-upgrade-policy/classify_test.go @@ -0,0 +1,46 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "testing" + +func TestClassifyAddColumnIsAdditive(t *testing.T) { + sql := "ALTER TABLE nvcf_api.functions_v3 ADD llm_config frozen;" + if got := Classify(sql); got != Additive { + t.Fatalf("Classify() = %v, want Additive", got) + } +} + +func TestClassifyDropTableIsDestructive(t *testing.T) { + sql := "DROP TABLE IF EXISTS nvcf_autoscaler.recently_invoked_functions_history;" + if got := Classify(sql); got != Destructive { + t.Fatalf("Classify() = %v, want Destructive", got) + } +} + +func TestClassifyAlterTableDropIsDestructive(t *testing.T) { + sql := "ALTER TABLE IF EXISTS nvcf_api.functions_deployment_v2 DROP IF EXISTS gpu_specs;" + if got := Classify(sql); got != Destructive { + t.Fatalf("Classify() = %v, want Destructive", got) + } +} + +func TestClassifyDropTypeIsDestructive(t *testing.T) { + sql := "DROP TYPE IF EXISTS nvct_api.health_udt;" + if got := Classify(sql); got != Destructive { + t.Fatalf("Classify() = %v, want Destructive", got) + } +} + +// migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql carries the +// comment "Tuned for high-churn write/delete workload: UCS + short gc_grace." +// A classifier that greps the raw text calls that table creation destructive. +func TestClassifyIgnoresKeywordsInComments(t *testing.T) { + sql := `-- Tuned for high-churn write/delete workload: UCS + short gc_grace. +-- We may DROP this table later. +CREATE TABLE IF NOT EXISTS nvcf_api.functions_v3 (id uuid PRIMARY KEY);` + if got := Classify(sql); got != Additive { + t.Fatalf("Classify() = %v, want Additive", got) + } +} diff --git a/tools/stack-upgrade-policy/config.go b/tools/stack-upgrade-policy/config.go new file mode 100644 index 000000000..84b7a1748 --- /dev/null +++ b/tools/stack-upgrade-policy/config.go @@ -0,0 +1,54 @@ +// 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" + "strings" +) + +// MetadataPath is the release metadata every subproject is already registered +// in. The migration paths live here rather than in a new per-stack file so +// that a stack has one place describing how it releases, not two. +const MetadataPath = "tools/ci/github-release-subprojects.json" + +const stackPrefix = "deploy/stacks/" + +// Stack is one released stack bundle and the paths whose migrations ship with +// it. A stack that declares no migration paths is a stack that ships no +// schema, and the check is a no-op for it. +type Stack struct { + ID string `json:"id"` + Path string `json:"path"` + MigrationPaths []string `json:"migration_paths"` +} + +type metadata struct { + Services []Stack `json:"services"` +} + +// LoadStack finds a stack by its release-metadata id. +func LoadStack(root, id string) (Stack, error) { + raw, err := os.ReadFile(filepath.Join(root, MetadataPath)) + if err != nil { + return Stack{}, err + } + var meta metadata + if err := json.Unmarshal(raw, &meta); err != nil { + return Stack{}, fmt.Errorf("%s: %w", MetadataPath, err) + } + for _, s := range meta.Services { + if s.ID != id { + continue + } + if !strings.HasPrefix(s.Path, stackPrefix) { + return Stack{}, fmt.Errorf("%q is not a stack: its path %q is not under %s", id, s.Path, stackPrefix) + } + return s, nil + } + return Stack{}, fmt.Errorf("no subproject %q in %s", id, MetadataPath) +} diff --git a/tools/stack-upgrade-policy/config_test.go b/tools/stack-upgrade-policy/config_test.go new file mode 100644 index 000000000..ee9d95282 --- /dev/null +++ b/tools/stack-upgrade-policy/config_test.go @@ -0,0 +1,74 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +const metadataFixture = `{ + "version": 1, + "services": [ + {"id": "nvcf-cli", "path": "src/clis/nvcf-cli"}, + {"id": "nvcf-self-managed-stack", "path": "deploy/stacks/self-managed", + "migration_paths": ["migrations/cassandra", "migrations/openbao"]}, + {"id": "nvcf-observability-stack", "path": "deploy/stacks/observability"} + ] +}` + +func metadataDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + p := filepath.Join(dir, MetadataPath) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(metadataFixture), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +func TestLoadStackReadsPathAndMigrationPaths(t *testing.T) { + s, err := LoadStack(metadataDir(t), "nvcf-self-managed-stack") + if err != nil { + t.Fatal(err) + } + if s.Path != "deploy/stacks/self-managed" { + t.Errorf("Path = %q", s.Path) + } + if len(s.MigrationPaths) != 2 { + t.Errorf("MigrationPaths = %v, want two", s.MigrationPaths) + } +} + +// A stack that declares no migration paths is not an error: the compute-plane +// and observability stacks ship no schema, so the check is a no-op for them +// until they need it. +func TestLoadStackAllowsNoMigrationPaths(t *testing.T) { + s, err := LoadStack(metadataDir(t), "nvcf-observability-stack") + if err != nil { + t.Fatal(err) + } + if len(s.MigrationPaths) != 0 { + t.Errorf("MigrationPaths = %v, want none", s.MigrationPaths) + } +} + +func TestLoadStackRejectsUnknownID(t *testing.T) { + if _, err := LoadStack(metadataDir(t), "no-such-stack"); err == nil { + t.Fatal("err = nil, want an error naming the unknown stack") + } +} + +// Guards against pointing the check at a service that is not a stack, which +// would silently compare against a tag series that has nothing to do with a +// published bundle. +func TestLoadStackRejectsANonStackService(t *testing.T) { + if _, err := LoadStack(metadataDir(t), "nvcf-cli"); err == nil { + t.Fatal("err = nil, want an error for a non-stack service") + } +} diff --git a/tools/stack-upgrade-policy/decide.go b/tools/stack-upgrade-policy/decide.go new file mode 100644 index 000000000..c9e2233ae --- /dev/null +++ b/tools/stack-upgrade-policy/decide.go @@ -0,0 +1,112 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "strings" +) + +// Status is what happened to a migration file between two points in history. +type Status int + +const ( + Added Status = iota + Modified + Deleted +) + +func (s Status) String() string { + switch s { + case Added: + return "added" + case Modified: + return "modified" + default: + return "deleted" + } +} + +// Change is one migration file and what happened to it. +type Change struct { + Path string + Status Status + Class Class +} + +// Evidence is every migration change a release candidate introduces since the +// stack's last published release. +type Evidence struct { + Changes []Change +} + +// Bump is the semver step a release candidate is proposing. +type Bump int + +const ( + NoBump Bump = iota + Patch + Minor + Major +) + +func (b Bump) String() string { + switch b { + case Major: + return "major" + case Minor: + return "minor" + case Patch: + return "patch" + default: + return "none" + } +} + +// Decision is the outcome for one release candidate. +type Decision struct { + // Qualifying reports whether the candidate contains a change a customer + // cannot safely skip past. + Qualifying bool + OK bool + Reason string +} + +// Decide compares migration evidence against the proposed version bump. +// +// Additive migrations never qualify. golang-migrate applies the ordered set +// per keyspace, so a cluster many versions behind still arrives at the right +// schema; nothing about skipping versions breaks it. +// +// A destructive migration or a deleted migration file does qualify. A drop +// cannot be undone without a restore, and a deleted migration is how a +// compatibility bridge stops shipping — once it is gone, a cluster arriving +// later has no way to run it. Both need the major boundary that tells a +// customer not to skip this release. +func Decide(ev Evidence, bump Bump) Decision { + var qualifying []Change + for _, c := range ev.Changes { + if c.Status == Deleted || c.Class == Destructive { + qualifying = append(qualifying, c) + } + } + if len(qualifying) == 0 { + return Decision{OK: true} + } + if bump == Major { + return Decision{Qualifying: true, OK: true} + } + + var b strings.Builder + fmt.Fprintf(&b, "proposed bump is %s, but this candidate contains %d change(s) a customer cannot skip past:\n", bump, len(qualifying)) + for _, c := range qualifying { + if c.Status == Deleted { + fmt.Fprintf(&b, " %s (deleted: a cluster arriving later cannot run it)\n", c.Path) + continue + } + fmt.Fprintf(&b, " %s (%s, destructive)\n", c.Path, c.Status) + } + b.WriteString("\nEither cut this as a major release, or keep the removed migration in place until the next major boundary.") + return Decision{Qualifying: true, Reason: b.String()} +} diff --git a/tools/stack-upgrade-policy/decide_test.go b/tools/stack-upgrade-policy/decide_test.go new file mode 100644 index 000000000..2d2d74bdb --- /dev/null +++ b/tools/stack-upgrade-policy/decide_test.go @@ -0,0 +1,84 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "testing" + +func TestDecideNoMigrationChangesPasses(t *testing.T) { + d := Decide(Evidence{}, Minor) + if !d.OK { + t.Fatalf("OK = false, want true (reason: %s)", d.Reason) + } + if d.Qualifying { + t.Fatal("Qualifying = true, want false") + } +} + +func TestDecideAdditiveOnlyPassesOnMinor(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/sis_api/09_add_reservation_backup_disabled.up.sql", Status: Added, Class: Additive}, + }} + d := Decide(ev, Minor) + if !d.OK { + t.Fatalf("OK = false, want true (reason: %s)", d.Reason) + } + if d.Qualifying { + t.Fatal("Qualifying = true, want false") + } +} + +func TestDecideDestructiveWithoutMajorFails(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/nvcf_api/07_drop_gpu_spec.up.sql", Status: Added, Class: Destructive}, + }} + d := Decide(ev, Minor) + if d.OK { + t.Fatal("OK = true, want false") + } + if !d.Qualifying { + t.Fatal("Qualifying = false, want true") + } + if d.Reason == "" { + t.Fatal("Reason is empty, want an explanation of what to do") + } +} + +func TestDecideDestructiveWithMajorPasses(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/nvcf_api/07_drop_gpu_spec.up.sql", Status: Added, Class: Destructive}, + }} + d := Decide(ev, Major) + if !d.OK { + t.Fatalf("OK = false, want true (reason: %s)", d.Reason) + } + if !d.Qualifying { + t.Fatal("Qualifying = false, want true") + } +} + +// A deleted migration is how the compatibility bridge disappears: the backfill +// task stops shipping, so a cluster arriving later has no way to run it. That +// is the change that forces a customer stop, and it is invisible to a +// classifier that only reads added files. +func TestDecideDeletedMigrationWithoutMajorFails(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/nvcf_api/05_add_model_specs.up.sql", Status: Deleted}, + }} + d := Decide(ev, Patch) + if d.OK { + t.Fatal("OK = true, want false") + } + if !d.Qualifying { + t.Fatal("Qualifying = false, want true") + } +} + +func TestDecideDeletedMigrationWithMajorPasses(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/nvcf_api/05_add_model_specs.up.sql", Status: Deleted}, + }} + if d := Decide(ev, Major); !d.OK { + t.Fatalf("OK = false, want true (reason: %s)", d.Reason) + } +} diff --git a/tools/stack-upgrade-policy/evidence.go b/tools/stack-upgrade-policy/evidence.go new file mode 100644 index 000000000..4a07fc748 --- /dev/null +++ b/tools/stack-upgrade-policy/evidence.go @@ -0,0 +1,88 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os/exec" + "strings" +) + +func runGit(root string, args ...string) (string, error) { + cmd := exec.Command("git", args...) + cmd.Dir = root + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return string(out), nil +} + +// LatestReleaseTag returns the highest published tag for a stack, for example +// deploy/stacks/self-managed/v1.0.0. Prereleases are skipped: they are not a +// version any customer was told to install, so they are not the baseline a +// release candidate is measured against. +func LatestReleaseTag(root, stackPath string) (string, error) { + out, err := runGit(root, "tag", "--list", stackPath+"/v*", "--sort=-v:refname") + if err != nil { + return "", err + } + for _, tag := range strings.Split(out, "\n") { + tag = strings.TrimSpace(tag) + if tag == "" { + continue + } + if strings.Contains(tag[strings.LastIndex(tag, "/")+1:], "-") { + continue + } + return tag, nil + } + return "", fmt.Errorf("no published release tag under %s", stackPath) +} + +// GatherEvidence reports every migration file that changed between a stack's +// last published release and HEAD. +// +// The comparison is against the release tag rather than against the pinned +// migrations image, because the stack does not pin that image: the +// cassandra.migrations.image block in global.yaml.gotmpl only emits a tag when +// an operator supplies one. Until it does, "what landed on main since the last +// stack release" is the closest available answer to what the bundle ships. +func GatherEvidence(root, baseRef string, paths []string) (Evidence, error) { + if len(paths) == 0 { + return Evidence{}, nil + } + args := append([]string{"diff", "--name-status", baseRef + "..HEAD", "--"}, paths...) + out, err := runGit(root, args...) + if err != nil { + return Evidence{}, err + } + + var ev Evidence + for _, line := range strings.Split(out, "\n") { + fields := strings.Split(strings.TrimSpace(line), "\t") + if len(fields) < 2 || fields[0] == "" { + continue + } + path := fields[len(fields)-1] + if !strings.HasSuffix(path, ".sql") { + continue + } + switch fields[0][0] { + case 'D': + ev.Changes = append(ev.Changes, Change{Path: path, Status: Deleted}) + case 'A', 'M', 'R', 'C': + status := Modified + if fields[0][0] == 'A' { + status = Added + } + sql, err := runGit(root, "show", "HEAD:"+path) + if err != nil { + return Evidence{}, err + } + ev.Changes = append(ev.Changes, Change{Path: path, Status: status, Class: Classify(sql)}) + } + } + return ev, nil +} diff --git a/tools/stack-upgrade-policy/evidence_test.go b/tools/stack-upgrade-policy/evidence_test.go new file mode 100644 index 000000000..fe7d6a6a8 --- /dev/null +++ b/tools/stack-upgrade-policy/evidence_test.go @@ -0,0 +1,143 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +func git(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.com", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.com", + ) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return string(out) +} + +func write(t *testing.T, dir, rel, content string) { + t.Helper() + p := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// fixture builds a repo with one released stack tag and returns its root. +func fixture(t *testing.T) string { + t.Helper() + dir := t.TempDir() + git(t, dir, "init", "-q", "-b", "main") + write(t, dir, "migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql", + "CREATE TABLE IF NOT EXISTS nvcf_api.functions_v3 (id uuid PRIMARY KEY);") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "base") + git(t, dir, "tag", "deploy/stacks/self-managed/v1.0.0") + return dir +} + +func TestLatestReleaseTagPicksHighestSemver(t *testing.T) { + dir := fixture(t) + git(t, dir, "tag", "deploy/stacks/self-managed/v0.20.7") + git(t, dir, "tag", "deploy/stacks/self-managed/v1.0.0-rc.1") + + got, err := LatestReleaseTag(dir, "deploy/stacks/self-managed") + if err != nil { + t.Fatal(err) + } + if want := "deploy/stacks/self-managed/v1.0.0"; got != want { + t.Fatalf("LatestReleaseTag() = %q, want %q", got, want) + } +} + +func TestGatherEvidenceReportsNothingWhenNoMigrationsChanged(t *testing.T) { + dir := fixture(t) + write(t, dir, "src/foo/main.go", "package main") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "unrelated") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + if len(ev.Changes) != 0 { + t.Fatalf("Changes = %v, want none", ev.Changes) + } +} + +func TestGatherEvidenceClassifiesAnAddedDestructiveMigration(t *testing.T) { + dir := fixture(t) + write(t, dir, "migrations/cassandra/keyspaces/nvcf_api/07_drop_gpu_spec.up.sql", + "ALTER TABLE IF EXISTS nvcf_api.functions_deployment_v2 DROP IF EXISTS gpu_specs;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "drop") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + if len(ev.Changes) != 1 { + t.Fatalf("Changes = %v, want exactly one", ev.Changes) + } + c := ev.Changes[0] + if c.Status != Added { + t.Errorf("Status = %v, want Added", c.Status) + } + if c.Class != Destructive { + t.Errorf("Class = %v, want Destructive", c.Class) + } +} + +func TestGatherEvidenceReportsADeletedMigration(t *testing.T) { + dir := fixture(t) + if err := os.Remove(filepath.Join(dir, "migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql")); err != nil { + t.Fatal(err) + } + git(t, dir, "add", "-A") + git(t, dir, "commit", "-qm", "remove the bridge") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + if len(ev.Changes) != 1 { + t.Fatalf("Changes = %v, want exactly one", ev.Changes) + } + if ev.Changes[0].Status != Deleted { + t.Fatalf("Status = %v, want Deleted", ev.Changes[0].Status) + } +} + +// An additive migration added on top of a released tag must not be reported as +// qualifying, or every ordinary schema addition becomes a customer stop. +func TestGatherEvidenceClassifiesAnAddedAdditiveMigration(t *testing.T) { + dir := fixture(t) + write(t, dir, "migrations/cassandra/keyspaces/sis_api/09_add_reservation_backup_disabled.up.sql", + "ALTER TABLE sis_api.reservations ADD backup_disabled boolean;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "add column") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + if len(ev.Changes) != 1 { + t.Fatalf("Changes = %v, want exactly one", ev.Changes) + } + if ev.Changes[0].Class != Additive { + t.Fatalf("Class = %v, want Additive", ev.Changes[0].Class) + } +} diff --git a/tools/stack-upgrade-policy/go.mod b/tools/stack-upgrade-policy/go.mod new file mode 100644 index 000000000..db4edb34d --- /dev/null +++ b/tools/stack-upgrade-policy/go.mod @@ -0,0 +1,3 @@ +module stack-upgrade-policy + +go 1.26 diff --git a/tools/stack-upgrade-policy/main.go b/tools/stack-upgrade-policy/main.go new file mode 100644 index 000000000..49639025e --- /dev/null +++ b/tools/stack-upgrade-policy/main.go @@ -0,0 +1,150 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Command stack-upgrade-policy reports the schema migrations a stack release +// candidate introduces, and fails when they do not match the version bump the +// candidate is proposing. +// +// stack-upgrade-policy --stack --bump +// +// The upgrade contract this enforces is that a customer installs each stack +// major version in order. That rule is derived from the version numbers +// themselves, so nothing has to publish a catalog or maintain a floor field +// per release. What the rule needs in exchange is that a major boundary is +// actually cut whenever the candidate contains something a customer cannot +// safely skip past. +// +// Deciding that from a stack-pin diff is not possible by eye: the diff is a +// list of chart versions moving, and nothing in "1.5.3 -> 2.0.0" says a +// migration landed. In a monorepo the evidence is computable instead, which +// also means it cannot be forgotten the way a declaration written in a pull +// request two months earlier can. +// +// Additive migrations never qualify. golang-migrate applies the ordered set +// per keyspace, so a cluster many versions behind still arrives at the right +// schema. Destructive migrations and deleted migration files do qualify: a +// drop is unrecoverable without a restore, and a deleted migration is how a +// compatibility bridge stops shipping, leaving a cluster that arrives later +// with no way to run it. +package main + +import ( + "encoding/json" + "flag" + "fmt" + "io" + "os" + "strings" +) + +func main() { + stack := flag.String("stack", "", "release-metadata id of the stack, for example nvcf-self-managed-stack") + bump := flag.String("bump", "", "proposed bump as a conventional-commit type, as reported by tools/ci/release-bump-type") + root := flag.String("root", ".", "repository root") + asJSON := flag.Bool("json", false, "emit the evidence report as JSON") + flag.Parse() + + if *stack == "" || *bump == "" { + flag.Usage() + os.Exit(2) + } + code, err := run(*root, *stack, *bump, *asJSON, os.Stdout, os.Stderr) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + os.Exit(code) +} + +// ParseBump maps a conventional-commit type onto the semver step it causes. +// The input is what tools/ci/release-bump-type reports, so that this and +// semantic-release agree on what the candidate is proposing rather than +// deriving it twice from different places. +func ParseBump(commitType string) (Bump, error) { + switch { + case strings.HasSuffix(commitType, "!"): + return Major, nil + case commitType == "feat": + return Minor, nil + case commitType == "fix": + return Patch, nil + default: + return NoBump, fmt.Errorf("unrecognized commit type %q: expected feat!, feat, or fix from release-bump-type", commitType) + } +} + +type reportChange struct { + Path string `json:"path"` + Status string `json:"status"` + Class string `json:"class"` +} + +type report struct { + Stack string `json:"stack"` + BaseTag string `json:"base_tag"` + Bump string `json:"bump"` + Qualifying bool `json:"qualifying"` + OK bool `json:"ok"` + Changes []reportChange `json:"changes"` +} + +func run(root, stackID, bumpType string, asJSON bool, out, errOut io.Writer) (int, error) { + stack, err := LoadStack(root, stackID) + if err != nil { + return 1, err + } + bump, err := ParseBump(bumpType) + if err != nil { + return 1, err + } + if len(stack.MigrationPaths) == 0 { + fmt.Fprintf(out, "%s declares no migration paths; nothing to check.\n", stack.ID) + return 0, nil + } + + baseTag, err := LatestReleaseTag(root, stack.Path) + if err != nil { + return 1, err + } + ev, err := GatherEvidence(root, baseTag, stack.MigrationPaths) + if err != nil { + return 1, err + } + decision := Decide(ev, bump) + + rep := report{Stack: stack.ID, BaseTag: baseTag, Bump: bump.String(), Qualifying: decision.Qualifying, OK: decision.OK} + for _, c := range ev.Changes { + rc := reportChange{Path: c.Path, Status: c.Status.String(), Class: c.Class.String()} + if c.Status == Deleted { + rc.Class = "" + } + rep.Changes = append(rep.Changes, rc) + } + + if asJSON { + enc := json.NewEncoder(out) + enc.SetIndent("", " ") + if err := enc.Encode(rep); err != nil { + return 1, err + } + } else { + writeText(out, rep) + } + if !decision.OK { + fmt.Fprintln(errOut, decision.Reason) + return 1, nil + } + return 0, nil +} + +func writeText(out io.Writer, rep report) { + fmt.Fprintf(out, "%s: %d migration change(s) since %s (proposed bump: %s)\n", + rep.Stack, len(rep.Changes), rep.BaseTag, rep.Bump) + for _, c := range rep.Changes { + if c.Class == "" { + fmt.Fprintf(out, " %-9s %s\n", c.Status, c.Path) + continue + } + fmt.Fprintf(out, " %-9s %-12s %s\n", c.Status, strings.ToLower(c.Class), c.Path) + } +} diff --git a/tools/stack-upgrade-policy/main_test.go b/tools/stack-upgrade-policy/main_test.go new file mode 100644 index 000000000..d58bef277 --- /dev/null +++ b/tools/stack-upgrade-policy/main_test.go @@ -0,0 +1,147 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "strings" + "testing" +) + +// release-bump-type reports the conventional-commit type, not a semver word, +// so that is what this has to accept. +func TestParseBumpAcceptsReleaseBumpTypeOutput(t *testing.T) { + for in, want := range map[string]Bump{"feat!": Major, "fix!": Major, "feat": Minor, "fix": Patch} { + got, err := ParseBump(in) + if err != nil { + t.Fatalf("ParseBump(%q): %v", in, err) + } + if got != want { + t.Errorf("ParseBump(%q) = %v, want %v", in, got, want) + } + } +} + +func TestParseBumpRejectsGarbage(t *testing.T) { + if _, err := ParseBump("probably-fine"); err == nil { + t.Fatal("err = nil, want an error rather than a silent Patch") + } +} + +// fullFixture is a repo carrying both the release metadata and a published +// stack tag, so run() can be exercised end to end. +func fullFixture(t *testing.T) string { + t.Helper() + dir := fixture(t) + write(t, dir, MetadataPath, `{"version":1,"services":[ + {"id":"nvcf-self-managed-stack","path":"deploy/stacks/self-managed", + "migration_paths":["migrations/cassandra"]}]}`) + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "metadata") + return dir +} + +func TestRunFailsOnADestructiveMigrationWithoutAMajorBump(t *testing.T) { + dir := fullFixture(t) + write(t, dir, "migrations/cassandra/keyspaces/nvct_api/05_drop_health_info.up.sql", + "ALTER TABLE nvct_api.tasks_v2 DROP IF EXISTS health_info;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "drop health_info") + + var out, errOut bytes.Buffer + code, err := run(dir, "nvcf-self-managed-stack", "feat", false, &out, &errOut) + if err != nil { + t.Fatal(err) + } + if code != 1 { + t.Fatalf("exit = %d, want 1\nstdout:\n%s\nstderr:\n%s", code, out.String(), errOut.String()) + } + if !strings.Contains(errOut.String(), "05_drop_health_info.up.sql") { + t.Fatalf("stderr does not name the offending file:\n%s", errOut.String()) + } +} + +func TestRunPassesTheSameChangeWithAMajorBump(t *testing.T) { + dir := fullFixture(t) + write(t, dir, "migrations/cassandra/keyspaces/nvct_api/05_drop_health_info.up.sql", + "ALTER TABLE nvct_api.tasks_v2 DROP IF EXISTS health_info;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "drop health_info") + + var out, errOut bytes.Buffer + code, err := run(dir, "nvcf-self-managed-stack", "feat!", false, &out, &errOut) + if err != nil { + t.Fatal(err) + } + if code != 0 { + t.Fatalf("exit = %d, want 0\nstderr:\n%s", code, errOut.String()) + } +} + +// The report is the reason this runs on every candidate, not only failing +// ones: a release cutter staring at a list of chart version bumps otherwise +// has no way to know a migration landed. +func TestRunReportsAdditiveMigrationsAndStillPasses(t *testing.T) { + dir := fullFixture(t) + write(t, dir, "migrations/cassandra/keyspaces/sis_api/09_add_reservation_backup_disabled.up.sql", + "ALTER TABLE sis_api.reservations ADD backup_disabled boolean;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "add column") + + var out, errOut bytes.Buffer + code, err := run(dir, "nvcf-self-managed-stack", "fix", false, &out, &errOut) + if err != nil { + t.Fatal(err) + } + if code != 0 { + t.Fatalf("exit = %d, want 0", code) + } + if !strings.Contains(out.String(), "09_add_reservation_backup_disabled.up.sql") { + t.Fatalf("report omits the additive migration:\n%s", out.String()) + } +} + +func TestRunIsANoOpForAStackWithNoMigrationPaths(t *testing.T) { + dir := fullFixture(t) + write(t, dir, MetadataPath, `{"version":1,"services":[ + {"id":"nvcf-observability-stack","path":"deploy/stacks/observability"}]}`) + write(t, dir, "migrations/cassandra/keyspaces/nvcf_api/07_drop_gpu_spec.up.sql", + "DROP TYPE IF EXISTS nvcf_api.gpu_spec_udt;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "unrelated drop") + git(t, dir, "tag", "deploy/stacks/observability/v0.3.0") + + var out, errOut bytes.Buffer + code, err := run(dir, "nvcf-observability-stack", "fix", false, &out, &errOut) + if err != nil { + t.Fatal(err) + } + if code != 0 { + t.Fatalf("exit = %d, want 0\nstderr:\n%s", code, errOut.String()) + } +} + +func TestRunEmitsJSON(t *testing.T) { + dir := fullFixture(t) + write(t, dir, "migrations/cassandra/keyspaces/sis_api/09_add.up.sql", + "ALTER TABLE sis_api.reservations ADD backup_disabled boolean;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "add column") + + var out, errOut bytes.Buffer + if _, err := run(dir, "nvcf-self-managed-stack", "fix", true, &out, &errOut); err != nil { + t.Fatal(err) + } + if !strings.Contains(out.String(), `"class": "Additive"`) { + t.Fatalf("json output missing class field:\n%s", out.String()) + } +} + +func TestRunRejectsAnUnknownStack(t *testing.T) { + dir := fullFixture(t) + var out, errOut bytes.Buffer + if _, err := run(dir, "not-a-stack", "fix", false, &out, &errOut); err == nil { + t.Fatal("err = nil, want an error naming the unknown stack") + } +} From f27c68275c6f5d2cf3ae4c5600a0becba514b643 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Mon, 21 Sep 2026 15:37:12 -0700 Subject: [PATCH 2/5] fix(stack): close three gaps CodeRabbit found in the upgrade-policy check Comment stripping only handled `--`, so a DROP inside a block comment or a CQL string literal read as executable. Both fail in the over-strict direction: they would have blocked a legitimate non-major release over SQL the database never runs. stripComments now scans line comments, block comments and quoted strings, emitting a space for each so removing one cannot weld two tokens into a third. The doubled-quote escape is handled, without which the scanner stays inside a string and misses every statement after it. A rename recorded only the new path, as Modified. Renaming a migration away therefore produced no Deleted change and Decide would permit a non-major release even though the old migration no longer ships. Renames now emit a delete for the old path and a change for the new one. Copies do not, because a copy leaves its source in place. The early return for a stack with no migration paths printed text before the JSON branch, so --json emitted non-JSON for the compute-plane and observability stacks. Both paths now go through one emit(), and the report carries `checked` so a consumer can tell "no schema shipped" from "no changes". None of the three is reachable from the current corpus: no migration uses a block comment, a string literal, or a rename. They were all live paths into a wrong answer. Co-Authored-By: Claude Opus 5 (1M context) --- tools/stack-upgrade-policy/classify.go | 49 ++++++++++++++++++-- tools/stack-upgrade-policy/classify_test.go | 30 ++++++++++++ tools/stack-upgrade-policy/evidence.go | 51 +++++++++++++++------ tools/stack-upgrade-policy/evidence_test.go | 33 +++++++++++++ tools/stack-upgrade-policy/main.go | 29 +++++++----- tools/stack-upgrade-policy/main_test.go | 24 ++++++++++ 6 files changed, 187 insertions(+), 29 deletions(-) diff --git a/tools/stack-upgrade-policy/classify.go b/tools/stack-upgrade-policy/classify.go index d29a2fd29..a4029536d 100644 --- a/tools/stack-upgrade-policy/classify.go +++ b/tools/stack-upgrade-policy/classify.go @@ -44,14 +44,53 @@ func Classify(sql string) Class { return Additive } +// stripComments blanks out everything CQL does not execute: line comments, +// block comments, and single-quoted strings. Keyword matching runs on what is +// left, so a DROP mentioned in a comment or a table's comment option is not +// mistaken for one the database will perform. +// +// Stripped regions become a space rather than nothing, so that removing a +// comment between two tokens cannot weld them into a third. func stripComments(sql string) string { var b strings.Builder - for line := range strings.SplitSeq(sql, "\n") { - if i := strings.Index(line, "--"); i >= 0 { - line = line[:i] + for i := 0; i < len(sql); { + switch { + case strings.HasPrefix(sql[i:], "--"): + end := strings.IndexByte(sql[i:], '\n') + if end < 0 { + i = len(sql) + } else { + i += end + } + b.WriteByte(' ') + case strings.HasPrefix(sql[i:], "/*"): + end := strings.Index(sql[i+2:], "*/") + if end < 0 { + i = len(sql) + } else { + i += 2 + end + 2 + } + b.WriteByte(' ') + case sql[i] == '\'': + i++ + for i < len(sql) { + if sql[i] != '\'' { + i++ + continue + } + // '' is an escaped quote inside the string, not its end. + if i+1 < len(sql) && sql[i+1] == '\'' { + i += 2 + continue + } + i++ + break + } + b.WriteByte(' ') + default: + b.WriteByte(sql[i]) + i++ } - b.WriteString(line) - b.WriteByte('\n') } return b.String() } diff --git a/tools/stack-upgrade-policy/classify_test.go b/tools/stack-upgrade-policy/classify_test.go index a842c05ea..429d99a2e 100644 --- a/tools/stack-upgrade-policy/classify_test.go +++ b/tools/stack-upgrade-policy/classify_test.go @@ -44,3 +44,33 @@ CREATE TABLE IF NOT EXISTS nvcf_api.functions_v3 (id uuid PRIMARY KEY);` t.Fatalf("Classify() = %v, want Additive", got) } } + +// CodeRabbit on #1983: stripComments only handled `--`, so a block comment +// containing DROP made an additive migration look destructive and would have +// failed a legitimate non-major release. +func TestClassifyIgnoresKeywordsInBlockComments(t *testing.T) { + sql := `/* DROP TABLE old_data; superseded, see NVCF-1234 */ +CREATE TABLE IF NOT EXISTS nvcf_api.new_data (id uuid PRIMARY KEY);` + if got := Classify(sql); got != Additive { + t.Fatalf("Classify() = %v, want Additive", got) + } +} + +// CQL carries table options as string literals, and a table whose comment +// mentions dropping rows is not a destructive migration. +func TestClassifyIgnoresKeywordsInStringLiterals(t *testing.T) { + sql := "CREATE TABLE nvcf_api.t (id uuid PRIMARY KEY) WITH comment = 'we never delete or drop here';" + if got := Classify(sql); got != Additive { + t.Fatalf("Classify() = %v, want Additive", got) + } +} + +// The doubled-quote escape must not leave the scanner stuck inside a string, +// or every statement after one would be skipped and a real DROP missed. +func TestClassifyHandlesEscapedQuotesAndStillSeesRealDrops(t *testing.T) { + sql := `CREATE TABLE nvcf_api.t (id uuid PRIMARY KEY) WITH comment = 'it''s fine'; +DROP TABLE IF EXISTS nvcf_api.old;` + if got := Classify(sql); got != Destructive { + t.Fatalf("Classify() = %v, want Destructive", got) + } +} diff --git a/tools/stack-upgrade-policy/evidence.go b/tools/stack-upgrade-policy/evidence.go index 4a07fc748..eda783045 100644 --- a/tools/stack-upgrade-policy/evidence.go +++ b/tools/stack-upgrade-policy/evidence.go @@ -60,28 +60,53 @@ func GatherEvidence(root, baseRef string, paths []string) (Evidence, error) { } var ev Evidence + add := func(path string, status Status) error { + if !strings.HasSuffix(path, ".sql") { + return nil + } + if status == Deleted { + ev.Changes = append(ev.Changes, Change{Path: path, Status: Deleted}) + return nil + } + sql, err := runGit(root, "show", "HEAD:"+path) + if err != nil { + return err + } + ev.Changes = append(ev.Changes, Change{Path: path, Status: status, Class: Classify(sql)}) + return nil + } + for _, line := range strings.Split(out, "\n") { fields := strings.Split(strings.TrimSpace(line), "\t") if len(fields) < 2 || fields[0] == "" { continue } - path := fields[len(fields)-1] - if !strings.HasSuffix(path, ".sql") { - continue - } + var err error switch fields[0][0] { case 'D': - ev.Changes = append(ev.Changes, Change{Path: path, Status: Deleted}) - case 'A', 'M', 'R', 'C': - status := Modified - if fields[0][0] == 'A' { - status = Added + err = add(fields[1], Deleted) + case 'R': + // A rename stops shipping the old path. Recording only the new one + // would hide a migration being renamed away. + if len(fields) < 3 { + continue + } + if err = add(fields[1], Deleted); err == nil { + err = add(fields[2], Modified) } - sql, err := runGit(root, "show", "HEAD:"+path) - if err != nil { - return Evidence{}, err + case 'C': + // A copy leaves its source in place, so nothing is deleted. + if len(fields) < 3 { + continue } - ev.Changes = append(ev.Changes, Change{Path: path, Status: status, Class: Classify(sql)}) + err = add(fields[2], Added) + case 'A': + err = add(fields[1], Added) + case 'M': + err = add(fields[1], Modified) + } + if err != nil { + return Evidence{}, err } } return ev, nil diff --git a/tools/stack-upgrade-policy/evidence_test.go b/tools/stack-upgrade-policy/evidence_test.go index fe7d6a6a8..33dc8c804 100644 --- a/tools/stack-upgrade-policy/evidence_test.go +++ b/tools/stack-upgrade-policy/evidence_test.go @@ -141,3 +141,36 @@ func TestGatherEvidenceClassifiesAnAddedAdditiveMigration(t *testing.T) { t.Fatalf("Class = %v, want Additive", ev.Changes[0].Class) } } + +// CodeRabbit on #1983: a rename was recorded only as Modified on the new path, +// so renaming a migration away produced no Deleted change and Decide would +// permit a non-major release even though the old migration no longer ships. +func TestGatherEvidenceTreatsARenameAsDeletingTheOldPath(t *testing.T) { + dir := fixture(t) + git(t, dir, "mv", + "migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql", + "migrations/cassandra/keyspaces/nvcf_api/03_init_tables_renamed.up.sql") + git(t, dir, "commit", "-qm", "rename the migration") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + var deleted, present int + for _, c := range ev.Changes { + if c.Status == Deleted { + deleted++ + if c.Path != "migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql" { + t.Errorf("deleted path = %q, want the original name", c.Path) + } + continue + } + present++ + } + if deleted != 1 { + t.Fatalf("deleted count = %d, want 1 (changes: %+v)", deleted, ev.Changes) + } + if present != 1 { + t.Fatalf("non-deleted count = %d, want 1 for the new path (changes: %+v)", present, ev.Changes) + } +} diff --git a/tools/stack-upgrade-policy/main.go b/tools/stack-upgrade-policy/main.go index 49639025e..9979bfa8b 100644 --- a/tools/stack-upgrade-policy/main.go +++ b/tools/stack-upgrade-policy/main.go @@ -85,6 +85,7 @@ type report struct { Bump string `json:"bump"` Qualifying bool `json:"qualifying"` OK bool `json:"ok"` + Checked bool `json:"checked"` Changes []reportChange `json:"changes"` } @@ -98,7 +99,7 @@ func run(root, stackID, bumpType string, asJSON bool, out, errOut io.Writer) (in return 1, err } if len(stack.MigrationPaths) == 0 { - fmt.Fprintf(out, "%s declares no migration paths; nothing to check.\n", stack.ID) + emit(out, asJSON, report{Stack: stack.ID, Bump: bump.String(), OK: true}) return 0, nil } @@ -112,7 +113,7 @@ func run(root, stackID, bumpType string, asJSON bool, out, errOut io.Writer) (in } decision := Decide(ev, bump) - rep := report{Stack: stack.ID, BaseTag: baseTag, Bump: bump.String(), Qualifying: decision.Qualifying, OK: decision.OK} + rep := report{Stack: stack.ID, BaseTag: baseTag, Bump: bump.String(), Qualifying: decision.Qualifying, OK: decision.OK, Checked: true} for _, c := range ev.Changes { rc := reportChange{Path: c.Path, Status: c.Status.String(), Class: c.Class.String()} if c.Status == Deleted { @@ -121,15 +122,7 @@ func run(root, stackID, bumpType string, asJSON bool, out, errOut io.Writer) (in rep.Changes = append(rep.Changes, rc) } - if asJSON { - enc := json.NewEncoder(out) - enc.SetIndent("", " ") - if err := enc.Encode(rep); err != nil { - return 1, err - } - } else { - writeText(out, rep) - } + emit(out, asJSON, rep) if !decision.OK { fmt.Fprintln(errOut, decision.Reason) return 1, nil @@ -137,7 +130,21 @@ func run(root, stackID, bumpType string, asJSON bool, out, errOut io.Writer) (in return 0, nil } +func emit(out io.Writer, asJSON bool, rep report) { + if asJSON { + enc := json.NewEncoder(out) + enc.SetIndent("", " ") + _ = enc.Encode(rep) + return + } + writeText(out, rep) +} + func writeText(out io.Writer, rep report) { + if !rep.Checked { + fmt.Fprintf(out, "%s declares no migration paths; nothing to check.\n", rep.Stack) + return + } fmt.Fprintf(out, "%s: %d migration change(s) since %s (proposed bump: %s)\n", rep.Stack, len(rep.Changes), rep.BaseTag, rep.Bump) for _, c := range rep.Changes { diff --git a/tools/stack-upgrade-policy/main_test.go b/tools/stack-upgrade-policy/main_test.go index d58bef277..6ab69f38b 100644 --- a/tools/stack-upgrade-policy/main_test.go +++ b/tools/stack-upgrade-policy/main_test.go @@ -5,6 +5,7 @@ package main import ( "bytes" + "encoding/json" "strings" "testing" ) @@ -145,3 +146,26 @@ func TestRunRejectsAnUnknownStack(t *testing.T) { t.Fatal("err = nil, want an error naming the unknown stack") } } + +// CodeRabbit on #1983: the no-migration-paths early return printed text before +// the JSON branch, so --json emitted non-JSON for the compute-plane and +// observability stacks and broke any consumer parsing it. +func TestRunEmitsJSONForAStackWithNoMigrationPaths(t *testing.T) { + dir := fullFixture(t) + write(t, dir, MetadataPath, `{"version":1,"services":[ + {"id":"nvcf-observability-stack","path":"deploy/stacks/observability"}]}`) + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "observability only") + + var out, errOut bytes.Buffer + if _, err := run(dir, "nvcf-observability-stack", "fix", true, &out, &errOut); err != nil { + t.Fatal(err) + } + var parsed map[string]any + if err := json.Unmarshal(out.Bytes(), &parsed); err != nil { + t.Fatalf("--json did not emit JSON: %v\ngot: %s", err, out.String()) + } + if parsed["stack"] != "nvcf-observability-stack" { + t.Errorf("stack = %v", parsed["stack"]) + } +} From b288d092e6726be9b297c2244c9e19abe9b250b5 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Mon, 21 Sep 2026 16:10:44 -0700 Subject: [PATCH 3/5] feat(self-managed): record the installed stack version in-cluster An upgrade has to know where it is starting from, and nothing in a cluster carries that today. Helm tracks chart versions per release; helmfile has no concept of the bundle's own version. So every cluster looks identical at upgrade time, and a gate has no basis to decide whether the jump it has been asked to make is one it can make safely. This writes a nvcf-upgrade-receipt ConfigMap naming the stack version that was installed. Only the writer ships here. The validating hook belongs in the release that first has something to validate against, and a writer cannot block an upgrade whereas a checker that misjudges can break every existing customer. Shipping it on the 1.0 line is the point. A cluster that reaches 2.0.0 with no receipt is indistinguishable from a 0.x install, which leaves a gate choosing between refusing everybody and checking nothing. Once any 1.x records a version, absence becomes a refusal that names a version the customer can actually install. The hook is post-install and post-upgrade, not pre-*: a receipt must not claim a version before that version has been applied. post-upgrade alone would skip the first cluster to receive this chart, because Helm runs post-install where no prior release exists -- precisely the clusters this exists for. Ordering is a stage boundary rather than a needs: edge. Under the helmfile version this stack pins, needs: places a release in a later DAG layer where it waits on every peer in the previous one, and one slow or failed peer silently skips it; see the admin-issuer-proxy comment in 02-core.yaml.gotmpl. A new last-numbered stage gets the ordering without that failure mode. The chart is local to the bundle rather than pulled from the registry, so the receipt works the moment this merges instead of waiting on a chart publish. Closes #1977 Co-Authored-By: Claude Opus 5 (1M context) --- deploy/stacks/self-managed/Makefile | 1 + .../charts/nvcf-upgrade-receipt/Chart.yaml | 8 +++ .../nvcf-upgrade-receipt/templates/job.yaml | 56 +++++++++++++++++++ .../nvcf-upgrade-receipt/templates/role.yaml | 18 ++++++ .../templates/rolebinding.yaml | 19 +++++++ .../templates/serviceaccount.yaml | 17 ++++++ .../charts/nvcf-upgrade-receipt/values.yaml | 19 +++++++ .../helmfile.d/04-upgrade-receipt.yaml.gotmpl | 30 ++++++++++ .../tests/upgrade-receipt-wiring.sh | 45 +++++++++++++++ 9 files changed, 213 insertions(+) create mode 100644 deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/Chart.yaml create mode 100644 deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/job.yaml create mode 100644 deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml create mode 100644 deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/rolebinding.yaml create mode 100644 deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/serviceaccount.yaml create mode 100644 deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/values.yaml create mode 100644 deploy/stacks/self-managed/helmfile.d/04-upgrade-receipt.yaml.gotmpl create mode 100755 deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh diff --git a/deploy/stacks/self-managed/Makefile b/deploy/stacks/self-managed/Makefile index ed014a117..c3ae1f41a 100644 --- a/deploy/stacks/self-managed/Makefile +++ b/deploy/stacks/self-managed/Makefile @@ -15,6 +15,7 @@ test: @tests/grpc-proxy-nats-endpoint.sh @tests/llm-pki-openbao-migration.sh @tests/api-keys-startup-probe.sh + @tests/upgrade-receipt-wiring.sh @tests/cassandra-openbao-credential-wiring.sh @tests/llm-pki-release.sh @tests/check-llm-pki-issuer.sh diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/Chart.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/Chart.yaml new file mode 100644 index 000000000..9abbe3869 --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/Chart.yaml @@ -0,0 +1,8 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +apiVersion: v2 +name: nvcf-upgrade-receipt +description: Records the installed NVCF stack version in-cluster so an upgrade can tell where it is starting from. +type: application +version: 0.1.0 +appVersion: "0.1.0" diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/job.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/job.yaml new file mode 100644 index 000000000..3b2e6848b --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/job.yaml @@ -0,0 +1,56 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +{{- $version := required "stackVersion is required: a receipt that names the wrong version is worse than no receipt" .Values.stackVersion }} +apiVersion: batch/v1 +kind: Job +metadata: + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} + annotations: + # post-* rather than pre-*: the receipt states what the cluster is running, + # so it must not be written until the release it describes has been applied. + # + # Both install and upgrade, because the first cluster to receive this chart + # has no prior release of it, and Helm runs post-install there rather than + # post-upgrade. Omitting post-install would leave exactly the clusters this + # exists for without a receipt. + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "0" + "helm.sh/hook-delete-policy": before-hook-creation +spec: + backoffLimit: 3 + ttlSecondsAfterFinished: 600 + template: + metadata: + name: {{ .Release.Name }} + spec: + restartPolicy: Never + serviceAccountName: {{ .Release.Name }} + {{- with .Values.imagePullSecrets }} + imagePullSecrets: +{{ toYaml . | indent 8 }} + {{- end }} + containers: + - name: receipt + image: "{{ with .Values.image.registry }}{{ . }}/{{ end }}{{ .Values.image.repository }}:{{ .Values.image.tag }}" + imagePullPolicy: {{ .Values.image.pullPolicy }} + env: + - name: RECEIPT_CONFIGMAP + value: {{ .Values.configMapName | quote }} + - name: INSTALLED_STACK_VERSION + value: {{ $version | quote }} + command: + - /bin/sh + - -c + - | + set -eu + # Rendered through apply rather than create so that the first + # install and every later upgrade take the same path. A receipt + # that only appears on a fresh install would be absent from + # exactly the clusters that are upgrading. + kubectl create configmap "${RECEIPT_CONFIGMAP}" \ + --from-literal=installed_stack_version="${INSTALLED_STACK_VERSION}" \ + --from-literal=recorded_at="$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ + --dry-run=client -o yaml \ + | kubectl apply -f - + echo "recorded installed_stack_version=${INSTALLED_STACK_VERSION} in ${RECEIPT_CONFIGMAP}" diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml new file mode 100644 index 000000000..1ad660c89 --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml @@ -0,0 +1,18 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} + annotations: + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "-5" + "helm.sh/hook-delete-policy": before-hook-creation +rules: + # get and patch are for the upgrade case where a receipt already exists; + # create is for the first install. Nothing here needs to read or write any + # other resource. + - apiGroups: [""] + resources: ["configmaps"] + verbs: ["get", "create", "patch"] diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/rolebinding.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/rolebinding.yaml new file mode 100644 index 000000000..4139a6c87 --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/rolebinding.yaml @@ -0,0 +1,19 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} + annotations: + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "-5" + "helm.sh/hook-delete-policy": before-hook-creation +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: {{ .Release.Name }} +subjects: + - kind: ServiceAccount + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/serviceaccount.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/serviceaccount.yaml new file mode 100644 index 000000000..dcdf2d5ef --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/serviceaccount.yaml @@ -0,0 +1,17 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +apiVersion: v1 +kind: ServiceAccount +metadata: + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} + annotations: + # Must exist before the Job that uses it, and survive long enough to be + # bound. A lower weight than the Job is what orders them. + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "-5" + "helm.sh/hook-delete-policy": before-hook-creation +{{- with .Values.imagePullSecrets }} +imagePullSecrets: +{{ toYaml . | indent 2 }} +{{- end }} diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/values.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/values.yaml new file mode 100644 index 000000000..87537031e --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/values.yaml @@ -0,0 +1,19 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +# The stack version this bundle installs. The Helmfile supplies it from the +# bundle's own VERSION file; there is no sensible default, so rendering fails +# rather than recording a version the cluster is not running. +stackVersion: "" + +image: + registry: "" + repository: alpine-k8s + tag: "1.33.1" + pullPolicy: IfNotPresent + +imagePullSecrets: [] + +# Name of the ConfigMap holding the receipt. An upgrade reads this to decide +# whether the jump it has been asked to make is one it can make safely. +configMapName: nvcf-upgrade-receipt diff --git a/deploy/stacks/self-managed/helmfile.d/04-upgrade-receipt.yaml.gotmpl b/deploy/stacks/self-managed/helmfile.d/04-upgrade-receipt.yaml.gotmpl new file mode 100644 index 000000000..a42e8f9fd --- /dev/null +++ b/deploy/stacks/self-managed/helmfile.d/04-upgrade-receipt.yaml.gotmpl @@ -0,0 +1,30 @@ +environments: + default: + values: + - ../environments/base.yaml + - ../environments/{{ requiredEnv "HELMFILE_ENV" }}.yaml + +--- + +{{- /* + A stage of its own, and the last one, so the receipt is written only after + every other release has been applied. Ordering is a stage boundary rather + than a needs: edge on purpose: under the helmfile version this stack pins, + needs: places a release in a later DAG layer where it waits on every peer in + the previous one, and a single slow or failed peer silently skips it. See the + admin-issuer-proxy comment in 02-core.yaml.gotmpl. +*/}} + +releases: + - name: upgrade-receipt + chart: ../charts/nvcf-upgrade-receipt + namespace: nvcf + values: + - stackVersion: {{ readFile "../VERSION" | trim | quote }} + image: + registry: {{ .Values.global.image.registry | quote }} + repository: {{ .Values.global.image.repository }}/alpine-k8s + {{- with .Values.global.imagePullSecrets }} + imagePullSecrets: + {{- toYaml . | nindent 10 }} + {{- end }} diff --git a/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh b/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh new file mode 100755 index 000000000..6161eeb9e --- /dev/null +++ b/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +# Test that the stack records the version it installed. +# +# An upgrade has to know where it is starting from, and nothing else in a +# cluster carries that: Helm tracks chart versions per release, and helmfile has +# no concept of the bundle's own version. Without this receipt every cluster +# looks identical to every other one at upgrade time. +# +# The assertions that matter are the hook kinds and the recorded version. A +# pre-* hook would claim a version before it was applied, and a post-upgrade +# hook alone would skip the first install of this chart, which is precisely the +# set of clusters that need a receipt written. +set -euo pipefail + +stack_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +expected_version="$(tr -d '[:space:]' < "$stack_dir/VERSION")" + +rendered="$(cd "$stack_dir" && HELMFILE_ENV=base helmfile \ + --file helmfile.d/04-upgrade-receipt.yaml.gotmpl template)" + +fail() { echo "FAIL: $1" >&2; exit 1; } + +grep -q 'kind: Job' <<<"$rendered" || fail "no Job rendered" +grep -q '"helm.sh/hook": post-install,post-upgrade' <<<"$rendered" \ + || fail "receipt must run on both install and upgrade, after the release it describes" +grep -q "value: \"${expected_version}\"" <<<"$rendered" \ + || fail "recorded version does not match VERSION (${expected_version})" +# The ConfigMap is created by the Job at run time, not rendered, so its name +# reaches the cluster as the env var the script reads. +grep -q 'value: "nvcf-upgrade-receipt"' <<<"$rendered" \ + || fail "receipt ConfigMap name is not the one an upgrade will read" + +for kind in ServiceAccount Role RoleBinding; do + grep -q "kind: ${kind}" <<<"$rendered" || fail "missing ${kind}; the Job cannot write the ConfigMap without it" +done +grep -qE '^\s+verbs: \["get", "create", "patch"\]' <<<"$rendered" \ + || fail "Role must allow get/create/patch on configmaps and nothing more" + +# The stage number is the ordering guarantee. needs: is deliberately not used +# here; see the comment in the stage file. +last_stage="$(ls "$stack_dir"/helmfile.d/*.gotmpl | sort | tail -1)" +[[ "$(basename "$last_stage")" == "04-upgrade-receipt.yaml.gotmpl" ]] \ + || fail "receipt is not the last stage; it would record a version before the stack finished applying" + +echo "PASS: upgrade-receipt-wiring" From 3ac39d83fe966167b19239b9bdb1346398f5d3af Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Mon, 21 Sep 2026 16:21:31 -0700 Subject: [PATCH 4/5] docs(ci): say what the pull_request-only guard actually rules out The previous comment asserted that a push to main has no baseline without showing what that costs, which is not enough to judge whether the guard is right. Review asked for a concrete case, so it now carries one: a branch adding a DROP migration under only `fix:` commits is what this job is there to stop, and the same commits on main would compare main against itself. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/build-test.yml | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 48c23e90d..e28e6fac7 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -189,8 +189,19 @@ jobs: stack-upgrade-policy: name: stack upgrade policy - # Only a pull request has a baseline to measure a proposed bump against. On - # a push to main the candidate and the baseline are the same commit. + # Pull requests only. The check weighs the migrations a candidate + # introduces against the version bump its commits imply, which needs two + # sides: a baseline to measure from, and a proposal to measure. A pull + # request has both -- origin/main is the baseline and the branch is the + # proposal. A push to main has already merged, so origin/main..HEAD is + # empty and there is no proposal left to judge. + # + # Concretely: a branch that adds + # migrations/cassandra/keyspaces/nvct_api/05_drop_health_info.up.sql and + # carries only `fix:` commits fails this job, because dropping a column + # cannot be skipped past and so needs a major, while `fix` asks for a + # patch. Once those same commits are on main, running here would compare + # main against itself and find nothing to report. if: github.event_name == 'pull_request' runs-on: ubuntu-latest strategy: From 69a3067730e34380ba30a10c46a9eecfd618ca27 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Mon, 21 Sep 2026 16:39:58 -0700 Subject: [PATCH 5/5] fix(self-managed): scope the receipt Role to the receipt ConfigMap The Role granted get, create and patch on every ConfigMap in the namespace, so anything able to run a pod under this ServiceAccount could rewrite unrelated ConfigMaps for as long as the binding existed. The Job only ever touches one object. get and patch now name that object through resourceNames. create stays unscoped in a rule of its own, because RBAC matches resourceNames against an object that does not exist yet and so never satisfies a create rule that names one; keeping it separate leaves the unscoped verb visible rather than buried alongside the scoped ones. Co-Authored-By: Claude Opus 5 (1M context) --- .../nvcf-upgrade-receipt/templates/role.yaml | 16 ++++++++++++---- .../self-managed/tests/upgrade-receipt-wiring.sh | 11 +++++++++-- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml index 1ad660c89..469367eea 100644 --- a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml @@ -10,9 +10,17 @@ metadata: "helm.sh/hook-weight": "-5" "helm.sh/hook-delete-policy": before-hook-creation rules: - # get and patch are for the upgrade case where a receipt already exists; - # create is for the first install. Nothing here needs to read or write any - # other resource. + # Scoped to the receipt itself. get and patch cover the upgrade case where a + # receipt already exists, and naming the resource keeps this identity from + # reaching any other ConfigMap in the namespace. - apiGroups: [""] resources: ["configmaps"] - verbs: ["get", "create", "patch"] + resourceNames: [{{ .Values.configMapName | quote }}] + verbs: ["get", "patch"] + # create cannot be scoped: RBAC matches resourceNames against an object that + # does not exist yet, so a create rule naming one is never satisfied. It is + # kept in its own rule so the unscoped verb is visible rather than buried + # alongside the scoped ones, and it is only reachable on a first install. + - apiGroups: [""] + resources: ["configmaps"] + verbs: ["create"] diff --git a/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh b/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh index 6161eeb9e..f59ec3bc8 100755 --- a/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh +++ b/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh @@ -33,8 +33,15 @@ grep -q 'value: "nvcf-upgrade-receipt"' <<<"$rendered" \ for kind in ServiceAccount Role RoleBinding; do grep -q "kind: ${kind}" <<<"$rendered" || fail "missing ${kind}; the Job cannot write the ConfigMap without it" done -grep -qE '^\s+verbs: \["get", "create", "patch"\]' <<<"$rendered" \ - || fail "Role must allow get/create/patch on configmaps and nothing more" +# get and patch are scoped to the receipt by name so this identity cannot +# reach any other ConfigMap. create cannot be scoped -- RBAC matches +# resourceNames against an object that does not exist yet. +grep -qE '^\s+resourceNames: \["nvcf-upgrade-receipt"\]' <<<"$rendered" \ + || fail "get/patch are not scoped to the receipt ConfigMap by name" +grep -qE '^\s+verbs: \["get", "patch"\]' <<<"$rendered" \ + || fail "scoped rule should carry only get and patch" +grep -qE '^\s+verbs: \["create"\]' <<<"$rendered" \ + || fail "create must remain, in its own rule, for the first install" # The stage number is the ordering guarantee. needs: is deliberately not used # here; see the comment in the stage file.