manifest: enable DockerV2List #115

runcom · 2016-10-01T10:39:50Z

Related to #114

Needed for projectatomic/docker#200 - w/o this pulling an image with a manifest list is broken into docker.
Rigth now, by not supporting the manifest list media type we always got the single manifest for the amd64 arch instead of the whole manifest list and this broke docker because computing the digest for that single manifest isn't the same as the digest from the manifest list.

Also note that if you pull-by-digest a manifest list in docker you get back exactly the same manifest list digest (and not the one from the target manifest) which is confusing to me but still, it's how docker works.

@mtrmac PTAL - this is blocking

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

runcom · 2016-10-02T15:33:46Z

Test are failing in skopeo because estesp/busybox is a manifest list (and we already had this issue in the past). We really need a way for this, otherwise we'll break docker in projectatomic/docker#200.

runcom · 2016-10-02T15:55:53Z

I need to fix image.getParsedManifest to deal with manifest lists (unsure on the approach though)

Manifest lists don't carry configuration or such, I think from a manifest list we need to resolve the target manifest for the current arch+platform (as docker does) and transparently work with that. Still, we need to enable this. I'll fix this tomorrow.

runcom · 2016-10-02T15:59:16Z

@mtrmac I believe that by transparently resolving the target manifest for the current platform+arch we won't break signatures since we're going to retrieve and play with the correct single manifest.
The fact that it's a manifest list to me means only that we need another step to retrieve the current correct manifest and nothing else changes. I hope I'm correct.

runcom · 2016-10-03T10:34:46Z

@mtrmac I've added a new GetTargetManifest(reference string) to image source in order to retrieve a manifest from a manifest list. It could be called GetManifestFromList() maybe and if the image's manifest for that image isn't a manifest list it errors out? wdyt?

OCI has this concept of a manifest list also, it makes sense have this new method in the image source interface. GetManifestFromList(arch, platform, ...) or GetTargetManifest(arch, platform, ...).

mtrmac

So if I understand this correctly, Manifest and ManifestDigest will return the data of the manifest list, but LayerInfos etc. would all work with the underlying manifest data.

This works for the interface of types.Image narrowly read, and I guess it does allow signing of, and verifying signatures of, manifest lists, but it seems that it would completely break copy.Image; that method does need to know that there is a separate manifest list and a separate manifest to copy, and to copy both (or to copy all of the listed images, even).

mtrmac · 2016-10-03T16:05:44Z

docker/docker_image_src.go

 }

