Embed Ref types directly into attestations #3389
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jedevc
requested review from
tonistiigi and
crazy-max
and removed request for
tonistiigi
|
The behaviour for The new behaviour here is only to attach the inline attestations if we have an image index - otherwise, we won't attempt to try and upgrade to an index (however, still undecided on whether we'd maybe want to have some cases where we would force the upgrade). So a single-platform image export with no extra attestation parameters wouldn't include the provenance attestation - but if we had |
jedevc
force-pushed
the
demo-undo-map-merge
branch
from
49ce4f6
to
e3fb802
Compare
This reverts commit f2c770e. This patch reworks the attestations code back out of the Refs map. The logic for this change is that the loss-of-granularity in the data structure for the protobuf requires convoluted logic in the exporter to be able to determine the correct multi-platform behaviour. Prior to attestations, Refs directly mapped each platform to a reference. Attestations changed this structure, which means that there is no longer a concrete way to determine if a reference should be exported with multi-platform semantics or not (e.g. should a local export with a single platform be flat/nested, should an image export use an oci manifest or an oci index). This patch restores the previous semantics, moving the attestion refs back, pushing the attestations back down into a separate struct. This has the disadvantage of requiring the result.Attestation struct to be generic, however, this does simplify some of the helper methods around it. By changing this logic, we're able to simplify a few key pieces of logic: - Fewer version interop concerns. Because we're not changing the structure of Ref/Refs between buildkit versions, mixing frontends and buildkit versions will *always* produce the expected result. If a frontend attaches attestations they won't be transmitted at all automatically. Additionally, because attestations can also be attached to a single Ref, the frontend doesn't need to force an index in the future. - Less complex exporter logic, we don't need to any magic around detecting the user intention with regards to multi-platform exports, we simply follow the previous semantics of len(res.Refs) > 0. We only need the small snippet that ensures that the inline-only attestations don't get attached to a single manifest, to keep the docker load case working. - Some simplification of the SBOM scanner code, we can return an Attestation[llb.State], which we can then easily Convert into an actual Attestation[Ref]. The patch looks large, but most of the changes are mostly just function signature changes - the meat of the changes is in the rework of the platform detection code. This is so we can support the use case of Ref + Attestations, Refs + Attestations (multi-platform=false) and Refs + Attestations (multi-platform=true). Signed-off-by: Justin Chadwell <me@jedevc.com>
We no longer need to (or should) force refs conversion. A single Ref is distinctly different from a Refs map with a single element - they express the user-specified difference between whether multi-platform exports should be used or not. It's important to preserve this sematic, especially on the side of the frontend, where it can't be recovered if lost. Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
demo-undo-map-merge
branch
from
e3fb802
to
db519c2
Compare
This reverts commit 6f21d6b. We don't need the exporter opts for multiplatform, they were a hack to attempt to preserve the semantics of converting between Ref->Refs freely (both buildkit-side and frontend-side). Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
The previous check only checks for untyped nil, however, it's possible for the provenance capture to be a typed nil as well, so we need to check for this scenario as well. Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
demo-undo-map-merge
branch
from
dfd9bd1
to
4e2f656
Compare
|
Discussion from sync:
|
tonistiigi
approved these changes
Dec 13, 2022
tonistiigi
reviewed
Dec 13, 2022
There was a problem hiding this comment.
We should have still used new numbers in proto. Error that happens in incompatibility(even though there is no release) atm is quite awful.
panic: Internal: grpc: error unmarshalling request: proto: InTotoSubject: wiretype end group for non-group
goroutine 1 [running]:
main.main()
This was referenced Dec 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
↪️ Reverts #3038 #3321 (and #3379 if merged)
⚠️ Breaks API compatibility from v0.11-rc1.
This patch reworks the attestations code back out of the Refs map. The logic for this change is that the loss-of-granularity in the data structure for the protobuf requires convoluted logic in the exporter to be able to determine the correct multi-platform behaviour.
Prior to attestations, Refs directly mapped each platform to a reference. Attestations changed this structure, which means that there is no longer a concrete way to determine if a reference should be exported with multi-platform semantics or not (e.g. should a local export with a single platform be flat/nested, should an image export use an oci manifest or an oci index).
This patch restores the previous semantics, moving the attestation refs back, pushing the attestations back down into a separate struct. This has the disadvantage of requiring the result.Attestation struct to be generic, however, this does simplify some of the helper methods around it.