namespaces: by default create cgroupns on cgroups v2 by giuseppe · Pull Request #4374 · containers/podman

giuseppe · 2019-10-30T07:51:23Z

change the default on cgroups v2 and create a new cgroup namespace.

When a cgroup namespace is used, processes inside the namespace are
only able to see cgroup paths relative to the cgroup namespace root
and not have full visibility on all the cgroups present on the
system.

The previous behaviour is maintained on a cgroups v1 host, where a
cgroup namespace is not created by default.

Closes: #4363

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

openshift-ci-robot · 2019-10-30T07:51:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

giuseppe · 2019-10-30T10:38:00Z

tests is failing with: [+0318s] Error: could not get runtime: please update to v2.0.1 or later: outdated conmon version

vrothberg · 2019-10-30T10:51:50Z

That's very curious. We merged the PR yesterday and all checks were green -> https://github.com/containers/libpod/pull/3792/checks

vrothberg · 2019-10-30T10:55:13Z

@cevich @haircommander PTAL

haircommander · 2019-10-30T12:31:24Z

So I had to manually push a version of the in_podman image that updated conmon. The in_podman image is updated as a post merge task on every PR. It seems any PR that isn't rebased to the new Dockerfile (with the new conmon) updates this image, and breaks subsequent PRs. I am not sure if there is a better work around other than rebasing ever live PR, and pushing the image manually until it's fixed... wdyt @cevich

rhatdan · 2019-10-30T13:05:31Z

LGTM

cevich · 2019-10-30T20:27:42Z

and pushing the image manually until it's fixed... wdyt @cevich

That was my bad, totally forgot that quay.io was set to re-build that image from Dockerfile.fedora instead of Dockerfile. That's fixed now and in_podman tests are passing with it.

rhatdan · 2019-10-31T13:42:45Z

@vrothberg @mheon @baude @TomSweeneyRedHat @QiWang19 @jwhonce Tests are passing, can we get this merged.

mheon · 2019-10-31T13:45:02Z

cmd/podman/common.go

Can't we dynamically set the default like we do for a few other things? A bit less ugly than special-casing the empty string

I was thinking about that, but one potential issue would be, if we default the CLI based on when the container was created, and the user switched cgroups, it could cause issues with existing containers.

Existing containers are an issue either way - this is all done at spec generation time, it'll be in the final, immutable spec that we put in the DB

I'd prefer we don't add this logic to the cmd/ package if possible.

Also, we need to be able to catch errors in the cgroups v2 detection. If we move it here, we would need to either ignore the error or panic.

rh-atomic-bot · 2019-10-31T18:39:58Z

