fix(kustomize): a digest: override no longer strips the tag out of the source file#231
Conversation
…e source file
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) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…e-render, delete the re-implementation (#233) * docs(support-boundary): record what building the kustomize observer patch changed The patch argued for in patching-kustomize.md has now been built and measured on feat/build-trace-observer in external-sources/kustomize (290d04199, on top of upstream 79bb1aa2b): 638 lines added across 11 files, upstream's api suite still green. Building it corrected the doc on three points, now recorded in a new section 9: - the patch should be an *additive* krusty.Options.Observer, not a change to the transformer annotation's semantics -- no golden churn, no kubectl output change, and it carries the entry index and field paths the annotation structurally cannot; - the zero-cost gate is structural rather than merely safe, and is now asserted by TestBuildTraceDoesNotChangeTheBuild; - two facts only the running code produced: matching a resource across a rename needs an EffectiveNamespace-normalised key, and ConfiguredIn is relative to the build root rather than the repo root. The measured trace also settles section 3's headline claim by test rather than by argument: an idempotent pin produces no event at all, which is the case the dye was invented for and the one leave-one-out probing cannot answer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(kustomize): a kustomization.yml render root gets its provenance renderFilesystem asked for buildMetadata only on a root spelled exactly "kustomization.yaml". Every other part of the analyzer accepts kustomization.yml too (isKustomizationFile), so a repository whose root is spelled .yml built happily with no buildMetadata at all: no config.kubernetes.io/origin, no alpha.config.kubernetes.io/transformations. Downstream that is not a missing annotation, it is a wrong answer. Every rendered object came back with an empty OriginPath and an empty transformation list, which reads as "this document is governed by no kustomization" -- so the images:/replicas: override chain was empty, and a live tag bump governed by an entry was written into the SOURCE MANIFEST, where the overlay shadows it straight back on the next render. Non-converging drift, silently, on every reconcile. Measured against main by the new test, which fails there with OriginPath="" and an empty chain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(kustomize): renderRootWith, the counterfactual render renderRoot becomes renderRootWith over an empty overlay. The new call builds a render root with in-memory replacements layered over the scanned files, so a question about what kustomize would do with a DIFFERENT tree goes through the same sandbox, the same pre-build refusals and the same krusty invocation as the render we ship. It is the primitive the rest of this workstream is made of. Both remaining questions are this call with a different overlay: dye an entry and see where the nonce lands (attribution), or apply a proposed write and see what it renders (verification). No behaviour change on its own: every existing caller renders with no overlay. See docs/design/support-boundary/render-attribution.md section 7. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(kustomize)!: verify a kustomize write by re-rendering it, not by simulating it simulateImageRender "verified" the projection's inversion by replaying OUR chain over the images OUR projection had planned. A check that shares the blind spot of the thing it checks cannot fail where that thing is wrong -- it agrees with itself -- so it did not catch a wrong attribution, it made one CONFIDENT. Both shipped image bugs (#231's digest/tag corruption, and the regex-vs-equality matcher) sailed straight through it. Replace it with the real thing. A flush that routes anything through a kustomization now re-renders every render root twice -- as it found the tree, and as it would leave it -- and proves both halves of the oracle: 1. every document the flush writes renders to exactly the live object, and 2. every object it does not write comes out byte-for-byte unchanged. (2) is the half that makes it safe for the projection to guess. A kustomization is shared context: an images: entry edited to converge one Deployment governs every other object it matches. A proposal that fixes its own target and moves a second object has written a live value into a file another render root also reads. It runs ONCE PER FLUSH, not once per resource, and that is correctness rather than economy. Asked resource by resource, "did anything else move?" is YES for the first of two Deployments that share an image and are bumped together -- so a per-resource oracle refuses a write that converges perfectly well. Only the whole batch can see that the sibling is moving because it, too, is being written to exactly where it now lives. That is why every document a flush touches declares a WriteIntent, and why a governed one declares it even when its own bytes did not change. A refused proposal REFUSES THE FLUSH -- an AcceptanceRefusedError surfacing as GitPathAccepted=False / WriteBoundaryRefused, naming the file and the object. It is not absorbed into a silent skip: a write that does not survive the re-render is one the entry overrides straight back on the next render, so absorbing it would leave the resource un-mirrored forever with nothing to show for it (render-attribution.md section 7). Two writes are declared UNCHECKED rather than compared, and it is stated rather than hidden: a SENSITIVE document (SOPS ciphertext -- no plaintext live object can equal it) and a bounded FIELD PATCH (a few audited assignments, never a whole object). Both are still held to disturbing nothing else. Attribution is untouched here and still comes from renderImage, so no routing changes. That is the point: the check now cannot share the blind spot of what it checks, which is what makes the attribution swap that follows safe to make at all. See docs/design/support-boundary/render-attribution.md section 5 and docs/design/support-boundary/render-root-scoping.md section 3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(kustomize)!: attribute override values with a dyed render, not a re-implementation Deletes the last of the hand-rolled kustomize: renderImage, imageSuppliers and isReplicaKind, the ~400 lines the write path used to decide which file an edit belongs in. The projection now inverts against what kustomize ACTUALLY renders, read off the renderer, instead of against our re-derivation of it. HOW IT WORKS. A render says web:2.0; it never says who supplied the 2.0, and kustomize keeps no field-level provenance anywhere, at any visibility level. So attribution cannot be READ out of it -- only inferred by questioning it. Write a unique nonce into every declared override entry, render the root a second time, and read the nonces off the output: wherever a dye lands, THAT entry supplied THAT field. Two builds per root, constant in the number of entries. WHY NOT LEAVE-ONE-OUT. Removing an entry and seeing what moves is blind exactly where it matters. A base at app:v1 under an overlay pinning newTag: v1 -- the state every repo is in the moment a release lands in both places -- moves NOTHING when the entry is removed, so the probe concludes the source file owns the tag, writes the user's next tag into the base, and the overlay overrides it straight back. Forever. Removal probes the VALUE, and values collide. A nonce nothing else can produce does not. Both cases are now tests (idempotent pin, and a tie between two entries). GUARDRAILS, all measured, none optional: - the nonce charset is a CORRECTNESS requirement. kustomize does not validate a tag, but it MATCHES on the image with a regex, so an out-of-charset dye leaves the image un-matchable and every later entry SILENTLY STOPS FIRING. Tag dyes stay in [a-zA-Z0-9_.{}-]; digest dyes carry the mandatory sha256: prefix. - newName is not a pure sink -- it is the join key for every later entry -- so it is dyed only when no other entry's name: matches it. And "matches" is asked of kustomize's own compiled pattern, not of string equality, because an entry name is a REGEX. - only a field an entry already DECLARES is dyed; injecting one would fabricate a supplier. - baseline first, then dye. A dyed build that fails where the real one succeeded means the dye hit something that is not a sink, and the answer is then NO ATTRIBUTION -- never a fallback to another heuristic. Nothing routes, and the oracle from the previous commit adjudicates whatever the source document alone can carry. THREE SHIPPED BUGS DIE WITH THE CODE THAT CAUSED THEM (see docs/UPGRADING.md): - B1: our image matcher was string equality; kustomize's is a regex over the whole image string. `- name: "ap."` matches `app` in kustomize and did not in us, so we believed the folder rendered one thing while it rendered another -- and wrote the difference into the source manifest, killing the entry. - B2: isReplicaKind listed Deployment, ReplicaSet, StatefulSet. kustomize's fieldspec also includes ReplicationController -- it says so in its own error message. A scale on an RC was written into the source document, where the transformer overrode it back. - B3: we collected ephemeralContainers (kustomize does NOT rewrite them) and missed volumes[].image.reference (kustomize DOES -- measured). Both directions mis-attributed. None of these are fixed. They are DELETED, along with the fieldspec we had no business keeping a second opinion about. The dye does not decide what kustomize touches; it reads where kustomize's own nonces came out. THE TEST NET IS REBUILT, which the signature change required and the change deserved. The 12 TestSplitDesired_* tests constructed (gitRaw, desired, overrides) BY HAND -- they asserted what we BELIEVED a folder renders to, which is the belief that was wrong twice in shipped code, so they could not have caught either bug and did not. Every case now builds a real tree, renders it with kustomize, reads the attribution off a dyed render, and drives the projection with the result. TestRenderImage_MatchesKustomizeOnTheHardCases is gone with its subject; the corpus differential is replaced by a stronger invariant across both corpora: AN IN-SYNC FOLDER MUST PROJECT TO A COMPLETE NO-OP -- which is exactly the property #231's digest/tag corruption violated. One more landmine confirmed while building it: a rendered object is not even a valid unstructured. kustomize hands numbers back as Go `int`, so DeepCopyJSON PANICS on one ("cannot deep copy int") and NestedInt64 reports found=false. The projection reads replica counts with a type switch, and the corpus test normalises through JSON. The corpus support baseline is byte-for-byte unchanged: this changes how attribution is derived, not which folders are supported. See docs/design/support-boundary/render-attribution.md sections 3, 6 and 7. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(kustomize): record the deleted bugs, and close the attribution ledger UPGRADING.md gets the entry B1, B2 and B3 are owed. All three shipped, and all three had the same shape: we believed a folder rendered one thing while kustomize rendered another, and the projection wrote the difference into the user's source manifest as though they had typed it. They arrive as deletions rather than fixes, which is the point. It also records the behaviour change that comes with them: a write routed through a kustomization is now re-rendered before it is committed, and one that does not reproduce the live object refuses the flush instead of landing. render-attribution.md marks section 7 done through stage 3, and records the two things building it corrected: - B4 is worse than the ledger said. A rendered object is not merely awkward to read a number off, it is not a valid unstructured at all: DeepCopyJSON PANICS on the Go int kustomize hands back. - the rename-chain guard has to use kustomize's own compiled regex, not the string equality section 3 proposed -- an entry name is a regex, so `mirror/ap.` matches `mirror/app` without equalling it, and an equality guard would dye the name, kill the next entry, and mis-attribute the tag. The ledger itself is kept as written rather than deleted, because how each bug was found is the whole argument for the method: not one came from reading kustomize's source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(support-boundary): move the kustomize-fork docs out of this repo The three docs about the kustomize build-trace fork (patching-kustomize, generated-repo-map, render-plan-artifact) now live in the fork itself, under plans/ in ConfigButler/kustomize-tracer, next to the code they describe and the tests that verify them. They remain in this repository's history; this removes them going forward. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(support-boundary): repoint the links to the relocated fork docs render-attribution.md linked patching-kustomize.md, which now lives in the fork (ConfigButler/kustomize-tracer, plans/). Left as a plain reference so the link checker does not chase it into a repository it cannot see. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(kustomize): close the two holes an adversarial review found in the oracle Both were in the new code, and both are proved by a test that fails without the fix. 1. THE WRITE AND THE INTENT ARE DIFFERENT OBJECTS. createNew strips metadata.namespace out of the bytes it writes when the destination inherits its namespace from a kustomization's namespace: transformer -- correct, because the transformer puts it back, and every sibling in that directory follows the convention. But the three writers then built the WriteIntent from that same stripped object. The render HAS the namespace, so the intent demanded a render that did not, and the oracle refused a flush it had just planned perfectly: the new resource was lost, and so was any governed write batched with it. Resync replays the same batch, so it never recovered. patchExisting had it right all along -- it strips a LOCAL copy and hands the untouched event to the intent -- which is exactly the asymmetry that gave the bug away. Intent recording moves out of the three writers, which only ever see the mutated object, and up to the callers, which still hold both. This fires on the most ordinary kustomize layout there is: a per-env folder with a namespace: and an images: entry, plus any new resource. 2. A NEW RESOURCE COULD NOT REACH THE ORACLE AT ALL. Its intent was never Governed (Governed came from dm.Overrides -- a PRE-batch store document, which a new document by definition does not have), so a flush containing only new resources never turned the oracle on. And createNew does not route a new document's values onto an entry: it has no override chain yet. So a new Deployment whose image an existing images: entry matches was committed at its live tag, the entry rewrote it on the next render, and the mirror asserted a state the folder does not produce -- forever. Silently. That is precisely the failure this whole path exists to prevent, in the one write path the oracle could not see. It still does not route (that needs attribution for a document that does not exist yet), but it is now put in front of the oracle, which turns a silent non-converging commit into a reported refusal naming the file and the object. "We cannot express this here" is an answer. Quietly writing a lie is not. Turning the oracle on is now a separate question from WriteIntent.Governed, because the two are not the same claim: Governed additionally ASSERTS the document is rendered, and a new document is not entitled to that -- its resources: entry can legitimately fail to be added (a kustomization with no resources: sequence), leaving the file written but outside every render. Conflating them refused that case too, which the placement suite caught. Found by an adversarial review of the diff. It also cleared the dye itself: nonce collision, by-position alignment, and the tag/digest routing all held. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(kustomize): deleting a resource also removes its resources: entry Registering a new resource in a kustomization's resources: list was only half the job. Nothing ever took the entry back out, so deleting the manifest left an entry naming a file that no longer exists -- and kustomize refuses to build over that: accumulating resources ... '/scan/bundle.yaml' doesn't exist The folder became undeployable, and the GitTarget was refused on the next reconcile. RemoveKustomizationResource is AppendKustomizationResource read backwards, with the same all-or-nothing semantics: idempotent, and a kustomization it cannot edit (multi-document, no resources: sequence, unparseable) is skipped with a diagnostic rather than having structure invented for it. THE ENTRY COMES OUT ONLY WHEN THE FILE ACTUALLY GOES. A file holding several documents survives the deletion of one of them, and its resources: entry must survive with it -- pulling it would un-deploy every OTHER resource in that file, which nobody asked to touch. That invariant is the first test, and it is one that bites: dropping the entry on any delete rather than only on the last one fails it immediately. Three unit tests at the writer (the multi-document guard, the last-document removal, and a delete batched with a governed write -- which without this could not build the counterfactual tree at all, so the oracle refused the whole flush), three at the editor, and an e2e that closes the round trip against the real controller: the placement spec creates a ConfigMap in a kustomize overlay and asserts the entry appears; the new spec deletes it, asserts the entry is gone, and then asks the cluster to `apply -k` the folder -- which is the whole point, and which a dangling entry fails. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(kustomize): drop what the projection swap made untrue render-root-scoping.md still listed renderImage and isReplicaKind as "still there" with their divergences described in the present tense, and still planned the oracle as future work whose differential test was to be a comparison against simulateImageRender. All three are deleted, so those rows and that step now describe code that does not exist. render-attribution.md loses its "one step left" framing and the long status block: the workstream is done, so the doc is a record of how attribution was decided, not a plan. No content is removed that still says something true. The bug ledger stays exactly as written, because how each bug was found is the argument for the method. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(kustomize): address review on #233 -- verify every delete, and ask both questions in anyOverrides Three findings from the review. One was right, one was wrong but pointed at a real gap next to it, and one was a lint break. 1. anyOverrides only asked whether a CHAIN existed, never whether an ATTRIBUTION did. A document can render an image with no images:/replicas: entry governing it: the chain is nil, the attribution is not. Two roots disagreeing only there would diverge in the fingerprint while ambiguous() stayed false, so the document would be silently un-routed with no diagnostic and no fan-in refusal. Routing was already safe (record() nils the attribution on any divergence), but the guard was not. The divergence is not constructible today -- a nil chain means no image transformer ran, which means the rendered value IS the source value, which is the same in every root -- so this changes no current behaviour. It is fixed because the invariant is not obvious enough to leave resting on that argument. Test added, as asked. 2. The claim was that applyDelete never sets putToKustomize, so a delete could commit a dangling resources: entry. That is not so: dropKustomizationResource sets the flag BEFORE attempting the removal, so a removal that fails still puts the flush to the oracle. Measured: forcing RemoveKustomizationResource to fail refuses the flush with kustomize's own "'/scan/bundle.yaml' doesn't exist". But the instinct was right about the case beside it. A delete that does NOT empty the file never set the flag at all, so the rule was "verified only when our own bookkeeping says the file went" rather than "verified because it is inside a render root". The whole point of the oracle is that it does not take our word for anything, so the rule is now uniform: any delete of a document whose file is named in a resources: list goes to the oracle. The lookup that decides that is now one function shared with the entry removal, so the two cannot drift. 3. MD028: a bare blank line ended the blockquote instead of continuing it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(support-boundary): the prompt for tolerating patches, with the ordering fact measured patches is the biggest refusal cause in the layout corpus (8, against namePrefix 6 and configMapGenerator 6), it refuses the whole GitTarget rather than the edit, and the dye shipped in #233 is the mechanism its milestone was waiting for. Carries the four things worth measuring rather than re-deriving, the load-bearing one being that patches run BEFORE the images:/replicas: transformers and the transformers WIN -- so a patch that sets a field an entry also governs is dead text. That is the fact that makes 'just edit the patch' wrong, and the reason the dye should be extended into the patch rather than the patch being read and reasoned about. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The bug
If any of your kustomizations use
images:withdigest:, the operator has been rewriting your source manifests. Shipped since v0.30.0, introduced with the images/replicas edit-through in #198.kustomize's image transformer treats tag and digest as mutually exclusive. Its own code says so, in a comment:
Our re-implemented transformer set the two components independently:
images:entryapp:1.0.0digest: sha256:bbbapp@sha256:bbbapp:1.0.0@sha256:bbbapp@sha256:oldnewTag: "2.0"app:2.0app:2.0@sha256:oldBelieving 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.0becameapp. On every reconcile, silently, with no refusal and no diagnostic.Proven by reintroducing the bug against the new regression test:
docs/UPGRADING.mdtells affected users to check their history; the fix stops the rewrite but cannot restore what was already committed.How it was found — and the rest of this PR
I did not find this by reading the code. I found it by rendering the corpus with kustomize itself and comparing.
internal/manifestanalyzer/kustomize_render.go— a sandboxedkrustybuild: in-memory filesystem, plugins disabled, and remote bases refused before the build. That last part is load-bearing and was measured, not assumed: kustomize resolves a remote base by shelling out to/usr/bin/git fetch, and does so underLoadRestrictionsRootOnlyand under an in-memory filesystem. No build option turns it off, so refusing first is the only thing that keeps "the operator never fetches a remote base" true.kustomize hands us the provenance we were computing by hand. With
buildMetadataon, every rendered object carriesconfig.kubernetes.io/origin(which source file produced it) andalpha.config.kubernetes.io/transformations(which kustomization's transformers touched it, in build order). That second one is the override chain — reconstructed today by a hand-written resource-DAG walk.Two differential tests. One renders every render root in both corpora and requires (a) our graph walk to attribute the same chain kustomize reports and (b) our
renderImageto produce the same image kustomize produces. The other 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 engines and requires byte-for-byte agreement.The first differential run found the two bugs above immediately.
Scope
The renderer is not on the write path yet. It is currently the oracle the re-implementation is checked against. Ripping out the re-implementation (
renderImage,imageSuppliers,simulateImageRender,isReplicaKind, and the DAG walk) is the next PR — and it is now a verified swap rather than a brave one.One fix to my own renderer while here: the remote-base pre-check originally scanned every kustomization in the tree, so an accepted folder was refused because an unrelated sibling had a remote base. It is now scoped to the graph reachable from the root being built.
Validation
task fmt·generate·manifests·vet·lint·lint-docs·testall pass.task test-e2e: 54 of 65 specs passed, 0 failed (11 skipped = the opt-in bi-directional corner).🤖 Generated with Claude Code