Include update operation in build admission controller

pkg/build/admission/admission.go

@@ -7,6 +7,7 @@ import (
 
 	"k8s.io/kubernetes/pkg/admission"
 	kapi "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/meta"
 	kclient "k8s.io/kubernetes/pkg/client/unversioned"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/util/sets"
@@ -35,7 +36,7 @@ type buildByStrategy struct {
 // on policy based on the build strategy type
 func NewBuildByStrategy(client client.Interface) admission.Interface {
 	return &buildByStrategy{
-		Handler: admission.NewHandler(admission.Create),
+		Handler: admission.NewHandler(admission.Create, admission.Update),
 		client:  client,
 	}
 }
@@ -49,6 +50,16 @@ func (a *buildByStrategy) Admit(attr admission.Attributes) error {
 	if resource := attr.GetResource(); resource != buildsResource && resource != buildConfigsResource {
 		return nil
 	}
+	// TODO: Remove this check when upstream patch adm ctrl is fixed
+	if attr.GetOperation() == admission.Update {
+		valid, err := isValidUpdate(attr.GetObject())
+		if err != nil {
+			return err
+		}
+		if !valid {
+			return nil
+		}
+	}
 	switch obj := attr.GetObject().(type) {
 	case *buildapi.Build:
 		return a.checkBuildAuthorization(obj, attr)
@@ -57,7 +68,7 @@ func (a *buildByStrategy) Admit(attr admission.Attributes) error {
 	case *buildapi.BuildRequest:
 		return a.checkBuildRequestAuthorization(obj, attr)
 	default:
-		return admission.NewForbidden(attr, fmt.Errorf("Unrecognized request object %#v", obj))
+		return admission.NewForbidden(attr, fmt.Errorf("unrecognized request object %#v", obj))
 	}
 }
 
@@ -143,3 +154,11 @@ func (a *buildByStrategy) checkAccess(strategy buildapi.BuildStrategy, subjectAc
 func notAllowed(strategy buildapi.BuildStrategy, attr admission.Attributes) error {
 	return admission.NewForbidden(attr, fmt.Errorf("build strategy %s is not allowed", buildapi.StrategyType(strategy)))
 }
+
+func isValidUpdate(obj runtime.Object) (bool, error) {
+	accessor, err := meta.Accessor(obj)
+	if err != nil {
+		return false, err
+	}
+	return len(accessor.ResourceVersion()) > 0, nil
+}

pkg/build/admission/admission_test.go

@@ -130,19 +130,22 @@ func TestBuildAdmission(t *testing.T) {
 		},
 	}
 
+	ops := []admission.Operation{admission.Create, admission.Update}
 	for _, test := range tests {
-		c := NewBuildByStrategy(fakeClient(test.expectedResource, test.reviewResponse, test.responseObject))
-		attrs := admission.NewAttributesRecord(test.object, test.kind, "default", "name", test.resource, test.subResource, admission.Create, fakeUser())
-		err := c.Admit(attrs)
-		if err != nil && test.expectAccept {
-			t.Errorf("%s: unexpected error: %v", test.name, err)
-		}
+		for _, op := range ops {
+			c := NewBuildByStrategy(fakeClient(test.expectedResource, test.reviewResponse, test.responseObject))
+			attrs := admission.NewAttributesRecord(test.object, test.kind, "default", "name", test.resource, test.subResource, op, fakeUser())
+			err := c.Admit(attrs)
+			if err != nil && test.expectAccept {
+				t.Errorf("%s: unexpected error: %v", test.name, err)
+			}
 
-		if !apierrors.IsForbidden(err) && !test.expectAccept {
-			if (len(test.expectedError) != 0) || (test.expectedError == err.Error()) {
-				continue
+			if !apierrors.IsForbidden(err) && !test.expectAccept {
+				if (len(test.expectedError) != 0) || (test.expectedError == err.Error()) {
+					continue
+				}
+				t.Errorf("%s: expecting reject error, got %v", test.name, err)
 			}
-			t.Errorf("%s: expecting reject error, got %v", test.name, err)
 		}
 	}
 }

test/integration/build_admission_test.go

@@ -54,6 +54,15 @@ func TestPolicyBasedRestrictionOfBuildCreateAndCloneByStrategy(t *testing.T) {
 		}
 	}
 
+	// make sure build updates are rejected
+	for _, strategy := range buildStrategyTypes() {
+		for clientType, client := range clients {
+			if _, err := updateBuild(t, client.Builds(testutil.Namespace()), builds[string(strategy)+clientType]); !kapierror.IsForbidden(err) {
+				t.Errorf("expected forbidden for strategy %s and client %s: got %v", strategy, clientType, err)
+			}
+		}
+	}
+
 	// make sure clone is rejected
 	for _, strategy := range buildStrategyTypes() {
 		for clientType, client := range clients {
@@ -100,6 +109,15 @@ func TestPolicyBasedRestrictionOfBuildConfigCreateAndInstantiateByStrategy(t *te
 		}
 	}
 
+	// make sure buildconfig updates are rejected
+	for _, strategy := range buildStrategyTypes() {
+		for clientType, client := range clients {
+			if _, err := updateBuildConfig(t, client.BuildConfigs(testutil.Namespace()), buildConfigs[string(strategy)+clientType]); !kapierror.IsForbidden(err) {
+				t.Errorf("expected forbidden for strategy %s and client %s: got %v", strategy, clientType, err)
+			}
+		}
+	}
+
 	// make sure instantiate is rejected
 	for _, strategy := range buildStrategyTypes() {
 		for clientType, client := range clients {
@@ -226,6 +244,11 @@ func createBuild(t *testing.T, buildInterface client.BuildInterface, strategy st
 	return buildInterface.Create(build)
 }
 
+func updateBuild(t *testing.T, buildInterface client.BuildInterface, build *buildapi.Build) (*buildapi.Build, error) {
+	build.Labels = map[string]string{"updated": "true"}
+	return buildInterface.Update(build)
+}
+
 func createBuildConfig(t *testing.T, buildConfigInterface client.BuildConfigInterface, strategy string) (*buildapi.BuildConfig, error) {
 	buildConfig := &buildapi.BuildConfig{}
 	buildConfig.GenerateName = strings.ToLower(string(strategy)) + "-buildconfig-"
@@ -246,3 +269,8 @@ func instantiateBuildConfig(t *testing.T, buildConfigInterface client.BuildConfi
 	req.Name = buildConfig.Name
 	return buildConfigInterface.Instantiate(req)
 }
+
+func updateBuildConfig(t *testing.T, buildConfigInterface client.BuildConfigInterface, buildConfig *buildapi.BuildConfig) (*buildapi.BuildConfig, error) {
+	buildConfig.Labels = map[string]string{"updated": "true"}
+	return buildConfigInterface.Update(buildConfig)
+}