Merge pull request #16647 from jim-minter/issue14909

Automatic merge from submit-queue (batch tested with PRs 16657, 16607, 16647, 16639, 16655).

use PodSecurityPolicySubjectReview in build controller to avoid actually submitting a pod

fixes #14909

pkg/build/controller/strategy/sti.go

@@ -4,17 +4,16 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/golang/glog"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apiserver/pkg/admission"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
-	"k8s.io/kubernetes/pkg/serviceaccount"
 
 	buildapi "github.com/openshift/origin/pkg/build/apis/build"
-
 	buildutil "github.com/openshift/origin/pkg/build/util"
+	"github.com/openshift/origin/pkg/security/apis/security"
+	securityinternalversion "github.com/openshift/origin/pkg/security/generated/internalclientset/typed/security/internalversion"
 )
 
 // SourceBuildStrategy creates STI(source to image) builds
@@ -23,8 +22,8 @@ type SourceBuildStrategy struct {
 	// Codec is the codec to use for encoding the output pod.
 	// IMPORTANT: This may break backwards compatibility when
 	// it changes.
-	Codec            runtime.Codec
-	AdmissionControl admission.Interface
+	Codec          runtime.Codec
+	SecurityClient securityinternalversion.SecurityInterface
 }
 
 // DefaultDropCaps is the list of capabilities to drop if the current user cannot run as root
