Filter out unrelated subscription resolution failures

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

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