Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

image: implement Docker v1 -> OCI conversion #380

Merged
merged 3 commits into from
Nov 15, 2017
Merged

image: implement Docker v1 -> OCI conversion #380

merged 3 commits into from
Nov 15, 2017

Conversation

jonboulle
Copy link
Contributor

Since we already know how to convert from Docker V1 to V2 and from
Docker V2 to OCI, we simply do a transitive manifest conversion.

Signed-off-by: Jonathan Boulle jonathanboulle@gmail.com

@jonboulle
Copy link
Contributor Author

jonboulle commented Nov 10, 2017 •
edited
Loading

This partially fixes containers/skopeo#290 (which I believe was erroneously closed):

# before (master)
; ./skopeo.old   copy docker://quay.io/coreos/etcd:latest oci:etcd:latest
Getting image source signatures
<...>
FATA[0002] 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 

# after (this patch)
; ./skopeo   copy docker://quay.io/coreos/etcd:latest oci:etcd:latest
Getting image source signatures
<...>
Copying config sha256:bd491080a0ef5d6d8da630b6d94de44b5a2b26039ea9a5c3fb15ebf3a169734b
 1.49 KB / 1.49 KB [========================================================] 0s
Writing manifest to image destination
Storing signatures
; jq < etcd/index.json 
{
  "schemaVersion": 2,
  "manifests": [
    {
      "mediaType": "application/vnd.oci.image.manifest.v1+json",
      "digest": "sha256:684e5dfd93467d9227ed217d5901ae2ff70c93095cf62b079e27e07c723e484e",
      "size": 1127,
      "annotations": {
        "org.opencontainers.image.ref.name": "latest"
      },
      "platform": {
        "architecture": "amd64",
        "os": "linux"
      }
    }
  ]
}
=====================] 0s
Writing manifest to image destination
Storing signatures

@jonboulle jonboulle mentioned this pull request Nov 10, 2017
Closed
mtrmac
mtrmac reviewed Nov 13, 2017
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

(It would be nice to have at least a smoke test to ensure this keeps working, the manifest conversions are more or less the trickiest code in the repo. Still, non-blocking, better to have an untested conversion than no conversion.)

image/docker_schema1.go Outdated
@@ -343,8 +354,8 @@ func (m *manifestSchema1) convertToManifestSchema2(uploadedLayerInfos []types.Bl
Digest: digest.FromBytes(configJSON),
}

m2 := manifestSchema2FromComponents(configDescriptor, nil, configJSON, layers)
return memoryImageFromManifest(m2), nil
m2 := manifestSchema2FromComponents(configDescriptor, nil, configJSON, layers).(*manifestSchema2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redeclare manifestSchema2FromComponents to return a *manifestSchema2 explicitly instead of making a runtime cast here.

Copy link
Collaborator

@mtrmac mtrmac Nov 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… actually, is either necessary? AFAICT both memoryImageFromManifest and .UpdatedImage() can work on the genericManifest interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, I'm dumb

image/docker_schema1.go Outdated
m2 := manifestSchema2FromComponents(configDescriptor, nil, configJSON, layers)
return memoryImageFromManifest(m2), nil
m2 := manifestSchema2FromComponents(configDescriptor, nil, configJSON, layers).(*manifestSchema2)
return m2, nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(…and then this return could be folded into the previous line. Non-blocking.)

image/docker_schema1.go Outdated
if err != nil {
return nil, err
}
return m2.UpdatedImage(options)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit less risky with &types.ManifestUpdateOptions{ManifestMIMEType:imgspecv1.MediaTypeImageManifest} instead of the full options, to minimize what the m2.UpdatedImage will do.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… also include the original options.InformationOnlymember.

@jonboulle jonboulle mentioned this pull request Nov 15, 2017
Closed
@jonboulle
Copy link
Contributor Author

Thanks for the review, updated to address comments

@mtrmac
Copy link
Collaborator

mtrmac commented Nov 15, 2017 •
edited by runcom
Loading

👍 Thanks!

Approved with PullApprove

@runcom
Copy link
Member

runcom commented Nov 15, 2017 •
edited
Loading

LGTM

Approved with PullApprove

@runcom
Copy link
Member

runcom commented Nov 15, 2017

Please rebase also

@jonboulle
image: implement Docker v1 -> OCI conversion
a0dfca0 
Since we already know how to convert from Docker V1 to V2 and from
Docker V2 to OCI, we simply do a transitive manifest conversion.

Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>
@jonboulle
image/docker: just use genericManifest instead
418a02c 
Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>
@jonboulle
image/docker: only pass through necessary options
4ffee0a 
Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>
@jonboulle
Copy link
Contributor Author

rebased, thanks!

@mtrmac mtrmac merged commit dc98c2a into containers:master Nov 15, 2017
@jonboulle jonboulle mentioned this pull request Dec 19, 2017
Merged
jonboulle added a commit to jonboulle/image that referenced this pull request Feb 12, 2018
@jonboulle
image: fix docker schema v1 -> OCI conversion
b060038 
Chasing containers#380; DiffIDs are required in the OCI format, but we were
skipping them because of this check. Without this, converting from a V1
image registry source to an OCI layout results in broken images.
@jonboulle jonboulle mentioned this pull request Feb 12, 2018
Merged
jonboulle added a commit to jonboulle/image that referenced this pull request Feb 12, 2018
@jonboulle
image: fix docker schema v1 -> OCI conversion
6d6a6ea 
Chasing containers#380; DiffIDs are required in the OCI format, but we were
skipping them because of this check. Without this, converting from a V1
image registry source to an OCI layout results in broken images.

Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrmac mtrmac mtrmac left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonboulle @mtrmac @runcom