Merge pull request #1642 from dinhxuanvu/ocs-lib-fix

openshift-merge-robot · web-flow · commit f0495d925d62 · 2020-07-22T05:34:08.000-04:00
Bug 1857424: fix(resolver): Exclude all installed packages in dependency search
diff --git a/pkg/controller/registry/registry_client.go b/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)
diff --git a/pkg/controller/registry/resolver/evolver.go b/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 {
diff --git a/pkg/controller/registry/resolver/fakes/fake_registry_client_interface.go b/pkg/controller/registry/resolver/fakes/fake_registry_client_interface.go
diff --git a/pkg/controller/registry/resolver/querier.go b/pkg/controller/registry/resolver/querier.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
 		}
 	}
diff --git a/pkg/controller/registry/resolver/querier_test.go b/pkg/controller/registry/resolver/querier_test.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)
diff --git a/pkg/controller/registry/resolver/util_test.go b/pkg/controller/registry/resolver/util_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
 				}
 
diff --git a/test/e2e/subscription_e2e_test.go b/test/e2e/subscription_e2e_test.go

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ type SourceRef struct {`
`28`	`28`	`}`
`29`	`29`
`30`	`30`	`type SourceQuerier interface {`
`31`		`- FindProvider(api opregistry.APIKey, initialSource CatalogKey, excludedPkgName string) (api.Bundle, CatalogKey, error)`
	`31`	`+ FindProvider(api opregistry.APIKey, initialSource CatalogKey, excludedPackages map[string]struct{}) (api.Bundle, CatalogKey, error)`
`32`	`32`	`FindBundle(pkgName, channelName, bundleName string, initialSource CatalogKey) (api.Bundle, CatalogKey, error)`
`33`	`33`	`FindLatestBundle(pkgName, channelName string, initialSource CatalogKey) (api.Bundle, CatalogKey, error)`
`34`	`34`	`FindReplacement(currentVersion semver.Version, bundleName, pkgName, channelName string, initialSource CatalogKey) (api.Bundle, *CatalogKey, error)`
`@@ -54,23 +54,23 @@ func (q *NamespaceSourceQuerier) Queryable() error {`
`54`	`54`	`return nil`
`55`	`55`	`}`
`56`	`56`
`57`		`-func (q NamespaceSourceQuerier) FindProvider(api opregistry.APIKey, initialSource CatalogKey, excludedPkgName string) (registryapi.Bundle, *CatalogKey, error) {`
	`57`	`+func (q NamespaceSourceQuerier) FindProvider(api opregistry.APIKey, initialSource CatalogKey, excludedPackages map[string]struct{}) (registryapi.Bundle, *CatalogKey, error) {`
`58`	`58`	`if initialSource.Name != "" && initialSource.Namespace != "" {`
`59`	`59`	`source, ok := q.sources[initialSource]`
`60`	`60`	`if ok {`
`61`		`- if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Group, api.Version, api.Kind, excludedPkgName); err == nil {`
	`61`	`+ if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Group, api.Version, api.Kind, excludedPackages); err == nil {`
`62`	`62`	`return bundle, &initialSource, nil`
`63`	`63`	`}`
`64`		`- if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Plural+"."+api.Group, api.Version, api.Kind, excludedPkgName); err == nil {`
	`64`	`+ if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Plural+"."+api.Group, api.Version, api.Kind, excludedPackages); err == nil {`
`65`	`65`	`return bundle, &initialSource, nil`
`66`	`66`	`}`
`67`	`67`	`}`
`68`	`68`	`}`
`69`	`69`	`for key, source := range q.sources {`
`70`		`- if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Group, api.Version, api.Kind, excludedPkgName); err == nil {`
	`70`	`+ if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Group, api.Version, api.Kind, excludedPackages); err == nil {`
`71`	`71`	`return bundle, &key, nil`
`72`	`72`	`}`
`73`		`- if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Plural+"."+api.Group, api.Version, api.Kind, excludedPkgName); err == nil {`
	`73`	`+ if bundle, err := source.FindBundleThatProvides(context.TODO(), api.Plural+"."+api.Group, api.Version, api.Kind, excludedPackages); err == nil {`
`74`	`74`	`return bundle, &key, nil`
`75`	`75`	`}`
`76`	`76`	`}`
Original file line number	Diff line number	Diff line change
`@@ -121,6 +121,7 @@ func TestNamespaceSourceQuerier_FindProvider(t *testing.T) {`
`121`	`121`	`CatalogKey{"test", "ns"}: &fakeSource,`
`122`	`122`	`CatalogKey{"test2", "ns"}: &fakeSource2,`
`123`	`123`	`}`
	`124`	`+ excludedPkgs := make(map[string]struct{})`
`124`	`125`	`bundle := &api.Bundle{CsvName: "test", PackageName: "testPkg", ChannelName: "testChannel"}`
`125`	`126`	`bundle2 := &api.Bundle{CsvName: "test2", PackageName: "testPkg2", ChannelName: "testChannel2"}`
`126`	`127`	`fakeSource.GetBundleThatProvidesStub = func(ctx context.Context, group, version, kind string) (*api.Bundle, error) {`
`@@ -135,13 +136,13 @@ func TestNamespaceSourceQuerier_FindProvider(t *testing.T) {`
`135`	`136`	`}`
`136`	`137`	`return bundle2, nil`
`137`	`138`	`}`
`138`		`- fakeSource.FindBundleThatProvidesStub = func(ctx context.Context, group, version, kind, pkgName string) (*api.Bundle, error) {`
	`139`	`+ fakeSource.FindBundleThatProvidesStub = func(ctx context.Context, group, version, kind string, excludedPkgs map[string]struct{}) (*api.Bundle, error) {`
`139`	`140`	`if group != "group" \|\| version != "version" \|\| kind != "kind" {`
`140`	`141`	`return nil, fmt.Errorf("Not Found")`
`141`	`142`	`}`
`142`	`143`	`return bundle, nil`
`143`	`144`	`}`
`144`		`- fakeSource2.FindBundleThatProvidesStub = func(ctx context.Context, group, version, kind, pkgName string) (*api.Bundle, error) {`
	`145`	`+ fakeSource2.FindBundleThatProvidesStub = func(ctx context.Context, group, version, kind string, excludedPkgs map[string]struct{}) (*api.Bundle, error) {`
`145`	`146`	`if group != "group2" \|\| version != "version2" \|\| kind != "kind2" {`
`146`	`147`	`return nil, fmt.Errorf("Not Found")`
`147`	`148`	`}`
`@@ -228,7 +229,7 @@ func TestNamespaceSourceQuerier_FindProvider(t *testing.T) {`
`228`	`229`	`q := &NamespaceSourceQuerier{`
`229`	`230`	`sources: tt.fields.sources,`
`230`	`231`	`}`
`231`		`- bundle, key, err := q.FindProvider(tt.args.api, tt.args.catalogKey, "")`
	`232`	`+ bundle, key, err := q.FindProvider(tt.args.api, tt.args.catalogKey, excludedPkgs)`
`232`	`233`	`require.Equal(t, tt.out.err, err)`
`233`	`234`	`require.Equal(t, tt.out.bundle, bundle)`
`234`	`235`	`require.Equal(t, tt.out.key, key)`
Original file line number	Diff line number	Diff line change
`@@ -360,15 +360,15 @@ func NewFakeSourceQuerier(bundlesByCatalog map[CatalogKey][]api.Bundle) Namesp`
`360`	`360`	`return nil, fmt.Errorf("no bundle found")`
`361`	`361`	`}`
`362`	`362`
`363`		`- source.FindBundleThatProvidesStub = func(ctx context.Context, groupOrName, version, kind, pkgName string) (*api.Bundle, error) {`
	`363`	`+ source.FindBundleThatProvidesStub = func(ctx context.Context, groupOrName, version, kind string, excludedPkgs map[string]struct{}) (*api.Bundle, error) {`
`364`	`364`	`bundles, ok := bundlesByCatalog[catKey]`
`365`	`365`	`if !ok {`
`366`	`366`	`return nil, fmt.Errorf("API (%s/%s/%s) not provided by a package in %s CatalogSource", groupOrName, version, kind, catKey)`
`367`	`367`	`}`
`368`	`368`	`sortedBundles := SortBundleInPackageChannel(bundles)`
`369`	`369`	`for k, v := range sortedBundles {`
`370`	`370`	`pkgname := getPkgName(k)`
`371`		`- if pkgname == pkgName {`
	`371`	`+ if _, ok := excludedPkgs[pkgname]; ok {`
`372`	`372`	`continue`
`373`	`373`	`}`
`374`	`374`