c8d/inspect: Add `Manifests` field #48264

vvoland · 2024-07-29T16:01:33Z

needs: c8d: Multi-platform image list #47526
needs: c8d/list: Fix Total size calculation #48330
follow up to: c8d: Multi-platform image list #47526

Add Manifests field to image inspect (/images/{name}/json) response.
This is the same as in /images/json.

- How to verify it

curl --unix-socket /var/run/docker.sock http://localhost/images/busybox/json?manifests=1

- Description for the changelog

`GET /images/{name}/json` response now will return the `Manifests` field containing information about the sub-manifests contained in the image index. This includes things like platform-specific manifests and build attestations.
The new field will only be populated if the request also sets the `manifests` query parameter to `true`.
This acts the same as in the `GET /images/json` endpoint.
WARNING: This is experimental and may change at any time without any backward compatibility.

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

laurazard

LGTM (took my brain a sec to wrap around the changes 😅 probably because I haven't looked at this in a while)

thaJeztah · 2024-08-14T11:03:07Z

api/types/image/image_inspect.go

+	//
+	// WARNING: This is experimental and may change at any time without any backward
+	// compatibility.
+	Manifests []ManifestSummary `json:"Manifests,omitempty"`


This field should probably be gated by API version.

Slightly wondering if this should be (at least currently?) an optional field; similar to how we have the "tree view" optional.

Ah right, added a version gate.

Why should it be optional though? You can only inspect one image anyway, so it won't bloat the transmitted data too much.

vvoland · 2024-08-14T12:19:30Z

Rebased on top of #48330 (as both touch the same code).

thaJeztah · 2024-08-15T09:57:29Z

Why should it be optional though? You can only inspect one image anyway, so it won't bloat the transmitted data too much.

Perhaps less relevant (but perhaps it is?) I was curious if there's a performance impact for getting all the info; but it's very much possible we would need (most of) the data in either case (that said; perhaps there's parts we could keep cached somewhere to save on creating the summary).

But regardless, depending on the number of architectures in an image, and depending on whether there's attestations, the Manifests still produce a lot of extra information. Here's alpine as an example;

docker pull --platform=linux/amd64 alpine

Using default tag: latest
latest: Pulling from library/alpine
c6a83fedfae6: Download complete
Digest: sha256:0a4eaa0eecf5f8c050e5bba433f58c052be7587ee8af3e8b3910ef9ab5fbe9f5
Status: Downloaded newer image for alpine:latest
docker.io/library/alpine:latest

docker image inspect alpine

This outputs;

A summary of the default platform, or if missing the first available platform
A summary of all associated manifests (images, attestations); also for those that are not currently pulled (Available: false);

[
    {
        "Id": "sha256:0a4eaa0eecf5f8c050e5bba433f58c052be7587ee8af3e8b3910ef9ab5fbe9f5",
        "RepoTags": [
            "alpine:latest"
        ],
        "RepoDigests": [
            "alpine@sha256:0a4eaa0eecf5f8c050e5bba433f58c052be7587ee8af3e8b3910ef9ab5fbe9f5"
        ],
        "Parent": "",
        "Comment": "",
        "Created": "2024-07-22T22:26:43.778747613Z",
        "DockerVersion": "",
        "Author": "",
        "Config": {
            "Hostname": "",
            "Domainname": "",
            "User": "",
            "AttachStdin": false,
            "AttachStdout": false,
            "AttachStderr": false,
            "Tty": false,
            "OpenStdin": false,
            "StdinOnce": false,
            "Env": [
                "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
            ],
            "Cmd": [
                "/bin/sh"
            ],
            "Image": "",
            "Volumes": null,
            "WorkingDir": "",
            "Entrypoint": null,
            "OnBuild": null,
            "Labels": null
        },
        "Architecture": "amd64",
        "Os": "linux",
        "Size": 2381,
        "GraphDriver": {
            "Data": null,
            "Name": "overlayfs"
        },
        "RootFS": {
            "Type": "layers",
            "Layers": [
                "sha256:78561cef0761903dd2f7d09856150a6d4fb48967a8f113f3e33d79effbf59a07"
            ]
        },
        "Metadata": {
            "LastTagTime": "2024-08-15T09:43:26.820642347Z"
        },
        "Manifests": [
            {
                "ID": "sha256:eddacbc7e24bf8799a4ed3cdcfa50d4b88a323695ad80f317b6629883b2c2a78",
                "Descriptor": {
                    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
                    "digest": "sha256:eddacbc7e24bf8799a4ed3cdcfa50d4b88a323695ad80f317b6629883b2c2a78",
                    "size": 528,
                    "platform": {
                        "architecture": "amd64",
                        "os": "linux"
                    }
                },
                "Available": true,
                "Size": {
                    "Content": 3624891,
                    "Total": 12083131
                },
                "Kind": "image",
                "ImageData": {
                    "Platform": {
                        "architecture": "amd64",
                        "os": "linux"
                    },
                    "Size": {
                        "Unpacked": 8458240
                    },
                    "Containers": null
                }
            },
            {
                "ID": "sha256:5c7e326e3c8a8c51654a6c5d94dac98d7f6fc4b2a762d86aaf67b7e76a6aee46",
                "Descriptor": {
                    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
                    "digest": "sha256:5c7e326e3c8a8c51654a6c5d94dac98d7f6fc4b2a762d86aaf67b7e76a6aee46",
                    "size": 528,
                    "platform": {
                        "architecture": "arm",
                        "os": "linux",
                        "variant": "v6"
                    }
                },
                "Available": false,
                "Size": {
                    "Content": 0,
                    "Total": 0
                },
                "Kind": "image",
                "ImageData": {
                    "Platform": {
                        "architecture": "arm",
                        "os": "linux",
                        "variant": "v6"
                    },
                    "Size": {
                        "Unpacked": 0
                    },
                    "Containers": null
                }
            },
            {
                "ID": "sha256:fda9b1b812b25c68f94da5b719039bfa9a3b76e167a8f87e7fc62cb159d21ca1",
                "Descriptor": {
                    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
                    "digest": "sha256:fda9b1b812b25c68f94da5b719039bfa9a3b76e167a8f87e7fc62cb159d21ca1",
                    "size": 528,
                    "platform": {
                        "architecture": "arm",
                        "os": "linux",
                        "variant": "v7"
                    }
                },
                "Available": false,
                "Size": {
                    "Content": 0,
                    "Total": 0
                },
                "Kind": "image",
                "ImageData": {
                    "Platform": {
                        "architecture": "arm",
                        "os": "linux",
                        "variant": "v7"
                    },
                    "Size": {
                        "Unpacked": 0
                    },
                    "Containers": null
                }
            },
            {
                "ID": "sha256:24ba417e25e780ff13c888ccb1badec5b027944666ff695681909bafe09a3944",
                "Descriptor": {
                    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
                    "digest": "sha256:24ba417e25e780ff13c888ccb1badec5b027944666ff695681909bafe09a3944",
                    "size": 528,
                    "platform": {
                        "architecture": "arm64",
                        "os": "linux",
                        "variant": "v8"
                    }
                },
                "Available": false,
                "Size": {
                    "Content": 0,
                    "Total": 0
                },
                "Kind": "image",
                "ImageData": {
                    "Platform": {
                        "architecture": "arm64",
                        "os": "linux",
                        "variant": "v8"
                    },
                    "Size": {
                        "Unpacked": 0
                    },
                    "Containers": null
                }
            },
            {
                "ID": "sha256:fa66aa594ffa884dff44f4a97821756545834505df611c375a30c45926402f89",
                "Descriptor": {
                    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
                    "digest": "sha256:fa66aa594ffa884dff44f4a97821756545834505df611c375a30c45926402f89",
                    "size": 528,
                    "platform": {
                        "architecture": "386",
                        "os": "linux"
                    }
                },
                "Available": false,
                "Size": {
                    "Content": 0,
                    "Total": 0
                },
                "Kind": "image",
                "ImageData": {
                    "Platform": {
                        "architecture": "386",
                        "os": "linux"
                    },
                    "Size": {
                        "Unpacked": 0
                    },
                    "Containers": null
                }
            },
            {
                "ID": "sha256:a01843eb870e11bb20c78a9068269c810f14dd5c49364064fa3f9cf798f666dd",
                "Descriptor": {
                    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
                    "digest": "sha256:a01843eb870e11bb20c78a9068269c810f14dd5c49364064fa3f9cf798f666dd",
                    "size": 528,
                    "platform": {
                        "architecture": "ppc64le",
                        "os": "linux"
                    }
                },
                "Available": false,
                "Size": {
                    "Content": 0,
                    "Total": 0
                },
                "Kind": "image",
                "ImageData": {
                    "Platform": {
                        "architecture": "ppc64le",
                        "os": "linux"
                    },
                    "Size": {
                        "Unpacked": 0
                    },
                    "Containers": null
                }
            },
            {
                "ID": "sha256:e99a4d9aa9f905cee4171c6d616e4008fa32202fa8aa8aa65efcafbc3a0f5fa5",
                "Descriptor": {
                    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
                    "digest": "sha256:e99a4d9aa9f905cee4171c6d616e4008fa32202fa8aa8aa65efcafbc3a0f5fa5",
                    "size": 528,
                    "platform": {
                        "architecture": "riscv64",
                        "os": "linux"
                    }
                },
                "Available": false,
                "Size": {
                    "Content": 0,
                    "Total": 0
                },
                "Kind": "image",
                "ImageData": {
                    "Platform": {
                        "architecture": "riscv64",
                        "os": "linux"
                    },
                    "Size": {
                        "Unpacked": 0
                    },
                    "Containers": null
                }
            },
            {
                "ID": "sha256:14da06d3a8959002fd621dd3994a254e2126d239f2fe69e829fd95d16ce81dea",
                "Descriptor": {
                    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
                    "digest": "sha256:14da06d3a8959002fd621dd3994a254e2126d239f2fe69e829fd95d16ce81dea",
                    "size": 528,
                    "platform": {
                        "architecture": "s390x",
                        "os": "linux"
                    }
                },
                "Available": false,
                "Size": {
                    "Content": 0,
                    "Total": 0
                },
                "Kind": "image",
                "ImageData": {
                    "Platform": {
                        "architecture": "s390x",
                        "os": "linux"
                    },
                    "Size": {
                        "Unpacked": 0
                    },
                    "Containers": null
                }
            }
        ]
    }
]

That data is "per request", but it's possible to inspect multiple images; that results in multiple requests, but the output on the client side will be a combination of all of those;

docker tag alpine blpine

docker image inspect alpine blpine

Some of that for sure is a limitation on the client-side; dumping the JSON output will always be verbose, and not very user-friendly (we need to start working on the "human friendly" versions of docker image inspect (docker image info ?)). But even with that, I'm wondering if the in-depth information is always relevant; more so if not all platforms were pulled.

So I'm wondering if we need to make these "opt-in" to have more granularity;

default could continue to be "only a summary of the default (or if missing: first) platform"
optional with the nested ones
perhaps some option to show/hide "non-available" ones

We also need to look at a --platform flag for this; not exactly sure (yet) what we want that to act like;

At least it can control "which platform" to produce the overall summary for (as the image config is only present for the overall summary, and may have relevant differences between platforms)
Do we want it to act as a filter for the nested manifests? Do we need a separate option for that (manifests=<all|available|platforms=....)?

vvoland · 2024-08-16T09:20:32Z

Perhaps less relevant (but perhaps it is?) I was curious if there's a performance impact for getting all the info; but it's very much possible we would need (most of) the data in either case (that said; perhaps there's parts we could keep cached somewhere to save on creating the summary).

We're already traversing all of these (to pick the "best representative" variant and calculate the size), so the only performance impact here is just storing the data in the JSON.

So I'm wondering if we need to make these "opt-in" to have more granularity;

IMO, opt-in for inspect is awkward because:

In most cases you use docker inspect directly
The extra manifests or platform opt-in only make sense for the image inspect
So we would either have to add these to the docker inspect (which would only for for image) or only to docker image inspect (which would make it impossible to use for docker inspect)

Also, docker inspect purpose was always to "return low-level information on Docker objects" - I think it's expected to have a non-human friendly output.

(we need to start working on the "human friendly" versions of docker image inspect (docker image info ?)).

And this is the direction I'd like to focus next - we really need that (and not just for images, but all "objects" in general). That's where we can have all the granularity we want and provide a better human-readable output, while docker inspect would still remain a "plumbing" command.

vvoland · 2024-10-28T12:21:43Z

@thaJeztah - do you think we should wait with this one until we have a higher level image info/show command?

vvoland · 2025-01-21T11:53:05Z

Updated; the field will now only be returned if the manifests query parameter is set.

vvoland · 2025-01-21T11:54:30Z

client/image_inspect.go

+type imageInspectClientOpt struct {
+	raw        *bytes.Buffer
+	apiOptions image.InspectOptions
+}
+
+type ImageInspectOpt func(*imageInspectClientOpt)
+
+// ImageInspectWithRawResponse instructs the client to additionally store the
+// raw inspect response in the provided buffer.
+func ImageInspectWithRawResponse(raw *bytes.Buffer) ImageInspectOpt {
+	return func(opts *imageInspectClientOpt) {
+		opts.raw = raw
+	}
+}
+
+// ImageInspectWithOpts sets the API options for the image inspect operation.
+func ImageInspectWithOpts(opts image.InspectOptions) ImageInspectOpt {
+	return func(clientOpts *imageInspectClientOpt) {
+		clientOpts.apiOptions = opts
+	}
+}
+
+// ImageInspect returns the image information.
+func (cli *Client) ImageInspect(ctx context.Context, imageID string, inspectOpts ...ImageInspectOpt) (image.InspectResponse, error) {


I can't say I fully like it. The "raw response" part is strictly a client feature, so it doesn't make sense to put it directly in the image.InspectOptions, on the other hand that forces us to have a client specific opts..

Any thoughts on this one? Would be nice to think this carefully, as it breaks the client interface.

The linter will complain about the usage of deprecated ImageInspectWithRaw, but for now I'll refrain from adjusting these in case we reach a different conclusion on the shape of this.

Hm; I'll have a look at this one!

(For the linter; if there's too many places to suppress it, we can also add a global exception in .golangci-lint.yaml)

Actually I can just break the Deprecated comment for now to make the linter happy 😅

thaJeztah

Thanks for updating; only had a quick glance over (just some quick comments, but didn't do a full review)

api/swagger.yaml

thaJeztah · 2025-01-21T16:31:00Z

client/image_inspect.go

+type imageInspectClientOpt struct {
+	raw        *bytes.Buffer
+	apiOptions image.InspectOptions
+}
+
+type ImageInspectOpt func(*imageInspectClientOpt)
+
+// ImageInspectWithRawResponse instructs the client to additionally store the
+// raw inspect response in the provided buffer.
+func ImageInspectWithRawResponse(raw *bytes.Buffer) ImageInspectOpt {
+	return func(opts *imageInspectClientOpt) {
+		opts.raw = raw
+	}
+}
+
+// ImageInspectWithOpts sets the API options for the image inspect operation.
+func ImageInspectWithOpts(opts image.InspectOptions) ImageInspectOpt {
+	return func(clientOpts *imageInspectClientOpt) {
+		clientOpts.apiOptions = opts
+	}
+}
+
+// ImageInspect returns the image information.
+func (cli *Client) ImageInspect(ctx context.Context, imageID string, inspectOpts ...ImageInspectOpt) (image.InspectResponse, error) {


Hm; I'll have a look at this one!

(For the linter; if there's too many places to suppress it, we can also add a global exception in .golangci-lint.yaml)

client/image_inspect.go

laurazard

LGTM

thaJeztah · 2025-01-29T12:12:41Z

daemon/containerd/image_inspect.go

+		if err != nil {
+			log.G(ctx).WithField("name", name).WithError(err).Error("failed to parse image name as reference")
+			// Include the malformed name in RepoTags to be consistent with `docker image ls`.
+			repoTags = append(repoTags, img.Name)


Pretty unrelated; I thought for a bit we still had a bug to fix around this;

c8d: image inspect should not contain duplicate RepoDigests #48747

But we fixed that "in post";

c8d/inspect: Fix duplicate RepoDigests #48777

Wondering now if we should look at handling de-duplicating here (in a follow-up); i.e., if it would be more efficient to do it right at the start, WDYT?

Right, let me update

It's fine for a follow up as it's not directly related! Was reviewing this PR, and I think (almost done) it looks good otherwise

Was wondering about the performance difference between collecting them to map & then copying keys to slices between collecting to slice and sorting it afterwards.
Results:

BenchmarkCollectRepoTagsAndDigests BenchmarkCollectRepoTagsAndDigests/slice BenchmarkCollectRepoTagsAndDigests/slice-8 627352 18742 ns/op 5644 B/op 118 allocs/op BenchmarkCollectRepoTagsAndDigests/map BenchmarkCollectRepoTagsAndDigests/map-8 635817 18702 ns/op 5397 B/op 116 allocs/op PASS

Benchmark code

package containerd import ( "context" "testing" c8dimages "github.com/containerd/containerd/v2/core/images" "github.com/containerd/log" "github.com/distribution/reference" "github.com/docker/docker/internal/sliceutil" ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) func BenchmarkCollectRepoTagsAndDigests(b *testing.B) { ctx := context.Background() tagged := []c8dimages.Image{ {Name: "docker.io/library/alpine:latest", Target: ocispec.Descriptor{Digest: "sha256:20e6ec8bc89f4bd24469283fe6c147cfed1ad837e435386ed050d8c57c58eb91"}}, {Name: "docker.io/library/alpine:3.14", Target: ocispec.Descriptor{Digest: "sha256:30e6ec8bc89f4bd24469283fe6c147cfed1ad837e435386ed050d8c57c58eb92"}}, {Name: "docker.io/library/alpine:3.13", Target: ocispec.Descriptor{Digest: "sha256:40e6ec8bc89f4bd24469283fe6c147cfed1ad837e435386ed050d8c57c58eb93"}}, {Name: "docker.io/library/alpine:3.13", Target: ocispec.Descriptor{Digest: "sha256:40e6ec8bc89f4bd24469283fe6c147cfed1ad837e435386ed050d8c57c58eb93"}}, {Name: "docker.io/library/alpine@sha256:20e6ec8bc89f4bd24469283fe6c147cfed1ad837e435386ed050d8c57c58eb91", Target: ocispec.Descriptor{Digest: "sha256:20e6ec8bc89f4bd24469283fe6c147cfed1ad837e435386ed050d8c57c58eb91"}}, {Name: "docker.io/library/alpine@sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300", Target: ocispec.Descriptor{Digest: "sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300"}}, } b.Run("slice", func(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { collectRepoTagsAndDigests(ctx, tagged) } b.ReportAllocs() }) b.Run("map", func(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { collectRepoTagsAndDigestsWithMap(ctx, tagged) } b.ReportAllocs() }) } func collectRepoTagsAndDigests(ctx context.Context, tagged []c8dimages.Image) (repoTags []string, repoDigests []string) { repoTags = make([]string, 0, len(tagged)) repoDigests = make([]string, 0, len(tagged)) for _, img := range tagged { if isDanglingImage(img) { if len(tagged) > 1 { // This is unexpected - dangling image should be deleted // as soon as another image with the same target is created. // Log a warning, but don't error out the whole operation. log.G(ctx).WithField("refs", tagged).Warn("multiple images have the same target, but one of them is still dangling") } continue } name, err := reference.ParseNamed(img.Name) if err != nil { log.G(ctx).WithField("name", name).WithError(err).Error("failed to parse image name as reference") // Include the malformed name in RepoTags to be consistent with `docker image ls`. repoTags = append(repoTags, img.Name) continue } repoTags = append(repoTags, reference.FamiliarString(name)) if _, ok := name.(reference.Digested); ok { repoDigests = append(repoDigests, reference.FamiliarString(name)) // Image name is a digested reference already, so no need to create a digested reference. continue } digested, err := reference.WithDigest(reference.TrimNamed(name), img.Target.Digest) if err != nil { // This could only happen if digest is invalid, but considering that // we get it from the Descriptor it's highly unlikely. // Log error just in case. log.G(ctx).WithError(err).Error("failed to create digested reference") continue } repoDigests = append(repoDigests, reference.FamiliarString(digested)) } return sliceutil.Dedup(repoTags), sliceutil.Dedup(repoDigests) } // collectRepoTagsAndDigestsWithMap returns repoTags and repoDigests for images with the same target using maps for deduplication. func collectRepoTagsAndDigestsWithMap(ctx context.Context, tagged []c8dimages.Image) (repoTags []string, repoDigests []string) { repoTagsMap := make(map[string]struct{}) repoDigestsMap := make(map[string]struct{}) for _, img := range tagged { if isDanglingImage(img) { if len(tagged) > 1 { // This is unexpected - dangling image should be deleted // as soon as another image with the same target is created. // Log a warning, but don't error out the whole operation. log.G(ctx).WithField("refs", tagged).Warn("multiple images have the same target, but one of them is still dangling") } continue } name, err := reference.ParseNamed(img.Name) if err != nil { log.G(ctx).WithField("name", name).WithError(err).Error("failed to parse image name as reference") // Include the malformed name in RepoTags to be consistent with `docker image ls`. repoTagsMap[img.Name] = struct{}{} continue } repoTagsMap[reference.FamiliarString(name)] = struct{}{} if _, ok := name.(reference.Digested); ok { repoDigestsMap[reference.FamiliarString(name)] = struct{}{} // Image name is a digested reference already, so no need to create a digested reference. continue } digested, err := reference.WithDigest(reference.TrimNamed(name), img.Target.Digest) if err != nil { // This could only happen if digest is invalid, but considering that // we get it from the Descriptor it's highly unlikely. // Log error just in case. log.G(ctx).WithError(err).Error("failed to create digested reference") continue } repoDigestsMap[reference.FamiliarString(digested)] = struct{}{} } repoTags = make([]string, 0, len(repoTagsMap)) repoDigests = make([]string, 0, len(repoDigestsMap)) // Convert maps to slices for tag := range repoTagsMap { repoTags = append(repoTags, tag) } for digest := range repoDigestsMap { repoDigests = append(repoDigests, digest) } return repoTags, repoDigests }

So not a big difference, slice is slightly faster but does more a bit more allocs than the map.

I'll go with map.

thaJeztah

LGTM

one question on the "raw" custom reader passed, but maybe I'm thinking wrong there (otherwise, we could consider un-exporting it in the meantime)

thaJeztah · 2025-01-29T14:03:12Z

api/server/router/image/image_routes.go

+	if r.Form.Get("manifests") != "" && versions.LessThan(httputils.VersionFromContext(ctx), "1.48") {
+		return errdefs.InvalidParameter(errors.New("manifests parameter is not supported for API version < 1.48"))
+	}


It's probably fine to have checks on both clients and API. I was considering that this is an option that we could consider to be ignored on the daemon side, i.e., I think the client would be able to gracefully handle responses where this information is not present.

No need to change, just as a comment, in case we want to revisit this.

Hmm right, older daemons that don't understand the new API wouldn't complain about this, so I think there's no point to make newer daemon fail in this case?

thaJeztah · 2025-01-29T14:18:50Z

api/types/image/opts.go

+type InspectOptions struct {
+	// Manifests returns the image manifests.
+	Manifests bool
+}
+


It just occurred to me that there's 2 option structs now;

backend.ImageInspectOptions (used by the API server)

image.InspectOptions (used by the client)

~~The last one, by design, is not exported; it's made internal through imageInspectClientOpt.apiOptions (non-exported type AND non-exported field)~~

~~Given that those are not exported currently, perhaps we should (for now at least) move this type to the client, and un-export it?~~

⚠️ 👉 edit: ignore me; there's ImageInspectWithOpts, which allows all options to be passed as a struct

thaJeztah · 2025-01-29T14:22:44Z

client/image_inspect.go

+type imageInspectClientOpt struct {
+	raw        *bytes.Buffer
+	apiOptions image.InspectOptions
+}
+
+type ImageInspectOpt func(*imageInspectClientOpt)


This part is OK for now, and I like the idea of non-exported fields for "client" and "api" options for clearer separation.

The only thing here that I want to look into a bit (not for this PR) is what design pattern is most logical; the confusing bit here is that ImageInspectOpt is an exported type, but it includes a non-exported type in its signature. That means that no implementations can exist outside of those implemented in this package (which currently is fine?).

Mostly wondering if the non-exported type in its signature could be confusing, and it it would make sense to (at some point) export the imageInspectClientOpt type itself, but keep its fields (those that we don't want to be set through other means than functional arguments) un-exported.

LOL; hope I'm making sense here!

Yeah so actually I wanted to go one step further and make the ImageInspectOpt itself unexported, because with all imageInspectClientOpt fields being private, there's no benefit for the user to be able to construct its own opt func.

That didn't work though, because linter was complaining about the exported functions using the unexported type.

Alternatively.. we could drop the func opts and just have ImageInspectOpt that would embed image.InspectOptions and exported Raw.

Added a commit with alternative approach: 51bfa52

Thoughts? @thaJeztah @laurazard

Buildkit uses interfaces for option types which makes it much easier to discover what can go there.

So it could be something like

type ImageOption interface { SetImageOption(*ImageOpt) }

Then you can do something like

type ImageOptFunc func(*ImageOpt) func(f ImageOptFunc) SetImageOption(o *ImageOpt) { f(o) }

client/image_inspect.go

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Don't use the `GetImage` call which returns a "best-effort" view of the image that is compatible with the old images.Image response. Instead, use the multi-platform view of the image to construct the inspect response. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Add `Manifests` field to image inspect (`/images/{name}/json`) response. This is the same as in `/images/json`. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland added area/api impact/api impact/changelog area/images containerd-integration Issues and PRs related to containerd integration labels Jul 29, 2024

vvoland added this to the 28.0.0 milestone Jul 29, 2024

vvoland self-assigned this Jul 29, 2024

vvoland changed the title ~~c8d/inspect: AddManifests field~~ c8d/inspect: Add Manifests field Jul 29, 2024

vvoland marked this pull request as ready for review August 8, 2024 10:16

vvoland requested a review from tianon as a code owner August 8, 2024 10:16

vvoland force-pushed the c8d-inspect-manifests branch from 013c580 to 61758d1 Compare August 8, 2024 10:17

vvoland requested review from thaJeztah and laurazard August 8, 2024 17:34

vvoland force-pushed the c8d-inspect-manifests branch 2 times, most recently from 54c91a4 to 49956b1 Compare August 9, 2024 10:00

vvoland added the process/cherry-pick/27.x label Aug 13, 2024

laurazard approved these changes Aug 14, 2024

View reviewed changes

thaJeztah reviewed Aug 14, 2024

View reviewed changes

vvoland force-pushed the c8d-inspect-manifests branch 3 times, most recently from 7cd9db0 to 8f3682a Compare August 14, 2024 12:19

vvoland force-pushed the c8d-inspect-manifests branch from 8f3682a to a2945a1 Compare August 14, 2024 14:14

vvoland requested a review from thaJeztah August 14, 2024 17:44

thaJeztah mentioned this pull request Aug 15, 2024

c8d: warnings printed when building multi-arch image #48335

Open

thaJeztah mentioned this pull request Aug 15, 2024

image/list: Add --tree flag docker/cli#4982

Merged

vvoland removed the process/cherry-pick/27.x label Aug 16, 2024

thaJeztah mentioned this pull request Oct 9, 2024

With containerd, I pulled images debian, ubuntu, nginx all latest, using --platform=linux/arm64/v8 and multiple strange observations. docker/cli#5523

Open

vvoland mentioned this pull request Nov 13, 2024

c8d/container/inspect: Return ImageManifestDescriptor #48855

Merged

thompson-shaun modified the milestones: 28.0.0, 29.0.0 Jan 17, 2025

vvoland force-pushed the c8d-inspect-manifests branch 2 times, most recently from e80f026 to ee89881 Compare January 21, 2025 11:52

vvoland commented Jan 21, 2025

View reviewed changes

vvoland requested review from laurazard and cpuguy83 January 21, 2025 11:54

vvoland force-pushed the c8d-inspect-manifests branch 2 times, most recently from 3397225 to 57aaf0d Compare January 21, 2025 15:43

thaJeztah reviewed Jan 21, 2025

View reviewed changes

vvoland force-pushed the c8d-inspect-manifests branch from 57aaf0d to f756c6f Compare January 21, 2025 16:38

laurazard approved these changes Jan 22, 2025

View reviewed changes

thaJeztah reviewed Jan 29, 2025

View reviewed changes

thaJeztah approved these changes Jan 29, 2025

View reviewed changes

vvoland added 4 commits January 29, 2025 17:56

c8d/list: multiPlatformSummary

8533554

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

c8d/inspect: Add Manifests field

4219ce7

Add `Manifests` field to image inspect (`/images/{name}/json`) response. This is the same as in `/images/json`. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Alternative client opts !fixup! c8d/inspect: Add Manifests field

51bfa52

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the c8d-inspect-manifests branch from f756c6f to 51bfa52 Compare January 29, 2025 16:56

thaJeztah added the release-blocker PRs we want to block a release on label Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c8d/inspect: Add `Manifests` field #48264

c8d/inspect: Add `Manifests` field #48264

vvoland commented Jul 29, 2024 •

edited

Loading

laurazard left a comment

thaJeztah Aug 14, 2024

vvoland Aug 14, 2024

vvoland commented Aug 14, 2024 •

edited

Loading

thaJeztah commented Aug 15, 2024

vvoland commented Aug 16, 2024

vvoland commented Oct 28, 2024

vvoland commented Jan 21, 2025

vvoland Jan 21, 2025

vvoland Jan 21, 2025

thaJeztah Jan 21, 2025

vvoland Jan 21, 2025

thaJeztah left a comment

thaJeztah Jan 21, 2025

laurazard left a comment

thaJeztah Jan 29, 2025

vvoland Jan 29, 2025

thaJeztah Jan 29, 2025

vvoland Jan 29, 2025

thaJeztah left a comment

thaJeztah Jan 29, 2025

vvoland Jan 29, 2025

thaJeztah Jan 29, 2025

thaJeztah Jan 29, 2025

vvoland Jan 29, 2025

vvoland Jan 29, 2025

cpuguy83 Jan 30, 2025

cpuguy83 Jan 30, 2025

c8d/inspect: Add Manifests field #48264

Are you sure you want to change the base?

c8d/inspect: Add Manifests field #48264

Conversation

vvoland commented Jul 29, 2024 • edited Loading

laurazard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvoland commented Aug 14, 2024 • edited Loading

thaJeztah commented Aug 15, 2024

vvoland commented Aug 16, 2024

vvoland commented Oct 28, 2024

vvoland commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c8d/inspect: Add `Manifests` field #48264

c8d/inspect: Add `Manifests` field #48264

vvoland commented Jul 29, 2024 •

edited

Loading

vvoland commented Aug 14, 2024 •

edited

Loading