test: RunAsUser causes pods to not start on Windows #110235

jsturtevant · 2022-05-26T18:11:16Z

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

#109946 Introduced Restricted policy for some of the pod tests. This broke due to #102849. See https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/ci-kubernetes-e2e-capz-master-containerd-windows/1529769915945324544

If the e2e suite is running with nodeOSDistro = windows then we set it to nil.

Which issue(s) this PR fixes:

Fixes #
Related to https://github.com/kubernetes/kubernetes/pull/109946/files#r881112011

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

/sig windows
/cc @marosset @liggitt @tallclair

liggitt · 2022-05-26T18:13:52Z

test/e2e/framework/pod/utils.go

+
+	# setting RunAsUser in Windows results invalid permissions being set on projected volumes
+	# https://github.com/kubernetes/kubernetes/issues/102849
+	if framework.NodeOSDistroIs("windows") {


huh... I didn't expect this bit... do we not have any e2es that test non-homogenous clusters?

I more expected to just drop RunAsUser

We have a test in Windows that explicitly tests that network connectivity work across Windows and Linux. But otherwise, the assumption is the cluster we run tests against would either be Linux nodes or Windows nodes not both (other than the control plane node for Linux)

Oh, ok so we should just drop the RunAsUser? (#109946 (comment))

If we drop it, the callers still need to set RunAsUser for a bunch of the tests to pass on linux, which is still going to run into the same issues of either breaking windows or needing to determine the node OS...

the callers still need to set RunAsUser for a bunch of the tests to pass on linux

is that true? I didn't think most tests were explicitly setting RunAsUser, and instead were running images that weren't built to use root by default

Most of them use the pause image, which should be ok, but there's also a handful using busybox which doesn't set a user.

test/e2e/framework/pod/utils.go

jsturtevant · 2022-05-26T20:47:12Z

/test pull-kubernetes-e2e-capz-windows-containerd

endocrimes · 2022-05-26T22:02:58Z

/test pull-kubernetes-node-kubelet-serial-containerd

jsturtevant · 2022-05-26T23:45:50Z

looking into the failure which seems to be that the apiserver pod didn't start. Don't believe it was related to this

/test pull-kubernetes-e2e-capz-windows-containerd

jsturtevant · 2022-05-27T16:39:51Z

/test pull-kubernetes-e2e-capz-windows-containerd

marosset · 2022-05-27T16:59:22Z

test/e2e/framework/pod/utils.go

 		SeccompProfile: &v1.SeccompProfile{Type: v1.SeccompProfileTypeRuntimeDefault},
 	}
+
+	if NodeOSDistroIs("windows") {


Since --node-os-distro is set as a e2e.test argument and we do schedule some linux containers during some Windows specific e2e tests this would add windows options with RunAsUserName set for those linux containers.
I don't think this will be an issue but did want to call that out.

option: make 2 versions of GetRestrictedPodSecurityContext, one that guesses the OS and one that lets it be explicitly specified.

Then, the Mixin function should use the PodOS if present.

func GetRestrictedPodSecurityContext() *v1.PodSecurityContext { if NodeOSDistroIs("windows") { return GetRestrictedPodSecurityContextForOS(v1.WindowsPodOS) // could just be a string until PodOS merges? } return GetRestrictedPodSecurityContextForOS(v1.LinuxPodOS) // default } func GetRestrictedPodSecurityContextForOS(os PodOS) *v1.PodSecurityContext { // Bulk of the work }

I looked at doing this but seems like it should really be done when // TODO(#105919): Handle PodOS for windows pods. is completed. It made things a bit messier than it is now because there are a couple different cases too cover (framework.NodeOSDistroIs and handling PodOSsince this needs to be addressed in MixinRestrictedPodSecurity as well.

jsturtevant · 2022-05-27T18:17:25Z

That failure was similiar to earlier, I haven't been able to figure out what it is yet. The control plane didn't start:

kube-apiserver-capz-conf-6bprnv-control-plane-frsww          capz-conf-6bprnv-control-plane-frsww Pending       []
kube-controller-manager-capz-conf-6bprnv-control-plane-frsww capz-conf-6bprnv-control-plane-frsww Pending       []
kube-scheduler-capz-conf-6bprnv-control-plane-frsww          capz-conf-6bprnv-control-plane-frsww Pending

/test pull-kubernetes-e2e-capz-windows-containerd

jsturtevant · 2022-05-27T19:48:02Z

Windows job passed, created kubernetes-sigs/cluster-api-provider-azure#2338 to track issue with capz cluster.

/assign @marosset @liggitt @tallclair

marosset · 2022-05-27T20:08:39Z

LGTM for me from Windows.
I noted one slight concern above and am satisfied with the approach

marosset · 2022-05-27T20:09:10Z

/triage accepted
/priority critical-urgent
(since this is breaking tests)

k8s-ci-robot · 2022-05-27T20:09:12Z

@marosset: The label(s) priority/critical-urgernt cannot be applied, because the repository doesn't have them.

In response to this:

/triage accepted
/priority critical-urgernt
(since this is breaking tests)

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.

ravisantoshgudimetla · 2022-05-27T20:54:10Z

test/e2e/framework/pod/utils.go

+		if NodeOSDistroIs("windows") && pod.Spec.SecurityContext.WindowsOptions == nil {
+			pod.Spec.SecurityContext.WindowsOptions = &v1.WindowsSecurityContextOptions{}
+			pod.Spec.SecurityContext.WindowsOptions.RunAsUserName = pointer.StringPtr(DefaultNonRootUserName)
+		}
 	}
 	for i := range pod.Spec.Containers {
 		mixinRestrictedContainerSecurityContext(&pod.Spec.Containers[i])


Do you want to add similar securityContext checks for containers? It may not be a problem unless pod's explicitly have containers that have Windows options set.

This doesn't look to set the users for the containers currently, so left it out for now.

marosset · 2022-05-31T16:20:06Z

/lgtm

jsturtevant · 2022-06-01T17:09:23Z

@liggitt @tallclair any further thoughts on these changes?

jsturtevant · 2022-06-01T20:27:18Z

/priority critical-urgent

dims · 2022-06-01T20:28:13Z

/approve

/hold for previous reviewers to chime in again.

k8s-ci-robot · 2022-06-01T20:28:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, jsturtevant

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/OWNERS~~ [dims]

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

tallclair · 2022-06-02T00:01:03Z

We might be able to clean this up further, but if this fixes currently failing tests we should probably merge it and iterate.

tallclair · 2022-06-01T23:53:25Z

test/e2e/framework/pod/utils.go

+// If the Node OS is windows, we return nill due to issue with invalid permissions set on projected volumes
+// https://github.com/kubernetes/kubernetes/issues/102849
+func GetDefaultNonRootUser() *int64 {
+	if NodeOSDistroIs("windows") {


nit: Is there a constant defined for this?

There doesn't seem to be, If you search for it you will find it mostly used like this https://github.com/kubernetes/kubernetes/search?q=NodeOSDistroIs&type=code.

tallclair · 2022-06-01T23:55:13Z

test/e2e/framework/pod/utils.go

 		SeccompProfile: &v1.SeccompProfile{Type: v1.SeccompProfileTypeRuntimeDefault},
 	}
+
+	if NodeOSDistroIs("windows") {


nit: consider just clearing the RunAsUser here, to cut down on the number of places we need to check the OS

I tried this but it is also needed in MixRestrictedPodSecurty and without using podOS (which has a TODO) I think it gets a bit unwieldy

tallclair · 2022-06-01T23:59:33Z

test/e2e/framework/pod/utils.go

 		SeccompProfile: &v1.SeccompProfile{Type: v1.SeccompProfileTypeRuntimeDefault},
 	}
+
+	if NodeOSDistroIs("windows") {


option: make 2 versions of GetRestrictedPodSecurityContext, one that guesses the OS and one that lets it be explicitly specified.

Then, the Mixin function should use the PodOS if present.

func GetRestrictedPodSecurityContext() *v1.PodSecurityContext { if NodeOSDistroIs("windows") { return GetRestrictedPodSecurityContextForOS(v1.WindowsPodOS) // could just be a string until PodOS merges? } return GetRestrictedPodSecurityContextForOS(v1.LinuxPodOS) // default } func GetRestrictedPodSecurityContextForOS(os PodOS) *v1.PodSecurityContext { // Bulk of the work }

jsturtevant · 2022-06-02T18:40:26Z

@tallclair the feedback is valid but after trying it out it really feels like those changes should come in with the // TODO(#105919): Handle PodOS for windows pods. Are you ok with merging as is and revisting when PodOS is handled?

marosset · 2022-06-02T20:39:25Z

@tallclair the feedback is valid but after trying it out it really feels like those changes should come in with the // TODO(#105919): Handle PodOS for windows pods. Are you ok with merging as is and revisting when PodOS is handled?

Our sig-windows jobs targeting master are currently failing some tests because of this issue.
Can we use these changes as a mitigation and follow up with a more permanent solution to help us get our CI signal back to green?

tallclair · 2022-06-03T19:19:50Z

/hold cancel

Yes, let's follow up.

k8s-ci-robot requested a review from liggitt May 26, 2022 18:11

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 26, 2022

k8s-ci-robot requested review from marosset and tallclair May 26, 2022 18:11

liggitt reviewed May 26, 2022

View reviewed changes

test/e2e/framework/pod/utils.go Show resolved Hide resolved

jsturtevant force-pushed the fix-new-restricted-pod-tests branch from ba86be9 to 2648881 Compare May 26, 2022 20:34

jsturtevant mentioned this pull request May 26, 2022

Restricted Pod E2E tests #109946

Merged

marosset added this to In Review (v1.25) in SIG-Windows May 26, 2022

RunAsUser causes pods to not start on Windows

061b8e8

jsturtevant force-pushed the fix-new-restricted-pod-tests branch from 2648881 to 061b8e8 Compare May 27, 2022 16:31

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. labels May 27, 2022

marosset reviewed May 27, 2022

View reviewed changes

jsturtevant mentioned this pull request May 27, 2022

API server gets stuck in pending during deployment when used in pre-submit kubernetes-sigs/cluster-api-provider-azure#2338

Open

k8s-ci-robot assigned liggitt, marosset and tallclair May 27, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 27, 2022

ravisantoshgudimetla reviewed May 27, 2022

View reviewed changes

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

jsturtevant mentioned this pull request Jun 1, 2022

[windows] Failure cluster [4482c2b4...] [sig-node] Pods should contain environment variables for services #110339

Closed

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 1, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2022

tallclair reviewed Jun 2, 2022

View reviewed changes

marosset mentioned this pull request Jun 2, 2022

enhance assertions in test/e2e/windows #110303

Merged

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2022

k8s-ci-robot merged commit b7337cb into kubernetes:master Jun 3, 2022

SIG-Windows automation moved this from In Review (v1.25) to Done (v1.25) Jun 3, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 3, 2022

test: RunAsUser causes pods to not start on Windows #110235

test: RunAsUser causes pods to not start on Windows #110235

Conversation

jsturtevant commented May 26, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant May 26, 2022 • edited

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

jsturtevant commented May 26, 2022

endocrimes commented May 26, 2022

jsturtevant commented May 26, 2022 • edited

jsturtevant commented May 27, 2022

marosset May 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant commented May 27, 2022

jsturtevant commented May 27, 2022

marosset commented May 27, 2022

marosset commented May 27, 2022 • edited

k8s-ci-robot commented May 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marosset commented May 31, 2022

jsturtevant commented Jun 1, 2022

jsturtevant commented Jun 1, 2022

dims commented Jun 1, 2022

k8s-ci-robot commented Jun 1, 2022

tallclair commented Jun 2, 2022

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

jsturtevant commented Jun 2, 2022

marosset commented Jun 2, 2022

tallclair commented Jun 3, 2022

jsturtevant May 26, 2022 •

edited

jsturtevant commented May 26, 2022 •

edited

marosset May 27, 2022 •

edited

marosset commented May 27, 2022 •

edited