integration: add OCI <-> Docker roundtrip tests #306

cyphar · 2017-02-15T14:27:48Z

Signed-off-by: Aleksa Sarai [email protected]

runcom · 2017-02-15T14:29:19Z

👍

awesome, thanks @cyphar

cyphar · 2017-02-15T14:31:25Z

Wait, wait. I pushed this so I can test it because Docker takes forever here. 😉

runcom · 2017-02-15T14:32:45Z

Wait, wait. I pushed this so I can test it because Docker takes forever here.

yeah, it was an OK from the on the intention 😄

cyphar · 2017-02-15T15:27:51Z

Grrr. Integration tests are failing because we don't have docker-archive. Maybe if I implement docker:// -> oci: conversion this will become easier?

runcom · 2017-02-15T15:29:01Z

Maybe if I implement docker:// -> oci: conversion this will become easier?

don't we have that already? 😕

cyphar · 2017-02-15T15:30:26Z

Actually, we do... I'm confused why I'm getting this error:

time="2017-02-15T15:26:41Z" level=fatal msg="Error creating an updated image manifest: Conversion of image manifest from application/vnd.docker.distribution.manifest.v1+prettyjws to application/vnd.oci.image.manifest.v1+json is not implemented"

What is ourRegistry running and why the hell is it broken?

runcom · 2017-02-15T15:34:19Z

integration/copy_test.go

+	defer os.RemoveAll(oci2)
+
+	// Docker -> OCI
+	assertSkopeoSucceeds(c, "", "--tls-verify=false", "--debug", "--remove-signatures", "copy", "docker://estesp/busybox:amd64", "oci:"+oci1+":latest")


just use busybox:latest - the amd64 tag is v2s1 only and v2s1->oci isn't implemented.

runcom · 2017-02-15T16:21:45Z

@cyphar you have some flags-related issues in the tests and also, please take into account my comment #306 (comment) about not using that busybox tag

cyphar · 2017-02-15T16:41:32Z

@runcom

time="2017-02-15T16:40:37Z" level=fatal msg="can not copy docker://estesp/busybox:latest: manifest contains multiple images"

😮

runcom · 2017-02-15T16:43:58Z

@cyphar I'm sorry I meant the official busybox:latest image not estesp/busybox:latest

mtrmac

Thanks! This is fine and useful as is, the possible expansion of the test is just something to consider, definitely not a blocker.

mtrmac · 2017-02-15T17:56:57Z

integration/copy_test.go

+	// Docker -> OCI
+	assertSkopeoSucceeds(c, "", "--tls-verify=false", "--debug", "copy", ourRegistry+"busybox:oci_copy", "oci:"+oci2+":another-tag")
+	// OCI -> Docker
+	assertSkopeoSucceeds(c, "", "--tls-verify=false", "--debug", "copy", "oci:"+oci2+":another-tag", ourRegistry+"busybox:oci_copy2")


Is there an obvious kind of comparison that can be done in here to make sure that we do not only don’t report any error, but that we actually copy and convert the images succesfully?

Perhaps check that oci1’s and busybix_oci_copy2 list of layer digests is exactly the same? That the config is identical between oci and oci2, between the two ourRegistry/busybox copies?

Yeah, I was thinking of doing that. First I'm just trying to get this to pass (I'm getting weird registry errors). Still working on it. And I can't run it locally because Docker appears to be broken on my machine.

mtrmac · 2017-02-15T18:00:36Z

👍 assuming the tests pass, it is not immediately obvious to me why they are / have been failing.

runcom · 2017-02-15T18:31:36Z

There seems to be another issue

cyphar · 2017-02-16T09:44:59Z

After running this locally, it looks like this is actually another issue with manifest conversion:

ERRO[0129] response completed with error                 err.code=manifest unknown err.detail=errors verifying manifest: unrecognized manifest content type  err.message=manifest unknown go.version=go1.7.5 http.request.host=localhost:5555 http.request.id=3c0d5fa4-9df2-4d45-8
d85-a2e81d3a69fe http.request.method=GET http.request.remoteaddr=127.0.0.1:58722 http.request.uri=/v2/busybox/manifests/latest http.request.useragent=Go-http-client/1.1 http.response.contenttype=application/json; charset=utf-8 http.response.duration=1.544431ms http.response
.status=404 http.response.written=84 instance.id=2334cfb3-48f9-4a5d-8692-0cbbc26c1cf8 vars.name=busybox vars.reference=latest version=v2.1.0+unknown
127.0.0.1 - - [16/Feb/2017:09:42:16 +0000] "GET /v2/busybox/manifests/latest HTTP/1.1" 404 84 "" "Go-http-client/1.1"

Which basically ends up boiling down to "we didn't change the manifest type when converting to Docker distribution from OCI":

%  cat /tmp/registry/docker/registry/v2/blobs/sha256/56/560799532c865dc38f12595b8a7eb8b602c9fcb608df90abe89499e6a0135483/data
{"schemaVersion":2,"config":{"mediaType":"application/vnd.oci.image.config.v1+json","size":575,"digest":"sha256:34166908e15c5badddab5191bca8bd0e4f429aeeecfba5ac37b387d9bb57b65b"},"layers":[{"mediaType":"application/vnd.oci.image.layer.v1.tar+gzip","size":677628,"digest":"sha256:4b0bc1c4050b03c95ef2a8e36e25feac42fd31283e8c30b3ee5df6b043155d3c"}]}

cyphar · 2017-02-16T09:45:28Z

So the plus side is that this test is already finding bugs. The downside is that this is a bug in the first place.

cyphar · 2017-02-16T10:09:08Z

Alright, this is actually caused by the same issue as containers/image#237. So it has been fixed in containers/image.

