Adds support for oci manifests and manifestlists #2076

mikebrow · 2016-11-21T21:36:51Z

Here is a draft of the oci manifest/manifest lists that addresses @stevvooe 's request in pr #2021 for oci manifest to be it's own schema.

Also fixes some test case failure issues.

Signed-off-by: Mike Brown brownwm@us.ibm.com

stevvooe · 2016-11-21T21:48:58Z

registry/handlers/images.go

+	isAnOCIManifest := isSchema2 && (schema2Manifest.MediaType == ocischema.MediaTypeManifest)
+	isAnOCIManifestList := isManifestList && (manifestList.MediaType == manifestlist.MediaTypeOCIManifestList)
+
+	badCombinations := [][]bool{


Classic example of why to avoid bools to ascertain support.

Please refactor this method to handle these different cases in a way that is maintainable in the future.

aight.. yeah was one of the things I didn't refactor from 2021 bit ugly ..

@mikebrow I see. 🐹 🎈

May want to refactor this before we do a big change...

I don't have a better suggestion for this yet.

Can replace the bad combo list with easier to read/update error checks.. like this:

if (isSchema2 && !isAnOCIManifest) && (supportsOCISchema && !supportsSchema2) { fmt.Printf("\n\nmanifest is schema2 but accept header only supports OCISchema \n\n") w.WriteHeader(http.StatusNotFound) return }

this comment will be addressed with update

stevvooe · 2016-11-21T21:55:42Z

manifest/manifestlist/manifestlist.go

+	MediaTypeManifestList = "application/vnd.docker.distribution.manifest.list.v2+json"
+	// MediaTypeOCIManifestList specifies the mediaType for OCI compliant manifest
+	// lists.
+	MediaTypeOCIManifestList = "application/vnd.oci.image.manifest.list.v1+json"


I'm not sure that these packages should do duel-OCI-docker support, as it may make subtle differences hard to manage for future contributors.

I should hope there are never any differences in functionality between OCIManifestList and docker ManifestList... Since it's currently in synch.. there is a bit of good karma here in keeping it the same package :)

OCI has added fields that Docker's format does not have.

codecov-io · 2016-11-21T21:55:43Z

Codecov Report

Merging #2076 into master will decrease coverage by 9.25%.
The diff coverage is 71.48%.

@@            Coverage Diff             @@
##           master    #2076      +/-   ##
==========================================
- Coverage    60.8%   51.55%   -9.26%     
==========================================
  Files         129      132       +3     
  Lines       11901    12122     +221     
==========================================
- Hits         7236     6249     -987     
- Misses       3764     5092    +1328     
+ Partials      901      781     -120

Impacted Files	Coverage Δ
blobs.go	`62.5% <ø> (ø)`	⬆️
manifest/schema2/manifest.go	`87.8% <100%> (+6.22%)`	⬆️
registry/storage/manifeststore.go	`84.05% <100%> (+2.7%)`	⬆️
registry/storage/registry.go	`90.54% <100%> (+0.39%)`	⬆️
registry/handlers/manifests.go	`53.64% <50%> (-1.64%)`	⬇️
registry/storage/ocimanifesthandler.go	`67.79% <67.79%> (ø)`
manifest/ocischema/builder.go	`73.8% <73.8%> (ø)`
manifest/manifestlist/manifestlist.go	`76.71% <80%> (+1.71%)`	⬆️
manifest/ocischema/manifest.go	`80.48% <80.48%> (ø)`
registry/storage/driver/gcs/gcs.go	`0.39% <0%> (-68.67%)`	⬇️
... and 8 more

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 749f6af...20aecf1. Read the comment docs.

stevvooe · 2016-11-21T21:58:03Z

manifest/ocischema/manifest.go

+}
+
+// Manifest defines a schema2 manifest.
+type Manifest struct {


Is there any way we can better leverage opencontainers/image-spec/specs-go?

How about we vendor it, import it, and refactor this code after they have an approved v1.0.0?

Otherwise, yes I can do that.

Side note: I see we are missing OCI manifest annotations support...

As an aside would have to do the split ocimanifest list out as a separate package if we're to bring in opencontainers/image-spec/specs-go.

@mikebrow annotations support, is that lacking on this PR or something needed on the OCi-side?

Lacking in the PR.. the field wasn't in the manifest struct.. was probably added to the oci spec after you guys started this code. Will pick it up when vendoring in the oci spec go code. Not sure what we'll need to do, if anything, to make it first class or if we should just store and retrieve.

stevvooe · 2016-11-21T21:59:44Z

@mikebrow This seems like a reasonable start.

Where does this sit vs #2021?

stevvooe · 2016-11-21T22:02:57Z

Where does this sit vs #2021?

For example, should we close it or just take it as a proposed change?

stevvooe · 2016-11-21T22:03:46Z

registry/storage/driver/storagedriver.go

@@ -110,7 +110,7 @@ type FileWriter interface {
 // number of path components separated by slashes, where each component is
 // restricted to alphanumeric characters or a period, underscore, or
 // hyphen.
-var PathRegexp = regexp.MustCompile(`^(/[A-Za-z0-9._-]+)+$`)
+var PathRegexp = regexp.MustCompile(`^(/[A-Za-z0-9._:-]+)+$`)


What new paths are we storing? This addition of : will break the registry on windows.

From Vince's notes: "Added : to the regexp used to determine valid paths, which is necessary for the prepending of oci:"

This is for prepending "oci:" for all tags applied to oci manifests to keep the oci: tags separate from the docker tags for docker images.

Guess windows has ":" reserved.

will change the prepending to "oci." instead of "oci:"

changing to oci. with the update

No, we simply don't use schemes and namespaces in tags. This is a horrid hack and not really workable.

Wish I new that before I tried to make it work :-)

Looks like this change still needs to be removed.

stevvooe · 2016-11-21T22:05:33Z

manifest/ocischema/builder.go

+
+	// Add config to the blob store
+	m.Config, err = mb.bs.Put(ctx, MediaTypeConfig, mb.configJSON)
+	// Override MediaType, since Put always replaces the specified media


`since Put always replaces the specified media

// type with application/octet-stream in the descriptor it returns.`

This behavior is not fixed. We can change that, if necessary.

Either way, this code should not make assumptions about what config is pointed to. It should really take this as an argument to the constructor.

So I took a look at changing this and that led me to the blobStore.Put code which states the following at line 77 of registry/storage/blobstore.go:

// TODO(stevvooe): Write out mediatype here, as well. return distribution.Descriptor{ Size: int64(len(p)), // NOTE(stevvooe): The central blob store firewalls media types from // other users. The caller should look this up and override the value // for the specific repository. MediaType: "application/octet-stream", Digest: dgst, }, bs.driver.PutContent(ctx, bp, p)

Simple enough to change this to use the passed in mediaType.. but not sure what you meant by it's supposed to firewall the media types.. :-) Just confirming.. do you want me to change this code in blobstore to use the passed in mediaType and go remove all the overwrites being done after puts?

or should this change coincide with some sort of filter/approver being added for puts coming in through the blobServiceListener?

mikebrow · 2016-11-21T22:05:49Z

I recoded 2021 (starting mostly from scratch)... to fix some testing bugs in #2021, and more importantly to move it from the design in 2021 where the docker schema2 was overloaded to also have oci support in it, to a design with a new schema type. You didn't like the design in 2021 so I started it over :-) This can be merged on top of 2021 or we can just start from here.

stevvooe · 2016-11-21T22:10:48Z

@mikebrow This approach looks like a good start.

Mostly, we would like to make this as simple as using distribution.UnmarshalManifest. I am seeing a few interface fixes we need to address to make this work in all cases. I wonder if we refactor the tests from there, if we can get a better gauge on the quality of the package structure.

vbatts · 2016-11-22T15:03:04Z

Thanks for doing this @mikebrow !

stevvooe · 2016-11-24T02:10:09Z

@vbatts Should we close #2021?

vbatts · 2016-11-24T02:38:42Z

Done

…

On Wed, Nov 23, 2016, 21:10 Stephen Day ***@***.***> wrote: @vbatts <https://github.com/vbatts> Should we close #2021 <#2021>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2076 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEF6WkWQo6dcvEmiNM9knqR8USHaZnkks5rBPIEgaJpZM4K4uct> .

vbatts · 2017-04-05T18:28:29Z

What's next here?

dmcgowan · 2017-04-05T18:53:04Z

@vbatts v1.0 spec hopefully :)

