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.

This also removes the "regular" `Delete` option for OperatorGroups: that
was too likely to cause confusion.

Refs:
 - https://issues.redhat.com/browse/ACM-11022
 - https://issues.redhat.com/browse/ACM-11077

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

api/v1beta1/operatorpolicy_types.go

@@ -51,9 +51,9 @@ func (ra RemovalAction) IsDeleteIfUnused() bool {
 
 type RemovalBehavior struct {
 	//+kubebuilder:default=DeleteIfUnused
-	//+kubebuilder:validation:Enum=Keep;Delete;DeleteIfUnused
+	//+kubebuilder:validation:Enum=Keep;DeleteIfUnused
 	// Specifies whether to delete the OperatorGroup; defaults to 'DeleteIfUnused' which
-	// will only delete the OperatorGroup if there is not another Subscription using it.
+	// will only delete the OperatorGroup if there is not another resource using it.
 	OperatorGroups RemovalAction `json:"operatorGroups,omitempty"`
 
 	//+kubebuilder:default=Delete

controllers/operatorpolicy_controller.go

@@ -226,7 +226,12 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
 		return earlyComplianceEvents, condChanged, err
 	}
 
-	earlyConds, changed, err := r.handleOpGroup(ctx, policy, desiredOG)
+	desiredSubName := ""
+	if desiredSub != nil {
+		desiredSubName = desiredSub.Name
+	}
+
+	earlyConds, changed, err := r.handleOpGroup(ctx, policy, desiredOG, desiredSubName)
 	earlyComplianceEvents = append(earlyComplianceEvents, earlyConds...)
 	condChanged = condChanged || changed
 
@@ -567,7 +572,10 @@ func buildOperatorGroup(
 }
 
 func (r *OperatorPolicyReconciler) handleOpGroup(
-	ctx context.Context, policy *policyv1beta1.OperatorPolicy, desiredOpGroup *operatorv1.OperatorGroup,
+	ctx context.Context,
+	policy *policyv1beta1.OperatorPolicy,
+	desiredOpGroup *operatorv1.OperatorGroup,
+	desiredSubName string,
 ) ([]metav1.Condition, bool, error) {
 	watcher := opPolIdentifier(policy.Namespace, policy.Name)
 
@@ -586,7 +594,7 @@ func (r *OperatorPolicyReconciler) handleOpGroup(
 		return r.musthaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups)
 	}
 
-	return r.mustnothaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups)
+	return r.mustnothaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups, desiredSubName)
 }
 
 func (r *OperatorPolicyReconciler) musthaveOpGroup(
@@ -716,44 +724,72 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
 	ctx context.Context,
 	policy *policyv1beta1.OperatorPolicy,
 	desiredOpGroup *operatorv1.OperatorGroup,
-	foundOpGroups []unstructured.Unstructured,
+	allFoundOpGroups []unstructured.Unstructured,
+	desiredSubName string,
 ) ([]metav1.Condition, bool, error) {
-	if len(foundOpGroups) == 0 {
+	if len(allFoundOpGroups) == 0 {
 		// Missing OperatorGroup: report Compliance
 		changed := updateStatus(policy, missingNotWantedCond("OperatorGroup"), missingNotWantedObj(desiredOpGroup))
 
 		return nil, changed, nil
 	}
 
-	var foundOpGroupName string
-	var foundOpGroup *unstructured.Unstructured
+	if len(allFoundOpGroups) > 1 {
+		// Don't try to choose one, just report this as NonCompliant.
+		return nil, updateStatus(policy, opGroupTooManyCond, opGroupTooManyObjs(allFoundOpGroups)...), nil
+	}
 
-	for _, opGroup := range foundOpGroups {
-		emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName
+	foundOpGroup := allFoundOpGroups[0]
 
-		if opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch {
-			foundOpGroupName = opGroup.GetName()
-			foundOpGroup = opGroup.DeepCopy()
+	removalBehavior := policy.Spec.RemovalBehavior.ApplyDefaults()
+	keep := removalBehavior.OperatorGroups.IsKeep()
 
-			break
+	// 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)
+
+		foundSubscriptions, err := r.DynamicWatcher.List(
+			watcher, subscriptionGVK, desiredOpGroup.Namespace, labels.Everything())
+		if err != nil {
+			return nil, false, fmt.Errorf("error listing Subscriptions: %w", err)
+		}
+
+		anotherSubFound := false
+
+		for _, sub := range foundSubscriptions {
+			if sub.GetName() != desiredSubName {
+				anotherSubFound = true
+
+				break
+			}
 		}
+
+		if anotherSubFound {
+			keep = true
+		}
+	}
+
+	if keep {
+		return nil, updateStatus(policy, keptCond("OperatorGroup"), leftoverObj(&foundOpGroup)), nil
 	}
 
-	if foundOpGroupName == "" {
+	emptyNameMatch := desiredOpGroup.Name == "" && foundOpGroup.GetGenerateName() == desiredOpGroup.GenerateName
+
+	if !(foundOpGroup.GetName() == desiredOpGroup.Name || emptyNameMatch) {
 		// no found OperatorGroup matches what the policy is looking for, report Compliance.
 		changed := updateStatus(policy, missingNotWantedCond("OperatorGroup"), missingNotWantedObj(desiredOpGroup))
 
 		return nil, changed, nil
 	}
 
-	desiredOpGroup.SetName(foundOpGroupName)
-
-	removalBehavior := policy.Spec.RemovalBehavior.ApplyDefaults()
-
-	if removalBehavior.OperatorGroups.IsKeep() {
-		changed := updateStatus(policy, keptCond("OperatorGroup"), leftoverObj(desiredOpGroup))
+	desiredOpGroup.SetName(foundOpGroup.GetName()) // set it for the generateName case
 
-		return nil, changed, nil
+	if 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
 	}
 
 	// The found OperatorGroup matches what is *not* wanted by the policy. Report NonCompliance.
@@ -763,22 +799,6 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
 		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)
-
-		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 len(foundSubscriptions) != 0 {
-			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

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

deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml

@@ -105,10 +105,9 @@ spec:
                     default: DeleteIfUnused
                     description: |-
                       Specifies whether to delete the OperatorGroup; defaults to 'DeleteIfUnused' which
-                      will only delete the OperatorGroup if there is not another Subscription using it.
+                      will only delete the OperatorGroup if there is not another resource using it.
                     enum:
                     - Keep
-                    - Delete
                     - DeleteIfUnused
                     type: string
                   subscriptions:

deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml

@@ -100,10 +100,9 @@ spec:
                     default: DeleteIfUnused
                     description: |-
                       Specifies whether to delete the OperatorGroup; defaults to 'DeleteIfUnused' which
-                      will only delete the OperatorGroup if there is not another Subscription using it.
+                      will only delete the OperatorGroup if there is not another resource using it.
                     enum:
                     - Keep
-                    - Delete
                     - DeleteIfUnused
                     type: string
                   subscriptions:

test/e2e/case38_install_operator_test.go

@@ -1295,8 +1295,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 	})
 	Describe("Testing CustomResourceDefinition reporting", Ordered, func() {
 		const (
-			opPolYAML = "../resources/case38_operator_install/operator-policy-authorino.yaml"
-			opPolName = "oppol-authorino"
+			opPolYAML = "../resources/case38_operator_install/operator-policy-no-group.yaml"
+			opPolName = "oppol-no-group"
 		)
 		BeforeAll(func() {
 			utils.Kubectl("delete", "crd", "--selector=olm.managed=true")
@@ -1340,7 +1340,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 			By("Waiting for a CRD to appear, which should indicate the operator is installing")
 			Eventually(func(ctx SpecContext) *unstructured.Unstructured {
 				crd, _ := clientManagedDynamic.Resource(gvrCRD).Get(ctx,
-					"authconfigs.authorino.kuadrant.io", metav1.GetOptions{})
+					"quayregistries.quay.redhat.com", metav1.GetOptions{})
 
 				return crd
 			}, olmWaitTimeout, 5, ctx).ShouldNot(BeNil())
@@ -1364,7 +1364,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 						Kind:       "CustomResourceDefinition",
 						APIVersion: "apiextensions.k8s.io/v1",
 						Metadata: policyv1.ObjectMetadata{
-							Name: "authconfigs.authorino.kuadrant.io",
+							Name: "quayregistries.quay.redhat.com",
 						},
 					},
 					Compliant: "Compliant",
@@ -1374,7 +1374,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 						Kind:       "CustomResourceDefinition",
 						APIVersion: "apiextensions.k8s.io/v1",
 						Metadata: policyv1.ObjectMetadata{
-							Name: "authorinos.operator.authorino.kuadrant.io",
+							Name: "quayecosystems.redhatcop.redhat.io",
 						},
 					},
 					Compliant: "Compliant",
@@ -2048,7 +2048,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 
 			By("Setting the removal behaviors to Delete")
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
-				`[{"op": "replace", "path": "/spec/removalBehavior/operatorGroups", "value": "Delete"},`+
+				`[{"op": "replace", "path": "/spec/removalBehavior/operatorGroups", "value": "DeleteIfUnused"},`+
 					`{"op": "replace", "path": "/spec/removalBehavior/subscriptions", "value": "Delete"},`+
 					`{"op": "replace", "path": "/spec/removalBehavior/clusterServiceVersions", "value": "Delete"},`+
 					`{"op": "replace", "path": "/spec/removalBehavior/installPlans", "value": "Delete"},`+
@@ -2378,7 +2378,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 				opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy)
 		})
 
