Correct compliance on no-op dryrun updates

Previously, the policy status would incorrectly have a noncompliant
event followed immediately by a compliant event when a dryrun update
would not make any changes on that object. This would cause the policy
status to repeatedly update if the policy was reconciling often - for
example if any of its watched objects were updating.

Now only the correct "compliant" event should be emitted in that case.

Refs:
 - https://issues.redhat.com/browse/ACM-25694

Signed-off-by: Justin Kulikauskas <[email protected]>

Author: JustinKuli

controllers/configurationpolicy_controller.go

@@ -3325,12 +3325,10 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 				diff = handleDiff(log, recordDiff, existingObjectCopy, mergedObjCopy, r.FullDiffs)
 			}
 
-			// Assume object is compliant by inverting the value of throwSpecViolation
-			r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "")
+			// treat the object as compliant, with no updates needed
+			r.setEvaluatedObject(obj.policy, obj.existingObj, true, "")
 
-			matchesAfterDryRun = true
-
-			return throwSpecViolation, "", diff, updateNeeded, updatedObj, matchesAfterDryRun
+			return false, "", diff, false, updatedObj, true
 		}
 
 		diff = handleDiff(log, recordDiff, existingObjectCopy, dryRunUpdatedObj, r.FullDiffs)
@@ -3340,7 +3338,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 	if isInform {
 		r.setEvaluatedObject(obj.policy, obj.existingObj, false, "")
 
-		return true, "", diff, false, nil, matchesAfterDryRun
+		return true, "", diff, false, nil, false
 	}
 
 	// If it's not inform (i.e. enforce), update the object
@@ -3360,7 +3358,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 		if err != nil && !k8serrors.IsNotFound(err) {
 			message = fmt.Sprintf(`%s failed to delete when recreating with the error %v`, getMsgPrefix(&obj), err)
 
-			return true, message, "", updateNeeded, nil, matchesAfterDryRun
+			return true, message, "", updateNeeded, nil, false
 		}
 
 		attempts := 0
@@ -3378,7 +3376,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 				message = getMsgPrefix(&obj) + " timed out waiting for the object to delete during recreate, " +
 					"will retry on the next policy evaluation"
 
-				return true, message, "", updateNeeded, nil, matchesAfterDryRun
+				return true, message, "", updateNeeded, nil, false
 			}
 
 			time.Sleep(time.Second)
@@ -3414,14 +3412,14 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
 			message = fmt.Sprintf("%s failed to %s with the error `%v`", getMsgPrefix(&obj), action, err)
 		}
 
-		return true, message, diff, updateNeeded, nil, matchesAfterDryRun
+		return true, message, diff, updateNeeded, nil, false
 	}
 
 	if !statusMismatch {
 		r.setEvaluatedObject(obj.policy, updatedObj, true, message)
 	}
 
-	return throwSpecViolation, "", diff, updateNeeded, updatedObj, matchesAfterDryRun
+	return throwSpecViolation, "", diff, updateNeeded, updatedObj, false
 }
 
 func getMsgPrefix(obj *singleObject) string {

test/e2e/case8_status_check_test.go

@@ -11,21 +11,21 @@ import (
 	"open-cluster-management.io/config-policy-controller/test/utils"
 )
 
-const (
-	case8ConfigPolicyNamePod         string = "policy-pod-to-check"
-	case8ConfigPolicyNameCheck       string = "policy-status-checker"
-	case8ConfigPolicyNameCheckFail   string = "policy-status-checker-fail"
-	case8ConfigPolicyNameEnforceFail string = "policy-status-enforce-fail"
-	case8PolicyYamlPod               string = "../resources/case8_status_check/case8_pod.yaml"
-	case8PolicyYamlCheck             string = "../resources/case8_status_check/case8_status_check.yaml"
-	case8PolicyYamlCheckFail         string = "../resources/case8_status_check/case8_status_check_fail.yaml"
-	case8PolicyYamlEnforceFail       string = "../resources/case8_status_check/case8_status_enforce_fail.yaml"
-	case8ConfigPolicyStatusPod       string = "policy-pod-invalid"
-	case8PolicyYamlBadPod            string = "../resources/case8_status_check/case8_pod_fail.yaml"
-	case8PolicyYamlSpecChange        string = "../resources/case8_status_check/case8_pod_change.yaml"
-)
-
 var _ = Describe("Test pod obj template handling", func() {
+	const (
+		case8ConfigPolicyNamePod         string = "policy-pod-to-check"
+		case8ConfigPolicyNameCheck       string = "policy-status-checker"
+		case8ConfigPolicyNameCheckFail   string = "policy-status-checker-fail"
+		case8ConfigPolicyNameEnforceFail string = "policy-status-enforce-fail"
+		case8PolicyYamlPod               string = "../resources/case8_status_check/case8_pod.yaml"
+		case8PolicyYamlCheck             string = "../resources/case8_status_check/case8_status_check.yaml"
+		case8PolicyYamlCheckFail         string = "../resources/case8_status_check/case8_status_check_fail.yaml"
+		case8PolicyYamlEnforceFail       string = "../resources/case8_status_check/case8_status_enforce_fail.yaml"
+		case8ConfigPolicyStatusPod       string = "policy-pod-invalid"
+		case8PolicyYamlBadPod            string = "../resources/case8_status_check/case8_pod_fail.yaml"
+		case8PolicyYamlSpecChange        string = "../resources/case8_status_check/case8_pod_change.yaml"
+	)
+
 	Describe("Create a policy on managed cluster in ns:"+testNamespace, Ordered, func() {
 		It("should create a policy properly on the managed cluster", func() {
 			By("Creating " + case8ConfigPolicyNamePod + " on managed")
@@ -204,6 +204,10 @@ var _ = Describe("Test related object property status", Ordered, func() {
 				matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun")
 
 				g.Expect(matchesAfterDryRun).To(BeTrue())
+
+				history, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "history")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(history).To(HaveLen(1))
 			}, defaultTimeoutSeconds, 1).Should(Succeed())
 		})
 
@@ -227,6 +231,10 @@ var _ = Describe("Test related object property status", Ordered, func() {
 				matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun")
 
 				g.Expect(matchesAfterDryRun).To(BeTrue())
+
+				history, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "history")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(history).To(HaveLen(2))
 			}, defaultTimeoutSeconds, 1).Should(Succeed())
 		})
 
@@ -251,6 +259,10 @@ var _ = Describe("Test related object property status", Ordered, func() {
 				matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun")
 
 				g.Expect(matchesAfterDryRun).To(BeFalse())
+
+				history, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "history")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(history).To(HaveLen(3))
 			}, defaultTimeoutSeconds, 1).Should(Succeed())
 		})
 
@@ -275,6 +287,10 @@ var _ = Describe("Test related object property status", Ordered, func() {
 				matchesAfterDryRun, _, _ := unstructured.NestedBool(relatedObj, "properties", "matchesAfterDryRun")
 
 				g.Expect(matchesAfterDryRun).To(BeFalse())
+
+				history, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "history")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(history).To(HaveLen(4))
 			}, defaultTimeoutSeconds, 1).Should(Succeed())
 		})