fix: return skip when celPreconditions/matchConditions aren't met (#9940)

* fix: return skip when cel preconditions aren't met

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

* fix test

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

* fix: return skip when matchConditions in VAPs aren't met

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

---------

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

pkg/engine/handlers/validation/validate_cel.go

@@ -12,6 +12,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/engine/internal"
 	engineutils "github.com/kyverno/kyverno/pkg/engine/utils"
 	celutils "github.com/kyverno/kyverno/pkg/utils/cel"
+	datautils "github.com/kyverno/kyverno/pkg/utils/data"
 	vaputils "github.com/kyverno/kyverno/pkg/validatingadmissionpolicy"
 	admissionregistrationv1alpha1 "k8s.io/api/admissionregistration/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
@@ -173,6 +174,13 @@ func (h validateCELHandler) Process(
 	}
 
 	for _, validationResult := range validationResults {
+		// no validations are returned if preconditions aren't met
+		if datautils.DeepEqual(validationResult, validatingadmissionpolicy.ValidateResult{}) {
+			return resource, handlers.WithResponses(
+				engineapi.RuleSkip(rule.Name, engineapi.Validation, "cel preconditions not met"),
+			)
+		}
+
 		for _, decision := range validationResult.Decisions {
 			switch decision.Action {
 			case validatingadmissionpolicy.ActionAdmit:

pkg/validatingadmissionpolicy/validate.go

@@ -9,6 +9,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/clients/dclient"
 	engineapi "github.com/kyverno/kyverno/pkg/engine/api"
 	celutils "github.com/kyverno/kyverno/pkg/utils/cel"
+	datautils "github.com/kyverno/kyverno/pkg/utils/data"
 	kubeutils "github.com/kyverno/kyverno/pkg/utils/kube"
 	"golang.org/x/text/cases"
 	"golang.org/x/text/language"
@@ -224,21 +225,26 @@ func validateResource(
 	versionedAttr, _ := admission.NewVersionedAttributes(a, a.GetKind(), nil)
 	validateResult := validator.Validate(context.TODO(), a.GetResource(), versionedAttr, nil, &namespace, celconfig.RuntimeCELCostBudget, nil)
 
-	isPass := true
+	// no validations are returned if match conditions aren't met
-	for _, policyDecision := range validateResult.Decisions {
+	if datautils.DeepEqual(validateResult, validatingadmissionpolicy.ValidateResult{}) {
-		if policyDecision.Evaluation == validatingadmissionpolicy.EvalError {
+		ruleResp = engineapi.RuleSkip(policy.GetName(), engineapi.Validation, "match conditions aren't met")
-			isPass = false
+	} else {
-			ruleResp = engineapi.RuleError(policy.GetName(), engineapi.Validation, policyDecision.Message, nil)
+		isPass := true
-			break
+		for _, policyDecision := range validateResult.Decisions {
-		} else if policyDecision.Action == validatingadmissionpolicy.ActionDeny {
+			if policyDecision.Evaluation == validatingadmissionpolicy.EvalError {
-			isPass = false
+				isPass = false
-			ruleResp = engineapi.RuleFail(policy.GetName(), engineapi.Validation, policyDecision.Message)
+				ruleResp = engineapi.RuleError(policy.GetName(), engineapi.Validation, policyDecision.Message, nil)
-			break
+				break
+			} else if policyDecision.Action == validatingadmissionpolicy.ActionDeny {
+				isPass = false
+				ruleResp = engineapi.RuleFail(policy.GetName(), engineapi.Validation, policyDecision.Message)
+				break
+			}
 		}
-	}
 
-	if isPass {
+		if isPass {
-		ruleResp = engineapi.RulePass(policy.GetName(), engineapi.Validation, "")
+			ruleResp = engineapi.RulePass(policy.GetName(), engineapi.Validation, "")
+		}
 	}
 
 	if binding != nil {

test/cli/test-validating-admission-policy/with-match-conditions/disallow-host-path.yaml

@@ -0,0 +1,17 @@
+apiVersion: admissionregistration.k8s.io/v1alpha1
+kind: ValidatingAdmissionPolicy
+metadata:
+  name: "disallow-host-path"
+spec:
+  matchConstraints:
+    resourceRules:
+    - apiGroups:   [""]
+      apiVersions: ["v1"]
+      operations:  ["CREATE", "UPDATE"]
+      resources:   ["pods"]
+  matchConditions:
+    - expression: "object.metadata.labels['color'] == 'red'"
+      name: "red-label"
+  validations:
+    - expression: "!has(object.spec.volumes) || object.spec.volumes.all(volume, !has(volume.hostPath))"
+      message: "HostPath volumes are forbidden. The field spec.volumes[*].hostPath must be unset."

test/cli/test-validating-admission-policy/with-match-conditions/kyverno-test.yaml

@@ -0,0 +1,27 @@
+apiVersion: cli.kyverno.io/v1alpha1
+kind: Test
+metadata:
+  name: kyverno-test.yaml
+policies:
+- disallow-host-path.yaml
+resources:
+- resources.yaml
+results:
+- isValidatingAdmissionPolicy: true
+  kind: Pod
+  policy: disallow-host-path
+  resources:
+  - bad-pod
+  result: fail
+- isValidatingAdmissionPolicy: true
+  kind: Pod
+  policy: disallow-host-path
+  resources:
+  - good-pod
+  result: pass
+- isValidatingAdmissionPolicy: true
+  kind: Pod
+  policy: disallow-host-path
+  resources:
+  - skipped-pod
+  result: skip

test/cli/test-validating-admission-policy/with-match-conditions/resources.yaml

@@ -0,0 +1,52 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: skipped-pod
+  labels:
+    color: blue
+spec:
+  containers:
+  - name: nginx-container
+    image: nginx:latest
+    volumeMounts:
+    - name: hostpath-volume
+      mountPath: /var/www/html
+  volumes:
+  - name: hostpath-volume
+    hostPath:
+      path: /var/log
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: bad-pod
+  labels:
+    color: red
+spec:
+  containers:
+  - name: nginx-container
+    image: nginx:latest
+    volumeMounts:
+    - name: hostpath-volume
+      mountPath: /var/www/html
+  volumes:
+  - name: hostpath-volume
+    hostPath:
+      path: /var/log
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: good-pod
+  labels:
+    color: red
+spec:
+  containers:
+  - name: nginx-container
+    image: nginx:latest
+    volumeMounts:
+    - name: hostpath-volume
+      mountPath: /var/www/html
+  volumes:
+  - name: hostpath-volume
+    emptyDir: {}

test/cli/test/cel-preconditions/disallow-host-path.yaml

@@ -0,0 +1,22 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: disallow-host-path
+spec:
+  validationFailureAction: Audit
+  background: false
+  rules:
+    - name: host-path
+      match:
+        any:
+        - resources:
+            kinds:
+              - Pod
+      celPreconditions:
+        - expression: "object.metadata.labels['color'] == 'red'"
+          name: "Label should be red"
+      validate:
+        cel:
+          expressions:
+            - expression: "!has(object.spec.volumes) || object.spec.volumes.all(volume, !has(volume.hostPath))"
+              message: "HostPath volumes are forbidden. The field spec.volumes[*].hostPath must be unset."

test/cli/test/cel-preconditions/kyverno-test.yaml

@@ -0,0 +1,27 @@
+apiVersion: cli.kyverno.io/v1alpha1
+kind: Test
+metadata:
+  name: kyverno-test.yaml
+policies:
+- disallow-host-path.yaml
+resources:
+- resources.yaml
+results:
+- kind: Pod
+  policy: disallow-host-path
+  resources:
+  - bad-pod
+  result: fail
+  rule: host-path
+- kind: Pod
+  policy: disallow-host-path
+  resources:
+  - good-pod
+  result: pass
+  rule: host-path
+- kind: Pod
+  policy: disallow-host-path
+  resources:
+  - skipped-pod
+  result: skip
+  rule: host-path

test/cli/test/cel-preconditions/resources.yaml

@@ -0,0 +1,52 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: skipped-pod
+  labels:
+    color: blue
+spec:
+  containers:
+  - name: nginx-container
+    image: nginx:latest
+    volumeMounts:
+    - name: hostpath-volume
+      mountPath: /var/www/html
+  volumes:
+  - name: hostpath-volume
+    hostPath:
+      path: /var/log
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: bad-pod
+  labels:
+    color: red
+spec:
+  containers:
+  - name: nginx-container
+    image: nginx:latest
+    volumeMounts:
+    - name: hostpath-volume
+      mountPath: /var/www/html
+  volumes:
+  - name: hostpath-volume
+    hostPath:
+      path: /var/log
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: good-pod
+  labels:
+    color: red
+spec:
+  containers:
+  - name: nginx-container
+    image: nginx:latest
+    volumeMounts:
+    - name: hostpath-volume
+      mountPath: /var/www/html
+  volumes:
+  - name: hostpath-volume
+    emptyDir: {}