Skip to content
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

BUILD-596: set HostUsers=false for unprivileged build pods + UserNamespaces #245

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/build/controller/strategy/custom.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func init() {

// CustomBuildStrategy creates a build using a custom builder image.
type CustomBuildStrategy struct {
ClearHostUsers bool // use "hostUsers: false" to ask to run in a user namespace because we're "stateless" (i.e., have no volumes which will persist beyond the life of this pod)
}

// CreateBuildPod creates the pod to be used for the Custom build
Expand Down Expand Up @@ -138,7 +139,7 @@ func (bs *CustomBuildStrategy) CreateBuildPod(build *buildv1.Build, additionalCA
setupBuildCAs(build, pod, additionalCAs, internalRegistryHost)
setupContainersStorage(pod, &pod.Spec.Containers[0])
if securityContext == nil || securityContext.Privileged == nil || !*securityContext.Privileged {
setupBuilderAutonsUser(build, strategy.Env, pod)
setupBuilderAutonsUser(build, strategy.Env, bs.ClearHostUsers, pod)
setupBuilderDeviceFUSE(pod)
}
return pod, nil
Expand Down
3 changes: 3 additions & 0 deletions pkg/build/controller/strategy/custom_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,5 +290,8 @@ func TestCustomCreateBuildPodAutonsUser(t *testing.T) {
func(build *buildv1.Build, env corev1.EnvVar) {
build.Spec.Strategy.CustomStrategy.Env = append(build.Spec.Strategy.CustomStrategy.Env, env)
},
func(clearHostUsersForUserNS bool) {
strategy.ClearHostUsers = clearHostUsersForUserNS
},
)
}
3 changes: 2 additions & 1 deletion pkg/build/controller/strategy/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func init() {
type DockerBuildStrategy struct {
Image string
BuildCSIVolumesEnabled bool
ClearHostUsers bool // use "hostUsers: false" to ask to run in a user namespace because we're "stateless" (i.e., have no volumes which will persist beyond the life of this pod)
}

// CreateBuildPod creates the pod to be used for the Docker build
Expand Down Expand Up @@ -210,7 +211,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildv1.Build, additionalCA
setupBuildCAs(build, pod, additionalCAs, internalRegistryHost)
setupContainersStorage(pod, &pod.Spec.Containers[0])
if securityContext == nil || securityContext.Privileged == nil || !*securityContext.Privileged {
setupBuilderAutonsUser(build, strategy.Env, pod)
setupBuilderAutonsUser(build, strategy.Env, bs.ClearHostUsers, pod)
setupBuilderDeviceFUSE(pod)
}
setupBlobCache(pod)
Expand Down
3 changes: 3 additions & 0 deletions pkg/build/controller/strategy/docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,5 +251,8 @@ func TestDockerCreateBuildPodAutonsUser(t *testing.T) {
func(build *buildv1.Build, env corev1.EnvVar) {
build.Spec.Strategy.DockerStrategy.Env = append(build.Spec.Strategy.DockerStrategy.Env, env)
},
func(clearHostUsersForUserNS bool) {
strategy.ClearHostUsers = clearHostUsersForUserNS
},
)
}
3 changes: 2 additions & 1 deletion pkg/build/controller/strategy/sti.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type SourceBuildStrategy struct {
Image string
SecurityClient securityclient.SecurityV1Interface
BuildCSIVolumeseEnabled bool
ClearHostUsers bool // use "hostUsers: false" to ask to run in a user namespace because we're "stateless" (i.e., have no volumes which will persist beyond the life of this pod)
}

// DefaultDropCaps is the list of capabilities to drop if the current user cannot run as root
Expand Down Expand Up @@ -217,7 +218,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildv1.Build, additionalCA
setupBuildCAs(build, pod, additionalCAs, internalRegistryHost)
setupContainersStorage(pod, &pod.Spec.Containers[0])
if securityContext == nil || securityContext.Privileged == nil || !*securityContext.Privileged {
setupBuilderAutonsUser(build, strategy.Env, pod)
setupBuilderAutonsUser(build, strategy.Env, bs.ClearHostUsers, pod)
setupBuilderDeviceFUSE(pod)
}
setupBlobCache(pod)
Expand Down
3 changes: 3 additions & 0 deletions pkg/build/controller/strategy/sti_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -317,5 +317,8 @@ func TestS2ICreateBuildPodAutonsUser(t *testing.T) {
func(build *buildv1.Build, env corev1.EnvVar) {
build.Spec.Strategy.SourceStrategy.Env = append(build.Spec.Strategy.SourceStrategy.Env, env)
},
func(clearHostUsersForUserNS bool) {
strategy.ClearHostUsers = clearHostUsersForUserNS
},
)
}
6 changes: 5 additions & 1 deletion pkg/build/controller/strategy/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
kvalidation "k8s.io/apimachinery/pkg/util/validation"
"k8s.io/klog/v2"
"k8s.io/kubernetes/pkg/apis/policy"
"k8s.io/utils/pointer"

