From afef83e37f855109f988e9911370ad89657fa0fa Mon Sep 17 00:00:00 2001 From: mprahl Date: Fri, 1 Mar 2024 14:37:14 -0500 Subject: [PATCH] Filter out unrelated subscription resolution failures Since OLM includes a subscription error on all subscriptions in the namespace, even if that subscription itself is unaffected, operator policy needs to know if the error is related to the subscription. This adds some regex filtering to parse the messages since the message format is consistent. Relates: https://issues.redhat.com/browse/ACM-10195 Signed-off-by: mprahl --- controllers/operatorpolicy_controller.go | 60 +++++++-- controllers/operatorpolicy_controller_test.go | 116 ++++++++++++++++++ test/e2e/case38_install_operator_test.go | 58 ++++++++- .../operator-policy-no-exist-enforce.yaml | 23 ++++ 4 files changed, 246 insertions(+), 11 deletions(-) create mode 100644 test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 87089a2f..1e01b80c 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "reflect" + "regexp" operatorv1 "github.com/operator-framework/api/pkg/operators/v1" operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -636,19 +637,35 @@ func (r *OperatorPolicyReconciler) handleSubscription( if !updateNeeded { subResFailed := mergedSub.Status.GetCondition(operatorv1alpha1.SubscriptionResolutionFailed) + // OLM includes the status of all subscriptions in the namespace. For example, if you have two subscriptions, + // where one is referencing a valid operator and the other isn't, both will have a failed subscription + // resolution condition. if subResFailed.Status == corev1.ConditionTrue { - cond := metav1.Condition{ - Type: subConditionType, - Status: metav1.ConditionFalse, - Reason: subResFailed.Reason, - Message: subResFailed.Message, - } + includesSubscription, err := messageIncludesSubscription(mergedSub, subResFailed.Message) + if err != nil { + log.Info( + "Failed to determine if the condition applied to this subscription. Assuming it does.", + "error", err.Error(), "subscription", mergedSub.Name, "package", mergedSub.Spec.Package, + "message", subResFailed.Message, + ) - if subResFailed.LastTransitionTime != nil { - cond.LastTransitionTime = *subResFailed.LastTransitionTime + includesSubscription = true } - return mergedSub, nil, updateStatus(policy, cond, nonCompObj(foundSub, subResFailed.Reason)), nil + if includesSubscription { + cond := metav1.Condition{ + Type: subConditionType, + Status: metav1.ConditionFalse, + Reason: subResFailed.Reason, + Message: subResFailed.Message, + } + + if subResFailed.LastTransitionTime != nil { + cond.LastTransitionTime = *subResFailed.LastTransitionTime + } + + return mergedSub, nil, updateStatus(policy, cond, nonCompObj(foundSub, subResFailed.Reason)), nil + } } return mergedSub, nil, updateStatus(policy, matchesCond("Subscription"), matchedObj(foundSub)), nil @@ -685,6 +702,31 @@ func (r *OperatorPolicyReconciler) handleSubscription( return mergedSub, earlyConds, true, nil } +// messageIncludesSubscription checks if the ConstraintsNotSatisfiable message includes the input +// subscription or package. Some examples that it catches: +// https://github.com/operator-framework/operator-lifecycle-manager/blob/dc0c564f62d526bae0467d53f439e1c91a17ed8a/pkg/controller/registry/resolver/resolver.go#L257-L267 +// - no operators found from catalog %s in namespace %s referenced by subscription %s +// - no operators found in package %s in the catalog referenced by subscription %s +// - no operators found in channel %s of package %s in the catalog referenced by subscription %s +// - no operators found with name %s in channel %s of package %s in the catalog referenced by subscription %s +// - multiple name matches for status.installedCSV of subscription %s/%s: %s +func messageIncludesSubscription(subscription *operatorv1alpha1.Subscription, message string) (bool, error) { + safeNs := regexp.QuoteMeta(subscription.Namespace) + safeSubName := regexp.QuoteMeta(subscription.Name) + safeSubNameWithNs := safeNs + `\/` + safeSubName + safePackageName := regexp.QuoteMeta(subscription.Spec.Package) + safePackageNameWithNs := safeNs + `\/` + safePackageName + // Craft a regex that looks for mention of the subscription or package. Notice that after the package or + // subscription name, it must either be the end of the string, white space, or a comma. This so that + // "gatekeeper-operator" doesn't erroneously match "gatekeeper-operator-product". + regex := fmt.Sprintf( + `(?:subscription (?:%s|%s)|package (?:%s|%s))(?:$|\s|,|:)`, + safeSubName, safeSubNameWithNs, safePackageName, safePackageNameWithNs, + ) + + return regexp.MatchString(regex, message) +} + func (r *OperatorPolicyReconciler) handleInstallPlan( ctx context.Context, policy *policyv1beta1.OperatorPolicy, sub *operatorv1alpha1.Subscription, ) (bool, error) { diff --git a/controllers/operatorpolicy_controller_test.go b/controllers/operatorpolicy_controller_test.go index 4cf1d59d..c183d838 100644 --- a/controllers/operatorpolicy_controller_test.go +++ b/controllers/operatorpolicy_controller_test.go @@ -1,8 +1,10 @@ package controllers import ( + "fmt" "testing" + operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -83,3 +85,117 @@ func TestBuildOperatorGroup(t *testing.T) { assert.Equal(t, ret.ObjectMeta.GetGenerateName(), "my-operators-") assert.Equal(t, ret.ObjectMeta.GetNamespace(), "my-operators") } + +func TestMessageIncludesSubscription(t *testing.T) { + t.Parallel() + + testCases := []struct { + subscriptionName string + packageName string + message string + expected bool + }{ + { + subscriptionName: "quay-does-not-exist", + packageName: "quay-does-not-exist", + message: "no operators found from catalog some-catalog in namespace default referenced by subscription " + + "quay-does-not-exist", + expected: true, + }, + { + subscriptionName: "quay", + packageName: "quay", + message: "no operators found from catalog some-catalog in namespace default referenced by subscription " + + "quay-operator-does-not-exist", + expected: false, + }, + { + subscriptionName: "quay-does-not-exist", + packageName: "quay-does-not-exist", + message: "no operators found in package quay-does-not-exist in the catalog referenced by subscription " + + "quay-does-not-exist", + expected: true, + }, + { + subscriptionName: "quay-does-not-exist", + packageName: "quay-does-not-exist", + message: "no operators found in package quay-does-not-exist in the catalog referenced by subscription " + + "quay-does-not-exist", + expected: true, + }, + { + subscriptionName: "quay-does-not-exist", + packageName: "quay-does-not-exist", + message: "no operators found in channel a channel of package quay-does-not-exist in the catalog " + + "referenced by subscription quay-does-not-exist", + expected: true, + }, + { + subscriptionName: "quay-does-not-exist", + packageName: "other", + message: "no operators found in channel a channel of package quay-does-not-exist in the catalog " + + "referenced by subscription quay-does-not-exist", + expected: true, + }, + { + subscriptionName: "other", + packageName: "quay-does-not-exist", + message: "no operators found in channel a channel of package quay-does-not-exist in the catalog " + + "referenced by subscription quay-does-not-exist", + expected: true, + }, + { + subscriptionName: "quay-does-not-exist", + packageName: "quay-does-not-exist", + //nolint: dupword + message: "no operators found with name quay-does-not-exist in channel channel of package " + + " quay-does-not-exist in the catalog referenced by subscription quay-does-not-exist", + expected: true, + }, + { + subscriptionName: "quay", + packageName: "quay", + //nolint: dupword + message: "no operators found with name quay-does-not-exist in channel channel of package " + + " quay-does-not-exist in the catalog referenced by subscription quay-does-not-exist", + expected: false, + }, + { + subscriptionName: "quay", + packageName: "quay", + message: "multiple name matches for status.installedCSV of subscription default/quay: quay.v123", + expected: true, + }, + { + subscriptionName: "quay", + packageName: "quay", + message: "multiple name matches for status.installedCSV of subscription some-ns/quay: quay.v123", + expected: false, + }, + } + + for i, test := range testCases { + test := test + + t.Run( + fmt.Sprintf("test[%d]", i), + func(t *testing.T) { + t.Parallel() + + subscription := &operatorv1alpha1.Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Name: test.subscriptionName, + Namespace: "default", + }, + Spec: &operatorv1alpha1.SubscriptionSpec{ + Package: test.packageName, + }, + } + + match, err := messageIncludesSubscription(subscription, test.message) + assert.Equal(t, err, nil) + assert.Equal(t, match, test.expected) + }, + ) + } +} diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go index 3820afd0..db5d0905 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -581,8 +581,10 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun }) Describe("Test health checks on OLM resources after OperatorPolicy operator installation", Ordered, func() { const ( - opPolYAML = "../resources/case38_operator_install/operator-policy-no-group-enforce.yaml" - opPolName = "oppol-no-group-enforce" + opPolYAML = "../resources/case38_operator_install/operator-policy-no-group-enforce.yaml" + opPolName = "oppol-no-group-enforce" + opPolNoExistYAML = "../resources/case38_operator_install/operator-policy-no-exist-enforce.yaml" + opPolNoExistName = "oppol-no-exist-enforce" ) BeforeAll(func() { utils.Kubectl("create", "ns", opPolTestNS) @@ -592,6 +594,9 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun createObjWithParent(parentPolicyYAML, parentPolicyName, opPolYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy) + + createObjWithParent(parentPolicyYAML, parentPolicyName, + opPolNoExistYAML, opPolTestNS, gvrPolicy, gvrOperatorPolicy) }) It("Should generate conditions and relatedobjects of CSV", func(ctx SpecContext) { @@ -650,6 +655,55 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun "All operator Deployments have their minimum availability", ) }) + + It("Should only be noncompliant if the subscription error relates to the one in the operator policy", func() { + By("Checking that " + opPolNoExistName + " is NonCompliant") + check( + opPolNoExistName, + true, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "Subscription", + APIVersion: "operators.coreos.com/v1alpha1", + }, + Compliant: "NonCompliant", + Reason: "ConstraintsNotSatisfiable", + }}, + metav1.Condition{ + Type: "SubscriptionCompliant", + Status: metav1.ConditionFalse, + Reason: "ConstraintsNotSatisfiable", + Message: "constraints not satisfiable: no operators found in package project-quay-does-not-exist" + + " in the catalog referenced by subscription project-quay-does-not-exist, subscription " + + "project-quay-does-not-exist exists", + }, + "constraints not satisfiable: no operators found in package project-quay-does-not-exist", + ) + + // Check if the subscription is still compliant on the operator policy trying to install a valid operator. + // This tests that subscription status filtering is working properly since OLM includes the + // subscription errors as a condition on all subscriptions in the namespace. + By("Checking that " + opPolName + " is still Compliant and unaffected by " + opPolNoExistName) + check( + opPolName, + false, + []policyv1.RelatedObject{{ + Object: policyv1.ObjectResource{ + Kind: "Subscription", + APIVersion: "operators.coreos.com/v1alpha1", + }, + Compliant: "Compliant", + Reason: "Resource found as expected", + }}, + metav1.Condition{ + Type: "SubscriptionCompliant", + Status: metav1.ConditionTrue, + Reason: "SubscriptionMatches", + Message: "the Subscription matches what is required by the policy", + }, + "the Subscription matches what is required by the policy", + ) + }) }) Describe("Test health checks on OLM resources on OperatorPolicy with failed CSV", Ordered, func() { const ( diff --git a/test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml b/test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml new file mode 100644 index 00000000..13657edb --- /dev/null +++ b/test/resources/case38_operator_install/operator-policy-no-exist-enforce.yaml @@ -0,0 +1,23 @@ +apiVersion: policy.open-cluster-management.io/v1beta1 +kind: OperatorPolicy +metadata: + name: oppol-no-exist-enforce + annotations: + policy.open-cluster-management.io/parent-policy-compliance-db-id: "124" + policy.open-cluster-management.io/policy-compliance-db-id: "64" + ownerReferences: + - apiVersion: policy.open-cluster-management.io/v1 + kind: Policy + name: parent-policy + uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation +spec: + remediationAction: enforce + severity: medium + complianceType: musthave + subscription: + channel: stable-3.8 + name: project-quay-does-not-exist + namespace: operator-policy-testns + installPlanApproval: Automatic + source: operatorhubio-catalog + sourceNamespace: olm