Codecov Report

Merging #3000 into master will increase coverage by <.01%.
The diff coverage is 43.75%.

@@            Coverage Diff             @@
##           master    #3000      +/-   ##
==========================================
+ Coverage   43.54%   43.54%   +<.01%     
==========================================
  Files         103      103              
  Lines       11015    11031      +16     
==========================================
+ Hits         4796     4803       +7     
- Misses       5483     5489       +6     
- Partials      736      739       +3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa328df...09cf2a6. Read the comment docs.

@justincormack @dmcgowan

I agree we should add this to the API now to be more consistent with descriptor type used on the client and server, @stevvooe wdyt?

Revoking my lgtm.

Turns out, the intent is not to store this field and they just want to stuff data here to feed diff options. Please don’t add fields that only hit your use case, leaving others to clean up your mess later.

If you want options on a diff call, add them to that specific endpoint.

If you want to add this field correctly, please update the pr.

@stevvooe Added a patch: 0b711d6

@stefanberger do we have a few tests to make sure this is being propagated to the image store?

@stevvooe Should be possible to extend some existing tests with annotations ... let me see.

@stevvooe Would content/local/store_test.go the right file to add a test case to ?

@stefanberger I don't think we use descriptors on the content store. content.Info is a different datastructure from descriptor. A descriptor identifies and annotates the content in an OCI image, where as the content.Info is just basic sizing and labeling. I think it would be best to start in https://github.com/containerd/containerd/blob/master/metadata/images_test.go. From there, I would also checkout any where github.com/containerd/containerd/api/types/descriptor.proto is used (github search or a grep would be fine: https://github.com/containerd/containerd/search?q=github.com%2Fcontainerd%2Fcontainerd%2Fapi%2Ftypes%2Fdescriptor.proto&unscoped_q=github.com%2Fcontainerd%2Fcontainerd%2Fapi%2Ftypes%2Fdescriptor.proto).

@stevvooe What is a valid test case? Do any existing images have Annotations that we can read back? Or just create a ocispec.Descriptor{} with an Annotations field and write the JSON into the store and read it back after?

@stefanberger I would recommend reading through the test cases and identifying areas where we would expect it to round trip, making sure that the value is correct. Yes, just put data on the Annotations field and make sure its not corrupted when landing in the database. Also, make sure that the field path is working correctly, so we can list images by annotation.

@stevvooe What is 'field path' and which command is relevant for listing images by annotation?

@stevvooe Please have a look at the patches I just added. Also, I am not sure whether fieldpaths are now sufficiently support with just these changes?

LGTM

I think this mostly hits everything required to add this field but need a second set of eyes to ensure its consistent with other image fields.

As for the approach for adding encryption parameters, after some thought, it would be best to add key id and other public parameters on this descriptor, that will become part of the image store. For the private key, however, that should come through as an option which is dumped after use. We don't want to risk storing an encryption key in the containerd database or logs.

LGTM

Just ran into this as well. Any chance this (and #3121) could be considered for backport to 1.2.x? I'd be happy to put together the PR if so.