🤖 Sync from open-cluster-management-io/config-policy-controller: #100

build/common/config/.golangci.yml

@@ -29,6 +29,9 @@ linters:
   enable-all: true
   disable:
     - bodyclose
+    # Disable contextcheck since we don't want to pass the context passed to PeriodicallyExecConfigPolicies to functions
+    # called within it or else cleanup won't occur
+    - contextcheck
     - cyclop
     - depguard
     - dupl

controllers/configurationpolicy_controller.go

@@ -151,15 +151,32 @@ func (r *ConfigurationPolicyReconciler) Reconcile(ctx context.Context, request c
 
 // PeriodicallyExecConfigPolicies loops through all configurationpolicies in the target namespace and triggers
 // template handling for each one. This function drives all the work the configuration policy controller does.
-func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies(freq uint, elected <-chan struct{}, test bool) {
+func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies(
+	ctx context.Context, freq uint, elected <-chan struct{},
+) {
 	log.Info("Waiting for leader election before periodically evaluating configuration policies")
-	<-elected
+
+	select {
+	case <-elected:
+	case <-ctx.Done():
+		return
+	}
 
 	const waiting = 10 * time.Minute
+	var exiting bool
+	// Loop twice after exit condition is received to account for race conditions and retries.
+	loopsAfterExit := 2
 
-	for {
+	for !exiting || (exiting && loopsAfterExit > 0) {
 		start := time.Now()
 
+		select {
+		case <-ctx.Done():
+			exiting = true
+			loopsAfterExit--
+		default:
+		}
+
 		policiesList := policyv1.ConfigurationPolicyList{}
 
 		var skipLoop bool
@@ -211,6 +228,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 +254,7 @@ func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies(freq uint
 
 			for i := range policiesList.Items {
 				policy := policiesList.Items[i]
-				if !shouldEvaluatePolicy(&policy) {
+				if !shouldEvaluatePolicy(&policy, cleanupImmediately) {
 					continue
 				}
 
@@ -257,10 +281,6 @@ func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies(freq uint
 			log.V(2).Info("Sleeping before reprocessing the configuration policies", "seconds", sleepTime)
 			time.Sleep(sleepTime)
 		}
-
-		if test {
-			return
-		}
 	}
 }
 
@@ -317,10 +337,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.")
 

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)
 				}
 			},

main.go

@@ -320,10 +320,16 @@ func main() {
 		os.Exit(1)
 	}
 
+	terminatingCtx := ctrl.SetupSignalHandler()
+	managerCtx, managerCancel := context.WithCancel(context.Background())
+
 	// PeriodicallyExecConfigPolicies is the go-routine that periodically checks the policies
 	log.V(1).Info("Perodically processing Configuration Policies", "frequency", frequency)
 
-	go reconciler.PeriodicallyExecConfigPolicies(frequency, mgr.Elected(), false)
+	go func() {
+		reconciler.PeriodicallyExecConfigPolicies(terminatingCtx, frequency, mgr.Elected())
+		managerCancel()
+	}()
 
 	// This lease is not related to leader election. This is to report the status of the controller
 	// to the addon framework. This can be seen in the "status" section of the ManagedClusterAddOn
@@ -361,7 +367,7 @@ func main() {
 
 	log.Info("Starting manager")
 
-	if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
+	if err := mgr.Start(managerCtx); err != nil {
 		log.Error(err, "Problem running manager")
 		os.Exit(1)
 	}