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

vbatts · 2016-10-27T16:08:32Z

This commit adds support to both the registry and clients for the OCI
image spec defined here: https://github.com/opencontainers/image-spec.

The results of these changes is that the registry can handle pushes and
pulls for both docker and OCI images, and will store them separately.
Clients will specify, via the Accept header, that when pulling they can
accept both OCI manifests and docker manifests. If a client supports
both OCI images and docker images, the registry will prefer to return an
OCI image.

Specifically the changes made by this commit are:

registry/handlers/images.go: Adds support for handling pushes and
pulls of manifests with an OCI mediatype, determined by the Accept
header. When an OCI manifest is pushed, the tag entered into the
tagstore is prepended with oci:, to allow the registry to store
both a docker manifest and an OCI manifest with the same name/tag.
This string is then added to OCI tags being pulled. This string was
chosen due to the : in it making it an invalid tag for a client to
use.
registry/storage/driver/storagedriver.go: Added : to the regexp used
to determine valid paths, which is necessary for the prepending of
oci:.
manifest/schema2/manifest.go, manifest/manifestlist/manifestlist.go:
Added the mediatypes for the OCI spec, and registered them. This
results in clients adding these types to the Accept headers when
pulling images.
manifest/schema2/builder.go: Added an argument to the manifest builder
for schema2 to enable it to build both docker images and OCI images.
registry/storage/manifeststore.go: adds the OCI mediatypes to allow
storage of OCI manifests in addition to docker manifests.
set default mediatypes used for manifest and manifest list

While this is a work-in-progress, it needs conversation/review to arrive at the best approach.

Signed-off-by: Vincent Batts vbatts@hashbangbash.com

This commit adds support to both the registry and clients for the OCI image spec defined here: https://github.com/opencontainers/image-spec. The results of these changes is that the registry can handle pushes and pulls for both docker and OCI images, and will store them separately. Clients will specify, via the Accept header, that when pulling they can accept both OCI manifests and docker manifests. If a client supports both OCI images and docker images, the registry will prefer to return an OCI image. Specifically the changes made by this commit are: - registry/handlers/images.go: Adds support for handling pushes and pulls of manifests with an OCI mediatype, determined by the Accept header. When an OCI manifest is pushed, the tag entered into the tagstore is prepended with `oci:`, to allow the registry to store both a docker manifest and an OCI manifest with the same name/tag. This string is then added to OCI tags being pulled. This string was chosen due to the `:` in it making it an invalid tag for a client to use. - registry/storage/driver/storagedriver.go: Added `:` to the regexp used to determine valid paths, which is necessary for the prepending of `oci:`. - manifest/schema2/manifest.go, manifest/manifestlist/manifestlist.go: Added the mediatypes for the OCI spec, and registered them. This results in clients adding these types to the Accept headers when pulling images. - manifest/schema2/builder.go: Added an argument to the manifest builder for schema2 to enable it to build both docker images and OCI images. - registry/storage/manifeststore.go: adds the OCI mediatypes to allow storage of OCI manifests in addition to docker manifests. - set default mediatypes used for manifest and manifest list Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>

vbatts · 2016-10-27T16:11:53Z

github.com/docker/distribution/registry/handlers tests fail for reasons I can not tell.

vbatts · 2016-10-27T16:12:11Z

cc @duglin @mikebrow

mikebrow · 2016-10-27T16:15:52Z

Whoot! Code looks good, I'll test it out.

dmcgowan · 2016-10-27T18:43:30Z

registry/handlers/images.go

@@ -183,6 +223,8 @@ func (imh *imageManifestHandler) GetImageManifest(w http.ResponseWriter, r *http
 	w.Header().Set("Docker-Content-Digest", imh.Digest.String())
 	w.Header().Set("Etag", fmt.Sprintf(`"%s"`, imh.Digest))
 	w.Write(p)
+
+	fmt.Printf("\n\nThey succeeded!\n\n\n")


This shouldn't be here...without 🎉, maybe fmt.Printf("\n\nThey succeeded! 🎉🎉🎉🎉\n\n\n")

dmcgowan · 2016-10-27T18:56:30Z

Based on the size of this change, not sure I see the advantage of merging it into the schema2 package. While OCI is based on schema2, they are not the same and shouldn't need to be treated as such. An OCI manifest will still need to be converted to a schema2 (or schema1 manifest) for compatibility with older client. I would rather see the addition of the OCI manifest support as similar we would for schema3 of the manifest, even though I wish it added more improvements over schema2 rather than just changing media types (like renaming layers to references).

vbatts · 2016-10-27T20:22:14Z

That is an interesting idea. The whole notion of the image spec is to be compatible, regardless of it being just a mediatype difference. What are the implications of it being a "schema 3"?

dmcgowan · 2016-10-27T21:19:27Z

The whole notion of the image spec is to be compatible

My point is that it isn't and conversion will be necessary not matter how similar it is. Yes the way it is now makes the conversion easier, but as long the manifest is just a set of digest references and types, the format really doesn't matter and converting is trivial. Just a change in the media types means docker will not understand the manifest and has to be updated, also making pull by digest of OCI images in older clients fail.

Knowing that we want to avoid having to make even more of these changes in the future, from my opinion OCI support should add something more than just supporting a similar manifest with different media type values. It should be an improvement based on what we learned from schema 2, I don't see any of that represented in this change. I am not saying that because I don't want OCI support, but rather I think that rushing to pin OCI image spec to a v1 and add support here gains nothing over schema 2 and just increases the support and conversion matrix. The implications of it being "schema 3" are that it is an improvement over schema 2 and requires clients explicitly listing this new version as supported, else we must convert to schema 2 and return. Even if the difference is just media types, conversion must happen, and digests will differ.

stevvooe · 2016-10-27T21:23:16Z

manifest/manifestlist/manifestlist.go

+
+	// DefaultMediaTypeManifestList is the default choice of manifest list type
+	// when marshalling/serving content
+	DefaultMediaTypeManifestList = MediaTypeOCIManifestList


Why would this be the default? The registry doesn't decide the serialization. It is up to the client.

stevvooe · 2016-10-27T21:23:55Z

manifest/schema2/builder.go

+)
+
+// ImageType for the Manifest to be built
+type ImageType int


