Remove CatalogSourceLister usage from resolver/cache. (#2648)

The cache hardcodes the use of a CatalogSourceLister (imported from a non-resolver package in the OLM module) in order to assign source priorities based on the spec.priority field of CatalogSource. Not all cache sources map to a CatalogSource, so this can instead accept a priority provider interface and the resolver -> non-resolver imports can be removed. Signed-off-by: Ben Luddy <bluddy@redhat.com>
operator-framework · Feb 16, 2022 · 5d3d2e7 · 5d3d2e7
1 parent 87bd5a7
commit 5d3d2e7
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 33 deletions.
diff --git a/pkg/controller/registry/resolver/cache/cache.go b/pkg/controller/registry/resolver/cache/cache.go
@@ -10,9 +10,6 @@ import (
 
 	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/util/errors"
-
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"
 )
 
 const existingOperatorKey = "@existing"
@@ -75,14 +72,24 @@ type OperatorCacheProvider interface {
 	Expire(catalog SourceKey)
 }
 
+type SourcePriorityProvider interface {
+	Priority(SourceKey) int
+}
+
+type constantSourcePriorityProvider int
+
+func (spp constantSourcePriorityProvider) Priority(SourceKey) int {
+	return int(spp)
+}
+
 type Cache struct {
-	logger       logrus.StdLogger
-	sp           SourceProvider
-	catsrcLister v1alpha1.CatalogSourceLister
-	snapshots    map[SourceKey]*snapshotHeader
-	ttl          time.Duration
-	sem          chan struct{}
-	m            sync.RWMutex
+	logger                 logrus.StdLogger
+	sp                     SourceProvider
+	sourcePriorityProvider SourcePriorityProvider
+	snapshots              map[SourceKey]*snapshotHeader
+	ttl                    time.Duration
+	sem                    chan struct{}
+	m                      sync.RWMutex
 }
 
 var _ OperatorCacheProvider = &Cache{}
@@ -95,9 +102,9 @@ func WithLogger(logger logrus.StdLogger) Option {
 	}
 }
 
