daemon: allow setting OCI annotations on containers by corhere · Pull Request #45025 · moby/moby

corhere · 2023-02-17T01:35:17Z

Relates to Support alternative runtimes with cri-dockerd #45030
Closes Pass container labels as annotations in OCI spec #39142
Closes Add flag annotation #37289
Closes Pass the label of docker container to the annotation of OCI runtime spec #36181
Relates to Pass through specific annotation to OCI runtime spec #37262

- What I did

Allow API clients to set and retrieve OCI annotations on a container.

- How I did it

Added a field to HostConfig and plumbed it into the OCI spec generation code.

- How to verify it
Create a container with annotations and start it. Inspect the running container using ctr and verify that the annotations are listed in the container spec.

A new integration test verifies that annotations can be round-tripped through the Engine API.

- Description for the changelog

Annotations—arbitrary non-identifying metadata—can now be attached to containers on creation which will be passed to the runtime when the container is started.

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

thaJeztah · 2023-02-17T01:48:57Z

Will these labels be committed in the image on docker commit ? (also see #42037) And does this mean that an image containing these labels will set them at runtime on the container?

corhere · 2023-02-17T01:56:15Z

Will these labels be committed in the image on docker commit ? (also see #42037)

Yes, until the aforementioned PR is merged.

And does this mean that an image containing these labels will set them at runtime on the container?

Yes. This could arguably be a feature.

thaJeztah · 2023-02-17T09:45:23Z

Yes. This could arguably be a feature.

We should probably look closely what's desirable in that case. I recall there was some prior discussion on passing through labels as annotations. I must admit I'm not fully aware how they're used / what they're able to control, but ISTR there were some concerns that had to be looked into (e.g. a malicious image could set a label and do other things than expected).

I guess to some extent that's not "our" issue (as we don't consume those annotations?).

It does somewhat bring back the issue though that we don't have a concept of "runtime" labels and "non-runtime" (image labels are runtime labels in our case, or at least inherited directly, and in case of "commit", baked into the image).

Probably also need to be looked at what the expectations are for docker container update, as there's been quite some requests on being able to update labels; prior discussions stranded on that one, but if that would be implemented, that may have an effect on the feature in this PR.

AkihiroSuda

And does this mean that an image containing these labels will set them at runtime on the container?

Yes. This could arguably be a feature.

This looks like an attack vector.

Specifying annotations should require a new API/CLI like
docker run --annotation=com.example.foo=bar.
(similar to podman run --annotation)

cpuguy83

SGTM

But need to update the API history doc and openapi yaml.

Signed-off-by: Cory Snider <csnider@mirantis.com>

corhere · 2023-02-21T18:37:05Z

+	if hostConfig != nil && versions.LessThan(version, "1.43") {
+		// Ignore OCIAnnotations because it was added in API v1.43.
+		hostConfig.OCIAnnotations = nil
+	}


I'm following past precedent by ignoring the field on create for older API versions but unconditionally including it in the /containers/{}/json response for all API versions.

thaJeztah

("requesting changes" just to have a quick check per my comment - changes otherwise look good to me)

thaJeztah · 2023-02-22T12:54:55Z

+	VolumeDriver    string            // Name of the volume driver used to mount volumes
+	VolumesFrom     []string          // List of volumes to take from other container
+	ConsoleSize     [2]uint           // Initial console size (height,width)
+	OCIAnnotations  map[string]string `json:",omitempty"` // Annotations to pass along to OCI runtime


Before we merge; quick check on the naming. Wondering if OCI prefix is needed in our API here. I get where we're coming from; mostly thinking along the lines of "(almost) every option we have in our API leads to <something> OCI spec.

So wondering if we should keep the abstraction - call it Annotations, and keep the fact that that (currently) gets passed on to the OCI to the description only.

I'm coming around to dropping the OCI prefix and adopting essentially the same semantics as Kubernetes Annotations. And by that I mean documenting that Engine API clients can also leverage annotations to attach and consume metadata on containers for their own purposes. Perhaps we should also follow their lead and enforce a particular syntax for annotation keys so that the namespace of an annotation is unambiguous. We could always loosen validation later but can't make it more restrictive.

Let's discuss / brainstorm in the call later Today (I also need to get my head around all of that). Quick comment;

Yes, I think Annotations could be useful for "our side" as well

I like namespaces / prefixes as they can definitely help prevent ambiguity (I think I may actually have been the one originally getting them into the "labels" definition 🤔 )

^^^ w.r.t. namespaces: I always keep in the back of my mind that API !== UX; namespaces are awkward to use, but are (in most cases) not meant for humans. So if we decide to use namespaces, we can still make the UX "different" (docker run --foo=bar doing docker run --annotation=awkwardly-long-namespace.option=bar behind the scenes)

What I like about how K8s goes about namespacing annotations is that it makes namespace prefixes optional and reserves the no-namespace "namespace" for users.

Yup, that was part of my original suggestion for labels; no-namespace is for users; don't use them for automation as they can easily conflict. For automation / tools, always use a prefix

For those interested; here's the PR with all the fun (well, there's been multiple before that, but that's the one that made it, and that one is already 200+ comments 🤣)

Proposal: One Meta Data to Rule Them All => Labels #9882

Decisions from today's maintainer call:

Drop the OCI prefix from the field name

Any annotation which is valid per the OCI Runtime spec is permitted

Keys can be any JSON string except for the empty string

Values can be any JSON string, including the empty string

Keys and values can be arbitrarily long

Allow clients to set annotations on a container which will applied to the container's OCI spec. Signed-off-by: Cory Snider <csnider@mirantis.com>

thaJeztah

LGTM, thanks!

thaJeztah · 2023-02-24T13:54:36Z

 	}