buildv1 "github.com/openshift/api/build/v1"
"github.com/openshift/library-go/pkg/build/naming"
Expand Down Expand Up @@ -570,9 +571,12 @@ func setupBuilderDeviceFUSE(pod *corev1.Pod) {

// Request that the builder be run in a user namespace, mapped from host ID
// ranges chosen by the node.
func setupBuilderAutonsUser(build *buildv1.Build, vars []corev1.EnvVar, pod *corev1.Pod) {
func setupBuilderAutonsUser(build *buildv1.Build, vars []corev1.EnvVar, clearHostUsersForUserNS bool, pod *corev1.Pod) {
metav1.SetMetaDataAnnotation(&pod.ObjectMeta, "io.openshift.builder", "")
metav1.SetMetaDataAnnotation(&pod.ObjectMeta, "io.kubernetes.cri-o.userns-mode", "auto:size=65536")
if clearHostUsersForUserNS {
pod.Spec.HostUsers = pointer.Bool(false)
}
}

// setupBuildCAs mounts certificate authorities for the build from a predetermined ConfigMap.
Expand Down
109 changes: 92 additions & 17 deletions pkg/build/controller/strategy/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -820,67 +820,135 @@ type buildPodCreator interface {
CreateBuildPod(build *buildv1.Build, additionalCAs map[string]string, internalRegistryHost string) (*corev1.Pod, error)
}

func testCreateBuildPodAutonsUser(t *testing.T, build *buildv1.Build, strategy buildPodCreator, addEnv func(build *buildv1.Build, env corev1.EnvVar)) {
func testCreateBuildPodAutonsUser(t *testing.T, build *buildv1.Build, strategy buildPodCreator, addEnv func(build *buildv1.Build, env corev1.EnvVar), clearHostUsersForUserNS func(clearHostUsersForUserNS bool)) {
for _, testCase := range []struct {
env string
expectError bool
privileged bool
annotations map[string]string
noAnnotations []string
env string
expectError bool
privileged bool
useUserNamespaceUsers bool
annotations map[string]string
noAnnotations []string
expectHostUsers bool
}{
{
env: "",
privileged: true,
env: "",
privileged: true,
useUserNamespaceUsers: false,
noAnnotations: []string{
"io.openshift.builder",
"io.kubernetes.cri-o.Devices",
"io.kubernetes.cri-o.userns-mode",
},
expectHostUsers: true,
},
{
env: "BUILD_PRIVILEGED=0",
privileged: false,
env: "",
privileged: true,
useUserNamespaceUsers: true,
noAnnotations: []string{
"io.openshift.builder",
"io.kubernetes.cri-o.Devices",
"io.kubernetes.cri-o.userns-mode",
},
expectHostUsers: true,
},
{
env: "BUILD_PRIVILEGED=0",
privileged: false,
useUserNamespaceUsers: false,
annotations: map[string]string{
"io.openshift.builder": "",
"io.kubernetes.cri-o.Devices": "/dev/fuse:rwm",
"io.kubernetes.cri-o.userns-mode": "auto:size=65536",
},
expectHostUsers: true, // won't set HostUsers: false without the UserNamespacesStatelessPodsSupport feature
},
{
env: "BUILD_PRIVILEGED=42",
privileged: true,
env: "BUILD_PRIVILEGED=0",
privileged: false,
useUserNamespaceUsers: true,
annotations: map[string]string{
"io.openshift.builder": "",
"io.kubernetes.cri-o.Devices": "/dev/fuse:rwm",
"io.kubernetes.cri-o.userns-mode": "auto:size=65536",
},
expectHostUsers: false,
},
{
env: "BUILD_PRIVILEGED=42",
privileged: true,
useUserNamespaceUsers: false,
noAnnotations: []string{
"io.openshift.builder",
"io.kubernetes.cri-o.Devices",
"io.kubernetes.cri-o.userns-mode",
},
expectHostUsers: true,
},
{
env: "BUILD_PRIVILEGED=false",
privileged: false,
env: "BUILD_PRIVILEGED=42",
privileged: true,
useUserNamespaceUsers: true,
noAnnotations: []string{
"io.openshift.builder",
"io.kubernetes.cri-o.Devices",
"io.kubernetes.cri-o.userns-mode",
},
expectHostUsers: true,
},
{
env: "BUILD_PRIVILEGED=false",
privileged: false,
useUserNamespaceUsers: false,
annotations: map[string]string{
"io.openshift.builder": "",
"io.kubernetes.cri-o.Devices": "/dev/fuse:rwm",
"io.kubernetes.cri-o.userns-mode": "auto:size=65536",
},
expectHostUsers: true, // won't set HostUsers: false without the UserNamespacesStatelessPodsSupport feature
},
{
env: "BUILD_PRIVILEGED=false",
privileged: false,
useUserNamespaceUsers: true,
annotations: map[string]string{
"io.openshift.builder": "",
"io.kubernetes.cri-o.Devices": "/dev/fuse:rwm",
"io.kubernetes.cri-o.userns-mode": "auto:size=65536",
},
expectHostUsers: false,
},
{
env: "BUILD_PRIVILEGED=true",
privileged: true,
env: "BUILD_PRIVILEGED=true",
privileged: true,
useUserNamespaceUsers: false,
noAnnotations: []string{
"io.openshift.builder",
"io.kubernetes.cri-o.Devices",
"io.kubernetes.cri-o.userns-mode",
},
expectHostUsers: true,
},
{
env: "BUILD_PRIVILEGED=true",
privileged: true,
useUserNamespaceUsers: true,
noAnnotations: []string{
"io.openshift.builder",
"io.kubernetes.cri-o.Devices",
"io.kubernetes.cri-o.userns-mode",
},
expectHostUsers: true,
},
} {
t.Run(testCase.env, func(t *testing.T) {
t.Run(testCase.env+fmt.Sprintf(",useUserNamespaceUsers=%t", testCase.useUserNamespaceUsers), func(t *testing.T) {
build := build.DeepCopy()
for _, envVar := range strings.Split(testCase.env, ":") {
if env := strings.SplitN(envVar, "=", 2); len(env) > 1 {
addEnv(build, corev1.EnvVar{Name: env[0], Value: env[1]})
}
}
clearHostUsersForUserNS(testCase.useUserNamespaceUsers)
actual, err := strategy.CreateBuildPod(build, nil, testInternalRegistryHost)
if err != nil {
t.Errorf("Unexpected error: %v", err)
Expand Down Expand Up @@ -915,6 +983,13 @@ func testCreateBuildPodAutonsUser(t *testing.T, build *buildv1.Build, strategy b
t.Errorf("Expected no %q annotation, but got one", annotation)
}
}
hostUsers := true
if actual.Spec.HostUsers != nil {
hostUsers = *actual.Spec.HostUsers
}
if hostUsers != testCase.expectHostUsers {
t.Errorf("Expected to see pod HostUsers=%t, got %t (%#v)", testCase.expectHostUsers, hostUsers, actual.Spec.HostUsers)
}
})
}
}
12 changes: 11 additions & 1 deletion pkg/cmd/controller/build.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package controller

import (
"fmt"
"strings"

clientset "k8s.io/client-go/kubernetes"
"k8s.io/klog/v2"
"k8s.io/kubernetes/pkg/features"

buildclient "github.com/openshift/client-go/build/clientset/versioned"
buildcontroller "github.com/openshift/openshift-controller-manager/pkg/build/controller/build"
Expand Down Expand Up @@ -55,12 +57,16 @@ func RunBuildController(ctx *ControllerContext) (bool, error) {

fg := ctx.OpenshiftControllerConfig.FeatureGates
csiVolumesEnabled := false
clearHostUsersForUserNS := false
if fg != nil {
for _, v := range fg {
v = strings.TrimSpace(v)
if v == "BuildCSIVolumes=true" {
csiVolumesEnabled = true
}
if v == fmt.Sprintf("%s=true", string(features.UserNamespacesStatelessPodsSupport)) {
clearHostUsersForUserNS = true
}
}
}

Expand All @@ -85,13 +91,17 @@ func RunBuildController(ctx *ControllerContext) (bool, error) {
DockerBuildStrategy: &buildstrategy.DockerBuildStrategy{
Image: imageTemplate.ExpandOrDie("docker-builder"),
BuildCSIVolumesEnabled: csiVolumesEnabled,
ClearHostUsers: clearHostUsersForUserNS,
},
SourceBuildStrategy: &buildstrategy.SourceBuildStrategy{
Image: imageTemplate.ExpandOrDie("docker-builder"),
SecurityClient: securityClient.SecurityV1(),
BuildCSIVolumeseEnabled: csiVolumesEnabled,
ClearHostUsers: clearHostUsersForUserNS,
},
CustomBuildStrategy: &buildstrategy.CustomBuildStrategy{
ClearHostUsers: clearHostUsersForUserNS,
},
CustomBuildStrategy: &buildstrategy.CustomBuildStrategy{},
BuildDefaults: builddefaults.BuildDefaults{Config: ctx.OpenshiftControllerConfig.Build.BuildDefaults},
BuildOverrides: buildoverrides.BuildOverrides{Config: ctx.OpenshiftControllerConfig.Build.BuildOverrides},
InternalRegistryHostname: ctx.OpenshiftControllerConfig.DockerPullSecret.InternalRegistryHostname,
Expand Down