From 739f1ccb1a8b6ade4908ff182687f776ab162120 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Tue, 23 Apr 2024 16:57:35 -0400 Subject: [PATCH 1/4] 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 (cherry picked from commit 21143ac833caa3da78b5ae90dfc85b342719f6f0) --- api/v1beta1/operatorpolicy_types.go | 4 +- controllers/operatorpolicy_controller.go | 94 +++++++----- controllers/operatorpolicy_status.go | 2 + ...luster-management.io_operatorpolicies.yaml | 3 +- ...luster-management.io_operatorpolicies.yaml | 3 +- test/e2e/case38_install_operator_test.go | 135 ++++++++++++++++-- .../operator-policy-mustnothave.yaml | 2 +- 7 files changed, 188 insertions(+), 55 deletions(-) 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 From e96a20120b7b00475c55b2fded2a935e319122f6 Mon Sep 17 00:00:00 2001 From: mprahl Date: Mon, 22 Apr 2024 12:44:43 -0400 Subject: [PATCH 2/4] Make the operator policy message case consistent Relates: https://issues.redhat.com/browse/ACM-11026 Signed-off-by: mprahl (cherry picked from commit 6790eff651cbf6f88bb117fdea524d9604149eff) --- controllers/operatorpolicy_status.go | 10 +++++----- test/e2e/case38_install_operator_test.go | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index ce0e2b31..aa1b72d4 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -696,7 +696,7 @@ var noCSVCond = metav1.Condition{ Type: csvConditionType, Status: metav1.ConditionFalse, Reason: "RelevantCSVNotFound", - Message: "A relevant installed ClusterServiceVersion could not be found", + Message: "a relevant installed ClusterServiceVersion could not be found", } // noCRDCond is a Compliant condition for when no CRDs are found @@ -704,7 +704,7 @@ var noCRDCond = metav1.Condition{ Type: crdConditionType, Status: metav1.ConditionTrue, Reason: "RelevantCRDNotFound", - Message: "No CRDs were found for the operator", + Message: "no CRDs were found for the operator", } // crdFoundCond is a Compliant condition for when CRDs are found @@ -712,7 +712,7 @@ var crdFoundCond = metav1.Condition{ Type: crdConditionType, Status: metav1.ConditionTrue, Reason: "RelevantCRDFound", - Message: "There are CRDs present for the operator", + Message: "there are CRDs present for the operator", } // buildDeploymentCond creates a Condition for deployments. If any are not at their @@ -758,7 +758,7 @@ var noDeploymentsCond = metav1.Condition{ Type: deploymentConditionType, Status: metav1.ConditionTrue, Reason: "NoRelevantDeployments", - Message: "The ClusterServiceVersion is missing, thus meaning there are no relevant deployments", + Message: "the ClusterServiceVersion is missing, thus meaning there are no relevant deployments", } // catalogSourceFindCond is a conditionally compliant condition with reason @@ -793,7 +793,7 @@ var catalogSourceUnknownCond = metav1.Condition{ Type: "CatalogSourcesUnknownState", Status: metav1.ConditionTrue, Reason: "LastObservedUnknown", - Message: "Could not determine last observed state of CatalogSource", + Message: "could not determine last observed state of CatalogSource", } // missingWantedObj returns a NonCompliant RelatedObject with reason = 'Resource not found but should exist' diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 96cf0ad6..e6eeedea 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -1328,9 +1328,9 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "CustomResourceDefinitionCompliant", Status: metav1.ConditionTrue, Reason: "RelevantCRDNotFound", - Message: "No CRDs were found for the operator", + Message: "no CRDs were found for the operator", }, - "No CRDs were found for the operator", + "no CRDs were found for the operator", ) }) @@ -1384,9 +1384,9 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "CustomResourceDefinitionCompliant", Status: metav1.ConditionTrue, Reason: "RelevantCRDFound", - Message: "There are CRDs present for the operator", + Message: "there are CRDs present for the operator", }, - "There are CRDs present for the operator", + "there are CRDs present for the operator", ) }) }) @@ -1667,9 +1667,9 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "CustomResourceDefinitionCompliant", Status: metav1.ConditionTrue, Reason: "RelevantCRDNotFound", - Message: "No CRDs were found for the operator", + Message: "no CRDs were found for the operator", }, - `No CRDs were found for the operator`, + `no CRDs were found for the operator`, ) check( opPolName, @@ -2295,7 +2295,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "CustomResourceDefinitionCompliant", Status: metav1.ConditionTrue, Reason: "RelevantCRDNotFound", - Message: "No CRDs were found for the operator", + Message: "no CRDs were found for the operator", }, `the CustomResourceDefinition was deleted`, ) From 7c301335909ea6fa33415315e9ab16b1e450c623 Mon Sep 17 00:00:00 2001 From: mprahl Date: Mon, 22 Apr 2024 12:47:42 -0400 Subject: [PATCH 3/4] Remove commas from operator policy messages This is to make it easier to delineate between each message section in the policy compliance message. Relates: https://issues.redhat.com/browse/ACM-11026 Signed-off-by: mprahl (cherry picked from commit 5fdf7393c2c2e6d12fdd90408564edea4e5230e3) --- controllers/operatorpolicy_status.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index aa1b72d4..d9a4a58e 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -758,7 +758,7 @@ var noDeploymentsCond = metav1.Condition{ Type: deploymentConditionType, Status: metav1.ConditionTrue, Reason: "NoRelevantDeployments", - Message: "the ClusterServiceVersion is missing, thus meaning there are no relevant deployments", + Message: "there are no relevant deployments because the ClusterServiceVersion is missing", } // catalogSourceFindCond is a conditionally compliant condition with reason From 9f22d3e50a1f859722a19004ae4f75549a33d985 Mon Sep 17 00:00:00 2001 From: mprahl Date: Tue, 23 Apr 2024 13:48:34 -0400 Subject: [PATCH 4/4] Include the CSV name in compliance messages Relates: https://issues.redhat.com/browse/ACM-11026 Signed-off-by: mprahl (cherry picked from commit 8206ac632f3e16604f119d79c54d9b99225fea51) --- controllers/operatorpolicy_controller.go | 11 +++--- controllers/operatorpolicy_status.go | 44 ++++++++++++++++++++---- test/e2e/case38_install_operator_test.go | 33 ++++++++++-------- 3 files changed, 63 insertions(+), 25 deletions(-) diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 805acd47..955dd1c5 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -1413,11 +1413,14 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV( return nil, updateStatus(policy, keptCond("ClusterServiceVersion"), relatedCSVs...), nil } + csvNames := make([]string, 0, len(csvList)) + for i := range csvList { relatedCSVs = append(relatedCSVs, foundNotWantedObj(&csvList[i])) + csvNames = append(csvNames, csvList[i].GetName()) } - changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion"), relatedCSVs...) + changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion", csvNames...), relatedCSVs...) if policy.Spec.RemediationAction.IsInform() { return nil, changed, nil @@ -1441,7 +1444,7 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV( err := r.Delete(ctx, &csvList[i]) if err != nil { - changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion"), relatedCSVs...) + changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion", csvNames...), relatedCSVs...) if anyAlreadyDeleting { // reset the "early" conditions to avoid flapping @@ -1459,10 +1462,10 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV( // reset the "early" conditions to avoid flapping earlyConds = []metav1.Condition{} - return earlyConds, updateStatus(policy, deletingCond("ClusterServiceVersion"), relatedCSVs...), nil + return earlyConds, updateStatus(policy, deletingCond("ClusterServiceVersion", csvNames...), relatedCSVs...), nil } - updateStatus(policy, deletedCond("ClusterServiceVersion"), relatedCSVs...) + updateStatus(policy, deletedCond("ClusterServiceVersion", csvNames...), relatedCSVs...) return earlyConds, true, nil } diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go index d9a4a58e..b3d2f0a6 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -428,12 +428,22 @@ func missingNotWantedCond(kind string) metav1.Condition { // foundNotWantedCond returns a NonCompliant condition with a Reason like '____Present' // and a Message like 'the ____ is present' -func foundNotWantedCond(kind string) metav1.Condition { +func foundNotWantedCond(kind string, identifiers ...string) metav1.Condition { + var extraInfo string + + if len(identifiers) == 1 { + extraInfo = fmt.Sprintf(" (%s) is", identifiers[0]) + } else if len(identifiers) > 1 { + extraInfo = fmt.Sprintf("s (%s) are", strings.Join(identifiers, ", ")) + } else { + extraInfo = " is" + } + return metav1.Condition{ Type: condType(kind), Status: metav1.ConditionFalse, Reason: kind + "Present", - Message: "the " + kind + " is present", + Message: "the " + kind + extraInfo + " present", } } @@ -450,23 +460,43 @@ func createdCond(kind string) metav1.Condition { // deletedCond returns a Compliant condition, with a Reason like '____Deleted', // and a Message like 'the ____ was deleted' -func deletedCond(kind string) metav1.Condition { +func deletedCond(kind string, identifiers ...string) metav1.Condition { + var extraInfo string + + if len(identifiers) == 1 { + extraInfo = fmt.Sprintf(" (%s) was", identifiers[0]) + } else if len(identifiers) > 1 { + extraInfo = fmt.Sprintf("s (%s) were", strings.Join(identifiers, ", ")) + } else { + extraInfo = " was" + } + return metav1.Condition{ Type: condType(kind), Status: metav1.ConditionTrue, Reason: kind + "Deleted", - Message: "the " + kind + " was deleted", + Message: "the " + kind + extraInfo + " deleted", } } // deletingCond returns a NonCompliant condition, with a Reason like '____Deleting', // and a Message like 'the ____ has a deletion timestamp' -func deletingCond(kind string) metav1.Condition { +func deletingCond(kind string, identifiers ...string) metav1.Condition { + var extraInfo string + + if len(identifiers) == 1 { + extraInfo = fmt.Sprintf(" (%s) has", identifiers[0]) + } else if len(identifiers) > 1 { + extraInfo = fmt.Sprintf("s (%s) have", strings.Join(identifiers, ", ")) + } else { + extraInfo = " has" + } + return metav1.Condition{ Type: condType(kind), Status: metav1.ConditionFalse, Reason: kind + "Deleting", - Message: "the " + kind + " has a deletion timestamp", + Message: "the " + kind + extraInfo + " a deletion timestamp", } } @@ -687,7 +717,7 @@ func buildCSVCond(csv *operatorv1alpha1.ClusterServiceVersion) metav1.Condition Type: condType(csv.Kind), Status: status, Reason: string(csv.Status.Reason), - Message: "ClusterServiceVersion - " + csv.Status.Message, + Message: "ClusterServiceVersion (" + csv.Name + ") - " + csv.Status.Message, } } diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index e6eeedea..3553af5e 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -745,12 +745,15 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Reason: "InstallSucceeded", }}, metav1.Condition{ - Type: "ClusterServiceVersionCompliant", - Status: metav1.ConditionTrue, - Reason: "InstallSucceeded", - Message: "ClusterServiceVersion - install strategy completed with no errors", + Type: "ClusterServiceVersionCompliant", + Status: metav1.ConditionTrue, + Reason: "InstallSucceeded", + Message: "ClusterServiceVersion (quay-operator.v3.8.13) - install strategy completed with " + + "no errors", }, - "ClusterServiceVersion - install strategy completed with no errors", + regexp.QuoteMeta( + "ClusterServiceVersion (quay-operator.v3.8.13) - install strategy completed with no errors", + ), ) }) @@ -863,11 +866,13 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "ClusterServiceVersionCompliant", Status: metav1.ConditionFalse, Reason: "UnsupportedOperatorGroup", - Message: "ClusterServiceVersion - AllNamespaces InstallModeType not supported," + - " cannot configure to watch all namespaces", + Message: "ClusterServiceVersion (etcdoperator.v0.9.2) - AllNamespaces InstallModeType not " + + "supported, cannot configure to watch all namespaces", }, - "ClusterServiceVersion - AllNamespaces InstallModeType not supported,"+ - " cannot configure to watch all namespaces", + regexp.QuoteMeta( + "ClusterServiceVersion (etcdoperator.v0.9.2) - AllNamespaces InstallModeType not supported,"+ + " cannot configure to watch all namespaces", + ), ) }) @@ -1808,9 +1813,9 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "ClusterServiceVersionCompliant", Status: metav1.ConditionFalse, Reason: "ClusterServiceVersionPresent", - Message: "the ClusterServiceVersion is present", + Message: "the ClusterServiceVersion (quay-operator.v3.8.13) is present", }, - `the ClusterServiceVersion is present`, + regexp.QuoteMeta("the ClusterServiceVersion (quay-operator.v3.8.13) is present"), ) check( opPolName, @@ -2142,9 +2147,9 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Type: "ClusterServiceVersionCompliant", Status: metav1.ConditionFalse, Reason: "ClusterServiceVersionDeleting", - Message: "the ClusterServiceVersion has a deletion timestamp", + Message: "the ClusterServiceVersion (" + csvName + ") has a deletion timestamp", }, - `the ClusterServiceVersion was deleted`, + regexp.QuoteMeta("the ClusterServiceVersion ("+csvName+") was deleted"), ) desiredCRDObjects := make([]policyv1.RelatedObject, 0) for _, name := range crdNames { @@ -2275,7 +2280,7 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Reason: "ClusterServiceVersionNotPresent", Message: "the ClusterServiceVersion is not present", }, - `the ClusterServiceVersion was deleted`, + regexp.QuoteMeta("the ClusterServiceVersion (quay-operator.v3.8.13) was deleted"), ) check( opPolName,