c8d/list: Fix Repo(Digests|Tags) for untagged images by vvoland · Pull Request #44840 · moby/moby

vvoland · 2023-01-18T09:29:04Z

Upstreams:

c8d/list: Make dangling images parseable by the cli rumpl/moby#126

Make images without tag show correctly in the docker image ls output.

Before:

$ docker pull docker.io/library/busybox@sha256:7b3ccabffc97de872a30dfd234fd972a66d247c8cfc69b0550f276481852627c
$ docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED          SIZE
$

After:

$ docker pull docker.io/library/busybox@sha256:7b3ccabffc97de872a30dfd234fd972a66d247c8cfc69b0550f276481852627c
$ docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED          SIZE
busybox      <none>    7b3ccabffc97   1 second ago     2.01MB
$

Signed-off-by: Paweł Gronowski [email protected]

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah

Left a question (and a note); also curious if this addresses any of the TODO's on the function (haven't looked closely); https://github.com/moby/moby/blob/master/daemon/containerd/image_list.go#L24-L28

thaJeztah · 2023-01-23T13:56:42Z

+		ref, err := reference.ParseNamed(img.Name())
+		if err == nil {


Do we know in what case this could produce an error? (i.e., how we can end up with an image with an invalid name?)

IIUC, previously, we would show img.Name() (even if that could theoretically be an invalid name); in the new situation we replace that with <none>:<none> (discarding the name altogether). I'm curious in what situations that would happen (as it means we're loosing information about the image in the response).

IMO, there are 2 scenarios:

User fiddles with the moby containerd namespace directly and creates an image with name that's not a valid reference.

We make some bug/error which creates image with bad name.

In both cases, I don't think we can guarantee that referencing these images by their name will succeed (as we do reference.ParseNormalizedName all over the place), so not presenting these names seems reasonable to me.

User fiddles with the moby containerd namespace directly and creates an image with name that's not a valid reference.

First of all, not sure if that's a scenario we must "ignore". Does containerd accept image names that we do not accept valid? (e.g. can I create an image in containerd that's named 🐳😄🐱 ?)

Second: if this happens, the current code would silently ignore this case

And (finally): if this happens: how can I remove said image? (if none of our code is able to handle the invalid name, then A) docker image rm wouldn't be able to delete it?, and B) this output does not allow me to "discover" the invalid name (because we hide it), which does not allow the user to correct their "fiddling" by removing it from containerd (as they wouldn't know what <none>:<none> image this is).

I think currently it could even mean that an image ends up with multiple <none>:<none> tags (one for each invalid image name). Which, thinking of that scenario, could be problematic on other ways (it's not dangling, as we only consider dangling if there's a single <none>:<none> reference)

We make some bug/error which creates image with bad name.

This definitely sounds like something we should not silently ignore.

First of all, not sure if that's a scenario we must "ignore". Does containerd accept image names that we do not accept valid? (e.g. can I create an image in containerd that's named 🐳😄🐱 ?)

Yeah, it happily does.

$ ctr --address /run/docker/containerd/containerd.sock -n moby image ls REF TYPE DIGEST SIZE PLATFORMS LABELS docker.io/library/alpine:latest application/vnd.docker.distribution.manifest.list.v2+json sha256:f271e74b17ced29b915d351685fd4644785c6d1559dd1f2d4189a5e851ef753a 3.1 MiB linux/386,linux/amd64,linux/arm/v6,linux/arm/v7,linux/arm64/v8,linux/ppc64le,linux/s390x - $ ctr --address /run/docker/containerd/containerd.sock -n moby image tag docker.io/library/alpine:latest 🐳😄🐱 🐳😄🐱 $ ctr --address /run/docker/containerd/containerd.sock -n moby image ls REF TYPE DIGEST SIZE PLATFORMS LABELS docker.io/library/alpine:latest application/vnd.docker.distribution.manifest.list.v2+json sha256:f271e74b17ced29b915d351685fd4644785c6d1559dd1f2d4189a5e851ef753a 3.1 MiB linux/386,linux/amd64,linux/arm/v6,linux/arm/v7,linux/arm64/v8,linux/ppc64le,linux/s390x - 🐳😄🐱 application/vnd.docker.distribution.manifest.list.v2+json sha256:f271e74b17ced29b915d351685fd4644785c6d1559dd1f2d4189a5e851ef753a 3.1 MiB linux/386,linux/amd64,linux/arm/v6,linux/arm/v7,linux/arm64/v8,linux/ppc64le,linux/s390x - $ docker image ls REPOSITORY TAG IMAGE ID CREATED SIZE alpine latest f271e74b17ce 44 seconds ago 3.26MB <none> <none> f271e74b17ce 3 seconds ago 3.26MB

