Implement schema2 manifest formats #1281

aaronlehmann · 2015-12-18T01:41:33Z

This PR introduces support for the schema2 manifest defined in #1068. This covers both an "image manifest", which supersedes the schema1 manifest format, and a "manifest list" that points to the appropriate manifests for specific platforms, to facilitate multi-architecture images.

The Accept header is used to gauge which schema versions are accepted by a client. If the client does not indicate that it accepts the schema2 format, and requests a manifest that is stored in this format, the registry will automatically convert it to schema1 format for backward compatibility.

schema1's ManifestBuilder has been renamed to ReferenceBuilder. This PR introduces a second schema1 ManifestBuilder called ConfigBuilder, which can generate a schema1 manifest from a Docker 1.10+ image configuration and a set of descriptors. This is used to enable the conversion of schema2 manifests to schema1 format for backward compatibility, and will also be used by the engine to push schema1 manifests to registries that don't yet support schema2.

aaronlehmann · 2015-12-18T02:38:27Z

BTW, corresponding engine-side code is at https://github.com/aaronlehmann/docker/commits/new-manifest

RichardScothern · 2015-12-18T06:01:32Z

manifest/manifestlist/manifestlist.go

+}
+
+// Target is not used with manifest lists.
+func (m ManifestList) Target() distribution.Descriptor {


Target() is no longer in the interface

stevvooe · 2015-12-18T19:34:27Z

blobs.go

@@ -61,6 +61,12 @@ type Descriptor struct {
 	// depend on the simplicity of this type.
 }

+// Descriptor returns the descriptor, to make it satisfy the Describable


This does not satisfy the Describable interface. Describable must describe itself (in this case, the descriptor), not the thing it is describing.

This does not satisfy the Describable interface. Describable must describe itself (in this case, the descriptor), not the thing it is describing.

There's a common use case where some code has a slice of descriptors and wants to add those descriptors to a manifest through a ManifestBuilder. Since ManifestBuilder.AppendReference takes a Describable, not having this method defined would force the creation of a wrapper type just to return a descriptor.

Instead of having Descriptor satisfy Describable, would you be okay with changing AppendReference to take a Descriptor?

I see. The idea of having an interface there is to allow implementations to type switch on elements being added to the manifest and support specialized behavior. I suppose having Descriptor implement Describable is a fine exception to make this work. Let's add a comment there, indicating this is the exception and not the rule.

Added a comment explaining this.

dmcgowan · 2015-12-22T18:47:34Z

registry/storage/manifeststore.go

+		case manifestlist.MediaTypeManifestList:
+			return ms.manifestListHandler.Unmarshal(ctx, dgst, content)
+		default:
+			return nil, distribution.ErrManifestVerification{errors.New("unrecognized manifest content type")}


might be useful to either add mediaType.MediaType to the error messages or add a debug log with it.

dmcgowan · 2015-12-22T19:19:18Z

manifest/manifestlist/manifestlist.go

+	ManifestList
+
+	// Canonical is the canonical byte representation of the Manifest.
+	Canonical []byte `json:"-"`


Not a fan of this getting. I see it is only used internally to the package and in api tests. I think it would be a better pattern to expose this only as a function then it can either be type asserted if it is needed or called directly, but never set outside the package. Same goes for manifest list. Schema1 does this pattern but schema1 is already all kinds of messed up 😛

It can probably be unexported in schema2 and manifestlist because Payload returns this value. OTOH, Payload returns an error value (currently hardcoded to nil), which will complicate engine code that needs to access the JSON.

Unexported Canonical in schema2 and manifestlist.

dmcgowan · 2015-12-22T22:49:53Z

LGTM

RichardScothern · 2016-01-07T19:53:23Z

registry/handlers/images.go

@@ -83,6 +91,64 @@ func (imh *imageManifestHandler) GetImageManifest(w http.ResponseWriter, r *http
 		return
 	}

+	supportsSchema2 := false
+	supportsManifestList := false
+	if acceptHeaders, ok := r.Header["Accept"]; ok {


I don't think it's a problem since you control the client behavior. but strictly, accept headers are supposed to be comma separated.

I think the Go HTTP library handles that when it generates this map? Note the map values are slices.

Make sure to use http.CanonicalHeader when accessing the header map directly.

Make sure to use http.CanonicalHeader when accessing the header map directly.

I agree that this is necessary in the general case, but it looks like this would be a noop if the key is already properly formatted.

stevvooe · 2016-01-07T22:18:18Z

manifest/schema2/builder.go

+	// calls to AppendReference.
+	layers []distribution.Descriptor
+}
+


Can you construct a builder from an existing manifest?

Can you construct a builder from an existing manifest?

The way this is done is by first creating a builder, then passing the references from the existing manifest to AppendReference. See convertSchema2Manifest for an example.

Should that be a specific method or does it not generalize well?

I think it shouldn't be a method in the ManifestBuilder interface, because it doesn't need to be implemented separately by every builder. It could be a utility function, but it would only factor out a few lines of code that only exist in one place, so I'm not sure it's a big win.

For reference, here's what this utility function would look like:

func ConvertManifest(builder distribution.ManifestBuilder, existingManifest distribution.Manifest) (distribution.Manifest, error) { for _, d := range existingManifest.References() { if err := builder.AppendReference(d); err != nil { return nil, err } } return builder.Build(imh) }

stevvooe · 2016-01-07T22:20:28Z

registry/handlers/app.go

@@ -67,6 +68,8 @@ type App struct {

 	redis *redis.Pool

+	trustKey libtrust.PrivateKey


Please document correct use of this field: only for signing backwards compat requests.

codecov-io · 2016-01-07T22:24:56Z

Current coverage is `58.15%`

Merging #1281 into master will increase coverage by +0.50% as of 6d17423

@@            master   #1281   diff @@
======================================
  Files          116     123     +7
  Stmts        10477   10948   +471
  Branches       718     763    +45
  Methods          0       0       
======================================
+ Hit           6041    6367   +326
- Partial        718     763    +45
- Missed        3718    3818   +100

Review entire Coverage Diff as of 6d17423

Uncovered Suggestions

+0.29% via ...ge/driver/gcs/gcs.go#185...216
+0.29% via ...ge/driver/gcs/gcs.go#104...135
+0.22% via ...ge/driver/gcs/gcs.go#474...497
See 7 more...

Powered by Codecov. Updated on successful CI builds.

stevvooe · 2016-01-07T22:25:54Z

registry/storage/manifeststore.go

-	}
-
 	return nil
+	panic("not supported")


Why not return 0, distribution.ErrUnsupported?

Why not return 0, distribution.ErrUnsupported?

I suspect this came about through a rebase. Reverting it to the ErrUnsupported version.

stevvooe · 2016-01-07T22:38:42Z

LGTM

Most comments are for clarification. We can probably defer the additions to another PR.

Add schema2 manifest implementation. Add a schema2 builder that creates a schema2 manifest from descriptors and a configuration. It will add the configuration to the blob store if necessary. Rename the original schema1 manifest builder to ReferenceBuilder, and create a ConfigBuilder variant that can build a schema1 manifest from an image configuration and set of descriptors. This will be used to translate schema2 manifests to the schema1 format for backward compatibliity, by adding the descriptors from the existing schema2 manifest to the schema1 builder. It will also be used by engine-side push code to create schema1 manifests from the new-style image configration, when necessary to push a schema1 manifest. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Create signedManifestHandler and schema2ManifestHandler. Use these to unmarshal and put the respective types of manifests from manifestStore. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

…o schema1 on the fly Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Convert a default platform's manifest to schema1 on the fly. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

This makes content type sniffing cleaner. The document just needs to be decoded into a manifest.Versioned structure. It's no longer a two-step process. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

aaronlehmann · 2016-01-07T23:29:34Z

Also added tests checking the serialized format of schema2 and manifestlist against constant values. I think this is ready to go.

Implement schema2 manifest formats

GordonTheTurtle added group/distribution labels Dec 18, 2015

aaronlehmann force-pushed the new-manifest branch 3 times, most recently from 7438f8e to 9b89d7b Compare December 18, 2015 02:28

RichardScothern reviewed Dec 18, 2015
View reviewed changes

aaronlehmann force-pushed the new-manifest branch from 9b89d7b to 91c1169 Compare December 18, 2015 19:17

stevvooe reviewed Dec 18, 2015
View reviewed changes

aaronlehmann mentioned this pull request Dec 18, 2015

docs/spec: Proposal for new manifest format #1068

Merged

aaronlehmann force-pushed the new-manifest branch 2 times, most recently from 667ec84 to 2591280 Compare December 18, 2015 20:09

aaronlehmann mentioned this pull request Dec 19, 2015

New image manifest format moby/moby#18785

Merged

dmcgowan reviewed Dec 22, 2015
View reviewed changes

aaronlehmann force-pushed the new-manifest branch from 2591280 to 2b7e821 Compare December 22, 2015 19:17

dmcgowan reviewed Dec 22, 2015
View reviewed changes

aaronlehmann force-pushed the new-manifest branch 2 times, most recently from 5183f5d to 21dece4 Compare December 22, 2015 22:17

RichardScothern reviewed Jan 7, 2016
View reviewed changes

aaronlehmann force-pushed the new-manifest branch from 9570c1b to bb51139 Compare January 7, 2016 22:03

stevvooe reviewed Jan 7, 2016
View reviewed changes

aaronlehmann force-pushed the new-manifest branch from bb51139 to 26f43f7 Compare January 7, 2016 23:19

aaronlehmann added 7 commits January 7, 2016 15:26

Factor out schema-specific portions of manifestStore

befd4d6

Create signedManifestHandler and schema2ManifestHandler. Use these to unmarshal and put the respective types of manifests from manifestStore. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Recognize clients that don't support schema2, and convert manifests t…

3f746a8

…o schema1 on the fly Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Add API unit testing for schema2 manifest

9284810

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Add support for manifest list ("fat manifest")

9c416f0

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Move MediaType into manifest.Versioned

6d17423

This makes content type sniffing cleaner. The document just needs to be decoded into a manifest.Versioned structure. It's no longer a two-step process. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

aaronlehmann force-pushed the new-manifest branch from 26f43f7 to 6d17423 Compare January 7, 2016 23:28

stevvooe added a commit that referenced this pull request Jan 8, 2016

Merge pull request #1281 from aaronlehmann/new-manifest

a7ae88d

Implement schema2 manifest formats

stevvooe merged commit a7ae88d into distribution:master Jan 8, 2016

aaronlehmann deleted the new-manifest branch January 8, 2016 01:22

aaronlehmann mentioned this pull request Jan 11, 2016

Remove tags referencing deleted manifests. #1319

Merged

thaJeztah pushed a commit to thaJeztah/distribution that referenced this pull request Apr 22, 2021

Merge pull request distribution#1281 from aaronlehmann/new-manifest

2b1c120

Implement schema2 manifest formats

thaJeztah pushed a commit to thaJeztah/distribution that referenced this pull request Jan 19, 2022

Merge pull request distribution#1281 from aaronlehmann/new-manifest

c426367

Implement schema2 manifest formats

GanfengTan mentioned this pull request Jun 16, 2022

[INLONG-4673][Docker] Support for building Docker images for the ARM architecture apache/inlong#4675

Merged

thaJeztah mentioned this pull request Apr 30, 2023

Descriptor: do not implement Describable interface #3886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement schema2 manifest formats #1281

Implement schema2 manifest formats #1281

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -67,6 +68,8 @@ type App struct {

		redis *redis.Pool

		trustKey libtrust.PrivateKey

Implement schema2 manifest formats #1281

Implement schema2 manifest formats #1281

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Current coverage is 58.15%

Uncovered Suggestions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Current coverage is `58.15%`