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) {