+	for k := range hostConfig.Annotations {
+		if k == "" {
+			return errors.Errorf("invalid Annotations: the empty string is not permitted as an annotation key")


I had a look if we should be using an errdefs.InvalidParameter() here (and noticed other errors didn't), but it looks like we wrap any errors returned here on the call sites, such as

moby/daemon/create.go

Lines 65 to 68 in 2f0e308

warnings, err := daemon.verifyContainerSettings(opts.params.HostConfig, opts.params.Config, false)

if err != nil {

return containertypes.CreateResponse{Warnings: warnings}, errdefs.InvalidParameter(err)

}

So in either case; it's good (currently) to not use an errdefs, but we could (should?) look at these at some points to assign error-types in these locations.

thaJeztah · 2023-02-24T13:57:25Z

@cpuguy83 LGTY? (I see there's a "change requested" from you, and didn't want to dismiss it)

AkihiroSuda · 2023-03-24T08:00:07Z

@corhere Do you have a PR for the CLI?

corhere · 2023-03-24T16:10:19Z

@AkihiroSuda no, I am not working on a CLI PR as my motivation is unblocking support for alternative runtimes in cri-dockerd.

AkihiroSuda · 2023-04-03T08:39:41Z

Opened a PR for the CLI:

cli/command/container: implement docker run --annotation docker/cli#4156

corhere mentioned this pull request Feb 17, 2023

Support for alternative runtimes Mirantis/cri-dockerd#111

Closed

corhere force-pushed the oci-annotation-passthru branch from ed04ebe to 9141b45 Compare February 17, 2023 04:36

thaJeztah added the status/1-design-review label Feb 17, 2023

AkihiroSuda requested changes Feb 17, 2023

View reviewed changes

corhere mentioned this pull request Feb 17, 2023

Support alternative runtimes with cri-dockerd #45030

Closed

2 tasks

corhere marked this pull request as draft February 17, 2023 13:50

corhere force-pushed the oci-annotation-passthru branch from 9141b45 to 45a23c1 Compare February 17, 2023 20:11

corhere changed the title ~~daemon: allow setting OCI annotations using container labels~~ daemon: allow setting OCI annotations on containers Feb 17, 2023

corhere force-pushed the oci-annotation-passthru branch from 45a23c1 to a2aa9c3 Compare February 17, 2023 20:30

corhere marked this pull request as ready for review February 17, 2023 20:30

corhere requested review from cpuguy83, neersighted, thaJeztah and vvoland February 17, 2023 20:31

corhere added area/api API area/runtime Runtime impact/api labels Feb 17, 2023

cpuguy83 requested changes Feb 17, 2023

View reviewed changes

docs/api: add v1.43 OpenAPI definitions

50d7164

Signed-off-by: Cory Snider <csnider@mirantis.com>

corhere force-pushed the oci-annotation-passthru branch from a2aa9c3 to 8656719 Compare February 21, 2023 18:34

corhere commented Feb 21, 2023

View reviewed changes

corhere requested review from AkihiroSuda and cpuguy83 February 21, 2023 18:37

neersighted approved these changes Feb 21, 2023

View reviewed changes

vvoland approved these changes Feb 22, 2023

View reviewed changes

AkihiroSuda approved these changes Feb 22, 2023

View reviewed changes

thaJeztah requested changes Feb 22, 2023

View reviewed changes

daemon: add annotations to container HostConfig

0ffaa6c

Allow clients to set annotations on a container which will applied to the container's OCI spec. Signed-off-by: Cory Snider <csnider@mirantis.com>

corhere force-pushed the oci-annotation-passthru branch from 8656719 to 0ffaa6c Compare February 23, 2023 23:59

thaJeztah approved these changes Feb 24, 2023

View reviewed changes

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

thaJeztah added impact/changelog status/2-code-review and removed status/1-design-review labels Feb 24, 2023

cpuguy83 approved these changes Feb 24, 2023

View reviewed changes

cpuguy83 merged commit 0021339 into moby:master Feb 24, 2023

This was referenced Feb 24, 2023

Pass container labels as annotations in OCI spec #39142

Closed

Pass the label of docker container to the annotation of OCI runtime spec #36181

Closed

Add flag annotation #37289

Closed

corhere deleted the oci-annotation-passthru branch February 24, 2023 16:51

AkihiroSuda mentioned this pull request Apr 3, 2023

cli/command/container: implement docker run --annotation docker/cli#4156

Merged

This was referenced Apr 3, 2023

Support setting custom OCI annotations compose-spec/compose-spec#331

Closed

Add annotations compose-spec/compose-spec#332

Merged

thaJeztah mentioned this pull request Jun 13, 2023

api: synchronize api/swagger.yaml with docs/api/v1.43.yaml #45467

Merged

AkihiroSuda mentioned this pull request Mar 31, 2024

Implement nerdctl run --annotation (introduced in Docker v24) containerd/nerdctl#2906

Merged

jiwahn mentioned this pull request May 28, 2026

Request: passing image config labels to containerd container labels #52718

Open

	warnings, err := daemon.verifyContainerSettings(opts.params.HostConfig, opts.params.Config, false)
	if err != nil {
	return containertypes.CreateResponse{Warnings: warnings}, errdefs.InvalidParameter(err)
	}

Uh oh!

Conversation

corhere commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Feb 17, 2023

Uh oh!

corhere commented Feb 17, 2023

Uh oh!

thaJeztah commented Feb 17, 2023

Uh oh!

AkihiroSuda left a comment • 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!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 22, 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!

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!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 24, 2023

Uh oh!

AkihiroSuda commented Mar 24, 2023

Uh oh!

corhere commented Mar 24, 2023

Uh oh!

AkihiroSuda commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

corhere commented Feb 17, 2023 •

edited

Loading

AkihiroSuda left a comment •

edited

Loading

thaJeztah Feb 22, 2023 •

edited

Loading