User ns reworked #4

mauriciovasquezbernal · 2020-06-24T19:42:41Z

It's a reworked version of #3.

rata

I did some silly comments.

I'll need to review with more time the changes for the kuberuntime. Is tomorrow okay? :)

pkg/kubelet/kubelet_usernamespace_linux.go

pkg/kubelet/container/runtime.go

GetRuntimeConfigInfo returns information about the configuration of the runtime. For now it only returns the uid/gid mappings configuration.

Implement a new helper that will be used in the following commits

Extend the CRI to include a new user namespace mode field to allow kubelet to specify what is the prefered user namespace mode for a given pod.

This commit implements support for user ns in the kubelet. The kubelet uses the GetRuntimeInfoConfig function of the runtime to query for the uid/gid configured mapping. Kubelet tries to use POD mode for the user namespace when possible, NODE is used when: - Feature is not supported nor enabled in the runtime - The value of the "alpha.kinvolk.io/userns" annotation is "node" - The pod specification is imcompatible with it -- Any host namespace is used (IPC, PID, NET) -- There is any host-path volume -- There is any non namespaced capability (MKNOD, SYS_TIME, SYS_MODULE) -- There is any privileged container -- The pod has PVC mounts Files under the pod volumes dir (/var/lib/kubelet/pods/xxxx/volumes) are chowned to the mapped user in the host if the user namespace is used.

rata

@mauriciovasquezbernal thanks a lot for the patch, looks very good!

Added some silly questions for some things I didn't follow and wanted to confirm if we want that behavior, but overall LGTM! :)

rata · 2020-07-06T19:46:06Z

pkg/kubelet/container/runtime.go

+		return false
+	}
+	if len(c.UserNamespaceConfig.UidMappings) == 1 &&
+		c.UserNamespaceConfig.UidMappings[0].HostID == uint32(0) && c.UserNamespaceConfig.UidMappings[0].Size == uint32(4294967295) {


nit picking: is the uint32() needed? I think we shouldn't need it. Or am I missing something?

I mean, golang does force to use some explicit conversions, but as explained here I don't think it should be needed to compare a var with a constant.

Am I missing something?

You're right. I'll remove them.

rata · 2020-07-06T19:49:20Z

pkg/kubelet/container/runtime.go

+		return false
+	}
+	if len(c.UserNamespaceConfig.UidMappings) == 1 &&
+		c.UserNamespaceConfig.UidMappings[0].HostID == uint32(0) && c.UserNamespaceConfig.UidMappings[0].Size == uint32(4294967295) {


why c.UserNamespaceConfig.UidMappings[0].Size == uint32(4294967295)? I mean, why equal to that constant value? It is set to that when the runtime doesn't support user ns?

4294967295 encapsulates the entire uid/gid range I believe. The initial root namespace has a uid_map of 0 0 4294967295. But I agree that the use of it here is unclear

Yes, the idea is that if the configured mapping is 0 0 4294967295 it means that the support is not enabled (all the ids in the container are mapped to the same ids in the host).

Ohh, I see. Thanks! Maybe a comment can make it very clear to everyone :)

rata · 2020-07-06T19:52:08Z

pkg/kubelet/container/runtime.go

