Skip to content

Add env var to disable default attestations#1612

Merged
jedevc merged 1 commit into
docker:masterfrom
cpuguy83:env_no_provenance
Feb 22, 2023
Merged

Add env var to disable default attestations#1612
jedevc merged 1 commit into
docker:masterfrom
cpuguy83:env_no_provenance

Conversation

@cpuguy83
Copy link
Copy Markdown
Contributor

@cpuguy83 cpuguy83 commented Feb 8, 2023

For certain cases we need to build with --provenance=false. However not all build envs (especially in the OSS ethos) have the latest buildx so just blanket setting --provenance=false will fail in these cases.

Having an env var allows people to set the value without having to worry about if the buildx version has the --provenance flag.

Comment thread build/build.go Outdated
if _, ok := opt.Attests["attest:provenance"]; !ok && supportsAttestations {
so.FrontendAttrs["attest:provenance"] = "mode=min,inline-only=true"
var noProv bool
if v, ok := os.LookupEnv("BUILDX_NO_DEFAULT_PROVENANCE"); ok {
Copy link
Copy Markdown
Member

@crazy-max crazy-max Feb 9, 2023

Choose a reason for hiding this comment

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

I recall we have

if v, ok := opt.BuildArgs["BUILDKIT_INLINE_CACHE"]; ok {
to inline cache metadata to image config. I wonder if the var should be named BUILDKIT_NO_DEFAULT_PROVENANCE instead for consistency even if BUILDKIT_INLINE_CACHE should have been BUILDX_INLINE_CACHE imo as this var is not handled in BuildKit but Buildx.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, we also have BUILDX_NO_DEFAULT_LOAD though 🤔

Consistency isn't great here, though I'd be tempted to go with the BUILDX_ prefix if possible.

I wonder if we should make the variable name more generic though, something like BUILDX_NO_DEFAULT_ATTESTATIONS - just in case we ever went the route of making some other attestations default as well (currently not planned, but better to have the breathing room IMHO).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@cpuguy83 wdyt?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BUILDX_NO_DEFAULT_ATTESTATIONS lgtm

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

For certain cases we need to build with `--provenance=false`.
However not all build envs (especially in the OSS ethos) have the latest
buildx so just blanket setting `--provenance=false` will fail in these
cases.

Having an env var allows people to set the value without having to worry
about if the buildx version has the `--provenance` flag.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Copy link
Copy Markdown
Collaborator

@jedevc jedevc left a comment

Choose a reason for hiding this comment

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

LGTM, @tonistiigi?

Probably worth cherry-picking to 0.10 as well?

@jedevc jedevc merged commit 1c6060f into docker:master Feb 22, 2023
@cpuguy83 cpuguy83 deleted the env_no_provenance branch February 22, 2023 20:21
@jedevc
Copy link
Copy Markdown
Collaborator

jedevc commented Mar 6, 2023

Cherry-picked to 0.10 in #1645.

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.

5 participants