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

docker pull is more restrictive now w.r.t. MediaTypes #30083

Closed
duglin opened this issue Jan 12, 2017 · 12 comments
Closed

docker pull is more restrictive now w.r.t. MediaTypes #30083

duglin opened this issue Jan 12, 2017 · 12 comments

Comments

@duglin
Copy link
Contributor

@duglin duglin commented Jan 12, 2017

Using docker v1.14.0 (master) I see this:

$ docker pull gcr.io/kubernetes-helm/tiller:v2.1.3
Error response from daemon: target is unknown

After some digging, this error is coming from:
https://github.com/docker/docker/blob/master/distribution/pull_v2.go#L366

		if !allowedMediatype {
			configClass := mediaTypeClasses[m.Manifest.Config.MediaType]
			if configClass == "" {
				configClass = "unknown"
			}
			return false, fmt.Errorf("target is %s", configClass)
		}

And the MediaType being returned from gcr.io is "text/html".

I believe in older version of this code we never checked the mediaType to see if its valid and that's why this works on older Dockers. I'm in talks with the gcr.io guys to see if they can fix their code so that they return a valid value, but in the meantime we may want to consider being more forgiving in our code to allow for non-compliant registries.

Or, in @stevvooe's words:
we should probably have a set of media types that revert to some default (like schema2)

@justincormack
Copy link
Contributor

@justincormack justincormack commented Jan 12, 2017

I presume this was changed when v2 plugins (with a different media type) were introduced.

@duglin
Copy link
Contributor Author

@duglin duglin commented Jan 12, 2017

if I did my git commands right, I think the PR that introduced it was: #29339

@waghpooja
Copy link

@waghpooja waghpooja commented Jan 17, 2017

docker versions 1.10 onwards and < 1.11.1 pushed schema2 manifests with config text/html as per #22378
So this change in 1.14.x will cause pull issues for images pushed with those versions.

@dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Jan 17, 2017

It certainly was my intent to handle former bugs and treat them as the known image type. We knew about this https://github.com/docker/docker/pull/29339/files#diff-8448734ad4bcad711a430d27d303ac16R26 and I am not against adding text/html with a comment linking to #22378. I stand by making the check more restrictive though as it allows us expand the set of media types without having to worry about breaking older clients which may treat them as an image with unknown results.

@hjacobs
Copy link

@hjacobs hjacobs commented Jan 19, 2017

This strict check also breaks our Pier One Docker registry: zalando-stups/pierone#130

We will probably do a quick hack to return whatever the Docker client now expects 😞

@dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Jan 19, 2017

@hjacobs do you have a sample manifest from one of these registries so we can see which media type it has in the manifests? If you don't want to share the whole manifest, just the media type of the config will do.

@hjacobs
Copy link

@hjacobs hjacobs commented Jan 19, 2017

@dmcgowan our Open Source registry is open, so you can see yourself: https://registry.opensource.zalan.do/v2/stups/openjdk/manifests/8-54

@hjacobs
Copy link

@hjacobs hjacobs commented Jan 19, 2017

It still works/worked with rc4 btw, so I downgraded now: sudo apt-get install docker-engine=1.13.0~rc4-0~ubuntu-xenial

aaronlehmann added a commit to aaronlehmann/docker that referenced this issue Jan 19, 2017
As noted by moby#30083, the new strict checking of mediatypes misses some
cases where earlier bugs caused nonstandard mediatypes to be stored in
manifests. Two of the known cases are text/html and application/json,
which were returned by certain registries and stored by earlier versions
of Docker. Add special cases for text/html and application/json.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
vieux added a commit to vieux/docker that referenced this issue Jan 25, 2017
As noted by moby#30083, the new strict checking of mediatypes misses some
cases where earlier bugs caused nonstandard mediatypes to be stored in
manifests. Two of the known cases are text/html and application/json,
which were returned by certain registries and stored by earlier versions
of Docker. Add special cases for text/html and application/json.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
(cherry picked from commit a215e15)
Signed-off-by: Victor Vieux <vieux@docker.com>
openstack-gerrit pushed a commit to openstack-archive/kolla-kubernetes that referenced this issue Jan 26, 2017
ATT-Comdev do not currently have an image with rbd for K8s 1.5.2, so
until we can move to our own images we should pin the version of K8s at 1.5.1

Also remove workaround for pulling images from gcr.io with docker-engine.
See: moby/moby#30083

This commit should not be merged until this is resolved in Halcyon.
See: att-comdev/halcyon-kubernetes#42

Change-Id: I85c7aa7ba6067ac53013f373dda08b3fac06625d
@uggedal
Copy link
Contributor

@uggedal uggedal commented Jan 26, 2017

This issue is present in 1.13.0 final as well.

@hjacobs
Copy link

@hjacobs hjacobs commented Jan 26, 2017

We fixed it (workaround!) in our Pier One Docker registry by rewriting the manifest on the fly: zalando-stups/pierone#131

@stevvooe
Copy link
Contributor

@stevvooe stevvooe commented Jan 26, 2017

We fixed it (workaround!) in our Pier One Docker registry by rewriting the manifest on the fly: zalando-stups/pierone#131

This might break content verification. Are you just setting the content type or are you tampering with the content? If you are tampering with the content, fetch by digest will break.

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jan 27, 2017

This is fixed by #30262, which will be in 1.13.1, so let me go ahead and close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

9 participants