Move mutator validation to the validating webhook (#1153)

Signed-off-by: Max Smythe <smythe@google.com>

pkg/webhook/mutation.go

@@ -15,21 +15,18 @@ package webhook
 import (
 	"context"
 	"errors"
-	"fmt"
 	"net/http"
 	"time"
 
 	"github.com/open-policy-agent/cert-controller/pkg/rotator"
 	opa "github.com/open-policy-agent/frameworks/constraint/pkg/client"
 	"github.com/open-policy-agent/gatekeeper/apis"
-	mutationsv1alpha1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"
 	"github.com/open-policy-agent/gatekeeper/pkg/controller/config/process"
 	"github.com/open-policy-agent/gatekeeper/pkg/mutation"
 	"github.com/open-policy-agent/gatekeeper/pkg/util"
 	admissionv1 "k8s.io/api/admission/v1"
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -117,19 +114,6 @@ func (h *mutationHandler) Handle(ctx context.Context, req admission.Request) adm
 		return admission.ValidationResponse(true, "Mutating only on create or update")
 	}
 
-	if userErr, err := h.validateGatekeeperResources(ctx, req); err != nil {
-		vResp := admission.ValidationResponse(false, err.Error())
-		if vResp.Result == nil {
-			vResp.Result = &metav1.Status{}
-		}
-		if userErr {
-			vResp.Result.Code = http.StatusUnprocessableEntity
-		} else {
-			vResp.Result.Code = http.StatusInternalServerError
-		}
-		return vResp
-	}
-
 	if h.isGatekeeperResource(ctx, req) {
 		return admission.ValidationResponse(true, "Not mutating gatekeeper resources")
 	}
@@ -230,50 +214,3 @@ func AppendMutationWebhookIfEnabled(webhooks []rotator.WebhookInfo) []rotator.We
 	}
 	return webhooks
 }
-
-// validateGatekeeperResources returns whether an issue is user error (vs internal) and any errors
-// validating internal resources
-func (h *mutationHandler) validateGatekeeperResources(ctx context.Context, req admission.Request) (bool, error) {
-	if req.AdmissionRequest.Kind.Group == mutationsGroup && req.AdmissionRequest.Kind.Kind == "AssignMetadata" {
-		return h.validateAssignMetadata(ctx, req)
-	}
-	if req.AdmissionRequest.Kind.Group == mutationsGroup && req.AdmissionRequest.Kind.Kind == "Assign" {
-		return h.validateAssign(ctx, req)
-	}
-	return false, nil
-}
-
-func (h *mutationHandler) validateAssignMetadata(ctx context.Context, req admission.Request) (bool, error) {
-	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, &mutationsv1alpha1.AssignMetadata{})
-	if err != nil {
-		return false, err
-	}
-	assignMetadata, ok := obj.(*mutationsv1alpha1.AssignMetadata)
-	if !ok {
-		return false, fmt.Errorf("Deserialized object is not of type AssignMetadata")
-	}
-	err = mutation.IsValidAssignMetadata(assignMetadata)
-	if err != nil {
-		return true, err
-	}
-
-	return false, nil
-}
-
-func (h *mutationHandler) validateAssign(ctx context.Context, req admission.Request) (bool, error) {
-	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, &mutationsv1alpha1.Assign{})
-	if err != nil {
-		return false, err
-	}
-	assign, ok := obj.(*mutationsv1alpha1.Assign)
-	if !ok {
-		return false, fmt.Errorf("Deserialized object is not of type Assign")
-	}
-
-	err = mutation.IsValidAssign(assign)
-	if err != nil {
-		return true, err
-	}
-
-	return false, nil
-}

pkg/webhook/mutation_test.go → pkg/webhook/mutator_validation_test.go

@@ -97,7 +97,7 @@ spec:
 	}
 	for _, tt := range tc {
 		t.Run(tt.Name, func(t *testing.T) {
-			handler := mutationHandler{webhookHandler: webhookHandler{}}
+			handler := validationHandler{webhookHandler: webhookHandler{}}
 			b, err := yaml.YAMLToJSON([]byte(tt.AssignMeta))
 			if err != nil {
 				t.Fatalf("Error parsing yaml: %s", err)
@@ -270,7 +270,7 @@ spec:
 	}
 	for _, tt := range tc {
 		t.Run(tt.Name, func(t *testing.T) {
-			handler := mutationHandler{webhookHandler: webhookHandler{}}
+			handler := validationHandler{webhookHandler: webhookHandler{}}
 			b, err := yaml.YAMLToJSON([]byte(tt.Assign))
 			if err != nil {
 				t.Fatalf("Error parsing yaml: %s", err)

pkg/webhook/policy.go

@@ -28,6 +28,7 @@ import (
 	"github.com/open-policy-agent/frameworks/constraint/pkg/core/templates"
 	rtypes "github.com/open-policy-agent/frameworks/constraint/pkg/types"
 	"github.com/open-policy-agent/gatekeeper/apis"
+	mutationsv1alpha1 "github.com/open-policy-agent/gatekeeper/apis/mutations/v1alpha1"
 	"github.com/open-policy-agent/gatekeeper/pkg/controller/config/process"
 	"github.com/open-policy-agent/gatekeeper/pkg/keys"
 	"github.com/open-policy-agent/gatekeeper/pkg/logging"
@@ -278,6 +279,10 @@ func (h *validationHandler) validateGatekeeperResources(ctx context.Context, req
 		if err := h.validateConfigResource(ctx, req); err != nil {
 			return true, err
 		}
+	case req.AdmissionRequest.Kind.Group == mutationsGroup && req.AdmissionRequest.Kind.Kind == "AssignMetadata":
+		return h.validateAssignMetadata(ctx, req)
+	case req.AdmissionRequest.Kind.Group == mutationsGroup && req.AdmissionRequest.Kind.Kind == "Assign":
+		return h.validateAssign(ctx, req)
 	}
 
 	return false, nil
@@ -332,6 +337,41 @@ func (h *validationHandler) validateConfigResource(ctx context.Context, req admi
 	return nil
 }
 
+func (h *validationHandler) validateAssignMetadata(ctx context.Context, req admission.Request) (bool, error) {
+	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, &mutationsv1alpha1.AssignMetadata{})
+	if err != nil {
+		return false, err
+	}
+	assignMetadata, ok := obj.(*mutationsv1alpha1.AssignMetadata)
+	if !ok {
+		return false, fmt.Errorf("Deserialized object is not of type AssignMetadata")
+	}
+	err = mutation.IsValidAssignMetadata(assignMetadata)
+	if err != nil {
+		return true, err
+	}
+
+	return false, nil
+}
+
+func (h *validationHandler) validateAssign(ctx context.Context, req admission.Request) (bool, error) {
+	obj, _, err := deserializer.Decode(req.AdmissionRequest.Object.Raw, nil, &mutationsv1alpha1.Assign{})
+	if err != nil {
+		return false, err
+	}
+	assign, ok := obj.(*mutationsv1alpha1.Assign)
+	if !ok {
+		return false, fmt.Errorf("Deserialized object is not of type Assign")
+	}
+
+	err = mutation.IsValidAssign(assign)
+	if err != nil {
+		return true, err
+	}
+
+	return false, nil
+}
+
 // traceSwitch returns true if a request should be traced
 func (h *validationHandler) reviewRequest(ctx context.Context, req admission.Request) (*rtypes.Responses, error) {
 	// if we have a maximum number of concurrent serving goroutines, try to acquire