Make docker pull detect plugin content and error out.
#29297
Conversation
| @@ -79,9 +79,9 @@ func Push(name string, rs registry.Service, metaHeader http.Header, authConfig * | |||
| return "", err | |||
| } | |||
| f.Close() | |||
| mt := MediaTypeLayer | |||
| mt := schema2.MediaTypeLayer | |||
There was a problem hiding this comment.
Changes to this file are not needed.
| MediaTypeLayer = "application/vnd.docker.image.rootfs.diff.tar.gzip" | ||
| DefaultTag = "latest" | ||
| ) | ||
| // DefaultTag is the default tag for plugins |
There was a problem hiding this comment.
Changes to this file are not needed.
| @@ -356,6 +360,12 @@ func (p *v2Puller) pullV2Tag(ctx context.Context, ref reference.Named) (tagUpdat | |||
| return false, fmt.Errorf("image manifest does not exist for tag or digest %q", tagOrDigest) | |||
| } | |||
|
|
|||
| if m, ok := manifest.(*schema2.DeserializedManifest); ok { | |||
| if m.Manifest.Config.MediaType == "application/vnd.docker.plugin.v0+json" { | |||
There was a problem hiding this comment.
Yes, "application/vnd.docker.plugin.v0+json" is indeed the right mediaType that image pull should check and error out.
There was a problem hiding this comment.
I'm confused about that, because a similar check is being removed in #29230.
There was a problem hiding this comment.
@aaronlehmann :
application/vnd.docker.plugin.v0+json is strictly for plugins pushed by 1.12 experimental.
What's being removed in #29230 is a temporary mediaType (which ofcourse wont be supported) we had for plugins in 1.13. This is now being permanently replaced by application/vnd.docker.plugin.v1+json
There was a problem hiding this comment.
Just confused about why we would add something relating to the temporary media type in a patch release, when references to that media type have been removed from the upcoming version. If application/vnd.docker.plugin.v0+json is indeed rare, shouldn't 1.12.x warn on an attempt to pull a plugin pushed by >= 1.13.x?
There was a problem hiding this comment.
I think you are getting confused between application/vnd.docker.plugin.image.v0+json (temporary) and application/vnd.docker.plugin.v0+json (permanent for 1.12.x).
What's being removed in this patch release PR and the other PR is the temporary mediatype.
There was a problem hiding this comment.
Why only check application/vnd.docker.plugin.v0+json if application/vnd.docker.plugin.v1+json is also a possibility to be encountered? If we are going to make this change I would think we would check for both or do a prefix check for application/vnd.docker.plugin.
There was a problem hiding this comment.
Sounds reasonable to me.
vieux
force-pushed
the
plugins_cherry-picks
branch
2 times, most recently
from
e9a981b to
2b32756
Compare
|
@dmcgowan I updated. |
|
@anusha-ragunathan tested manually works as expected:
|
|
The only problem is that `docker pull vieux/sshfs -> Error response from daemon: target is a plugin - Usedocker plugin install`` is not true. You cannot install a 1.13 plugin using 1.12 experimental and expect it to work. So updating the error message to simply say |
Signed-off-by: Anusha Ragunathan <[email protected]> (cherry picked from commit 9b6dcc8) Signed-off-by: Victor Vieux <[email protected]>
vieux
force-pushed
the
plugins_cherry-picks
branch
from
2b32756 to
39fbd60
Compare
|
@anusha-ragunathan updated. |
|
LGTM (granted tests pass) |
| @@ -143,8 +143,7 @@ func Pull(name string, rs registry.Service, metaheader http.Header, authConfig * | |||
| logrus.Debugf("pull.go: error in json.Unmarshal(): %v", err) | |||
| return nil, err | |||
| } | |||
| if m.Config.MediaType != MediaTypeConfig && | |||
| m.Config.MediaType != "application/vnd.docker.plugin.image.v0+json" { | |||
| if m.Config.MediaType != MediaTypeConfig { | |||
There was a problem hiding this comment.
was application/vnd.docker.plugin.image.v0+json intended as a placeholder for application/vnd.docker.plugin.v1+json? What happens when application/vnd.docker.plugin.v1+json is encountered?
There was a problem hiding this comment.
Yes, application/vnd.docker.plugin.image.v0+json was a placeholder for the final type that 1.13 would ship with.
If 1.12.x encounters a plugin with mediatype application/vnd.docker.plugin.v1+json, it implies that the plugin was created using 1.13+. Hence it is not supported in 1.12.x
There was a problem hiding this comment.
Why remove it then and not just update it to application/vnd.docker.plugin.v1+json
There was a problem hiding this comment.
thanks @anusha-ragunathan for explaining, the MediaTypeConfig here is application/vnd.docker.plugin.v0+json and if anything else is encountered this condition is hit, including the 1.13 type 😄
|
LGTM |
ping @anusha-ragunathan
can you make sure I used the right media type ?