Kubelet: Add security context for Windows containers #64009
Conversation
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
the
size/XXL
k8s-ci-robot
added
the
cncf-cla: yes
|
@feiskyer: GitHub didn't allow me to request PR reviews from the following users: taylorb-microsoft, PatrickLang. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
|
/retest |
|
i am asking @bsteciuk to review this as well. at a high level it looks good to me but i don't know enough to do a good code review here |
|
/sig windows /retest |
k8s-ci-robot
added
sig/windows
|
/retest ping @dchen1107 @bsteciuk |
| // setup security context | ||
| effectiveSc := securitycontext.DetermineEffectiveSecurityContext(pod, container) | ||
| // RunAsUser only supports int64 from Kubernetes API, but Windows containers only support username. | ||
| // TODO: change RunAsUser to intstr in Kubernetes API. |
There was a problem hiding this comment.
Do we need to change it? I'm wondering whether its better to use one field as IntStr that requires different types for Windows vs Linux, or if we should just have two different fields - Int for Linux, Str for Windows?
There was a problem hiding this comment.
My preference is to keep the fields separate but open to feedback
There was a problem hiding this comment.
From CRI perspective, it supports both int64 and string for Linux containers but only string for Windows containers. While in today's API, only int64 is supported yet. So changing kubernetes API to IntStr will work for both Linux and Windows.
There was a problem hiding this comment.
Ah, I see. So converging DetermineEffectiveSecurityContext will require the API change later. This PR just adds the new field run_as_user which is Windows only.
There was a problem hiding this comment.
I think username could also be verified, e.g. administrator on Windows or root on Linux.
There was a problem hiding this comment.
If the docker images will run as user "foo" by default inside the container, (today) kubelet can't check what uid "foo" maps to, and whether it's actually root on the host.
How does this work for windows?...
There was a problem hiding this comment.
Privileged mode isn't implemented on Windows, and the container administrator account is separate because the filesystem and registry (where the user profile is) are separate from the host's. Putting a check on username won't add any security.
There was a problem hiding this comment.
If you do want it for consistency with Linux, it could be done but would need to check against localized variants of "Administrator" since microsoft/windowsservercore is localized
There was a problem hiding this comment.
If the docker images will run as user "foo" by default inside the container, (today) kubelet can't check what uid "foo" maps to, and whether it's actually root on the host.
@yujuhong Good catch. Even non-root username could have uid 0 on Linux, so username won't make sense for linux security context.
If you do want it for consistency with Linux, it could be done but would need to check against localized variants of "Administrator" since microsoft/windowsservercore is localized
@PatrickLang I think checking localized variants not possible because kubelet won't handle container localization.
Let me remove the todo item.
feiskyer
added
priority/important-soon
|
/milestone v1.11 |
|
/kind api-change |
feiskyer
added
kind/bug
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
This PR does not look critical for 1.11, and is not in an ready-to-merge state. As such, it will be removed from the 1.11 milestone by the milestone bot when Code Freeze starts at midnight UTC / 5pm PDT. If you feel it is critical to 1.11, please upgrade its priority to critical-urgent |
|
@tallclair or @derekwaynecarr any updates? There were some questions about usernames but no changes requested. |
|
Filed #64801 to investigate PodSecurityPolicy later per @tallclair 's feedback |
|
/lgtm |
k8s-ci-robot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, PatrickLang, tallclair, yujuhong 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:
Approvers can indicate their approval by writing |
feiskyer
added
priority/critical-urgent
Yep, we should add windows tests definitely. But the test infra is still not ready yet. We expect it ready next release. |
|
[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process @PatrickLang @derekwaynecarr @feiskyer @tallclair @yujuhong Pull Request Labels
|
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/test pull-kubernetes-integration |
|
Automatic merge from submit-queue (batch tested with PRs 64009, 64780, 64354, 64727, 63650). If you want to cherry-pick this change to another branch, please follow the instructions here. |
|
@feiskyer: The following test failed, say
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. |
What this PR does / why we need it:
This PR adds windows containers to Kubelet CRI and also implements security context setting for docker containers.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
RunAsUser from Kubernetes API only accept int64 today, which is not supported on Windows. It should be changed to intstr for working with both Windows and Linux containers in a separate PR.
Release note:
/cc @PatrickLang @taylorb-microsoft @michmike @JiangtianLi @yujuhong @dchen1107