FIX: Bug 1982737 - Make malformed CSV fail nicely

- Modified bundle unpacking to fail installPlan with a nice message if
after unbundling csv manifests APIVersion or Kind are blank.

- Adds retryablError type to return to checked when syncing install plans
 to see if the error causing issues is retryingable or should cause the
 IP to be transitioned to failed state.

- Makes installPlan fail with a nicer more user friendly message than
the current difficult to understand message regarding serviceAccounts.

- Adds tests using internal magic-catalog library to  confirm that unpacking
fails when APIVersion is blank.

Signed-off-by: Noah Sapse <[email protected]>

Author: nsapse

Diff for: pkg/controller/errors/errors.go

@@ -47,6 +47,21 @@ func IsMultipleExistingCRDOwnersError(err error) bool {
 	return false
 }
 
+type RetryableError struct {
+	error
+}
+
+func NewRetryableError(err error) RetryableError {
+	return RetryableError{err}
+}
+func IsRetryable(err error) bool {
+	switch err.(type) {
+	case RetryableError:
+		return true
+	}
+	return false
+}
+
 // GroupVersionKindNotFoundError occurs when we can't find an API via discovery
 type GroupVersionKindNotFoundError struct {
 	Group   string

Diff for: pkg/controller/operators/catalog/operator.go

@@ -1388,7 +1388,13 @@ type UnpackedBundleReference struct {
 	Properties             string `json:"properties"`
 }
 
-// unpackBundles makes one walk through the bundlelookups and attempts to progress them
+/* unpackBundles makes one walk through the bundlelookups and attempts to progress them
+Returns:
+    unpacked: bool - If the bundle was successfully unpacked
+    out:      *v1alpha1.InstallPlan - the resulting installPlan
+	error:    error
+*/
+
 func (o *Operator) unpackBundles(plan *v1alpha1.InstallPlan) (bool, *v1alpha1.InstallPlan, error) {
 	out := plan.DeepCopy()
 	unpacked := true
@@ -1439,6 +1445,10 @@ func (o *Operator) unpackBundles(plan *v1alpha1.InstallPlan) (bool, *v1alpha1.In
 		// Ensure that bundle can be applied by the current version of OLM by converting to steps
 		steps, err := resolver.NewStepsFromBundle(res.Bundle(), out.GetNamespace(), res.Replaces, res.CatalogSourceRef.Name, res.CatalogSourceRef.Namespace)
 		if err != nil {
+			if retryable := olmerrors.IsRetryable(err); !retryable {
+				return false, nil, err
+			}
+
 			errs = append(errs, fmt.Errorf("failed to turn bundle into steps: %v", err))
 			unpacked = false
 			continue
@@ -1627,6 +1637,14 @@ func (o *Operator) syncInstallPlans(obj interface{}) (syncError error) {
 	if len(plan.Status.BundleLookups) > 0 {
 		unpacked, out, err := o.unpackBundles(plan)
 		if err != nil {
+			// If the error was fatal capture and fail
+			if retryable := olmerrors.IsRetryable(err); !retryable {
+				if err := o.transitionInstallPlanToFailed(plan, logger, v1alpha1.InstallPlanReasonInstallCheckFailed, err.Error()); err != nil {
+					// retry for failure to update status
+					syncError = err
+					return
+				}
+			}
 			// Retry sync if non-fatal error
 			syncError = fmt.Errorf("bundle unpacking failed: %v", err)
 			return

Diff for: pkg/controller/registry/resolver/steps.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"strings"
 
+	olmerrors "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/errors"
 	"github.com/operator-framework/operator-registry/pkg/api"
 	extScheme "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/scheme"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -118,6 +119,15 @@ func NewStepResourceFromBundle(bundle *api.Bundle, namespace, replaces, catalogS
 		return nil, err
 	}
 
+	// Check unpacked bundled for for missing APIVersion or Kind
+	if csv.APIVersion == "" {
+		return nil, olmerrors.NewRetryableError(fmt.Errorf("bundle CSV %s missing APIVersion", csv.Name))
+	}
+
+	if csv.Kind == "" {
+		return nil, olmerrors.NewRetryableError(fmt.Errorf("bundle CSV %s missing Kind", csv.Name))
+	}
+
 	csv.SetNamespace(namespace)
 	csv.Spec.Replaces = replaces
 	anno, err := projection.PropertiesAnnotationFromPropertyList(bundle.Properties)

Diff for: test/e2e/catalog_e2e_test.go

@@ -6,12 +6,15 @@ package e2e
 import (
 	"context"
 	"fmt"
+	"log"
 	"net"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
 
 	operatorsv1 "github.com/operator-framework/api/pkg/operators/v1"
+	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	k8serror "k8s.io/apimachinery/pkg/api/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -40,6 +43,7 @@ import (
 const (
 	openshiftregistryFQDN = "image-registry.openshift-image-registry.svc:5000"
 	catsrcImage           = "docker://quay.io/olmtest/catsrc-update-test:"
+	badCSVDir             = "bad-csv"
 )
 
 var _ = Describe("Starting CatalogSource e2e tests", func() {
@@ -1340,6 +1344,69 @@ var _ = Describe("Starting CatalogSource e2e tests", func() {
 			}
 		}
 	})
+
+	When("A CatalogSource is created with an operator that has a CSV with missing metadata.ApiVersion", func() {
+
+		var (
+			magicCatalog      MagicCatalog
+			catalogSourceName string
+			subscription      *operatorsv1alpha1.Subscription
+			c                 client.Client
+		)
+
+		BeforeEach(func() {
+			c = ctx.Ctx().Client()
+
+			provider, err := NewFileBasedFiledBasedCatalogProvider(filepath.Join(testdataDir, badCSVDir, "bad-csv.yaml"))
+			Expect(err).To(BeNil())
+
+			catalogSourceName = genName("cat-bad-csv")
+			magicCatalog = NewMagicCatalog(c, ns.GetName(), catalogSourceName, provider)
+			Expect(magicCatalog.DeployCatalog(context.Background())).To(BeNil())
+
+		})
+
+		AfterEach(func() {
+			TeardownNamespace(ns.GetName())
+		})
+
+		When("A Subscription is created catalogSource built with the malformed CSV", func() {
+			BeforeEach(func ()  {
+				subscription = &operatorsv1alpha1.Subscription{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      fmt.Sprintf("%s-sub", catalogSourceName),
+						Namespace: ns.GetName(),
+					},
+					Spec: &operatorsv1alpha1.SubscriptionSpec{
+						CatalogSource:          catalogSourceName,
+						CatalogSourceNamespace: ns.GetName(),
+						Channel:                "stable",
+						Package:                "packageA",
+					},
+				}
+				Expect(c.Create(context.Background(), subscription)).To(BeNil())
+			})
+
+			It("fails with a ResolutionFailed error condition, and a message that highlights the missing field in the CSV", func() {
+
+				subscription, err := fetchSubscription(crc, subscription.GetNamespace(), subscription.GetName(), subscriptionHasInstallPlanChecker)
+				Expect(err).Should(BeNil())
+				installPlanName := subscription.Status.Install.Name
+
+				// ensure we wait for the installPlan to fail before moving forward then fetch the subscription again
+				installPlan, err := fetchInstallPlan(GinkgoT(), crc, installPlanName, subscription.GetNamespace(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
+				Expect(err).To(BeNil())
+				log.Print(fmt.Printf("installPlan %s failed: re-fetching subscription", installPlan.Name))
+				subscription, err = fetchSubscription(crc, subscription.GetNamespace(), subscription.GetName(), subscriptionHasInstallPlanChecker)
+				Expect(err).To(BeNil())
+
+				// expect the message that API missing
+				failingCondition := subscription.Status.GetCondition(operatorsv1alpha1.SubscriptionInstallPlanFailed)
+				Expect(failingCondition.Message).To(ContainSubstring("missing APIVersion"))
+			})
+		})
+	})
+
 })
 
 func getOperatorDeployment(c operatorclient.ClientInterface, namespace string, operatorLabels labels.Set) (*appsv1.Deployment, error) {

Diff for: test/e2e/testdata/bad-csv/bad-csv.yaml

@@ -0,0 +1,25 @@
+---
+schema: olm.package
+name: packageA
+defaultChannel: stable
+---
+schema: olm.channel
+package: packageA
+name: stable
+entries:
+  - name: bad-csv
+---
+schema: olm.bundle
+name: bad-csv
+package: packageA
+image: quay.io/nsapse/bad-etcd:latest
+properties:
+  - type: olm.gvk
+    value:
+      group: example.com
+      kind: TestA
+      version: v1alpha1
+  - type: olm.package
+    value:
+      packageName: packageA
+      version: 1.0.0