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

Default "latest" tag does not work when saving an untagged image #890

Closed
lingsamuel opened this issue Dec 29, 2020 · 5 comments · Fixed by #893
Closed

Default "latest" tag does not work when saving an untagged image #890

lingsamuel opened this issue Dec 29, 2020 · 5 comments · Fixed by #893

Comments

@lingsamuel
Copy link

lingsamuel commented Dec 29, 2020

Save an image using tarball.Write with an empty tag, load will fail.

if tags, ok := imageToTags[img]; ok && tags != nil {
imageToTags[img] = append(tags, tag.String())
} else {
imageToTags[img] = []string{tag.String()}
}

Here use tag.String(), but tag.String() uses original:

func (t Tag) String() string {
return t.original
}

So the default "latest" tag does not work when saving the image.

Sample code:

func main() {
	ref, err := name.ParseReference("nginx")
	if err != nil {
		panic(err)
	}

	img, err := daemon.Image(ref)
	if err != nil {
		panic(err)
	}

	w, err := os.Create("./nginx.img")
	if err != nil {
		panic(err)
	}
	defer w.Close()

	tarball.Write(ref, img, w)
}
$ docker load -i ./nginx.img
invalid tag "nginx"
@lingsamuel lingsamuel changed the title NewTag should use a default "latest" tag Default "latest" tag not work Dec 29, 2020
@lingsamuel lingsamuel changed the title Default "latest" tag not work Default "latest" tag does not work when saving an untagged image Dec 29, 2020
@jonjohnsonjr
Copy link
Collaborator

Interesting. So docker save writes this out as nginx:latest, with no hostname. For #527 we stripped both the hostname defaulting and the tag defaulting to preserve the original input, but docker won't load images without a tag specified.

If we just call tag.Name() here, that would break #527, so perhaps we can just do some hacky work in tarball to see if tag.String() contains : and replace it with tag.String()+":"+tag.Identifier() if it doesn't...

I'm not sure if this would break anything -- @smukherj1 do you remember this at all? You seemed only concerned with hostnames in that issue, not tags, wdyt?

@lingsamuel
Copy link
Author

@jonjohnsonjr yes, docker save writes nginx:latest, xref kubernetes/minikube#10047 (comment)

@afbjorklund
Copy link
Contributor

afbjorklund commented Dec 30, 2020

And of course podman save uses "docker.io/library/nginx:latest", so everyone is different...

Docker will happily load it, but then lie about the image name: Loaded image: nginx:latest
As opposed to when loading in Podman: Loaded image(s): docker.io/library/nginx:latest

If trying to use crane pull (i.e. above) and then podman, you get localhost/nginx:latest
That is: podman will add a default "localhost" registry to all the images without one present.

This breaks all kinds of caching and expectations, when trying to move beyond Docker...
(with the deprecation of Docker, we want to stay agnostic between container runtimes)


The workaround for minikube, is to add explicit "docker.io/" and ":latest" to everything. 😔

There are even more elaborate workarounds for this, like the "short names" in podman v3:
https://www.redhat.com/sysadmin/container-image-short-names

And we still end up with multiple "latest", so now we need pull policies on when to update.

The previous experiment of trying to use digests, worked even worse (forking* this library).
Trying to use Id made some assumptions about "compability" between client and server.

* See #703

All we wanted was a simple way to load images into CRI, after first saving them on the host...
Especially now with the pull limits on docker.io, we want to cache images (~/.minikube/cache).

@jonjohnsonjr
Copy link
Collaborator

@afbjorklund yeah this is generally frustrating... deviating from docker's behavior anywhere (even with the best intentions) leads to sorrow.

If trying to use crane pull (i.e. above) and then podman, you get localhost/nginx:latest
That is: podman will add a default "localhost" registry to all the images without one present.

oof

The workaround for minikube, is to add explicit "docker.io/" and ":latest" to everything. 😔

I think we could match docker's behavior here without much fuss, if that would make things simpler?

And we still end up with multiple "latest", so now we need pull policies on when to update.

Indeed it's terrible :/

The previous experiment of trying to use digests, worked even worse (forking* this library).

Yeah...

Trying to use Id made some assumptions about "compability" between client and server.

Interesting -- I'd love to hear more.

All we wanted was a simple way to load images into CRI, after first saving them on the host...

I think this is achievable, and I have some ideas that might be relevant, but I'd like to be tactical about this in terms of benefit per unit time spent... if we append :latest here, does that help in the short term?

I'd love to chat about a better long-term solution if you're interested, though time zones make that difficult...

@afbjorklund
Copy link
Contributor

afbjorklund commented Dec 30, 2020

All we wanted was a simple way to load images into CRI, after first saving them on the host...

I think this is achievable, and I have some ideas that might be relevant, but I'd like to be tactical about this in terms of benefit per unit time spent... if we append :latest here, does that help in the short term?

Yeah, I think that will fix this particular issue - and then we can have a separate discussion about the rest...

$ docker save busybox > busybox.tar
$ docker load < busybox.tar
Loaded image: busybox:latest
$
$ crane pull busybox busybox.tar
$ docker load -i busybox.tar 
1dad141bdb55: Loading layer [==================================================>]  766.6kB/766.6kB
invalid tag "busybox"

So it needs to provide the default tag (like you said). Then we can handle quirks of other systems separately.

$ podman load -i busybox.tar 
Getting image source signatures
Copying blob 1dad141bdb55 done  
Copying config a77dce18d0 done  
Writing manifest to image destination
Storing signatures
Loaded image(s): localhost/busybox:latest
$ sudo ctr images import busybox.tar 
unpacking docker.io/library/busybox:latest (sha256:5d043540942a67f5b672d6866b3447d2a301744e563f25ce2ce8f4b5b124c0c0)...done

I'd love to chat about a better long-term solution if you're interested, though time zones make that difficult...

I think this will be on the roadmap for next year, trying to introduce CRI as a mandatory component in minikube.
Previously we have tried to make the same with CNI, and support at least three of the major network providers.

This will make it necessary to remove some of the assumptions and hardcodings on Docker, that are there now...
The most likely is that we will keep Docker support, but replace the dockershim with a proper CRI implementation.

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

Successfully merging a pull request may close this issue.

3 participants