-		It("should delete the operator group when there is only one subscription", func(ctx SpecContext) {
+		It("should delete the inferred operator group when there is only one subscription", func(ctx SpecContext) {
 			// enforce it as a musthave in order to install the operator
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
 				`[{"op": "replace", "path": "/spec/complianceType", "value": "musthave"},`+
@@ -2427,7 +2427,121 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 			}, eventuallyTimeout, 3, ctx).Should(BeEmpty())
 		})
 
-		It("should not delete the operator group when there is another subscription", func(ctx SpecContext) {
+		It("should delete the specified operator group when there is only one subscription", func(ctx SpecContext) {
+			// enforce it as a musthave in order to install the operator
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/complianceType", "value": "musthave"},`+
+					`{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"},`+
+					`{"op": "replace", "path": "/spec/removalBehavior/operatorGroups", "value": "DeleteIfUnused"},`+
+					`{"op": "add", "path": "/spec/operatorGroup", "value": {"name": "scoped-operator-group", `+
+					`"namespace": "operator-policy-testns", "targetNamespaces": ["operator-policy-testns"]}}]`)
+
+			By("Waiting for a CRD to appear, which should indicate the operator is installing.")
+			Eventually(func(ctx SpecContext) *unstructured.Unstructured {
+				crd, _ := clientManagedDynamic.Resource(gvrCRD).Get(ctx,
+					"quayregistries.quay.redhat.com", metav1.GetOptions{})
+
+				return crd
+			}, olmWaitTimeout, 5, ctx).ShouldNot(BeNil())
+
+			By("Waiting for the policy to become compliant, indicating the operator is installed")
+			Eventually(func(g Gomega) string {
+				pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName,
+					opPolTestNS, true, eventuallyTimeout)
+				compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+
+				return compliance
+			}, olmWaitTimeout, 5, ctx).Should(Equal("Compliant"))
+
+			By("Verifying that an operator group exists")
+			Eventually(func(g Gomega) []unstructured.Unstructured {
+				list, err := clientManagedDynamic.Resource(gvrOperatorGroup).Namespace(opPolTestNS).
+					List(ctx, metav1.ListOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+
+				return list.Items
+			}, eventuallyTimeout, 3, ctx).ShouldNot(BeEmpty())
+
+			// revert it to mustnothave
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/complianceType", "value": "mustnothave"}]`)
+
+			By("Verifying that the operator group was removed")
+			Eventually(func(g Gomega) []unstructured.Unstructured {
+				list, err := clientManagedDynamic.Resource(gvrOperatorGroup).Namespace(opPolTestNS).
+					List(ctx, metav1.ListOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+
+				return list.Items
+			}, eventuallyTimeout, 3, ctx).Should(BeEmpty())
+		})
+
+		It("should keep the specified operator group when it is owned by something", func(ctx SpecContext) {
+			// enforce it as a musthave in order to install the operator
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/complianceType", "value": "musthave"},`+
+					`{"op": "replace", "path": "/spec/remediationAction", "value": "enforce"},`+
+					`{"op": "replace", "path": "/spec/removalBehavior/operatorGroups", "value": "DeleteIfUnused"},`+
+					`{"op": "add", "path": "/spec/operatorGroup", "value": {"name": "scoped-operator-group", `+
+					`"namespace": "operator-policy-testns", "targetNamespaces": ["operator-policy-testns"]}}]`)
+
+			By("Waiting for a CRD to appear, which should indicate the operator is installing.")
+			Eventually(func(ctx SpecContext) *unstructured.Unstructured {
+				crd, _ := clientManagedDynamic.Resource(gvrCRD).Get(ctx,
+					"quayregistries.quay.redhat.com", metav1.GetOptions{})
+
+				return crd
+			}, olmWaitTimeout, 5, ctx).ShouldNot(BeNil())
+
+			By("Waiting for the policy to become compliant, indicating the operator is installed")
+			Eventually(func(g Gomega) string {
+				pol := utils.GetWithTimeout(clientManagedDynamic, gvrOperatorPolicy, opPolName,
+					opPolTestNS, true, eventuallyTimeout)
+				compliance, found, err := unstructured.NestedString(pol.Object, "status", "compliant")
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(found).To(BeTrue())
+
+				return compliance
+			}, olmWaitTimeout, 5, ctx).Should(Equal("Compliant"))
+
+			By("Verifying that an operator group exists")
+			Eventually(func(g Gomega) []unstructured.Unstructured {
+				list, err := clientManagedDynamic.Resource(gvrOperatorGroup).Namespace(opPolTestNS).
+					List(ctx, metav1.ListOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+
+				return list.Items
+			}, eventuallyTimeout, 3, ctx).ShouldNot(BeEmpty())
+
+			By("Creating and setting an owner for the operator group")
+			utils.Kubectl("create", "configmap", "ownercm", "-n", opPolTestNS, "--from-literal=foo=bar")
+
+			ownerCM := utils.GetWithTimeout(clientManagedDynamic, gvrConfigMap, "ownercm",
+				opPolTestNS, true, eventuallyTimeout)
+			ownerUID := string(ownerCM.GetUID())
+			Expect(ownerUID).NotTo(BeEmpty())
+
+			utils.Kubectl("patch", "operatorgroup", "scoped-operator-group", "-n", opPolTestNS, "--type=json", "-p",
+				`[{"op": "add", "path": "/metadata/ownerReferences", "value": [{"apiVersion": "v1",
+				"kind": "ConfigMap", "name": "ownercm", "uid": "`+ownerUID+`"}]}]`)
+
+			// revert it to mustnothave
+			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
+				`[{"op": "replace", "path": "/spec/complianceType", "value": "mustnothave"}]`)
+
+			By("Verifying the operator group was not removed")
+			Consistently(func(g Gomega) []unstructured.Unstructured {
+				list, err := clientManagedDynamic.Resource(gvrOperatorGroup).Namespace(opPolTestNS).
+					List(ctx, metav1.ListOptions{})
+				g.Expect(err).NotTo(HaveOccurred())
+
+				return list.Items
+			}, consistentlyDuration, 3, ctx).ShouldNot(BeEmpty())
+		})
+
+		It("should not delete the inferred operator group when there is another subscription", func(ctx SpecContext) {
 			// enforce it as a musthave in order to install the operator
 			utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
 				`[{"op": "replace", "path": "/spec/complianceType", "value": "musthave"},`+
@@ -2494,9 +2608,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
 	})
 	Describe("Testing defaulted values in an OperatorPolicy", func() {
 		const (
-			opPolYAML = "../resources/case38_operator_install/operator-policy-authorino.yaml"
-			opPolName = "oppol-authorino"
-			subName   = "authorino-operator"
+			opPolYAML = "../resources/case38_operator_install/operator-policy-no-group.yaml"
+			opPolName = "oppol-no-group"
 		)
 
 		BeforeEach(func() {

test/resources/case38_operator_install/operator-policy-mustnothave.yaml

@@ -25,7 +25,7 @@ spec:
   versions:
     - quay-operator.v3.8.13
   removalBehavior:
-    operatorGroups: Delete
+    operatorGroups: DeleteIfUnused
     subscriptions: Delete
     clusterServiceVersions: Delete
     installPlans: Delete