runcom · 2017-02-16T10:12:25Z

Alright, this is actually caused by the same issue as containers/image#237. So it has been fixed in containers/image.

can you revendor as part of this PR?

This includes fixes required to add OCI roundtrip integration tests (namely f9214e1d9d5d ("oci: remove MIME type autodetection")). Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2017-02-16T10:36:36Z

@runcom Doing it as we speak. It required re-vendoring other things. Oh, and I'm also adding image-tools validation so we can be even more sure about the validity of the output.

runcom · 2017-02-16T10:54:35Z

@cyphar you may need to revendor the image-spec as well https://travis-ci.org/projectatomic/skopeo/builds/202194334#L1698

cyphar · 2017-02-16T10:56:29Z

@runcom Trust me, it's much more than that. Wait for the commit message, I've had to revendor at least 7 projects.

runcom · 2017-02-16T11:26:34Z

@runcom Trust me, it's much more than that. Wait for the commit message, I've had to revendor at least 7 projects.

I see, ping me when done pls, I want this merged asap

This test is just a general smoke test to make sure there are no errors with skopeo, but also verifying that after passing through several translation steps an OCI image will remain in fully working order. Signed-off-by: Aleksa Sarai <[email protected]>

In order to make sure that we don't create invalid OCI images that are consistently invalid, add additional checks to ensure that both of the generated OCI images in the round-trip test are valid according to the upstream validator. This commit vendors the following packages (deep breath): * oci/image-tools@7575a093636327fc41bdb043a581d04b78cae233, which requires * oci/[email protected] [revendor, but is technically an update because I couldn't figure out what version was vendored last time] * oci/[email protected] * xeipuuv/gojsonschema@6b67b3f * xeipuuv/gojsonreference@e02fc20 * xeipuuv/gojsonpointer@e0fe6f6 * go4org/go4@7ce08ca Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2017-02-16T11:51:20Z

@runcom There, done. PTAL.

cyphar · 2017-02-16T12:01:40Z

The "warnings" in the log are because opencontainers/image-spec#548 (or opencontainers/image-spec#560) hasn't been merged yet. But the actual validation is working as expected, and this is similar validation to the kind I'm doing in umoci (though in umoci it's much more extreme).

cyphar · 2017-02-16T12:07:26Z

I will be adding many more tests once docker-archive is merged. This is just a stop-gap to guarantee things don't break in the meantime.

cyphar · 2017-02-16T12:15:34Z

❤️

cyphar force-pushed the oci-roundtrip-tests branch 2 times, most recently from 542242c to 6fb41d2 Compare February 15, 2017 15:11

cyphar force-pushed the oci-roundtrip-tests branch from 6fb41d2 to 1aab322 Compare February 15, 2017 15:33

runcom reviewed Feb 15, 2017

View reviewed changes

cyphar force-pushed the oci-roundtrip-tests branch 2 times, most recently from 923cd6d to 0af8ec3 Compare February 15, 2017 16:25

cyphar force-pushed the oci-roundtrip-tests branch from 0af8ec3 to 92be06e Compare February 15, 2017 17:38

cyphar mentioned this pull request Feb 15, 2017

integration: add test for OCI <-> docker-archive translation #280

Closed

mtrmac approved these changes Feb 15, 2017

View reviewed changes

cyphar force-pushed the oci-roundtrip-tests branch from 92be06e to fe6ac38 Compare February 15, 2017 17:58

cyphar force-pushed the oci-roundtrip-tests branch from fe6ac38 to 8feae02 Compare February 15, 2017 18:13

vendor: update c/image@a074c669cf383ef8c6b61ccbb850a3b5c3d3dd26

79ef111

This includes fixes required to add OCI roundtrip integration tests (namely f9214e1d9d5d ("oci: remove MIME type autodetection")). Signed-off-by: Aleksa Sarai <[email protected]>

cyphar force-pushed the oci-roundtrip-tests branch from 8feae02 to 3a7beeb Compare February 16, 2017 10:27

cyphar added 2 commits February 16, 2017 22:48

cyphar force-pushed the oci-roundtrip-tests branch from 3a7beeb to eea384c Compare February 16, 2017 11:51

runcom merged commit 226dc99 into containers:master Feb 16, 2017

cyphar deleted the oci-roundtrip-tests branch February 16, 2017 12:15

cyphar mentioned this pull request Feb 16, 2017

OCI sources give wrong mediatype #305

Closed

cyphar mentioned this pull request Oct 8, 2017

skopeo copy does not merge OCI indexes #405

Closed

github-actions bot added the locked - please file new issue/PR label Oct 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2023

integration: add OCI <-> Docker roundtrip tests #306

integration: add OCI <-> Docker roundtrip tests #306

Uh oh!

Conversation

cyphar commented Feb 15, 2017

Uh oh!

runcom commented Feb 15, 2017

Uh oh!

cyphar commented Feb 15, 2017

Uh oh!

runcom commented Feb 15, 2017

Uh oh!

cyphar commented Feb 15, 2017

Uh oh!

runcom commented Feb 15, 2017

Uh oh!

cyphar commented Feb 15, 2017

Uh oh!

runcom Feb 15, 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!

runcom commented Feb 15, 2017

Uh oh!

cyphar commented Feb 15, 2017

Uh oh!

runcom commented Feb 15, 2017 • 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 Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

cyphar Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Feb 15, 2017

Uh oh!

runcom commented Feb 15, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

runcom commented Feb 16, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

runcom commented Feb 16, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

runcom commented Feb 16, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

cyphar commented Feb 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

runcom Feb 15, 2017 •

edited

Loading

runcom commented Feb 15, 2017 •

edited

Loading