Skip to content

fix(convertor): drop BuildKit attestation manifests instead of panicking#379

Open
mvanhorn wants to merge 1 commit into
containerd:mainfrom
mvanhorn:fix/378-skip-attestation-manifests
Open

fix(convertor): drop BuildKit attestation manifests instead of panicking#379
mvanhorn wants to merge 1 commit into
containerd:mainfrom
mvanhorn:fix/378-skip-attestation-manifests

Conversation

@mvanhorn

Copy link
Copy Markdown

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-container driver since v0.10, so ordinary multi-platform images now carry an attestation manifest in their index. Converting such an image crashes the tool with panic: 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 0 in NewOverlayBDBuilderEngine() / NewTurboOCIBuilderEngine()) when an OCI index contains a BuildKit attestation manifest — for example an image built with docker buildx --provenance=true, --sbom=true, or --attest. Provenance is enabled by default with the Buildx docker-container driver 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 to buildOne(), 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 an unknown/unknown platform and the annotation vnd.docker.reference.type: attestation-manifest.

This change:

  • adds isAttestationManifest(), which detects an attestation entry by BuildKit's explicit vnd.docker.reference.type: attestation-manifest annotation (the accompanying unknown/unknown platform is a shared BuildKit convention, so it is not used as an identifier on its own to avoid dropping unrelated artifacts);
  • in the index-traversal loop, skips attestation manifests (with a warning) instead of converting them, and drops them from the rebuilt index.

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:

  • Does the affected code have corresponding tests, e.g. unit test, E2E test? — added a unit test for isAttestationManifest covering the annotation, the annotation together with an unknown/unknown platform, an unknown/unknown platform without the annotation (not an attestation), a normal platform manifest, and an empty descriptor.
  • Does this change require a documentation update? — no.
  • Does this introduce breaking changes that would require an announcement or bumping the major version? — no; it only changes behavior for inputs that previously panicked.
  • Do all new files have an appropriate license header? — the new test file carries the standard Apache-2.0 header.

…f panicking

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@liulanzheng

Copy link
Copy Markdown
Member

@WaberZhuang

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 via vnd.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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

convertor: panic on BuildKit attestation manifests

3 participants