From 7fa42934ea0ce5a5f4c589767ce88b8fac7f6c68 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Mon, 26 Jul 2021 17:08:04 -0400 Subject: [PATCH] resolver: remove legacy support for fallback parsing of CSVs 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 --- pkg/controller/registry/resolver/operators.go | 25 ----- .../registry/resolver/operators_test.go | 103 ++---------------- 2 files changed, 8 insertions(+), 120 deletions(-) diff --git a/pkg/controller/registry/resolver/operators.go b/pkg/controller/registry/resolver/operators.go index 36a8662c36..32c38b3f09 100644 --- a/pkg/controller/registry/resolver/operators.go +++ b/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 - } - o := &Operator{ name: bundle.CsvName, replaces: bundle.Replaces, diff --git a/pkg/controller/registry/resolver/operators_test.go b/pkg/controller/registry/resolver/operators_test.go index 4f3897fb94..eb295deec4 100644 --- a/pkg/controller/registry/resolver/operators_test.go +++ b/pkg/controller/registry/resolver/operators_test.go @@ -4,8 +4,6 @@ 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" @@ -13,6 +11,7 @@ import ( 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{