Support MediaType on OCI Layers #357

williammartin · 2017-10-11T15:47:58Z

According to the image spec [1], descriptors should contain
media types. Support them on the returned BlobInfo for layers.

[1] https://github.com/opencontainers/image-spec/blob/master/descriptor.md

runcom · 2017-10-11T15:50:30Z

LGTM

thanks @williammartin

williammartin · 2017-10-11T16:22:08Z

@runcom @mtrmac It looks like maybe with Github's flakiness that nothing has poked Travis to start building, can you kick this off?

mtrmac · 2017-10-11T17:54:10Z

ACK in principle, but looking at the references in types.go, quite a few assume that BlobInfo only has Size and Digest; could you edit them to clarify whether/how they are expected to work with the MediaType value?

The code base so far tends to use MIME instead of Media, unlike Moby, to be more explicit about the authority/format, unless an external format mandates e.g. a JSON field name. See e.g. ManifestMIMEType instances in types.go. (Honestly not sure whether this matters all that much.)

As for kicking the tests, unsure how to do that.

williammartin · 2017-10-11T17:59:15Z

Sure, probably if I change something the tests will get kicked off anyway. I'll try to figure out something more consistent.

BooleanCat · 2017-10-13T13:38:54Z

@mtrmac We've updated the BlobInfo related comments. Please take a look :)

Also, seems like Travis still hasn't triggered even with a new commit.

BooleanCat · 2017-10-13T15:10:37Z

It seems that Travis CI has marked this PR as "Abuse detected" and refuses to run tests. I've contacted Travis support.

BooleanCat · 2017-10-13T15:37:11Z

@runcom @mtrmac
Could you please trigger the Travis build?

Response from Travis:

Hi Thomas,
Thanks for writing in.

We have found a bug in our abuse detection system which has been flagging users wrongly.

We're working on fixing it.

In the meantime, I have marked off the user account containers as trusted, so that the abuse detection checks are not run for it at all.

Could you trigger another build and let us know if it works?

Happy building!

williammartin · 2017-10-13T15:53:08Z

@BooleanCat I think you should just amend the commit and force push, which will retrigger the build.

BooleanCat · 2017-10-16T08:14:03Z

@mtrmac Tests are green, they failed once because of a cache loading error. so I re-triggered.

tscolari · 2017-10-17T15:59:37Z

@mtrmac could you review this after their last change?
This is blocking the last story to enable OCI buildpacks on Cloudfoundry! 😃

mtrmac

Sorry for the delay.

Two of the added comments document something not actually implemented; it’s perfectly fine to adjust the comments, let’s not block the working part on implementing things which nobody found urgently needed (yet).

[We do eventually want the MIME type updated e.g. after compressing an OCI layer during push. But again, by no means blocking this PR.]

mtrmac · 2017-10-21T20:31:03Z

types/types.go

