chown cgroup to process uid in container namespace

[frasertweedale]

Delegating cgroups to the container enables more complex workloads,
including systemd-based workloads. The OCI runtime-spec was
recently updated to explicitly admit such delegation, through
specification of cgroup ownership semantics:

opencontainers/runtime-spec#1123

Pursuant to the updated OCI runtime-spec, change the ownership of
the container's cgroup directory and particular files therein, when
using cgroups v2 and when the cgroupfs is to be mounted read/write.

As a result of this change, systemd workloads can run in isolated
user namespaces on OpenShift when the sandbox's cgroupfs is mounted
read/write.

It might be possible to implement this feature in other cgroup
managers, but that work is deferred.

[kolyshkin]

In general this kinda makes sense, although I'm afraid systemd might overwrite that at some random moment.

This also needs test(s).

[frasertweedale]

In general this kinda makes sense, although I'm afraid systemd might overwrite that at some random moment.

This also needs test(s).

You can use systemd transient unit API to set a different user. Unfortunately, the uid needs to
have a corresponding passwd entity or else systemd refuses to proceed.

I filed an issue against systemd to relax this requirement. It was provisionally rejected:
systemd/systemd#19781. Another approach is to ship an NSSwitch
module to synthesise passwd entites for UIDs in recognised subuid ranges. But with changes
to support subuid ranges themselves coming from NSSwitch, I don't think it's a good idea
(right now). See this blog post for more detail: https://frasertweedale.github.io/blog-redhat/posts/2021-06-09-systemd-cgroups-subuid.html#discussion-and-next-steps .

I would like if we can push back and convince systemd project (i.e. Lennart) to just relax the checks
in systemd. Then runc could achieve correct cgroup ownership via systemd transient unit API. But I
don't think that can be hoped for.

[AkihiroSuda]

For security reason, the default behavior should not be changed.
We can probably have a feature gate as an annotation

https://github.com/containers/crun/blob/main/crun.1.md#extensions-to-oci

https://github.com/opencontainers/runc/blob/master/docs/systemd.md#auxiliary-properties

[frasertweedale]

For security reason, the default behavior should not be changed.
We can probably have a feature gate as an annotation

https://github.com/containers/crun/blob/main/crun.1.md#extensions-to-oci

https://github.com/opencontainers/runc/blob/master/docs/systemd.md#auxiliary-properties

User namespaces is already behind an annotation (io.kubernetes.cri-o.userns-mode). And user namespaces are somewhat hobbled without the ability to manage cgroups (for the workloads that I care about anyway). So, do we really need a separate annotation to gate this feature? I don't mind if the answer is yes, but something to consider...

[AkihiroSuda]

I'm talking about OCI Runtime Spec annotations, not about Kubernetes annotations.

CRI-O may set the suggest OCI Runtime Spec annotation by default at their own risk.

[frasertweedale]

@AkihiroSuda thanks for the clarification. Fair enough. I'll gate it. I'll try to make all the suggested changes next week.

[frasertweedale]

@AkihiroSuda what would be an appropriate key/value for the annotation? "org.opencontainers.runc.chown-cgroup": "{true,false}"

[AkihiroSuda]

"org.opencontainers.runc.chown-cgroup": "{true,false}"

SGTM, but in the long term we should have proper JSON entry in OCI Runtime Spec https://github.com/opencontainers/runtime-spec

[frasertweedale]

"org.opencontainers.runc.chown-cgroup": "{true,false}"

SGTM, but in the long term we should have proper JSON entry in OCI Runtime Spec https://github.com/opencontainers/runtime-spec

Better would be to adjust the semantics such that the container process can create scopes/slices in its own cgroup, and manage its child processes/threads. But that is a discussion to be had over there, not here :)

[frasertweedale]

Thanks @kolyshkin and @AkihiroSuda for your reviews. I implemented all of your requested changes, and added a test.

By the way, for testing with OpenShift, do not use 4.8.0, there is a regression in CRI-O 1.21 that prevents k8s annotations from being propagated to the OCI configuration. CRI-O 1.20 (OpenShift 4.7.x) is fine.

[frasertweedale]

Ping @kolyshkin and @AkihiroSuda - are you able to re-review this PR soon?

[kolyshkin]

Overall looks good and ready; only left a single suggestion.

[kolyshkin]

LGTM

[kolyshkin]

LGTM

[frasertweedale]

Thanks to all who have reviewed this PR and given feedback. We need more approvals and then someone can press the merge button. Ping @cyphar, @AkihiroSuda, @giuseppe, @jfroy.

[frasertweedale]

Any takers? @mrunalp?

[cyphar]

LGTM.

[frasertweedale]

Many thanks, @cyphar and all reviewers.

[frasertweedale]

I created #3311 to backport this feature to the release-1.0 branch.