-
Notifications
You must be signed in to change notification settings - Fork 556
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
Add --platform flag to cosign sbom download #1975
Conversation
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
This commit implements the --platform flag of `cosign sbom download` that allows the user to fetch an sbom for a specific arch. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
Codecov Report
@@ Coverage Diff @@
## main #1975 +/- ##
==========================================
- Coverage 34.71% 28.69% -6.02%
==========================================
Files 153 133 -20
Lines 10037 8092 -1945
==========================================
- Hits 3484 2322 -1162
+ Misses 6166 5463 -703
+ Partials 387 307 -80
Continue to review full report at Codecov.
|
So, I think we can proceed with this, and we'll be better off than we are today, but I think we might want to think about all the crazy ways people may expect to use this. The main problem is that there may be multiple matching platforms, e.g.,:
In this case I think we should probably handle multiple matching platforms with an error, since we don't know which one to download (and we shouldn't download multiple). The problem is that I'd suggest testing this out against |
This conversation is making me want to finally provide a method in ggcr to filter manifests by platform, encoding all this bonkers crazy logic. |
This commit modifies the --platform flag in cosign download to improve the handling of platform strings and matching. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
This commit refactors the platforms logic to its own image and improves the user help when there is no sbom at the index level. cosign will now show the user which platforms are available to use with --platform. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
Add single image with platform case and update args in calls to download functions Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
61d4163
to
f0afd0d
Compare
OK, I've improved the platform parsing by following the way Here is a sample of the new platform handling
|
@@ -944,7 +949,7 @@ func TestAttachSBOM(t *testing.T) { | |||
// Upload it! | |||
must(attach.SBOMCmd(ctx, options.RegistryOptions{}, "./testdata/bom-go-mod.spdx", "spdx", imgName), t) |
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.
Should attach also take a platform? 🤔
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.
OK, I'll add --platform
to cosign attach sbom
in a follow-up
Refacor listing platform to a method of platformList Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
return strings.Join(r, ", ") | ||
} | ||
|
||
func SBOMCmd( |
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.
(not blocking) It looks like the []string
return value is only ever read in test/e2e_test.go, and never in the actual CLI command. Is that right?
Maybe we should just have this return error
alone, and refactor tests to not rely on the []string
return value.
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.
Yes, I agree. I think the command was meant to return more than one sbom but it is short-circuited to spit out the one sbom. We should discuss if it will be returning more than one SBOM and decide how to change this. I think the logic to download the SBOMs (and the rest) should me moved away from the CLI to a more general package.
cmd/cosign/cli/download/sbom.go
Outdated
} | ||
|
||
func findSignedImage(ctx context.Context, sii oci.SignedImageIndex, iDigest v1.Hash) (se oci.SignedEntity, err error) { | ||
if err := walk.SignedEntity(ctx, sii, func(ctx context.Context, e oci.SignedEntity) error { |
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.
sii.SignedImage(iDigest)
should do the same, without making you write the walk
logic yourself. Let me know if that doesn't work for some reason.
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.
Yay thanks for the tip @imjasonh 🎉 I pushed another commit dropping the function and switching to SignedImage()
This commit drops the findSignedImage() function in favor of the SignedImge() function of the OCI package. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@chainguard.dev>
Summary
This PR adds a
--platform
flag tocosign sbom download
. When downloading an SBOM from a multiarch index, the new flag lets the user specify the SBOM of one of the images listed in the index.When trying to download an SBOM from an inbox that does not have one attached, cosign will now present the user with the available architectures:
Ticket Link
Fixes #1086
/cc @jdolitsky @imjasonh @mattmoor
Release Note