feat(codebuild): retain a digest-pinned rollback target across an in-place rebuild - #173
Conversation
An in-place rebuild moves the image tag on to the new digest and prunes the manifest it left untagged, so a consumer pinning the immutable digest reference loses that digest as soon as the next build runs. There is no way to keep a rollback target. Declare the additionalTags input the fix will honour, and grow the image build fixture family into the three-step case that proves the behaviour against a real registry: build, rebuild in place so the digest moves, then build FROM the pinned predecessor. The replace step asserts twice, both times over the registry rather than over anything the plugin records — the FROM cannot resolve unless the predecessor survived, and its guard fails if the pin was moved on to the rebuilt manifest. It declares no pin of its own, since a replace is a create, where every declared pin is new by definition. The provisioner is deliberately untouched, so this run is red: the field is accepted and ignored, the predecessor is pruned, and the last build cannot resolve its base image. That is what separates the fix from a test that could never have failed.
An in-place rebuild moves imageTag to the new digest and leaves the manifest it came from untagged, and an untagged predecessor is exactly what the resource prunes. A consumer pinning the immutable digest — the documented way to use this resource — therefore lost the image it pinned as soon as the next build ran, with no way to keep a rollback target. Honour the additionalTags listing: immutable pins placed on the manifest a build pushes, alongside the mutable tag. A pinned predecessor is not untagged, so the existing prune skips it on its own — retention comes from the prune's own rule rather than from weakening it, and the teardown path is unchanged. Pins are create-once. A pin new to an apply is placed on the manifest that build produced; one already naming a different image fails the apply rather than being moved, since repointing it would destroy the rollback target the operator believes that name still holds. A pin carried over is left alone. Placement re-registers the identical manifest bytes and media type, so it is digest-preserving by construction, and pins are excluded from the build-config hash: adding one places a tag on the image already pushed instead of rebuilding one that would not be the image being pinned. The whole declared listing is reported back from every path that writes properties, Read included. The caller rebuilds its stored model of a list-valued property from what the plugin returns, so a build that placed a pin but stayed silent about it would drop the listing from that model and classify the same pin as new on the next apply. Read has no way to recover a pin from the registry alone — it names a predecessor, indistinguishable from any other tag in a shared repository — so it reports the pins the caller's model declares, checked for existence against ECR, which leaves an out-of-band deletion visible as drift. The request id grows a pin listing and a new-pin listing, and still decodes the form emitted before pins existed, so a build in flight across an upgrade is polled rather than stranded.
…epository The image-build fixtures retain an immutable pin on the predecessor manifest, which is the whole behaviour under test. That pin is exactly what leaves the push-target repository holding an image at teardown, and CloudControl refuses to delete a repository that still contains images, so the whole-stack Destroy failed while the fixture managed the repository itself. `emptyOnDelete` does not rescue it: CloudControl's delete carries no resource model, so the repository's delete handler never sees the property. Make the push target a persistent, formae-unmanaged prerequisite instead, provisioned and emptied by the cleanup script and referenced by name — the same arrangement the CloudTrail fixture uses for its log-delivery bucket, for the same reason. The repository URI and ARN are assembled literally from the account id, since there is no managed resource to take a resolvable from. The cleanup script creates the repository if absent and deletes every image it holds, but never deletes the repository, and the prefix sweep skips it. Emptying is bounded rather than run to a fixed point: batch-delete-image exits zero even when individual ids land in its failures, so an unbounded loop would re-list the same images forever.
The `additionalTags` documentation told operators that a repository which accumulates pins should declare `emptyOnDelete` or carry a lifecycle policy. The first half of that is false: `EmptyOnDelete` is a write-only property and CloudControl's delete request carries no resource model, so the repository's delete handler never receives it and always deletes without force. State what is actually true instead. A repository that accumulates pins cannot be emptied by tearing the ImageBuild down, because every pin is by design a manifest meant to outlive the build that produced it, so its lifecycle is the operator's to manage — a lifecycle policy that ages pins out, or an out-of-band delete. Note also that a managed repository still holding images cannot currently be destroyed at all, rather than pointing at a property that has no effect.
JeroenSoeters
left a comment
JeroenSoeters
left a comment
There was a problem hiding this comment.
Approved. Reviewed the whole diff, the ledger, and the conformance evidence independently rather than on the ledger's word.
The evidence is on the right SHA. Run 31765439183 concluded success with headSha 4f5b2c31639627a7a16f6a3468719775a757e0ed, which is this PR's head, not an ancestor. Every column green including Destroy and OOB Del, the two that failed in round 3. The deliberate RED (31758446538 on b375892) is genuine: the test job ran 4m36s creating real resources and ended FAIL ... 226.410s on a "does not exist" signature, so the fixture does discriminate.
The assertion that matters is Replace: FROM <repo>:sdk-pin-<runid> cannot resolve unless the predecessor survived the prune, and RUN test ! -f /rebuilt fails if the pin moved onto the rebuilt manifest. Both held against real AWS. That is the defect demonstrated fixed, not a test asserting intent.
Everything I said I would check at review time, checked:
- The ledger's one open item is established by reading the engine, with file and line, not assumed: a terminal
Statusfailure persists nothing (resource_updater.go:947/:994, re-gated inresource_persister.go:304). The honest consequence, a manifest leaked on the failure path only, is recorded rather than argued away. - Delete stays scoped to the
NativeIDtag.TestDeleteRemovesPushedImagematches onlen(ImageIds) == 1and the tag, so it fails if teardown ever grows to cover pins. Correctly reused instead of duplicated. BatchDeleteImageis never called when the predecessor still carries a tag:TestStatusSucceededSkipsPruneWhenPriorStillTagged.decodeRequestIDaccepts the new form and the pre-pins form, so a build in flight across a plugin upgrade is polled rather than stranded. Ten fields rather than the planned nine, with the reason recorded and no released version having emitted nine.validateInputcaps at 20, rejects duplicates, rejects a pin equal toimageTag, and applies the tag pattern.TestPlacePinsPreservesAnOCIIndexMediaTypecovers the index media type round-trip.- CHANGELOG covers both
additionalTagsand the predecessor prune, the latter never previously written down despite shipping with the resource.
The inertness property holds, which was the blocker condition: AdditionalTags is omitempty, placePins returns before touching ECR when the listing is empty, TestPlacePinsWithoutPinsTouchesECRNotAtAll asserts it, and buildConfigHashBody deliberately excludes the field so adding a pin cannot force a rebuild. With additionalTags unset nothing changes at all.
Two things I want to note as good judgement rather than as findings. Surfacing a non-ImageNotFound BatchGetImage failure instead of reading it as "the tag is free" was self-raised and is exactly right: reading "could not say" as "absent" would move a pin, which is the one thing the create-once rule exists to prevent. And bounding the clean-environment.sh emptying loop at 20 passes, because batch-delete-image exits 0 with ids in failures[], avoids hanging a CI job under set -euo pipefail in a way that would have been painful to diagnose.
The unmanaged push-target repository is the right call and is prior art, not a deviation: testdata/cloudtrail-trail.pkl:18-23 does the same for its log bucket for the identical reason. The section-28 exemption is live rather than dead code, since formae-plugin-sdk-test-imgbuild-<acct> does contain the plugin-sdk-test substring the sweep filters on.
The accumulation cost is real and correctly stated in three places rather than hidden: a repository carrying pins cannot be emptied by tearing the ImageBuild down, and emptyOnDelete will not rescue it. Pointing at PLA-657 instead of recommending a property that does not work is the honest version.
No unresolved blocker, correctness or security finding. checks is green; the skipped conformance/integration jobs are the normal PR-time state.
1 participant
Summary
AWS::CodeBuild::ImageBuilddeletes the predecessor image on every successful in-place rebuild. A consumer that pins the immutableimageRefdigest — the documented way to use this resource — finds that digest unpullable as soon as the next build runs, so there is no way to keep a rollback target. The symptom is a running service that survives only because its image is still cached on the host, and cannot pull the moment a new task starts.Removing the prune is not the fix: it is load-bearing. An in-place rebuild moves the tag to a new manifest and leaves the old one untagged, and without the prune a co-managed repository never empties enough to tear down.
This adds an optional
additionalTagslisting. Each entry is a create-once immutable pin placed on the manifest the build produced, so when the next rebuild moves the mutable tag off that manifest, the predecessor still carries a tag of its own — and the existing prune, which deletes a manifest only when it carries no tags, skips it on its own. Retention falls out of the prune's existing rule rather than from weakening it:prunePriorDigestanddeletePushedImageare unchanged, and a repository's teardown behaviour is unaffected for anyone who declares no pins.The contract, which is the substantive part:
additionalTagsis mutable, not create-only. A create-only tag list would re-create the resource when a tag is added, and the delete half of that replace would destroy the predecessor the field exists to keep.additionalTagsdeclared, behaviour is unchanged: same prune, same delete, byte-identical builds.Pins are placed by re-registering the exact manifest bytes and media type already in the repository, which is digest-preserving by construction — the pin resolves to the identical image rather than to a rebuilt one. Placement happens in the plugin rather than in the buildspec, so the build project's service role needs nothing new and a placement failure surfaces as a resource error with a real message instead of an opaque red build.
Two consequences are documented rather than papered over. A repository that accumulates pins cannot be emptied by tearing the
ImageBuilddown, so its lifecycle is the operator's to manage — and a formae-managed ECR repository still holding images cannot currently be destroyed at all, which is tracked separately. A pin placed and later dropped from the listing leaves a durable tag that is not removable through this resource.The conformance fixtures push to a persistent, formae-unmanaged repository provisioned by the CI cleanup script, following the existing precedent for the CloudTrail log-delivery bucket — a container that cannot be destroyed while non-empty is the same problem, and managing it in the fixture breaks the whole-stack destroy. The replace fixture asserts against the real registry rather than against the plugin's own bookkeeping: it builds
FROM <repo>:<pin>, which cannot resolve unless the predecessor survived the prune, and runsRUN test ! -f /rebuilt, which fails if the pin moved onto the rebuilt manifest.The CHANGELOG also records the predecessor-prune behaviour itself, which has been present since the resource landed and was never written down.