From de42c0870cf87e52f7abc46bad142c7433a8ddef Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 13:37:48 +0000 Subject: [PATCH] fix(kustomize): a digest: override no longer strips the tag out of the source file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kustomize's image transformer treats tag and digest as mutually exclusive. Its own code says so: // overriding tag or digest will replace both original tag and digest values case NewTag != "" && Digest != "": tag = NewTag; digest = Digest case NewTag != "": tag = NewTag; digest = "" case Digest != "": tag = ""; digest = Digest Our re-implemented transformer (added with the images/replicas edit-through in #198, released since v0.30.0) set the two components independently, so it believed `app:1.0.0` + `digest: sha256:bbb` rendered to `app:1.0.0@sha256:bbb` where kustomize renders `app@sha256:bbb`. Believing the wrong render, the projection compared the real live object against it, concluded the user had REMOVED the tag, and wrote the tag out of the source manifest: `app:1.0.0` became `app`. Every reconcile, silently, with no refusal and no diagnostic. The mirror image (`newTag:` on a digest-carrying source image) drops the digest the same way. Found by rendering the corpus with kustomize itself and comparing, which is the point of the rest of this change: - internal/manifestanalyzer/kustomize_render.go — a sandboxed krusty build (in-memory filesystem, plugins disabled, remote bases refused BEFORE the build, because kustomize fetches one by shelling out to `git`). It returns each object with kustomize's own provenance: config.kubernetes.io/origin says which file produced it, and alpha.config.kubernetes.io/transformations says which kustomization's transformers touched it, in build order — the override chain, handed to us by the thing that applies it. - Two differential tests. One renders every render root in both corpora and requires our graph walk to attribute the same chain kustomize reports, and our renderImage to produce the same image kustomize produces. One drives the cases a re-implementation actually has to get right — chained renames, digest precedence, a registry port that looks like a tag — through both, and requires them to agree byte for byte. The renderer is not on the write path yet; it is the oracle the re-implementation is checked against. Removing the re-implementation is the next change, and it is now a verified swap rather than a brave one. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/UPGRADING.md | 26 ++ .../kustomize-support-boundary.md | 15 + go.mod | 4 +- go.sum | 6 + internal/manifestanalyzer/kustomize_render.go | 263 +++++++++++++++++ .../kustomize_render_semantics_test.go | 141 +++++++++ .../manifestanalyzer/kustomize_render_test.go | 277 ++++++++++++++++++ .../manifestanalyzer/overrides_projection.go | 92 ++++-- .../overrides_projection_test.go | 35 ++- 9 files changed, 836 insertions(+), 23 deletions(-) create mode 100644 internal/manifestanalyzer/kustomize_render.go create mode 100644 internal/manifestanalyzer/kustomize_render_semantics_test.go create mode 100644 internal/manifestanalyzer/kustomize_render_test.go diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 54c86425..adbfd50b 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,32 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Unreleased — a `digest:` override no longer strips the tag out of your source file (next patch; bug fix) + +**If any of your kustomizations use `images:` with `digest:`, or `newTag:` on an image +that carries a digest, the operator has been rewriting your source manifests. This stops.** + +kustomize's image transformer treats tag and digest as mutually exclusive — its own code +says *"overriding tag or digest will replace both original tag and digest values"*. Our +re-implementation set the two components independently, so: + +| Source image | `images:` entry | kustomize renders | We believed | +|---|---|---|---| +| `app:1.0.0` | `digest: sha256:bbb` | `app@sha256:bbb` | `app:1.0.0@sha256:bbb` | +| `app@sha256:old` | `newTag: "2.0"` | `app:2.0` | `app:2.0@sha256:old` | + +Believing the wrong render, the projection compared the real live object against it, +concluded the user had *removed* the tag, and wrote the tag out of the source document — +`app:1.0.0` became `app`. On every reconcile, silently, with no refusal and no diagnostic. + +**Migration** + +- **Check the affected files.** Any manifest referenced by a kustomization whose `images:` + entry sets `digest:` may have lost its tag in Git. The fix stops the rewrite but does not + restore what was already written; recover the tag from history if you need it. +- Nothing to configure. The behaviour is simply correct now, and pinned against a real + `kustomize build` so it cannot regress. + ## Unreleased — `kustomization.yaml` is now read by kustomize itself (next minor; behavior change) The analyzer used to decode `kustomization.yaml` with a hand-written walk over a generic diff --git a/docs/design/support-boundary/kustomize-support-boundary.md b/docs/design/support-boundary/kustomize-support-boundary.md index 0ea2b8be..dc124f72 100644 --- a/docs/design/support-boundary/kustomize-support-boundary.md +++ b/docs/design/support-boundary/kustomize-support-boundary.md @@ -284,6 +284,21 @@ kustomize" means two different things: renderer. The re-implementation is being removed.** This reverses the earlier position — kept here because the reasoning was wrong in a specific, instructive way. +> **Landed so far.** The typed `kustomization.yaml` parse (#229), and +> [`kustomize_render.go`](../../../internal/manifestanalyzer/kustomize_render.go) — a +> sandboxed `krusty` build that returns each object with kustomize's own provenance +> (`config.kubernetes.io/origin` says which file produced it; +> `alpha.config.kubernetes.io/transformations` says which kustomization's transformers +> touched it, in order). The renderer is not yet on the write path: it is currently the +> differential oracle the re-implementation is checked against, which is what makes +> removing the re-implementation safe rather than brave. +> +> **What the oracle found immediately:** the re-implemented image transformer treated +> tag and digest as independent, where kustomize replaces both. A folder using `digest:` +> had the tag written out of its source manifest on every reconcile. Two silent-corruption +> bugs, in shipped code, found by the first differential run — which is the whole argument +> for this decision, made concrete. + The old position was that re-implementing the narrow transformer subset "keeps the refusal boundary honest: we refuse exactly what we do not model," and that `krusty` was at best a *verification oracle* comparing against our own projection. diff --git a/go.mod b/go.mod index 21379e5e..8f2851dd 100644 --- a/go.mod +++ b/go.mod @@ -31,6 +31,7 @@ require ( sigs.k8s.io/cli-utils v0.37.2 sigs.k8s.io/controller-runtime v0.24.1 sigs.k8s.io/kustomize/api v0.21.1 + sigs.k8s.io/kustomize/kyaml v0.21.1 sigs.k8s.io/yaml v1.6.0 ) @@ -88,6 +89,7 @@ require ( github.com/klauspost/cpuid/v2 v2.3.0 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect + github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pjbgf/sha1cd v0.6.0 // indirect github.com/prometheus/client_model v0.6.2 // indirect @@ -100,6 +102,7 @@ require ( github.com/stretchr/objx v0.5.3 // indirect github.com/x448/float16 v0.8.4 // indirect github.com/xanzy/ssh-agent v0.3.3 // indirect + github.com/xlab/treeprint v1.2.0 // indirect github.com/yuin/gopher-lua v1.1.2 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.68.0 // indirect @@ -137,7 +140,6 @@ require ( k8s.io/streaming v0.36.2 // indirect sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.34.0 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect - sigs.k8s.io/kustomize/kyaml v0.21.1 // indirect sigs.k8s.io/randfill v1.0.0 // indirect sigs.k8s.io/structured-merge-diff/v6 v6.4.0 // indirect ) diff --git a/go.sum b/go.sum index d2558a2f..1bc42869 100644 --- a/go.sum +++ b/go.sum @@ -179,6 +179,8 @@ github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJ github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFdJifH4BDsTlE89Zl93FEloxaWZfGcifgq8= github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= +github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 h1:n6/2gBQ3RWajuToeY6ZtZTIKv2v7ThUy5KKusIT0yc0= +github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00/go.mod h1:Pm3mSP3c5uWn86xMLZ5Sa7JB9GsEZySvHYXCTK4E9q4= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f h1:KUppIJq7/+SVif2QVs3tOP0zanoHgBEVAwHxUSIzRqU= @@ -225,6 +227,7 @@ github.com/stretchr/objx v0.5.3/go.mod h1:rDQraq+vQZU7Fde9LOZLr8Tax6zZvy4kuNKF+Q github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/tidwall/gjson v1.18.0 h1:FIDeeyB800efLX89e5a8Y0BNH+LOngJyGrIWxG2FKQY= @@ -239,6 +242,8 @@ github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= github.com/xanzy/ssh-agent v0.3.3 h1:+/15pJfg/RsTxqYcX6fHqOXZwwMP+2VyYWJeWM2qQFM= github.com/xanzy/ssh-agent v0.3.3/go.mod h1:6dzNDKs0J9rVPHPhaGCukekBHKqfl+L3KghI1Bc68Uw= +github.com/xlab/treeprint v1.2.0 h1:HzHnuAF1plUN2zGlAFHbSQP2qJ0ZAD3XF5XD7OesXRQ= +github.com/xlab/treeprint v1.2.0/go.mod h1:gj5Gd3gPdKtR1ikdDK6fnFLdmIS0X30kTTuNd/WEJu0= github.com/yuin/gopher-lua v1.1.2 h1:yF/FjE3hD65tBbt0VXLE13HWS9h34fdzJmrWRXwobGA= github.com/yuin/gopher-lua v1.1.2/go.mod h1:7aRmXIWl37SqRf0koeyylBEzJ+aPt8A+mmkQ4f1ntR8= github.com/zeebo/xxh3 v1.1.0 h1:s7DLGDK45Dyfg7++yxI0khrfwq9661w9EN78eP/UZVs= @@ -336,6 +341,7 @@ gopkg.in/warnings.v0 v0.1.2 h1:wFXVbFY8DY5/xOe1ECiWdKCzZlxgshcYVNkBHstARME= gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= k8s.io/api v0.36.2 h1:TF6YDLIzKfccK7cq9YpTcGX8TJmEkHVRv78DM51fRYY= diff --git a/internal/manifestanalyzer/kustomize_render.go b/internal/manifestanalyzer/kustomize_render.go new file mode 100644 index 00000000..f9aaf555 --- /dev/null +++ b/internal/manifestanalyzer/kustomize_render.go @@ -0,0 +1,263 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "errors" + "fmt" + "path" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/kustomize/api/krusty" + "sigs.k8s.io/kustomize/api/resmap" + kustypes "sigs.k8s.io/kustomize/api/types" + "sigs.k8s.io/kustomize/kyaml/filesys" + "sigs.k8s.io/yaml" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// This file renders a kustomize render root with kustomize itself, rather than +// re-implementing its transformers. It is the ground truth the projection is +// checked against: what we believe a folder renders to becomes what the library +// Flux renders with says it renders to. +// +// See docs/design/support-boundary/kustomize-support-boundary.md §7. + +// The provenance kustomize emits when buildMetadata asks for it. These are the +// reason we need not walk the resources graph ourselves: +// +// config.kubernetes.io/origin: path: ../base/deployment.yaml +// alpha.config.kubernetes.io/transformations: +// - configuredIn: ../base/kustomization.yaml +// configuredBy: {apiVersion: builtin, kind: ImageTagTransformer} +// +// The first says which source file produced the object. The second says which +// kustomization's transformers touched it, in build order — the override chain, +// handed to us by the renderer that applies it. +const ( + originAnnotation = "config.kubernetes.io/origin" + transformationsAnnotation = "alpha.config.kubernetes.io/transformations" +) + +// imageTagTransformer and replicaCountTransformer are the builtin transformers +// behind the two edit-through channels, as kustomize names them in the +// transformations annotation. +const ( + imageTagTransformer = "ImageTagTransformer" + replicaCountTransformer = "ReplicaCountTransformer" +) + +// errRemoteBase refuses a build whose kustomization reaches outside the repository. +// +// The check must run BEFORE krusty, never inside it: kustomize resolves a remote +// base by shelling out to `git fetch`, and it does so under LoadRestrictionsRootOnly +// and under an in-memory filesystem alike (both measured). No build option turns it +// off, so refusing first is the only thing that keeps "the operator never fetches a +// remote base" true. +var errRemoteBase = errors.New("kustomization reaches a remote base; the operator never fetches one") + +// renderedObject is one object kustomize produced, with the provenance saying +// where it came from and what shaped it. +type renderedObject struct { + // Object is the rendered result: what the GitOps controller will apply. + Object *unstructured.Unstructured + // OriginPath is the source file that produced it, relative to the scan root. + // Empty for a generated resource (which the acceptance gate refuses anyway). + OriginPath string + // TransformedBy lists the kustomizations whose transformers touched it, in + // build order (innermost base first) — the override chain, from kustomize. + TransformedBy []transformation +} + +// transformation is one entry of kustomize's transformations annotation: which +// kustomization configured which builtin transformer. +type transformation struct { + // ConfiguredIn is the kustomization file, relative to the scan root. + ConfiguredIn string + // Kind is the builtin transformer, e.g. "ImageTagTransformer". + Kind string +} + +// renderMountPoint is where the scanned tree is mounted in the in-memory +// filesystem. The whole scan is mounted, not just the render root, so a +// kustomization can read a base beside or below it. This is a READ scope: the +// write jail is enforced in the writer (L1) and is not this function's job. +const renderMountPoint = "/scan" + +// renderRoot builds one render root with kustomize and returns every object it +// produces, carrying provenance. rootDir is slash-relative to the scan root, and +// files are the scan's YAML files, which become an in-memory filesystem — so the +// build never touches the real disk, never executes a plugin, and never reaches the +// network. +func renderRoot(files []manifestedit.FileContent, rootDir string) ([]renderedObject, error) { + if err := refuseRemoteBases(parseKustomizations(files), rootDir); err != nil { + return nil, err + } + fSys, err := renderFilesystem(files, rootDir) + if err != nil { + return nil, err + } + k := krusty.MakeKustomizer(&krusty.Options{ + LoadRestrictions: kustypes.LoadRestrictionsRootOnly, + PluginConfig: kustypes.DisabledPluginConfig(), // no exec, no Go plugins + }) + resMap, err := k.Run(fSys, path.Join(renderMountPoint, rootDir)) + if err != nil { + return nil, fmt.Errorf("kustomize build: %w", err) + } + return collectRendered(resMap, rootDir) +} + +// refuseRemoteBases refuses the build when any kustomization THIS ROOT REACHES +// declares a remote base. +// +// Scoping it to the reachable graph is deliberate, and it is both safer and more +// accurate than a scan-wide check: kustomize only fetches what it actually loads, +// so a remote base in an unrelated sibling folder cannot make this build reach the +// network — and refusing on its account would refuse a folder that is perfectly +// renderable. +func refuseRemoteBases(kusts map[string]*kustomizationDoc, rootDir string) error { + visited := map[string]struct{}{} + var walk func(dir string) error + walk = func(dir string) error { + if _, seen := visited[dir]; seen { + return nil + } + visited[dir] = struct{}{} + cur := kusts[dir] + if cur == nil { + return nil + } + if hasRemoteResource(cur.resources) { + return fmt.Errorf("%s: %w", cur.path, errRemoteBase) + } + for _, entry := range cur.resources { + target := cleanJoin(dir, entry) + if target == "" { + continue + } + if _, isKust := kusts[target]; isKust { + if err := walk(target); err != nil { + return err + } + } + } + return nil + } + return walk(rootDir) +} + +// renderFilesystem materialises the scan's files in memory and asks the render root +// for provenance. +func renderFilesystem(files []manifestedit.FileContent, rootDir string) (filesys.FileSystem, error) { + fSys := filesys.MakeFsInMemory() + rootKust := path.Join(rootDir, "kustomization.yaml") + + for _, f := range files { + rel := filepathToSlash(f.Path) + content := f.Content + + // Only the root needs to ask for provenance: the annotations describe the + // whole build, bases included. + if rel == rootKust { + var k kustypes.Kustomization + if err := k.Unmarshal(content); err != nil { + return nil, fmt.Errorf("%s: %w", rel, err) + } + k.FixKustomization() + var err error + if content, err = withBuildMetadata(k); err != nil { + return nil, fmt.Errorf("%s: %w", rel, err) + } + } + if err := fSys.WriteFile(path.Join(renderMountPoint, rel), content); err != nil { + return nil, fmt.Errorf("%s: %w", rel, err) + } + } + return fSys, nil +} + +// withBuildMetadata re-serialises a kustomization with the provenance build +// metadata added. It rewrites only our in-memory render copy; the user's file is +// never touched, so losing their comments here costs nothing. +func withBuildMetadata(k kustypes.Kustomization) ([]byte, error) { + k.BuildMetadata = []string{kustypes.OriginAnnotations, kustypes.TransformerAnnotations} + out, err := yaml.Marshal(&k) + if err != nil { + return nil, fmt.Errorf("re-serialising kustomization for render: %w", err) + } + return out, nil +} + +// collectRendered turns kustomize's ResMap into rendered objects, lifting the +// provenance off each one and then stripping it: the annotations are our +// scaffolding, not part of what the folder renders to, and an object carrying them +// would compare unequal to the live object it describes. +func collectRendered(resMap resmap.ResMap, rootDir string) ([]renderedObject, error) { + out := make([]renderedObject, 0, resMap.Size()) + for _, res := range resMap.Resources() { + m, err := res.Map() + if err != nil { + return nil, fmt.Errorf("reading rendered resource: %w", err) + } + obj := &unstructured.Unstructured{Object: m} + ro := renderedObject{ + Object: obj, + OriginPath: originOf(obj, rootDir), + TransformedBy: transformationsOf(obj, rootDir), + } + unstructured.RemoveNestedField(obj.Object, "metadata", "annotations", originAnnotation) + unstructured.RemoveNestedField(obj.Object, "metadata", "annotations", transformationsAnnotation) + if len(obj.GetAnnotations()) == 0 { + unstructured.RemoveNestedField(obj.Object, "metadata", "annotations") + } + out = append(out, ro) + } + return out, nil +} + +// originOf reads the source file an object was rendered from, normalised from +// render-root-relative ("../base/deployment.yaml") to scan-root-relative. +func originOf(obj *unstructured.Unstructured, rootDir string) string { + raw := obj.GetAnnotations()[originAnnotation] + if raw == "" { + return "" + } + var origin struct { + Path string `json:"path"` + } + if err := yaml.Unmarshal([]byte(raw), &origin); err != nil || origin.Path == "" { + return "" + } + return path.Clean(path.Join(rootDir, origin.Path)) +} + +// transformationsOf reads the ordered transformer chain kustomize applied, so the +// writer knows which kustomizations govern this object and in what order. +func transformationsOf(obj *unstructured.Unstructured, rootDir string) []transformation { + raw := obj.GetAnnotations()[transformationsAnnotation] + if raw == "" { + return nil + } + var entries []struct { + ConfiguredIn string `json:"configuredIn"` + ConfiguredBy struct { + Kind string `json:"kind"` + } `json:"configuredBy"` + } + if err := yaml.Unmarshal([]byte(raw), &entries); err != nil { + return nil + } + out := make([]transformation, 0, len(entries)) + for _, e := range entries { + if e.ConfiguredIn == "" { + continue + } + out = append(out, transformation{ + ConfiguredIn: path.Clean(path.Join(rootDir, e.ConfiguredIn)), + Kind: e.ConfiguredBy.Kind, + }) + } + return out +} diff --git a/internal/manifestanalyzer/kustomize_render_semantics_test.go b/internal/manifestanalyzer/kustomize_render_semantics_test.go new file mode 100644 index 00000000..788bebd0 --- /dev/null +++ b/internal/manifestanalyzer/kustomize_render_semantics_test.go @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// The layout corpus barely exercises images:, so on its own it cannot license +// deleting our re-implemented image transformer. This table does: it drives the +// cases the re-implementation actually has to get right — chained renames, digest +// precedence, a registry port that looks like a tag — through BOTH kustomize and +// renderImage, and requires them to agree byte for byte. +// +// Every row is a claim about kustomize's semantics that our code depends on. If +// kustomize ever changes one, this test fails instead of the operator silently +// writing a file that renders to something else. + +func TestRenderImage_MatchesKustomizeOnTheHardCases(t *testing.T) { + cases := []struct { + name string + source string // the container image in the source Deployment + images string // the images: block of the overlay kustomization + }{ + { + name: "newTag only", + source: "ghcr.io/org/web:1.0", + images: " - name: ghcr.io/org/web\n newTag: \"2.0\"\n", + }, + { + name: "newName only", + source: "ghcr.io/org/web:1.0", + images: " - name: ghcr.io/org/web\n newName: ghcr.io/org/web-hardened\n", + }, + { + name: "newName and newTag in one entry", + source: "ghcr.io/org/web:1.0", + images: " - name: ghcr.io/org/web\n newName: ghcr.io/org/hardened\n newTag: \"3.0\"\n", + }, + { + name: "digest replaces the tag", + source: "ghcr.io/org/web:1.0", + images: " - name: ghcr.io/org/web\n digest: sha256:abc123\n", + }, + { + // kustomize's own doc on types.Image: "If digest is present NewTag value + // is ignored." Our renderImage must not disagree. + name: "digest wins over newTag in the same entry", + source: "ghcr.io/org/web:1.0", + images: " - name: ghcr.io/org/web\n newTag: \"9.9\"\n digest: sha256:abc123\n", + }, + { + // The chain interaction that makes a naive inversion wrong: the first + // entry renames, and only then does the second entry's name match. + name: "a rename makes a later entry match", + source: "ghcr.io/org/web:1.0", + images: " - name: ghcr.io/org/web\n newName: ghcr.io/org/renamed\n" + + " - name: ghcr.io/org/renamed\n newTag: \"4.0\"\n", + }, + { + name: "the last matching entry wins", + source: "ghcr.io/org/web:1.0", + images: " - name: ghcr.io/org/web\n newTag: \"2.0\"\n" + + " - name: ghcr.io/org/web\n newTag: \"5.0\"\n", + }, + { + name: "an entry that matches nothing changes nothing", + source: "ghcr.io/org/web:1.0", + images: " - name: ghcr.io/org/other\n newTag: \"7.0\"\n", + }, + { + // The parseImageRef edge case: the colon in a registry port is not a tag + // separator. + name: "a registry port is not a tag", + source: "localhost:5000/org/web:1.0", + images: " - name: localhost:5000/org/web\n newTag: \"2.0\"\n", + }, + { + name: "an untagged source image", + source: "ghcr.io/org/web", + images: " - name: ghcr.io/org/web\n newTag: \"2.0\"\n", + }, + { + name: "a source image that already carries a digest", + source: "ghcr.io/org/web@sha256:oldoldold", + images: " - name: ghcr.io/org/web\n newTag: \"2.0\"\n", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + files := imageFixture(tc.source, tc.images) + + // What kustomize actually renders. + rendered, err := renderRoot(files, ".") + require.NoError(t, err) + require.Len(t, rendered, 1) + slots := collectContainerSlots(rendered[0].Object.Object) + require.Len(t, slots, 1) + kustomizeSays := slots[0].image + + // What our re-implemented chain renders. + kusts := parseKustomizations(files) + doc := kusts["."] + require.NotNil(t, doc) + ours, _ := renderImage(parseImageRef(tc.source), doc.images) + + require.Equal(t, kustomizeSays, ours.String(), + "kustomize renders %q; renderImage renders %q", kustomizeSays, ours.String()) + }) + } +} + +// imageFixture is a one-file render root: a Deployment with one container, plus a +// kustomization carrying the images: block under test. +func imageFixture(sourceImage, imagesBlock string) []manifestedit.FileContent { + deployment := fmt.Sprintf(`apiVersion: apps/v1 +kind: Deployment +metadata: + name: web + namespace: app +spec: + template: + spec: + containers: + - name: web + image: %s +`, sourceImage) + + kustomization := "resources:\n - deployment.yaml\nimages:\n" + imagesBlock + + return []manifestedit.FileContent{ + {Path: "deployment.yaml", Content: []byte(deployment)}, + {Path: "kustomization.yaml", Content: []byte(kustomization)}, + } +} diff --git a/internal/manifestanalyzer/kustomize_render_test.go b/internal/manifestanalyzer/kustomize_render_test.go new file mode 100644 index 00000000..1c7ce1eb --- /dev/null +++ b/internal/manifestanalyzer/kustomize_render_test.go @@ -0,0 +1,277 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/yaml" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// This is the differential test that licenses deleting the re-implemented +// transformers. For every kustomize render root in both corpora it renders the +// folder with kustomize itself and checks two independent claims: +// +// 1. the override CHAIN our resources-graph walk attributes to a document is the +// same chain kustomize says it applied (its transformations annotation), and +// 2. the IMAGE our renderImage chain produces is byte-for-byte the image kustomize +// produces. +// +// A disagreement is either a bug in our re-implementation or a misunderstanding of +// kustomize. Either way it is exactly what we want to find before the write path +// starts trusting the renderer. + +func TestRenderRoot_ChainAndImagesAgreeWithKustomize(t *testing.T) { + roots := allCorpusRenderRoots(t) + require.NotEmpty(t, roots, "no render roots found — the test would prove nothing") + + compared, skipped := 0, 0 + for _, root := range roots { + t.Run(root.name, func(t *testing.T) { + rendered, err := renderRoot(root.files, root.dir) + if err != nil { + // A folder we refuse (remote base, generators, patches, plugins) + // need not render: the gate refuses it and the writer never sees it. + skipped++ + t.Skipf("not renderable, and refused by the acceptance gate: %v", err) + } + + kusts := parseKustomizations(root.files) + for _, ro := range rendered { + if ro.OriginPath == "" { + continue // a generated resource; generators are refused + } + src := sourceDocFor(t, root.files, ro) + if src == nil { + continue // renamed by a transformer we refuse; not a supported shape + } + chain, ambiguous := ourChainFor(kusts, root.files, ro.OriginPath) + assertChainMatchesKustomize(t, ro, chain, ambiguous) + if ambiguous { + continue // we route nothing through it; there is no claim to check + } + compared += assertImagesMatchKustomize(t, ro, src, chain) + } + }) + } + t.Logf("compared %d rendered images against the hand-rolled chain (%d roots skipped as refused)", + compared, skipped) +} + +// assertChainMatchesKustomize checks that the kustomizations our graph walk +// attributes to a document are exactly the ones kustomize says ran an +// ImageTagTransformer over it, in the same order. +// +// The ambiguous case is a deliberate, documented divergence rather than a bug. When +// more than one render root reaches a file with differing chains, we attach NO chain +// (fan-in = 1: we will not route an edit through a file two roots disagree about). +// kustomize, asked to build one root, naturally reports the transformer that root +// ran. So for an ambiguous file we assert the opposite thing: that kustomize DID +// apply a chain and we deliberately declined to claim one. +func assertChainMatchesKustomize( + t *testing.T, + ro renderedObject, + chain *KustomizeOverrides, + ambiguous bool, +) { + t.Helper() + var kustomizeSays []string + for _, tr := range ro.TransformedBy { + if tr.Kind == imageTagTransformer { + kustomizeSays = append(kustomizeSays, tr.ConfiguredIn) + } + } + if ambiguous { + require.Nil(t, chain, "%s: an ambiguous file must carry no chain", ro.OriginPath) + require.NotEmpty(t, kustomizeSays, + "%s: we refused to attribute a chain, so kustomize must have applied one — "+ + "otherwise the ambiguity refusal is guarding nothing", ro.OriginPath) + return + } + + var weSay []string + seen := map[string]bool{} + if chain != nil { + for _, img := range chain.Images { + if !seen[img.Source] { + seen[img.Source] = true + weSay = append(weSay, img.Source) + } + } + } + require.Equal(t, kustomizeSays, weSay, + "%s: kustomize ran ImageTagTransformer configured in %v; our graph walk attributes %v", + ro.OriginPath, kustomizeSays, weSay) +} + +// assertImagesMatchKustomize renders each source container image through our own +// chain and requires it to equal what kustomize actually produced. Returns the +// number of images compared. +func assertImagesMatchKustomize( + t *testing.T, + ro renderedObject, + src *unstructured.Unstructured, + chain *KustomizeOverrides, +) int { + t.Helper() + var entries []ImageOverride + if chain != nil { + entries = chain.Images + } + ours := map[string]string{} + for _, slot := range collectContainerSlots(src.Object) { + got, _ := renderImage(parseImageRef(slot.image), entries) + ours[slot.key] = got.String() + } + + compared := 0 + for _, slot := range collectContainerSlots(ro.Object.Object) { + got, known := ours[slot.key] + if !known { + continue + } + want := slot.image // kustomize's render is the expected truth + compared++ + require.Equal(t, want, got, + "%s container %s: kustomize renders %q, our chain renders %q", + ro.OriginPath, slot.key, want, got) + } + return compared +} + +// ourChainFor is the override chain our resources-graph walk attributes to a file, +// and whether the walk found the file ambiguous (reached by more than one render +// root with differing chains, which we refuse to route through). +func ourChainFor( + kusts map[string]*kustomizationDoc, + files []manifestedit.FileContent, + originPath string, +) (*KustomizeOverrides, bool) { + a := kustomizeOverrideAssignments(kusts, resourceFilePaths(files))[originPath] + if a == nil { + return nil, false + } + if a.ambiguous() { + return nil, true + } + return a.overrides, false +} + +// sourceDocFor finds the document in the origin file that produced a rendered +// object, matched on kind + name (a transformer that rewrites either is refused). +func sourceDocFor( + t *testing.T, + files []manifestedit.FileContent, + ro renderedObject, +) *unstructured.Unstructured { + t.Helper() + for _, f := range files { + if filepathToSlash(f.Path) != ro.OriginPath { + continue + } + for _, chunk := range bytes.Split(f.Content, []byte("\n---")) { + obj := map[string]interface{}{} + if err := yaml.Unmarshal(chunk, &obj); err != nil || len(obj) == 0 { + continue + } + u := &unstructured.Unstructured{Object: obj} + if u.GetKind() == ro.Object.GetKind() && u.GetName() == ro.Object.GetName() { + return u + } + } + } + return nil +} + +type corpusRoot struct { + name string + dir string + files []manifestedit.FileContent +} + +// allCorpusRenderRoots collects every render root from both corpora: the layout +// corpus (real-world repo shapes) and the contextual-namespace fixtures (which are +// what pin the override projection today). +func allCorpusRenderRoots(t *testing.T) []corpusRoot { + t.Helper() + var out []corpusRoot + out = append(out, renderRootsUnder(t, filepath.Join("..", "..", "test", "fixtures", "gitops-layouts"), 2)...) + out = append(out, renderRootsUnder(t, filepath.Join("testdata", "contextual-namespace"), 2)...) + return out +} + +// renderRootsUnder treats every directory at the given depth as one fixture (its +// own scan root) and enumerates the render roots inside it. +func renderRootsUnder(t *testing.T, corpus string, depth int) []corpusRoot { + t.Helper() + var out []corpusRoot + for _, fixtureDir := range dirsAtDepth(t, corpus, depth) { + files := readYAMLTree(t, fixtureDir) + if len(files) == 0 { + continue + } + for _, dir := range renderRoots(parseKustomizations(files)) { + out = append(out, corpusRoot{ + name: strings.ReplaceAll(filepath.Join(filepath.Base(filepath.Dir(fixtureDir)), + filepath.Base(fixtureDir), dir), string(filepath.Separator), "/"), + dir: dir, + files: files, + }) + } + } + return out +} + +// dirsAtDepth lists the directories exactly depth levels below root. +func dirsAtDepth(t *testing.T, root string, depth int) []string { + t.Helper() + dirs := []string{root} + for range depth { + var next []string + for _, d := range dirs { + entries, err := os.ReadDir(d) + require.NoError(t, err) + for _, e := range entries { + if e.IsDir() { + next = append(next, filepath.Join(d, e.Name())) + } + } + } + dirs = next + } + return dirs +} + +// readYAMLTree reads every YAML file under root as scan-root-relative FileContent. +func readYAMLTree(t *testing.T, root string) []manifestedit.FileContent { + t.Helper() + var out []manifestedit.FileContent + err := filepath.Walk(root, func(p string, info os.FileInfo, err error) error { + if err != nil || info.IsDir() { + return err //nolint:wrapcheck // test helper + } + if !strings.HasSuffix(p, ".yaml") && !strings.HasSuffix(p, ".yml") { + return nil + } + content, err := os.ReadFile(p) + if err != nil { + return err //nolint:wrapcheck // test helper + } + rel, err := filepath.Rel(root, p) + if err != nil { + return err //nolint:wrapcheck // test helper + } + out = append(out, manifestedit.FileContent{Path: filepath.ToSlash(rel), Content: content}) + return nil + }) + require.NoError(t, err) + return out +} diff --git a/internal/manifestanalyzer/overrides_projection.go b/internal/manifestanalyzer/overrides_projection.go index 4f96ed02..005566d5 100644 --- a/internal/manifestanalyzer/overrides_projection.go +++ b/internal/manifestanalyzer/overrides_projection.go @@ -104,6 +104,21 @@ type imageSuppliers struct { // renderImage runs the override chain over a source image, kustomize-style: // each entry whose name matches the image's CURRENT name rewrites the // components it declares, in chain order. +// +// Tag and digest are MUTUALLY EXCLUSIVE, and this is the part that is easy to get +// wrong — we did. Quoting kustomize's own image transformer +// (filters/imagetag/updater.go, SetImageValue): +// +// // overriding tag or digest will replace both original tag and digest values +// case NewTag != "" && Digest != "": tag = NewTag; digest = Digest +// case NewTag != "": tag = NewTag; digest = "" +// case Digest != "": tag = ""; digest = Digest +// +// Setting the two components independently makes us believe a folder renders to +// `web:1.0@sha256:abc` where kustomize renders `web@sha256:abc`. The projection +// then reads the difference as a user removing the tag and rewrites the tag out of +// the source file — silent corruption, on every reconcile. Pinned against a real +// `kustomize build` by TestRenderImage_MatchesKustomizeOnTheHardCases. func renderImage(src imageRef, entries []ImageOverride) (imageRef, imageSuppliers) { cur := src var sup imageSuppliers @@ -116,13 +131,16 @@ func renderImage(src imageRef, entries []ImageOverride) (imageRef, imageSupplier cur.name = e.NewName sup.name = e } - if e.HasNewTag { - cur.tag = e.NewTag - sup.tag = e - } - if e.HasDigest { - cur.digest = e.Digest - sup.digest = e + switch { + case e.HasNewTag && e.HasDigest: + cur.tag, cur.digest = e.NewTag, e.Digest + sup.tag, sup.digest = e, e + case e.HasNewTag: + cur.tag, cur.digest = e.NewTag, "" + sup.tag, sup.digest = e, e + case e.HasDigest: + cur.tag, cur.digest = "", e.Digest + sup.tag, sup.digest = e, e } } return cur, sup @@ -277,29 +295,61 @@ func invertImage(slot containerSlot, src string, entries []ImageOverride) (slotP } } if live.tag != rendered.tag { - switch { - case sup.tag == nil: - newSrc.tag = live.tag - case live.tag == "": - return plan, false // tag removal cannot be expressed on an entry - default: - route(sup.tag, "newTag", live.tag) + if !routeComponent(sup.tag, declaresNewTag, live.tag, + func(v string) { newSrc.tag = v }, + func(e *ImageOverride, v string) { route(e, "newTag", v) }) { + return plan, false } } if live.digest != rendered.digest { - switch { - case sup.digest == nil: - newSrc.digest = live.digest - case live.digest == "": - return plan, false // digest removal cannot be expressed on an entry - default: - route(sup.digest, "digest", live.digest) + if !routeComponent(sup.digest, declaresDigest, live.digest, + func(v string) { newSrc.digest = v }, + func(e *ImageOverride, v string) { route(e, "digest", v) }) { + return plan, false } } plan.fileImage = newSrc.String() return plan, true } +// declaresNewTag / declaresDigest report whether an entry actually carries the key +// the writer would have to set. An entry can GOVERN a component without declaring +// it: a digest entry clears the tag, and a newTag entry clears the digest. +func declaresNewTag(e *ImageOverride) bool { return e.HasNewTag } +func declaresDigest(e *ImageOverride) bool { return e.HasDigest } + +// routeComponent decides where one changed image component (tag or digest) goes: +// into the source file when no entry supplies it, onto the supplying entry when +// that entry declares the key, or nowhere at all. It reports false when the change +// is unroutable, which abandons routing for the whole object (write-through). +// +// The two unroutable cases are worth naming: +// +// - a REMOVAL of a component an entry supplies — there is no way to say "no tag" +// on an entry that sets one; and +// - a change to a component an entry governs but does not declare — a digest entry +// clears the tag, so setting a tag has no key to land in, and writing it into the +// source file would be undone by the very next render. +func routeComponent( + sup *ImageOverride, + declares func(*ImageOverride) bool, + live string, + setSource func(string), + route func(*ImageOverride, string), +) bool { + switch { + case sup == nil: + setSource(live) // the source file supplies it; the change flows into the file + case live == "": + return false + case !declares(sup): + return false + default: + route(sup, live) + } + return true +} + // collectConsistentEdits dedupes the per-container edits, refusing when two // containers demand different values for the same entry field. Output order is // deterministic (path, section, index, field). diff --git a/internal/manifestanalyzer/overrides_projection_test.go b/internal/manifestanalyzer/overrides_projection_test.go index 67a41f67..68983bd3 100644 --- a/internal/manifestanalyzer/overrides_projection_test.go +++ b/internal/manifestanalyzer/overrides_projection_test.go @@ -149,9 +149,15 @@ func TestSplitDesired_NameChangeRoutedToNewName(t *testing.T) { // TestSplitDesired_DigestRoutedToEntry: a live digest change whose supplier is a // digest entry updates the entry; tag and name keep their source form. +// +// Note the live image carries NO tag. A digest entry replaces the tag as well as +// the digest (kustomize: "overriding tag or digest will replace both original tag +// and digest values"), so `app@sha256:...` is the only thing this folder can +// render. This test used to assert `app:1.0.0@sha256:ccc` — a live state kustomize +// cannot produce — because renderImage set the two components independently. func TestSplitDesired_DigestRoutedToEntry(t *testing.T) { git := deploymentObj("app:1.0.0", nil) - desired := desiredOf(deploymentObj("app:1.0.0@sha256:ccc", nil)) + desired := desiredOf(deploymentObj("app@sha256:ccc", nil)) ov := &KustomizeOverrides{Images: []ImageOverride{ imgEntry("app", map[string]string{"digest": "sha256:bbb"}), }} @@ -169,6 +175,33 @@ func TestSplitDesired_DigestRoutedToEntry(t *testing.T) { } } +// TestSplitDesired_DigestEntryDoesNotStripTheSourceTag pins the corruption that a +// hand-written image transformer caused, so it cannot come back. +// +// Source `app:1.0.0`, an entry supplying only a digest. kustomize renders +// `app@sha256:bbb` — the digest REPLACES the tag. We used to believe the render was +// `app:1.0.0@sha256:bbb`, so on seeing the real live object (no tag) the projection +// concluded the user had removed the tag and rewrote `app:1.0.0` to `app` in the +// source file. On every reconcile, silently. +// +// The source document must come back untouched. +func TestSplitDesired_DigestEntryDoesNotStripTheSourceTag(t *testing.T) { + git := deploymentObj("app:1.0.0", nil) + // What the folder actually renders to, mirrored back unchanged by the user. + desired := desiredOf(deploymentObj("app@sha256:bbb", nil)) + ov := &KustomizeOverrides{Images: []ImageOverride{ + imgEntry("app", map[string]string{"digest": "sha256:bbb"}), + }} + + out, edits := SplitDesiredForOverrides(git, desired, ov) + if got := desiredImage(t, out); got != "app:1.0.0" { + t.Errorf("source image = %q, want it untouched at app:1.0.0 — the tag must not be stripped", got) + } + if len(edits) != 0 { + t.Errorf("live matches the render, so nothing should be routed; got %+v", edits) + } +} + // TestSplitDesired_RemovalUnroutable: live drops the digest an entry supplies; // nothing can express that on the entry, so the whole object writes through. func TestSplitDesired_RemovalUnroutable(t *testing.T) {