Discussed this a little bit yesterday. Given the current timeline we should aim for having this in the 2.8 registry release. We can possibly time such a release around the release of v1.0 image spec as well. The 2.7 release should probably be done sooner and not wait for this change.

Would it be helpful if we milestoned this? Maybe we can consider merging this using the image spec release candidate after we cut a 2.7 release branch.

vbatts · 2017-04-05T19:30:36Z

good to hear. All the pieces of the OCI mediatypes that this deals with are pretty calm.

…

On Wed, Apr 5, 2017 at 2:53 PM Derek McGowan ***@***.***> wrote: @vbatts <https://github.com/vbatts> v1.0 spec hopefully :) Discussed this a little bit yesterday. Given the current timeline we should aim for having this is a 2.8 registry release. We can possibly time such a release around the release of v1.0 image spec as well. The 2.7 release should probably be done sooner and not wait for this change. Would it be helpful if we milestoned this? Maybe we can consider merging this with the image spec candidate after we cut a 2.7 release branch. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2076 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEF6cx8q71aQlYIQ9mQRu_Z4T16IcrMks5rs-MVgaJpZM4K4uct> .

vbatts · 2017-07-08T17:07:37Z

@mikebrow ready to rebase this??? 🤓

stevvooe · 2017-07-10T18:04:21Z

