Fix evaluation time when the config-policy-controller is uninstalled

When a hosted cluster is removed from OCM, it triggers the hosted cluster instance of the config-policy-controller to be uninstalled on the hosting cluster. If any ConfigurationPolicy uses pruneObjectBehavior, they will have finalizers set on them. During an uninstall, the finalizers are immediately removed on the next evaluation of the ConfigurationPolicy with pruneObjectBehavior set so that the uninstall can proceed immediately. The issue is if the ConfigurationPolicy sets evaluationInterval to a long value, the finalizer won't be removed until the next evaluation time, which could be hours. This is not an issue when it's not deployed in hosted mode because the CRD is also deleted at the same time, which causes the ConfigurationPolicy to have a deletionTimestamp which then causes immediate evaluation for the finalizer to be removed. Relates: https://issues.redhat.com/browse/ACM-3233 Signed-off-by: mprahl <mprahl@users.noreply.github.com>
stolostron · Feb 3, 2023 · eb8f720 · eb8f720
1 parent 81a1487
commit eb8f720
Show file tree

Hide file tree

Showing 2 changed files with 72 additions and 4 deletions.
diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go
@@ -211,6 +211,13 @@ func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies(freq uint
 			}
 		}
 
+		cleanupImmediately, err := r.cleanupImmediately()
+		if err != nil {
+			log.Error(err, "Failed to determine if it's time to cleanup immediately")
+
+			skipLoop = true
+		}
+
 		// This is done every loop cycle since the channel needs to be variable in size to account for the number of
 		// policies changing.
 		policyQueue := make(chan *policyv1.ConfigurationPolicy, len(policiesList.Items))
@@ -230,7 +237,7 @@ func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies(freq uint
 
 			for i := range policiesList.Items {
 				policy := policiesList.Items[i]
-				if !shouldEvaluatePolicy(&policy) {
+				if !shouldEvaluatePolicy(&policy, cleanupImmediately) {
 					continue
 				}
 
@@ -317,10 +324,18 @@ func (r *ConfigurationPolicyReconciler) refreshDiscoveryInfo() error {
 // shouldEvaluatePolicy will determine if the policy is ready for evaluation by examining the
 // status.lastEvaluated and status.lastEvaluatedGeneration fields. If a policy has been updated, it
 // will always be triggered for evaluation. If the spec.evaluationInterval configuration has been
-// met, then that will also trigger an evaluation.
-func shouldEvaluatePolicy(policy *policyv1.ConfigurationPolicy) bool {
+// met, then that will also trigger an evaluation. If cleanupImmediately is true, then only policies
+// with finalizers will be ready for evaluation regardless of the last evaluation.
+// cleanupImmediately should be set true when the controller is getting uninstalled.
+func shouldEvaluatePolicy(policy *policyv1.ConfigurationPolicy, cleanupImmediately bool) bool {
 	log := log.WithValues("policy", policy.GetName())
 
+	// If it's time to clean up such as when the config-policy-controller is being uninstalled, only evaluate policies
+	// with a finalizer to remove the finalizer.
+	if cleanupImmediately {
+		return len(policy.Finalizers) != 0
+	}
+
 	if policy.ObjectMeta.DeletionTimestamp != nil {
 		log.V(2).Info("The policy has been deleted and is waiting for object cleanup. Will evaluate it now.")
 

diff --git a/controllers/configurationpolicy_controller_test.go b/controllers/configurationpolicy_controller_test.go
@@ -550,6 +550,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 		complianceState         policyv1.ComplianceState
 		expected                bool
 		deletionTimestamp       *metav1.Time
+		cleanupImmediately      bool
+		finalizers              []string
 	}{
 		{
 			"Just evaluated and the generation is unchanged",
@@ -559,6 +561,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.Compliant,
 			false,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"The generation has changed",
@@ -568,6 +572,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.Compliant,
 			true,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"lastEvaluated not set",
@@ -577,6 +583,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.Compliant,
 			true,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Invalid lastEvaluated",
@@ -586,6 +594,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.Compliant,
 			true,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Unknown compliance state",
@@ -595,6 +605,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.UnknownCompliancy,
 			true,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Default evaluation interval with a past lastEvaluated when compliant",
@@ -604,6 +616,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.Compliant,
 			true,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Default evaluation interval with a past lastEvaluated when noncompliant",
@@ -613,6 +627,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.NonCompliant,
 			true,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Never evaluation interval with past lastEvaluated when compliant",
@@ -622,6 +638,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.Compliant,
 			false,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Never evaluation interval with past lastEvaluated when noncompliant",
@@ -631,6 +649,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.NonCompliant,
 			false,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Invalid evaluation interval when compliant",
@@ -640,6 +660,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.Compliant,
 			true,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Invalid evaluation interval when noncompliant",
@@ -649,6 +671,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.NonCompliant,
 			true,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Custom evaluation interval that hasn't past yet when compliant",
@@ -658,6 +682,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.Compliant,
 			false,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Custom evaluation interval that hasn't past yet when noncompliant",
@@ -667,6 +693,8 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.NonCompliant,
 			false,
 			nil,
+			false,
+			[]string{},
 		},
 		{
 			"Deletion timestamp is non nil",
@@ -676,6 +704,30 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 			policyv1.NonCompliant,
 			true,
 			&metav1.Time{Time: time.Now()},
+			false,
+			[]string{},
+		},
+		{
+			"Finalizer and the controller is being deleted",
+			time.Now().UTC().Add(-13 * time.Hour).Format(time.RFC3339),
+			2,
+			policyv1.EvaluationInterval{NonCompliant: "12h"},
+			policyv1.NonCompliant,
+			true,
+			&metav1.Time{Time: time.Now()},
+			true,
+			[]string{pruneObjectFinalizer},
+		},
+		{
+			"No finalizer and the controller is being deleted",
+			time.Now().UTC().Add(-13 * time.Hour).Format(time.RFC3339),
+			2,
+			policyv1.EvaluationInterval{NonCompliant: "12h"},
+			policyv1.NonCompliant,
+			false,
+			&metav1.Time{Time: time.Now()},
+			true,
+			[]string{},
 		},
 	}
 
@@ -693,8 +745,9 @@ func TestShouldEvaluatePolicy(t *testing.T) {
 				policy.Spec.EvaluationInterval = test.evaluationInterval
 				policy.Status.ComplianceState = test.complianceState
 				policy.ObjectMeta.DeletionTimestamp = test.deletionTimestamp
+				policy.ObjectMeta.Finalizers = test.finalizers
 
-				if actual := shouldEvaluatePolicy(policy); actual != test.expected {
+				if actual := shouldEvaluatePolicy(policy, test.cleanupImmediately); actual != test.expected {
 					t.Fatalf("expected %v but got %v", test.expected, actual)
 				}
 			},