Fixup attestation platforms for the local exporter #3321
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
force-pushed
the
fixup-attestation-platforms
branch
from
c1d426b
to
57f7716
Compare
jedevc
force-pushed
the
fixup-attestation-platforms
branch
2 times, most recently
from
5616047
to
583e480
Compare
crazy-max
reviewed
Dec 6, 2022
jedevc
force-pushed
the
fixup-attestation-platforms
branch
from
583e480
to
4dc3a5c
Compare
tonistiigi
reviewed
Dec 8, 2022
Signed-off-by: Justin Chadwell <me@jedevc.com>
Originally, for a Build, we avoided sending any of the FrontendAttrs to the main Solve call, and sent them all to the sub-Solve calls. However, when we added attestations, we had to add an additional Filter, to ensure those args were sent to both locations. While unintuitive, there wasn't logical location to put them, which would have applied to the entire build. This pattern has become fairly common, for instances where the buildkit backend can provide a fallback when the frontend does not support a specified option, for example, with SOURCE_DATE_EPOCH. This means that this filtering must be updated for each instance - instead of this, we can remove the filtering entirely, which should provide for easier upgrade paths going forwards. Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
This is required now that the correspondence between platforms and refs has been broken. Without it, there's no way to detect between the following cases from the output that they emit: buildctl build --opt platform=linux/amd64 --opt attest:sbom buildctl build --opt platform=linux/amd64 --opt attest:sbom --opt build-arg:BUILDKIT_MULTI_PLATFORM=true We'd like the former to produce a flat file structure through the local exporter, and the latter to produce an explicitly nested file structure through the same exporter. However, both of these produce Refs instead of a singular Ref, so we can't just look at the Result to know which one. Similar to how we handle the SOURCE_DATE_EPOCH build-arg, we can handle the multi-platform args at the control API boundary, setting the explicit option multi-platform in the exporter if it is set. In the future, we can guide users away from the BUILDKIT_MULTI_PLATFORM args entirely, and towards the multi-platform exporter option if we want. Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
fixup-attestation-platforms
branch
from
4dc3a5c
to
6f21d6b
Compare
tonistiigi
approved these changes
Dec 8, 2022
This was referenced Dec 9, 2022
3 tasks
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.
⬆️ Follow-up to #3197.
This PR corrects the behaviour of the local exporter with the
multi-platformargument. Essentially, the problem boils down to the fact that we can't distinguish at the exporter level between the results produced by the following:We'd like the former to produce a flat file structure through the local exporter, and the latter to produce an explicitly nested file structure through the same exporter. However, both of these produce Refs instead of a singular Ref, so we can't just look at the Result to know which one.
To solve this, we can handle this case like
SOURCE_DATE_EPOCH, and detect in the control API boundary if the multi-platform option is set and directly forward it to the exporter, which we can use to make the right call about what format of output to produce. As part of this, we fixup the client to send all frontend attributes to the main solve request for Solves inside Builds - since we would now be having to filter through all attestation arguments,SOURCE_DATE_EPOCHandBUILDKIT_MULTI_PLATFORM/multi-platform- which is frustrating and limits upgrade paths. This behavior was previously broken on master, the fallback forSOURCE_DATE_EPOCHin the control API boundary would never be activated by buildctl or buildx, since they use Solves inside Builds exclusively.We should probably pick this into the release, since without it,
BUILDKIT_MULTI_PLATFORMwon't work as intended for the local exporter. Additionally, the fallback forSOURCE_DATE_EPOCHwon't work correctly.