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 <mprahl@users.noreply.github.com>

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) {

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,115 @@ 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",
+			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",
+			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)
+			},
+		)
+	}
+}

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 (

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