diff --git a/api/v1beta1/operatorpolicy_types.go b/api/v1beta1/operatorpolicy_types.go index 89fd69ca..1819ef5d 100644 --- a/api/v1beta1/operatorpolicy_types.go +++ b/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 diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index fd248aeb..805acd47 100644 --- a/controllers/operatorpolicy_controller.go +++ b/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 diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index de222a85..ce0e2b31 100644 --- 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), diff --git a/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml b/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml index b293df9f..93f851e7 100644 --- a/deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml +++ b/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: diff --git a/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml b/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml index 30a22aba..9096dbf8 100644 --- a/deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml +++ b/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: diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 61fa106b..96cf0ad6 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/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() { diff --git a/test/resources/case38_operator_install/operator-policy-mustnothave.yaml b/test/resources/case38_operator_install/operator-policy-mustnothave.yaml index 4b994677..97db9aa0 100644 --- a/test/resources/case38_operator_install/operator-policy-mustnothave.yaml +++ b/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