Why introduce a parallel type system when we already have mediatypes?

stevvooe · 2016-10-27T21:26:25Z

manifest/manifestlist/manifestlist.go


 // SchemaVersion provides a pre-initialized version structure for this
 // packages version of the manifest.
 var SchemaVersion = manifest.Versioned{
 	SchemaVersion: 2,
-	MediaType:     MediaTypeManifestList,
+	MediaType:     DefaultMediaTypeManifestList,


@vbatts We should really remove the media type embedded in the content itself. It makes this code much more problematic. Changing the type shouldn't change the content.

We should really remove the media type embedded in the content itself. It makes this code much more problematic. Changing the type shouldn't change the content.

IIRC this was necessary because the registry doesn't keep track of the media types of manifests. Changing it would require creating the infrastructure for the registry to do this.

stevvooe · 2016-10-27T21:29:31Z

This should really be a separate package. While OCI and schema2 are similar, these packages are designed to represent one media type each.

mikebrow · 2016-10-27T22:31:54Z

Separate package is how was going to do my version of this, since the style of the code appeared to be one package per media type. But I don't think it matters much. A separate package for each new media type x schema type.. Pro: Easier to read as separate... Code + Type Isolation... Con: Cut and Pasting a lot of code to keep them separate when the internals are not really different. My guess is if they did it with a separate package someone would be arguing to merge them :)

stevvooe · 2016-11-01T01:00:14Z

@mikebrow The packages may share code, but we are not taking on the technical debt of making a single package do two things.

vbatts · 2016-11-11T23:05:32Z

Ah

On Fri, Nov 11, 2016, 17:50 Aaron Lehmann notifications@github.com wrote:

@aaronlehmann commented on this pull request.

In manifest/manifestlist/manifestlist.go
#2021:

// SchemaVersion provides a pre-initialized version structure for this
// packages version of the manifest.
var SchemaVersion = manifest.Versioned{
SchemaVersion: 2,

MediaType: MediaTypeManifestList,

MediaType: DefaultMediaTypeManifestList,

We should really remove the media type embedded in the content itself. It
makes this code much more problematic. Changing the type shouldn't change
the content.

IIRC this was necessary because the registry doesn't keep track of the
media types of manifests. Changing it would require creating the
infrastructure for the registry to do this.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2021, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEF6c47o-FyD9Z_HaIeljebyi6TwqiWks5q9PElgaJpZM4KiiaR
.

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

Deferring to #2076

GordonTheTurtle added the status/0-triage label Oct 27, 2016

dmcgowan reviewed Oct 27, 2016

View reviewed changes

stevvooe reviewed Oct 27, 2016

View reviewed changes

runcom mentioned this pull request Oct 28, 2016

OCI image source containers/image#141

Closed

mikebrow mentioned this pull request Nov 21, 2016

Adds support for oci manifests and manifestlists #2076

Merged

dmcgowan added status/1-design-review and removed status/0-triage labels Nov 23, 2016

vbatts closed this Nov 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vbatts commented Oct 27, 2016

vbatts commented Oct 27, 2016

vbatts commented Oct 27, 2016

mikebrow commented Oct 27, 2016

dmcgowan Oct 27, 2016

vbatts Oct 27, 2016

dmcgowan commented Oct 27, 2016

vbatts commented Oct 27, 2016

dmcgowan commented Oct 27, 2016

stevvooe Oct 27, 2016

stevvooe Oct 27, 2016

stevvooe Oct 27, 2016

aaronlehmann Nov 11, 2016

stevvooe commented Oct 27, 2016

mikebrow commented Oct 27, 2016

stevvooe commented Nov 1, 2016

vbatts commented Nov 11, 2016

@aaronlehmann commented on this pull request.

vbatts commented Nov 24, 2016

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

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

Conversation

vbatts commented Oct 27, 2016

vbatts commented Oct 27, 2016

vbatts commented Oct 27, 2016

mikebrow commented Oct 27, 2016

dmcgowan Oct 27, 2016

Choose a reason for hiding this comment

vbatts Oct 27, 2016

Choose a reason for hiding this comment

dmcgowan commented Oct 27, 2016

vbatts commented Oct 27, 2016

dmcgowan commented Oct 27, 2016

stevvooe Oct 27, 2016

Choose a reason for hiding this comment

stevvooe Oct 27, 2016

Choose a reason for hiding this comment

stevvooe Oct 27, 2016

Choose a reason for hiding this comment

aaronlehmann Nov 11, 2016

Choose a reason for hiding this comment

stevvooe commented Oct 27, 2016

mikebrow commented Oct 27, 2016

stevvooe commented Nov 1, 2016

vbatts commented Nov 11, 2016

@aaronlehmann commented on this pull request.

vbatts commented Nov 24, 2016