diff --git a/pkg/engine/api/policycontext.go b/pkg/engine/api/policycontext.go index 41df6b1e8c..23ed9d5855 100644 --- a/pkg/engine/api/policycontext.go +++ b/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) diff --git a/pkg/engine/handlers/validation/utils.go b/pkg/engine/handlers/validation/utils.go index 1f6c9e1a94..9e264479ac 100644 --- a/pkg/engine/handlers/validation/utils.go +++ b/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"}, diff --git a/pkg/engine/handlers/validation/validate_assert.go b/pkg/engine/handlers/validation/validate_assert.go index b9c4619ab6..0cb3857bc6 100644 --- a/pkg/engine/handlers/validation/validate_assert.go +++ b/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 { - return nil, nil + 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") } payload["object"] = policyContext.OldResource().Object payload["oldObject"] = nil payload["operation"] = kyvernov1.Create - asserttion := assert.Parse(ctx, rule.Validation.Assert.Value) - errs, err := assert.Assert(ctx, nil, asserttion, payload, bindings) - if err != nil { - return nil, fmt.Errorf("failed to apply assertion: %w", err) + defer func() { + if err = policyContext.SetOperation(kyvernov1.Update); err != nil { + logger.Error(errors.Wrapf(err, "failed to reset operation"), "") + } + + 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 } diff --git a/pkg/engine/handlers/validation/validate_pss.go b/pkg/engine/handlers/validation/validate_pss.go index 7caab62175..d330c08433 100644 --- a/pkg/engine/handlers/validation/validate_pss.go +++ b/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 { - return nil, nil - } - if err := policyContext.SetResources(emptyResource, oldResource); err != 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 { - return nil, errors.Wrapf(err, "failed to reset resources") + if err = policyContext.SetOperation(kyvernov1.Update); err != nil { + 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 { - return nil, errors.Wrapf(err, "failed to reset operation") - } + _, resp = h.validate(ctx, logger, policyContext, oldResource, rule, engineLoader, exceptions) - return resp, nil + return } func convertChecks(checks []pssutils.PSSCheckResult, kind string) (newChecks []pssutils.PSSCheckResult) { diff --git a/pkg/engine/handlers/validation/validate_resource.go b/pkg/engine/handlers/validation/validate_resource.go index 141e25ad6c..c821d0377f 100644 --- a/pkg/engine/handlers/validation/validate_resource.go +++ b/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 { - return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "resource not matched", v.rule.ReportProperties), nil - } - - if err := v.policyContext.SetResources(emptyResource, oldResource); err != 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 { - return nil, errors.Wrapf(err, "failed to reset resources") + if err = v.policyContext.SetOperation(kyvernov1.Update); err != nil { + 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 { - return nil, errors.Wrapf(err, "failed to reset operation") - } + resp = v.validate(ctx) - return resp, nil + return } func (v *validator) validateForEach(ctx context.Context) *engineapi.RuleResponse { diff --git a/pkg/engine/handlers/validation/validate_resource_test.go b/pkg/engine/handlers/validation/validate_resource_test.go index ce5bcc0283..90af4877b7 100644 --- a/pkg/engine/handlers/validation/validate_resource_test.go +++ b/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": { diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/README.md b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/README.md new file mode 100644 index 0000000000..b762a366b4 --- /dev/null +++ b/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. diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/chainsaw-test.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/chainsaw-test.yaml new file mode 100755 index 0000000000..e5155013c1 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress-ready.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress-ready.yaml new file mode 100644 index 0000000000..4176002cea --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress-update.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress-update.yaml new file mode 100644 index 0000000000..ee8cafa458 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/ingress.yaml new file mode 100644 index 0000000000..c932cb3722 --- /dev/null +++ b/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 diff --git a/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/policy.yaml b/test/conformance/chainsaw/validate/clusterpolicy/standard/enforce/validate-existing-with-precondition/policy.yaml new file mode 100644 index 0000000000..02419339f4 --- /dev/null +++ b/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\" }}"