From 0276169988b924d87858ef0cb12dcfb15272e32d Mon Sep 17 00:00:00 2001 From: mprahl Date: Mon, 22 Apr 2024 12:44:43 -0400 Subject: [PATCH 1/3] Make the operator policy message case consistent Relates: https://issues.redhat.com/browse/ACM-11026 Signed-off-by: mprahl --- 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 de222a85..2e2603ca 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 61fa106b..272c3102 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 4ab61a04cbc423a62b7a9c37f8bb337629d355dd Mon Sep 17 00:00:00 2001 From: mprahl Date: Mon, 22 Apr 2024 12:47:42 -0400 Subject: [PATCH 2/3] 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 --- 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 2e2603ca..976947e6 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 3a2f21ea8b4f71afcc11691500418525c5cc7f47 Mon Sep 17 00:00:00 2001 From: mprahl Date: Tue, 23 Apr 2024 13:48:34 -0400 Subject: [PATCH 3/3] Include the CSV name in compliance messages Relates: https://issues.redhat.com/browse/ACM-11026 Signed-off-by: mprahl --- 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 fd248aeb..96cbc17a 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -1393,11 +1393,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 @@ -1421,7 +1424,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 @@ -1439,10 +1442,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 976947e6..f5c4a702 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 272c3102..40f80ccb 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,