+		return false
+	}
+	if len(c.UserNamespaceConfig.UidMappings) == 1 &&
+		c.UserNamespaceConfig.UidMappings[0].HostID == uint32(0) && c.UserNamespaceConfig.UidMappings[0].Size == uint32(0) {


Idem regarding uint32(). Is it needed?

rata · 2020-07-06T19:57:17Z

pkg/kubelet/container/helpers_test.go

+	single := &RuntimeConfigInfo{
+		UserNamespaceConfig: UserNamespaceConfigInfo{
+			UidMappings: []*UserNSMapping{
+				&UserNSMapping{


Maybe add a test with more than one mapping too, given that this is a slice?

rata · 2020-07-06T19:57:34Z

pkg/kubelet/container/runtime.go

+	if len(c.UserNamespaceConfig.UidMappings) == 1 &&
+		c.UserNamespaceConfig.UidMappings[0].HostID == uint32(0) && c.UserNamespaceConfig.UidMappings[0].Size == uint32(0) {


Why if len(...) == 1 and hostID == 0 and size == 0? Not sure I follow what you are trying to detect.

Is it just when the size is 0? Why does it matter, in that case, that the hostID is 0 too? Or why wouldn't it be a problem if the specified mappings are more than one?

It's trying to detect the case where a mapping like 0 1000 0 is configured. You're totally right, the whole logic to check if the usernamespace is supported or enabled has to be checked again. I think it could be simplified a lot but I avoided touching that at this time.

Makes sense

pkg/kubelet/kuberuntime/kuberuntime_manager.go

rata · 2020-07-06T20:45:05Z

pkg/kubelet/dockershim/docker_service.go

+		gidMapping.Size_ = gidMappingSize
+	} else {
+		uidMapping.Size_ = uint32(4294967295)
+		gidMapping.Size_ = uint32(4294967295)


What is 4294967295? The max value for something? Is it constant across platforms?

rata · 2020-07-06T20:46:27Z

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2/api.proto

+   // container_id is the starting id for the mapping inside the container.
+   uint32 container_id = 1;
+   // host_id is the starting id for the mapping on the host.
+   uint32 host_id = 2;


Why use uint32 for container and host UIDs? Is it because in all platforms will be 32 or less?

I think we talked about this in the past, about being maybe an unsigned int in C, but can't find that thread now. Sorry to re-ask :-/

rata · 2020-07-06T20:51:32Z

pkg/kubelet/kuberuntime/kuberuntime_manager_test.go

+			&kubecontainer.RuntimeConfigInfo{},
+			false,
+		},
+		"Single mapping": {


Idem, will be nice to test more than once mapping, just in case

rata · 2020-07-06T20:55:11Z

pkg/kubelet/kuberuntime/kuberuntime_manager.go

@@ -24,6 +24,8 @@ import (
 	"time"

 	cadvisorapi "github.com/google/cadvisor/info/v1"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"


status and codes can be too generic and, in the future, clash with other imports. Maybe should we use a name that makes it clear that is a grpc specific thing?

Not now, but in the future. Ah, and only if this codes continues to live here :)

cdesiniotis

Hi! Overall this is really good work. I left a few comments/questions.

cdesiniotis · 2020-07-06T23:09:47Z

pkg/kubelet/container/runtime.go

+	if len(c.UserNamespaceConfig.UidMappings) == 1 &&
+		c.UserNamespaceConfig.UidMappings[0].HostID == uint32(0) && c.UserNamespaceConfig.UidMappings[0].Size == uint32(4294967295) {


I think this would be more clear.

Suggested change

if len(c.UserNamespaceConfig.UidMappings) == 1 &&

c.UserNamespaceConfig.UidMappings[0].HostID == uint32(0) && c.UserNamespaceConfig.UidMappings[0].Size == uint32(4294967295) {

if len(c.UserNamespaceConfig.UidMappings) == 1 &&

c.UserNamespaceConfig.UidMappings[0].HostID == c.UserNamespaceConfig.UidMappings[0].ContainerID {

cdesiniotis · 2020-07-06T23:13:45Z

pkg/kubelet/container/runtime.go

+		return false
+	}
+	if len(c.UserNamespaceConfig.UidMappings) == 1 &&
+		c.UserNamespaceConfig.UidMappings[0].HostID == uint32(0) && c.UserNamespaceConfig.UidMappings[0].Size == uint32(4294967295) {


4294967295 encapsulates the entire uid/gid range I believe. The initial root namespace has a uid_map of 0 0 4294967295. But I agree that the use of it here is unclear

cdesiniotis · 2020-07-06T23:24:03Z

pkg/kubelet/dockershim/docker_service.go

+
+// getRemappedNonRootHostID parses docker info to determine ID on the host usernamespace which is mapped to {U/G}ID 0 in the container user-namespace
+func getRemappedNonRootHostID(dockerInfo *dockertypes.Info) (uint32, error) {
+	remappedNonRootHostID64, err := strconv.ParseUint(strings.Split(path.Base(dockerInfo.DockerRootDir), ".")[0], 10, 0)


The output of path.Base(. . .) is /var/lib/docker if userns is not enabled, and /var/lib/docker/1000.1000 if userns is enabled (replace 1000 with whatever uid remapping is configured). I agree this is a little hard to review, but it works :)

cdesiniotis · 2020-07-06T23:42:17Z

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1alpha2/api.proto

+    // User namespace for this container/sandbox.
+    // Note: There is currently no way to set CONTAINER scoped user namespace in the Kubernetes API.
+    // NODE is the default value. Kubelet will set it to POD if pod spec indicates to use user-namespace remapping
+    // Namespaces currently set by the kubelet: POD, NODE


Does POD mean that all pods will be remapped to the same namespace (assuming userns are supported/enabled at runtime)? If so, I think the term POD is unclear as it suggests that each pod gets mapped to its own namespace. What happened to NODE_WIDE_REMAPPED?

Pod means each pod will have it's own namespace. The mapping, at least in this PoC, is the same for all pods, though.

NODE_WIDE_REMAPPED is not implemented here. We find out it was easier to implement a per-pod user ns, that also seems better. IIUC, NODE_WIDE_REMAPPED was proposed years ago before most container runtime added support for user ns and as a way to run containers as root but not be root on the host. We can implement that if there is still value, though :)

Thanks for the clarification. I don't think NODE_WIDE_REMAPPED is necessary. But maybe a comment here clarifying what POD means (and what it will mean in the future) would be helpful.

cdesiniotis · 2020-07-06T23:48:35Z

pkg/kubelet/kubelet.go

-		makeIPTablesUtilChains:                  kubeCfg.MakeIPTablesUtilChains,
-		iptablesMasqueradeBit:                   int(kubeCfg.IPTablesMasqueradeBit),
-		iptablesDropBit:                         int(kubeCfg.IPTablesDropBit),
-		experimentalHostUserNamespaceDefaulting: utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalHostUserNamespaceDefaultingGate),


Is there a reason why this entire sections shows as a diff? I believe you are just removing the experimentalHostUserNamespaceDefaulting feature

cdesiniotis · 2020-07-06T23:50:29Z

pkg/kubelet/kubelet_pods.go

@@ -70,6 +70,18 @@ import (
 const (
 	managedHostsHeader                = "# Kubernetes-managed hosts file.\n"
 	managedHostsHeaderWithHostNetwork = "# Kubernetes-managed hosts file (host network).\n"
+
+	// Kinvolk alpha annotation for user namespaces
+	kivolkUsernsAnn = "alpha.kinvolk.io/userns"


Is using an annotation a temporary fix? What's the reasoning behind not updating the pod spec?

We are using an annotation to easily test on real clusters without changing the pod.Spec. Upstream PR will change the pod spec.

cdesiniotis · 2020-07-06T23:51:54Z

pkg/kubelet/kubelet_pods.go

@@ -1798,3 +1805,52 @@ func (kl *Kubelet) hasHostMountPVC(pod *v1.Pod) bool {
 	}
 	return false
 }
+


nit: add comment for UserNamespaceForPod() for consistency

rata · 2020-07-07T13:48:12Z

Hi! Overall this is really good work. I left a few comments/questions.

thanks for the review and your answers, really helpful! :)

cdesiniotis · 2020-07-07T16:30:12Z

Is this expected to work with dockershim at the moment?

mauriciovasquezbernal · 2020-07-07T17:55:59Z

Yes, it should work when using the userns-remap option in docker https://docs.docker.com/engine/security/userns-remap/.

cdesiniotis · 2020-07-07T18:34:47Z

Hmm okay. I get permission issues when kubelet attempts to mount from /var/lib/pods/ to /var/lib/docker/231072.231072/. I am using Docker 19.03 (and the latest versions of containderd/runc that ship with it). Is there something I am missing with my setup?

mauriciovasquezbernal · 2020-07-07T20:42:50Z

@cdesiniotis I have the following:

$ dockerd --version
Docker version 19.03.9, build 9d988398e7
 $ sudo cat /etc/subuid
mvb:500000:65536
$ sudo cat /etc/subgid
mvb:500000:65536
$ sudo dockerd --userns-remap="mvb:mvb"

Can you share the exact error you're getting?

cdesiniotis · 2020-07-07T21:40:03Z

@mauriciovasquezbernal My setup:

$ dockerd --version
Docker version 19.03.12, build 48a66213fe
$ sudo cat /etc/subuid
dockremap:231072:65536
$ sudo cat /etc/subgid
dockremap:231072:65536
$ sudo dockerd --userns-remap="default"

I run some pods. When I specify to use the pod userns it fails:

$ cluster/kubectl.sh apply -f nodens.yaml
pod/node-userns created
$ cluster/kubectl.sh get pods
NAME          READY   STATUS    RESTARTS   AGE
node-userns   1/1     Running   0          4s
$ cluster/kubectl.sh apply -f podns.yaml
pod/pod-userns created
$ cluster/kubectl.sh get pods
NAME          READY   STATUS               RESTARTS   AGE
node-userns   1/1     Running              0          22s
pod-userns    0/1     ContainerCannotRun   0          4s
$ cluster/kubectl.sh apply -f default.yaml
pod/default created
$ cluster/kubectl.sh get pods
NAME          READY   STATUS               RESTARTS   AGE
node-userns   1/1     Running              0          38s
pod-userns    0/1     ContainerCannotRun   0          20s
default       0/1     ContainerCannotRun   0          2s

Looking in the kubelet logs at /tmp/kubelet.log I see error messages like the following for the pods that fail:

E0707 21:17:01.437863   15695 remote_runtime.go:222] StartContainer "b48e2e5bc448b58b43a6eed9488e418b3d4753b26303688790e790c3ce2fae88" from runtime service failed: rpc error: code = Unknown desc = failed to star
t container "b48e2e5bc448b58b43a6eed9488e418b3d4753b26303688790e790c3ce2fae88": Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "process_linux.go:
449: container init caused \"rootfs_linux.go:58: mounting \\\"/var/lib/kubelet/pods/8696e9a0-836c-4539-95c9-3a3ab735dd6c/containers/container1/c1987dfa\\\" to rootfs \\\"/var/lib/docker/231072.231072/overlay2/bb0fe03
d4faab00737d5ed23fb0d6d571af66f8d4c978e346ea8f7871a71e7d9/merged\\\" at \\\"/dev/termination-log\\\" caused \\\"stat /var/lib/kubelet/pods/8696e9a0-836c-4539-95c9-3a3ab735dd6c/containers/container1/c1987dfa: permissi
on denied\\\"\"": unknown

mauriciovasquezbernal · 2020-07-08T00:09:21Z

@cdesiniotis oh right. I suppose you are running containerd with systemd, if that's the case it needs to have the supplemental group 0 (SupplementaryGroups=0 in the systemd unit). More details in opencontainers/runc#2484.

cdesiniotis · 2020-07-08T17:03:47Z

@mauriciovasquezbernal Ah that was it. Thank you for the help!

rata · 2020-07-08T18:40:28Z

pkg/kubelet/dockershim/docker_service.go

@@ -81,6 +85,10 @@ const (
 	// to kubelet behavior and system settings in addition to any API flags that may be introduced.
 )

+var (
+	linuxIDMappingRegexp = regexp.MustCompile("([aA-zZ]+):([0-9]+):([0-9]+)")


It seems that the first field can be numbers too. We probably need to modify this. Or maybe just use Split() with : as separator? Might be simpler, not sure

From https://docs.docker.com/engine/security/userns-remap/#prerequisites:

Each file contains three fields: the username or ID of the user, followed by a beginning UID or GID (which is treated as UID or GID 0 within the namespace) and a maximum number of UIDs or GIDs available to the user

Additionally, a typical accepted regex for usernames in Linux is "^[a-z][-a-z0-9_]*\$", so need to account for numbers in the first field anyways.

mauriciovasquezbernal · 2020-07-08T22:40:01Z

@rata @cdesiniotis I'm checking the state of the art on the proposals for this feature first. I'll address your comments once I have a clear view of what are the next steps on this implementation. Thanks a lot for the review.

cdesiniotis · 2020-07-09T03:01:38Z

@mauriciovasquezbernal Thanks for your work! I am willing to help/review, so feel free to include me for review on this (or related work). Also, what are the current plans for upstream work (i.e. KEP and upstream PR)? Are you (@rata @mauriciovasquezbernal) still planning to lead that effort?

Add two examples with SYS_ADMIN and NET_ADMIN capabilities.

- Differenciate when it's a limitation on the Pod spec or in the runtime - Don't drop the content of the error message shown to the user

There are some features that *could* be not compatible with user namespaces: - hostpath volumes - PVC - sharing other host namespaces - some capabilities The current code forbids to use 'Pod' mode when one of those features is present on the PodSpec. This commit relaxes that logic and replaces it by a warning only for the case where host namespaces are shared given that it's the only case that could present issues when creating the container.

mauriciovasquezbernal force-pushed the mauricio/userns branch from a0ab60f to b18cd2d Compare June 24, 2020 21:31

rata reviewed Jun 24, 2020

View reviewed changes

pkg/kubelet/kubelet_usernamespace_linux.go Outdated Show resolved Hide resolved

pkg/kubelet/container/runtime.go Outdated Show resolved Hide resolved

pkg/kubelet/container/runtime.go Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/userns branch 4 times, most recently from 41c3fe9 to 8992dbc Compare July 2, 2020 20:38

mauriciovasquezbernal added 5 commits July 3, 2020 15:58

CRI: Add GetRuntimeConfigInfo

f4e1796

GetRuntimeConfigInfo returns information about the configuration of the runtime. For now it only returns the uid/gid mappings configuration.

kubelet/container: Implement GetPodVolumesDir

c1386c3

Implement a new helper that will be used in the following commits

CRI: Add user namespace field

d54cddc

Extend the CRI to include a new user namespace mode field to allow kubelet to specify what is the prefered user namespace mode for a given pod.

Add pod examples for user namespace support

31640cb

mauriciovasquezbernal force-pushed the mauricio/userns branch 2 times, most recently from f223792 to 31640cb Compare July 6, 2020 15:30

rata reviewed Jul 6, 2020

View reviewed changes

cdesiniotis reviewed Jul 7, 2020

View reviewed changes

mauriciovasquezbernal mentioned this pull request Jul 7, 2020

[1.3 - DO NOT MERGE] User namespace support containerd/cri#1527

Closed

rata reviewed Jul 8, 2020

View reviewed changes

Add pod examples for capabilities

15111e3

Add two examples with SYS_ADMIN and NET_ADMIN capabilities.

mauriciovasquezbernal force-pushed the mauricio/userns branch from f4ddac6 to 6e902e7 Compare September 3, 2020 13:33

Fix typo: kivolkUsernsAnn -> kinvolkUsernsAnn

ac6a9ac

mauriciovasquezbernal added 2 commits September 3, 2020 08:46

kubelet: Improve error message when user namespaces can't be used

8966f54

- Differenciate when it's a limitation on the Pod spec or in the runtime - Don't drop the content of the error message shown to the user

cmd/kubelet: Add log indicating if compiled with user namespaces support

b14dc20

mauriciovasquezbernal force-pushed the mauricio/userns branch from 6e902e7 to b14dc20 Compare September 3, 2020 13:53

mauriciovasquezbernal closed this Jan 12, 2023

		if len(c.UserNamespaceConfig.UidMappings) == 1 &&
		c.UserNamespaceConfig.UidMappings[0].HostID == uint32(0) && c.UserNamespaceConfig.UidMappings[0].Size == uint32(0) {

User ns reworked #4

User ns reworked #4

Conversation

mauriciovasquezbernal commented Jun 24, 2020

rata left a comment

Choose a reason for hiding this comment

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

mauriciovasquezbernal Jul 7, 2020 • edited Loading

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

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

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

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

rata commented Jul 7, 2020 • edited Loading

cdesiniotis commented Jul 7, 2020

mauriciovasquezbernal commented Jul 7, 2020

cdesiniotis commented Jul 7, 2020

mauriciovasquezbernal commented Jul 7, 2020

cdesiniotis commented Jul 7, 2020 • edited Loading

mauriciovasquezbernal commented Jul 8, 2020

cdesiniotis commented Jul 8, 2020

rata Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Jul 8, 2020

cdesiniotis commented Jul 9, 2020

mauriciovasquezbernal Jul 7, 2020 •

edited

Loading

rata commented Jul 7, 2020 •

edited

Loading

cdesiniotis commented Jul 7, 2020 •

edited

Loading

rata Jul 8, 2020 •

edited

Loading