Improve OperatorGroup removal logic

In some clusters, a default OperatorGroup might be provided in a namespace for "global" operators. This OperatorGroup was not being correctly identified by the policy: in fact any pre-existing group that did not match the group specified by the policy, or the one that would be generated when the policy does not specify a group, was not reported correctly. Now, pre-existing "special" operator groups should be reported and handled correctly. If they are owned by another resource, they will be considered as "used" for the purposes of the DeleteIfUnused setting. Refs: - https://issues.redhat.com/browse/ACM-11022 - https://issues.redhat.com/browse/ACM-11077 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
open-cluster-management-io · Apr 22, 2024 · a90a129 · a90a129
1 parent 72f27a1
commit a90a129
Show file tree

Hide file tree

Showing 3 changed files with 260 additions and 51 deletions.
diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go
@@ -725,63 +725,127 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
 		return nil, changed, nil
 	}
 
-	var foundOpGroupName string
-	var foundOpGroup *unstructured.Unstructured
+	removalBehavior := policy.Spec.RemovalBehavior.ApplyDefaults()
+	keep := removalBehavior.OperatorGroups.IsKeep()
 
-	for _, opGroup := range foundOpGroups {
-		emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName
+	// if DeleteIfUnused and there are other subscriptions, then keep the operator group
+	if removalBehavior.OperatorGroups.IsDeleteIfUnused() {
+		// Check the namespace for any subscriptions, including the sub for this mustnothave policy,
+		// since deleting the OperatorGroup before that could cause problems
+		watcher := opPolIdentifier(policy.Namespace, policy.Name)
 
-		if opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch {
-			foundOpGroupName = opGroup.GetName()
-			foundOpGroup = opGroup.DeepCopy()
+		foundSubscriptions, err := r.DynamicWatcher.List(
+			watcher, subscriptionGVK, desiredOpGroup.Namespace, labels.Everything())
+		if err != nil {
+			return nil, false, fmt.Errorf("error listing Subscriptions: %w", err)
+		}
 
-			break
+		if len(foundSubscriptions) != 0 {
+			keep = true
 		}
 	}
 
-	if foundOpGroupName == "" {
-		// no found OperatorGroup matches what the policy is looking for, report Compliance.
-		changed := updateStatus(policy, missingNotWantedCond("OperatorGroup"), missingNotWantedObj(desiredOpGroup))
+	if keep {
+		leftovers := make([]policyv1.RelatedObject, 0, len(foundOpGroups))
 
-		return nil, changed, nil
+		for _, opGroup := range foundOpGroups {
+			opGroup := opGroup
+
+			leftovers = append(leftovers, leftoverObj(&opGroup))
+		}
+
+		return nil, updateStatus(policy, keptCond("OperatorGroup"), leftovers...), nil
 	}
 
-	desiredOpGroup.SetName(foundOpGroupName)
+	// If an OperatorGroup is specified in the policy, only consider that one
+	if policy.Spec.OperatorGroup != nil {
+		var foundOpGroupName string
+		var foundOpGroup *unstructured.Unstructured
 
-	removalBehavior := policy.Spec.RemovalBehavior.ApplyDefaults()
+		for _, opGroup := range foundOpGroups {
+			emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName
 
-	if removalBehavior.OperatorGroups.IsKeep() {
-		changed := updateStatus(policy, keptCond("OperatorGroup"), leftoverObj(desiredOpGroup))
+			if opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch {
+				foundOpGroupName = opGroup.GetName()
+				foundOpGroup = opGroup.DeepCopy()
 
-		return nil, changed, nil
-	}
+				break
+			}
+		}
 
-	// The found OperatorGroup matches what is *not* wanted by the policy. Report NonCompliance.
-	changed := updateStatus(policy, foundNotWantedCond("OperatorGroup"), foundNotWantedObj(desiredOpGroup))
+		if foundOpGroupName == "" {
+			// no found OperatorGroup matches what the policy is looking for, report Compliance.
+			changed := updateStatus(policy, missingNotWantedCond("OperatorGroup"), missingNotWantedObj(desiredOpGroup))
 
-	if policy.Spec.RemediationAction.IsInform() {
-		return nil, changed, nil
-	}
+			return nil, changed, nil
+		}
 
-	if removalBehavior.OperatorGroups.IsDeleteIfUnused() {
-		// Check the namespace for any subscriptions, including the sub for this mustnothave policy,
-		// since deleting the OperatorGroup before that could cause problems
-		watcher := opPolIdentifier(policy.Namespace, policy.Name)
+		desiredOpGroup.SetName(foundOpGroupName)
 
-		foundSubscriptions, err := r.DynamicWatcher.List(
-			watcher, subscriptionGVK, desiredOpGroup.Namespace, labels.Everything())
-		if err != nil {
-			return nil, false, fmt.Errorf("error listing Subscriptions: %w", err)
+		if removalBehavior.OperatorGroups.IsDeleteIfUnused() && len(foundOpGroup.GetOwnerReferences()) != 0 {
+			// the OperatorGroup specified in the policy might be used or managed by something else
+			// so we will keep it.
+			return nil, updateStatus(policy, keptCond("OperatorGroup"), leftoverObj(desiredOpGroup)), nil
 		}
 
-		if len(foundSubscriptions) != 0 {
+		// The found OperatorGroup matches what is *not* wanted by the policy. Report NonCompliance.
+		changed := updateStatus(policy, foundNotWantedCond("OperatorGroup"), foundNotWantedObj(desiredOpGroup))
+
+		if policy.Spec.RemediationAction.IsInform() {
 			return nil, changed, nil
 		}
+
+		if foundOpGroup.GetDeletionTimestamp() != nil {
+			// No "early" condition because that would cause the status to flap
+			return nil, updateStatus(policy, deletingCond("OperatorGroup"), deletingObj(desiredOpGroup)), nil
+		}
+
+		earlyConds := []metav1.Condition{}
+
+		if changed {
+			earlyConds = append(earlyConds, calculateComplianceCondition(policy))
+		}
+
+		err := r.Delete(ctx, desiredOpGroup)
+		if err != nil {
+			return earlyConds, changed, fmt.Errorf("error deleting the OperatorGroup: %w", err)
+		}
+
+		desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Delete stripped this information
+
+		updateStatus(policy, deletedCond("OperatorGroup"), deletedObj(desiredOpGroup))
+
+		return earlyConds, true, nil
 	}
 
-	if foundOpGroup.GetDeletionTimestamp() != nil {
-		// No "early" condition because that would cause the status to flap
-		return nil, updateStatus(policy, deletingCond("OperatorGroup"), deletingObj(desiredOpGroup)), nil
+	// The policy does not specify the OperatorGroup: we must consider each of them for deletion
+	relatedOpGroups := make([]policyv1.RelatedObject, 0, len(foundOpGroups))
+	leftovers := make([]policyv1.RelatedObject, 0)
+	toDelete := make([]unstructured.Unstructured, 0)
+
+	for _, opGroup := range foundOpGroups {
+		opGroup := opGroup
+
+		if removalBehavior.OperatorGroups.IsDeleteIfUnused() && len(opGroup.GetOwnerReferences()) != 0 {
+			leftover := leftoverObj(&opGroup)
+			relatedOpGroups = append(relatedOpGroups, leftover)
+			leftovers = append(leftovers, leftover)
+		} else {
+			relatedOpGroups = append(relatedOpGroups, foundNotWantedObj(&opGroup))
+			toDelete = append(toDelete, opGroup)
+		}
+	}
+
+	condition := keptCond("OperatorGroup")
+
+	if len(toDelete) != 0 {
+		condition = foundNotWantedCond("OperatorGroup")
+	}
+
+	changed := updateStatus(policy, condition, relatedOpGroups...)
+
+	if policy.Spec.RemediationAction.IsInform() || len(toDelete) == 0 {
+		return nil, changed, nil
 	}
 
 	earlyConds := []metav1.Condition{}
@@ -790,16 +854,46 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
 		earlyConds = append(earlyConds, calculateComplianceCondition(policy))
 	}
 
-	err := r.Delete(ctx, desiredOpGroup)
-	if err != nil {
-		return earlyConds, changed, fmt.Errorf("error deleting the OperatorGroup: %w", err)
+	relatedOpGroups = leftovers // append the deleting and deleted ones to this
+	anyAlreadyDeleting := false
+	deletionErrs := make([]string, 0)
+
+	for _, opGroupToDelete := range toDelete {
+		opGroupToDelete := opGroupToDelete
+
+		if opGroupToDelete.GetDeletionTimestamp() != nil {
+			anyAlreadyDeleting = true
+
+			relatedOpGroups = append(relatedOpGroups, deletingObj(&opGroupToDelete))
+
+			continue
+		}
+
+		err := r.Delete(ctx, &opGroupToDelete)
+		if err != nil {
+			deletionErrs = append(deletionErrs, err.Error())
+			relatedOpGroups = append(relatedOpGroups, foundNotWantedObj(&opGroupToDelete))
+
+			continue
+		}
+
+		opGroupToDelete.SetGroupVersionKind(operatorGroupGVK) // Delete might have stripped this
+		relatedOpGroups = append(relatedOpGroups, deletedObj(&opGroupToDelete))
 	}
 
-	desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Delete stripped this information
+	var deletionErr error
+	if len(deletionErrs) != 0 {
+		deletionErr = fmt.Errorf("error deleting an OperatorGroup: %v", deletionErrs)
+	}
 
-	updateStatus(policy, deletedCond("OperatorGroup"), deletedObj(desiredOpGroup))
+	if anyAlreadyDeleting {
+		// don't send the "early" conditions to avoid flapping.
+		return nil, updateStatus(policy, deletingCond("OperatorGroup"), relatedOpGroups...), deletionErr
+	}
 
-	return earlyConds, true, nil
+	updateStatus(policy, deletedCond("OperatorGroup"), relatedOpGroups...)
+
+	return earlyConds, true, deletionErr
 }
 
 func (r *OperatorPolicyReconciler) handleSubscription(

diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go
@@ -850,6 +850,8 @@ func deletedObj(obj client.Object) policyv1.RelatedObject {
 	}
 }
 
+// deletingObj returns a NonCompliant RelatedObject with
+// reason = 'The object is being deleted but has not been removed yet'
 func deletingObj(obj client.Object) policyv1.RelatedObject {
 	return policyv1.RelatedObject{
 		Object:    policyv1.ObjectResourceFromObj(obj),