The image will still be shown, and can be referenced by the ID. In this case the ID will be ambiguous because we tagged it from alpine, so he won't be able to delete this 🐳😄🐱 image by using Docker.

We could use the isDanglingImage from #44781 here and only override the repo tags to <none>:<none> for our special "dangling" images and still persist the non-standard names.

This definitely sounds like something we should not silently ignore.

Good point; warning would be useful here

Bumping this; I think this PR is ready to bring in once these concerns are addressed.

I added some error logging and removed overriding the tag with <none> for non-dangling images.

ERRO[2023-01-30T14:20:45.364773471Z] failed to parse image name as reference digest="sha256:f271e74b17ced29b915d351685fd4644785c6d1559dd1f2d4189a5e851ef753a" error="invalid reference format" name="🐳😄🐱"

As for images with invalid name:

IIUC, previously, we would show img.Name() (even if that could theoretically be an invalid name); in the new situation we replace that with : (discarding the name altogether). I'm curious in what situations that would happen (as it means we're loosing information about the image in the response).

Previous code did set the repoTags={img.Name()} but doing that made the CLI not show them at all. This is because CLI attempts to parse the repoTags as a named reference (it needs to be able to show the REPO and TAG), and ignores the image if it fails to.
https://github.com/docker/cli/blob/4a500f690f2ef80707839d7b0bbbf38b46b36991/cli/command/formatter/image.go#L116-L119

So, keeping the previous approach (repoTags=invalid name) won't show them in the docker images output at all:

$ ctr --address /run/docker/containerd/containerd.sock -n moby image tag docker.io/library/alpine:l🐳😄🐱🐳😄🐱 🐳😄🐱 $ docker image ls REPOSITORY TAG IMAGE ID CREATED SIZE alpine latest f271e74b17ce 23 seconds ago 3.26MB $ curl -s --unix-socket /var/run/docker.sock http://localhost/images/json | jq [ { "Containers": -1, "Created": 1675087679, "Id": "sha256:f271e74b17ced29b915d351685fd4644785c6d1559dd1f2d4189a5e851ef753a", "Labels": null, "ParentId": "", "RepoDigests": [ "docker.io/library/alpine@sha256:f271e74b17ced29b915d351685fd4644785c6d1559dd1f2d4189a5e851ef753a" ], "RepoTags": [ "docker.io/library/alpine:latest" ], "SharedSize": -1, "Size": 3262892, "VirtualSize": 8138752 }, { "Containers": -1, "Created": 1675087699, "Id": "sha256:f271e74b17ced29b915d351685fd4644785c6d1559dd1f2d4189a5e851ef753a", "Labels": null, "ParentId": "", "RepoDigests": [ "<none>@<none>" ], "RepoTags": [ "🐳😄🐱" ], "SharedSize": -1, "Size": 3262892, "VirtualSize": 8138752 } ]

But at least that looks somewhat better than overriding it (as the API still returns that information).
Possibly we could add some warning to the CLI that warns about a tag that's not a valid reference?

neersighted

We discussed this extensively in the context of #44989 -- there were a couple takeaways:

These PRs both introduce isDanglingImage and <none>:<none> strings -- so we'd like to see the common parts factored out into a third PR that these two can then be re-stacked on.

In particular, centralizing the <none> string constants in the presentation layer and using a sentinel value (possibly the empty string) to represent untagged images in the plumbing layer makes a lot of sense. We should quarantine those values to the API, so that we can possibly deprecate them in favor of client-side presentation in a future API version.

Likewise, isDanglingImage should be factored into a common place for both these PRs (and then consumed by restacking on top of the common PR), as otherwise when both are merged the symbols will collide as they're in the same package.

Signed-off-by: Paweł Gronowski <[email protected]>

vvoland · 2023-02-22T16:11:06Z

I don't think there's a need for third PR - this is the first PR that makes the use of isDanglingImage, and #44989 just mistakenly happened to also include the changes from this PR (first version of this PR).
So I'd suggest finishing this one up and then rebase #44989.

Extracted the <none> strings from ImageService to the API. They will be applied if RepoTags and RepoDigests are empty. I think there's no need for any sentinel value, as empty slice seems like a logical representation of an untagged image.

vvoland · 2023-02-22T16:14:18Z

+		if len(img.RepoTags) == 0 && len(img.RepoDigests) == 0 {
+			img.RepoTags = append(img.RepoTags, "<none>:<none>")
+			img.RepoDigests = append(img.RepoDigests, "<none>@<none>")
+		}
+	}