-func WithCatalogSourceLister(catalogSourceLister v1alpha1.CatalogSourceLister) Option {
+func WithSourcePriorityProvider(spp SourcePriorityProvider) Option {
 	return func(c *Cache) {
-		c.catsrcLister = catalogSourceLister
+		c.sourcePriorityProvider = spp
 	}
 }
 
@@ -112,11 +119,11 @@ func New(sp SourceProvider, options ...Option) *Cache {
 			logger.SetOutput(io.Discard)
 			return logger
 		}(),
-		sp:           sp,
-		catsrcLister: operatorlister.NewLister().OperatorsV1alpha1().CatalogSourceLister(),
-		snapshots:    make(map[SourceKey]*snapshotHeader),
-		ttl:          5 * time.Minute,
-		sem:          make(chan struct{}, MaxConcurrentSnapshotUpdates),
+		sp:                     sp,
+		sourcePriorityProvider: constantSourcePriorityProvider(0),
+		snapshots:              make(map[SourceKey]*snapshotHeader),
+		ttl:                    5 * time.Minute,
+		sem:                    make(chan struct{}, MaxConcurrentSnapshotUpdates),
 	}
 
 	for _, opt := range options {
@@ -223,14 +230,10 @@ func (c *Cache) Namespaced(namespaces ...string) MultiCatalogOperatorFinder {
 		ctx, cancel := context.WithTimeout(context.Background(), CachePopulateTimeout)
 
 		hdr := snapshotHeader{
-			key:    miss,
-			expiry: now.Add(c.ttl),
-			pop:    cancel,
-		}
-
-		// Ignoring error and treat catsrc priority as 0 if not found.
-		if catsrc, _ := c.catsrcLister.CatalogSources(miss.Namespace).Get(miss.Name); catsrc != nil {
-			hdr.priority = catsrc.Spec.Priority
+			key:      miss,
+			expiry:   now.Add(c.ttl),
+			pop:      cancel,
+			priority: c.sourcePriorityProvider.Priority(miss),
 		}
 
 		hdr.m.Lock()

diff --git a/pkg/controller/registry/resolver/resolver.go b/pkg/controller/registry/resolver/resolver.go
@@ -13,7 +13,6 @@ import (
 
 	"github.com/operator-framework/api/pkg/constraints"
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
-	v1alpha1listers "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/cache"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/projection"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/solver"
@@ -32,9 +31,9 @@ type SatResolver struct {
 	systemConstraintsProvider solver.ConstraintProvider
 }
 
-func NewDefaultSatResolver(rcp cache.SourceProvider, catsrcLister v1alpha1listers.CatalogSourceLister, logger logrus.FieldLogger) *SatResolver {
+func NewDefaultSatResolver(rcp cache.SourceProvider, sourcePriorityProvider cache.SourcePriorityProvider, logger logrus.FieldLogger) *SatResolver {
 	return &SatResolver{
-		cache: cache.New(rcp, cache.WithLogger(logger), cache.WithCatalogSourceLister(catsrcLister)),
+		cache: cache.New(rcp, cache.WithLogger(logger), cache.WithSourcePriorityProvider(sourcePriorityProvider)),
 		log:   logger,
 		pc: &predicateConverter{
 			celEnv: constraints.NewCelEnvironment(),

diff --git a/pkg/controller/registry/resolver/resolver_test.go b/pkg/controller/registry/resolver/resolver_test.go
@@ -505,7 +505,7 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) {
 	}
 
 	satResolver := SatResolver{
-		cache: cache.New(ssp, cache.WithCatalogSourceLister(&stubCatalogSourceLister{
+		cache: cache.New(ssp, cache.WithSourcePriorityProvider(catsrcPriorityProvider{lister: &stubCatalogSourceLister{
 			catsrcs: []*v1alpha1.CatalogSource{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -517,7 +517,7 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) {
 					},
 				},
 			},
-		})),
+		}})),
 	}
 
 	operators, err := satResolver.SolveOperators([]string{"olm"}, []*v1alpha1.ClusterServiceVersion{}, subs)
@@ -545,7 +545,7 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) {
 	}
 
 	satResolver = SatResolver{
-		cache: cache.New(ssp, cache.WithCatalogSourceLister(&stubCatalogSourceLister{
+		cache: cache.New(ssp, cache.WithSourcePriorityProvider(catsrcPriorityProvider{lister: &stubCatalogSourceLister{
 			catsrcs: []*v1alpha1.CatalogSource{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -566,7 +566,7 @@ func TestSolveOperators_CatsrcPrioritySorting(t *testing.T) {
 					},
 				},
 			},
-		})),
+		}})),
 	}
 
 	operators, err = satResolver.SolveOperators([]string{"olm"}, []*v1alpha1.ClusterServiceVersion{}, subs)

diff --git a/pkg/controller/registry/resolver/step_resolver.go b/pkg/controller/registry/resolver/step_resolver.go
@@ -45,6 +45,18 @@ type OperatorStepResolver struct {
 
 var _ StepResolver = &OperatorStepResolver{}
 
+type catsrcPriorityProvider struct {
+	lister v1alpha1listers.CatalogSourceLister
+}
+
+func (pp catsrcPriorityProvider) Priority(key cache.SourceKey) int {
+	catsrc, err := pp.lister.CatalogSources(key.Namespace).Get(key.Name)
+	if err != nil {
+		return 0
+	}
+	return catsrc.Spec.Priority
+}
+
 func NewOperatorStepResolver(lister operatorlister.OperatorLister, client versioned.Interface, kubeclient kubernetes.Interface,
 	globalCatalogNamespace string, provider RegistryClientProvider, log logrus.FieldLogger) *OperatorStepResolver {
 	stepResolver := &OperatorStepResolver{
@@ -53,7 +65,7 @@ func NewOperatorStepResolver(lister operatorlister.OperatorLister, client versio
 		client:                 client,
 		kubeclient:             kubeclient,
 		globalCatalogNamespace: globalCatalogNamespace,
-		satResolver:            NewDefaultSatResolver(SourceProviderFromRegistryClientProvider(provider, log), lister.OperatorsV1alpha1().CatalogSourceLister(), log),
+		satResolver:            NewDefaultSatResolver(SourceProviderFromRegistryClientProvider(provider, log), catsrcPriorityProvider{lister: lister.OperatorsV1alpha1().CatalogSourceLister()}, log),
 		log:                    log,
 	}