resolver: remove legacy support for fallback parsing of CSVs (#2404)

When no APIs or properties are present on bundles, the resolver
currently falls back to a legacy mode where that information is parsed
from CSVs present in the index.

This legacy fallback method causes the resolver to incorrectly identify
multiple heads for a channel when:
a) The index contains CSVs only on channel head bundles, and
b) A package contains two channels, where one channel contains the head
   of the other channel and there is at least one other node between
   these channel head nodes in that channel.
c) The bundles have no properties, provided APIs, or required APIs

Conditions a) and b) are extremely prevalent, so this bug will often be
encountered whenever c) itself is true.

This commit removes support for the legacy CSV parsing fallback, which
means that only first class fields in the GRPC API will be used during
resolutions.

Signed-off-by: Joe Lanford <[email protected]>

Co-authored-by: Joe Lanford <[email protected]>

Author: timflannagan

pkg/controller/registry/resolver/operators.go

@@ -278,31 +278,6 @@ func NewOperatorFromBundle(bundle *api.Bundle, startingCSV string, sourceKey reg
 		properties = append(properties, ps...)
 	}
 
-	// legacy support - if the grpc api doesn't contain required/provided apis, fallback to csv parsing
-	if len(required) == 0 && len(provided) == 0 && len(properties) == 0 {
-		// fallback to csv parsing
-		if bundle.CsvJson == "" {
-			if bundle.GetBundlePath() != "" {
-				return nil, fmt.Errorf("couldn't parse bundle path, missing provided and required apis")
-			}
-
-			return nil, fmt.Errorf("couldn't parse bundle, missing provided and required apis")
-		}
-
-		csv := &v1alpha1.ClusterServiceVersion{}
-		if err := json.Unmarshal([]byte(bundle.CsvJson), csv); err != nil {
-			return nil, err
-		}
-
-		op, err := NewOperatorFromV1Alpha1CSV(csv)
-		if err != nil {
-			return nil, err
-		}
-		op.sourceInfo = sourceInfo
-		op.bundle = bundle
-		return op, nil
-	}
-
 	return &Operator{
 		name:         bundle.CsvName,
 		replaces:     bundle.Replaces,

pkg/controller/registry/resolver/operators_test.go

@@ -4,15 +4,14 @@ import (
 	"encoding/json"
 	"testing"
 
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
-
 	"github.com/blang/semver/v4"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	opver "github.com/operator-framework/api/pkg/lib/version"
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
 	"github.com/operator-framework/operator-registry/pkg/api"
 	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
 )
@@ -1078,8 +1077,7 @@ func TestNewOperatorFromBundle(t *testing.T) {
 				sourceKey: registry.CatalogKey{Name: "source", Namespace: "testNamespace"},
 			},
 			want: &Operator{
-				// lack of full api response falls back to csv name
-				name:         "testCSV",
+				name:         "testBundle",
 				version:      &version.Version,
 				providedAPIs: EmptyAPISet(),
 				requiredAPIs: EmptyAPISet(),
@@ -1155,81 +1153,16 @@ func TestNewOperatorFromBundle(t *testing.T) {
 			},
 		},
 		{
-			name: "BundleReplaceOverrides",
+			name: "BundleIgnoreCSV",
 			args: args{
-				bundle:    bundleNoAPIs,
+				bundle:    bundleWithAPIsUnextracted,
 				sourceKey: registry.CatalogKey{Name: "source", Namespace: "testNamespace"},
 			},
 			want: &Operator{
-				// lack of full api response falls back to csv name
-				name:         "testCSV",
+				name:         "testBundle",
 				providedAPIs: EmptyAPISet(),
 				requiredAPIs: EmptyAPISet(),
-				bundle:       bundleNoAPIs,
-				version:      &version.Version,
-				sourceInfo: &OperatorSourceInfo{
-					Package: "testPackage",
-					Channel: "testChannel",
-					Catalog: registry.CatalogKey{Name: "source", Namespace: "testNamespace"},
-				},
-			},
-		},
-		{
-			name: "BundleCsvFallback",
-			args: args{
-				bundle:    bundleWithAPIsUnextracted,
-				sourceKey: registry.CatalogKey{Name: "source", Namespace: "testNamespace"},
-			},
-			want: &Operator{
-				name: "testCSV",
-				providedAPIs: APISet{
-					opregistry.APIKey{
-						Group:   "crd.group.com",
-						Version: "v1",
-						Kind:    "OwnedCRD",
-						Plural:  "owneds",
-					}: struct{}{},
-					opregistry.APIKey{
-						Group:   "apis.group.com",
-						Version: "v1",
-						Kind:    "OwnedAPI",
-						Plural:  "ownedapis",
-					}: struct{}{},
-				},
-				requiredAPIs: APISet{
-					opregistry.APIKey{
-						Group:   "crd.group.com",
-						Version: "v1",
-						Kind:    "RequiredCRD",
-						Plural:  "requireds",
-					}: struct{}{},
-					opregistry.APIKey{
-						Group:   "apis.group.com",
-						Version: "v1",
-						Kind:    "RequiredAPI",
-						Plural:  "requiredapis",
-					}: struct{}{},
-				},
-				properties: []*api.Property{
-					{
-						Type:  "olm.gvk",
-						Value: "{\"group\":\"crd.group.com\",\"kind\":\"OwnedCRD\",\"version\":\"v1\"}",
-					},
-					{
-						Type:  "olm.gvk",
-						Value: "{\"group\":\"apis.group.com\",\"kind\":\"OwnedAPI\",\"version\":\"v1\"}",
-					},
-					{
-						Type:  "olm.gvk.required",
-						Value: "{\"group\":\"apis.group.com\",\"kind\":\"RequiredAPI\",\"version\":\"v1\"}",
-					},
-					{
-						Type:  "olm.gvk.required",
-						Value: "{\"group\":\"crd.group.com\",\"kind\":\"RequiredCRD\",\"version\":\"v1\"}",
-					},
-				},
-				bundle:  bundleWithAPIsUnextracted,
-				version: &version.Version,
+				bundle:       bundleWithAPIsUnextracted,
 				sourceInfo: &OperatorSourceInfo{
 					Package: "testPackage",
 					Channel: "testChannel",
@@ -1238,14 +1171,14 @@ func TestNewOperatorFromBundle(t *testing.T) {
 			},
 		},
 		{
-			name: "bundle in default channel",
+			name: "BundleInDefaultChannel",
 			args: args{
 				bundle:         bundleNoAPIs,
 				sourceKey:      registry.CatalogKey{Name: "source", Namespace: "testNamespace"},
 				defaultChannel: "testChannel",
 			},
 			want: &Operator{
-				name:         "testCSV",
+				name:         "testBundle",
 				version:      &version.Version,
 				providedAPIs: EmptyAPISet(),
 				requiredAPIs: EmptyAPISet(),
@@ -1258,26 +1191,6 @@ func TestNewOperatorFromBundle(t *testing.T) {
 				},
 			},
 		},
-		{
-			name: "BundleNoAPIs",
-			args: args{
-				bundle:    bundleNoAPIs,
-				sourceKey: registry.CatalogKey{Name: "source", Namespace: "testNamespace"},
-			},
-			want: &Operator{
-				// lack of full api response falls back to csv name
-				name:         "testCSV",
-				version:      &version.Version,
-				providedAPIs: EmptyAPISet(),
-				requiredAPIs: EmptyAPISet(),
-				bundle:       bundleNoAPIs,
-				sourceInfo: &OperatorSourceInfo{
-					Package: "testPackage",
-					Channel: "testChannel",
-					Catalog: registry.CatalogKey{Name: "source", Namespace: "testNamespace"},
-				},
-			},
-		},
 		{
 			name: "BundleWithPropertiesAndDependencies",
 			args: args{