refactor: separate policy mutation/validation handlers from server (#3905)

* refactor: webhooks server logger

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>

* refactor: separate policy mutation/validation handlers from server

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>

Co-authored-by: Vyankatesh Kudtarkar <vyankateshkd@gmail.com>

cmd/kyverno/main.go

@@ -37,6 +37,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/version"
 	"github.com/kyverno/kyverno/pkg/webhookconfig"
 	"github.com/kyverno/kyverno/pkg/webhooks"
+	webhookspolicy "github.com/kyverno/kyverno/pkg/webhooks/policy"
 	webhookgenerate "github.com/kyverno/kyverno/pkg/webhooks/updaterequest"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	kubeinformers "k8s.io/client-go/informers"
@@ -405,7 +406,10 @@ func main() {
 	// -- annotations on resources with update details on mutation JSON patches
 	// -- generate policy violation resource
 	// -- generate events on policy and resource
+	policyHandlers := webhookspolicy.NewHandlers(dynamicClient, openAPIController)
+
 	server, err := webhooks.NewWebhookServer(
+		policyHandlers,
 		kyvernoClient,
 		dynamicClient,
 		certManager.GetTLSPemPair,

pkg/webhooks/handlers.go

@@ -3,8 +3,6 @@ package webhooks
 import (
 	"fmt"
 	"net/http"
-	"reflect"
-	"strings"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -13,9 +11,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/common"
 	"github.com/kyverno/kyverno/pkg/engine"
 	enginectx "github.com/kyverno/kyverno/pkg/engine/context"
-	policyvalidate "github.com/kyverno/kyverno/pkg/policy"
 	"github.com/kyverno/kyverno/pkg/policycache"
-	"github.com/kyverno/kyverno/pkg/policymutation"
 	"github.com/kyverno/kyverno/pkg/userinfo"
 	"github.com/kyverno/kyverno/pkg/utils"
 	admissionutils "github.com/kyverno/kyverno/pkg/utils/admission"
@@ -23,20 +19,6 @@ import (
 	admissionv1 "k8s.io/api/admission/v1"
 )
 
-// TODO: use admission review sub resource ?
-func isStatusUpdate(old, new kyverno.PolicyInterface) bool {
-	if !reflect.DeepEqual(old.GetAnnotations(), new.GetAnnotations()) {
-		return false
-	}
-	if !reflect.DeepEqual(old.GetLabels(), new.GetLabels()) {
-		return false
-	}
-	if !reflect.DeepEqual(old.GetSpec(), new.GetSpec()) {
-		return false
-	}
-	return true
-}
-
 func errorResponse(logger logr.Logger, err error, message string) *admissionv1.AdmissionResponse {
 	logger.Error(err, message)
 	return admissionutils.ResponseFailure(false, message+": "+err.Error())
@@ -49,60 +31,6 @@ func (ws *WebhookServer) admissionHandler(logger logr.Logger, filter bool, inner
 	return handlers.Monitor(ws.webhookMonitor, handlers.Admission(logger, inner))
 }
 
-func (ws *WebhookServer) policyMutation(logger logr.Logger, request *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse {
-	policy, oldPolicy, 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 default value, check kyverno controller logs for details: %v", err))
-	}
-
-	if oldPolicy != nil && isStatusUpdate(oldPolicy, policy) {
-		logger.V(4).Info("skip policy mutation on status update")
-		return admissionutils.Response(true)
-	}
-
-	startTime := time.Now()
-	logger.V(3).Info("start policy change mutation")
-	defer logger.V(3).Info("finished policy change mutation", "time", time.Since(startTime).String())
-
-	// Generate JSON Patches for defaults
-	if patches, updateMsgs := policymutation.GenerateJSONPatchesForDefaults(policy, logger); len(patches) != 0 {
-		return admissionutils.ResponseWithMessageAndPatch(true, strings.Join(updateMsgs, "'"), patches)
-	}
-
-	return admissionutils.Response(true)
-}
-
-//policyValidation performs the validation check on policy resource
-func (ws *WebhookServer) policyValidation(logger logr.Logger, request *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse {
-	policy, oldPolicy, 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))
-	}
-
-	if oldPolicy != nil && isStatusUpdate(oldPolicy, policy) {
-		logger.V(4).Info("skip policy validation on status update")
-		return admissionutils.Response(true)
-	}
-
-	startTime := time.Now()
-	logger.V(3).Info("start policy change validation")
-	defer logger.V(3).Info("finished policy change validation", "time", time.Since(startTime).String())
-
-	response, err := policyvalidate.Validate(policy, ws.client, false, ws.openAPIController)
-	if err != nil {
-		logger.Error(err, "policy validation errors")
-		return admissionutils.ResponseWithMessage(false, err.Error())
-	}
-
-	if response != nil && len(response.Warnings) != 0 {
-		return response
-	}
-
-	return admissionutils.Response(true)
-}
-
 // resourceMutation mutates resource
 func (ws *WebhookServer) resourceMutation(logger logr.Logger, request *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse {
 	if excludeKyvernoResources(request.Kind.Kind) {

pkg/webhooks/policy/handlers.go

@@ -0,0 +1,95 @@
+package policy
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+	"time"
+
+	"github.com/go-logr/logr"
+	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
+	client "github.com/kyverno/kyverno/pkg/dclient"
+	"github.com/kyverno/kyverno/pkg/openapi"
+	policyvalidate "github.com/kyverno/kyverno/pkg/policy"
+	"github.com/kyverno/kyverno/pkg/policymutation"
+	admissionutils "github.com/kyverno/kyverno/pkg/utils/admission"
+	admissionv1 "k8s.io/api/admission/v1"
+)
+
+type Handlers interface {
+	// Mutate performs the mutation of policy resources
+	Mutate(logr.Logger, *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse
+	// Validate performs the validation check on policy resources
+	Validate(logr.Logger, *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse
+}
+
+type handlers struct {
+	client            client.Interface
+	openAPIController *openapi.Controller
+}
+
+func NewHandlers(
+	client client.Interface,
+	openAPIController *openapi.Controller,
+) Handlers {
+	return &handlers{
+		client:            client,
+		openAPIController: openAPIController,
+	}
+}
+
+func (h *handlers) Validate(logger logr.Logger, request *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse {
+	policy, oldPolicy, 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))
+	}
+	if oldPolicy != nil && isStatusUpdate(oldPolicy, policy) {
+		logger.V(4).Info("skip policy validation on status update")
+		return admissionutils.Response(true)
+	}
+	startTime := time.Now()
+	logger.V(3).Info("start policy change validation")
+	defer logger.V(3).Info("finished policy change validation", "time", time.Since(startTime).String())
+	response, err := policyvalidate.Validate(policy, h.client, false, h.openAPIController)
+	if err != nil {
+		logger.Error(err, "policy validation errors")
+		return admissionutils.ResponseWithMessage(false, err.Error())
+	}
+	if response != nil && len(response.Warnings) != 0 {
+		return response
+	}
+	return admissionutils.Response(true)
+}
+
+func (h *handlers) Mutate(logger logr.Logger, request *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse {
+	policy, oldPolicy, 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 default value, check kyverno controller logs for details: %v", err))
+	}
+	if oldPolicy != nil && isStatusUpdate(oldPolicy, policy) {
+		logger.V(4).Info("skip policy mutation on status update")
+		return admissionutils.Response(true)
+	}
+	startTime := time.Now()
+	logger.V(3).Info("start policy change mutation")
+	defer logger.V(3).Info("finished policy change mutation", "time", time.Since(startTime).String())
+	if patches, updateMsgs := policymutation.GenerateJSONPatchesForDefaults(policy, logger); len(patches) != 0 {
+		return admissionutils.ResponseWithMessageAndPatch(true, strings.Join(updateMsgs, "'"), patches)
+	}
+	return admissionutils.Response(true)
+}
+
+func isStatusUpdate(old, new kyvernov1.PolicyInterface) bool {
+	if !reflect.DeepEqual(old.GetAnnotations(), new.GetAnnotations()) {
+		return false
+	}
+	if !reflect.DeepEqual(old.GetLabels(), new.GetLabels()) {
+		return false
+	}
+	if !reflect.DeepEqual(old.GetSpec(), new.GetSpec()) {
+		return false
+	}
+	return true
+}

pkg/webhooks/server.go

@@ -38,6 +38,13 @@ import (
 	rbaclister "k8s.io/client-go/listers/rbac/v1"
 )
 
+type Handlers interface {
+	// Mutate performs the mutation of policy resources
+	Mutate(logr.Logger, *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse
+	// Validate performs the validation check on policy resources
+	Validate(logr.Logger, *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse
+}
+
 // WebhookServer contains configured TLS server with MutationWebhook.
 type WebhookServer struct {
 	server *http.Server
@@ -94,6 +101,7 @@ type WebhookServer struct {
 // NewWebhookServer creates new instance of WebhookServer accordingly to given configuration
 // Policy Controller and Kubernetes Client should be initialized in configuration
 func NewWebhookServer(
+	policyHandlers Handlers,
 	kyvernoClient kyvernoclient.Interface,
 	client client.Interface,
 	tlsPair func() ([]byte, []byte, error),
@@ -150,8 +158,8 @@ func NewWebhookServer(
 	verifyLogger := ws.log.WithName("verify")
 	mux.HandlerFunc("POST", config.MutatingWebhookServicePath, ws.admissionHandler(resourceLogger.WithName("mutate"), true, ws.resourceMutation))
 	mux.HandlerFunc("POST", config.ValidatingWebhookServicePath, ws.admissionHandler(resourceLogger.WithName("validate"), true, ws.resourceValidation))
-	mux.HandlerFunc("POST", config.PolicyMutatingWebhookServicePath, ws.admissionHandler(policyLogger.WithName("mutate"), true, ws.policyMutation))
-	mux.HandlerFunc("POST", config.PolicyValidatingWebhookServicePath, ws.admissionHandler(policyLogger.WithName("validate"), true, ws.policyValidation))
+	mux.HandlerFunc("POST", config.PolicyMutatingWebhookServicePath, ws.admissionHandler(policyLogger.WithName("mutate"), true, policyHandlers.Mutate))
+	mux.HandlerFunc("POST", config.PolicyValidatingWebhookServicePath, ws.admissionHandler(policyLogger.WithName("validate"), true, policyHandlers.Validate))
 	mux.HandlerFunc("POST", config.VerifyMutatingWebhookServicePath, ws.admissionHandler(verifyLogger.WithName("mutate"), false, handlers.Verify(ws.webhookMonitor, ws.log.WithName("verifyHandler"))))
 	mux.HandlerFunc("GET", config.LivenessServicePath, handlers.Probe(ws.webhookRegister.Check))
 	mux.HandlerFunc("GET", config.ReadinessServicePath, handlers.Probe(nil))