Add node e2e tests for hostPid #44119

feiskyer · 2017-04-05T23:56:23Z

For #44118. Add node e2e tests for hostPid.

cc @yujuhong @kubernetes/sig-node-pr-reviews

k8s-reviewable · 2017-04-05T23:56:30Z

This change is

yujuhong · 2017-04-06T00:21:03Z

test/e2e_node/security_context_test.go

+				}
+
+				// the pid of 'sh' should not be 1 if it is in hostPid mode.
+				if logs == "1\n" {


This would not work in the future if we enable shared pid namespace in the pod. Maybe we can create two host pid pods and check whether they can see each other.

You can also create a non-host-pid-ns pod, and verify that it cannot see others.

This would not work in the future if we enable shared pid namespace in the pod. Maybe we can create to host pid pods and check whether they can see each other.

That's right. Will update.

yujuhong · 2017-04-06T17:26:05Z

test/e2e_node/security_context_test.go

+		busyboxPodName := "busybox-hostpid-" + string(uuid.NewUUID())
+		nginxPodName := "nginx-hostpid-" + string(uuid.NewUUID())
+		It("should create the pod in hostPid", func() {
+			podClient.CreateSync(makeHostPidPod(nginxPodName,


The test will pass if there happens to be any nginx process running on the node. We should make sure the pid actually matches. You can get the pid of the process by exec into the container and cat the /var/run/nginx.pid file.

In the other container, you should make sure if the output includes the pid we are looking for.

I'd still like to see a test for a non-hostpid pod to make sure we don't always use host pid.

Agree. @xlgao-zju We could also test hostpid and hostipc in similar way.

yujuhong · 2017-04-06T17:26:20Z

test/e2e_node/security_context_test.go

+		}
+		busyboxPodName := "busybox-hostpid-" + string(uuid.NewUUID())
+		nginxPodName := "nginx-hostpid-" + string(uuid.NewUUID())
+		It("should create the pod in hostPid", func() {


nit: s/in hostPid/in the host PID namespace

yujuhong · 2017-04-06T18:32:04Z

Adding @Random-Liu since we need an approver.

Random-Liu · 2017-04-06T18:43:37Z

test/e2e_node/security_context_test.go

+		busyboxPodName := "busybox-hostpid-" + string(uuid.NewUUID())
+		nginxPodName := "nginx-hostpid-" + string(uuid.NewUUID())
+		It("should create the pod in hostPid", func() {
+			podClient.CreateSync(makeHostPidPod(nginxPodName,


Agree. @xlgao-zju We could also test hostpid and hostipc in similar way.

Random-Liu · 2017-04-06T18:51:27Z

test/e2e_node/security_context_test.go

+
+			podClient.CreateSync(makeHostPidPod(busyboxPodName,
+				"gcr.io/google_containers/busybox:1.24",
+				[]string{"sh", "-c", "pgrep nginx; sleep 240"}))


Why sleep 240? Maybe just tail -f /dev/null

Random-Liu · 2017-04-06T18:52:50Z

test/e2e_node/security_context_test.go

+				[]string{"sh", "-c", "pgrep nginx; sleep 240"}))
+
+			Eventually(func() error {
+				logs, err := framework.GetPodLogs(f.ClientSet, f.Namespace.Name, busyboxPodName, busyboxPodName)


Even if you go with current solution, we should trim spaces here to make sure logs is actually empty.

And I think yuju's suggestion is better and more reliable here.

Random-Liu · 2017-04-07T03:06:13Z

LGTM

Random-Liu · 2017-04-07T03:33:58Z

test/e2e_node/security_context_test.go, line 63 at r4 (raw file):

			podClient.Create(makeHostPidPod(podName,
				"gcr.io/google_containers/busybox:1.24",
				[]string{"sh", "-c", "pidof nginx"},

Why not just pidof nginx || true?

Comments from Reviewable

feiskyer · 2017-04-07T03:37:31Z

Why not just pidof nginx || true?

Good idea. Updated.

Random-Liu · 2017-04-07T04:21:28Z

Review status: 0 of 2 files reviewed at latest revision, 8 unresolved discussions.

test/e2e_node/security_context_test.go, line 88 at r5 (raw file):

			createAndWaitHostPidPod(busyboxPodName, true)

			Eventually(func() error {

Why use Eventually here?

test/e2e_node/security_context_test.go, line 113 at r5 (raw file):

			createAndWaitHostPidPod(busyboxPodName, false)

			Eventually(func() error {

Why use Eventually here?

Comments from Reviewable

xlgao-zju · 2017-04-07T04:55:42Z

/cc @Helen-Xie @heartlock

feiskyer · 2017-04-07T07:36:48Z

@Random-Liu Fixed. PTAL.

Random-Liu · 2017-04-07T18:38:03Z

/lgtm
/approve

k8s-github-robot · 2017-04-07T18:38:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Random-Liu, feiskyer

Needs approval from an approver in each of these OWNERS Files:

~~test/e2e_node/OWNERS~~ [Random-Liu]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-04-07T19:23:38Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-04-07T20:10:04Z

Automatic merge from submit-queue

Automatic merge from submit-queue Automated cherry pick of #44097 and #44119 Cherry pick #44097 and #44119 for release-1.6. Fix container hostPid settings. **Release note**: ```release-note Fix container hostPid settings when CRI is enabled. ```

k8s-cherrypick-bot · 2017-04-10T16:54:32Z

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Automatic merge from submit-queue Automated cherry pick of kubernetes#44097 and kubernetes#44119 Cherry pick kubernetes#44097 and kubernetes#44119 for release-1.6. Fix container hostPid settings. **Release note**: ```release-note Fix container hostPid settings when CRI is enabled. ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 5, 2017

k8s-github-robot assigned dchen1107 Apr 6, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Apr 6, 2017

yujuhong reviewed Apr 6, 2017

View reviewed changes

feiskyer force-pushed the e2e-node branch 2 times, most recently from 88b199f to 3c21c6b Compare April 6, 2017 02:11

enisoc mentioned this pull request Apr 6, 2017

Automated cherry pick of #44097 and #44119 #44116

Merged

yujuhong reviewed Apr 6, 2017

View reviewed changes

yujuhong assigned yujuhong and unassigned dchen1107 Apr 6, 2017

yujuhong added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 6, 2017

yujuhong assigned Random-Liu Apr 6, 2017

yujuhong added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 6, 2017

yujuhong added this to the v1.6 milestone Apr 6, 2017

yujuhong added the cherrypick-candidate label Apr 6, 2017

Random-Liu reviewed Apr 6, 2017

View reviewed changes

feiskyer force-pushed the e2e-node branch from 3c21c6b to 5cc02ca Compare April 7, 2017 02:11

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 7, 2017

feiskyer force-pushed the e2e-node branch from 5cc02ca to 7b7f9ae Compare April 7, 2017 02:23

feiskyer force-pushed the e2e-node branch 2 times, most recently from 440503f to dc034fc Compare April 7, 2017 03:33

feiskyer force-pushed the e2e-node branch from dc034fc to 0840c28 Compare April 7, 2017 03:37

feiskyer force-pushed the e2e-node branch 2 times, most recently from 438f1ca to 06b89fd Compare April 7, 2017 04:58

Add node e2e tests for hostPid

a696e86

feiskyer force-pushed the e2e-node branch from 06b89fd to a696e86 Compare April 7, 2017 05:42

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2017

k8s-github-robot merged commit a635aed into kubernetes:master Apr 7, 2017

feiskyer deleted the e2e-node branch April 8, 2017 01:35

k8s-cherrypick-bot removed the cherrypick-candidate label Apr 10, 2017

feiskyer mentioned this pull request Apr 27, 2017

Add node e2e test for security context #44118

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add node e2e tests for hostPid #44119

Add node e2e tests for hostPid #44119

feiskyer commented Apr 5, 2017

k8s-reviewable commented Apr 5, 2017

yujuhong Apr 6, 2017 •

edited

Loading

feiskyer Apr 6, 2017

yujuhong Apr 6, 2017

Random-Liu Apr 6, 2017

feiskyer Apr 7, 2017

yujuhong Apr 6, 2017

feiskyer Apr 7, 2017

yujuhong commented Apr 6, 2017

Random-Liu Apr 6, 2017

Random-Liu Apr 6, 2017

Random-Liu Apr 6, 2017

Random-Liu commented Apr 7, 2017

Random-Liu commented Apr 7, 2017

feiskyer commented Apr 7, 2017

Random-Liu commented Apr 7, 2017

xlgao-zju commented Apr 7, 2017

feiskyer commented Apr 7, 2017

Random-Liu commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

k8s-cherrypick-bot commented Apr 10, 2017

Add node e2e tests for hostPid #44119

Add node e2e tests for hostPid #44119

Conversation

feiskyer commented Apr 5, 2017

k8s-reviewable commented Apr 5, 2017

yujuhong Apr 6, 2017 • 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

yujuhong commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Apr 7, 2017

Random-Liu commented Apr 7, 2017

feiskyer commented Apr 7, 2017

Random-Liu commented Apr 7, 2017

xlgao-zju commented Apr 7, 2017

feiskyer commented Apr 7, 2017

Random-Liu commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

k8s-cherrypick-bot commented Apr 10, 2017

yujuhong Apr 6, 2017 •

edited

Loading