Optionally run e2e pod as privileged for SELinux #83727

bertinatto · 2019-10-10T12:33:03Z

This PR adds back the SecurityContext field missed during the consolidation of redundant code in #79730.

Also, it only runs the pod as privileged if the system has SELinux enabled and the volume isn't block (due to moby/moby#35991).

What type of PR is this?

/kind flake

What this PR does / why we need it:

This is necessary for running some e2e tests on SELinux systems.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig storage
CC @jsafrane @msau42

neolit123 · 2019-10-10T22:34:28Z

/approve
/retest

msau42 · 2019-10-11T00:10:06Z

test/e2e/framework/volume/fixtures.go

@@ -564,7 +565,7 @@ func testVolumeContent(client clientset.Interface, pod *v1.Pod, fsGroup *int64,
 // Multiple Tests can be specified to mount multiple volumes to a single
 // pod.
 func TestVolumeClient(client clientset.Interface, config TestConfig, fsGroup *int64, fsType string, tests []Test) {
-	clientPod, err := runVolumeTesterPod(client, config, "client", fsGroup, tests)
+	clientPod, err := runVolumeTesterPod(client, config, "client", fsGroup, false, tests)


How come this one is false and the other is true?

Can we also make privileged a configurable parameter? I would like to be able to have most e2es not use privileged pods if the system doesn't support SELinux: #80186

I can do that, but because of #83727 (comment), we may have to go privileged if the system supports SELinux and the volume is not block.

Can we also make privileged a configurable parameter?

So I figured this is a bit tricky to solve. These are the possible solutions that I thought about and their problems:

Make pods privileged:

We don't want this, specially where they are not required (on non-SELinux systems)

Make pods unprivileged and add label svirt_sandbox_file_t to node dirs where pods manipulate data (currently it's /tmp for hostPath)

Need to manually chcon -R -t svirt_sandbox_file_t /dir_where_container_writes_files in the host where test runs

Detect if node, where test is running, has SELinux enabled and make the pod privileged

I attempted this one, but it's tricky because we don't always know the node where the pod is going to be scheduled (so we could send `getenforcea command there and

Have an env variable that toggles privileged

Not sure if this is a good idea, it needs to be toggled manually

It's not node-specific

Are there other alternatives?

bertinatto · 2019-10-11T12:31:47Z

Two jobs are failing because of a docker issue that prevents a privileged pod from mapping devices:

$ docker versioni

Client:
 Version:           18.09.9
 API version:       1.39
 Go version:        go1.11.13
 Git commit:        039a7df9ba
 Built:             Wed Sep  4 16:51:52 2019
 OS/Arch:           linux/amd64
 Experimental:      false

Server: Docker Engine - Community
 Engine:
  Version:          18.09.9
  API version:      1.39 (minimum version 1.12)
  Go version:       go1.11.13
  Git commit:       039a7df
  Built:            Wed Sep  4 16:22:32 2019
  OS/Arch:          linux/amd64
  Experimental:     false

$ docker run -w /opt  --rm -it --device=/dev/console:/opt/10 busybox /bin/sh -c 'ls -l /opt/'
total 0
crw-------    1 root     root        5,   1 Oct 11 12:27 10
 
$ docker run -w /opt  --privileged --rm -it --device=/dev/console:/opt/10 busybox /bin/sh -c 'ls -l /opt/'
total 0

test/e2e/0

test/e2e/framework/volume/fixtures.go

bertinatto · 2019-10-14T12:15:32Z

test/e2e/framework/volume/fixtures.go

+		// a privileged container, so we don't go privileged for block volumes.
+		// https://github.com/moby/moby/issues/35991
+		privileged := false
+		if selinux.SELinuxEnabled() && test.Mode != v1.PersistentVolumeBlock {


This needs to be replaced by something else because it doesn't detect if SELinux is enabled on the host

@bertinatto could you please create an issue for this action item ?

Sure, I'll create an issue as soon as I figure this out

alejandrox1

/priority important-soon
/milestone v1.17

bertinatto · 2019-10-14T15:17:44Z

/retest

msau42 · 2019-10-15T02:19:38Z

/assign @jingxu97
to check impact on windows tests

k8s-ci-robot · 2019-10-15T09:07:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, neolit123

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/e2e/framework/volume/OWNERS~~ [neolit123]

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

jingxu97 · 2019-10-16T17:24:40Z

test/e2e/framework/volume/fixtures.go

+		// We need to make the container privileged when SELinux is enabled on the
+		// host,  so the test can write data to a location like /tmp. Also, due to
+		// the Docker bug below, it's not currently possible to map a device with
+		// a privileged container, so we don't go privileged for block volumes.


Does mean for block volume, we cannot set SELinux enabled?

Actually, for block volume, we cannot make the container privileged. Specially if the container runtime is docker (due to the bug below).

To clarify, it shouldn't be a problem to run the test with a unprivileged container for block volumes.

The reason we make it privileged for FS volumes is that the test writes data to the host's /tmp directory, which is not the case for block volumes.

Is there another directory we can safely write to that does not require privileged?

Is there another directory we can safely write to that does not require privileged?

Not that I'm aware of.

By default, containers can only RW to files/directories labeled svirt_sandbox_file_t. So if a process breaks out of the container, it won't be able to change the host FS.

We could add the label above to a pre-defined directory, say /tmp/k8s-e2e, and change the test to read/write data there, rather than the default /tmp. See my comment #83727 (comment). I've tried this and it worked.

The downside of the approach above is that we need to change the test bootstrap to do chcon -R -t svirt_sandbox_file_t /tmp/k8s-e2e in the host before running the test.

I don't know if we are able to do that, however, this is IMO the right approach to solve our SELinux issues in e2e tests.

I would prefer we look into this approach of prepping a special test directory first if the environment uses SELinux. That way we can remove the privileged setting on many tests (and maybe be able to add them to the conformance suite). But this is sufficient for now.

Just to clarify: we can remove the privileged setting if we prep a special test directory on SELinux environments. All containers would run as unprivileged.

FYI, I created the issue #84585 to track this.

jingxu97 · 2019-10-16T17:26:06Z

test/e2e/framework/volume/fixtures.go

@@ -577,7 +588,7 @@ func TestVolumeClient(client clientset.Interface, config TestConfig, fsGroup *in
 // starting and auxiliary pod which writes the file there.
 // The volume must be writable.
 func InjectContent(client clientset.Interface, config TestConfig, fsGroup *int64, fsType string, tests []Test) {
-	injectorPod, err := runVolumeTesterPod(client, config, "injector", fsGroup, tests)
+	injectorPod, err := runVolumeTesterPod(client, config, "injector", true, fsGroup, tests)


I think we cannot set this to true for all cases. For example, for windows, we cannot run privileged container.

Added a check for Windows

bertinatto

@jingxu97, PTAL

bertinatto · 2019-10-25T07:09:04Z

test/e2e/framework/volume/fixtures.go

+		// We need to make the container privileged when SELinux is enabled on the
+		// host,  so the test can write data to a location like /tmp. Also, due to
+		// the Docker bug below, it's not currently possible to map a device with
+		// a privileged container, so we don't go privileged for block volumes.


Actually, for block volume, we cannot make the container privileged. Specially if the container runtime is docker (due to the bug below).

bertinatto · 2019-10-25T07:33:07Z

test/e2e/framework/volume/fixtures.go

@@ -577,7 +588,7 @@ func TestVolumeClient(client clientset.Interface, config TestConfig, fsGroup *in
 // starting and auxiliary pod which writes the file there.
 // The volume must be writable.
 func InjectContent(client clientset.Interface, config TestConfig, fsGroup *int64, fsType string, tests []Test) {
-	injectorPod, err := runVolumeTesterPod(client, config, "injector", fsGroup, tests)
+	injectorPod, err := runVolumeTesterPod(client, config, "injector", true, fsGroup, tests)


Added a check for Windows

kikisdeliveryservice · 2019-10-26T18:58:36Z

Bug Triage Release Team checking in. Code freeze for the 1.17 release is on November 14. Is this still intended for 1.17? Thanks!

timothysc · 2019-10-28T17:37:07Z

/assign @BCLAU @timothysc

@BCLAU - could you verify that this will work for windows use cases?

claudiubelu · 2019-10-30T16:24:28Z

/assign @BCLAU @timothysc

@BCLAU - could you verify that this will work for windows use cases?

As far as this PR goes, lgtm. The only case where privileged could be true, it has been treated and set to false for windows, which is good. I don't think those tests can run on Windows at the moment, especially because of the SecurityContext here: https://github.com/kubernetes/kubernetes/pull/83727/files#diff-2ff410ea588732f4fe497de3d8dbb7deR482 But It's good that extra work is not being added for the time when we'll have those running on Windows as well.

msau42

/lgtm
/approve

msau42 · 2019-10-31T00:15:21Z

test/e2e/framework/volume/fixtures.go

+		// We need to make the container privileged when SELinux is enabled on the
+		// host,  so the test can write data to a location like /tmp. Also, due to
+		// the Docker bug below, it's not currently possible to map a device with
+		// a privileged container, so we don't go privileged for block volumes.


I would prefer we look into this approach of prepping a special test directory first if the environment uses SELinux. That way we can remove the privileged setting on many tests (and maybe be able to add them to the conformance suite). But this is sufficient for now.

k8s-ci-robot · 2019-10-31T02:24:20Z

@bertinatto: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-csi-serial	816b325dd84310acfbf0503be14aa102a58b4a9f	link	`/test pull-kubernetes-e2e-gce-csi-serial`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fejta-bot · 2019-10-31T05:05:23Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from jsafrane and saad-ali October 10, 2019 12:35

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2019

msau42 reviewed Oct 11, 2019

View reviewed changes

bertinatto force-pushed the e2e_hostpath_selinux branch 2 times, most recently from 7137c6b to c41dc58 Compare October 11, 2019 13:16

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2019

jsafrane reviewed Oct 11, 2019

View reviewed changes

test/e2e/0 Outdated Show resolved Hide resolved

test/e2e/framework/volume/fixtures.go Outdated Show resolved Hide resolved

bertinatto force-pushed the e2e_hostpath_selinux branch from c41dc58 to 2a012f0 Compare October 11, 2019 15:29

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2019

bertinatto force-pushed the e2e_hostpath_selinux branch from 2a012f0 to 6229a2b Compare October 11, 2019 15:43

bertinatto commented Oct 14, 2019

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 14, 2019

bertinatto force-pushed the e2e_hostpath_selinux branch from 52e0642 to 2e7508d Compare October 14, 2019 14:46

alejandrox1 reviewed Oct 14, 2019

View reviewed changes

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Oct 14, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 14, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 14, 2019

bertinatto force-pushed the e2e_hostpath_selinux branch from 2e7508d to 816b325 Compare October 14, 2019 15:07

k8s-ci-robot assigned jingxu97 Oct 15, 2019

bertinatto force-pushed the e2e_hostpath_selinux branch from 816b325 to 45bffab Compare October 15, 2019 09:06

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2019

jingxu97 reviewed Oct 16, 2019

View reviewed changes

Optionally run e2e pod as privileged for SELinux

7e72c70

bertinatto force-pushed the e2e_hostpath_selinux branch from 45bffab to 7e72c70 Compare October 25, 2019 07:32

bertinatto commented Oct 25, 2019

View reviewed changes

k8s-ci-robot assigned claudiubelu and timothysc Oct 28, 2019

msau42 reviewed Oct 31, 2019

View reviewed changes

k8s-ci-robot assigned msau42 Oct 31, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2019

k8s-ci-robot merged commit 10ded88 into kubernetes:master Oct 31, 2019

bertinatto mentioned this pull request Oct 31, 2019

Run unprivileged containers in storage e2e tests #84585

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally run e2e pod as privileged for SELinux #83727

Optionally run e2e pod as privileged for SELinux #83727

bertinatto commented Oct 10, 2019 •

edited

neolit123 commented Oct 10, 2019

msau42 Oct 11, 2019

bertinatto Oct 11, 2019

bertinatto Oct 15, 2019

bertinatto commented Oct 11, 2019

bertinatto Oct 14, 2019

alejandrox1 Oct 14, 2019

bertinatto Oct 15, 2019

alejandrox1 left a comment

bertinatto commented Oct 14, 2019

msau42 commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

jingxu97 Oct 16, 2019

bertinatto Oct 25, 2019

bertinatto Oct 25, 2019

msau42 Oct 25, 2019

bertinatto Oct 29, 2019

bertinatto Oct 29, 2019

msau42 Oct 31, 2019

bertinatto Oct 31, 2019

jingxu97 Oct 16, 2019

bertinatto Oct 25, 2019

bertinatto left a comment

bertinatto Oct 25, 2019

bertinatto Oct 25, 2019

kikisdeliveryservice commented Oct 26, 2019 •

edited

timothysc commented Oct 28, 2019

claudiubelu commented Oct 30, 2019

msau42 left a comment

msau42 Oct 31, 2019

k8s-ci-robot commented Oct 31, 2019 •

edited

fejta-bot commented Oct 31, 2019

Optionally run e2e pod as privileged for SELinux #83727

Optionally run e2e pod as privileged for SELinux #83727

Conversation

bertinatto commented Oct 10, 2019 • edited

neolit123 commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertinatto commented Oct 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandrox1 left a comment

Choose a reason for hiding this comment

bertinatto commented Oct 14, 2019

msau42 commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

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

bertinatto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kikisdeliveryservice commented Oct 26, 2019 • edited

timothysc commented Oct 28, 2019

claudiubelu commented Oct 30, 2019

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 31, 2019 • edited

fejta-bot commented Oct 31, 2019

bertinatto commented Oct 10, 2019 •

edited

kikisdeliveryservice commented Oct 26, 2019 •

edited

k8s-ci-robot commented Oct 31, 2019 •

edited