@@ -189,32 +188,29 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 }
 
 func (bs *SourceBuildStrategy) canRunAsRoot(build *buildapi.Build) bool {
-	var rootUser int64
-	rootUser = 0
-	pod := &kapi.Pod{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      buildapi.GetBuildPodName(build) + "-admissioncheck",
-			Namespace: build.Namespace,
-		},
-		Spec: kapi.PodSpec{
-			ServiceAccountName: build.Spec.ServiceAccount,
-			Containers: []kapi.Container{
-				{
-					Name:  "sti-build",
-					Image: bs.Image,
-					SecurityContext: &kapi.SecurityContext{
-						RunAsUser: &rootUser,
+	rootUser := int64(0)
+
+	review, err := bs.SecurityClient.PodSecurityPolicySubjectReviews(build.Namespace).Create(
+		&security.PodSecurityPolicySubjectReview{
+			Spec: security.PodSecurityPolicySubjectReviewSpec{
+				Template: kapi.PodTemplateSpec{
+					Spec: kapi.PodSpec{
+						ServiceAccountName: build.Spec.ServiceAccount,
+						Containers: []kapi.Container{
+							{
+								SecurityContext: &kapi.SecurityContext{
+									RunAsUser: &rootUser,
+								},
+							},
+						},
 					},
 				},
 			},
-			RestartPolicy: kapi.RestartPolicyNever,
 		},
-	}
-	userInfo := serviceaccount.UserInfo(build.Namespace, build.Spec.ServiceAccount, "")
-	attrs := admission.NewAttributesRecord(pod, nil, kapi.Kind("Pod").WithVersion(""), pod.Namespace, pod.Name, kapi.Resource("pods").WithVersion(""), "", admission.Create, userInfo)
-	err := bs.AdmissionControl.Admit(attrs)
+	)
 	if err != nil {
-		glog.V(2).Infof("Admit for root user returned error: %v", err)
+		utilruntime.HandleError(err)
+		return false
 	}
-	return err == nil
+	return review.Status.AllowedBy != nil
 }

pkg/build/controller/strategy/sti_test.go

@@ -1,7 +1,6 @@
 package strategy
 
 import (
-	"fmt"
 	"reflect"
 	"strings"
 	"testing"
@@ -12,7 +11,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation"
-	"k8s.io/apiserver/pkg/admission"
+	clienttesting "k8s.io/client-go/testing"
 	kapi "k8s.io/kubernetes/pkg/api"
 	kapihelper "k8s.io/kubernetes/pkg/api/helper"
 	"k8s.io/kubernetes/pkg/api/v1"
@@ -21,21 +20,26 @@ import (
 	_ "github.com/openshift/origin/pkg/build/apis/build/install"
 	"github.com/openshift/origin/pkg/build/util"
 	buildutil "github.com/openshift/origin/pkg/build/util"
+	"github.com/openshift/origin/pkg/security/apis/security"
+	securityinternalversion "github.com/openshift/origin/pkg/security/generated/internalclientset/typed/security/internalversion"
+	"github.com/openshift/origin/pkg/security/generated/internalclientset/typed/security/internalversion/fake"
 )
 
-type FakeAdmissionControl struct {
-	admit bool
-}
-
-func (a *FakeAdmissionControl) Admit(attr admission.Attributes) (err error) {
-	if a.admit {
-		return nil
-	}
-	return fmt.Errorf("pod not allowed")
-}
+func newFakeSecurityClient(rootAllowed bool) securityinternalversion.SecurityInterface {
+	securityClient := &fake.FakeSecurity{Fake: &clienttesting.Fake{}}
+	securityClient.AddReactor("*", "*", func(clienttesting.Action) (bool, runtime.Object, error) {
+		var ref *kapi.ObjectReference
+		if rootAllowed {
+			ref = &kapi.ObjectReference{} // i.e., not nil
+		}
 
-func (a *FakeAdmissionControl) Handles(operation admission.Operation) bool {
-	return true
+		return true, &security.PodSecurityPolicySubjectReview{
+			Status: security.PodSecurityPolicySubjectReviewStatus{
+				AllowedBy: ref,
+			},
+		}, nil
+	})
+	return securityClient
 }
 
 func TestSTICreateBuildPodRootNotAllowed(t *testing.T) {
@@ -50,9 +54,9 @@ var nodeSelector = map[string]string{"node": "mynode"}
 
 func testSTICreateBuildPod(t *testing.T, rootAllowed bool) {
 	strategy := &SourceBuildStrategy{
-		Image:            "sti-test-image",
-		Codec:            kapi.Codecs.LegacyCodec(buildapi.LegacySchemeGroupVersion),
-		AdmissionControl: &FakeAdmissionControl{admit: rootAllowed},
+		Image:          "sti-test-image",
+		Codec:          kapi.Codecs.LegacyCodec(buildapi.LegacySchemeGroupVersion),
+		SecurityClient: newFakeSecurityClient(rootAllowed),
 	}
 
 	build := mockSTIBuild()
@@ -169,9 +173,9 @@ func testSTICreateBuildPod(t *testing.T, rootAllowed bool) {
 
 func TestS2IBuildLongName(t *testing.T) {
 	strategy := &SourceBuildStrategy{
-		Image:            "sti-test-image",
-		Codec:            kapi.Codecs.LegacyCodec(buildapi.LegacySchemeGroupVersion),
-		AdmissionControl: &FakeAdmissionControl{admit: true},
+		Image:          "sti-test-image",
+		Codec:          kapi.Codecs.LegacyCodec(buildapi.LegacySchemeGroupVersion),
+		SecurityClient: newFakeSecurityClient(true),
 	}
 	build := mockSTIBuild()
 	build.Name = strings.Repeat("a", validation.DNS1123LabelMaxLength*2)

pkg/cmd/server/origin/controller/build.go

@@ -42,6 +42,7 @@ func (c *BuildControllerConfig) RunController(ctx ControllerContext) (bool, erro
 	kubeClient := ctx.ClientBuilder.KubeInternalClientOrDie(bootstrappolicy.InfraBuildControllerServiceAccountName)
 	buildClient := ctx.ClientBuilder.OpenshiftInternalBuildClientOrDie(bootstrappolicy.InfraBuildControllerServiceAccountName)
 	externalKubeClient := ctx.ClientBuilder.ClientOrDie(bootstrappolicy.InfraBuildControllerServiceAccountName)
+	securityClient := ctx.ClientBuilder.OpenshiftInternalSecurityClientOrDie(bootstrappolicy.InfraBuildControllerServiceAccountName)
 
 	buildInformer := ctx.BuildInformers.Build().InternalVersion().Builds()
 	buildConfigInformer := ctx.BuildInformers.Build().InternalVersion().BuildConfigs()
@@ -66,8 +67,8 @@ func (c *BuildControllerConfig) RunController(ctx ControllerContext) (bool, erro
 		SourceBuildStrategy: &buildstrategy.SourceBuildStrategy{
 			Image: c.S2IImage,
 			// TODO: this will be set to --storage-version (the internal schema we use)
-			Codec:            c.Codec,
-			AdmissionControl: sccAdmission,
+			Codec:          c.Codec,
+			SecurityClient: securityClient.Security(),
 		},
 		CustomBuildStrategy: &buildstrategy.CustomBuildStrategy{
 			// TODO: this will be set to --storage-version (the internal schema we use)

pkg/cmd/server/origin/controller/interfaces.go

@@ -20,6 +20,7 @@ import (
 	quotainformer "github.com/openshift/origin/pkg/quota/generated/informers/internalversion"
 	quotaclient "github.com/openshift/origin/pkg/quota/generated/internalclientset"
 	securityinformer "github.com/openshift/origin/pkg/security/generated/informers/internalversion"
+	securityclient "github.com/openshift/origin/pkg/security/generated/internalclientset"
 	templateinformer "github.com/openshift/origin/pkg/template/generated/informers/internalversion"
 	templateclient "github.com/openshift/origin/pkg/template/generated/internalclientset"
 )
@@ -96,6 +97,9 @@ type ControllerClientBuilder interface {
 
 	OpenshiftInternalNetworkClient(name string) (networkclientinternal.Interface, error)
 	OpenshiftInternalNetworkClientOrDie(name string) networkclientinternal.Interface
+
+	OpenshiftInternalSecurityClient(name string) (securityclient.Interface, error)
+	OpenshiftInternalSecurityClientOrDie(name string) securityclient.Interface
 }
 
 // InitFunc is used to launch a particular controller.  It may run additional "should I activate checks".
@@ -251,3 +255,25 @@ func (b OpenshiftControllerClientBuilder) OpenshiftInternalNetworkClientOrDie(na
 	}
 	return client
 }
+
+// OpenshiftInternalSecurityClient provides a REST client for the security API.
+// If the client cannot be created because of configuration error, this function
+// will error.
+func (b OpenshiftControllerClientBuilder) OpenshiftInternalSecurityClient(name string) (securityclient.Interface, error) {
+	clientConfig, err := b.Config(name)
+	if err != nil {
+		return nil, err
+	}
+	return securityclient.NewForConfig(clientConfig)
+}
+
+// OpenshiftInternalSecurityClientOrDie provides a REST client for the security API.
+// If the client cannot be created because of configuration error, this function
+// will panic.
+func (b OpenshiftControllerClientBuilder) OpenshiftInternalSecurityClientOrDie(name string) securityclient.Interface {
+	client, err := b.OpenshiftInternalSecurityClient(name)
+	if err != nil {
+		glog.Fatal(err)
+	}
+	return client
+}