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.

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

manifest subcommand creates invalid manifest list #1135

Closed
dmcgowan opened this issue Jun 20, 2018 · 7 comments

Comments

Projects
None yet
7 participants
@dmcgowan
Copy link
Member

commented Jun 20, 2018

Currently creating manifest lists from the experimental manifest subcommand uses an incorrect for the manifest, creating invalid manifests. These manifests fail to be pullable with containerd since containerd validates the size. This has lead to broken images being pushed to registries.

See conversation from containerd/containerd#2401

@tonistiigi

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

From looking at the code the issue seems to be that the original manifest is not stored in the original form and instead it is reserialized inside a new json object ImageManifest. Even though it uses Payload() function and variables suggests the size is taken over raw data it has actually been remarshalled in the disk already and the original form may be lost.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

ping @clnperez @estesp PTAL

@clnperez

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

Odd. There was an issue (that I can't find now) about this. And I went to a lot of trouble to get it back to looking exactly the way it looked originally. The issue had to do with the manifest changing (just the tabs in it), and so the hash was different. If anyone has that in their history or inbox please link.

@dmcgowan what version of the cli do you have?

@clnperez

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2018

Ah, hidden in collapsed history. Starting here: hinshun commented on Nov 2, 2017

So it should be fixed in the original, but is not, apparently. I'll take a look.

@vielmetti

This comment has been minimized.

Copy link

commented Jun 22, 2018

Downstream impact includes a broken CoreDNS for Kubernetes release at kubernetes/kubernetes#65253

which is marked as priority/critical-urgent in that repo.

@flx42

This comment has been minimized.

Copy link

commented Jun 22, 2018

Answering containerd/containerd#2401 (comment)

@tonistiigi so you're right if you restrict yourself exclusively to code comments of the exported functions/fields. But then, only 3 lines below, this can be confusing:
https://github.com/docker/distribution/blob/749f6afb4572201e3c37325d0ffedb6f32be8950/manifest/schema2/manifest.go#L92-L93

Anyway, the current manifest code seems to need the unmarshalled schema2.DeserializedManifest, in order to perform the blob mount requests to the registry. So carrying an immutable blob of the struct + the struct would be redundant with what's already present indocker/distribution.

@luxas

This comment has been minimized.

Copy link

commented Jun 25, 2018

cc @dims @mkumatag

flx42 added a commit to flx42/cli that referenced this issue Jun 25, 2018

Store the V2 manifest as an opaque blob
Fixes: docker#1135
Tested-by: Christy Norman <christy@linux.vnet.ibm.com>
Signed-off-by: Felix Abecassis <fabecassis@nvidia.com>

flx42 added a commit to flx42/cli that referenced this issue Jun 25, 2018

Store the V2 manifest as an opaque blob
Fixes: docker#1135
Tested-by: Christy Norman <christy@linux.vnet.ibm.com>
Signed-off-by: Felix Abecassis <fabecassis@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.