Merge pull request #307 from perdasilva/bz_1982737

Bug 1982737: Make malformed CSV fail nicely (#2673)

Author: openshift-merge-robot

staging/operator-lifecycle-manager/pkg/controller/errors/errors.go

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

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go

@@ -1376,7 +1376,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
@@ -1427,6 +1433,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 fatal := olmerrors.IsFatal(err); fatal {
+				return false, nil, err
+			}
+
 			errs = append(errs, fmt.Errorf("failed to turn bundle into steps: %v", err))
 			unpacked = false
 			continue
@@ -1615,6 +1625,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 fatal := olmerrors.IsFatal(err); fatal {
+				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

staging/operator-lifecycle-manager/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.NewFatalError(fmt.Errorf("bundle CSV %s missing APIVersion", csv.Name))
+	}
+
+	if csv.Kind == "" {
+		return nil, olmerrors.NewFatalError(fmt.Errorf("bundle CSV %s missing Kind", csv.Name))
+	}
+
 	csv.SetNamespace(namespace)
 	csv.Spec.Replaces = replaces
 	anno, err := projection.PropertiesAnnotationFromPropertyList(bundle.Properties)

staging/operator-lifecycle-manager/test/e2e/catalog_e2e_test.go

@@ -7,11 +7,13 @@ import (
 	"context"
 	"fmt"
 	"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 +42,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 +1343,68 @@ 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
+				_, err = fetchInstallPlan(GinkgoT(), crc, installPlanName, subscription.GetNamespace(), buildInstallPlanPhaseCheckFunc(operatorsv1alpha1.InstallPlanPhaseFailed))
+				Expect(err).To(BeNil())
+				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) {

staging/operator-lifecycle-manager/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/olmtest/missing_api_version:latest
+properties:
+  - type: olm.gvk
+    value:
+      group: example.com
+      kind: TestA
+      version: v1alpha1
+  - type: olm.package
+    value:
+      packageName: packageA
+      version: 1.0.0