From 136273b969baed96010c425800ebaa497afc36c8 Mon Sep 17 00:00:00 2001 From: Stephanie Baum Date: Fri, 11 Sep 2026 16:38:43 -0700 Subject: [PATCH] fix(ci): track database image releases Declare the Cassandra and OpenBao image-to-chart ownership edges, including composite wrapper tags and repeated upgrade values. Extend and test the release bumper so future image releases update every owned chart field. No third-party dependencies are added or updated. Relates to #1781 Signed-off-by: Stephanie Baum --- .github/workflows/chart-version-bump.yml | 14 +- tools/chart-service-edge/main.go | 5 + tools/chart-service-edge/main_test.go | 8 ++ tools/chart-service-edge/metadata.go | 37 ++++- tools/chart-version-bumper/artifact.go | 144 +++++++++++++++++++ tools/chart-version-bumper/chart.go | 167 +++++++++++++++++++---- tools/chart-version-bumper/main.go | 40 ++++-- tools/chart-version-bumper/main_test.go | 119 +++++++++++++++- tools/chart-version-bumper/metadata.go | 64 +++++++-- tools/ci/github-release-subprojects.json | 58 +++++++- 10 files changed, 582 insertions(+), 74 deletions(-) create mode 100644 tools/chart-version-bumper/artifact.go diff --git a/.github/workflows/chart-version-bump.yml b/.github/workflows/chart-version-bump.yml index 31a47ee6a7..dc1a829a54 100644 --- a/.github/workflows/chart-version-bump.yml +++ b/.github/workflows/chart-version-bump.yml @@ -62,11 +62,13 @@ jobs: # fails any workflow that pins one. go-version-file: tools/go-toolchain/go.mod - - name: Test the bumper - # The bumper rewrites version fields in shipped charts, so its tests run - # here rather than somewhere that might not be reached. A test that - # gates nothing is not a test. - run: go test -C tools/chart-version-bumper ./... + - name: Test the chart release tools + # The bumper rewrites version fields in shipped charts, and the edge + # audit decodes the same ownership declarations. Exercise both schemas + # before either tool acts on a release. + run: | + go test -C tools/chart-version-bumper ./... + go test -C tools/chart-service-edge ./... - name: Test the commit type helper # It decides the semver step of the chart release this bump causes. @@ -242,7 +244,7 @@ jobs: body="$(printf '%s\n' \ "Opened by \`.github/workflows/chart-version-bump.yml\` when \`${TAG}\` was published." \ "" \ - "The released tag carries the version, so this is a direct update rather than a lookup of the newest published image." \ + "The released tag identifies the version and source tree, so this is a direct update rather than a lookup of the newest published image." \ "" \ "Merging this does not move the self-managed stack. A chart version reaches the stack only once the chart itself is released, and publishing that chart release is what triggers \`stack-pin-bump.yml\`." \ "" \ diff --git a/tools/chart-service-edge/main.go b/tools/chart-service-edge/main.go index 1e0ac8b518..d23c588500 100644 --- a/tools/chart-service-edge/main.go +++ b/tools/chart-service-edge/main.go @@ -33,6 +33,11 @@ // // "deploys": ["", ...] // +// A multi-image chart instead uses an object with values_paths to identify the +// exact fields owned by each service. It can use values_files for repeated pins +// in additional values files and sets app_version when that service also owns +// Chart.yaml's appVersion. +// // listing the release-metadata ids of the services whose images it ships. A // chart that ships no first-party image (an upstream dependency, or resources // only) declares "deploys": [] to say so deliberately. diff --git a/tools/chart-service-edge/main_test.go b/tools/chart-service-edge/main_test.go index fadffd5a0a..23351645fa 100644 --- a/tools/chart-service-edge/main_test.go +++ b/tools/chart-service-edge/main_test.go @@ -213,3 +213,11 @@ func TestEmptyStringDeployEntryIsRejected(t *testing.T) { t.Fatal("an empty string-form deploys entry must fail to decode") } } + +func TestObjectDeployRequiresValuesPaths(t *testing.T) { + var m Metadata + err := json.Unmarshal([]byte(`{"services":[{"id":"c","path":"deploy/helm/c","deploys":[{"service":"svc"}]}]}`), &m) + if err == nil { + t.Fatal("an object deploy without values_paths must fail to decode") + } +} diff --git a/tools/chart-service-edge/metadata.go b/tools/chart-service-edge/metadata.go index 3f703626f4..404825d3a3 100644 --- a/tools/chart-service-edge/metadata.go +++ b/tools/chart-service-edge/metadata.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "strings" ) @@ -34,12 +35,22 @@ type Entry struct { // decide which values.yaml line belongs to it. A chart with more than one // first-party image cannot use that evidence, so a deploy entry may instead // be an object naming the exact values.yaml paths (dotted, for example -// "otelCollector.imageTag") that carry that service's tag. This tool only -// audits which service ids are declared, so it does not care which shape a -// given entry takes; UnmarshalJSON exists so decoding either shape succeeds. +// "otelCollector.imageTag") that carry that service's tag. The object may set +// values_files for repeated pins in other values files and app_version when the +// same service owns the chart's appVersion. This tool only audits which service +// ids are declared, so UnmarshalJSON exists primarily to make both shapes +// available to that audit. type Deploy struct { Service string ValuesPaths []string + ValuesFiles []ValuesFile + AppVersion bool +} + +// ValuesFile names an additional values file and the service-owned paths in it. +type ValuesFile struct { + File string `json:"file"` + Paths []string `json:"paths"` } func (d *Deploy) UnmarshalJSON(b []byte) error { @@ -52,8 +63,10 @@ func (d *Deploy) UnmarshalJSON(b []byte) error { return nil } var obj struct { - Service string `json:"service"` - ValuesPaths []string `json:"values_paths"` + Service string `json:"service"` + ValuesPaths []string `json:"values_paths"` + ValuesFiles []ValuesFile `json:"values_files"` + AppVersion bool `json:"app_version"` } if err := json.Unmarshal(b, &obj); err != nil { return fmt.Errorf("deploys entry: %w", err) @@ -61,7 +74,19 @@ func (d *Deploy) UnmarshalJSON(b []byte) error { if obj.Service == "" { return fmt.Errorf("deploys entry missing \"service\"") } - *d = Deploy{Service: obj.Service, ValuesPaths: obj.ValuesPaths} + if len(obj.ValuesPaths) == 0 && len(obj.ValuesFiles) == 0 { + return fmt.Errorf("object deploys entry for %s requires values_paths or values_files", obj.Service) + } + for _, valuesFile := range obj.ValuesFiles { + clean := filepath.Clean(valuesFile.File) + if filepath.IsAbs(valuesFile.File) || clean == "." || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return fmt.Errorf("deploys entry for %s: values_files file %q must be relative to the chart path", obj.Service, valuesFile.File) + } + if len(valuesFile.Paths) == 0 { + return fmt.Errorf("deploys entry for %s: values_files file %q requires paths", obj.Service, valuesFile.File) + } + } + *d = Deploy{Service: obj.Service, ValuesPaths: obj.ValuesPaths, ValuesFiles: obj.ValuesFiles, AppVersion: obj.AppVersion} return nil } diff --git a/tools/chart-version-bumper/artifact.go b/tools/chart-version-bumper/artifact.go new file mode 100644 index 0000000000..997b14456e --- /dev/null +++ b/tools/chart-version-bumper/artifact.go @@ -0,0 +1,144 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os/exec" + "path/filepath" + "regexp" + "strings" +) + +const ( + releasePlaceholder = "${release}" + upstreamPlaceholder = "${upstream}" +) + +// ArtifactVersion describes how a service release version becomes the version +// embedded in the artifact it publishes. Most services need no declaration: +// their artifact version is the release version. Wrapper images can combine an +// upstream version from the released source tree with their release version. +type ArtifactVersion struct { + SourceFile string `json:"source_file"` + SourcePattern string `json:"source_pattern"` + Format string `json:"format"` +} + +// Release identifies a published service version and the artifact version a +// chart must consume. Version remains the release version so chart semver +// follows the released wrapper, not an upstream version embedded in its tag. +type Release struct { + ServiceID string + Version string + ArtifactVersion string + entry Entry +} + +// ReleaseForTag resolves a release tag and, when declared, derives its artifact +// version from the exact source tree the tag names. +func (m *Metadata) ReleaseForTag(root, tag string) (Release, error) { + serviceID, version, err := m.ServiceForTag(tag) + if err != nil { + return Release{}, err + } + var entry Entry + for _, candidate := range m.Services { + if candidate.ID == serviceID { + entry = candidate + break + } + } + release := Release{ServiceID: serviceID, Version: version, ArtifactVersion: version, entry: entry} + if entry.ArtifactVersion == nil { + return release, nil + } + artifact, err := entry.ArtifactVersion.render(root, tag, entry.Path, version) + if err != nil { + return Release{}, fmt.Errorf("resolve artifact version for %s: %w", serviceID, err) + } + release.ArtifactVersion = artifact + return release, nil +} + +func (a ArtifactVersion) render(root, tag, servicePath, version string) (string, error) { + if err := a.validate(); err != nil { + return "", err + } + upstream := "" + if strings.Contains(a.Format, upstreamPlaceholder) { + cleanSource := filepath.Clean(a.SourceFile) + if filepath.IsAbs(a.SourceFile) || cleanSource == "." || cleanSource == ".." || strings.HasPrefix(cleanSource, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("source_file %q must name a relative file inside the service path", a.SourceFile) + } + path := filepath.Join(servicePath, cleanSource) + content, err := exec.Command("git", "-C", root, "show", tag+":"+filepath.ToSlash(path)).CombinedOutput() + if err != nil { + return "", fmt.Errorf("read %s from tag %s: %w: %s", path, tag, err, strings.TrimSpace(string(content))) + } + re := regexp.MustCompile(a.SourcePattern) + matches := re.FindAllSubmatch(content, -1) + index := re.SubexpIndex("upstream") + if len(matches) != 1 || index < 0 || index >= len(matches[0]) || len(matches[0][index]) == 0 { + return "", fmt.Errorf("source_pattern must match %s exactly once with a non-empty upstream capture", path) + } + upstream = string(matches[0][index]) + } + artifact := strings.Replace(a.Format, releasePlaceholder, version, 1) + artifact = strings.Replace(artifact, upstreamPlaceholder, upstream, 1) + return artifact, nil +} + +func (a ArtifactVersion) validate() error { + if strings.Count(a.Format, releasePlaceholder) != 1 { + return fmt.Errorf("format must contain %s exactly once", releasePlaceholder) + } + usesUpstream := strings.Contains(a.Format, upstreamPlaceholder) + if strings.Count(a.Format, upstreamPlaceholder) > 1 { + return fmt.Errorf("format may contain %s at most once", upstreamPlaceholder) + } + if usesUpstream != (a.SourceFile != "" && a.SourcePattern != "") { + return fmt.Errorf("source_file and source_pattern are required exactly when format uses %s", upstreamPlaceholder) + } + if strings.Contains(strings.ReplaceAll(strings.ReplaceAll(a.Format, releasePlaceholder, ""), upstreamPlaceholder, ""), "${") { + return fmt.Errorf("format contains an unknown placeholder") + } + if a.SourcePattern != "" { + re, err := regexp.Compile(a.SourcePattern) + if err != nil { + return fmt.Errorf("compile source_pattern: %w", err) + } + if re.SubexpIndex("upstream") < 0 { + return fmt.Errorf("source_pattern must define a named upstream capture") + } + } + return nil +} + +// releaseVersionFromArtifact recovers the service release version from a +// chart's current artifact tag. This keeps the workflow's major/minor/patch +// decision based on the wrapper release even when the tag also embeds an +// upstream version. +func (a ArtifactVersion) releaseVersionFromArtifact(artifact string) (string, error) { + if err := a.validate(); err != nil { + return "", err + } + pattern := regexp.QuoteMeta(a.Format) + pattern = strings.Replace(pattern, regexp.QuoteMeta(upstreamPlaceholder), `.+?`, 1) + pattern = strings.Replace(pattern, regexp.QuoteMeta(releasePlaceholder), `(?P.+?)`, 1) + re := regexp.MustCompile("^" + pattern + "$") + match := re.FindStringSubmatch(artifact) + index := re.SubexpIndex("release") + if match == nil || index < 0 || match[index] == "" { + return "", fmt.Errorf("artifact version %q does not match format %q", artifact, a.Format) + } + return match[index], nil +} + +func (r Release) currentReleaseVersion(artifact string) (string, error) { + if r.entry.ArtifactVersion == nil { + return artifact, nil + } + return r.entry.ArtifactVersion.releaseVersionFromArtifact(artifact) +} diff --git a/tools/chart-version-bumper/chart.go b/tools/chart-version-bumper/chart.go index 6d2c5acdd0..e26f50a860 100644 --- a/tools/chart-version-bumper/chart.go +++ b/tools/chart-version-bumper/chart.go @@ -321,26 +321,32 @@ func Apply(root string, chart Entry, version string, p Plan) error { // PlanForValuesPaths decides what to do for a chart whose deploy edge names // the exact values.yaml paths holding this service's tag, rather than relying -// on appVersion agreement. appVersion is never part of this plan: in a -// multi-image chart it belongs to whichever other service, if any, uses the -// default single-image evidence. -func PlanForValuesPaths(root string, chart Entry, version string, paths []string) (Plan, error) { +// on single-image discovery. When ownsAppVersion is true, agreement between +// those paths and appVersion is required and all of them move together. +func PlanForValuesPaths(root string, chart Entry, version string, paths []string, files []ValuesFile, ownsAppVersion bool) (Plan, error) { chartYAML, valuesYAML := ChartFiles(root, chart.Path) if chartYAML == "" { return Plan{Action: ActionSkip, Detail: fmt.Sprintf("no Chart.yaml under %s", chart.Path)}, nil } - b, err := os.ReadFile(valuesYAML) + specs, err := declaredValuesSpecs(root, chart, valuesYAML, paths, files) if err != nil { - return Plan{}, fmt.Errorf("read %s: %w", valuesYAML, err) + return Plan{}, err } - lines := strings.Split(string(b), "\n") - values := make([]string, len(paths)) - for i, p := range paths { - _, v, err := resolveValuesPath(lines, p) + var names, values []string + for _, spec := range specs { + b, err := os.ReadFile(spec.path) if err != nil { - return Plan{ActionRefuse, err.Error(), "", nil}, nil + return Plan{}, fmt.Errorf("read %s: %w", spec.path, err) + } + lines := strings.Split(string(b), "\n") + for _, path := range spec.paths { + _, value, err := resolveValuesPath(lines, path) + if err != nil { + return Plan{ActionRefuse, valuesPathName(spec.label, err.Error()), "", nil}, nil + } + names = append(names, valuesPathName(spec.label, path)) + values = append(values, value) } - values[i] = v } // The declared paths are the ownership evidence here, in place of the @@ -352,16 +358,36 @@ func PlanForValuesPaths(root string, chart Entry, version string, paths []string if v != current { return Plan{ ActionRefuse, - fmt.Sprintf("declared values paths disagree: %s", describePaths(paths, values)), + fmt.Sprintf("declared values paths disagree: %s", describePaths(names, values)), "", values, }, nil } } + detail := "declared values path(s): " + strings.Join(names, ", ") + if ownsAppVersion { + b, err := os.ReadFile(chartYAML) + if err != nil { + return Plan{}, fmt.Errorf("read %s: %w", chartYAML, err) + } + match := appVersionRE.FindStringSubmatch(string(b)) + if match == nil { + return Plan{ActionRefuse, "chart declares no appVersion", current, values}, nil + } + if match[2] != current { + return Plan{ + ActionRefuse, + fmt.Sprintf("appVersion %s does not match declared values path(s) %s", match[2], describePaths(names, values)), + current, + values, + }, nil + } + detail += " and appVersion" + } if floating[current] { return Plan{ActionRefuse, "image tag is floating (" + current + ")", current, values}, nil } - return Plan{ActionValuesPaths, "declared values path(s): " + strings.Join(paths, ", "), current, values}, nil + return Plan{ActionValuesPaths, detail, current, values}, nil } // describePaths pairs each declared path with the value found there, for a @@ -374,25 +400,112 @@ func describePaths(paths, values []string) string { return strings.Join(parts, ", ") } -// ApplyValuesPaths writes the released version to every path this deploy edge -// named, and nothing else: appVersion and any other image in the chart are -// left alone. -func ApplyValuesPaths(root string, chart Entry, version string, paths []string) error { - _, valuesYAML := ChartFiles(root, chart.Path) - b, err := os.ReadFile(valuesYAML) +// ApplyValuesPaths writes the released artifact version to every path this +// deploy edge named and, when ownsAppVersion is true, to appVersion. Other +// images in the chart are left alone. +func ApplyValuesPaths(root string, chart Entry, version string, paths []string, files []ValuesFile, ownsAppVersion bool) error { + chartYAML, valuesYAML := ChartFiles(root, chart.Path) + specs, err := declaredValuesSpecs(root, chart, valuesYAML, paths, files) if err != nil { - return fmt.Errorf("read %s: %w", valuesYAML, err) + return err + } + type update struct { + path string + text string } - lines := strings.Split(string(b), "\n") - for _, p := range paths { - line, _, err := resolveValuesPath(lines, p) + updates := make([]update, 0, len(specs)) + for _, spec := range specs { + b, err := os.ReadFile(spec.path) if err != nil { + return fmt.Errorf("read %s: %w", spec.path, err) + } + lines := strings.Split(string(b), "\n") + for _, path := range spec.paths { + line, _, err := resolveValuesPath(lines, path) + if err != nil { + return fmt.Errorf("%s: %w", spec.path, err) + } + m := scalarValueRE.FindStringSubmatch(lines[line]) + lines[line] = m[1] + scalarLike(lines[line][len(m[1]):], version) + m[3] + } + updates = append(updates, update{path: spec.path, text: strings.Join(lines, "\n")}) + } + if !ownsAppVersion { + for _, update := range updates { + if err := writeFilePreservingMode(update.path, update.text); err != nil { + return err + } + } + return nil + } + + // Read and prepare both files before the first write. A missing or malformed + // Chart.yaml must not leave values.yaml moved on its own. + chartBytes, err := os.ReadFile(chartYAML) + if err != nil { + return fmt.Errorf("read %s: %w", chartYAML, err) + } + chartText := string(chartBytes) + match := appVersionRE.FindStringSubmatch(chartText) + if match == nil { + return fmt.Errorf("appVersion not found in %s", chartYAML) + } + replaced := false + chartText = appVersionRE.ReplaceAllStringFunc(chartText, func(line string) string { + if replaced { + return line + } + replaced = true + groups := appVersionRE.FindStringSubmatch(line) + return groups[1] + scalarLike(line[len(groups[1]):], version) + groups[3] + }) + if err := writeFilePreservingMode(chartYAML, chartText); err != nil { + return err + } + for _, update := range updates { + if err := writeFilePreservingMode(update.path, update.text); err != nil { return err } - m := scalarValueRE.FindStringSubmatch(lines[line]) - lines[line] = m[1] + scalarLike(lines[line][len(m[1]):], version) + m[3] } - return writeFilePreservingMode(valuesYAML, strings.Join(lines, "\n")) + return nil +} + +type declaredValuesSpec struct { + path string + label string + paths []string +} + +func declaredValuesSpecs(root string, chart Entry, valuesYAML string, paths []string, files []ValuesFile) ([]declaredValuesSpec, error) { + var specs []declaredValuesSpec + if len(paths) > 0 { + specs = append(specs, declaredValuesSpec{path: valuesYAML, paths: paths}) + } + for _, file := range files { + clean := filepath.Clean(file.File) + if filepath.IsAbs(file.File) || clean == "." || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("values file %q must be relative to the chart path", file.File) + } + if len(file.Paths) == 0 { + return nil, fmt.Errorf("values file %q declares no paths", file.File) + } + specs = append(specs, declaredValuesSpec{ + path: filepath.Join(root, chart.Path, clean), + label: filepath.ToSlash(clean), + paths: file.Paths, + }) + } + if len(specs) == 0 { + return nil, fmt.Errorf("no declared values paths for chart %s", chart.ID) + } + return specs, nil +} + +func valuesPathName(file, path string) string { + if file == "" { + return path + } + return file + ":" + path } // scalarLike renders version the way the value it replaces was written: diff --git a/tools/chart-version-bumper/main.go b/tools/chart-version-bumper/main.go index 9bbf1f636d..40729317d4 100644 --- a/tools/chart-version-bumper/main.go +++ b/tools/chart-version-bumper/main.go @@ -2,7 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 // Command chart-version-bumper moves a chart's version fields to a newly -// released service version. +// released service artifact version. // // chart-version-bumper --tag src/control-plane-services/notary/v1.9.0 [--write] // @@ -86,17 +86,21 @@ func Run(root, tag string, write bool, out, errOut io.Writer) (int, error) { return 1, err } - serviceID, version, err := meta.ServiceForTag(tag) + release, err := meta.ReleaseForTag(root, tag) if err != nil { return 1, err } - charts := meta.ChartsDeploying(serviceID) - fmt.Fprintf(out, "%s -> service %s, version %s\n", tag, serviceID, version) + charts := meta.ChartsDeploying(release.ServiceID) + fmt.Fprintf(out, "%s -> service %s, version %s", tag, release.ServiceID, release.Version) + if release.ArtifactVersion != release.Version { + fmt.Fprintf(out, ", artifact %s", release.ArtifactVersion) + } + fmt.Fprintln(out) if len(charts) == 0 { // Not an error. Plenty of services ship no chart, and chart-service-edge // is what reports charts that have not declared an edge yet. - fmt.Fprintf(out, "no chart declares that it deploys %s; nothing to do\n", serviceID) + fmt.Fprintf(out, "no chart declares that it deploys %s; nothing to do\n", release.ServiceID) return 0, nil } @@ -105,10 +109,10 @@ func Run(root, tag string, write bool, out, errOut io.Writer) (int, error) { for _, chart := range charts { var p Plan var err error - if len(chart.ValuesPaths) > 0 { - p, err = PlanForValuesPaths(root, chart.Entry, version, chart.ValuesPaths) + if len(chart.ValuesPaths) > 0 || len(chart.ValuesFiles) > 0 { + p, err = PlanForValuesPaths(root, chart.Entry, release.ArtifactVersion, chart.ValuesPaths, chart.ValuesFiles, chart.AppVersion) } else { - p, err = PlanFor(root, chart.Entry, version) + p, err = PlanFor(root, chart.Entry, release.ArtifactVersion) } if err != nil { return 1, err @@ -120,17 +124,23 @@ func Run(root, tag string, write bool, out, errOut io.Writer) (int, error) { case ActionSkip: fmt.Fprintf(out, " %s: skipped, %s\n", chart.ID, p.Detail) default: - if p.Current == version { - fmt.Fprintf(out, " %s: already %s\n", chart.ID, version) + if p.Current == release.ArtifactVersion { + fmt.Fprintf(out, " %s: already %s\n", chart.ID, release.ArtifactVersion) + continue + } + currentRelease, err := release.currentReleaseVersion(p.Current) + if err != nil { + fmt.Fprintf(errOut, " %s: REFUSED, %s\n", chart.ID, err) + refused++ continue } - fmt.Fprintf(out, " %s: %s -> %s (%s)\n", chart.ID, p.Current, version, p.Detail) - level = HigherLevel(level, BumpLevel(p.Current, version)) + fmt.Fprintf(out, " %s: %s -> %s (%s)\n", chart.ID, p.Current, release.ArtifactVersion, p.Detail) + level = HigherLevel(level, BumpLevel(currentRelease, release.Version)) if write { - if len(chart.ValuesPaths) > 0 { - err = ApplyValuesPaths(root, chart.Entry, version, chart.ValuesPaths) + if len(chart.ValuesPaths) > 0 || len(chart.ValuesFiles) > 0 { + err = ApplyValuesPaths(root, chart.Entry, release.ArtifactVersion, chart.ValuesPaths, chart.ValuesFiles, chart.AppVersion) } else { - err = Apply(root, chart.Entry, version, p) + err = Apply(root, chart.Entry, release.ArtifactVersion, p) } if err != nil { return 1, err diff --git a/tools/chart-version-bumper/main_test.go b/tools/chart-version-bumper/main_test.go index ef982155ed..790d331b45 100644 --- a/tools/chart-version-bumper/main_test.go +++ b/tools/chart-version-bumper/main_test.go @@ -7,6 +7,7 @@ import ( "bytes" "fmt" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -72,6 +73,32 @@ func (f *fixture) read(t *testing.T, id, name string) string { return string(b) } +func (f *fixture) source(t *testing.T, path, content string) { + t.Helper() + full := filepath.Join(f.root, path) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func (f *fixture) tag(t *testing.T, tag string) { + t.Helper() + commands := [][]string{ + {"init", "--quiet"}, + {"add", "."}, + {"-c", "user.name=Test", "-c", "user.email=test@example.com", "commit", "--quiet", "-m", "fixture"}, + {"tag", tag}, + } + for _, args := range commands { + if output, err := exec.Command("git", append([]string{"-C", f.root}, args...)...).CombinedOutput(); err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, output) + } + } +} + const meta = `{"services":[ {"id":"svc","path":"src/svc"}, {"id":"other","path":"src/other"}, @@ -358,8 +385,8 @@ func TestRealChartsResolve(t *testing.T) { continue } for _, d := range e.Deploys { - if len(d.ValuesPaths) > 0 { - if _, err := PlanForValuesPaths(root, e, "9.9.9", d.ValuesPaths); err != nil { + if len(d.ValuesPaths) > 0 || len(d.ValuesFiles) > 0 { + if _, err := PlanForValuesPaths(root, e, "9.9.9", d.ValuesPaths, d.ValuesFiles, d.AppVersion); err != nil { t.Errorf("planning %s (%s values paths) failed: %v", e.ID, d.Service, err) } continue @@ -594,6 +621,72 @@ func TestValuesPathsAllMoveTogether(t *testing.T) { } } +func TestCompositeArtifactVersionMovesDeclaredPathsAndAppVersion(t *testing.T) { + f := newFixture(t, `{"services":[ + {"id":"wrapper","path":"infra/wrapper","artifact_version":{ + "source_file":"Dockerfile", + "source_pattern":"(?m)^ARG UPSTREAM=(?P[0-9]+\\.[0-9]+\\.[0-9]+)$", + "format":"${upstream}-nv-${release}" + }}, + {"id":"c","path":"deploy/helm/c","deploys":[{ + "service":"wrapper", + "values_paths":["server.image.tag","agent.image.tag"], + "values_files":[{"file":"upgrade/values.yaml","paths":["server.image.tag","agent.image.tag"]}], + "app_version":true + }]} + ]}`) + f.chart(t, "c", "2.5.5-nv-1.3.3", "server:\n image:\n tag: 2.5.5-nv-1.3.3\nagent:\n image:\n tag: 2.5.5-nv-1.3.3") + f.source(t, "deploy/helm/c/upgrade/values.yaml", "server:\n image:\n tag: 2.5.5-nv-1.3.3\nagent:\n image:\n tag: 2.5.5-nv-1.3.3\n") + f.source(t, "infra/wrapper/Dockerfile", "ARG UPSTREAM=2.6.2\n") + f.tag(t, "infra/wrapper/v1.3.4") + + // The working tree may have advanced since the release. The image tag must + // come from the tagged source, not whatever the default branch says now. + f.source(t, "infra/wrapper/Dockerfile", "ARG UPSTREAM=9.9.9\n") + code, out, errOut := f.run(t, "infra/wrapper/v1.3.4", true) + if code != 0 { + t.Fatalf("want a clean composite bump, got %d\n%s%s", code, out, errOut) + } + if !strings.Contains(out, "version 1.3.4, artifact 2.6.2-nv-1.3.4") { + t.Fatalf("release and artifact versions were not reported separately:\n%s", out) + } + if !strings.Contains(out, "bump: patch") { + t.Fatalf("chart semver must follow wrapper 1.3.3 -> 1.3.4, not the embedded upstream version:\n%s", out) + } + if got := f.read(t, "c", "Chart.yaml"); !strings.Contains(got, `appVersion: "2.6.2-nv-1.3.4"`) { + t.Fatalf("appVersion did not move to the composite artifact version:\n%s", got) + } + if got := f.read(t, "c", "values.yaml"); strings.Count(got, "tag: 2.6.2-nv-1.3.4") != 2 { + t.Fatalf("both declared image fields must move to the composite artifact version:\n%s", got) + } + if got := f.read(t, "c", "upgrade/values.yaml"); strings.Count(got, "tag: 2.6.2-nv-1.3.4") != 2 { + t.Fatalf("repeated pins in an additional values file must move too:\n%s", got) + } +} + +func TestAppVersionOwningPathsRefuseExistingDrift(t *testing.T) { + f := newFixture(t, `{"services":[ + {"id":"svc","path":"src/svc"}, + {"id":"c","path":"deploy/helm/c","deploys":[{ + "service":"svc","values_paths":["server.image.tag"],"app_version":true + }]} + ]}`) + f.chart(t, "c", "1.0.0", "server:\n image:\n tag: 1.0.1") + beforeChart := f.read(t, "c", "Chart.yaml") + beforeValues := f.read(t, "c", "values.yaml") + + code, _, errOut := f.run(t, "src/svc/v1.0.2", true) + if code != RefusedExit { + t.Fatalf("want refusal, got %d", code) + } + if !strings.Contains(errOut, "appVersion 1.0.0 does not match declared values path(s) server.image.tag=1.0.1") { + t.Fatalf("want the appVersion drift reason:\n%s", errOut) + } + if f.read(t, "c", "Chart.yaml") != beforeChart || f.read(t, "c", "values.yaml") != beforeValues { + t.Fatal("a drift refusal must not modify either chart file") + } +} + func TestValuesPathsDisagreeRefusesRatherThanForcingAgreement(t *testing.T) { f := newFixture(t, `{"services":[ {"id":"sidecar","path":"src/sidecar"}, @@ -679,7 +772,7 @@ func TestDeployEntryAcceptsBothStringAndObjectShapesInOneList(t *testing.T) { // appVersion evidence) with an object (byoo-otel-collector, explicit // paths) in the same deploys list. m := decodeMetadata(t, `{"services":[ - {"id":"c","path":"deploy/helm/c","deploys":["nvca",{"service":"sidecar","values_paths":["a.tag"]}]} + {"id":"c","path":"deploy/helm/c","deploys":["nvca",{"service":"sidecar","values_paths":["a.tag"],"app_version":true}]} ]}`) entry := m.Services[0] if len(entry.Deploys) != 2 { @@ -688,11 +781,29 @@ func TestDeployEntryAcceptsBothStringAndObjectShapesInOneList(t *testing.T) { if entry.Deploys[0].Service != "nvca" || len(entry.Deploys[0].ValuesPaths) != 0 { t.Fatalf("the string form should decode as a bare service with no paths: %+v", entry.Deploys[0]) } - if entry.Deploys[1].Service != "sidecar" || strings.Join(entry.Deploys[1].ValuesPaths, ",") != "a.tag" { + if entry.Deploys[1].Service != "sidecar" || strings.Join(entry.Deploys[1].ValuesPaths, ",") != "a.tag" || !entry.Deploys[1].AppVersion { t.Fatalf("the object form should decode its service and paths: %+v", entry.Deploys[1]) } } +func TestObjectDeployRequiresValuesPaths(t *testing.T) { + f := newFixture(t, `{"services":[ + {"id":"c","path":"deploy/helm/c","deploys":[{"service":"svc"}]} + ]}`) + if _, err := LoadMetadata(f.root); err == nil { + t.Fatal("an object deploy without values_paths must fail to decode") + } +} + +func TestArtifactVersionMetadataIsValidatedAtLoad(t *testing.T) { + f := newFixture(t, `{"services":[ + {"id":"svc","path":"infra/svc","artifact_version":{"format":"${upstream}"}} + ]}`) + if _, err := LoadMetadata(f.root); err == nil || !strings.Contains(err.Error(), "service svc artifact_version") { + t.Fatalf("invalid artifact_version must fail metadata loading, got %v", err) + } +} + func decodeMetadata(t *testing.T, body string) *Metadata { t.Helper() f := newFixture(t, body) diff --git a/tools/chart-version-bumper/metadata.go b/tools/chart-version-bumper/metadata.go index 2eb6127b0a..fbc2785423 100644 --- a/tools/chart-version-bumper/metadata.go +++ b/tools/chart-version-bumper/metadata.go @@ -19,9 +19,10 @@ const ChartPrefix = "deploy/helm/" // Entry is one subproject in the release metadata. type Entry struct { - ID string `json:"id"` - Path string `json:"path"` - Deploys []Deploy `json:"deploys"` + ID string `json:"id"` + Path string `json:"path"` + Deploys []Deploy `json:"deploys"` + ArtifactVersion *ArtifactVersion `json:"artifact_version"` } // Deploy is one service a chart ships. A plain JSON string names the service @@ -33,12 +34,23 @@ type Entry struct { // without saying which one is this service's. For that case a deploy entry // is an object naming ValuesPaths, the exact values.yaml paths (dotted, for // example "otelCollector.imageTag") that carry this service's tag. Those -// paths move to the released version and nothing else does: appVersion is -// left alone, because in a multi-image chart it belongs to whichever other -// service (if any) uses the default single-image evidence. +// paths move to the released artifact version. ValuesFiles can name repeated +// pins in additional values files under the chart path. AppVersion is left +// alone by default, because in a multi-image chart it may belong to another +// service. Set AppVersion when this service owns both the declared paths and +// the chart's appVersion. type Deploy struct { Service string ValuesPaths []string + ValuesFiles []ValuesFile + AppVersion bool +} + +// ValuesFile names additional values outside the chart's primary values.yaml +// that repeat a service pin, such as overrides used with Helm --reuse-values. +type ValuesFile struct { + File string `json:"file"` + Paths []string `json:"paths"` } func (d *Deploy) UnmarshalJSON(b []byte) error { @@ -51,8 +63,10 @@ func (d *Deploy) UnmarshalJSON(b []byte) error { return nil } var obj struct { - Service string `json:"service"` - ValuesPaths []string `json:"values_paths"` + Service string `json:"service"` + ValuesPaths []string `json:"values_paths"` + ValuesFiles []ValuesFile `json:"values_files"` + AppVersion bool `json:"app_version"` } if err := json.Unmarshal(b, &obj); err != nil { return fmt.Errorf("deploys entry: %w", err) @@ -60,7 +74,19 @@ func (d *Deploy) UnmarshalJSON(b []byte) error { if obj.Service == "" { return fmt.Errorf("deploys entry missing \"service\"") } - *d = Deploy{Service: obj.Service, ValuesPaths: obj.ValuesPaths} + if len(obj.ValuesPaths) == 0 && len(obj.ValuesFiles) == 0 { + return fmt.Errorf("object deploys entry for %s requires values_paths or values_files", obj.Service) + } + for _, valuesFile := range obj.ValuesFiles { + clean := filepath.Clean(valuesFile.File) + if filepath.IsAbs(valuesFile.File) || clean == "." || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return fmt.Errorf("deploys entry for %s: values_files file %q must be relative to the chart path", obj.Service, valuesFile.File) + } + if len(valuesFile.Paths) == 0 { + return fmt.Errorf("deploys entry for %s: values_files file %q requires paths", obj.Service, valuesFile.File) + } + } + *d = Deploy{Service: obj.Service, ValuesPaths: obj.ValuesPaths, ValuesFiles: obj.ValuesFiles, AppVersion: obj.AppVersion} return nil } @@ -80,14 +106,26 @@ func LoadMetadata(root string) (*Metadata, error) { if err := json.Unmarshal(b, &m); err != nil { return nil, fmt.Errorf("parse %s: %w", path, err) } + for _, entry := range m.Services { + if entry.ArtifactVersion == nil { + continue + } + if strings.HasPrefix(entry.Path, ChartPrefix) { + return nil, fmt.Errorf("parse %s: chart %s may not declare artifact_version", path, entry.ID) + } + if err := entry.ArtifactVersion.validate(); err != nil { + return nil, fmt.Errorf("parse %s: service %s artifact_version: %w", path, entry.ID, err) + } + } return &m, nil } // ServiceForTag maps a release tag to the service that owns it and the version // the tag carries. // -// The released tag carries the version, so there is no "newest version" lookup -// and none of the ordering questions that come with one. +// The released tag carries the release version and identifies its exact source +// tree, so there is no "newest version" lookup and none of the ordering +// questions that come with one. func (m *Metadata) ServiceForTag(tag string) (serviceID, version string, err error) { bestPath, bestID := "", "" for _, e := range m.Services { @@ -118,6 +156,8 @@ func (m *Metadata) ServiceForTag(tag string) (serviceID, version string, err err type ChartDeploy struct { Entry ValuesPaths []string + ValuesFiles []ValuesFile + AppVersion bool } // ChartsDeploying returns the chart entries that declare they deploy @@ -130,7 +170,7 @@ func (m *Metadata) ChartsDeploying(serviceID string) []ChartDeploy { } for _, d := range e.Deploys { if d.Service == serviceID { - out = append(out, ChartDeploy{Entry: e, ValuesPaths: d.ValuesPaths}) + out = append(out, ChartDeploy{Entry: e, ValuesPaths: d.ValuesPaths, ValuesFiles: d.ValuesFiles, AppVersion: d.AppVersion}) break } } diff --git a/tools/ci/github-release-subprojects.json b/tools/ci/github-release-subprojects.json index 83c1aa7d9b..55261dc243 100644 --- a/tools/ci/github-release-subprojects.json +++ b/tools/ci/github-release-subprojects.json @@ -231,12 +231,32 @@ "id": "cassandra", "path": "deploy/helm/cassandra", "service_name": "helm-nvcf-cassandra", - "legacy_tag_prefix": "helm-nvcf-cassandra-v" + "legacy_tag_prefix": "helm-nvcf-cassandra-v", + "deploys": [ + { + "service": "cassandra-image", + "values_paths": [ + "cassandra.image.tag" + ], + "app_version": true + }, + { + "service": "cassandra-migrations", + "values_paths": [ + "cassandra.migrations.image.tag" + ] + } + ] }, { "id": "cassandra-image", "path": "infra/cassandra", - "service_name": "nvcf-cassandra" + "service_name": "nvcf-cassandra", + "artifact_version": { + "source_file": "Dockerfile", + "source_pattern": "(?m)^FROM cassandra:(?P[0-9]+\\.[0-9]+\\.[0-9]+)@sha256:", + "format": "${upstream}-nv-${release}" + } }, { "id": "api-keys-colocated", @@ -248,12 +268,42 @@ "id": "openbao", "path": "deploy/helm/openbao", "service_name": "helm-nvcf-openbao-server", - "legacy_tag_prefix": "helm-nvcf-openbao-server-v" + "legacy_tag_prefix": "helm-nvcf-openbao-server-v", + "deploys": [ + { + "service": "openbao-image", + "values_paths": [ + "openbao.server.image.tag", + "openbao.injector.agentImage.tag" + ], + "values_files": [ + { + "file": "upgrade/values-upgrades.yaml", + "paths": [ + "openbao.server.image.tag", + "openbao.injector.agentImage.tag" + ] + } + ], + "app_version": true + }, + { + "service": "openbao-migrations", + "values_paths": [ + "openbao.migrations.image.tag" + ] + } + ] }, { "id": "openbao-image", "path": "infra/openbao", - "service_name": "nvcf-openbao" + "service_name": "nvcf-openbao", + "artifact_version": { + "source_file": "Dockerfile", + "source_pattern": "(?m)^ARG BAO_VERSION=(?P[0-9]+\\.[0-9]+\\.[0-9]+)$", + "format": "${upstream}-nv-${release}" + } }, { "id": "cert-manager",