Skip to content

Fix attestation overrides with duplicate types #1699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 25, 2023
Merged

Fix attestation overrides with duplicate types #1699

merged 1 commit into from
May 25, 2023

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented Mar 28, 2023 •
edited by crazy-max
Loading

Fixes the following issue described by @crazy-max:

For this command:

/usr/bin/docker buildx bake --set *.output=type=image,name=localhost:5000/name/app:latest,push=true --set *.cache-from=type=gha,scope=attests-image --set *.cache-to=type=gha,scope=attests-image,mode=max --metadata-file /tmp/docker-actions-toolkit-N7ECkf/metadata-file --provenance mode=max,builder-id=https://github.com/docker/bake-action/actions/runs/4540267123 --sbom true image --print

I have the --sbom true flag and also type=sbom in my bake definition so error looks good. But I wonder if --provenance or --sbom for bake cmd should just overwrite the one defined in the definition instead of checking for dup?
I say that because, e.g. --sbom is shorthand for --set=*.attest=type=sbom, which is an override and imo should just override 😄 We have the same for --push which override the output field atm. I think that makes sense.

Essentially, we could not specify --sbom=<bool> as a flag if type=sbom was set on an individual bake target, even though the expected behavior is that the command line should override this. This behavior was introduced in #1475.

This PR adds a collection of test cases to ensure correct behavior when the magic internal value disabled is set (which is what is configured when the --sbom flag is used), as well as fixing the underlying cause.

The fix is fairly simple - we can allow merging duplicate types for the attestation key iff disabled is set on that attestation field. This isn't quite the same as reverting #1475, since the values are correctly merged if the user provides duplicate flags.

@jedevc jedevc requested review from tonistiigi and crazy-max March 28, 2023 10:52
@jedevc jedevc mentioned this pull request Mar 28, 2023
Closed
tonistiigi
tonistiigi reviewed Mar 31, 2023
View reviewed changes
bake/bake_test.go Outdated
// with disabled=true override
m, _, err = ReadTargets(ctx, []File{fp}, []string{"default"}, []string{"*.attest=type=sbom,disabled=true"}, nil)
require.NoError(t, err)
require.Equal(t, []string{"type=sbom,generator=custom", "type=sbom,disabled=true"}, m["default"].Attest)
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand how these make sense. If the user sets that sbom should be disabled then why don't we just set it like that.

I think --set should either replace the attestation with the same type or append the non-type keys. Adding a second record doesn't make sense to me. If that sounds too magical then we can replace the whole block.

@crazy-max crazy-max mentioned this pull request Apr 24, 2023
Merged
@jedevc jedevc force-pushed the bake-attestation-override branch from b97e2a4 to 938b333 Compare May 9, 2023 13:24
@jedevc
Copy link
Collaborator Author

jedevc commented May 9, 2023

Have tidied this up now (apologies for the delay).

A better fix is just to "replace the attestation with the same type", with the later attestations overriding the former ones. We still need some additional logic for the build command to ensure that the pattern from #1475 doesn't cause confusion (some more details in the commit message).

@jedevc jedevc force-pushed the bake-attestation-override branch from 938b333 to da43f1a Compare May 9, 2023 13:25
@jedevc jedevc added this to the v0.11.0 milestone May 11, 2023
@tonistiigi tonistiigi self-requested a review May 11, 2023 15:53
tonistiigi
tonistiigi reviewed May 16, 2023
View reviewed changes
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Why are we doing this in the buildopt level? This means that --print will show one set of values, but what actually gets built is different.

@jedevc jedevc added kind/bug Something isn't working area/bake labels May 18, 2023
@jedevc
bake: merge attest entries
90c849f 
This ensures that `target.attest=["type=sbom,<value>"]` can be
appropriately merged when `--sbom=true` or `--set
target.attest=type=sbom`.

To merge, we simply naively take the last valid value.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the bake-attestation-override branch from da43f1a to 90c849f Compare May 19, 2023 13:34
@jedevc
Copy link
Collaborator Author

jedevc commented May 19, 2023

🤦 done another time, this time doing so at the bake level, so should preserve with --print.

@jedevc jedevc requested review from tonistiigi and crazy-max May 19, 2023 13:36
@jedevc jedevc merged commit 69a9c66 into docker:master May 25, 2023
@jedevc jedevc deleted the bake-attestation-override branch June 7, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@crazy-max crazy-max Awaiting requested review from crazy-max

Assignees
No one assigned
Labels
area/bake kind/bug Something isn't working
Projects
None yet
Milestone
v0.11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jedevc @tonistiigi @crazy-max