☔ The latest upstream changes (presumably #4354) made this pull request unmergeable. Please resolve the merge conflicts.

giuseppe · 2019-11-05T07:43:09Z

rebased

AkihiroSuda · 2019-11-05T10:44:55Z

What should be the default value for --privileged mode?
I think private is ok, but some people may expect host for privileged containers? (Is there any such use-case?)

giuseppe · 2019-11-05T10:55:14Z

I think private is ok, but some people may expect host for privileged containers? (Is there any such use-case?)

Good point! I think we should keep --cgroupns=host when --privileged is used. Privileged containers are expected to have full control on the host. I've changed the patch to address that.

AkihiroSuda · 2019-11-05T11:06:45Z

Privileged containers are expected to have full control on the host.

True, but privileged containers still unshare other namespaces except User Namespace?
So I think it makes more sense to use private ns for privileged containers, unless there is specific usecase to require host ns

rhatdan · 2019-11-05T14:25:07Z

I agree with @AkihiroSuda. --privileged should not modify the default behaviour of namespaces.
If a user wants to work on the host cgroupns, then they have to set the option.

change the default on cgroups v2 and create a new cgroup namespace. When a cgroup namespace is used, processes inside the namespace are only able to see cgroup paths relative to the cgroup namespace root and not have full visibility on all the cgroups present on the system. The previous behaviour is maintained on a cgroups v1 host, where a cgroup namespace is not created by default. Closes: containers#4363 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2019-11-05T16:29:53Z

I agree with @AkihiroSuda. --privileged should not modify the default behaviour of namespaces.
If a user wants to work on the host cgroupns, then they have to set the option.

ok I've reverted the change

rhatdan · 2019-11-05T19:16:09Z

LGTM

rhatdan · 2019-11-05T19:16:30Z

@AkihiroSuda @mheon @vrothberg @TomSweeneyRedHat @QiWang19 @baude PTAL

mheon · 2019-11-05T19:23:07Z

/lgtm

For cgroup v1, we were unable to change the default because of compatibility issue. For cgroup v2, we should change the default right now because switching to cgroup v2 is already breaking change. See also containers/podman#4363 containers/podman#4374 Privileged containers also use cgroupns=private by default. containers/podman#4374 (comment) Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

In cgroup v1 container implementations, cgroupns is not used by default because it was not available in the kernel until kernel 4.6 (May 2016), and the default behavior will not change on cgroup v1 environments, because changing the default will break compatibility and surprise users. For cgroup v2, implementations are going to unshare cgroupns by default so as to hide /sys/fs/cgroup from containers. * Discussion: containers/podman#4363 * Podman PR (merged): containers/podman#4374 * Moby PR: moby/moby#40174 This PR enables cgroupns for containers, but pod sandboxes are untouched because probably there is no need to do. Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

For cgroup v1, we were unable to change the default because of compatibility issue. For cgroup v2, we should change the default right now because switching to cgroup v2 is already breaking change. See also containers/podman#4363 containers/podman#4374 Privileged containers also use cgroupns=private by default. containers/podman#4374 (comment) Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp> Upstream-commit: 19baeaca267d5710907ac1b3c3972d44725fe8ad Component: engine

openshift-ci-robot requested review from baude and rhatdan October 30, 2019 07:51

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S labels Oct 30, 2019

giuseppe mentioned this pull request Oct 30, 2019

cgroupns=private should be enabled by default for unified mode? #4363

Closed

giuseppe force-pushed the create-cgroupns-by-default-on-cgroupsv2 branch from 439b243 to 245dc68 Compare October 30, 2019 08:25

openshift-ci-robot added size/M and removed size/S labels Oct 30, 2019

giuseppe force-pushed the create-cgroupns-by-default-on-cgroupsv2 branch from 245dc68 to 9eeb4e6 Compare October 30, 2019 09:08

mheon reviewed Oct 31, 2019

View reviewed changes

giuseppe force-pushed the create-cgroupns-by-default-on-cgroupsv2 branch from 9eeb4e6 to 1ba9902 Compare November 5, 2019 07:42

giuseppe force-pushed the create-cgroupns-by-default-on-cgroupsv2 branch from 1ba9902 to 8a4ee14 Compare November 5, 2019 10:53

giuseppe force-pushed the create-cgroupns-by-default-on-cgroupsv2 branch from 8a4ee14 to b8514ca Compare November 5, 2019 16:29

openshift-ci-robot assigned mheon Nov 5, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2019

openshift-merge-robot merged commit 7eda1b0 into containers:master Nov 5, 2019

rh-atomic-bot mentioned this pull request Nov 5, 2019

Split up create config handling of namespaces and security #4265

Merged

5 tasks

AkihiroSuda mentioned this pull request Jan 9, 2020

cgroup2: unshare cgroup namespace for containers containerd/cri#1371

Merged

rgulewich mentioned this pull request Apr 22, 2020

support --privileged --cgroupns=private on cgroup v1 moby/moby#40788

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Conversation

giuseppe commented Oct 30, 2019

Uh oh!

openshift-ci-robot commented Oct 30, 2019

Uh oh!

giuseppe commented Oct 30, 2019

Uh oh!

vrothberg commented Oct 30, 2019

Uh oh!

vrothberg commented Oct 30, 2019

Uh oh!

haircommander commented Oct 30, 2019

Uh oh!

rhatdan commented Oct 30, 2019

Uh oh!

cevich commented Oct 30, 2019

Uh oh!

rhatdan commented Oct 31, 2019

Uh oh!

mheon Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

rhatdan Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

mheon Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

giuseppe Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

rh-atomic-bot commented Oct 31, 2019

Uh oh!

giuseppe commented Nov 5, 2019

Uh oh!

AkihiroSuda commented Nov 5, 2019

Uh oh!

giuseppe commented Nov 5, 2019

Uh oh!

AkihiroSuda commented Nov 5, 2019

Uh oh!

rhatdan commented Nov 5, 2019

Uh oh!

giuseppe commented Nov 5, 2019

Uh oh!

rhatdan commented Nov 5, 2019

Uh oh!

rhatdan commented Nov 5, 2019

Uh oh!

mheon commented Nov 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants