diff --git a/pkg/engine/handlers/validation/validate_assert.go b/pkg/engine/handlers/validation/validate_assert.go index e81bce0af0..511fef8645 100644 --- a/pkg/engine/handlers/validation/validate_assert.go +++ b/pkg/engine/handlers/validation/validate_assert.go @@ -112,20 +112,31 @@ func (h validateAssertHandler) Process( } // compose a response if len(errs) != 0 { - allowExisitingViolations := rule.HasValidateAllowExistingViolations() - if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations { - errs, err := validateOldObject(ctx, policyContext, rule, payload, bindings) - if err != nil { - logger.V(2).Info("warning: failed to validate old object, skipping the rule evaluation as pre-existing violations are allowed", "rule", rule.Name, "error", err.Error()) - return resource, handlers.WithSkip(rule, engineapi.Validation, "failed to validate old object, skipping as preexisting violations are allowed") - } + var action kyvernov1.ValidationFailureAction + if rule.Validation.FailureAction != nil { + action = *rule.Validation.FailureAction + } else { + action = policyContext.Policy().GetSpec().ValidationFailureAction + } - logger.V(3).Info("old object verification", "errors", errs) - if len(errs) != 0 { - logger.V(3).Info("skipping modified resource as validation results have not changed") - return resource, handlers.WithSkip(rule, engineapi.Validation, "skipping modified resource as validation results have not changed") + // process the old object for UPDATE admission requests in case of enforce policies + if action == kyvernov1.Enforce { + allowExisitingViolations := rule.HasValidateAllowExistingViolations() + if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations { + errs, err := validateOldObject(ctx, 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") + } + + logger.V(3).Info("old object verification", "errors", errs) + if len(errs) != 0 { + logger.V(2).Info("warning: skipping the rule evaluation as pre-existing violations are allowed", "rule", rule.Name) + return resource, handlers.WithSkip(rule, engineapi.Validation, "skipping the rule evaluation as pre-existing violations are allowed") + } } } + var responses []*engineapi.RuleResponse for _, err := range errs { responses = append(responses, engineapi.RuleFail(rule.Name, engineapi.Validation, err.Error(), rule.ReportProperties)) diff --git a/pkg/engine/handlers/validation/validate_pss.go b/pkg/engine/handlers/validation/validate_pss.go index 56e92f7224..4d3f2cee0a 100644 --- a/pkg/engine/handlers/validation/validate_pss.go +++ b/pkg/engine/handlers/validation/validate_pss.go @@ -133,23 +133,31 @@ func (h validatePssHandler) validate( } msg := fmt.Sprintf(`Validation rule '%s' failed. It violates PodSecurity "%s:%s": %s`, rule.Name, podSecurity.Level, podSecurity.Version, pss.FormatChecksPrint(pssChecks)) ruleResponse := engineapi.RuleFail(rule.Name, engineapi.Validation, msg, rule.ReportProperties).WithPodSecurityChecks(podSecurityChecks) - allowExisitingViolations := rule.HasValidateAllowExistingViolations() - if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations { - logger.V(4).Info("is update request") - priorResp, err := h.validateOldObject(ctx, logger, policyContext, resource, rule, engineLoader, exceptions) - if err != nil { - logger.V(2).Info("warning: failed to validate old object, skipping the rule evaluation as pre-existing violations are allowed", "rule", rule.Name, "error", err.Error()) - return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "failed to validate old object, skipping as preexisting violations are allowed", rule.ReportProperties) - } + var action kyvernov1.ValidationFailureAction + if rule.Validation.FailureAction != nil { + action = *rule.Validation.FailureAction + } else { + action = policyContext.Policy().GetSpec().ValidationFailureAction + } - if ruleResponse.Status() == priorResp.Status() { - logger.V(3).Info("skipping modified resource as validation results have not changed", "oldResp", priorResp, "newResp", ruleResponse) - if ruleResponse.Status() == engineapi.RuleStatusPass { - return resource, ruleResponse + // process the old object for UPDATE admission requests in case of enforce policies + if action == kyvernov1.Enforce { + allowExisitingViolations := rule.HasValidateAllowExistingViolations() + if engineutils.IsUpdateRequest(policyContext) && allowExisitingViolations { + priorResp, err := h.validateOldObject(ctx, logger, policyContext, resource, rule, engineLoader, exceptions) + if err != nil { + logger.V(4).Info("warning: failed to validate old object", "rule", rule.Name, "error", err.Error()) + return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "failed to validate old object", rule.ReportProperties) + } + + if ruleResponse.Status() == priorResp.Status() { + logger.V(2).Info("warning: skipping the rule evaluation as pre-existing violations are allowed", "oldResp", priorResp, "newResp", ruleResponse) + if ruleResponse.Status() == engineapi.RuleStatusPass { + return resource, ruleResponse + } + return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "skipping the rule evaluation as pre-existing violations are allowed", rule.ReportProperties) } - return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "skipping modified resource as validation results have not changed", rule.ReportProperties) } - logger.V(4).Info("old object response is different", "oldResp", priorResp, "newResp", ruleResponse) } return resource, ruleResponse diff --git a/pkg/engine/handlers/validation/validate_resource.go b/pkg/engine/handlers/validation/validate_resource.go index 6dc94fcbab..acffdc2457 100644 --- a/pkg/engine/handlers/validation/validate_resource.go +++ b/pkg/engine/handlers/validation/validate_resource.go @@ -146,20 +146,30 @@ func (v *validator) validate(ctx context.Context) *engineapi.RuleResponse { v.log.V(2).Info("invalid validation rule: podSecurity, cel, patterns, or deny expected") } - allowExisitingViolations := v.rule.HasValidateAllowExistingViolations() - if engineutils.IsUpdateRequest(v.policyContext) && allowExisitingViolations && v.nesting == 0 { // is update request and is the root level validate - priorResp, err := v.validateOldObject(ctx) - if err != nil { - v.log.V(2).Info("warning: failed to validate old object, skipping the rule evaluation as pre-existing violations are allowed", "rule", v.rule.Name, "error", err.Error()) - return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "failed to validate old object, skipping as preexisting violations are allowed", ruleResponse.Properties()) - } + var action kyvernov1.ValidationFailureAction + if v.rule.Validation.FailureAction != nil { + action = *v.rule.Validation.FailureAction + } else { + action = v.policyContext.Policy().GetSpec().ValidationFailureAction + } - 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 + // process the old object for UPDATE admission requests in case of enforce policies + if action == kyvernov1.Enforce { + allowExisitingViolations := v.rule.HasValidateAllowExistingViolations() + if engineutils.IsUpdateRequest(v.policyContext) && allowExisitingViolations && v.nesting == 0 { // is update request and is the root level validate + priorResp, err := v.validateOldObject(ctx) + if err != nil { + v.log.V(4).Info("warning: failed to validate old object", "rule", v.rule.Name, "error", err.Error()) + return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "failed to validate old object", ruleResponse.Properties()) + } + + if engineutils.IsSameRuleResponse(ruleResponse, priorResp) { + v.log.V(2).Info("warning: skipping the rule evaluation as pre-existing violations are allowed") + if ruleResponse.Status() == engineapi.RuleStatusPass { + return ruleResponse + } + return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "skipping the rule evaluation as pre-existing violations are allowed", v.rule.ReportProperties) } - return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "skipping modified resource as validation results have not changed", v.rule.ReportProperties) } } diff --git a/pkg/engine/handlers/validation/validate_resource_test.go b/pkg/engine/handlers/validation/validate_resource_test.go index c2eee5c430..ce5bcc0283 100644 --- a/pkg/engine/handlers/validation/validate_resource_test.go +++ b/pkg/engine/handlers/validation/validate_resource_test.go @@ -150,9 +150,9 @@ var ( }, "name": "check-image", "validate": { - "failureAction": "Audit", - "allowExistingViolations": true, - "foreach": [ + "failureAction": "Enforce", + "allowExistingViolations": true, + "foreach": [ { "deny": { "conditions": { diff --git a/test/conformance/chainsaw/reports/admission/update-deployment/README.md b/test/conformance/chainsaw/reports/admission/update-deployment/README.md new file mode 100644 index 0000000000..e34ba40e30 --- /dev/null +++ b/test/conformance/chainsaw/reports/admission/update-deployment/README.md @@ -0,0 +1,15 @@ +## Description + +This test verifies that policy report doesn't change when a resource is updated and the engine response is the same as before. + +A policy in Audit mode is created. +A deployment is created, the deployment violates the policy and we assert the policy report contains a `warn` result. +The deployment is then updated but it still violates the policy. + +## Expected result + +When the resource is updated and it still violates the policy, the policy report should not change. + +## Related issue(s) + +- https://github.com/kyverno/kyverno/issues/10169 \ No newline at end of file diff --git a/test/conformance/chainsaw/reports/admission/update-deployment/chainsaw-test.yaml b/test/conformance/chainsaw/reports/admission/update-deployment/chainsaw-test.yaml new file mode 100755 index 0000000000..f03b4bd360 --- /dev/null +++ b/test/conformance/chainsaw/reports/admission/update-deployment/chainsaw-test.yaml @@ -0,0 +1,41 @@ +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + creationTimestamp: null + name: update-deployment +spec: + steps: + - name: step-01 + try: + - apply: + file: policy.yaml + - assert: + file: policy-assert.yaml + - name: step-02 + try: + - apply: + file: deployment.yaml + - assert: + file: deployment.yaml + - name: step-03 + try: + - sleep: + duration: 5s + - name: step-04 + try: + - assert: + file: report-assert.yaml + - name: step-05 + try: + - apply: + file: update-deployment.yaml + - assert: + file: update-deployment.yaml + - name: step-06 + try: + - sleep: + duration: 5s + - name: step-07 + try: + - assert: + file: report-assert.yaml diff --git a/test/conformance/chainsaw/reports/admission/update-deployment/deployment.yaml b/test/conformance/chainsaw/reports/admission/update-deployment/deployment.yaml new file mode 100644 index 0000000000..60d5d76774 --- /dev/null +++ b/test/conformance/chainsaw/reports/admission/update-deployment/deployment.yaml @@ -0,0 +1,19 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: nginx + name: nginx-test +spec: + replicas: 1 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx + image: nginx:latest diff --git a/test/conformance/chainsaw/reports/admission/update-deployment/policy-assert.yaml b/test/conformance/chainsaw/reports/admission/update-deployment/policy-assert.yaml new file mode 100644 index 0000000000..94bd911ff5 --- /dev/null +++ b/test/conformance/chainsaw/reports/admission/update-deployment/policy-assert.yaml @@ -0,0 +1,9 @@ +apiVersion: kyverno.io/v1 +kind: ClusterPolicy +metadata: + name: require-multiple-replicas +status: + conditions: + - reason: Succeeded + status: "True" + type: Ready diff --git a/test/conformance/chainsaw/reports/admission/update-deployment/policy.yaml b/test/conformance/chainsaw/reports/admission/update-deployment/policy.yaml new file mode 100644 index 0000000000..f0235313b7 --- /dev/null +++ b/test/conformance/chainsaw/reports/admission/update-deployment/policy.yaml @@ -0,0 +1,29 @@ +apiVersion: kyverno.io/v1 +kind: ClusterPolicy +metadata: + name: require-multiple-replicas + annotations: + policies.kyverno.io/category: Best Practises + policies.kyverno.io/minversion: 1.9.2 + policies.kyverno.io/severity: low + policies.kyverno.io/subject: Deployment,StatefulSet + policies.kyverno.io/title: Require Multiple Replicas + policies.kyverno.io/scored: "false" +spec: + background: false + rules: + - name: require-multiple-replicas + match: + any: + - resources: + kinds: + - Deployment + - StatefulSet + operations: + - CREATE + - UPDATE + validate: + pattern: + spec: + replicas: ">1" + validationFailureAction: Audit diff --git a/test/conformance/chainsaw/reports/admission/update-deployment/report-assert.yaml b/test/conformance/chainsaw/reports/admission/update-deployment/report-assert.yaml new file mode 100644 index 0000000000..3541c2268e --- /dev/null +++ b/test/conformance/chainsaw/reports/admission/update-deployment/report-assert.yaml @@ -0,0 +1,23 @@ +apiVersion: wgpolicyk8s.io/v1alpha2 +kind: PolicyReport +metadata: + ownerReferences: + - apiVersion: apps/v1 + kind: Deployment + name: nginx-test +results: +- message: 'validation error: rule require-multiple-replicas failed at path /spec/replicas/' + policy: require-multiple-replicas + result: warn + rule: require-multiple-replicas + source: kyverno +scope: + apiVersion: apps/v1 + kind: Deployment + name: nginx-test +summary: + error: 0 + fail: 0 + pass: 0 + skip: 0 + warn: 1 diff --git a/test/conformance/chainsaw/reports/admission/update-deployment/update-deployment.yaml b/test/conformance/chainsaw/reports/admission/update-deployment/update-deployment.yaml new file mode 100644 index 0000000000..902f9d82ba --- /dev/null +++ b/test/conformance/chainsaw/reports/admission/update-deployment/update-deployment.yaml @@ -0,0 +1,19 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: nginx + name: nginx-test +spec: + replicas: 1 + selector: + matchLabels: + app: nginx + template: + metadata: + labels: + app: nginx + spec: + containers: + - name: nginx-1 + image: nginx:latest