fix: skip processing the oldObject for audit policies (#10233) (#11204)

* fix: skip processing the oldObject for audit policies



* fix: modify error and skip messages



* fix: modify the log level



---------

Signed-off-by: Mariam Fahmy <mariam.fahmy@nirmata.com>
Co-authored-by: Mariam Fahmy <mariam.fahmy@nirmata.com>

pkg/engine/handlers/validation/validate_assert.go

@@ -112,20 +112,31 @@ func (h validateAssertHandler) Process(
 	}
 	// compose a response
 	if len(errs) != 0 {
-		allowExisitingViolations := rule.HasValidateAllowExistingViolations()
+		var action kyvernov1.ValidationFailureAction
-		if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations {
+		if rule.Validation.FailureAction != nil {
-			errs, err := validateOldObject(ctx, policyContext, rule, payload, bindings)
+			action = *rule.Validation.FailureAction
-			if err != nil {
+		} else {
-				logger.V(2).Info("warning: failed to validate old object, skipping the rule evaluation as pre-existing violations are allowed", "rule", rule.Name, "error", err.Error())
+			action = policyContext.Policy().GetSpec().ValidationFailureAction
-				return resource, handlers.WithSkip(rule, engineapi.Validation, "failed to validate old object, skipping as preexisting violations are allowed")
+		}
-			}
 
-			logger.V(3).Info("old object verification", "errors", errs)
+		// process the old object for UPDATE admission requests in case of enforce policies
-			if len(errs) != 0 {
+		if action == kyvernov1.Enforce {
-				logger.V(3).Info("skipping modified resource as validation results have not changed")
+			allowExisitingViolations := rule.HasValidateAllowExistingViolations()
-				return resource, handlers.WithSkip(rule, engineapi.Validation, "skipping modified resource as validation results have not changed")
+			if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations {
+				errs, err := validateOldObject(ctx, policyContext, rule, payload, bindings)
+				if err != nil {
+					logger.V(4).Info("warning: failed to validate old object", "rule", rule.Name, "error", err.Error())
+					return resource, handlers.WithSkip(rule, engineapi.Validation, "failed to validate old object")
+				}
+
+				logger.V(3).Info("old object verification", "errors", errs)
+				if len(errs) != 0 {
+					logger.V(2).Info("warning: skipping the rule evaluation as pre-existing violations are allowed", "rule", rule.Name)
+					return resource, handlers.WithSkip(rule, engineapi.Validation, "skipping the rule evaluation as pre-existing violations are allowed")
+				}
 			}
 		}
+
 		var responses []*engineapi.RuleResponse
 		for _, err := range errs {
 			responses = append(responses, engineapi.RuleFail(rule.Name, engineapi.Validation, err.Error(), rule.ReportProperties))

pkg/engine/handlers/validation/validate_pss.go

@@ -133,23 +133,31 @@ func (h validatePssHandler) validate(
 		}
 		msg := fmt.Sprintf(`Validation rule '%s' failed. It violates PodSecurity "%s:%s": %s`, rule.Name, podSecurity.Level, podSecurity.Version, pss.FormatChecksPrint(pssChecks))
 		ruleResponse := engineapi.RuleFail(rule.Name, engineapi.Validation, msg, rule.ReportProperties).WithPodSecurityChecks(podSecurityChecks)
-		allowExisitingViolations := rule.HasValidateAllowExistingViolations()
+		var action kyvernov1.ValidationFailureAction
-		if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations {
+		if rule.Validation.FailureAction != nil {
-			logger.V(4).Info("is update request")
+			action = *rule.Validation.FailureAction
-			priorResp, err := h.validateOldObject(ctx, logger, policyContext, resource, rule, engineLoader, exceptions)
+		} else {
-			if err != nil {
+			action = policyContext.Policy().GetSpec().ValidationFailureAction
-				logger.V(2).Info("warning: failed to validate old object, skipping the rule evaluation as pre-existing violations are allowed", "rule", rule.Name, "error", err.Error())
+		}
-				return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "failed to validate old object, skipping as preexisting violations are allowed", rule.ReportProperties)
-			}
 
-			if ruleResponse.Status() == priorResp.Status() {
+		// process the old object for UPDATE admission requests in case of enforce policies
-				logger.V(3).Info("skipping modified resource as validation results have not changed", "oldResp", priorResp, "newResp", ruleResponse)
+		if action == kyvernov1.Enforce {
-				if ruleResponse.Status() == engineapi.RuleStatusPass {
+			allowExisitingViolations := rule.HasValidateAllowExistingViolations()
-					return resource, ruleResponse
+			if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations {
+				priorResp, err := h.validateOldObject(ctx, logger, policyContext, resource, rule, engineLoader, exceptions)
+				if err != nil {
+					logger.V(4).Info("warning: failed to validate old object", "rule", rule.Name, "error", err.Error())
+					return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "failed to validate old object", rule.ReportProperties)
+				}
+
+				if ruleResponse.Status() == priorResp.Status() {
+					logger.V(2).Info("warning: skipping the rule evaluation as pre-existing violations are allowed", "oldResp", priorResp, "newResp", ruleResponse)
+					if ruleResponse.Status() == engineapi.RuleStatusPass {
+						return resource, ruleResponse
+					}
+					return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "skipping the rule evaluation as pre-existing violations are allowed", rule.ReportProperties)
 				}
-				return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "skipping modified resource as validation results have not changed", rule.ReportProperties)
 			}
-			logger.V(4).Info("old object response is different", "oldResp", priorResp, "newResp", ruleResponse)
 		}
 
 		return resource, ruleResponse

pkg/engine/handlers/validation/validate_resource.go

@@ -146,20 +146,30 @@ func (v *validator) validate(ctx context.Context) *engineapi.RuleResponse {
 		v.log.V(2).Info("invalid validation rule: podSecurity, cel, patterns, or deny expected")
 	}
 
-	allowExisitingViolations := v.rule.HasValidateAllowExistingViolations()
+	var action kyvernov1.ValidationFailureAction
-	if engineutils.IsUpdateRequest(v.policyContext) && allowExisitingViolations && v.nesting == 0 { // is update request and is the root level validate
+	if v.rule.Validation.FailureAction != nil {
-		priorResp, err := v.validateOldObject(ctx)
+		action = *v.rule.Validation.FailureAction
-		if err != nil {
+	} else {
-			v.log.V(2).Info("warning: failed to validate old object, skipping the rule evaluation as pre-existing violations are allowed", "rule", v.rule.Name, "error", err.Error())
+		action = v.policyContext.Policy().GetSpec().ValidationFailureAction
-			return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "failed to validate old object, skipping as preexisting violations are allowed", ruleResponse.Properties())
+	}
-		}
 
-		if engineutils.IsSameRuleResponse(ruleResponse, priorResp) {
+	// process the old object for UPDATE admission requests in case of enforce policies
-			v.log.V(3).Info("skipping modified resource as validation results have not changed")
+	if action == kyvernov1.Enforce {
-			if ruleResponse.Status() == engineapi.RuleStatusPass {
+		allowExisitingViolations := v.rule.HasValidateAllowExistingViolations()
-				return ruleResponse
+		if engineutils.IsUpdateRequest(v.policyContext) && allowExisitingViolations && v.nesting == 0 { // is update request and is the root level validate
+			priorResp, err := v.validateOldObject(ctx)
+			if err != nil {
+				v.log.V(4).Info("warning: failed to validate old object", "rule", v.rule.Name, "error", err.Error())
+				return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "failed to validate old object", ruleResponse.Properties())
+			}
+
+			if engineutils.IsSameRuleResponse(ruleResponse, priorResp) {
+				v.log.V(2).Info("warning: skipping the rule evaluation as pre-existing violations are allowed")
+				if ruleResponse.Status() == engineapi.RuleStatusPass {
+					return ruleResponse
+				}
+				return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "skipping the rule evaluation as pre-existing violations are allowed", v.rule.ReportProperties)
 			}
-			return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "skipping modified resource as validation results have not changed", v.rule.ReportProperties)
 		}
 	}
 

pkg/engine/handlers/validation/validate_resource_test.go

@@ -150,9 +150,9 @@ var (
         },
         "name": "check-image",
         "validate": {
-    			"failureAction": "Audit",
+    	    "failureAction": "Enforce",
-					"allowExistingViolations": true,
+		    "allowExistingViolations": true,
-          "foreach": [
+            "foreach": [
             {
               "deny": {
                 "conditions": {

test/conformance/chainsaw/reports/admission/update-deployment/README.md

@@ -0,0 +1,15 @@
+## Description
+
+This test verifies that policy report doesn't change when a resource is updated and the engine response is the same as before.
+
+A policy in Audit mode is created.
+A deployment is created, the deployment violates the policy and we assert the policy report contains a `warn` result.
+The deployment is then updated but it still violates the policy.
+
+## Expected result
+
+When the resource is updated and it still violates the policy, the policy report should not change.
+
+## Related issue(s)
+
+- https://github.com/kyverno/kyverno/issues/10169

test/conformance/chainsaw/reports/admission/update-deployment/chainsaw-test.yaml

@@ -0,0 +1,41 @@
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: Test
+metadata:
+  creationTimestamp: null
+  name: update-deployment
+spec:
+  steps:
+  - name: step-01
+    try:
+    - apply:
+        file: policy.yaml
+    - assert:
+        file: policy-assert.yaml
+  - name: step-02
+    try:
+    - apply:
+        file: deployment.yaml
+    - assert:
+        file: deployment.yaml
+  - name: step-03
+    try:
+    - sleep:
+        duration: 5s
+  - name: step-04
+    try:
+    - assert:
+        file: report-assert.yaml
+  - name: step-05
+    try:
+    - apply:
+        file: update-deployment.yaml
+    - assert:
+        file: update-deployment.yaml
+  - name: step-06
+    try:
+    - sleep:
+        duration: 5s
+  - name: step-07
+    try:
+    - assert:
+        file: report-assert.yaml

test/conformance/chainsaw/reports/admission/update-deployment/deployment.yaml

@@ -0,0 +1,19 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    app: nginx
+  name: nginx-test
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: nginx
+  template:
+    metadata:
+      labels:
+        app: nginx
+    spec:
+      containers:
+        - name: nginx
+          image: nginx:latest

test/conformance/chainsaw/reports/admission/update-deployment/policy-assert.yaml

@@ -0,0 +1,9 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: require-multiple-replicas
+status:
+  conditions:
+  - reason: Succeeded
+    status: "True"
+    type: Ready

test/conformance/chainsaw/reports/admission/update-deployment/policy.yaml

@@ -0,0 +1,29 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: require-multiple-replicas
+  annotations:
+    policies.kyverno.io/category: Best Practises
+    policies.kyverno.io/minversion: 1.9.2
+    policies.kyverno.io/severity: low
+    policies.kyverno.io/subject: Deployment,StatefulSet
+    policies.kyverno.io/title: Require Multiple Replicas
+    policies.kyverno.io/scored: "false"
+spec:
+  background: false
+  rules:
+    - name: require-multiple-replicas
+      match:
+        any:
+          - resources:
+              kinds:
+                - Deployment
+                - StatefulSet
+              operations:
+                - CREATE
+                - UPDATE
+      validate:
+        pattern:
+          spec:
+            replicas: ">1"
+  validationFailureAction: Audit

test/conformance/chainsaw/reports/admission/update-deployment/report-assert.yaml

@@ -0,0 +1,23 @@
+apiVersion: wgpolicyk8s.io/v1alpha2
+kind: PolicyReport
+metadata:
+  ownerReferences:
+  - apiVersion: apps/v1
+    kind: Deployment
+    name: nginx-test
+results:
+- message: 'validation error: rule require-multiple-replicas failed at path /spec/replicas/'
+  policy: require-multiple-replicas
+  result: warn
+  rule: require-multiple-replicas
+  source: kyverno
+scope:
+  apiVersion: apps/v1
+  kind: Deployment
+  name: nginx-test
+summary:
+  error: 0
+  fail: 0
+  pass: 0
+  skip: 0
+  warn: 1

test/conformance/chainsaw/reports/admission/update-deployment/update-deployment.yaml

@@ -0,0 +1,19 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    app: nginx
+  name: nginx-test
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: nginx
+  template:
+    metadata:
+      labels:
+        app: nginx
+    spec:
+      containers:
+        - name: nginx-1
+          image: nginx:latest