Merge pull request #1664 from dinhxuanvu/ocs-lib-fix-4.5

Bug 1859493: fix(resolver): Exclude all installed packages in dependency search

Author: openshift-merge-robot

pkg/controller/operators/catalog/operator.go

@@ -1359,7 +1359,7 @@ func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiexte
 		return err
 	}
 	for _, version := range oldCRD.Spec.Versions {
-		if !version.Served {
+		if version.Served {
 			gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: version.Name, Resource: oldCRD.Spec.Names.Plural}
 			err := validateExistingCRs(dynamicClient, gvr, convertedCRD)
 			if err != nil {
@@ -1382,7 +1382,7 @@ func validateV1Beta1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *ap
 		return err
 	}
 	for _, version := range oldCRD.Spec.Versions {
-		if !version.Served {
+		if version.Served {
 			gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: version.Name, Resource: oldCRD.Spec.Names.Plural}
 			err := validateExistingCRs(dynamicClient, gvr, convertedCRD)
 			if err != nil {

pkg/controller/registry/registry_client.go

@@ -21,7 +21,7 @@ type ChannelEntryStream interface {
 // ClientInterface that extends client.Interface
 type ClientInterface interface {
 	client.Interface
-	FindBundleThatProvides(ctx context.Context, group, version, kind, excludedPkgName string) (*registryapi.Bundle, error)
+	FindBundleThatProvides(ctx context.Context, group, version, kind string, excludedPackages map[string]struct{}) (*registryapi.Bundle, error)
 	GetLatestChannelEntriesThatProvide(ctx context.Context, group, version, kind string) (*ChannelEntryIterator, error)
 }
 
@@ -81,14 +81,14 @@ func (rc *Client) GetLatestChannelEntriesThatProvide(ctx context.Context, group,
 
 // FindBundleThatProvides returns a bundle that provides the request API and
 // doesn't belong to the provided package
-func (rc *Client) FindBundleThatProvides(ctx context.Context, group, version, kind, excludedPkgName string) (*registryapi.Bundle, error) {
+func (rc *Client) FindBundleThatProvides(ctx context.Context, group, version, kind string, excludedPackages map[string]struct{}) (*registryapi.Bundle, error) {
 	it, err := rc.GetLatestChannelEntriesThatProvide(ctx, group, version, kind)
 	if err != nil {
 		return nil, err
 	}
-	entry := rc.filterChannelEntries(ctx, it, excludedPkgName)
+	entry := rc.filterChannelEntries(ctx, it, excludedPackages)
 	if entry == nil {
-		return nil, fmt.Errorf("Unable to find a channel entry which doesn't belong to package %s's default channel", excludedPkgName)
+		return nil, fmt.Errorf("Unable to find a channel entry that satisfies the requirements")
 	}
 	bundle, err := rc.Client.Registry.GetBundle(ctx, &registryapi.GetBundleRequest{PkgName: entry.PackageName, ChannelName: entry.ChannelName, CsvName: entry.BundleName})
 	if err != nil {
@@ -100,12 +100,12 @@ func (rc *Client) FindBundleThatProvides(ctx context.Context, group, version, ki
 // FilterChannelEntries filters out channel entries that provide the requested
 // API and come from the same package with original operator and returns the
 // first entry on the list from the default channel of that package
-func (rc *Client) filterChannelEntries(ctx context.Context, it *ChannelEntryIterator, excludedPkgName string) *opregistry.ChannelEntry {
+func (rc *Client) filterChannelEntries(ctx context.Context, it *ChannelEntryIterator, excludedPackages map[string]struct{}) *opregistry.ChannelEntry {
 	defChannels := make(map[string]string, 0)
 
 	var entries []*opregistry.ChannelEntry
 	for e := it.Next(); e != nil; e = it.Next() {
-		if e.PackageName != excludedPkgName {
+		if _, ok := excludedPackages[e.PackageName]; !ok {
 			// keep track of the default channel for each package
 			if _, ok := defChannels[e.PackageName]; !ok {
 				defChannel, err := rc.getDefaultPackageChannel(ctx, e.PackageName)

pkg/controller/registry/resolver/evolver.go

@@ -132,9 +132,14 @@ func (e *NamespaceGenerationEvolver) queryForRequiredAPIs() error {
 			initialSource = operator.SourceInfo()
 			break
 		}
+		// Get the list of installed operators in the namespace
+		opList := make(map[string]struct{})
+		for _, operator := range e.gen.Operators() {
+			opList[operator.SourceInfo().Package] = struct{}{}
+		}
 
 		// attempt to find a bundle that provides that api
-		if bundle, key, err := e.querier.FindProvider(*api, initialSource.Catalog, initialSource.Package); err == nil {
+		if bundle, key, err := e.querier.FindProvider(*api, initialSource.Catalog, opList); err == nil {
 			// add a bundle that provides the api to the generation
 			o, err := NewOperatorFromBundle(bundle, "", *key)
 			if err != nil {

pkg/controller/registry/resolver/fakes/fake_registry_client_interface.go

@@ -28,7 +28,7 @@ type SourceRef struct {
 }
 
 type SourceQuerier interface {
-	FindProvider(api opregistry.APIKey, initialSource CatalogKey, excludedPkgName string) (*api.Bundle, *CatalogKey, error)
+	FindProvider(api opregistry.APIKey, initialSource CatalogKey, excludedPackages map[string]struct{}) (*api.Bundle, *CatalogKey, error)
 	FindBundle(pkgName, channelName, bundleName string, initialSource CatalogKey) (*api.Bundle, *CatalogKey, error)
 	FindLatestBundle(pkgName, channelName string, initialSource CatalogKey) (*api.Bundle, *CatalogKey, error)
 	FindReplacement(currentVersion *semver.Version, bundleName, pkgName, channelName string, initialSource CatalogKey) (*api.Bundle, *CatalogKey, error)
@@ -54,23 +54,23 @@ func (q *NamespaceSourceQuerier) Queryable() error {
 	return nil
 }
 
-func (q *NamespaceSourceQuerier) FindProvider(api opregistry.APIKey, initialSource CatalogKey, excludedPkgName string) (*registryapi.Bundle, *CatalogKey, error) {
+func (q *NamespaceSourceQuerier) FindProvider(api opregistry.APIKey, initialSource CatalogKey, excludedPackages map[string]struct{}) (*registryapi.Bundle, *CatalogKey, error) {
 	if initialSource.Name != "" && initialSource.Namespace != "" {
 		source, ok := q.sources[initialSource]
 		if ok {
-			if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Group, api.Version, api.Kind, excludedPkgName); err == nil {
+			if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Group, api.Version, api.Kind, excludedPackages); err == nil {
 				return bundle, &initialSource, nil
 			}
-			if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Plural+"."+api.Group, api.Version, api.Kind, excludedPkgName); err == nil {
+			if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Plural+"."+api.Group, api.Version, api.Kind, excludedPackages); err == nil {
 				return bundle, &initialSource, nil
 			}
 		}
 	}
 	for key, source := range q.sources {
-		if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Group, api.Version, api.Kind, excludedPkgName); err == nil {
+		if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Group, api.Version, api.Kind, excludedPackages); err == nil {
 			return bundle, &key, nil
 		}
-		if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Plural+"."+api.Group, api.Version, api.Kind, excludedPkgName); err == nil {
+		if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Plural+"."+api.Group, api.Version, api.Kind, excludedPackages); err == nil {
 			return bundle, &key, nil
 		}
 	}

pkg/controller/registry/resolver/querier.go

@@ -121,6 +121,7 @@ func TestNamespaceSourceQuerier_FindProvider(t *testing.T) {
 		CatalogKey{"test", "ns"}:  &fakeSource,
 		CatalogKey{"test2", "ns"}: &fakeSource2,
 	}
+	excludedPkgs := make(map[string]struct{})
 	bundle := &api.Bundle{CsvName: "test", PackageName: "testPkg", ChannelName: "testChannel"}
 	bundle2 := &api.Bundle{CsvName: "test2", PackageName: "testPkg2", ChannelName: "testChannel2"}
 	fakeSource.GetBundleThatProvidesStub = func(ctx context.Context, group, version, kind string) (*api.Bundle, error) {
@@ -135,13 +136,13 @@ func TestNamespaceSourceQuerier_FindProvider(t *testing.T) {
 		}
 		return bundle2, nil
 	}
-	fakeSource.FindBundleThatProvidesStub = func(ctx context.Context, group, version, kind, pkgName string) (*api.Bundle, error) {
+	fakeSource.FindBundleThatProvidesStub = func(ctx context.Context, group, version, kind string, excludedPkgs map[string]struct{}) (*api.Bundle, error) {
 		if group != "group" || version != "version" || kind != "kind" {
 			return nil, fmt.Errorf("Not Found")
 		}
 		return bundle, nil
 	}
-	fakeSource2.FindBundleThatProvidesStub = func(ctx context.Context, group, version, kind, pkgName string) (*api.Bundle, error) {
+	fakeSource2.FindBundleThatProvidesStub = func(ctx context.Context, group, version, kind string, excludedPkgs map[string]struct{}) (*api.Bundle, error) {
 		if group != "group2" || version != "version2" || kind != "kind2" {
 			return nil, fmt.Errorf("Not Found")
 		}
@@ -228,7 +229,7 @@ func TestNamespaceSourceQuerier_FindProvider(t *testing.T) {
 			q := &NamespaceSourceQuerier{
 				sources: tt.fields.sources,
 			}
-			bundle, key, err := q.FindProvider(tt.args.api, tt.args.catalogKey, "")
+			bundle, key, err := q.FindProvider(tt.args.api, tt.args.catalogKey, excludedPkgs)
 			require.Equal(t, tt.out.err, err)
 			require.Equal(t, tt.out.bundle, bundle)
 			require.Equal(t, tt.out.key, key)

pkg/controller/registry/resolver/querier_test.go

@@ -360,15 +360,15 @@ func NewFakeSourceQuerier(bundlesByCatalog map[CatalogKey][]*api.Bundle) *Namesp
 			return nil, fmt.Errorf("no bundle found")
 		}
 
-		source.FindBundleThatProvidesStub = func(ctx context.Context, groupOrName, version, kind, pkgName string) (*api.Bundle, error) {
+		source.FindBundleThatProvidesStub = func(ctx context.Context, groupOrName, version, kind string, excludedPkgs map[string]struct{}) (*api.Bundle, error) {
 			bundles, ok := bundlesByCatalog[catKey]
 			if !ok {
 				return nil, fmt.Errorf("API (%s/%s/%s) not provided by a package in %s CatalogSource", groupOrName, version, kind, catKey)
 			}
 			sortedBundles := SortBundleInPackageChannel(bundles)
 			for k, v := range sortedBundles {
 				pkgname := getPkgName(k)
-				if pkgname == pkgName {
+				if _, ok := excludedPkgs[pkgname]; ok {
 					continue
 				}
 

pkg/controller/registry/resolver/util_test.go

@@ -3,6 +3,9 @@ package e2e
 import (
 	"context"
 	"fmt"
+	"strings"
+	"time"
+
 	"github.com/blang/semver"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -13,7 +16,6 @@ import (
 	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"time"
 )
 
 var _ = Describe("CRD Versions", func() {
@@ -307,20 +309,25 @@ var _ = Describe("CRD Versions", func() {
 		Expect(err).ToNot(HaveOccurred(), "could not update subscription")
 
 		// fetch new subscription
-		s, err := fetchSubscription(crc, testNamespace, subscriptionName, subscriptionStateAtLatestChecker)
+		s, err := fetchSubscription(crc, testNamespace, subscriptionName, subscriptionHasInstallPlanDifferentChecker(installPlanName))
 		Expect(err).ToNot(HaveOccurred())
 		Expect(s).ToNot(BeNil())
 		Expect(s.Status.InstallPlanRef).ToNot(Equal(nil))
 
 		// Check the error on the installplan - should be related to data loss and the CRD upgrade missing a stored version
 		Eventually(func() bool {
 			ip, err := crc.OperatorsV1alpha1().InstallPlans(testNamespace).Get(context.TODO(), s.Status.InstallPlanRef.Name, metav1.GetOptions{})
-			Expect(err).ToNot(HaveOccurred(), "could not get installplan")
-
-			Expect(ip.Status.Phase).To(Equal(operatorsv1alpha1.InstallPlanPhaseFailed))
-			Expect(ip.Status.Conditions[len(ip.Status.Conditions)-1].Message).To(ContainSubstring("risk of data loss"))
+			if k8serrors.IsNotFound(err) {
+				return false
+			}
+			if ip.Status.Phase != operatorsv1alpha1.InstallPlanPhaseFailed {
+				return false
+			}
+			if !strings.Contains(ip.Status.Conditions[len(ip.Status.Conditions)-1].Message, "risk of data loss") {
+				return false
+			}
 			return true
-		}).Should(BeTrue())
+		}, pollDuration, pollInterval).Should(BeTrue())
 	})
 
 	// Create a CRD on cluster with v1alpha1 (storage)