New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Windows: ensure runAsNonRoot does case-insensitive comparison on username #111009
Windows: ensure runAsNonRoot does case-insensitive comparison on username #111009
Conversation
/assign @jsturtevant @dchen1107 @mrunalp @sjenning |
66894d0
to
5eab5bf
Compare
/retest |
5eab5bf
to
d655f12
Compare
} | ||
return nil | ||
} | ||
} | ||
if len(username) > 0 && username == windowsRootUserName { | ||
if len(username) > 0 && strings.EqualFold(username, windowsRootUserName) { | ||
return fmt.Errorf("container's runAsUser (%s) which will be regarded as root identity and will break non-root policy (pod: %q, container: %s)", username, format.Pod(pod), container.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be *effectiveSc.WindowsOptions.RunAsUserName
here too
framework.ExpectNotEqual(event, nil, "event should not be empty") | ||
framework.Logf("Got event: %v", event) | ||
expectedEventError := "container's runAsUser (ContainerAdministrator) which will be regarded as root identity and will break non-root policy" | ||
framework.ExpectEqual(true, strings.Contains(event.Message, expectedEventError), "Event error should indicate non-root policy caused container to not start") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the pod failed to start as expected but this check isn't passing as expected. The output message is
container's runAsUser (ContainerUser)
looks like the log error statement above isn't right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I thought I fixed that in line 62 of security_context_windows.go
in this PR.
Hmm
d655f12
to
1821275
Compare
@jsturtevant - I'm pretty sure that Looking at the test logs at https://prow.k8s.io/log?container=test&id=1545445015973531648&job=pull-kubernetes-e2e-capz-windows-containerd I see it building K8s binaries using version |
These new test cases are passing for me when I run then manually on a cluster I created using a kubelet built from this commit. |
I have confirmed that we 1) do build windows binaries and upload them to a storage bucket form windows CAPZ PR jobs and 2) we do not use those binaries when provisioning the windows nodes (we use the latest CI binaries instead) kubernetes-sigs/cluster-api-provider-azure#2467 should address this |
/test pull-kubernetes-e2e-capz-windows-containerd-test |
2 similar comments
/test pull-kubernetes-e2e-capz-windows-containerd-test |
/test pull-kubernetes-e2e-capz-windows-containerd-test |
/retest |
/test pull-kubernetes-e2e-capz-windows-containerd |
/triage accepted |
/test pull-kubernetes-e2e-capz-windows-containerd ^ although green on github, the tests did not actually run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
tests now run successfully.
/hold
unhold when you're ready to merge (CI pipelines are a bit of a mess today because of go1.19 stuff, so leaving it for you)
/hold cancel |
…1009-upstream-release-1.23 Automated cherry pick of #111009: Windows: ensure runAsNonRoot does case-insensitive comparison
…1009-upstream-release-1.24 Automated cherry pick of #111009: Windows: ensure runAsNonRoot does case-insensitive comparison
…ase-insensitive comparison (#112213) * Windows: ensure runAsNonRoot does case-insensitive comparison on user name Signed-off-by: Mark Rossetti <marosset@microsoft.com> * fixing build error in test/e2e/windows/security_context.go Signed-off-by: Mark Rossetti <marosset@microsoft.com> Co-authored-by: Mark Rossetti <marosset@microsoft.com>
Signed-off-by: Mark Rossetti marosset@microsoft.com
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes an issue where kubelet is doing a case-sensitive comparison for 'ContainerAdministrator' (root account for Windows containers) if
runAsNonRoot
is set. Windows usernames are case-insensitive so the check should be case-insensitive as wellWhich issue(s) this PR fixes:
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.:
/sig windows node
/area kubelet