-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Kube Gen run as user/group issues #11944
Conversation
@@ -942,7 +942,7 @@ USER test1` | |||
pod := new(v1.Pod) | |||
err = yaml.Unmarshal(kube.Out.Contents(), pod) | |||
Expect(err).To(BeNil()) | |||
Expect(*pod.Spec.Containers[0].SecurityContext.RunAsUser).To(Equal(int64(10001))) | |||
Expect(pod.Spec.Containers[0].SecurityContext.RunAsUser).To(BeNil()) |
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.
Do we have a check to make sure runasuser still works?
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.
yep, there is an explicit --user
test
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.
I think the new option is not needed for this
libpod/options.go
Outdated
// WithDefinedUser informs us whether or not we have explicitly defined a user or if podman is using the default user | ||
func WithDefinedUser() CtrCreateOption { | ||
return func(ctr *Container) error { | ||
if ctr.valid { | ||
return define.ErrCtrFinalized | ||
} | ||
|
||
ctr.config.DefinedUser = true | ||
return nil | ||
} | ||
} |
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.
Why do we need a extra option for this? IMO all you need to do is:
if container.user != image.user then add user to yaml
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.
That would cause this change to affect podman build, right? when generating the kube we don't have access to the image file just the container we are extracting info from see libpod/kube.go L855
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 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.
Ok cool, that should have the same affect I think, I will try your approach.
Removed the inclusion of RunAsUser or RunAsGroup unless a container is run with the --user flag. When building from an image the user will be pulled from there anyway resolves containers#11914 Signed-off-by: cdoern <cdoern@redhat.com>
LGTM |
/lgtm |
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
Nice and simple :)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoern, Luap99 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 |
Removed the inclusion of RunAsUser or RunAsGroup unless a container is run with the --user flag. When building from an image
the user will be pulled from there anyway
resolves #11914
Signed-off-by: cdoern cdoern@redhat.com