set annotations when creating sanbodx and containers by cncal · Pull Request #369 · Mirantis/cri-dockerd

cncal · 2024-05-27T08:52:25Z

Proposed Changes

Docker now allows client to set annotaion when creating sandbox and containers that is significant for some low-level runtimes(e.g. kata-containers) as they use it to determine whether the creating is a pod_sandbox or a pod_container.

Docker now allows client to set annotaion when creating sandbox and containers that is significant for some low-level runtimes(e.g. kata-containers) as they use it to determine whether the creating is a pod_sandbox or a pod_container. Signed-off-by: cncal <flycalvin@qq.com>

cncal · 2024-05-27T08:56:54Z

@corhere @nwneisen PTAL.

corhere · 2024-05-27T18:36:29Z

core/convert.go

+	// TODO: Currently docker ContainerList api doesn't return annotations, so annotations
+	//       still need to be extracted from labels here.


I'm not super comfortable with having different sources of truth for annotations depending on the CRI API endpoint queried. It would be straightforward to extend the ContainerList API to return container annotations; perhaps we should do that first so support can be added properly to cri-dockerd?

Actually I'm working on it moby/moby#47866. May I mark this PR a WIP util docker-side supports it ?

moby/moby#47866 has been merged into master branch, but released in Pre-release https://github.com/moby/moby/releases/tag/v27.0.0-rc.1. May I bump github.com/docker/docker from 26.1.2+incompatible to v27.0.0-rc.1+incompatible? @corhere

corhere · 2024-05-27T18:38:18Z

core/container_create.go

+	// TODO: Docker supports annotations for now, so labels and annotations might be separated
+	//       in the future. Keeping them merged is just for backward compatibility.


Backward compatibility with what? AIUI no containers will be persisted across an upgrade (daemon restart) so there is no need for containers started by cri-dockerd v0.3.x to be understandable by v0.4.x, or vice versa.

There are two main considerations for backward compatibility:

I‘m not sure if some runtimes use the label to make some judgments

Docker ContainerList api doesn't return annotations, so keeping them merged is needed for later extraction.

AkihiroSuda

Seems vulnerable to container breakout attacks via systemd properties

Same as GHSA-2cgq-h8xw-2v5j

containerd/CRI needs an explicit allow list for specifying OCI annotations
https://github.com/containerd/containerd/blob/v1.7.17/docs/cri/config.md?plain=1#L310

cncal · 2024-05-28T06:19:11Z

@AkihiroSuda Yes, I noticed that, but docker does not support setting up pod_annotations or container_annotations for runtime. Also, filetering annotations should be supported at docker side or cri-dockerd？

AkihiroSuda · 2024-05-28T06:46:52Z

@AkihiroSuda Yes, I noticed that, but docker does not support setting up pod_annotations or container_annotations for runtime.

So, cri-dockerd will have to implement the allow list, or at least, check potentiallyUnsafeConfigAnnotations https://github.com/opencontainers/runc/blob/v1.2.0-rc.1/features.go#L67-L71

Also, filetering annotations should be supported at docker side or cri-dockerd？

cri-dockerd side.

cncal · 2024-05-28T07:28:44Z

How about adding potentiallyUnsafeConfigAnnotations as a cri-dockerd flag used to filter annotations from k8s ?

AkihiroSuda · 2024-05-28T07:33:12Z

How about adding potentiallyUnsafeConfigAnnotations as a cri-dockerd flag used to filter annotations from k8s ?

Yes, cri-dockerd may run runc features and check the potentiallyUnsafeConfigAnnotations field to filter unsafe annotations that are passed via Kubernetes annotations.
This needs runc v1.2, which is still RC, though.

cncal · 2024-05-28T08:31:36Z

Yes, cri-dockerd may run runc features and check the potentiallyUnsafeConfigAnnotations field to filter unsafe annotations that are passed via Kubernetes annotations.

Sorry, I don't get it. You mean we get the RuntimeHandler(e.g. runc) from k8s request at first and run $runtime features command to fetch the potentiallyUnsafeConfigAnnotations value? What if RuntimeHandler is user-defined(may be runc with some special options named runc-1) ? As far as know, kata and gvisor do not have features subcommand for now.

AkihiroSuda · 2024-05-28T08:40:34Z

Yes, cri-dockerd may run runc features and check the potentiallyUnsafeConfigAnnotations field to filter unsafe annotations that are passed via Kubernetes annotations.

Sorry, I don't get it. You mean we get the RuntimeHandler(e.g. runc) from k8s request at first and run $runtime features command to fetch the potentiallyUnsafeConfigAnnotations value?

Right. $runtime features can be invoked before handling Kubernetes requests though.

What if RuntimeHandler is user-defined(may be runc with some special options named runc-1) ? As far as know, kata and gvisor do not have features subcommand for now.

Every annotation should be assumed unsafe, and should not be propagated in this case.

cncal · 2024-05-28T08:50:23Z

Yes, cri-dockerd may run runc features and check the potentiallyUnsafeConfigAnnotations field to filter unsafe annotations that are passed via Kubernetes annotations.

Sorry, I don't get it. You mean we get the RuntimeHandler(e.g. runc) from k8s request at first and run $runtime features command to fetch the potentiallyUnsafeConfigAnnotations value?

Right. $runtime features can be invoked before handling Kubernetes requests though.

What if RuntimeHandler is user-defined(may be runc with some special options named runc-1) ? As far as know, kata and gvisor do not have features subcommand for now.

Every annotation should be assumed unsafe, and should not be propagated in this case.

@corhere @nwneisen Any suggestions?

corhere · 2024-06-06T20:15:10Z

Runtime features are surfaced in the Info API, keyed by the runtime name. moby/moby#46647

cncal · 2024-06-07T02:48:15Z

Runtime features are surfaced in the Info API, keyed by the runtime name. moby/moby#46647

Thanks @corhere , it sounds good.

corhere reviewed May 27, 2024

View reviewed changes

AkihiroSuda suggested changes May 28, 2024

View reviewed changes

cncal marked this pull request as draft June 9, 2024 08:21

		// TODO: Currently docker ContainerList api doesn't return annotations, so annotations
		// still need to be extracted from labels here.

		// TODO: Docker supports annotations for now, so labels and annotations might be separated
		// in the future. Keeping them merged is just for backward compatibility.

Conversation

cncal commented May 27, 2024

Proposed Changes

Uh oh!

cncal commented May 27, 2024

Uh oh!

corhere May 27, 2024

Choose a reason for hiding this comment

Uh oh!

cncal May 28, 2024 • 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!

cncal Jun 14, 2024

Choose a reason for hiding this comment

Uh oh!

corhere May 27, 2024

Choose a reason for hiding this comment

Uh oh!

cncal May 28, 2024

Choose a reason for hiding this comment

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!

cncal commented May 28, 2024

Uh oh!

AkihiroSuda commented May 28, 2024

Uh oh!

cncal commented May 28, 2024

Uh oh!

AkihiroSuda commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cncal commented May 28, 2024

Uh oh!

AkihiroSuda commented May 28, 2024

Uh oh!

cncal commented May 28, 2024

Uh oh!

corhere commented Jun 6, 2024

Uh oh!

cncal commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cncal May 28, 2024 •

edited

Loading

AkihiroSuda left a comment •

edited

Loading

AkihiroSuda commented May 28, 2024 •

edited

Loading