@@ -156,6 +157,7 @@ type ImageDestination interface {
 	// PutBlob writes contents of stream and returns data representing the result (with all data filled in).


The “all data filled in” is not currently true for return values of PutBlob; most (all?) implementations currently fill in only digest and size.

Same for ReapplyBlob.

(It would be nice if it were true eventually.)

mtrmac · 2017-10-21T20:32:33Z

types/types.go

@@ -260,7 +262,7 @@ type ManifestUpdateOptions struct {
 // only to make writing struct literals possible.
 type ManifestUpdateInformation struct {
 	Destination  ImageDestination // and yes, UpdatedManifest may write to Destination (see the schema2 → schema1 conversion logic in image/docker_schema2.go)
-	LayerInfos   []BlobInfo       // Complete BlobInfos (size+digest) which have been uploaded, in order (the root layer first, and then successive layered layers)
+	LayerInfos   []BlobInfo       // Complete BlobInfos (size+digest+urls+mediatype+annotations) which have been uploaded, in order (the root layer first, and then successive layered layers)


This is not currently true; copy.Image sets InformationOnly.LayerInfos based on one of

Image.LayerInfos, which is documented above as ”MediaType may be [only] optionally provided”, and says nothing about URLs/Annotations (in particular schema[12] LayerInfos does not set MediaType)

The return value of ReapplyBlob/PutBlob (which is supposed to have “all data filled in” but actually all implementations just set digest + size)

AFAICS LayerInfos is currently used only in the schema1→schema2 conversion, which really only the size, so for now just documenting these two fields might be fine.

mtrmac · 2017-10-21T20:36:42Z

types/types.go

@@ -249,7 +251,7 @@ type Image interface {

 // ManifestUpdateOptions is a way to pass named optional arguments to Image.UpdatedManifest
 type ManifestUpdateOptions struct {
-	LayerInfos              []BlobInfo // Complete BlobInfos (size+digest+urls) which should replace the originals, in order (the root layer first, and then successive layered layers)
+	LayerInfos              []BlobInfo // Complete BlobInfos (size+digest+urls+mediatype+annotations) which should replace the originals, in order (the root layer first, and then successive layered layers)


This is not currently implemented that way.

Same as ManifestUpdateInformation.LayerInfos below, the URLs+MIME type + annotations are not currently available when called by copy.Image.

Also, the UpdatedImage implementations currently do not use the MIME type from this array (but they do use URLs+annotations). Changing the two lines would be trivial, but when copy.Image does not set the values, that would break things.

So, I guess, for now explicitly document that the MIME type is ignored?

According to the image spec [1], descriptors should contain media types. Support them on the returned BlobInfo for layers. [1] https://github.com/opencontainers/image-spec/blob/master/descriptor.md Signed-off-by: Claudia Beresford <cberesford@pivotal.io> Signed-off-by: William Martin <wmartin@pivotal.io> Signed-off-by: Topher Bullock <cbullock@pivotal.io>

BooleanCat · 2017-10-23T13:30:35Z

@mtrmac We've addressed the comments above, let us know what you think.

Unblocking the garden team while waiting for the PR to be merged. containers/image#357 [#152029459]

mtrmac · 2017-10-25T19:17:01Z

👍 Thanks!

BooleanCat force-pushed the blobinfo-media-type branch from 096eaed to ea0d5ca Compare October 13, 2017 13:37

BooleanCat force-pushed the blobinfo-media-type branch 2 times, most recently from faa22e6 to e6e2c40 Compare October 16, 2017 07:53

danail-branekov mentioned this pull request Oct 17, 2017

Support for uncompressed local OCI layers cloudfoundry/grootfs#7

Merged

mtrmac reviewed Oct 21, 2017

View reviewed changes

BooleanCat force-pushed the blobinfo-media-type branch from e6e2c40 to 282a651 Compare October 23, 2017 12:46

BooleanCat force-pushed the blobinfo-media-type branch from 282a651 to e13bf55 Compare October 23, 2017 13:29

tscolari added a commit to cloudfoundry/grootfs that referenced this pull request Oct 23, 2017

Change containers/image dep to forked repo

4170d08

Unblocking the garden team while waiting for the PR to be merged. containers/image#357 [#152029459]

mtrmac merged commit acdbc97 into containers:master Oct 25, 2017

mtrmac mentioned this pull request Feb 5, 2021

copy: compute blob compression on reused blobs based on source MediaType #1138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MediaType on OCI Layers #357

Support MediaType on OCI Layers #357

williammartin commented Oct 11, 2017

runcom commented Oct 11, 2017 •

edited

williammartin commented Oct 11, 2017

mtrmac commented Oct 11, 2017

williammartin commented Oct 11, 2017

BooleanCat commented Oct 13, 2017 •

edited

BooleanCat commented Oct 13, 2017

BooleanCat commented Oct 13, 2017

williammartin commented Oct 13, 2017

BooleanCat commented Oct 16, 2017

tscolari commented Oct 17, 2017

mtrmac left a comment

mtrmac Oct 21, 2017

mtrmac Oct 21, 2017

mtrmac Oct 21, 2017

BooleanCat commented Oct 23, 2017

mtrmac commented Oct 25, 2017 •

edited by runcom

		@@ -156,6 +157,7 @@ type ImageDestination interface {
		// PutBlob writes contents of stream and returns data representing the result (with all data filled in).

Support MediaType on OCI Layers #357

Support MediaType on OCI Layers #357

Conversation

williammartin commented Oct 11, 2017

runcom commented Oct 11, 2017 • edited

williammartin commented Oct 11, 2017

mtrmac commented Oct 11, 2017

williammartin commented Oct 11, 2017

BooleanCat commented Oct 13, 2017 • edited

BooleanCat commented Oct 13, 2017

BooleanCat commented Oct 13, 2017

williammartin commented Oct 13, 2017

BooleanCat commented Oct 16, 2017

tscolari commented Oct 17, 2017

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Oct 21, 2017

Choose a reason for hiding this comment

mtrmac Oct 21, 2017

Choose a reason for hiding this comment

mtrmac Oct 21, 2017

Choose a reason for hiding this comment

BooleanCat commented Oct 23, 2017

mtrmac commented Oct 25, 2017 • edited by runcom

runcom commented Oct 11, 2017 •

edited

BooleanCat commented Oct 13, 2017 •

edited

mtrmac commented Oct 25, 2017 •

edited by runcom