Add default_systcls option to crio.conf #1721

umohnani8 · 2018-08-06T17:02:26Z

Users can add default sysctls that they want all cri-o containers
to have in the crio.conf file now.
Adding net.ipv4.ping_group_range as a default so we can drop the
NET_RAW capability.

Remove sysctl parsing code from cri-o as we extract the sysctls from k8s directly.
Had a TODO from a while back to remove it, so removing.

Drop the NET_RAW capability from the list of cri-o defaults
as we are using the sysctl net.ipv4.ping_group_range instead
to allow users to do a ping.

Signed-off-by: umohnani8 umohnani@redhat.com

umohnani8 · 2018-08-06T17:02:58Z

@rhatdan PTAL. For now the default sysctl is net.ipv4.ping_group_range=0 0, what should I change the value of this sysctl to be?

mrunalp · 2018-08-06T18:13:05Z

@umohnani8 You will have to restrict it to /proc/1/gid_map values on the extreme else sysctl will fail.

mrunalp · 2018-08-06T18:14:05Z

lib/config.go


+// DefaultSysctls for the default-sysctls option in the crio.conf file
+var DefaultSysctls = []string{
+	"net.ipv4.ping_group_range=0 0",


We cannot hard code this. For user namespaces, the gid_map could be different so will have to populate the range and validate it against user ns group mappings.

I guess we need to see if these are relative to the USERNS. IE Does this happen in runc after the USERNS is setup, then it should work.

it should happen from inside the container once the user namespace is configured. Some sysctl are not allowed from a namespace and the OCI runtime will fail to set these, but I don't think this PR has to worry of it.

mrunalp

We need a test to check that we can ping without cap_net_raw.

rhatdan · 2018-08-07T12:21:45Z

Why do you think we need to check against GIDMAP? If the sysctl is executed within the userns which it must be then the only issue might be if 0 was not mapped? I guess we could attempt to set that up and see if it fails.

@giuseppe PTAL

umohnani8 · 2018-08-07T14:41:50Z

@mrunalp the tests that ping a pod from another pod should be testing this change automatically - https://github.com/kubernetes-incubator/cri-o/blob/master/test/network.bats#L103.

umohnani8 · 2018-08-08T15:36:14Z

/test all

umohnani8 · 2018-08-08T18:47:57Z

/test critest_rhel

umohnani8 · 2018-08-08T20:40:33Z

/test integration_rhel

umohnani8 · 2018-08-08T20:40:53Z

/test kata-containers

umohnani8 · 2018-08-09T13:44:35Z

/test all

umohnani8 · 2018-08-13T20:43:25Z

/test all

mrunalp · 2018-08-20T20:44:40Z

/test all

umohnani8 · 2018-08-21T16:50:24Z

/test integration_rhel

mrunalp · 2018-08-21T17:32:30Z

We can skip the test for RHEL 7 by checking for the setting on the host.

umohnani8 · 2018-08-21T18:21:36Z

/test all

mrunalp · 2018-08-21T23:26:02Z

/test all

umohnani8 · 2018-08-22T17:24:14Z

/test all

mrunalp · 2018-08-22T20:54:21Z

Just debugged this with @umohnani8. This sysctl isn't available when using a user namespace on RHEL. We should use some other sysctl for testing such as net.ipv4.ip_forward.

umohnani8 · 2018-08-23T17:53:08Z

/test all

umohnani8 · 2018-08-23T22:06:23Z

Tests are green! @mrunalp PTAL

rhatdan · 2018-08-24T07:48:32Z

server/sandbox_run_linux.go


+	// Add default sysctls given in crio.conf
+	for _, defaultSysctl := range s.config.DefaultSysctls {
+		split := strings.Split(defaultSysctl, "=")


Should this be a SplitN(defaultSysctl, "=",2)
TO make sure the sysctl does not include a "=", I know it is doubtful, but...

rhatdan · 2018-08-24T07:50:13Z

server/sandbox_run_linux.go

+		split := strings.Split(defaultSysctl, "=")
+		if len(split) == 2 {
+			if err := validateSysctl(split[0], hostNetwork, hostIPC); err != nil {
+				logrus.Warnf("%v - skipping...", err)


skipping %s ..., err, defaultSysctl
Should this be an error and refuse to start the container?

I was thinking why error out completely when it is just possible that the user made a typo or something in the sysctls they have enabled. Made more sense to log it and just continue to create the error.
If you think it is better to fail completely I can change it.

Well we at lease need more info on which sysctl failed.

Oh, yeah adding that.

Users can add default sysctls that they want all cri-o containers to have in the crio.conf file now. Adding net.ipv4.ping_group_range as a default so we can drop the NET_RAW capability. Signed-off-by: umohnani8 <umohnani@redhat.com>

We extract the sysctls from k8s so we don't need to extract it in cri-o separately. Had a TODO from a while back to remove it, so removing. Signed-off-by: umohnani8 <umohnani@redhat.com>

rhatdan · 2018-08-24T13:47:04Z

/test all

rhatdan · 2018-08-24T13:48:28Z

LGTM

umohnani8 · 2018-08-27T17:04:40Z

@mrunalp this is ready to merge :)

umohnani8 requested review from mrunalp and runcom as code owners August 6, 2018 17:02

k8s-ci-robot added cncf-cla: yes size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 6, 2018

mrunalp reviewed Aug 6, 2018

View reviewed changes

mrunalp requested changes Aug 6, 2018

View reviewed changes

umohnani8 force-pushed the sysctl branch 3 times, most recently from 5de0ca1 to 037a50f Compare August 7, 2018 14:40

umohnani8 force-pushed the sysctl branch 2 times, most recently from e985e5e to 25b669b Compare August 8, 2018 14:11

umohnani8 force-pushed the sysctl branch from 25b669b to 63937f7 Compare August 9, 2018 12:58

umohnani8 force-pushed the sysctl branch 3 times, most recently from 736c8aa to 32fa9e7 Compare August 13, 2018 15:52

umohnani8 force-pushed the sysctl branch 2 times, most recently from eca1b8f to f89f82e Compare August 20, 2018 19:20

umohnani8 force-pushed the sysctl branch from f89f82e to 5a471e1 Compare August 21, 2018 17:41

umohnani8 force-pushed the sysctl branch 2 times, most recently from be35082 to d243d65 Compare August 21, 2018 19:43

umohnani8 force-pushed the sysctl branch 3 times, most recently from 6a1d44a to 130d02e Compare August 22, 2018 17:12

umohnani8 force-pushed the sysctl branch 3 times, most recently from 73d80b4 to 58b0316 Compare August 23, 2018 14:13

rhatdan reviewed Aug 24, 2018

View reviewed changes

umohnani8 added 2 commits August 24, 2018 09:43

Add default_systcls option to crio.conf

0337ab8

Users can add default sysctls that they want all cri-o containers to have in the crio.conf file now. Adding net.ipv4.ping_group_range as a default so we can drop the NET_RAW capability. Signed-off-by: umohnani8 <umohnani@redhat.com>

Remove sysctl parsing code from cri-o

841539d

We extract the sysctls from k8s so we don't need to extract it in cri-o separately. Had a TODO from a while back to remove it, so removing. Signed-off-by: umohnani8 <umohnani@redhat.com>

umohnani8 force-pushed the sysctl branch from 58b0316 to 841539d Compare August 24, 2018 13:44

mrunalp approved these changes Aug 29, 2018

View reviewed changes

rhatdan merged commit 3fc75c5 into cri-o:master Aug 29, 2018

umohnani8 deleted the sysctl branch January 31, 2019 19:29

umohnani8 mentioned this pull request Mar 1, 2019

REQUEST: New membership for umohnani8 kubernetes/org#550

Closed

6 tasks

umohnani8 mentioned this pull request May 17, 2019

Remove NET_RAW capability and add net.ipv4.ping_group_range sysctl #2378

Closed

snir911 mentioned this pull request Nov 17, 2020

CreateContainer failed: EINVAL: Invalid argument: unknown on Openshift 4.6 kata-containers/kata-containers#1047

Closed

prezha mentioned this pull request Mar 17, 2024

add default sysctls to allow privileged ports with no capabilities kubernetes/minikube#18421

Merged

Add default_systcls option to crio.conf #1721

Add default_systcls option to crio.conf #1721

Uh oh!

Conversation

umohnani8 commented Aug 6, 2018

Uh oh!

umohnani8 commented Aug 6, 2018

Uh oh!

mrunalp commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Aug 7, 2018

Uh oh!

umohnani8 commented Aug 7, 2018

Uh oh!

umohnani8 commented Aug 8, 2018

Uh oh!

umohnani8 commented Aug 8, 2018

Uh oh!

umohnani8 commented Aug 8, 2018

Uh oh!

umohnani8 commented Aug 8, 2018

Uh oh!

umohnani8 commented Aug 9, 2018

Uh oh!

umohnani8 commented Aug 13, 2018

Uh oh!

mrunalp commented Aug 20, 2018

Uh oh!

umohnani8 commented Aug 21, 2018

Uh oh!

mrunalp commented Aug 21, 2018

Uh oh!

umohnani8 commented Aug 21, 2018

Uh oh!

mrunalp commented Aug 21, 2018

Uh oh!

umohnani8 commented Aug 22, 2018

Uh oh!

mrunalp commented Aug 22, 2018

Uh oh!

umohnani8 commented Aug 23, 2018

Uh oh!

umohnani8 commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Aug 24, 2018

Uh oh!

rhatdan commented Aug 24, 2018

Uh oh!

umohnani8 commented Aug 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!