Merge pull request #152 from nirmata/NDEV-20176-1.11-extended

NDEV-20176 : Backport (fix) skip processing the oldObject for audit policies for n4k-1.11

pkg/engine/handlers/validation/validate_pss.go

@@ -78,23 +78,26 @@ func (h validatePssHandler) validate(
 	} else {
 		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).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)
-			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")
-			}
+		action := policyContext.Policy().GetSpec().ValidationFailureAction
+
+		// 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)
+				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")
+				}
 
-			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
+				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")
 				}
-				return resource, engineapi.RuleSkip(rule.Name, engineapi.Validation, "skipping modified resource as validation results have not changed")
 			}
-			logger.V(4).Info("old object response is different", "oldResp", priorResp, "newResp", ruleResponse)
 		}
 
 		return resource, ruleResponse

pkg/engine/handlers/validation/validate_resource.go

@@ -129,23 +129,27 @@ 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")
-		}
+	action := v.policyContext.Policy().GetSpec().ValidationFailureAction
+
+	// 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")
+			}
 
-		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
+			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")
 			}
-			return engineapi.RuleSkip(v.rule.Name, engineapi.Validation, "skipping modified resource as validation results have not changed")
 		}
 	}
-
 	return ruleResponse
 }
 

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

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

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

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

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

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

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