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>
open-cluster-management-io · Mar 1, 2024 · c6fb27f · c6fb27f
1 parent 0671703
commit c6fb27f
Show file tree

Hide file tree

Showing 2 changed files with 102 additions and 11 deletions.
diff --git 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,37 @@ 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 := isApplicableConstraintsNotSatisfiable(
+				mergedSub.Name, mergedSub.Spec.Package, 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 +704,24 @@ func (r *OperatorPolicyReconciler) handleSubscription(
 	return mergedSub, earlyConds, true, nil
 }
 
+// isApplicableConstraintsNotSatisfiable 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
+func isApplicableConstraintsNotSatisfiable(subscriptionName, packageName, message string) (bool, error) {
+	safeSubName := regexp.QuoteMeta(subscriptionName)
+	safePackageName := regexp.QuoteMeta(packageName)
+	// 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 := `(?:subscription ` + safeSubName + `|package ` + safePackageName + `)(?:$|\s|,)`
+
+	return regexp.MatchString(regex, message)
+}
+
 func (r *OperatorPolicyReconciler) handleInstallPlan(
 	ctx context.Context, policy *policyv1beta1.OperatorPolicy, sub *operatorv1alpha1.Subscription,
 ) (bool, error) {

diff --git 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 (