build: add OCI artifact default opt-out - #3991
Conversation
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
a94cd36 to
6ee129a
Compare
|
|
||
| const ( | ||
| noDefaultAttestationsEnv = "BUILDX_NO_DEFAULT_ATTESTATIONS" | ||
| noDefaultOCIArtifactEnv = "BUILDX_NO_DEFAULT_OCI_ARTIFACT" |
There was a problem hiding this comment.
Thinking out loud; what are the formats we (currently) support for including attestations? Would it make sense to have a "attestations-format" selection instead of a boolean? (e.g. BUILDX_ATTESTATIONS_FORMAT=(oci|foo|bar|compat|legacy)) or was the old format not complying to any standards? (ISTR there were options for legacy registries, so those would still be valid and potentially work around the issue with non-OCI-compliant registries?)
There was a problem hiding this comment.
I'd rather keep this as a narrow opt-out for now (specially since this change aims for a patch release). BuildKit currently exposes this as a boolean exporter option, oci-artifact=true|false, so mirroring that as BUILDX_NO_DEFAULT_OCI_ARTIFACT keeps Buildx from inventing a new format-selection layer on top.
The explicit format selection already exists through --output type=image,oci-artifact=false when users want to choose it directly. This env var only changes the default when the option was not set, similar to BUILDX_NO_DEFAULT_ATTESTATIONS.
A generic BUILDX_ATTESTATIONS_FORMAT=legacy|compat|oci feels a bit too broad to me because Buildx would then need to define what those names mean and keep mapping them to BuildKit exporter behavior. If BuildKit grows more attestation storage formats later, I think that should first be exposed as a BuildKit exporter option, and Buildx can follow that model.
There was a problem hiding this comment.
Yeah, that's ok with me; mostly wanted to avoid having to introduce yet-another env-var if there's more options than enable/disable (not even sure what format the "disable" means and if that's a legacy fallback defined by OCI?)
| if isMobyWorker(sb) { | ||
| t.Skip("attestations are not supported by the docker worker") | ||
| } |
There was a problem hiding this comment.
Hum indeed, should rework this 😅
relates to
This adds
BUILDX_NO_DEFAULT_OCI_ARTIFACT=trueas a compatibility escape hatch for Buildx users who need to keep provenance attestations enabled while disabling the newer OCI artifact attestation storage format.When this environment variable is set, Buildx now sets
oci-artifact=falsewhen attestation support is available and the user did not already setoci-artifactexplicitly. This keeps explicit exporter options authoritative, mirrors the existingBUILDX_NO_DEFAULT_ATTESTATIONScompatibility pattern, and applies to bothbuildx buildandbuildx bake.BuildKit started defaulting attestation manifests to OCI artifacts in moby/buildkit#6914 after moby/buildkit#6171 tracked registry support for that format. The issue reported in moby/buildkit#7007 showed that GitLab can still reject a push when the attestation manifest has a
subjectthat points at a manifest that is new in the same push. The initial BuildKit-side fix in moby/buildkit#7012 changed the manifest push order, but the follow-up discussion in moby/buildkit#7014 pointed out that this is a registry compliance issue because OCI distribution requires registries to initially accept valid manifests whosesubjectdoes not exist yet.This PR does not change the default behavior and does not make Buildx prefer the non-OCI artifact format for compliant registries. It only provides a narrow opt-out for environments that need time to deal with registry compatibility problems, while preserving provenance attestations instead of requiring users to disable them entirely with
BUILDX_NO_DEFAULT_ATTESTATIONS.