fix: allow changes to preexisting resource in violation of a policy in Enforce (#9027) (#9139)

* fix: allow changes to preexisting resource in violation of a policy in Enforce



* fix: missing error check



* fix: tests



* nit: cleanup



* fix



* fix: update old policy context



* fix: preconditions always retured true

internal.CheckPreconditions always returned true when v.anyAllConditions, it should be populated with rule.RawAnyAllConditions when newValidator() is used to create a validator



* fix: fix chainsaw test



* fix: nit



* debug



* feat: update test



* fix: add namespace



* feat: add test for bad to good conversion



* feat: add test step



---------

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

@@ -25,6 +25,7 @@ type PolicyContext interface {
 	Element() unstructured.Unstructured
 	SetElement(element unstructured.Unstructured)
 
+	OldPolicyContext() (PolicyContext, error)
 	JSONContext() enginecontext.Interface
 	Copy() PolicyContext
 }

pkg/engine/handlers/validation/validate_resource.go

@@ -18,6 +18,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/utils/api"
 	datautils "github.com/kyverno/kyverno/pkg/utils/data"
 	stringutils "github.com/kyverno/kyverno/pkg/utils/strings"
+	"github.com/pkg/errors"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
@@ -55,6 +56,7 @@ type validator struct {
 }
 
 func newValidator(log logr.Logger, contextLoader engineapi.EngineContextLoader, ctx engineapi.PolicyContext, rule kyvernov1.Rule) *validator {
+	anyAllConditions, _ := datautils.ToMap(rule.RawAnyAllConditions)
 	return &validator{
 		log:              log,
 		rule:             rule,
@@ -63,6 +65,7 @@ func newValidator(log logr.Logger, contextLoader engineapi.EngineContextLoader,
 		pattern:          rule.Validation.GetPattern(),
 		anyPattern:       rule.Validation.GetAnyPattern(),
 		deny:             rule.Validation.Deny,
+		anyAllConditions: anyAllConditions,
 		forEach:          rule.Validation.ForEachValidation,
 	}
 }
@@ -121,6 +124,22 @@ func (v *validator) validate(ctx context.Context) *engineapi.RuleResponse {
 		}
 
 		ruleResponse := v.validateResourceWithRule()
+
+		if engineutils.IsUpdateRequest(v.policyContext) {
+			priorResp, err := v.validateOldObject(ctx)
+			if err != nil {
+				return engineapi.RuleError(v.rule.Name, engineapi.Validation, "failed to validate old object", err)
+			}
+
+			if engineutils.IsSameRuleResponse(ruleResponse, priorResp) {
+				v.log.V(3).Info("skipping modified resource as validation results have not changed")
+				if ruleResponse.Status() == engineapi.RuleStatusPass {
+					return ruleResponse
+				}
+				return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "skipping modified resource as validation results have not changed")
+			}
+		}
+
 		return ruleResponse
 	}
 
@@ -133,6 +152,20 @@ func (v *validator) validate(ctx context.Context) *engineapi.RuleResponse {
 	return nil
 }
 
