-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
test: RunAsUser causes pods to not start on Windows #110235
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,16 @@ func GetTestImageID(id imageutils.ImageID) imageutils.ImageID { | |
return id | ||
} | ||
|
||
// GetDefaultNonRootUser returns default non root user | ||
// 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") { | ||
return nil | ||
} | ||
return pointer.Int64(DefaultNonRootUser) | ||
} | ||
|
||
// GeneratePodSecurityContext generates the corresponding pod security context with the given inputs | ||
// If the Node OS is windows, currently we will ignore the inputs and return nil. | ||
// TODO: Will modify it after windows has its own security context | ||
|
@@ -123,15 +133,25 @@ func GetLinuxLabel() *v1.SELinuxOptions { | |
// DefaultNonRootUser is the default user ID used for running restricted (non-root) containers. | ||
const DefaultNonRootUser = 1000 | ||
|
||
// DefaultNonRootUserName is the default username in Windows used for running restricted (non-root) containers | ||
const DefaultNonRootUserName = "ContainerUser" | ||
|
||
// GetRestrictedPodSecurityContext returns a restricted pod security context. | ||
// This includes setting RunAsUser for convenience, to pass the RunAsNonRoot check. | ||
// Tests that require a specific user ID should override this. | ||
func GetRestrictedPodSecurityContext() *v1.PodSecurityContext { | ||
return &v1.PodSecurityContext{ | ||
psc := &v1.PodSecurityContext{ | ||
RunAsNonRoot: pointer.BoolPtr(true), | ||
RunAsUser: pointer.Int64(DefaultNonRootUser), | ||
RunAsUser: GetDefaultNonRootUser(), | ||
SeccompProfile: &v1.SeccompProfile{Type: v1.SeccompProfileTypeRuntimeDefault}, | ||
} | ||
|
||
if NodeOSDistroIs("windows") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. option: make 2 versions of 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
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at doing this but seems like it should really be done when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: consider just clearing the RunAsUser here, to cut down on the number of places we need to check the OS There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
psc.WindowsOptions = &v1.WindowsSecurityContextOptions{} | ||
psc.WindowsOptions.RunAsUserName = pointer.StringPtr(DefaultNonRootUserName) | ||
} | ||
|
||
return psc | ||
} | ||
jsturtevant marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// GetRestrictedContainerSecurityContext returns a minimal restricted container security context. | ||
|
@@ -164,11 +184,15 @@ func MixinRestrictedPodSecurity(pod *v1.Pod) error { | |
pod.Spec.SecurityContext.RunAsNonRoot = pointer.BoolPtr(true) | ||
} | ||
if pod.Spec.SecurityContext.RunAsUser == nil { | ||
pod.Spec.SecurityContext.RunAsUser = pointer.Int64Ptr(DefaultNonRootUser) | ||
pod.Spec.SecurityContext.RunAsUser = GetDefaultNonRootUser() | ||
} | ||
if pod.Spec.SecurityContext.SeccompProfile == nil { | ||
pod.Spec.SecurityContext.SeccompProfile = &v1.SeccompProfile{Type: v1.SeccompProfileTypeRuntimeDefault} | ||
} | ||
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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't look to set the users for the containers currently, so left it out for now. |
||
|
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.
nit: Is there a constant defined for this?
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.
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.