@mikebrow Pretty please? It is time... 💯 🥇

mikebrow · 2017-07-10T18:15:37Z

cool.. I'm on it..

vbatts · 2017-07-10T18:26:00Z

👾 ✨

mikebrow · 2017-07-10T20:51:33Z

PR rebased.. and nit comments addressed.

mikebrow · 2017-07-10T20:57:47Z

The only comment I didn't address (I believe) is vendoring the image spec and refactoring this code to use it. Want to do that on a subsequent PR or this one? Oh yeah and it's missing annotations support.

mikebrow · 2017-07-10T21:17:25Z

I see two commits went into schema2 that we might want put into ocischema as well.. 8867e8f
9ab7b0e

OCI Image manifests and indexes are supported both with and without an embeded MediaType (the field is reserved according to the spec). Test storing and retrieving both types from the manifest store. Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>

GordonTheTurtle · 2018-06-19T16:31:50Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "ocitype" git@github.com:mikebrow/distribution.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354379640
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Handle OCI manifests and image indexes without a media type Signed-off-by: Mike Brown <brownwm@us.ibm.com>

mikebrow · 2018-06-20T17:40:41Z

@dmcgowan @crosbymichael FYI.. this is the PR for OCI support in the registry that we discussed at the dockercon containerd maintainer face to face.

crosbymichael · 2018-06-25T14:47:36Z

Thanks for the ping @mikebrow

dmcgowan · 2018-06-25T20:05:53Z

manifest/ocischema/builder.go

+}
+
+// SetMediaType is for testing purposes, we want to be able to create an OCI image with
+// either an MediaType either empty, or with the OCI image value


This needs to be reworded

reworded...

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

crosbymichael · 2018-07-19T20:11:42Z

I was testing this with containerd and pushes work but on a pull i get an error

registry logs:

