Correct to Pending status after late events

Previously, if the policy controller sent out a compliance event after
the policy transitioned to a Pending state (because things are async),
that compliance event would override the Pending state, and might never
be corrected. Now, the template-sync should re-reconcile in that case,
and apply the Pending state again.

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

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

Author: JustinKuli

controllers/templatesync/predicate.go

@@ -0,0 +1,50 @@
+// Copyright (c) 2023 Red Hat, Inc.
+// Copyright Contributors to the Open Cluster Management project
+
+package templatesync
+
+import (
+	policiesv1 "open-cluster-management.io/governance-policy-propagator/api/v1"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+)
+
+// templatePredicates filters out changes to policies that don't need to be
+// considered by the template-sync controller.
+func templatePredicates() predicate.Funcs {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			oldPolicy := e.ObjectOld.(*policiesv1.Policy)
+			updatedPolicy := e.ObjectNew.(*policiesv1.Policy)
+
+			if oldPolicy.Generation != updatedPolicy.Generation {
+				// The spec changed - templates need to be updated.
+				return true
+			}
+
+			if hasAnyDependencies(updatedPolicy) {
+				// if it has dependencies, and it's not currently Pending, then
+				// it needs to re-calculate if it *should* be Pending.
+				return updatedPolicy.Status.ComplianceState != "Pending"
+			}
+
+			return false
+		},
+	}
+}
+
+// hasAnyDependencies returns true if the policy has any Dependencies or if
+// any of its templates have any ExtraDependencies.
+func hasAnyDependencies(pol *policiesv1.Policy) bool {
+	if len(pol.Spec.Dependencies) > 0 {
+		return true
+	}
+
+	for _, tmpl := range pol.Spec.PolicyTemplates {
+		if len(tmpl.ExtraDependencies) > 0 {
+			return true
+		}
+	}
+
+	return false
+}

controllers/templatesync/template_sync.go

@@ -37,7 +37,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
-	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
@@ -62,7 +61,7 @@ func (r *PolicyReconciler) Setup(mgr ctrl.Manager, depEvents *source.Channel) er
 	return ctrl.NewControllerManagedBy(mgr).
 		Named(ControllerName).
 		For(&policiesv1.Policy{}).
-		WithEventFilter(predicate.GenerationChangedPredicate{}).
+		WithEventFilter(templatePredicates()).
 		Watches(depEvents, &handler.EnqueueRequestForObject{}).
 		Complete(r)
 }

test/e2e/case12_ordering_test.go

@@ -5,6 +5,7 @@ package e2e
 
 import (
 	"context"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -338,7 +339,46 @@ var _ = Describe("Test dependency logic in template sync", Ordered, func() {
 		)
 
 		// should be noncompliant - template A is pending and B is noncompliant
-		By("Checking if policy status is pending")
+		By("Checking if policy status is noncompliant")
 		Eventually(checkCompliance(case12Plc2TemplatesName), defaultTimeoutSeconds, 1).Should(Equal("NonCompliant"))
 	})
+	It("Should correct a late compliance event while the policy is pending", func() {
+		By("Creating a dep on hub cluster in ns:" + clusterNamespaceOnHub)
+		hubPolicyApplyAndDeferCleanup(case12DepYaml, case12DepName)
+
+		By("Creating a policy on hub cluster in ns:" + clusterNamespaceOnHub)
+		hubPolicyApplyAndDeferCleanup(case12PolicyYaml, case12PolicyName)
+
+		By("Generating a noncompliant event on the dependency")
+		generateEventOnPolicy(
+			case12DepName,
+			"namespace-foo-setup-configpolicy",
+			"there is violation",
+			"NonCompliant",
+		)
+
+		By("Checking if dependency status is noncompliant")
+		Eventually(checkCompliance(case12DepName), defaultTimeoutSeconds, 1).Should(Equal("NonCompliant"))
+
+		By("Checking if policy status is pending")
+		Eventually(checkCompliance(case12PolicyName), defaultTimeoutSeconds*2, 1).Should(Equal("Pending"))
+
+		By("Generating an (incorrect, late) compliance event on the policy")
+		generateEventOnPolicy(
+			case12PolicyName,
+			"case12-config-policy",
+			"No violation detected",
+			"Compliant",
+		)
+
+		// Sleep 5 seconds to avoid the Compliant status that only sometimes appears;
+		// otherwise, the next Eventually might finish *before* that status comes and goes.
+		time.Sleep(5 * time.Second)
+
+		By("Checking if policy status is pending")
+		Eventually(checkCompliance(case12PolicyName), defaultTimeoutSeconds, 1).Should(Equal("Pending"))
+
+		By("Checking if policy status is consistently pending")
+		Consistently(checkCompliance(case12PolicyName), "15s", 1).Should(Equal("Pending"))
+	})
 })