I was thinking whether it would make sense to separate these, but original code did the same thing (except it checked if they're nil, not empty) and it doesn't seem like there's a reason why only one of them would be empty, so I just left the original logic.

Sounds good to me.

neersighted

Fair point that we could make the second PR conditional on this one. Also I didn't mean that we needed a specific sentinel value, a zero-value is just fine as long as it doesn't violate assumptions made up the call stack. This LGTM now that we've isolated the presentation of <none> values to the API layer; let's merge this and rebase the second PR on top.

neersighted · 2023-02-23T01:40:33Z

Also, can we ask that you look into deprecating the daemon-side <none> with an API version bump as a follow-up?

rumpl · 2023-02-23T08:23:31Z

+
+		if err != nil {
+			if !isDanglingImage(rawImg) {
+				logger.WithError(err).Error("failed to parse image name as reference")


This log looks out of place?

It's there to issue an error in case we encounter an image like "🐳😄🐱", see: #44840 (comment)
We don't want this error to be logged for dangling images - which have moby-dangling@sha256:... format, so they're not a valid Named reference (missing registry).

thaJeztah

LGTM

left one suggestion (let me know if you think that would be useful)

Show dangling images in `docker image ls` output. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland added area/images Image Service containerd-integration Issues and PRs related to containerd integration status/2-code-review labels Jan 18, 2023

neersighted requested changes Jan 22, 2023

View reviewed changes

Comment thread daemon/containerd/image_list.go Outdated

vvoland force-pushed the c8d-list-dangling-upstream branch from f9ffa30 to 1293a3c Compare January 23, 2023 11:40

rumpl approved these changes Jan 23, 2023

View reviewed changes

thaJeztah reviewed Jan 23, 2023

View reviewed changes

vvoland mentioned this pull request Jan 23, 2023

daemon/c8d: Implement docker tag #44781

Merged

thaJeztah mentioned this pull request Jan 24, 2023

consider making image names less permissive containerd/containerd#7986

Closed

vvoland force-pushed the c8d-list-dangling-upstream branch from 1293a3c to 223b60a Compare January 31, 2023 15:46

neersighted approved these changes Feb 2, 2023

View reviewed changes

vvoland commented Feb 2, 2023

View reviewed changes

Comment thread daemon/containerd/image_list.go Outdated

vvoland force-pushed the c8d-list-dangling-upstream branch from 223b60a to c12d8c1 Compare February 8, 2023 14:40

neersighted requested changes Feb 20, 2023

View reviewed changes

api: Move Repo(Digests|Tags) <none> fallback from daemon

2f9e3cc

Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-list-dangling-upstream branch from c12d8c1 to 93719be Compare February 22, 2023 16:10

vvoland commented Feb 22, 2023

View reviewed changes

neersighted requested review from rumpl and thaJeztah February 23, 2023 01:38

neersighted approved these changes Feb 23, 2023

View reviewed changes

neersighted requested a review from corhere February 23, 2023 01:40

rumpl reviewed Feb 23, 2023

View reviewed changes

Comment thread daemon/containerd/image_list.go Outdated

vvoland requested a review from rumpl February 23, 2023 09:52

vvoland force-pushed the c8d-list-dangling-upstream branch from 93719be to 09dd725 Compare February 23, 2023 09:53

rumpl approved these changes Feb 23, 2023

View reviewed changes

vvoland mentioned this pull request Feb 23, 2023

api: Remove <none> in Repo(Tags|Digests) for >= 1.43 #45068

Merged

thaJeztah added this to the v-next milestone Feb 23, 2023

thaJeztah approved these changes Feb 23, 2023

View reviewed changes

Comment thread daemon/containerd/image_list.go

neersighted added the kind/bugfix PR's that fix bugs label Feb 23, 2023

corhere approved these changes Feb 23, 2023

View reviewed changes

c8d/list: Fix Repo(Digests|Tags) for untagged images

f8791db

Show dangling images in `docker image ls` output. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-list-dangling-upstream branch from 09dd725 to f8791db Compare February 23, 2023 18:55

neersighted approved these changes Feb 23, 2023

View reviewed changes

neersighted merged commit a9f17a2 into moby:master Feb 23, 2023

neersighted added status/4-merge and removed status/2-code-review labels Feb 23, 2023

vvoland mentioned this pull request Feb 24, 2023

list: Backport changes from upstream rumpl/moby#134

Merged

laurazard mentioned this pull request Feb 26, 2023

containerd integration: handle multi-platform images #44989

Merged

vvoland mentioned this pull request Mar 30, 2023

c8d/inspect: Handle dangling images #45231

Merged

thaJeztah mentioned this pull request May 7, 2025

containerd integration: verify RepoDigests, RepoTags works for untagged or multiple tags #43861

Closed

Conversation

vvoland commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

neersighted commented Feb 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vvoland commented Jan 18, 2023 •

edited

Loading

vvoland Jan 23, 2023 •

edited

Loading