fix(convertor): drop BuildKit attestation manifests instead of panicking#379
Open
mvanhorn wants to merge 1 commit into
Open
fix(convertor): drop BuildKit attestation manifests instead of panicking#379mvanhorn wants to merge 1 commit into
mvanhorn wants to merge 1 commit into
Conversation
…f panicking Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Member
There was a problem hiding this comment.
Pull request overview
This pull request updates the userspace convertor’s multi-platform index traversal to detect and omit BuildKit attestation manifests (e.g., provenance/SBOM entries) from conversion output, preventing a known panic when these “zero-rootfs-layer” manifests are mistakenly processed as runnable image manifests.
Changes:
- Adds
isAttestationManifest()to detect BuildKit attestation entries viavnd.docker.reference.type=attestation-manifest. - Skips converting attestation descriptors during index traversal (with a warning) and removes them from the rebuilt output index.
- Adds unit tests covering attestation detection cases, including unknown/unknown platform handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/convertor/builder/builder.go | Detects and drops BuildKit attestation manifests during index conversion to avoid the zero-layer panic and keep the output index consistent. |
| cmd/convertor/builder/builder_attestation_test.go | Adds unit coverage for attestation-manifest detection logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The userspace convertor panics on OCI indexes that contain a BuildKit attestation manifest (images built with
docker buildx --provenance=true/--sbom=true). This detects attestation manifests by their explicit annotation and drops them from the converted index (with a warning) instead of feeding a zero-layer manifest into the rootfs builder engine.Motivation
Provenance is enabled by default with the Buildx
docker-containerdriver since v0.10, so ordinary multi-platform images now carry an attestation manifest in their index. Converting such an image crashes the tool withpanic: index out of range [0] with length 0, which makes overlaybd conversion unusable for the common default Buildx output until the attestation entry is manually stripped. Closes the crash so these images convert cleanly.What this PR does / why we need it:
The userspace convertor panics (
panic: index out of range [0] with length 0inNewOverlayBDBuilderEngine()/NewTurboOCIBuilderEngine()) when an OCI index contains a BuildKit attestation manifest — for example an image built withdocker buildx --provenance=true,--sbom=true, or--attest. Provenance is enabled by default with the Buildxdocker-containerdriver since v0.10, so this is hit by ordinary multi-platform images.An attestation manifest uses the image-manifest media type, so
graphBuilder.process()dispatched it tobuildOne(), which builds a rootfs-layer engine over a manifest that has zero layers — hence the index-out-of-range panic. BuildKit marks these entries with anunknown/unknownplatform and the annotationvnd.docker.reference.type: attestation-manifest.This change:
isAttestationManifest(), which detects an attestation entry by BuildKit's explicitvnd.docker.reference.type: attestation-manifestannotation (the accompanyingunknown/unknownplatform is a shared BuildKit convention, so it is not used as an identifier on its own to avoid dropping unrelated artifacts);Attestation manifests reference their subject platform manifest by digest (
vnd.docker.reference.digest). Since conversion changes that manifest's digest, the attestation cannot be re-associated with the converted image, and carrying the descriptor through unchanged would leave a dangling reference in the output index. Dropping it (the "omit the descriptor with a warning" option noted in the issue) keeps the index self-consistent and stops the crash. Preserving provenance/SBOM across conversion — which requires rewriting and re-pushing the attestation's reference, and the cross-repository blob-copy question raised in the issue — is left as a follow-up.Which issue(s) this PR fixes:
Fixes #378
Please check the following list:
isAttestationManifestcovering the annotation, the annotation together with anunknown/unknownplatform, anunknown/unknownplatform without the annotation (not an attestation), a normal platform manifest, and an empty descriptor.