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