lazy evaluate vars in conditions (#7238)

* lazy evaluate vars in conditions

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* remove unnecessary conversion

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* fix test

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* Update test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/03-manifests.yaml

Signed-off-by: shuting <shutting06@gmail.com>

* Update test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/README.md

Signed-off-by: shuting <shutting06@gmail.com>

* added error check in test

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

---------

Signed-off-by: Jim Bugwadia <jim@nirmata.com>
Signed-off-by: shuting <shutting06@gmail.com>
Co-authored-by: shuting <shutting06@gmail.com>
Co-authored-by: kyverno-bot <104836976+kyverno-bot@users.noreply.github.com>

pkg/engine/background.go

@@ -94,27 +94,21 @@ func (e *engine) filterRule(
 		return nil
 	}
 
-	ruleCopy := rule.DeepCopy()
-	if after, err := variables.SubstituteAllInPreconditions(logger, ctx, ruleCopy.GetAnyAllConditions()); err != nil {
-		logger.V(4).Info("failed to substitute vars in preconditions, skip current rule", "rule name", ruleCopy.Name)
-		return nil
-	} else {
-		ruleCopy.SetAnyAllConditions(after)
-	}
-
 	// operate on the copy of the conditions, as we perform variable substitution
-	copyConditions, err := engineutils.TransformConditions(ruleCopy.GetAnyAllConditions())
+	copyConditions, err := engineutils.TransformConditions(rule.GetAnyAllConditions())
 	if err != nil {
 		logger.V(4).Info("cannot copy AnyAllConditions", "reason", err.Error())
-		return nil
+		return engineapi.RuleError(rule.Name, ruleType, "failed to convert AnyAllConditions", err)
 	}
 
 	// evaluate pre-conditions
-	if val, msg := variables.EvaluateConditions(logger, ctx, copyConditions); !val {
-		logger.V(4).Info("skip rule as preconditions are not met", "rule", ruleCopy.Name, "message", msg)
-		return engineapi.RuleSkip(ruleCopy.Name, ruleType, "")
+	if val, msg, err := variables.EvaluateConditions(logger, ctx, copyConditions); err != nil {
+		return engineapi.RuleError(rule.Name, ruleType, "failed to evaluate conditions", err)
+	} else if !val {
+		logger.V(4).Info("skip rule as preconditions are not met", "rule", rule.Name, "message", msg)
+		return engineapi.RuleSkip(rule.Name, ruleType, "")
 	}
 
 	// build rule Response
-	return engineapi.RulePass(ruleCopy.Name, ruleType, "")
+	return engineapi.RulePass(rule.Name, ruleType, "")
 }

pkg/engine/internal/imageverifier.go

@@ -174,8 +174,7 @@ func EvaluateConditions(
 	if err != nil {
 		return false, "", fmt.Errorf("failed to substitute variables in attestation conditions: %w", err)
 	}
-	pass, msg := variables.EvaluateAnyAllConditions(log, ctx, c)
-	return pass, msg, nil
+	return variables.EvaluateAnyAllConditions(log, ctx, c)
 }
 
 // verify applies policy rules to each matching image. The policy rule results and annotation patches are

pkg/engine/internal/preconditions.go

@@ -11,29 +11,19 @@ import (
 )
 
 func CheckPreconditions(logger logr.Logger, jsonContext enginecontext.Interface, anyAllConditions apiextensions.JSON) (bool, string, error) {
-	preconditions, err := variables.SubstituteAllInPreconditions(logger, jsonContext, anyAllConditions)
-	if err != nil {
-		return false, "", fmt.Errorf("failed to substitute variables in preconditions: %w", err)
-	}
-	typeConditions, err := utils.TransformConditions(preconditions)
+	typeConditions, err := utils.TransformConditions(anyAllConditions)
 	if err != nil {
 		return false, "", fmt.Errorf("failed to parse preconditions: %w", err)
 	}
 
-	val, msg := variables.EvaluateConditions(logger, jsonContext, typeConditions)
-	return val, msg, nil
+	return variables.EvaluateConditions(logger, jsonContext, typeConditions)
 }
 
 func CheckDenyPreconditions(logger logr.Logger, jsonContext enginecontext.Interface, anyAllConditions apiextensions.JSON) (bool, string, error) {
-	preconditions, err := variables.SubstituteAll(logger, jsonContext, anyAllConditions)
-	if err != nil {
-		return false, "", fmt.Errorf("failed to substitute variables in deny conditions: %w", err)
-	}
-	typeConditions, err := utils.TransformConditions(preconditions)
+	typeConditions, err := utils.TransformConditions(anyAllConditions)
 	if err != nil {
 		return false, "", fmt.Errorf("failed to parse deny conditions: %w", err)
 	}
 
-	val, msg := variables.EvaluateConditions(logger, jsonContext, typeConditions)
-	return val, msg, nil
+	return variables.EvaluateConditions(logger, jsonContext, typeConditions)
 }

pkg/engine/validation_test.go

@@ -1918,7 +1918,7 @@ func Test_Flux_Kustomization_PathNotPresent(t *testing.T) {
 			// referred variable path not present
 			resourceRaw:      []byte(`{"apiVersion":"kustomize.toolkit.fluxcd.io/v1beta1","kind":"Kustomization","metadata":{"name":"dev-team","namespace":"apps"},"spec":{"serviceAccountName":"dev-team","interval":"5m","sourceRef":{"kind":"GitRepository","name":"dev-team"},"prune":true,"validation":"client"}}`),
 			expectedResults:  []engineapi.RuleStatus{engineapi.RuleStatusPass, engineapi.RuleStatusError},
-			expectedMessages: []string{"validation rule 'serviceAccountName' passed.", "failed to check deny preconditions: failed to substitute variables in deny conditions: failed to resolve request.object.spec.sourceRef.namespace at path /0/key: JMESPath query failed: Unknown key \"namespace\" in path"},
+			expectedMessages: []string{"validation rule 'serviceAccountName' passed.", "failed to check deny preconditions: failed to substitute variables in condition key: failed to resolve request.object.spec.sourceRef.namespace at path : JMESPath query failed: Unknown key \"namespace\" in path"},
 		},
 		{
 			name:      "resource-with-violation",

pkg/engine/variables/evaluate.go

@@ -1,6 +1,8 @@
 package variables
 
 import (
+	"fmt"
+
 	"github.com/go-logr/logr"
 	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
 	"github.com/kyverno/kyverno/pkg/engine/context"
@@ -9,42 +11,50 @@ import (
 )
 
 // Evaluate evaluates the condition
-func Evaluate(log logr.Logger, ctx context.EvalInterface, condition kyvernov1.Condition) (bool, string) {
-	// get handler for the operator
-	handle := operator.CreateOperatorHandler(log, ctx, condition.Operator)
-	if handle == nil {
-		return false, condition.Message
+func Evaluate(logger logr.Logger, ctx context.EvalInterface, condition kyvernov1.Condition) (bool, string, error) {
+	key, err := SubstituteAllInPreconditions(logger, ctx, condition.GetKey())
+	if err != nil {
+		return false, "", fmt.Errorf("failed to substitute variables in condition key: %w", err)
 	}
-
-	return handle.Evaluate(condition.GetKey(), condition.GetValue()), condition.Message
+	value, err := SubstituteAllInPreconditions(logger, ctx, condition.GetValue())
+	if err != nil {
+		return false, "", fmt.Errorf("failed to substitute variables in condition value: %w", err)
+	}
+	handler := operator.CreateOperatorHandler(logger, ctx, condition.Operator)
+	if handler == nil {
+		return false, "", fmt.Errorf("failed to create handler for condition operator: %w", err)
+	}
+	return handler.Evaluate(key, value), condition.Message, nil
 }
 
 // EvaluateConditions evaluates all the conditions present in a slice, in a backwards compatible way
-func EvaluateConditions(log logr.Logger, ctx context.EvalInterface, conditions interface{}) (bool, string) {
+func EvaluateConditions(log logr.Logger, ctx context.EvalInterface, conditions interface{}) (bool, string, error) {
 	switch typedConditions := conditions.(type) {
 	case kyvernov1.AnyAllConditions:
 		return evaluateAnyAllConditions(log, ctx, typedConditions)
 	case []kyvernov1.Condition: // backwards compatibility
 		return evaluateOldConditions(log, ctx, typedConditions)
 	}
-	return false, "invalid condition"
+	return false, "", fmt.Errorf("invalid condition")
 }
 
-func EvaluateAnyAllConditions(log logr.Logger, ctx context.EvalInterface, conditions []kyvernov1.AnyAllConditions) (bool, string) {
+func EvaluateAnyAllConditions(log logr.Logger, ctx context.EvalInterface, conditions []kyvernov1.AnyAllConditions) (bool, string, error) {
 	var conditionTrueMessages []string
 	for _, c := range conditions {
-		if val, msg := evaluateAnyAllConditions(log, ctx, c); !val {
-			return false, msg
+		if val, msg, err := evaluateAnyAllConditions(log, ctx, c); err != nil {
+			return false, "", err
+		} else if !val {
+			return false, msg, nil
 		} else {
 			conditionTrueMessages = append(conditionTrueMessages, msg)
 		}
 	}
 
-	return true, stringutils.JoinNonEmpty(conditionTrueMessages, ";")
+	return true, stringutils.JoinNonEmpty(conditionTrueMessages, ";"), nil
 }
 
 // evaluateAnyAllConditions evaluates multiple conditions as a logical AND (all) or OR (any) operation depending on the conditions
-func evaluateAnyAllConditions(log logr.Logger, ctx context.EvalInterface, conditions kyvernov1.AnyAllConditions) (bool, string) {
+func evaluateAnyAllConditions(log logr.Logger, ctx context.EvalInterface, conditions kyvernov1.AnyAllConditions) (bool, string, error) {
 	anyConditions, allConditions := conditions.AnyConditions, conditions.AllConditions
 	anyConditionsResult, allConditionsResult := true, true
 	var conditionFalseMessages []string
@@ -54,7 +64,9 @@ func evaluateAnyAllConditions(log logr.Logger, ctx context.EvalInterface, condit
 	if anyConditions != nil {
 		anyConditionsResult = false
 		for _, condition := range anyConditions {
-			if val, msg := Evaluate(log, ctx, condition); val {
+			if val, msg, err := Evaluate(log, ctx, condition); err != nil {
+				return false, "", err
+			} else if val {
 				anyConditionsResult = true
 				conditionTrueMessages = append(conditionTrueMessages, msg)
 				break
@@ -70,7 +82,9 @@ func evaluateAnyAllConditions(log logr.Logger, ctx context.EvalInterface, condit
 
 	// update the allConditionsResult if they are present
 	for _, condition := range allConditions {
-		if val, msg := Evaluate(log, ctx, condition); !val {
+		if val, msg, err := Evaluate(log, ctx, condition); err != nil {
+			return false, "", err
+		} else if !val {
 			allConditionsResult = false
 			conditionFalseMessages = append(conditionFalseMessages, msg)
 			log.V(3).Info("a condition failed in 'all' block", "condition", condition, "message", msg)
@@ -82,22 +96,24 @@ func evaluateAnyAllConditions(log logr.Logger, ctx context.EvalInterface, condit
 
 	finalResult := anyConditionsResult && allConditionsResult
 	if finalResult {
-		return finalResult, stringutils.JoinNonEmpty(conditionTrueMessages, "; ")
+		return finalResult, stringutils.JoinNonEmpty(conditionTrueMessages, "; "), nil
 	}
 
-	return finalResult, stringutils.JoinNonEmpty(conditionFalseMessages, "; ")
+	return finalResult, stringutils.JoinNonEmpty(conditionFalseMessages, "; "), nil
 }
 
 // evaluateOldConditions evaluates multiple conditions when those conditions are provided in the old manner i.e. without 'any' or 'all'
-func evaluateOldConditions(log logr.Logger, ctx context.EvalInterface, conditions []kyvernov1.Condition) (bool, string) {
+func evaluateOldConditions(log logr.Logger, ctx context.EvalInterface, conditions []kyvernov1.Condition) (bool, string, error) {
 	var conditionTrueMessages []string
 	for _, condition := range conditions {
-		if val, msg := Evaluate(log, ctx, condition); !val {
-			return false, msg
+		if val, msg, err := Evaluate(log, ctx, condition); err != nil {
+			return false, "", err
+		} else if !val {
+			return false, msg, nil
 		} else {
 			conditionTrueMessages = append(conditionTrueMessages, msg)
 		}
 	}
 
-	return true, stringutils.JoinNonEmpty(conditionTrueMessages, ";")
+	return true, stringutils.JoinNonEmpty(conditionTrueMessages, ";"), nil
 }

pkg/engine/variables/evaluate_test.go

@@ -1,7 +1,6 @@
 package variables
 
 import (
-	"encoding/json"
 	"testing"
 
 	"github.com/go-logr/logr"
@@ -380,7 +379,7 @@ func TestEvaluate(t *testing.T) {
 
 	ctx := context.NewContext(jmespath.New(config.NewDefaultConfiguration(false)))
 	for _, tc := range testCases {
-		if val, _ := Evaluate(logr.Discard(), ctx, tc.Condition); val != tc.Result {
+		if val, _, _ := Evaluate(logr.Discard(), ctx, tc.Condition); val != tc.Result {
 			t.Errorf("%v - expected result to be %v", tc.Condition, tc.Result)
 		}
 	}
@@ -414,21 +413,7 @@ func Test_Eval_Equal_Var_Pass(t *testing.T) {
 		RawValue: kyverno.ToJSON("temp"),
 	}
 
-	conditionJSON, err := json.Marshal(condition)
-	assert.Nil(t, err)
-
-	var conditionMap interface{}
-	err = json.Unmarshal(conditionJSON, &conditionMap)
-	assert.Nil(t, err)
-
-	conditionWithResolvedVars, _ := SubstituteAllInPreconditions(logr.Discard(), ctx, conditionMap)
-	conditionJSON, err = json.Marshal(conditionWithResolvedVars)
-	assert.Nil(t, err)
-
-	err = json.Unmarshal(conditionJSON, &condition)
-	assert.Nil(t, err)
-
-	val, _ := Evaluate(logr.Discard(), ctx, condition)
+	val, _, _ := Evaluate(logr.Discard(), ctx, condition)
 	assert.True(t, val)
 }
 
@@ -458,9 +443,9 @@ func Test_Eval_Equal_Var_Fail(t *testing.T) {
 		RawValue: kyverno.ToJSON("temp1"),
 	}
 
-	if val, _ := Evaluate(logr.Discard(), ctx, condition); val {
-		t.Error("expected to fail")
-	}
+	val, _, err := Evaluate(logr.Discard(), ctx, condition)
+	assert.Nil(t, err)
+	assert.Equal(t, false, val, "expected to fail")
 }
 
 func Test_Condition_Messages(t *testing.T) {
@@ -489,7 +474,7 @@ func Test_Condition_Messages(t *testing.T) {
 				{
 					RawKey:   kyverno.ToJSON("{{request.object.metadata.name}}"),
 					Operator: kyverno.ConditionOperators["Equal"],
-					RawValue: kyverno.ToJSON("temp"),
+					RawValue: kyverno.ToJSON("temp2"),
 					Message:  "invalid name",
 				},
 				{
@@ -502,15 +487,15 @@ func Test_Condition_Messages(t *testing.T) {
 		},
 	}
 
-	val, msg := EvaluateAnyAllConditions(logr.Discard(), ctx, conditions)
+	val, msg, err := EvaluateAnyAllConditions(logr.Discard(), ctx, conditions)
+	assert.Nil(t, err)
 	assert.Equal(t, false, val)
 	assert.Contains(t, msg, "invalid name; invalid foo")
 
+	conditions[0].AnyConditions[0].RawValue = kyverno.ToJSON("temp")
 	conditions[0].AnyConditions[1].RawValue = kyverno.ToJSON("bar")
-	conditions, err = SubstituteAllInConditions(logr.Discard(), ctx, conditions)
+	val, msg, err = EvaluateAnyAllConditions(logr.Discard(), ctx, conditions)
 	assert.Nil(t, err)
-
-	val, msg = EvaluateAnyAllConditions(logr.Discard(), ctx, conditions)
 	assert.Equal(t, true, val)
 	assert.Equal(t, "invalid name", msg)
 
@@ -518,14 +503,16 @@ func Test_Condition_Messages(t *testing.T) {
 	conditions[0].AllConditions = append(conditions[0].AllConditions, conditions[0].AnyConditions[1])
 	conditions[0].AllConditions[1].RawValue = kyverno.ToJSON("bar2")
 
-	val, msg = EvaluateAnyAllConditions(logr.Discard(), ctx, conditions)
+	val, msg, err = EvaluateAnyAllConditions(logr.Discard(), ctx, conditions)
+	assert.Nil(t, err)
 	assert.Equal(t, false, val)
 	assert.Contains(t, msg, "invalid foo")
 
 	conditions[0].AnyConditions[0].RawValue = kyverno.ToJSON("temp1")
 	conditions[0].AnyConditions[1].RawValue = kyverno.ToJSON("bar2")
 	conditions[0].AllConditions[1].Message = "invalid foo2"
-	val, msg = EvaluateAnyAllConditions(logr.Discard(), ctx, conditions)
+	val, msg, err = EvaluateAnyAllConditions(logr.Discard(), ctx, conditions)
+	assert.Nil(t, err)
 	assert.Equal(t, false, val)
 	assert.Contains(t, msg, "invalid name; invalid foo; invalid foo2")
 }

test/cli/test/nil-values-in-variables/limit-duration/kyverno-test.yaml

@@ -8,7 +8,7 @@ results:
     rule: certificate-duration-max-100days 
     resource: letsencrypt-crt
     kind: Certificate
-    result: error
+    result: skip
   - policy: cert-manager-limit-duration
     rule: certificate-duration-max-100days 
     resource: acme-crt

test/cli/test/nil-values-in-variables/limit-duration/limit-duration.yaml

@@ -11,7 +11,7 @@ metadata:
     policies.kyverno.io/description: >-
       Kubernetes managed non-letsencrypt certificates have to be renewed in every 100 days.
 spec:
-  validationFailureAction: audit
+  validationFailureAction: Audit
   background: false
   rules:
   - name: certificate-duration-max-100days 
@@ -24,7 +24,7 @@ spec:
       - key: "{{ contains(request.object.spec.issuerRef.name, 'letsencrypt') }}"
         operator: Equals
         value: False
-      - key: "{{ request.object.spec.duration }}"
+      - key: '{{ request.object.spec.duration }}'
         operator: NotEquals
         value: ""
     validate:

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/README.md

@@ -0,0 +1,11 @@
+## Description
+
+This test verifies a variable definition is not evaluated until the condition is used
+
+## Expected Behavior
+
+The policy should not cause an error if the first condition (any) passes. The policy should cause an error if the first condition (all) fails.
+
+## Reference Issues
+
+https://github.com/kyverno/kyverno/issues/7211

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/01-assert.yaml

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

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/01-manifests.yaml

@@ -0,0 +1,32 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: preconditions
+spec:
+  validationFailureAction: Enforce
+  background: false
+  rules:
+    - name: test
+      match:
+        any:
+        - resources:
+            kinds:
+              - Pod
+      context:
+        - name: nothere
+          apiCall:
+            urlPath: /api/v1/namespaces/missing/configmaps/nothere
+      preconditions:
+        any:
+        - key: "{{ request.name }}"
+          operator: Equals
+          value: test
+          message: this pod is not allowed
+        - key: "{{ nothere }}"
+          operator: Equals
+          value: hello
+          message: value mismatch
+      validate:
+        pattern:
+          metadata:
+            name: "*"

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/02-assert.yaml

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

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/02-test.yaml

@@ -0,0 +1,7 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+apply:
+  - file: pod-good.yaml
+    shouldFail: false
+  - file: pod-bad.yaml
+    shouldFail: true

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/03-cleanup.yaml

@@ -0,0 +1,6 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+delete:
+- apiVersion: v1
+  kind: Pod
+  name: test

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/03-manifests.yaml

@@ -0,0 +1,30 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: preconditions
+spec:
+  validationFailureAction: Enforce
+  background: false
+  rules:
+    - name: test
+      match:
+        any:
+        - resources:
+            kinds:
+              - Pod
+      context:
+        - name: nothere
+          apiCall:
+            urlPath: /api/v1/namespaces/missing/configmaps/nothere
+      validate:
+        deny:
+           conditions:
+              all:
+              - key: "{{ request.name }}"
+                operator: Equals
+                value: test
+                message: this pod is not allowed
+              - key: "{{ nothere }}"
+                operator: Equals
+                value: hello
+                message: value mismatch

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/04-test.yaml

@@ -0,0 +1,7 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+apply:
+  - file: pod-good.yaml
+    shouldFail: true
+  - file: pod-bad.yaml
+    shouldFail: false

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/pod-bad.yaml

@@ -0,0 +1,8 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: other
+spec:
+  containers:
+  - image: ghcr.io/kyverno/test-verify-image:unsigned
+    name: test

test/conformance/kuttl/validate/clusterpolicy/standard/variables/lazyload/conditions/pod-good.yaml

@@ -0,0 +1,8 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: test
+spec:
+  containers:
+  - image: ghcr.io/kyverno/test-verify-image:unsigned
+    name: test