+func (v *validator) validateOldObject(ctx context.Context) (*engineapi.RuleResponse, error) {
+	pc := v.policyContext
+	oldPc, err := v.policyContext.OldPolicyContext()
+	if err != nil {
+		return nil, errors.Wrapf(err, "cannot get old policy context")
+	}
+
+	v.policyContext = oldPc
+	resp := v.validate(ctx)
+	v.policyContext = pc
+
+	return resp, nil
+}
+
 func (v *validator) validateForEach(ctx context.Context) *engineapi.RuleResponse {
 	applyCount := 0
 	for _, foreach := range v.forEach {

pkg/engine/policycontext/policy_context.go

@@ -10,6 +10,7 @@ import (
 	enginectx "github.com/kyverno/kyverno/pkg/engine/context"
 	"github.com/kyverno/kyverno/pkg/engine/jmespath"
 	admissionutils "github.com/kyverno/kyverno/pkg/utils/admission"
+	"github.com/pkg/errors"
 	admissionv1 "k8s.io/api/admission/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -61,6 +62,26 @@ func (c *PolicyContext) Policy() kyvernov1.PolicyInterface {
 	return c.policy
 }
 
+func (c *PolicyContext) OldPolicyContext() (engineapi.PolicyContext, error) {
+	if c.Operation() != kyvernov1.Update {
+		return nil, errors.New("cannot create old policy context")
+	}
+	copy := c.copy()
+	oldJsonContext := copy.jsonContext
+	copy.oldResource = unstructured.Unstructured{}
+	copy.newResource = c.oldResource
+
+	if err := oldJsonContext.AddResource(nil); err != nil {
+		return nil, errors.Wrapf(err, "failed to replace object in the JSON context")
+	}
+	if err := oldJsonContext.AddOldResource(copy.OldResource().Object); err != nil {
+		return nil, errors.Wrapf(err, "failed to replace old object in the JSON context")
+	}
+
+	copy.jsonContext = oldJsonContext
+	return copy, nil
+}
+
 func (c *PolicyContext) NewResource() unstructured.Unstructured {
 	return c.newResource
 }
@@ -228,6 +249,7 @@ func NewPolicyContext(
 	if admissionInfo != nil {
 		policyContext = policyContext.WithAdmissionInfo(*admissionInfo)
 	}
+
 	return policyContext, nil
 }
 
@@ -256,6 +278,7 @@ func NewPolicyContextFromAdmissionRequest(
 		WithAdmissionOperation(true).
 		WithResourceKind(gvk, request.SubResource).
 		WithRequestResource(request.Resource)
+
 	return policyContext, nil
 }
 

pkg/engine/utils/utils.go

@@ -85,3 +85,19 @@ func TransformConditions(original apiextensions.JSON) (interface{}, error) {
 	}
 	return nil, fmt.Errorf("invalid preconditions")
 }
+
+func IsSameRuleResponse(r1 *engineapi.RuleResponse, r2 *engineapi.RuleResponse) bool {
+	if r1.Name() != r2.Name() ||
+		r1.RuleType() != r2.RuleType() ||
+		r1.Message() != r2.Message() ||
+		r1.Status() != r2.Status() {
+		return false
+	}
+
+	return true
+}
+
+func IsUpdateRequest(ctx engineapi.PolicyContext) bool {
+	// is the OldObject and NewObject are available, the request is an UPDATE
+	return ctx.OldResource().Object != nil && ctx.NewResource().Object != nil
+}

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/01-badpod.yaml

@@ -0,0 +1,13 @@
+---
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: TestStep
+metadata:
+  creationTimestamp: null
+  name: badpod
+spec:
+  timeouts: {}
+  try:
+  - apply:
+      file: bad-pod.yaml
+  - assert:
+      file: bad-pod-ready.yaml

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

@@ -0,0 +1,13 @@
+---
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: TestStep
+metadata:
+  creationTimestamp: null
+  name: policy
+spec:
+  timeouts: {}
+  try:
+  - apply:
+      file: policy.yaml
+  - assert:
+      file: policy-ready.yaml

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/03-goodpod.yaml

@@ -0,0 +1,13 @@
+---
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: TestStep
+metadata:
+  creationTimestamp: null
+  name: goodpod
+spec:
+  timeouts: {}
+  try:
+  - apply:
+      file: good-pod.yaml
+  - assert:
+      file: good-pod-ready.yaml

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/04-bad-pod-update.yaml

@@ -0,0 +1,13 @@
+---
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: TestStep
+metadata:
+  creationTimestamp: null
+  name: badpod
+spec:
+  timeouts: {}
+  try:
+  - script:
+      content: ./bad-pod-update-test.sh
+    timeout: 30s
+

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/05-good-pod-update.yaml

@@ -0,0 +1,12 @@
+---
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: TestStep
+metadata:
+  creationTimestamp: null
+  name: badpod
+spec:
+  timeouts: {}
+  try:
+  - script:
+      content: ./good-pod-update-test.sh
+    timeout: 30s

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/06-bad-to-good.yaml

@@ -0,0 +1,12 @@
+---
+apiVersion: chainsaw.kyverno.io/v1alpha1
+kind: TestStep
+metadata:
+  creationTimestamp: null
+  name: badtogoodpod
+spec:
+  timeouts: {}
+  try:
+  - script:
+      content: ./update-bad-pod-to-comply.sh
+    timeout: 30s

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

@@ -0,0 +1,15 @@
+## Description
+
+This test mainly verifies that an enforce validate policy does not block changes in old objects that were present before policy was created
+
+## Expected Behavior
+
+1. A pod is created that violates the policy.
+2. The policy is applied.
+3. A pod is created that follows the policy.
+4. Violating changes on bad pad does not cause error.
+5. Violating changes in good pod causes error.
+6. The bad pod once passed the policy, will be tracked by the policy and return error on bad changes.
+## Reference Issue(s)
+
+8837

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod-ready.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: badpod
+  namespace: default

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod-update-test.sh

@@ -0,0 +1,8 @@
+if kubectl label po badpod foo=bad1 --overwrite 2>&1 | grep -q  "validation error: rule check-labels" 
+then 
+  echo "Test failed, updating violating preexisting resource should not throw error"
+  exit 1
+else 
+  echo "Test succeed, updating violating preexisting resource does not throw error"
+  exit 0
+fi

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: badpod
+  namespace: default
+  labels:
+    foo: bad
+spec:
+  containers:
+  - name: container01
+    image: busybox:1.35
+    args:
+    - sleep
+    - 1d

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod-ready.yaml

@@ -0,0 +1,5 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: goodpod
+  namespace: default

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod-update-test.sh

@@ -0,0 +1,8 @@
+if kubectl label po goodpod foo=bad1 --overwrite 2>&1 | grep -q  "validation error: rule check-labels" 
+then 
+  echo "Test succeed, updating violating resource throws error"
+  exit 0
+else 
+  echo "Test failed, updating violating resource did not throw error"
+  exit 1
+fi

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod.yaml

@@ -0,0 +1,14 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  name: goodpod
+  namespace: default
+  labels:
+    foo: bar
+spec:
+  containers:
+  - name: container01
+    image: busybox:1.35
+    args:
+    - sleep
+    - 1d

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

@@ -0,0 +1,4 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: check-labels

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

@@ -0,0 +1,19 @@
+apiVersion: kyverno.io/v1
+kind: ClusterPolicy
+metadata:
+  name: check-labels
+spec:
+  validationFailureAction: Enforce
+  background: true
+  rules:
+    - name: check-labels
+      match:
+        any:
+        - resources:
+            kinds:
+              - Pod
+      validate:
+        pattern:
+          metadata:
+            labels:
+              =(foo): "bar"

test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/update-bad-pod-to-comply.sh

@@ -0,0 +1,9 @@
+kubectl label po badpod foo=bar --overwrite
+if kubectl label po badpod foo=bad1 --overwrite 2>&1 | grep -q  "validation error: rule check-labels" 
+then 
+  echo "Test succeed, updating violating resource throws error"
+  exit 0
+else 
+  echo "Test failed, updating violating resource did not throw error"
+  exit 1
+fi