refactor: webhook block and unit tests (#4531)

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

pkg/utils/engine/response.go

@@ -22,10 +22,8 @@ func BlockRequest(er *response.EngineResponse, failurePolicy kyvernov1.FailurePo
 	if er.IsFailed() && er.GetValidationFailureAction() == kyvernov1.Enforce {
 		return true
 	}
-
 	if er.IsError() && failurePolicy == kyvernov1.Fail {
 		return true
 	}
-
 	return false
 }

pkg/webhooks/resource/handlers.go

@@ -349,7 +349,7 @@ func (h *handlers) handleVerifyImages(logger logr.Logger, request *admissionv1.A
 	}
 
 	failurePolicy := policyContext.Policy.GetSpec().GetFailurePolicy()
-	blocked := blockRequest(engineResponses, failurePolicy, logger)
+	blocked := webhookutils.BlockRequest(engineResponses, failurePolicy, logger)
 	if !isResourceDeleted(policyContext) {
 		events := generateEvents(engineResponses, blocked, logger)
 		h.eventGen.Add(events...)
@@ -357,7 +357,7 @@ func (h *handlers) handleVerifyImages(logger logr.Logger, request *admissionv1.A
 
 	if blocked {
 		logger.V(4).Info("admission request blocked")
-		return false, getBlockedMessages(engineResponses), nil, nil
+		return false, webhookutils.GetBlockedMessages(engineResponses), nil, nil
 	}
 
 	prInfos := policyreport.GeneratePRsFromEngineResponse(engineResponses, logger)

pkg/webhooks/resource/utils.go

@@ -16,9 +16,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/engine/variables"
 	"github.com/kyverno/kyverno/pkg/policyreport"
 	admissionutils "github.com/kyverno/kyverno/pkg/utils/admission"
-	engineutils2 "github.com/kyverno/kyverno/pkg/utils/engine"
 	"github.com/kyverno/kyverno/pkg/webhooks/updaterequest"
-	yamlv2 "gopkg.in/yaml.v2"
 	admissionv1 "k8s.io/api/admission/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
@@ -68,68 +66,6 @@ func buildDeletionPrInfo(oldR unstructured.Unstructured) policyreport.Info {
 	}
 }
 
-// returns true -> if there is even one policy that blocks resource request
-// returns false -> if all the policies are meant to report only, we dont block resource request
-func blockRequest(engineReponses []*response.EngineResponse, failurePolicy kyvernov1.FailurePolicyType, log logr.Logger) bool {
-	for _, er := range engineReponses {
-		if engineutils2.BlockRequest(er, failurePolicy) {
-			log.V(2).Info("blocking admission request", "policy", er.PolicyResponse.Policy.Name)
-			return true
-		}
-	}
-
-	log.V(4).Info("allowing admission request")
-	return false
-}
-
-// getBlockedMessages gets the error messages for rules with error or fail status
-func getBlockedMessages(engineResponses []*response.EngineResponse) string {
-	if len(engineResponses) == 0 {
-		return ""
-	}
-
-	failures := make(map[string]interface{})
-	hasViolations := false
-	for _, er := range engineResponses {
-		ruleToReason := make(map[string]string)
-		for _, rule := range er.PolicyResponse.Rules {
-			if rule.Status != response.RuleStatusPass {
-				ruleToReason[rule.Name] = rule.Message
-				if rule.Status == response.RuleStatusFail {
-					hasViolations = true
-				}
-			}
-		}
-
-		failures[er.PolicyResponse.Policy.Name] = ruleToReason
-	}
-
-	if len(failures) == 0 {
-		return ""
-	}
-
-	r := engineResponses[0].PolicyResponse.Resource
-	resourceName := fmt.Sprintf("%s/%s/%s", r.Kind, r.Namespace, r.Name)
-	action := getAction(hasViolations, len(failures))
-
-	results, _ := yamlv2.Marshal(failures)
-	msg := fmt.Sprintf("\n\npolicy %s for resource %s: \n\n%s", resourceName, action, results)
-	return msg
-}
-
-func getAction(hasViolations bool, i int) string {
-	action := "error"
-	if hasViolations {
-		action = "violation"
-	}
-
-	if i > 1 {
-		action = action + "s"
-	}
-
-	return action
-}
-
 func getErrorMsg(engineReponses []*response.EngineResponse) string {
 	var str []string
 	var resourceInfo string

pkg/webhooks/resource/validation.go

@@ -84,7 +84,7 @@ func (v *validationHandler) handleValidation(
 		}
 	}
 
-	blocked := blockRequest(engineResponses, failurePolicy, logger)
+	blocked := webhookutils.BlockRequest(engineResponses, failurePolicy, logger)
 	if deletionTimeStamp == nil {
 		events := generateEvents(engineResponses, blocked, logger)
 		v.eventGen.Add(events...)
@@ -93,7 +93,7 @@ func (v *validationHandler) handleValidation(
 	if blocked {
 		logger.V(4).Info("admission request blocked")
 		v.generateMetrics(request, admissionRequestTimestamp, engineResponses, metricsConfig, logger)
-		return false, getBlockedMessages(engineResponses), nil
+		return false, webhookutils.GetBlockedMessages(engineResponses), nil
 	}
 
 	v.generateReportChangeRequests(request, engineResponses, policyContext, logger)

pkg/webhooks/resource/validation_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/engine/context"
 	"github.com/kyverno/kyverno/pkg/engine/response"
 	"github.com/kyverno/kyverno/pkg/engine/utils"
+	webhookutils "github.com/kyverno/kyverno/pkg/webhooks/utils"
 	"gotest.tools/assert"
 )
 
@@ -539,7 +540,7 @@ func TestValidate_failure_action_overrides(t *testing.T) {
 			}
 
 			failurePolicy := kyvernov1.Fail
-			blocked := blockRequest([]*response.EngineResponse{er}, failurePolicy, log.Log.WithName("WebhookServer"))
+			blocked := webhookutils.BlockRequest([]*response.EngineResponse{er}, failurePolicy, log.Log.WithName("WebhookServer"))
 			assert.Assert(t, tc.blocked == blocked)
 		})
 	}
