c8d/push: Support --platform switch

[vvoland]

Add a platform parameter to the POST /images/{id}/push that allows to specify a single-platform manifest to be pushed instead of the whole image index.

- What I did

- How I did it

- How to verify it

- Description for the changelog

- containerd image store: `POST /images/{name}/push` now supports a `platform` parameter that allows pushing a specific platform-manifest from the multi-platform image. 

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

[cpuguy83]

Noting what we discussed on zoom:

I think the platform option needs to support something like platform=default so the client doesn't have to know the platform of the daemon.
I think buildkit already has an option like this we can just reuse.

buildx also supports --platform=local which would be the platform of the client.

[vvoland]

I added the local and remote special values.

remote is handled on the server (this PR), the local is handled in the CLI PR: docker/cli#4984.

[rumpl]

The image you are talking about is the img parameter right? That implements images.Image? I'm confused, I don't see any containerd.Image here

[vvoland]

It was meant to describe the img ImageManifest parameter which is an implementation of the containerd.Image interface.

But I think it doesn't really make sense to include here.. Moved this comment to the ImageManifest struct Godoc.

[cpuguy83]

cc @tianon

More platform strings :)

[tianon]

oof, and generated in client code to pass to the daemon 😭

local in the client is not local to the daemon 😭

[vvoland]

Made local mean the platform local to the client for consistency with buildx --platform local.

That's why there's also remote (although I think I'd prefer to call it host) that is handled on the daemon side and defaults to the daemon platform:

moby/api/server/router/image/image_routes.go

Lines 554 to 559 in dfe5a6c

	func parsePlatform(platformStr string) (ocispec.Platform, error) {
	if platformStr == "remote" {
	return platforms.DefaultSpec(), nil
	}
	return platforms.Parse(platformStr)
	}

Happy to hear your suggestions if you have better ideas 😄

[tianon]

Definitely creeping into "build" vs "host" vs "target" confusion territory here -- what's the use case for building for the client-local host's platform when talking to a different daemon?

(Some of the context for this discussion is containerd/platforms#6, which actually makes platforms.DefaultString() unsafe to pass across process/machine boundaries like this, but I think the issue here is larger.)

[cpuguy83]

Probably because I'd mentioned it in an earlier review, but more "the client shouldn't have to know the daemon's platform... also should we do the buildx thing for client platform?"
It may well be not needed.

[vvoland]

what's the use case for building for the client-local host's platform when talking to a different daemon?

Personally, --platform local was useful to me for extracting binaries out of docker images to use on my host system, like:

echo 'FROM moby/moby-bin:master' | docker buildx build  --platform local -o type=local,dest=asdadsad -

So, mostly thinking of remote daemon or, a more down-to-earth case: DD on non-Linux platforms - in the above case I really don't care about the VM, I just want to get the image for my host.

However, local might be less useful than the remote/host/daemon for the interactive case, because most of the time I know what platform my client host is running on. Still, it could be useful in scripts.

Also, in case of a remote Docker host - users might want to push the image for the platform of their host:

$ docker -c beef tag <some-image> myregistry.local/some-image
$ docker -c beef push --platform local myregistry.local/some-image
$ docker run myregistry.local/some-image

But still, I think that's mostly useful for scripts, as in the interactive case it's highly likely that user would pass the platform explicitly anyway.

[vvoland]

Anyway, I'm fine with dropping these for now. We can always add them later if there's a real demand for it.

[dmcgowan]

I would advise against using a single string field to represent the platform in the API. The first issue is highlighted here that it is too tempting to override it with special values. The second issue is that the string format is now a part of the API which it is not intended for. In containerd we only use the string for CLI or config, but the API always uses the structured form (https://github.com/containerd/containerd/blob/main/api/types/platform.proto) or its embedded in json in the original form (https://github.com/opencontainers/image-spec/blob/main/specs-go/v1/descriptor.go#L53).

The point of this change is to make the API endpoint behavior more explicit which filling in a variable "remote" value doesn't really accomplish. The way the current API behaves is already "--platform remote" and I worry by adding this flag here we end up changing the API to default to only push all platforms and end up recommending users to add "--platform remote". I think its best to keep the default to push whatever what pulled for that same image, so the equivalent of "--platform remote" if only a single platform was pulled or push all platforms if we can support pull all platforms. The case of pushing a single platform which differs from the remote after pulling multiple platforms seems like an edge case and is likely better handled with a separate command to edit an image with a new manifest.

[cpuguy83]

Can we get an integration test for this?