From ab7675f0595e92f42529f681be55fc12fc19dc53 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Wed, 29 Oct 2025 15:49:09 -0400 Subject: [PATCH] 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 --- controllers/configurationpolicy_controller.go | 18 ++++---- test/e2e/case8_status_check_test.go | 44 +++++++++++++------ 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index c8a286bd..e7a3f2de 100644 --- a/controllers/configurationpolicy_controller.go +++ b/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 { diff --git a/test/e2e/case8_status_check_test.go b/test/e2e/case8_status_check_test.go index 15e0dbf4..fc909706 100644 --- a/test/e2e/case8_status_check_test.go +++ b/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()) })