Fix for image spec #223

runcom · 2017-01-21T13:58:31Z

opencontainers/image-spec#411 removed the MediaType field in Manifest[List] causing builds error here. Fix that by teaching docker v2s2 to convert to OCI and remove that ugly manifest conversion in OCI image destination.

@mtrmac @cyphar PTAL

runcom · 2017-01-22T21:18:48Z

created a skopeo PR to validate this containers/skopeo#294

mtrmac

I very much like moving the conversion to image/; the piece in docker.newImageSource really does not belong there I think, though.

mtrmac · 2017-01-23T15:20:56Z

docker/docker_image_src.go

+	// and OCI doesn't understand it (it cannot convert from v2s1 to OCI)
+	for _, mt := range requestedManifestMIMETypes {
+		if mt == imgspecv1.MediaTypeImageManifest {
+			requestedManifestMIMETypes = append(requestedManifestMIMETypes, manifest.DockerV2Schema2MediaType)


This is ugly and suprising for an ImageSource to do (consider a caller who does not expect to use image.Image). Can we instead add the v2s1→OCI conversion? It might well be possible to do by creating a temporary v2s2 memoryImage (using the existing code) and then using the existing v2s2→OCI conversion.

(We haven’t supported a v2s1→OCI conversion before, so AFAICT this is not an essential part of this PR.)

makes sense to have v2s1 -> OCI at this point, so yeah, I'll implement this.

Considering the failing tests, could we merge a version without that conversion now, and deal with the v2s1→OCI conversion as a separate issue or PR?

wait, tests will fail w/o this mime type check, shall I leave this here?

What is the failure / code path?

basic skopeo copy docker://busybox oci:something, we need this to ask for at least v2s2 if we want OCI conversion (since we cannot convert from v2s1 which is the default when the registry doesn't understand the media type, see the comment, the test failure is in skopeo tests)

What is the failure / code path?

Let me guess: ociDest.SupportedManifestMIMETypes returns OCI only, and so copy.Image asks for unsupported manifest MIME type, and that gets sent to the registry.

Adding the OCI conversion is the long-term fix.

Short-term, can we:

move the array from dockerImageDestinationSupportedManifestMIMETypes into a global variable (shared by src and dest)

in newImageSource, ignore requestedManifestMIMETypes if it has no value common with the known supported MIME types (as defined by the variable from above)?

That’s still kind of a hack, but at least it is conceptually clean: dockerImage* knows what it supports/accepts, and does not need to know about OCI.

(*shrug*, if you want to merge it with the existing hack instead, feel free. Just please make sure to file an issue to have this cleaned up in the future.)

mtrmac · 2017-01-23T15:26:39Z

image/docker_schema2.go

+	// The only difference between OCI and DockerSchema2 is the mediatypes.
+	config.MediaType = imgspecv1.MediaTypeImageConfig
+
+	configBytes, err := m.ConfigBlob()


Is this really necessary to read the config here? It would perhaps be preferable to preserve the ImageSource and pass a nil configBlob, the way manifestOCI1.convertToManifestSchema2 does it.

mtrmac · 2017-01-23T15:30:05Z

oci/layout/oci_dest.go

-		om.MediaType = imgspecv1.MediaTypeImageManifest
-		for i, l := range om.Layers {
-			if l.MediaType == manifest.DockerV2Schema2ForeignLayerMediaType {
-				om.Layers[i].MediaType = imgspecv1.MediaTypeImageLayerNonDistributable


This …NonDistributable conversion is not present in the new conversion code, is that intentional?

I missed it somehow

mtrmac · 2017-01-23T15:31:25Z

oci/layout/oci_dest.go

+	}
+	// TODO(runcom): not fully sure how to prevent a docker v2s2 from slipping in
+	// any idea?!?!
+	if ociMan.SchemaVersion != 2 {


I am tempted no to try at all… But if we should, I think it would be much cleaner to have the caller supply a mimeType as a parameter than guessing by parsing contents in an unknown format in here.

let's skip this altogether for now and re-iterate on adding an optional mime type, but then, ppl might cheat on mime type as well no?

A lying caller will get what is coming to it :)

Right now we do do this wrong, because copy.determineManifestConversion can decide to keep an unsupported type anyway (it is treating SupportedManifestMIMETypes as a best-effort request), and PutManifest isn’t told about that. But we would be sending the correct MIME type to PutManifest if there was a way to do that.

mtrmac · 2017-01-23T15:37:07Z

image/docker_schema2.go

 	return memoryImageFromManifest(&copy), nil
 }

+func (m *manifestSchema2) convertToOCIManifest() (types.Image, error) {


Also, docker_schema2.go has (unlike …schema1.go) a reasonable test coverage, and it would be nice to preserve that (the conversions happen frequently enough for users to care but perhaps not frequently enough to prevent us from breaking them without noticing). This should be just a pretty trivial variant of TestConvertToManifestSchema2.

yup, have it on my todo, just wanted to bring this PR up for general consensus on the way I'm fixing this.

runcom · 2017-01-23T17:27:36Z

~~@mtrmac PTAL (warning, wrote it really fast just to prove it)~~

runcom · 2017-01-23T17:29:15Z

No, wait, we need the conversion, otherwise, the old createManifest in oci_dest.go can't work since OCI isn't defining the MediaType field anymore...

mtrmac · 2017-01-23T17:37:53Z

docker/docker_image_dest.go

+	manifest.DockerV2Schema1MediaType,
+}
+
+func supportedManifestMIMETypesMap() map[string]bool {


I think a map[string]struct{} is more idiomatic, but I really don’t care enough.

arg, that's how it was, then changed my mind, I'll put it back with struct{}

on a second though, I like comparing with just ! - we change this any time though...

Sure, whatever works.

mtrmac · 2017-01-23T17:39:21Z

docker/docker_image_src.go

 		requestedManifestMIMETypes = manifest.DefaultRequestedManifestMIMETypes
 	}
+	for _, mtrequested := range requestedManifestMIMETypes {
+		if !supportedManifestMIMETypesMap()[mtrequested] {


This is still O(N^2): move the call supportedManifestMIMETypesMap outside the loop, save it in a variable. Then it is O(N).

(Or don’t bother and just write two nested loops. The arrays are small enough.)

This is still O(N^2): move the call supportedManifestMIMETypesMap outside the loop, save it in a variable. Then it is O(N).

silly me, ETOOLATE

mtrmac · 2017-01-23T17:40:23Z

docker/docker_image_src.go

+	for _, mtrequested := range requestedManifestMIMETypes {
+		if !supportedManifestMIMETypesMap()[mtrequested] {
+			requestedManifestMIMETypes = manifest.DefaultRequestedManifestMIMETypes
+			break


This would ignore the input value if any value is unrecognized; I think it should ignore input only if all values are unrecognized (so that a caller can say “i prefer v2s1, then OCI, then v2s2 as a last resort”.)

runcom · 2017-01-24T12:00:43Z

@mtrmac PTAL

mtrmac

Thanks! Just a minor nit.

WRT merging, do you want to include #225 in this so that we have a single PR which passes tests and fixes the build?

mtrmac · 2017-01-24T14:41:17Z

docker/docker_image_src.go

+		if !mts[mtrequested] {
+			continue
+		}
+		reset = false


break here, and then it’s more natural to reword as if mts[mtrequested] { EDIT reset = false;break }.

mtrmac · 2017-01-24T14:44:25Z

docker/docker_image_src.go

 	}
+	var (
+		mts   = supportedManifestMIMETypesMap()
+		reset = true


(and as long you are changing this) this would benefit from either clearer variable names, or a comment explicitly describing what this loop intends to do. Preferably the former, perhaps naming them supportedMIMEs and acceptableRequestedMIMEs (with the opposite semantics to reset, starting as false).

…also, no need for the var() here AFAICS, just use :=. Or does that have an idiomatic meaning?

runcom · 2017-01-24T14:59:31Z

WRT merging, do you want to include #225 in this so that we have a single PR which passes tests and fixes the build?

yeah, I'll cherry-pick #225 here

Instead, copy the error.NewAggregate implementation inside. This allows use of this package in OpenShift, which uses an older version of k8s.io packages. Signed-off-by: Miloslav Trmač <[email protected]>

Instead, inline net.SetTransportDefaults and copy net.NewProxierWithNoProxyCIDR. This allows use of this package in OpenShift, which uses an older version of k8s.io packages. Signed-off-by: Miloslav Trmač <[email protected]>

…o/client-go Because this is an entirely new package, depending on any version of it should hopefully not be problematic for downstreams. Signed-off-by: Miloslav Trmač <[email protected]>

runcom · 2017-01-24T15:25:53Z

@mtrmac I've cherry-picked #225 here and hopefully addressed your comments, PTAL

mtrmac · 2017-01-24T15:31:48Z

docker/docker_image_src.go

+			continue
+		}
+		acceptableRequestedMIMEs = false
+		break


This rejects requestedManifestMIMETypes if any content is unrecognized; shouldn’t it reject them only if none are recognized?

(i.e. when reaming reset to acceptableRequestedMIMEs, only the last if was flipped AFAICS.)

I believe I've fixed it now - sorry I'm in a meeting but want to get this in asap to unblock other PRs

I was thinking

supportedMIMEs := supportedManifestMIMETypesMap() acceptableRequestedMIMEs := false for _, mtrequested := range requestedManifestMIMETypes { if supportedMIMEs[mtrequested] { acceptableRequestedMIMEs = true break } } if !acceptableRequestedMIMEs { requestedManifestMIMETypes = manifest.DefaultRequestedManifestMIMETypes }

Perhaps I am being an idiot?

fixed and force pushed

runcom · 2017-01-24T23:33:40Z

another issue just popped up https://travis-ci.org/containers/image/builds/195004166#L346 - perhaps image-spec shouldn't really have a vendor directory at all. Commented here #223

Signed-off-by: Antonio Murdaca <[email protected]>

mtrmac · 2017-01-25T13:44:35Z

another issue just popped up https://travis-ci.org/containers/image/builds/195004166#L346 - perhaps image-spec shouldn't really have a vendor directory at all. Commented here #223

( opencontainers/image-spec#527 , for the record.)

mtrmac · 2017-01-25T13:49:29Z

👍 pending tests

runcom · 2017-01-25T13:53:51Z

@mtrmac will we wait till the image-spec-vendor-dir is gone before merging this?

mtrmac · 2017-01-25T14:13:49Z

@mtrmac will we wait till the image-spec-vendor-dir is gone before merging this?

I dunno, what other options do we have? Just keep our old checkouts on our development machines, and ignore Travis and keep merging other PRs? Ewww.

Or, give up and set up containers/image with our own vendoring mechanism? That would work for us but I worry that it would make it even more difficult for third parties to use the library, as conflicts would inevitably happen.

Is there a chance that the vendoring will be fixed quickly?

runcom · 2017-01-25T14:16:47Z

Is there a chance that the vendoring will be fixed quickly?

not sure, I hope it'll be really soon

Due to the new vendoring logic in the golang compiler it can cause issues for projects that import a package that has vendored a package that is used locally. See containers/image#223 This change moves the vendored sources to the package that uses them, rather than for the whole project. Also is explictly is not vendoring code repos from "github.com/opencontainers/". For now we'll consider these non-remote. Though versioning may likely be future concern. Fixes opencontainers#527 Obsoletes opencontainers#528 Signed-off-by: Vincent Batts <[email protected]>

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2017-01-27T22:17:59Z

seems like something else in OCI image-spec broke this..

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2017-01-27T22:46:07Z

alright, pretty sure I've this sorted out now, @mtrmac PTAL

mtrmac

Yay!!

This looks really good, it seems a skopeo revendor will fix everything.

mtrmac · 2017-01-28T09:14:22Z

image/docker_schema2.go

+		if m.LayersDescriptors[idx].MediaType == manifest.DockerV2Schema2ForeignLayerMediaType {
+			layers[idx].MediaType = imgspecv1.MediaTypeImageLayerNonDistributable
+		} else {
+			layers[idx].MediaType = imgspecv1.MediaTypeImageLayerGzip


This works for now (i.e. at it was perfectly valid a week ago), longer-term we should actually know whether the input is gzipped. Filed #229.

runcom · 2017-01-28T09:30:27Z

alright let's merge this (and take care of this comment #229 (comment) later)

runcom · 2017-01-28T11:52:43Z

👍

mtrmac · 2017-01-28T12:31:01Z

👍

runcom mentioned this pull request Jan 21, 2017

Fix top-level source example in godoc #217

Merged

runcom force-pushed the fix-for-image-spec branch 4 times, most recently from 2a89e0f to 3215330 Compare January 22, 2017 21:16

runcom mentioned this pull request Jan 22, 2017

fix OCI image-spec dependency containers/skopeo#294

Merged

runcom force-pushed the fix-for-image-spec branch from 3215330 to 15621ae Compare January 22, 2017 21:27

mtrmac requested changes Jan 23, 2017

View reviewed changes

mtrmac reviewed Jan 23, 2017

View reviewed changes

runcom force-pushed the fix-for-image-spec branch 2 times, most recently from 24efd99 to a5c3380 Compare January 23, 2017 17:27

mtrmac requested changes Jan 23, 2017

View reviewed changes

mtrmac mentioned this pull request Jan 23, 2017

Remove dependencies on k8s.io/apimachinery #225

Closed

runcom force-pushed the fix-for-image-spec branch 2 times, most recently from e19c1ba to 8246d68 Compare January 24, 2017 11:59

runcom force-pushed the fix-for-image-spec branch from 8246d68 to ab0bcd5 Compare January 24, 2017 12:06

runcom mentioned this pull request Jan 24, 2017

docker: mimic docker upstream registry authentication #211

Merged

mtrmac reviewed Jan 24, 2017

View reviewed changes

mtrmac added 3 commits January 24, 2017 16:00

Do not depend on k8s.io/apimachinery/pkg/util/error

7f03b77

Instead, copy the error.NewAggregate implementation inside. This allows use of this package in OpenShift, which uses an older version of k8s.io packages. Signed-off-by: Miloslav Trmač <[email protected]>

Do not depend on k8s.io/apimachinery/pkg/util/net

6bb5100

Instead, inline net.SetTransportDefaults and copy net.NewProxierWithNoProxyCIDR. This allows use of this package in OpenShift, which uses an older version of k8s.io packages. Signed-off-by: Miloslav Trmač <[email protected]>

Update for move of …/pkg/util/homedir from k8s.io/kubernetes to k8s.i…

4f6f739

…o/client-go Because this is an entirely new package, depending on any version of it should hopefully not be problematic for downstreams. Signed-off-by: Miloslav Trmač <[email protected]>

runcom force-pushed the fix-for-image-spec branch from ab0bcd5 to df1e240 Compare January 24, 2017 15:25

mtrmac reviewed Jan 24, 2017

View reviewed changes

runcom force-pushed the fix-for-image-spec branch from df1e240 to d982fea Compare January 24, 2017 15:40

mtrmac mentioned this pull request Jan 24, 2017

Allow choosing between a gpgme and openpgp signature backend using a build tag #207

Merged

runcom force-pushed the fix-for-image-spec branch from d982fea to 6f10a87 Compare January 24, 2017 23:26

fix OCI image-spec dependency

6a51d25

Signed-off-by: Antonio Murdaca <[email protected]>

mtrmac approved these changes Jan 25, 2017

View reviewed changes

runcom mentioned this pull request Jan 25, 2017

Canonicalize docker references in signature.VerifyDockerManifestSignature #227

Merged

vbatts mentioned this pull request Jan 26, 2017

vendoring: isolate the vendor directory opencontainers/image-spec#532

Closed

runcom mentioned this pull request Jan 27, 2017

ListPodSandbox: bad wiretype for field runtime.PodSandboxFilter.State cri-o/cri-o#339

Closed

mtrmac mentioned this pull request Jan 27, 2017

Problem with package bumping (via vendor.sh) cri-o/cri-o#343

Closed

runcom force-pushed the fix-for-image-spec branch from 6f10a87 to 6a51d25 Compare January 27, 2017 22:10

openshift: fix import for k8s.io/client-go/util/homedir

3f53928

Signed-off-by: Antonio Murdaca <[email protected]>

runcom added 2 commits January 27, 2017 23:45

image: adapt to latest image-spec with gzip layer mediatype

707ca5d

Signed-off-by: Antonio Murdaca <[email protected]>

storage: adapt to latest containers/storage

18949fa

Signed-off-by: Antonio Murdaca <[email protected]>

mtrmac approved these changes Jan 28, 2017

View reviewed changes

runcom mentioned this pull request Jan 28, 2017

docker→OCI conversion should differentiate compressed and uncompressed layers #229

Closed

runcom merged commit fbc7955 into containers:master Jan 28, 2017

runcom deleted the fix-for-image-spec branch January 28, 2017 11:52

mtrmac mentioned this pull request Mar 14, 2017

Reject duplicate JSON members when parsing signatures #252

Merged

Fix for image spec #223

Fix for image spec #223

Uh oh!

Conversation

runcom commented Jan 21, 2017

Uh oh!

runcom commented Jan 22, 2017

Uh oh!

mtrmac left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Jan 23, 2017

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!

mtrmac Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

runcom commented Jan 24, 2017

Uh oh!

mtrmac left a comment

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

runcom commented Jan 23, 2017 •

edited

Loading

mtrmac Jan 23, 2017 •

edited

Loading

runcom Jan 24, 2017 •

edited

Loading

runcom commented Jan 24, 2017 •

edited

Loading

mtrmac commented Jan 25, 2017 •

edited by runcom

Loading