-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
UPSTREAM: 58977: Fix pod sandbox privilege. #18820
UPSTREAM: 58977: Fix pod sandbox privilege. #18820
Conversation
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@@ -302,7 +302,7 @@ func GetContainerSpec(pod *v1.Pod, containerName string) *v1.Container { | |||
|
|||
// HasPrivilegedContainer returns true if any of the containers in the pod are privileged. | |||
func HasPrivilegedContainer(pod *v1.Pod) bool { | |||
for _, c := range pod.Spec.Containers { | |||
for _, c := range append(pod.Spec.Containers, pod.Spec.InitContainers...) { |
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.
This change seems necessary but I don't think the container where we're seeing the rejection in the bug is an init container(we do have privileged init containers in our build pods but they only come into play in certain circumstances), so i'm uncertain this will actually fix the issue we're seeing.
https://github.com/openshift/origin/blob/master/pkg/build/controller/strategy/sti.go#L73-L177
Basically we use privileged init containers to pull images if you have an "image source input" in your build.
the build in bug https://bugzilla.redhat.com/show_bug.cgi?id=1550015 would not be doing that:
oc new-app --template=nodejs-mongo-persistent
so i think it is failing in the main container, as defined here:
https://github.com/openshift/origin/blob/master/pkg/build/controller/strategy/sti.go#L81-L101
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.
also since our main container is always privileged, this change wouldn't have any effect on whether this function returns true or false for our pods (it would have returned true before, and it will still return true)
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.
Yeah that's the one we see failing. Need to debug why the privileged isn't taking effect.
Yep it is failing in the main container. Can we make it explicitly privileged? I have searched the templates and can’t find where we are setting privileged for it. Kube calculates pod priv from containers so we should explicitly make a container priv if it needs it.
… On Mar 3, 2018, at 10:56 AM, Ben Parees ***@***.***> wrote:
@bparees commented on this pull request.
In vendor/k8s.io/kubernetes/pkg/kubelet/container/helpers.go:
> @@ -302,7 +302,7 @@ func GetContainerSpec(pod *v1.Pod, containerName string) *v1.Container {
// HasPrivilegedContainer returns true if any of the containers in the pod are privileged.
func HasPrivilegedContainer(pod *v1.Pod) bool {
- for _, c := range pod.Spec.Containers {
+ for _, c := range append(pod.Spec.Containers, pod.Spec.InitContainers...) {
This change seems necesary but I don't think the container where we're seeing the rejection in the bug is an init container(we do have privileged init containers in our build pods but they only come into play in certain circumstances), so i'm uncertain this will actually fix the issue we're seeing.
https://github.com/openshift/origin/blob/master/pkg/build/controller/strategy/sti.go#L73-L177
Basically we use privileged init containers to pull images if you have an "image source input" in your build.
the build in bug https://bugzilla.redhat.com/show_bug.cgi?id=1550015 would not be doing that:
oc new-app --template=nodejs-mongo-persistent
so i think it is failing in the main container, as defined here:
https://github.com/openshift/origin/blob/master/pkg/build/controller/strategy/sti.go#L81-L101
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or mute the thread.
|
the root cause for the build failure was identified and was unrelated to this change. the issue was related to our moving docker storage on crio nodes under /var/lib/containers and selinux labeling was wrong. either way, this is still a useful fix to have. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, runcom 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 |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. |
Aim to fix the selinux issue we're seeing between cri-o and docker on builds
/cc @bparees @smarterclayton @mrunalp @rhatdan @derekwaynecarr
Signed-off-by: Antonio Murdaca runcom@redhat.com