+// GetTargetManifest ...
+func (s *dockerImageSource) GetTargetManifest(reference string) ([]byte, string, error) {


This will need a documented semantics for reference (this is definitely not a Docker reference, nor a ImageReference. I guess the input should be a digest, of perhaps a types.BlobInfo, like in GetBlob.

OCI has this concept of a manifest list also, it makes sense have this new method in the image source interface. GetManifestFromList(arch, platform, ...) or GetTargetManifest(arch, platform, ...).

I think parsing the lists and matching by arch/platform belongs to the manifest parsing code in image, not in the individual ImageSources.

There really should be a single function with this code, shared by GetTargetManifest and ensureManifestIsLoaded.

I think parsing the lists and matching by arch/platform belongs to the manifest parsing code in image, not in the individual ImageSources.

isn't it that way now? that code is in image/docker_list.go

The method declaration quoted above, and “new method in the image source interface”, are in ImageSource.

(To be clear, ImageSource.GetManifestByDigest is perfectly fine; I didn’t like the rch/platform logic in the sources.)

mtrmac · 2016-10-03T16:09:24Z

image/docker_list.go

+	if err != nil {
+		return nil, err
+	}
+	switch mt {


This switch should really be shared with the one in genericImage.getParsedManifest. Add an genericImage.manifestInstanceFromBlob and call it from both places?

mtrmac · 2016-10-03T16:16:50Z

I guess, to proceed for now, add an Image.IsMultiArchImage (Image.IsManifestList to more directly refer to the implementation?), and completely refuse to work with such things at the start of copy.Image?

That might allow not regressing docker, while we would more or less still refuse to sign/verify such images in skopeo. Assuming that they don’t become widespread…

mtrmac · 2016-10-03T16:21:36Z

Test are failing in skopeo because estesp/busybox is a manifest list (and we already had this issue in the past). We really need a way for this, otherwise we'll break docker in projectatomic/docker#200.

We use estesp/busybox mostly to get a schema1 manifest for OpenShfit, and that runs into other difficulties for #88 .

I think a natural way to solve both is to use the v2 docker registry, which accepts schema 2, in most of those tests (e.g. those which deal with signature and policy semantics); then we can use an ordinary non-manifest-list image like busybox for them, and then add a separate set of tests for OpenShift which focus primarily on the copying of data in and out in various schemas etc.

We’ll see which one of us will run into these test issues harder/sooner and actually update the tests that way 😀

runcom · 2016-10-04T11:21:03Z

We’ll see which one of us will run into these test issues harder/sooner and actually update the tests that way

I have that on my todo list since this stuff were in skopeo :( I'll try to find some time to really do that I promise :(

runcom · 2016-10-04T11:29:12Z

@mtrmac updated the code, I still have to write down doc for GetTargetManifest and fix the other image transports, I'd like your ack before moving on :P

mtrmac · 2016-10-04T14:38:01Z

Make copy.Image fail if IsMultiImage, and consider this ACKed.

runcom · 2016-10-06T13:16:31Z

@mtrmac PTAL

mtrmac

ACK apart from the two minor points.

mtrmac · 2016-10-06T14:23:14Z

image/docker_list.go

+}
+
+// A ManifestDescriptor references a platform-specific manifest.
+type ManifestDescriptor struct {


This can be a private type

mtrmac · 2016-10-06T14:23:21Z

image/docker_list.go

+}
+
+type manifestList struct {
+	src           types.ImageSource


Unused AFAICS.

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

runcom · 2016-10-06T14:28:33Z

@mtrmac fixed those 2 minor points

mtrmac · 2016-10-06T14:35:25Z

👍, thanks!

runcom · 2016-10-06T14:36:05Z

lgtm (merging after tests)

runcom · 2016-10-06T15:19:16Z

merging, I'll fix skopeo asap and use a not-manifest-list image (like the official busybox)

mtrmac · 2016-10-06T15:38:22Z

merging, I'll fix skopeo asap and use a not-manifest-list image (like the official busybox)

With these non-trivial test failures, I would really prefer for the test fix PR to be ready and reviewed before the containers/image part goes in. Not doing that effectively prevents merging anything else into containers/image.

mtrmac · 2016-10-06T20:05:18Z

copy/copy.go

+		return fmt.Errorf("can not copy %s: manifest contains multiple images", transports.ImageName(srcRef))
+	}
+
 	// Please keep this policy check BEFORE reading any other information about the image.


Heh, right below the added code is this comment and neither of as has noticed :) So much for comments.

I’ll fix that…

Signed-off-by: Miloslav Trmač <[email protected]>

runcom force-pushed the manifeset-list branch from 55a12ce to d3e0f6a Compare October 1, 2016 18:08

runcom force-pushed the manifeset-list branch from d3e0f6a to 1b5410c Compare October 3, 2016 10:32

runcom changed the title ~~manifest: enable DockerV2List~~ WIP: manifest: enable DockerV2List Oct 3, 2016

runcom mentioned this pull request Oct 3, 2016

Manifest lists break signatures verification? #114

Closed

mtrmac reviewed Oct 3, 2016

View reviewed changes

runcom force-pushed the manifeset-list branch from 1b5410c to f7e97bd Compare October 4, 2016 11:25

runcom force-pushed the manifeset-list branch from f7e97bd to af29222 Compare October 6, 2016 13:15

runcom changed the title ~~WIP: manifest: enable DockerV2List~~ manifest: enable DockerV2List Oct 6, 2016

mtrmac reviewed Oct 6, 2016

View reviewed changes

runcom force-pushed the manifeset-list branch from af29222 to 1544909 Compare October 6, 2016 14:25

manifest: enable DockerV2List

9810790

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

runcom force-pushed the manifeset-list branch from 1544909 to 9810790 Compare October 6, 2016 14:28

runcom merged commit 3c70e7a into containers:master Oct 6, 2016

runcom deleted the manifeset-list branch October 6, 2016 15:19

runcom mentioned this pull request Oct 6, 2016

vendor containers/image containers/skopeo#226

Merged

mtrmac reviewed Oct 6, 2016

View reviewed changes

mtrmac added a commit to mtrmac/image that referenced this pull request Oct 6, 2016

DO NOT MERGE: Deactivate containers#115 to allow testing

364c110

Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac mentioned this pull request Oct 6, 2016

Refactor c/i/image #122

Merged

mtrmac added a commit to mtrmac/image that referenced this pull request Oct 7, 2016

DO NOT MERGE: Deactivate containers#115 to allow testing

da0b11b

Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac mentioned this pull request Oct 7, 2016

types,docker: add DockerAuthConfig #113

Merged

mtrmac mentioned this pull request Oct 31, 2016

[RFC] types: ImageSource: support getting a specific manifest #25

Closed

mtrmac mentioned this pull request Sep 13, 2017

Hotfix: Do not fetch manifest lists from Docker registries #344

Merged

manifest: enable DockerV2List #115

manifest: enable DockerV2List #115

Uh oh!

Conversation

runcom commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Oct 2, 2016

Uh oh!

runcom commented Oct 2, 2016

Uh oh!

runcom commented Oct 2, 2016

Uh oh!

runcom commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 3, 2016 • 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!

mtrmac Oct 3, 2016

Choose a reason for hiding this comment

Uh oh!

runcom Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 3, 2016 • 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!

mtrmac commented Oct 3, 2016

Uh oh!

mtrmac commented Oct 3, 2016

Uh oh!

runcom commented Oct 4, 2016

Uh oh!

runcom commented Oct 4, 2016

Uh oh!

mtrmac commented Oct 4, 2016

Uh oh!

runcom commented Oct 6, 2016

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

runcom commented Oct 6, 2016

Uh oh!

mtrmac commented Oct 6, 2016 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Oct 6, 2016

Uh oh!

mtrmac commented Oct 6, 2016

Uh oh!

mtrmac Oct 6, 2016 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

runcom commented Oct 1, 2016 •

edited

Loading

runcom commented Oct 3, 2016 •

edited

Loading

mtrmac Oct 3, 2016 •

edited

Loading

mtrmac Oct 3, 2016 •

edited

Loading

mtrmac commented Oct 6, 2016 •

edited by runcom

Loading

runcom commented Oct 6, 2016 •

edited

Loading

mtrmac Oct 6, 2016 •

edited

Loading