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>

controllers/operatorpolicy_controller.go

@@ -725,63 +725,126 @@ 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))
+		for _, opGroup := range foundOpGroups {
+			opGroup := opGroup
 
-		return nil, changed, nil
+			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
+	}
+
+	// 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)
+		}
 	}
 
-	if foundOpGroup.GetDeletionTimestamp() != nil {
-		// No "early" condition because that would cause the status to flap
-		return nil, updateStatus(policy, deletingCond("OperatorGroup"), deletingObj(desiredOpGroup)), nil
+	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 +853,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(

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