fix: properly verify precondition in old object validation (#11644) (#11705)

* fix: properly verify precondition in old object validation



* fix: tests



* fix: assert bug



* fix: properly update the values



---------

Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Co-authored-by: shuting <shuting@nirmata.com>

pkg/engine/api/policycontext.go

@@ -17,9 +17,9 @@ type PolicyContext interface {
 	NewResource() unstructured.Unstructured
 	OldResource() unstructured.Unstructured
 	SetResources(oldResource, newResource unstructured.Unstructured) error
+	SetOperation(kyvernov1.AdmissionOperation) error
 	AdmissionInfo() kyvernov2.RequestInfo
 	Operation() kyvernov1.AdmissionOperation
-	SetOperation(op kyvernov1.AdmissionOperation) error
 	NamespaceLabels() map[string]string
 	RequestResource() metav1.GroupVersionResource
 	ResourceKind() (schema.GroupVersionKind, string)

pkg/engine/handlers/validation/utils.go

@@ -1,14 +1,24 @@
 package validation
 
 import (
+	"github.com/go-logr/logr"
 	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
 	kyvernov2 "github.com/kyverno/kyverno/api/kyverno/v2"
+	enginecontext "github.com/kyverno/kyverno/pkg/engine/context"
+	"github.com/kyverno/kyverno/pkg/engine/internal"
 	engineutils "github.com/kyverno/kyverno/pkg/engine/utils"
 	authenticationv1 "k8s.io/api/authentication/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
-func matchResource(resource unstructured.Unstructured, rule kyvernov1.Rule, namespaceLabels map[string]string, policyNamespace string, operation kyvernov1.AdmissionOperation) bool {
+func matchResource(logger logr.Logger, resource unstructured.Unstructured, rule kyvernov1.Rule, namespaceLabels map[string]string, policyNamespace string, operation kyvernov1.AdmissionOperation, jsonContext enginecontext.Interface) bool {
+	if rule.RawAnyAllConditions != nil {
+		preconditionsPassed, _, err := internal.CheckPreconditions(logger, jsonContext, rule.RawAnyAllConditions)
+		if !preconditionsPassed || err != nil {
+			return false
+		}
+	}
+
 	// cannot use admission info from the current request as the user can be different, if the rule matches on old request user info, it should skip
 	admissionInfo := kyvernov2.RequestInfo{
 		Roles:        []string{"kyverno:invalidrole"},

pkg/engine/handlers/validation/validate_assert.go

@@ -16,6 +16,7 @@ import (
 	enginectx "github.com/kyverno/kyverno/pkg/engine/context"
 	"github.com/kyverno/kyverno/pkg/engine/handlers"
 	engineutils "github.com/kyverno/kyverno/pkg/engine/utils"
+	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/client-go/tools/cache"
@@ -123,7 +124,7 @@ func (h validateAssertHandler) Process(
 		if action.Enforce() {
 			allowExisitingViolations := rule.HasValidateAllowExistingViolations()
 			if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations {
-				errs, err := validateOldObject(ctx, policyContext, rule, payload, bindings)
+				errs, err := validateOldObject(ctx, logger, 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")
@@ -149,25 +150,37 @@ func (h validateAssertHandler) Process(
 	)
 }
 
-func validateOldObject(ctx context.Context, policyContext engineapi.PolicyContext, rule kyvernov1.Rule, payload map[string]any, bindings binding.Bindings) (field.ErrorList, error) {
+func validateOldObject(ctx context.Context, logger logr.Logger, policyContext engineapi.PolicyContext, rule kyvernov1.Rule, payload map[string]any, bindings binding.Bindings) (errs field.ErrorList, err error) {
 	if policyContext.Operation() != kyvernov1.Update {
 		return nil, nil
 	}
 
 	oldResource := policyContext.OldResource()
 
-	if ok := matchResource(oldResource, rule, policyContext.NamespaceLabels(), policyContext.Policy().GetNamespace(), kyvernov1.Create); !ok {
+	if err := policyContext.SetOperation(kyvernov1.Create); err != nil { // simulates the condition when old object was "created"
-		return nil, nil
+		return nil, errors.Wrapf(err, "failed to set operation")
 	}
 
 	payload["object"] = policyContext.OldResource().Object
 	payload["oldObject"] = nil
 	payload["operation"] = kyvernov1.Create
 
-	asserttion := assert.Parse(ctx, rule.Validation.Assert.Value)
+	defer func() {
-	errs, err := assert.Assert(ctx, nil, asserttion, payload, bindings)
+		if err = policyContext.SetOperation(kyvernov1.Update); err != nil {
-	if err != nil {
+			logger.Error(errors.Wrapf(err, "failed to reset operation"), "")
-		return nil, fmt.Errorf("failed to apply assertion: %w", err)
+		}
+
+		payload["object"] = policyContext.NewResource().Object
+		payload["oldObject"] = policyContext.OldResource().Object
+		payload["operation"] = kyvernov1.Update
+	}()
+
+	if ok := matchResource(logger, oldResource, rule, policyContext.NamespaceLabels(), policyContext.Policy().GetNamespace(), kyvernov1.Create, policyContext.JSONContext()); !ok {
+		return
 	}
-	return errs, nil
+
+	assertion := assert.Parse(ctx, rule.Validation.Assert.Value)
+	errs, err = assert.Assert(ctx, nil, assertion, payload, bindings)
+
+	return
 }

pkg/engine/handlers/validation/validate_pss.go

@@ -172,7 +172,7 @@ func (h validatePssHandler) validateOldObject(
 	rule kyvernov1.Rule,
 	engineLoader engineapi.EngineContextLoader,
 	exceptions []*kyvernov2.PolicyException,
-) (*engineapi.RuleResponse, error) {
+) (resp *engineapi.RuleResponse, err error) {
 	if policyContext.Operation() != kyvernov1.Update {
 		return nil, nil
 	}
@@ -181,27 +181,30 @@ func (h validatePssHandler) validateOldObject(
 	oldResource := policyContext.OldResource()
 	emptyResource := unstructured.Unstructured{}
 
-	if ok := matchResource(oldResource, rule, policyContext.NamespaceLabels(), policyContext.Policy().GetNamespace(), kyvernov1.Create); !ok {
+	if err = policyContext.SetResources(emptyResource, oldResource); err != nil {
-		return nil, nil
-	}
-	if err := policyContext.SetResources(emptyResource, oldResource); err != nil {
 		return nil, errors.Wrapf(err, "failed to set resources")
 	}
-	if err := policyContext.SetOperation(kyvernov1.Create); err != nil { // simulates the condition when old object was "created"
+	if err = policyContext.SetOperation(kyvernov1.Create); err != nil { // simulates the condition when old object was "created"
 		return nil, errors.Wrapf(err, "failed to set operation")
 	}
 
-	_, resp := h.validate(ctx, logger, policyContext, oldResource, rule, engineLoader, exceptions)
+	defer func() {
+		if err = policyContext.SetResources(oldResource, newResource); err != nil {
+			logger.Error(errors.Wrapf(err, "failed to reset resources"), "")
+		}
 
-	if err := policyContext.SetResources(oldResource, newResource); err != nil {
+		if err = policyContext.SetOperation(kyvernov1.Update); err != nil {
-		return nil, errors.Wrapf(err, "failed to reset resources")
+			logger.Error(errors.Wrapf(err, "failed to reset operations"), "")
+		}
+	}()
+
+	if ok := matchResource(logger, oldResource, rule, policyContext.NamespaceLabels(), policyContext.Policy().GetNamespace(), kyvernov1.Create, policyContext.JSONContext()); !ok {
+		return
 	}
 
-	if err := policyContext.SetOperation(kyvernov1.Update); err != nil {
+	_, resp = h.validate(ctx, logger, policyContext, oldResource, rule, engineLoader, exceptions)
-		return nil, errors.Wrapf(err, "failed to reset operation")
-	}
 
-	return resp, nil
+	return
 }
 
 func convertChecks(checks []pssutils.PSSCheckResult, kind string) (newChecks []pssutils.PSSCheckResult) {

pkg/engine/handlers/validation/validate_resource.go

@@ -175,7 +175,7 @@ func (v *validator) validate(ctx context.Context) *engineapi.RuleResponse {
 	return ruleResponse
 }
 
-func (v *validator) validateOldObject(ctx context.Context) (*engineapi.RuleResponse, error) {
+func (v *validator) validateOldObject(ctx context.Context) (resp *engineapi.RuleResponse, err error) {
 	if v.policyContext.Operation() != kyvernov1.Update {
 		return nil, errors.New("invalid operation")
 	}
@@ -184,28 +184,32 @@ func (v *validator) validateOldObject(ctx context.Context) (*engineapi.RuleRespo
 	oldResource := v.policyContext.OldResource()
 	emptyResource := unstructured.Unstructured{}
 
-	if ok := matchResource(oldResource, v.rule, v.policyContext.NamespaceLabels(), v.policyContext.Policy().GetNamespace(), kyvernov1.Create); !ok {
+	if err = v.policyContext.SetResources(emptyResource, oldResource); err != nil {
-		return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "resource not matched", v.rule.ReportProperties), nil
-	}
-
-	if err := v.policyContext.SetResources(emptyResource, oldResource); err != nil {
 		return nil, errors.Wrapf(err, "failed to set resources")
 	}
-	if err := v.policyContext.SetOperation(kyvernov1.Create); err != nil { // simulates the condition when old object was "created"
+
+	if err = v.policyContext.SetOperation(kyvernov1.Create); err != nil { // simulates the condition when old object was "created"
 		return nil, errors.Wrapf(err, "failed to set operation")
 	}
 
-	resp := v.validate(ctx)
+	defer func() {
+		if err = v.policyContext.SetResources(oldResource, newResource); err != nil {
+			v.log.Error(errors.Wrapf(err, "failed to reset resources"), "")
+		}
 
-	if err := v.policyContext.SetResources(oldResource, newResource); err != nil {
+		if err = v.policyContext.SetOperation(kyvernov1.Update); err != nil {
-		return nil, errors.Wrapf(err, "failed to reset resources")
+			v.log.Error(errors.Wrapf(err, "failed to reset operation"), "")
+		}
+	}()
+
+	if ok := matchResource(v.log, oldResource, v.rule, v.policyContext.NamespaceLabels(), v.policyContext.Policy().GetNamespace(), kyvernov1.Create, v.policyContext.JSONContext()); !ok {
+		resp = engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "resource not matched", nil)
+		return
 	}
 
-	if err := v.policyContext.SetOperation(kyvernov1.Update); err != nil {
+	resp = v.validate(ctx)
-		return nil, errors.Wrapf(err, "failed to reset operation")
-	}
 
-	return resp, nil
+	return
 }
 
 func (v *validator) validateForEach(ctx context.Context) *engineapi.RuleResponse {

pkg/engine/handlers/validation/validate_resource_test.go

@@ -72,7 +72,7 @@ var (
 						"UPDATE"
 					  ],
 					  "kinds": [
-						"Namespace"
+						"Pod"
 					  ]
 					}
 				  }
@@ -100,7 +100,7 @@ var (
 						"UPDATE"
 					  ],
 					  "kinds": [
-						"Namespace"
+						"Pod"
 					  ]
 					}
 				  }
@@ -151,7 +151,7 @@ var (
         "name": "check-image",
         "validate": {
     	    "failureAction": "Enforce",
-		    "allowExistingViolations": true,
+		      "allowExistingViolations": true,
             "foreach": [
             {
               "deny": {

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/README.md

@@ -0,0 +1,9 @@
+## Description
+
+This test verifies that preconditions are respected when validating old object
+
+## Expected Behavior
+
+1. A policy is created that matches only update operations
+2. An ingress is created
+3. An update is sent to the ingress, since the policy did not match create operation in precondition the validation should not skip in this case because of skip existing violation behaviour.

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/chainsaw-test.yaml

@@ -0,0 +1,33 @@
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: Test
+metadata:
+  name: enforce-validate-existing
+spec:
+  steps:
+  - name: create policy
+    use:
+      template: ../../../../../_step-templates/create-policy.yaml
+      with:
+        bindings:
+        - name: file
+          value: policy.yaml
+  - name: wait policy ready
+    use:
+      template: ../../../../../_step-templates/cluster-policy-ready.yaml
+      with:
+        bindings:
+        - name: name
+          value: prevent-ingress-annotation
+  - name: step-02
+    try:
+    - apply:
+        file: ingress.yaml
+    - assert:
+        file: ingress-ready.yaml
+  - name: step-03
+    try:
+    - apply:
+        expect:
+        - check:
+            ($error != null): true
+        file: ingress-update.yaml

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress-ready.yaml

@@ -0,0 +1,4 @@
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  name: minimal-ingress

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress-update.yaml

@@ -0,0 +1,18 @@
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  name: minimal-ingress
+  annotations:
+    alb.ingress.kubernetes.io/scheme: test-update
+spec:
+  ingressClassName: nginx-example
+  rules:
+  - http:
+      paths:
+      - path: /testpath
+        pathType: Prefix
+        backend:
+          service:
+            name: test
+            port:
+              number: 80

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress.yaml

@@ -0,0 +1,18 @@
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  name: minimal-ingress
+  annotations:
+    alb.ingress.kubernetes.io/scheme: test
+spec:
+  ingressClassName: nginx-example
+  rules:
+  - http:
+      paths:
+      - path: /testpath
+        pathType: Prefix
+        backend:
+          service:
+            name: test
+            port:
+              number: 80

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/policy.yaml

@@ -0,0 +1,31 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: prevent-ingress-annotation
+spec:
+  validationFailureAction: Enforce
+  rules:
+  - name: prevent-ingress-annotation-subnet
+    match:
+      any:
+      - resources:
+          kinds:
+          - Ingress
+    preconditions:
+      any:
+      - key: "{{request.operation || 'BACKGROUND'}}"
+        operator: Equals
+        value: UPDATE
+    validate:
+      message: |
+        alb.ingress.kubernetes.io/scheme cannot be updated
+      pattern:
+        deny:
+          conditions:
+            any:
+            - key: "{{ request.object.metadata.annotations.\"alb.ingress.kubernetes.io/scheme\" || '' }}"
+              operator: Equals
+              value: ""
+            - key: "{{ request.object.metadata.annotations.\"alb.ingress.kubernetes.io/scheme\" }}"
+              operator: NotEquals
+              value: "{{ request.oldObject.metadata.annotations.\"alb.ingress.kubernetes.io/scheme\" }}"