Improve OperatorGroup removal logic

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