-
Notifications
You must be signed in to change notification settings - Fork 617
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.
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
Conversation
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) |
There was a problem hiding this comment.
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.
b97e2a4 to
938b333
Compare
|
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). |
938b333 to
da43f1a
Compare
tonistiigi
left a comment
There was a problem hiding this 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.
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>
da43f1a to
90c849f
Compare
|
🤦 done another time, this time doing so at the bake level, so should preserve with |
Fixes the following issue described by @crazy-max:
Essentially, we could not specify
--sbom=<bool>as a flag iftype=sbomwas 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
disabledis set (which is what is configured when the--sbomflag 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
disabledis 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.