daemon: allow setting OCI annotations on containers #45025

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)

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

api/server/router/container/container_routes.go

+	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

api/types/container/hostconfig.go

+	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

daemon/container.go

@@ -305,6 +305,11 @@ func validateHostConfig(hostConfig *containertypes.HostConfig) error {
 	if !hostConfig.Isolation.IsValid() {
 		return errors.Errorf("invalid isolation '%s' on %s", hostConfig.Isolation, runtime.GOOS)
 	}
+	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

This comment was marked as outdated.

Sign in to view

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

This comment was marked as outdated.

Sign in to view

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 thaJeztah, vvoland, neersighted and cpuguy83 February 17, 2023 20:31

corhere added area/api area/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 cpuguy83 and AkihiroSuda 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: allow setting OCI annotations on containers #45025

daemon: allow setting OCI annotations on containers #45025

corhere commented Feb 17, 2023 •

edited

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

AkihiroSuda left a comment •

edited

cpuguy83 left a comment

corhere Feb 21, 2023

thaJeztah left a comment

thaJeztah Feb 22, 2023 •

edited

corhere Feb 23, 2023

thaJeztah Feb 23, 2023

corhere Feb 23, 2023

thaJeztah Feb 23, 2023

thaJeztah Feb 23, 2023

corhere Feb 23, 2023

thaJeztah left a comment

thaJeztah Feb 24, 2023

thaJeztah commented Feb 24, 2023

AkihiroSuda commented Mar 24, 2023

corhere commented Mar 24, 2023

AkihiroSuda commented Apr 3, 2023

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

daemon: allow setting OCI annotations on containers #45025

daemon: allow setting OCI annotations on containers #45025

Conversation

corhere commented Feb 17, 2023 • edited

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

AkihiroSuda left a comment • edited

Choose a reason for hiding this comment

cpuguy83 left a 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

thaJeztah Feb 22, 2023 • edited

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

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Feb 24, 2023

AkihiroSuda commented Mar 24, 2023

corhere commented Mar 24, 2023

AkihiroSuda commented Apr 3, 2023

corhere commented Feb 17, 2023 •

edited

AkihiroSuda left a comment •

edited

thaJeztah Feb 22, 2023 •

edited