Skip to content
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 annotations for upload blob. #2188

Merged
merged 5 commits into from Oct 5, 2022

Conversation

cldmnky
Copy link
Contributor

@cldmnky cldmnky commented Aug 21, 2022

This PR adds support for adding annotations to images and index manifests when uploading blobs (#2149)

Summary

We use cosign upload blob to upload and sign releases of a cli tool. We. would like to add a bit more metadata to our artifacts when we upload them, such as release name and other things.

Our custom tool is then able to query or registry and display the annotations on the artifacts for our users.

Added tests for the UploadFiles() function since the permutations increased. The tests should cover the change.

Release Note

Added an --annotations flag to the cosign upload blob command that sets (unsigned) annotations on the OCI images written by the command.

Documentation

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2022

Codecov Report

Merging #2188 (5c8b2dd) into main (8f29f03) will increase coverage by 1.19%.
The diff coverage is 36.00%.

@@            Coverage Diff             @@
##             main    #2188      +/-   ##
==========================================
+ Coverage   26.03%   27.22%   +1.19%     
==========================================
  Files         131      131              
  Lines        7689     7709      +20     
==========================================
+ Hits         2002     2099      +97     
+ Misses       5445     5341     -104     
- Partials      242      269      +27     
Impacted Files Coverage Δ
cmd/cosign/cli/options/upload.go 0.00% <0.00%> (ø)
cmd/cosign/cli/policy_init.go 1.38% <0.00%> (ø)
cmd/cosign/cli/upload.go 0.00% <0.00%> (ø)
pkg/cosign/remote/index.go 65.34% <69.23%> (+43.12%) ⬆️
cmd/cosign/cli/commands.go 0.00% <0.00%> (ø)
pkg/blob/load.go 74.28% <0.00%> (+5.71%) ⬆️
pkg/oci/layout/index.go 28.57% <0.00%> (+28.57%) ⬆️
pkg/oci/layout/write.go 37.50% <0.00%> (+37.50%) ⬆️
pkg/oci/layout/signatures.go 53.84% <0.00%> (+53.84%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cpanato cpanato requested review from mattmoor, n3wscott and dlorenc and removed request for n3wscott August 22, 2022 12:20
Magnus Bengtsson added 2 commits August 22, 2022 20:49
Add tests for UploadFile()

Signed-off-by: Magnus Bengtsson <magnus.bengtsson@betssongroup.com>
Signed-off-by: Magnus Bengtsson <magnus.bengtsson@betssongroup.com>
Magnus Bengtsson added 2 commits August 23, 2022 11:40
Signed-off-by: Magnus Bengtsson <magnus.bengtsson@betssongroup.com>
Signed-off-by: Magnus Bengtsson <magnus.bengtsson@betssongroup.com>
Signed-off-by: Magnus Bengtsson <magnus.bengtsson@betssongroup.com>
@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@cldmnky
Copy link
Contributor Author

cldmnky commented Sep 30, 2022

@priyawadhwa @dlorenc @mattmoor how do you want to proceed with this?

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

sorry for the delayed review! left a couple comments :)

@@ -46,6 +46,9 @@ func NewFile(payload []byte, opts ...Option) (oci.File, error) {
return nil, err
}

// Add annotations from options
img = mutate.Annotations(img, o.Annotations).(v1.Image)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling mutate again, I think you can add the annotations in the addendum on line 42?

	img, err := mutate.Append(base, mutate.Addendum{
		Layer: layer,
                Annotations: annotations,
	})

Copy link
Contributor Author

@cldmnky cldmnky Oct 4, 2022

Choose a reason for hiding this comment

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

Hmmm when using addendum the annotations are not correctly added to the image as it seems. I do believe that the image needs to be annotated (as for CreatedAt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the delayed review! left a couple comments :)

No worries!

@@ -141,6 +145,9 @@ func UploadFiles(ref name.Reference, files []File, getMt MediaTypeGetter, remote
}

if len(files) > 1 {
if annotations != nil {
idx = mutate.Annotations(idx, annotations).(v1.ImageIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're already including annotations when the file is initially created in line 112, do we need to add them in again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is only run if we have an image index, the annotations are (also) added to the image index, the annotations on line 112 are added to the the image it self. So when accessing a multiplatform image index the annotations needs to be on the index, for a plain image it's enough for them to be on the image.

The tests should handle this :)

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@priyawadhwa priyawadhwa merged commit dc40467 into sigstore:main Oct 5, 2022
@github-actions github-actions bot added this to the v1.13.0 milestone Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants