From 1f4181645bac6a327ec76891faebae5946288cd5 Mon Sep 17 00:00:00 2001 From: Vishal Choudhary Date: Tue, 12 Dec 2023 14:47:53 +0530 Subject: [PATCH] fix: allow changes to preexisting resource in violation of a policy in Enforce (#9027) * fix: allow changes to preexisting resource in violation of a policy in Enforce Signed-off-by: Vishal Choudhary * fix: missing error check Signed-off-by: Vishal Choudhary * fix: tests Signed-off-by: Vishal Choudhary * nit: cleanup Signed-off-by: Vishal Choudhary * fix Signed-off-by: Vishal Choudhary * fix: update old policy context Signed-off-by: Vishal Choudhary * 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 Signed-off-by: Vishal Choudhary * fix: fix chainsaw test Signed-off-by: Vishal Choudhary * fix: nit Signed-off-by: Vishal Choudhary * debug Signed-off-by: Vishal Choudhary * feat: update test Signed-off-by: Vishal Choudhary * fix: add namespace Signed-off-by: Vishal Choudhary * feat: add test for bad to good conversion Signed-off-by: Vishal Choudhary * feat: add test step Signed-off-by: Vishal Choudhary --------- Signed-off-by: Vishal Choudhary Co-authored-by: shuting --- pkg/engine/api/policycontext.go | 1 + .../handlers/validation/validate_resource.go | 49 ++++++++++++++++--- pkg/engine/policycontext/policy_context.go | 23 +++++++++ pkg/engine/utils/utils.go | 16 ++++++ .../enforce-validate-existing/01-badpod.yaml | 13 +++++ .../enforce-validate-existing/02-policy.yaml | 13 +++++ .../enforce-validate-existing/03-goodpod.yaml | 13 +++++ .../04-bad-pod-update.yaml | 13 +++++ .../05-good-pod-update.yaml | 12 +++++ .../06-bad-to-good.yaml | 12 +++++ .../enforce-validate-existing/README.md | 15 ++++++ .../bad-pod-ready.yaml | 5 ++ .../bad-pod-update-test.sh | 8 +++ .../enforce-validate-existing/bad-pod.yaml | 14 ++++++ .../good-pod-ready.yaml | 5 ++ .../good-pod-update-test.sh | 8 +++ .../enforce-validate-existing/good-pod.yaml | 14 ++++++ .../policy-ready.yaml | 4 ++ .../enforce-validate-existing/policy.yaml | 19 +++++++ .../update-bad-pod-to-comply.sh | 9 ++++ 20 files changed, 258 insertions(+), 8 deletions(-) create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/01-badpod.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/02-policy.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/03-goodpod.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/04-bad-pod-update.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/05-good-pod-update.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/06-bad-to-good.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/README.md create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod-ready.yaml create mode 100755 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod-update-test.sh create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod-ready.yaml create mode 100755 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod-update-test.sh create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/policy-ready.yaml create mode 100644 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/policy.yaml create mode 100755 test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/update-bad-pod-to-comply.sh diff --git a/pkg/engine/api/policycontext.go b/pkg/engine/api/policycontext.go index 001e1b78f4..0119057681 100644 --- a/pkg/engine/api/policycontext.go +++ b/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 } diff --git a/pkg/engine/handlers/validation/validate_resource.go b/pkg/engine/handlers/validation/validate_resource.go index d62d142d40..99dcad7dd4 100644 --- a/pkg/engine/handlers/validation/validate_resource.go +++ b/pkg/engine/handlers/validation/validate_resource.go @@ -19,6 +19,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" "k8s.io/client-go/tools/cache" @@ -72,15 +73,17 @@ 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, - policyContext: ctx, - contextLoader: contextLoader, - pattern: rule.Validation.GetPattern(), - anyPattern: rule.Validation.GetAnyPattern(), - deny: rule.Validation.Deny, - forEach: rule.Validation.ForEachValidation, + log: log, + rule: rule, + policyContext: ctx, + contextLoader: contextLoader, + pattern: rule.Validation.GetPattern(), + anyPattern: rule.Validation.GetAnyPattern(), + deny: rule.Validation.Deny, + anyAllConditions: anyAllConditions, + forEach: rule.Validation.ForEachValidation, } } @@ -138,6 +141,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 } @@ -150,6 +169,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 { diff --git a/pkg/engine/policycontext/policy_context.go b/pkg/engine/policycontext/policy_context.go index 72c4bb4be1..7e6b42de15 100644 --- a/pkg/engine/policycontext/policy_context.go +++ b/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 } @@ -229,6 +250,7 @@ func NewPolicyContext( if admissionInfo != nil { policyContext = policyContext.WithAdmissionInfo(*admissionInfo) } + return policyContext, nil } @@ -257,6 +279,7 @@ func NewPolicyContextFromAdmissionRequest( WithAdmissionOperation(true). WithResourceKind(gvk, request.SubResource). WithRequestResource(request.Resource) + return policyContext, nil } diff --git a/pkg/engine/utils/utils.go b/pkg/engine/utils/utils.go index 3e0e1e03fe..0e73e6be06 100644 --- a/pkg/engine/utils/utils.go +++ b/pkg/engine/utils/utils.go @@ -93,3 +93,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 +} diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/01-badpod.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/01-badpod.yaml new file mode 100644 index 0000000000..33c738f694 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/02-policy.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/02-policy.yaml new file mode 100644 index 0000000000..e521d0d761 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/03-goodpod.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/03-goodpod.yaml new file mode 100644 index 0000000000..fa9bbabaf4 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/04-bad-pod-update.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/04-bad-pod-update.yaml new file mode 100644 index 0000000000..f29508c290 --- /dev/null +++ b/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 + diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/05-good-pod-update.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/05-good-pod-update.yaml new file mode 100644 index 0000000000..4f1bc78b57 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/06-bad-to-good.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/06-bad-to-good.yaml new file mode 100644 index 0000000000..8e509729ae --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/README.md b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/README.md new file mode 100644 index 0000000000..b80a445992 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod-ready.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod-ready.yaml new file mode 100644 index 0000000000..8f64ef5e24 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod-update-test.sh b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod-update-test.sh new file mode 100755 index 0000000000..6521f19d1c --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/bad-pod.yaml new file mode 100644 index 0000000000..49031dc2f8 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod-ready.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod-ready.yaml new file mode 100644 index 0000000000..210e0fb885 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod-update-test.sh b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod-update-test.sh new file mode 100755 index 0000000000..6e7187cf91 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/good-pod.yaml new file mode 100644 index 0000000000..3c10e0b7dc --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/policy-ready.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/policy-ready.yaml new file mode 100644 index 0000000000..100a267bab --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/policy.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/policy.yaml new file mode 100644 index 0000000000..2393c4e79d --- /dev/null +++ b/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" diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/update-bad-pod-to-comply.sh b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/enforce-validate-existing/update-bad-pod-to-comply.sh new file mode 100755 index 0000000000..9d44d8851d --- /dev/null +++ b/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