[Enhancement] Follow Docker's image nameing convention in k3d image import (#653, @cimnine)

[cimnine]

What

Once upon a time, all Docker images were stored either locally or on Docker Hub. They were called repoName/imageName:versionName and all was well.

Then people started to build other registries. Eventually it was decided that the registry will just be prefixed to the image name: registry-name.example/repoName/imageName:versionName. But to maintain compatibility, images without a registry prefix were still fetched from Docker Hub, the default registry.

The Docker Hub registry eventually was named docker.io and images that were tagged docker.io/repoName/imageName:versionName were equivalent to images tagged as repoName/imageName:versionName.

Probably on a different occasion and for different reaons, latest would be assumed by Docker if no versionName part was given.

These two conventions are what this PR adds to k3d for the image import command.

• That images tagged as docker.io/repoName/imageName:versionName can be imported to a cluster, i.e. that k3d image import 'docker.io/repoName/imageName:versionName' just works. Because currently it fails, whereas k3d image import 'repoName/imageName:versionName' does not. And neither does k3d image import 'any-other.registry/repoName/imageName:versionName'.
• That images without a :versionName have :latest added automatically.

(Side Note: The story about how docker.io came to be the default registry prefix is most probably not historically accurate. If it were, it would be so by pure luck.)

Why

It is not immediately obvious for the user – and neither is it documented – that k3d is more strict regarding to how it treats image tags than Docker is.
It's especially confusing if an image was previously built locally with docker build -t 'docker.io/repoName/imageName:tagName', but the same image can later not be imported by that same name.

Implications

I'm not aware of any obvious implications. It allows behaviour that IMO could be expected but that is currently resulting in an error.

I would not consider this change to be a breaking change.

[iwilltry42]

Hi @cimnine , thanks for opening this PR!
Beautiful story 😉

It is not immediately obvious for the user – and neither is it documented – that k3d is more strict regarding to how it treats image tags than Docker is.

That's because there is absolutely no effort in being anything more strict about this than docker.
In fact, k3d is even doing the exact same thing as the official docker CLI.
Compare https://github.com/docker/cli/blob/master/cli/command/image/save.go#L39-L61 with https://github.com/rancher/k3d/blob/main/tools/cmd/image.go#L15-L26 .
The issue is actually within how k3d checks from where to get the image (local tar or runtime), where we check the available lists of images and do a comparison.
The reason for this on the other hand is, that we try not to have to do any kind of error handling or log feedback in the tools container (i.e. we have to make sure beforehand, that the image saving will work).

The logic that you're adding makes total sense regarding this unintentional "strictness" 👍

Thanks for the contribution already! :)

(just two small change requests)

[cimnine]

Thank you for the appreciation. I've made some changes upon your feedback. Do you want me to squash the commits before merging?

[iwilltry42]

Wouldn't this bring the same result (last component contains a :) (not tested)?

Suggested change

	// e.g. imageName has no colon -> false
	// e.g. repoName/imageName has no colon -> false
	// e.g. registry/repoName/imageName has no colon -> false
	// e.g. registry:1234/repoName/imageName has colon, but before the slash -> false
	// e.g. imageName:versionName has colon -> true
	// e.g. repoName/imageName:versionName has colon after slash -> true
	// e.g. registry/repoName/imageName:versionName has colon after slash -> true
	// e.g. registry:1234/repoName/imageName:versionName has colon after slash -> false
	if !strings.Contains(imageTag, ":") {
	return false
	}
	if !strings.Contains(imageTag, "/") {
	// happens if someone refers to a library image by just it's imageName (e.g. `postgres` instead of `library/postgres`)
	return strings.Contains(imageTag, ":")
	}
	indexOfSlash := strings.Index(imageTag, "/") // can't be -1 because the existence of a '/' is ensured above
	substringAfterSlash := imageTag[indexOfSlash:]
	return strings.Contains(substringAfterSlash, ":")
	imageSplit := strings.Split(imageTag, "/")
	retrun strings.Contains(imageSplit[len(imageSplit)-1], ":")

Only case in which both would "break" is if someone only provides the registry hostname + port without an image name, but that's simple human error that we shouldn't need to account for.

[cimnine]

It's true, that this could be shortened like this. The first two if's are just shortcuts. And to me, they're also mental shortcuts. I.e. if the whole string does not have a colon, well then why look any further? From then on, everything else depends on the position of the slash, so if there's not even a slash, again, why look further?

But everything could be done by just looking at the result of Index(imageTag, "/") and proceeding from there.

Personnally, I would refrain from using Split. As we just need to ensure we're not looking into the hostname, it's only the first slash that matters. So Index(imageTag, "/") and then shortening the string to imageTag[indexOfSlash:] would do the job perfectly, no need to split all the other parts.

[iwilltry42]

@cimnine

I almost suspected this. I've now re-written this part and took the liberty to extract some of the code I've touched into it's own functions. I believe this also makes the code easier to read.

Please always feel free to simplify the code :)

I think that removing "cruft" output by the runtime when listing images, as you were doing it before, is easier than trying to make the user input match with what's given back from the runtime? 🤔

[cimnine]

I think that removing "cruft" output by the runtime when listing images, as you were doing it before, is easier than trying to make the user input match with what's given back from the runtime?

Given all the edge cases you've mentioned, I see this now. How do you like my current attemp?

[iwilltry42]

This looks way cleaner, thank you! :)