time="2018-07-19T20:10:24.236940177Z" level=error msg="response completed with error" err.code=unknown err.detail="unrecognized manifest schema version 0" err.message="unknown error" go.version=go1.10.3 http.request.host="127.0.0.1:5000" http.request.id=3886b765-a36d-48a5-973c-bc5fc60c6bf9 http.request.method=HEAD http.request.remoteaddr="127.0.0.1:40058" http.request.uri="/v2/redis/manifests/latest" http.request.useragent="Go-http-client/1.1" http.response.contenttype="application/json; charset=utf-8" http.response.duration=6.019726ms http.response.status=500 http.response.written=70 vars.name=redis vars.reference=latest

ctr

sudo ctr images pull --plain-http 127.0.0.1:5000/redis:latest
ctr: failed to resolve reference "127.0.0.1:5000/redis:latest": unexpected status code http://127.0.0.1:5000/v2/redis/manifests/latest: 500 Internal Server Error

crosbymichael · 2018-07-19T20:18:09Z

manifest/ocischema/builder.go

+// valid media type for oci image manifests currently: "" or "application/vnd.oci.image.manifest.v1+json"
+func (mb *Builder) SetMediaType(mediaType string) {
+	if mediaType != "" && mediaType != v1.MediaTypeImageManifest {
+		panic("Invalid media type for OCI image manifest")


Are we sure we want a panic here?

Good point. Addressed.

crosbymichael · 2018-07-19T20:19:04Z

manifest/ocischema/manifest.go

+	}
+)
+
+func init() {


is this how distribution handles things or can this be moved out of an init?

it's how distribution registers the manifest media types (schema types), see:
https://github.com/docker/distribution/blob/master/manifest/schema2/manifest.go#L56
for the example that registers docker's schema2 manifest.

In this case we're setting up to handle "application/vnd.oci.image.manifest.v1+json"

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

mikebrow · 2018-07-19T21:30:05Z

I was testing this with containerd and pushes work but on a pull i get an error
registry logs:

..."unrecognized manifest schema version 0" err.message="

@crosbymichael

Initially I was thinking it made sense, given oci schema's near identical alignment with docker schema2 manifests to go with version 2 as the initial oci schema manifest version number.

WDYT make it 0, 1, or stick with 2?

jonjohnsonjr · 2018-07-19T21:33:51Z

@mikebrow

This REQUIRED property specifies the image manifest schema version. For this version of the specification, this MUST be 2 to ensure backward compatibility with older versions of Docker. The value of this field will not change. This field MAY be removed in a future version of the specification.

https://github.com/opencontainers/image-spec/blob/master/manifest.md#image-manifest-property-descriptions

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

crosbymichael · 2018-07-20T16:04:43Z

LGTM

dmcgowan · 2018-07-20T17:21:16Z

LGTM

Thanks everyone for testing this and your patience. Note to use the OCI manifests with Docker you will need to update to a newer version which has support for these media types. Support is being backported to all supported version of Docker released in the last year.

vbatts · 2018-07-20T19:30:59Z

Oh man y'all

…

On Fri, Jul 20, 2018, 13:21 Derek McGowan ***@***.***> wrote: Merged #2076 <#2076>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2076 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEF6cD5qxiv_CFCIKq2uSwLCnYOC8Wiks5uIhGcgaJpZM4K4uct> .

stevvooe reviewed Nov 21, 2016

View reviewed changes

dmcgowan added the status/1-design-review label Nov 23, 2016

vbatts mentioned this pull request Nov 24, 2016

[WIP] Adds support for the OCI image spec #2021

Closed

runcom mentioned this pull request Nov 24, 2016

Fixing panic on layer pull as well as provide mechanism for auth containers/oci-fetch#11

Open

vbatts mentioned this pull request Jan 11, 2017

image-layout: describe an OPTIONAL index opencontainers/image-spec#438

Closed

jonjohnsonjr mentioned this pull request Apr 28, 2017

MediaType validation too aggressive #2259

Closed

owtaylor mentioned this pull request Jun 21, 2017

Preserve annotations in OCI images containers/skopeo#369

Closed

mikebrow force-pushed the ocitype branch from 9bdf196 to e2d959e Compare July 10, 2017 20:50

owtaylor mentioned this pull request Jun 19, 2018

Add Taylor #2614

Closed

GordonTheTurtle added the dco/no label Jun 19, 2018

Merge pull request #1 from owtaylor/oci-media-types

321d636

Handle OCI manifests and image indexes without a media type Signed-off-by: Mike Brown <brownwm@us.ibm.com>

mikebrow force-pushed the ocitype branch from 985bc28 to 321d636 Compare June 19, 2018 16:47

GordonTheTurtle removed the dco/no label Jun 19, 2018

sargun mentioned this pull request Jun 25, 2018

Please tag new release #2623

Closed

dmcgowan reviewed Jun 25, 2018

View reviewed changes

address lint and gofmt issues

e8d7941

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

mikebrow force-pushed the ocitype branch from 80d36d7 to e8d7941 Compare June 25, 2018 20:45

This was referenced Jun 26, 2018

Support creating and pushing OCI types moby/buildkit#467

Merged

Register OCI media types moby/moby#37359

Merged

crosbymichael reviewed Jul 19, 2018

View reviewed changes

address review comment regarding panic use

5f588fb

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

added test for initial oci schema version

20aecf1

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

dmcgowan merged commit 0dae095 into distribution:master Jul 20, 2018

wking mentioned this pull request Aug 2, 2018

Proposal: New Mediatype - Container Image Encryption opencontainers/image-spec#747

Open

caervs mentioned this pull request Feb 11, 2019

Add annotations to ManifestList #2828

Closed

mtrmac mentioned this pull request Apr 25, 2019

Integration test bucket needs: containers/skopeo#184

Closed

thaJeztah mentioned this pull request Oct 11, 2019

Moby broken by data valdiation in unmarshaller #3024

Closed

Adds support for oci manifests and manifestlists #2076

Adds support for oci manifests and manifestlists #2076

Conversation

mikebrow commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 21, 2016 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

stevvooe commented Nov 21, 2016

stevvooe commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

mikebrow Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow commented Nov 21, 2016

stevvooe commented Nov 21, 2016

vbatts commented Nov 22, 2016

stevvooe commented Nov 24, 2016

vbatts commented Nov 24, 2016 via email

vbatts commented Apr 5, 2017

dmcgowan commented Apr 5, 2017 • edited Loading

vbatts commented Apr 5, 2017 via email

vbatts commented Jul 8, 2017

stevvooe commented Jul 10, 2017

mikebrow commented Jul 10, 2017

vbatts commented Jul 10, 2017

mikebrow commented Jul 10, 2017 • edited Loading

mikebrow commented Jul 10, 2017 • edited Loading

mikebrow commented Jul 10, 2017

GordonTheTurtle commented Jun 19, 2018

mikebrow commented Jun 20, 2018

crosbymichael commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosbymichael commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

mikebrow commented Jul 19, 2018

jonjohnsonjr commented Jul 19, 2018

crosbymichael commented Jul 20, 2018

dmcgowan commented Jul 20, 2018

vbatts commented Jul 20, 2018 via email

codecov-io commented Nov 21, 2016 •

edited by codecov bot

Loading

mikebrow Dec 8, 2016 •

edited

Loading

mikebrow Nov 30, 2016 •

edited

Loading

stevvooe Nov 21, 2016 •

edited

Loading

mikebrow Dec 8, 2016 •

edited

Loading

dmcgowan commented Apr 5, 2017 •

edited

Loading

mikebrow commented Jul 10, 2017 •

edited

Loading

mikebrow commented Jul 10, 2017 •

edited

Loading

mikebrow Jul 19, 2018 •

edited

Loading