@@ -594,7 +595,7 @@ func Test_RuleSelector(t *testing.T) {
 	assert.Assert(t, resp.PolicyResponse.RulesErrorCount == 0)
 
 	log := log.Log.WithName("Test_RuleSelector")
-	blocked := blockRequest([]*response.EngineResponse{resp}, kyvernov1.Fail, log)
+	blocked := webhookutils.BlockRequest([]*response.EngineResponse{resp}, kyvernov1.Fail, log)
 	assert.Assert(t, blocked == true)
 
 	applyOne := kyvernov1.ApplyOne
@@ -604,6 +605,6 @@ func Test_RuleSelector(t *testing.T) {
 	assert.Assert(t, resp.PolicyResponse.RulesAppliedCount == 1)
 	assert.Assert(t, resp.PolicyResponse.RulesErrorCount == 0)
 
-	blocked = blockRequest([]*response.EngineResponse{resp}, kyvernov1.Fail, log)
+	blocked = webhookutils.BlockRequest([]*response.EngineResponse{resp}, kyvernov1.Fail, log)
 	assert.Assert(t, blocked == false)
 }

pkg/webhooks/utils/block.go

@@ -0,0 +1,65 @@
+package utils
+
+import (
+	"fmt"
+
+	"github.com/go-logr/logr"
+	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
+	"github.com/kyverno/kyverno/pkg/engine/response"
+	engineutils "github.com/kyverno/kyverno/pkg/utils/engine"
+	"gopkg.in/yaml.v2"
+)
+
+func getAction(hasViolations bool, i int) string {
+	action := "error"
+	if hasViolations {
+		action = "violation"
+	}
+	if i > 1 {
+		action = action + "s"
+	}
+	return action
+}
+
+// returns true -> if there is even one policy that blocks resource request
+// returns false -> if all the policies are meant to report only, we dont block resource request
+func BlockRequest(engineResponses []*response.EngineResponse, failurePolicy kyvernov1.FailurePolicyType, log logr.Logger) bool {
+	for _, er := range engineResponses {
+		if engineutils.BlockRequest(er, failurePolicy) {
+			log.V(2).Info("blocking admission request", "policy", er.PolicyResponse.Policy.Name)
+			return true
+		}
+	}
+	log.V(4).Info("allowing admission request")
+	return false
+}
+
+// GetBlockedMessages gets the error messages for rules with error or fail status
+func GetBlockedMessages(engineResponses []*response.EngineResponse) string {
+	if len(engineResponses) == 0 {
+		return ""
+	}
+	failures := make(map[string]interface{})
+	hasViolations := false
+	for _, er := range engineResponses {
+		ruleToReason := make(map[string]string)
+		for _, rule := range er.PolicyResponse.Rules {
+			if rule.Status != response.RuleStatusPass {
+				ruleToReason[rule.Name] = rule.Message
+				if rule.Status == response.RuleStatusFail {
+					hasViolations = true
+				}
+			}
+		}
+		failures[er.PolicyResponse.Policy.Name] = ruleToReason
+	}
+	if len(failures) == 0 {
+		return ""
+	}
+	r := engineResponses[0].PolicyResponse.Resource
+	resourceName := fmt.Sprintf("%s/%s/%s", r.Kind, r.Namespace, r.Name)
+	action := getAction(hasViolations, len(failures))
+	results, _ := yaml.Marshal(failures)
+	msg := fmt.Sprintf("\n\npolicy %s for resource %s: \n\n%s", resourceName, action, results)
+	return msg
+}

pkg/webhooks/utils/block_test.go

@@ -0,0 +1,292 @@
+package utils
+
+import (
+	"testing"
+
+	"github.com/go-logr/logr"
+	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
+	"github.com/kyverno/kyverno/pkg/engine/response"
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_getAction(t *testing.T) {
+	type args struct {
+		hasViolations bool
+		i             int
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{{
+		name: "violation",
+		args: args{true, 1},
+		want: "violation",
+	}, {
+		name: "violations",
+		args: args{true, 5},
+		want: "violations",
+	}, {
+		name: "error",
+		args: args{false, 1},
+		want: "error",
+	}, {
+		name: "errors",
+		args: args{false, 5},
+		want: "errors",
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := getAction(tt.args.hasViolations, tt.args.i)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestBlockRequest(t *testing.T) {
+	type args struct {
+		engineResponses []*response.EngineResponse
+		failurePolicy   kyvernov1.FailurePolicyType
+		log             logr.Logger
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{{
+		name: "failure - enforce",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						ValidationFailureAction: kyvernov1.Enforce,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-fail",
+								Status:  response.RuleStatusFail,
+								Message: "message fail",
+							},
+						},
+					},
+				},
+			},
+			failurePolicy: kyvernov1.Fail,
+			log:           logr.Discard(),
+		},
+		want: true,
+	}, {
+		name: "failure - audit",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						ValidationFailureAction: kyvernov1.Audit,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-fail",
+								Status:  response.RuleStatusFail,
+								Message: "message fail",
+							},
+						},
+					},
+				},
+			},
+			failurePolicy: kyvernov1.Fail,
+			log:           logr.Discard(),
+		},
+		want: false,
+	}, {
+		name: "error - fail",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						ValidationFailureAction: kyvernov1.Audit,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-error",
+								Status:  response.RuleStatusError,
+								Message: "message error",
+							},
+						},
+					},
+				},
+			},
+			failurePolicy: kyvernov1.Fail,
+			log:           logr.Discard(),
+		},
+		want: true,
+	}, {
+		name: "error - ignore",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						ValidationFailureAction: kyvernov1.Audit,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-error",
+								Status:  response.RuleStatusError,
+								Message: "message error",
+							},
+						},
+					},
+				},
+			},
+			failurePolicy: kyvernov1.Ignore,
+			log:           logr.Discard(),
+		},
+		want: false,
+	}, {
+		name: "warning - ignore",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						ValidationFailureAction: kyvernov1.Audit,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-warning",
+								Status:  response.RuleStatusWarn,
+								Message: "message warning",
+							},
+						},
+					},
+				},
+			},
+			failurePolicy: kyvernov1.Ignore,
+			log:           logr.Discard(),
+		},
+		want: false,
+	}, {
+		name: "warning - fail",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						ValidationFailureAction: kyvernov1.Audit,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-warning",
+								Status:  response.RuleStatusWarn,
+								Message: "message warning",
+							},
+						},
+					},
+				},
+			},
+			failurePolicy: kyvernov1.Fail,
+			log:           logr.Discard(),
+		},
+		want: false,
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := BlockRequest(tt.args.engineResponses, tt.args.failurePolicy, tt.args.log)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestGetBlockedMessages(t *testing.T) {
+	type args struct {
+		engineResponses []*response.EngineResponse
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{{
+		name: "failure - enforce",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						Policy: response.PolicySpec{
+							Name: "test",
+						},
+						ValidationFailureAction: kyvernov1.Enforce,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-fail",
+								Status:  response.RuleStatusFail,
+								Message: "message fail",
+							},
+						},
+						Resource: response.ResourceSpec{
+							Kind:      "foo",
+							Namespace: "bar",
+							Name:      "baz",
+						},
+					},
+				},
+			},
+		},
+		want: "\n\npolicy foo/bar/baz for resource violation: \n\ntest:\n  rule-fail: message fail\n",
+	}, {
+		name: "error - enforce",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						Policy: response.PolicySpec{
+							Name: "test",
+						},
+						ValidationFailureAction: kyvernov1.Enforce,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-error",
+								Status:  response.RuleStatusError,
+								Message: "message error",
+							},
+						},
+						Resource: response.ResourceSpec{
+							Kind:      "foo",
+							Namespace: "bar",
+							Name:      "baz",
+						},
+					},
+				},
+			},
+		},
+		want: "\n\npolicy foo/bar/baz for resource error: \n\ntest:\n  rule-error: message error\n",
+	}, {
+		name: "error and failure - enforce",
+		args: args{
+			engineResponses: []*response.EngineResponse{
+				{
+					PolicyResponse: response.PolicyResponse{
+						Policy: response.PolicySpec{
+							Name: "test",
+						},
+						ValidationFailureAction: kyvernov1.Enforce,
+						Rules: []response.RuleResponse{
+							{
+								Name:    "rule-fail",
+								Status:  response.RuleStatusFail,
+								Message: "message fail",
+							},
+							{
+								Name:    "rule-error",
+								Status:  response.RuleStatusError,
+								Message: "message error",
+							},
+						},
+						Resource: response.ResourceSpec{
+							Kind:      "foo",
+							Namespace: "bar",
+							Name:      "baz",
+						},
+					},
+				},
+			},
+		},
+		want: "\n\npolicy foo/bar/baz for resource violation: \n\ntest:\n  rule-error: message error\n  rule-fail: message fail\n",
+	}}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := GetBlockedMessages(tt.args.engineResponses)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}

pkg/webhooks/utils/warning.go

@@ -16,6 +16,5 @@ func GetWarningMessages(engineResponses []*response.EngineResponse) []string {
 			}
 		}
 	}
-
 	return warnings
 }