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