refactor: admission response utils (#5234)

- refactor: admission response utils
- unit tests

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

pkg/utils/admission/response.go

@@ -0,0 +1,30 @@
+package admission
+
+import (
+	admissionv1 "k8s.io/api/admission/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func Response(err error, warnings ...string) *admissionv1.AdmissionResponse {
+	response := &admissionv1.AdmissionResponse{
+		Allowed: err == nil,
+	}
+	if err != nil {
+		response.Result = &metav1.Status{
+			Status:  metav1.StatusFailure,
+			Message: err.Error(),
+		}
+	}
+	response.Warnings = warnings
+	return response
+}
+
+func ResponseSuccess(warnings ...string) *admissionv1.AdmissionResponse {
+	return Response(nil, warnings...)
+}
+
+func MutationResponse(patch []byte, warnings ...string) *admissionv1.AdmissionResponse {
+	response := ResponseSuccess(warnings...)
+	response.Patch = patch
+	return response
+}

pkg/utils/admission/response_test.go

@@ -0,0 +1,169 @@
+package admission
+
+import (
+	"errors"
+	"reflect"
+	"testing"
+
+	admissionv1 "k8s.io/api/admission/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestResponse(t *testing.T) {
+	type args struct {
+		err      error
+		warnings []string
+	}
+	tests := []struct {
+		name string
+		args args
+		want *admissionv1.AdmissionResponse
+	}{{
+		name: "no error, no warnings",
+		args: args{
+			err:      nil,
+			warnings: nil,
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed: true,
+		},
+	}, {
+		name: "no error, warnings",
+		args: args{
+			err:      nil,
+			warnings: []string{"foo", "bar"},
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed:  true,
+			Warnings: []string{"foo", "bar"},
+		},
+	}, {
+		name: "error, no warnings",
+		args: args{
+			err:      errors.New("an error has occured"),
+			warnings: nil,
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed: false,
+			Result: &metav1.Status{
+				Status:  metav1.StatusFailure,
+				Message: "an error has occured",
+			},
+		},
+	}, {
+		name: "error, warnings",
+		args: args{
+			err:      errors.New("an error has occured"),
+			warnings: []string{"foo", "bar"},
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed: false,
+			Result: &metav1.Status{
+				Status:  metav1.StatusFailure,
+				Message: "an error has occured",
+			},
+			Warnings: []string{"foo", "bar"},
+		},
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := Response(tt.args.err, tt.args.warnings...); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("Response() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestResponseSuccess(t *testing.T) {
+	type args struct {
+		warnings []string
+	}
+	tests := []struct {
+		name string
+		args args
+		want *admissionv1.AdmissionResponse
+	}{{
+		name: "no warnings",
+		args: args{
+			warnings: nil,
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed: true,
+		},
+	}, {
+		name: "warnings",
+		args: args{
+			warnings: []string{"foo", "bar"},
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed:  true,
+			Warnings: []string{"foo", "bar"},
+		},
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := ResponseSuccess(tt.args.warnings...); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("ResponseSuccess() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestMutationResponse(t *testing.T) {
+	type args struct {
+		patch    []byte
+		warnings []string
+	}
+	tests := []struct {
+		name string
+		args args
+		want *admissionv1.AdmissionResponse
+	}{{
+		name: "no patch, no warnings",
+		args: args{
+			patch:    nil,
+			warnings: nil,
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed: true,
+		},
+	}, {
+		name: "no patch, warnings",
+		args: args{
+			patch:    nil,
+			warnings: []string{"foo", "bar"},
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed:  true,
+			Warnings: []string{"foo", "bar"},
+		},
+	}, {
+		name: "patch, no warnings",
+		args: args{
+			patch:    []byte{1, 2, 3, 4},
+			warnings: nil,
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed: true,
+			Patch:   []byte{1, 2, 3, 4},
+		},
+	}, {
+		name: "patch, warnings",
+		args: args{
+			patch:    []byte{1, 2, 3, 4},
+			warnings: []string{"foo", "bar"},
+		},
+		want: &admissionv1.AdmissionResponse{
+			Allowed:  true,
+			Patch:    []byte{1, 2, 3, 4},
+			Warnings: []string{"foo", "bar"},
+		},
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := MutationResponse(tt.args.patch, tt.args.warnings...); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("MutationResponse() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

pkg/utils/admission/utils.go

@@ -6,7 +6,6 @@ import (
 
 	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
 	admissionv1 "k8s.io/api/admission/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func UnmarshalPolicy(kind string, raw []byte) (kyvernov1.PolicyInterface, error) {
@@ -42,68 +41,6 @@ func GetPolicies(request *admissionv1.AdmissionRequest) (kyvernov1.PolicyInterfa
 	return policy, nil, nil
 }
 
-func Response(allowed bool) *admissionv1.AdmissionResponse {
-	r := &admissionv1.AdmissionResponse{
-		Allowed: allowed,
-	}
-	return r
-}
-
-func ResponseWithMessage(allowed bool, msg string) *admissionv1.AdmissionResponse {
-	r := Response(allowed)
-	r.Result = &metav1.Status{
-		Message: msg,
-	}
-	return r
-}
-
-func ResponseWithMessageAndPatch(allowed bool, msg string, patch []byte) *admissionv1.AdmissionResponse {
-	r := ResponseWithMessage(allowed, msg)
-	r.Patch = patch
-	return r
-}
-
-func ResponseStatus(allowed bool, status, msg string) *admissionv1.AdmissionResponse {
-	r := Response(allowed)
-	r.Result = &metav1.Status{
-		Status:  status,
-		Message: msg,
-	}
-	return r
-}
-
-func ResponseFailure(msg string) *admissionv1.AdmissionResponse {
-	return ResponseStatus(false, metav1.StatusFailure, msg)
-}
-
-func ResponseSuccess() *admissionv1.AdmissionResponse {
-	return Response(true)
-}
-
-func ResponseSuccessWithWarnings(warnings []string) *admissionv1.AdmissionResponse {
-	r := Response(true)
-	r.Warnings = warnings
-	return r
-}
-
-func ResponseSuccessWithPatch(patch []byte) *admissionv1.AdmissionResponse {
-	r := Response(true)
-	if len(patch) > 0 {
-		r.Patch = patch
-	}
-	return r
-}
-
-func ResponseSuccessWithPatchAndWarnings(patch []byte, warnings []string) *admissionv1.AdmissionResponse {
-	r := Response(true)
-	if len(patch) > 0 {
-		r.Patch = patch
-	}
-
-	r.Warnings = warnings
-	return r
-}
-
 func GetResourceName(request *admissionv1.AdmissionRequest) string {
 	resourceName := request.Kind.Kind + "/" + request.Name
 	if request.Namespace != "" {
@@ -111,15 +48,3 @@ func GetResourceName(request *admissionv1.AdmissionRequest) string {
 	}
 	return resourceName
 }
-
-func ValidationResponse(err error, warnings ...string) *admissionv1.AdmissionResponse {
-	response := Response(err == nil)
-	if err != nil {
-		response.Result = &metav1.Status{
-			Status:  metav1.StatusFailure,
-			Message: err.Error(),
-		}
-	}
-	response.Warnings = warnings
-	return response
-}

pkg/webhooks/handlers/protect.go

@@ -1,6 +1,7 @@
 package handlers
 
 import (
+	"errors"
 	"fmt"
 	"time"
 
@@ -18,14 +19,14 @@ func Protect(inner AdmissionHandler) AdmissionHandler {
 		newResource, oldResource, err := utils.ExtractResources(nil, request)
 		if err != nil {
 			logger.Error(err, "Failed to extract resources")
-			return admissionutils.ResponseFailure(err.Error())
+			return admissionutils.Response(err)
 		}
 		for _, resource := range []unstructured.Unstructured{newResource, oldResource} {
 			resLabels := resource.GetLabels()
 			if resLabels[kyvernov1.LabelAppManagedBy] == kyvernov1.ValueKyvernoApp {
 				if request.UserInfo.Username != fmt.Sprintf("system:serviceaccount:%s:%s", config.KyvernoNamespace(), config.KyvernoServiceAccountName()) {
 					logger.Info("Access to the resource not authorized, this is a kyverno managed resource and should be altered only by kyverno")
-					return admissionutils.ResponseFailure("A kyverno managed resource can only be modified by kyverno")
+					return admissionutils.Response(errors.New("A kyverno managed resource can only be modified by kyverno"))
 				}
 			}
 		}

pkg/webhooks/handlers/verify.go

@@ -19,8 +19,8 @@ func Verify() AdmissionHandler {
 		bytes, err := patch.ToPatchBytes()
 		if err != nil {
 			logger.Error(err, "failed to build patch bytes")
-			return admissionutils.ResponseFailure(err.Error())
+			return admissionutils.Response(err)
 		}
-		return admissionutils.ResponseSuccessWithPatch(bytes)
+		return admissionutils.MutationResponse(bytes)
 	}
 }

pkg/webhooks/policy/handlers.go

@@ -1,7 +1,6 @@
 package policy
 
 import (
-	"fmt"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -28,21 +27,21 @@ func NewHandlers(client dclient.Interface, openApiManager openapi.Manager) webho
 func (h *handlers) Validate(logger logr.Logger, request *admissionv1.AdmissionRequest, _ time.Time) *admissionv1.AdmissionResponse {
 	if request.SubResource != "" {
 		logger.V(4).Info("skip policy validation on status update")
-		return admissionutils.Response(true)
+		return admissionutils.ResponseSuccess()
 	}
 	policy, _, err := admissionutils.GetPolicies(request)
 	if err != nil {
 		logger.Error(err, "failed to unmarshal policies from admission request")
-		return admissionutils.ResponseWithMessage(true, fmt.Sprintf("failed to validate policy, check kyverno controller logs for details: %v", err))
+		return admissionutils.Response(err)
 	}
 	warnings, err := policyvalidate.Validate(policy, h.client, false, h.openApiManager)
 	if err != nil {
 		logger.Error(err, "policy validation errors")
-		return admissionutils.ResponseWithMessage(false, err.Error())
+		return admissionutils.Response(err)
 	}
-	return admissionutils.ValidationResponse(err, warnings...)
+	return admissionutils.Response(err, warnings...)
 }
 
 func (h *handlers) Mutate(logger logr.Logger, request *admissionv1.AdmissionRequest, _ time.Time) *admissionv1.AdmissionResponse {
-	return admissionutils.Response(true)
+	return admissionutils.ResponseSuccess()
 }

pkg/webhooks/resource/handlers.go

@@ -1,6 +1,7 @@
 package resource
 
 import (
+	"errors"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -133,18 +134,13 @@ func (h *handlers) Validate(logger logr.Logger, request *admissionv1.AdmissionRe
 	ok, msg, warnings := vh.HandleValidation(h.metricsConfig, request, policies, policyContext, namespaceLabels, startTime)
 	if !ok {
 		logger.Info("admission request denied")
-		return admissionutils.ResponseFailure(msg)
+		return admissionutils.Response(errors.New(msg))
 	}
 
 	defer h.handleDelete(logger, request)
 	go h.createUpdateRequests(logger, request, policyContext, generatePolicies, mutatePolicies, startTime)
 
-	if warnings != nil {
-		return admissionutils.ResponseSuccessWithWarnings(warnings)
-	}
-
-	logger.V(4).Info("completed validating webhook")
-	return admissionutils.ResponseSuccess()
+	return admissionutils.Response(nil, warnings...)
 }
 
 func (h *handlers) Mutate(logger logr.Logger, request *admissionv1.AdmissionRequest, failurePolicy string, startTime time.Time) *admissionv1.AdmissionResponse {
@@ -167,7 +163,7 @@ func (h *handlers) Mutate(logger logr.Logger, request *admissionv1.AdmissionRequ
 	policyContext, err := h.pcBuilder.Build(request, mutatePolicies...)
 	if err != nil {
 		logger.Error(err, "failed to build policy context")
-		return admissionutils.ResponseFailure(err.Error())
+		return admissionutils.Response(err)
 	}
 	// update container images to a canonical form
 	if err := enginectx.MutateResourceWithImageInfo(request.Object.Raw, policyContext.JSONContext); err != nil {
@@ -177,30 +173,26 @@ func (h *handlers) Mutate(logger logr.Logger, request *admissionv1.AdmissionRequ
 	mutatePatches, mutateWarnings, err := mh.HandleMutation(h.metricsConfig, request, mutatePolicies, policyContext, startTime)
 	if err != nil {
 		logger.Error(err, "mutation failed")
-		return admissionutils.ResponseFailure(err.Error())
+		return admissionutils.Response(err)
 	}
 	newRequest := patchRequest(mutatePatches, request, logger)
 	// rebuild context to process images updated via mutate policies
 	policyContext, err = h.pcBuilder.Build(newRequest, mutatePolicies...)
 	if err != nil {
 		logger.Error(err, "failed to build policy context")
-		return admissionutils.ResponseFailure(err.Error())
+		return admissionutils.Response(err)
 	}
 	ivh := imageverification.NewImageVerificationHandler(logger, h.kyvernoClient, h.eventGen, h.admissionReports)
 	imagePatches, imageVerifyWarnings, err := ivh.Handle(h.metricsConfig, newRequest, verifyImagesPolicies, policyContext)
 	if err != nil {
 		logger.Error(err, "image verification failed")
-		return admissionutils.ResponseFailure(err.Error())
+		return admissionutils.Response(err)
 	}
 	patch := jsonutils.JoinPatches(mutatePatches, imagePatches)
-	if len(mutateWarnings) > 0 || len(imageVerifyWarnings) > 0 {
-		warnings := append(mutateWarnings, imageVerifyWarnings...)
-		logger.V(2).Info("mutation webhook", "warnings", warnings)
-		return admissionutils.ResponseSuccessWithPatchAndWarnings(patch, warnings)
-	}
-	admissionResponse := admissionutils.ResponseSuccessWithPatch(patch)
-	logger.V(4).Info("completed mutating webhook", "response", admissionResponse)
-	return admissionResponse
+	var warnings []string
+	warnings = append(warnings, mutateWarnings...)
+	warnings = append(warnings, imageVerifyWarnings...)
+	return admissionutils.MutationResponse(patch, warnings...)
 }
 
 func (h *handlers) handleDelete(logger logr.Logger, request *admissionv1.AdmissionRequest) {

pkg/webhooks/resource/utils.go

@@ -1,6 +1,8 @@
 package resource
 
 import (
+	"errors"
+
 	"github.com/go-logr/logr"
 	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
 	kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1"
@@ -18,7 +20,7 @@ type updateRequestResponse struct {
 
 func errorResponse(logger logr.Logger, err error, message string) *admissionv1.AdmissionResponse {
 	logger.Error(err, message)
-	return admissionutils.ResponseFailure(message + ": " + err.Error())
+	return admissionutils.Response(errors.New(message + ": " + err.Error()))
 }
 
 func patchRequest(patches []byte, request *admissionv1.AdmissionRequest, logger logr.Logger) *admissionv1.AdmissionRequest {