Fix for resolution error on channels with deprecated inner entries. (#2313)

Resolution would fail with an error in the presence of channels
containing more than two entries with a deprecated inner
entry (e.g. V1 -> V2 -> V3, with deprecated V2). This was due to
special handling that filtered out deprecated bundles at the cache
query layer. A channel's complete replaces-chain must be returned from
cache queries in order to correctly determine the relative order
between entries in the channel.

Signed-off-by: Ben Luddy <[email protected]>

Author: benluddy

pkg/controller/registry/resolver/cache.go

@@ -392,9 +392,6 @@ type OperatorPredicate func(*Operator) bool
 func (s *CatalogSnapshot) Find(p ...OperatorPredicate) []*Operator {
 	s.m.RLock()
 	defer s.m.RUnlock()
-	if len(p) > 0 {
-		p = append(p, WithoutDeprecatedProperty())
-	}
 	return Filter(s.operators, p...)
 }
 
@@ -453,17 +450,6 @@ func WithPackage(pkg string) OperatorPredicate {
 	}
 }
 
-func WithoutDeprecatedProperty() OperatorPredicate {
-	return func(o *Operator) bool {
-		for _, p := range o.bundle.GetProperties() {
-			if p.GetType() == opregistry.DeprecatedType {
-				return false
-			}
-		}
-		return true
-	}
-}
-
 func WithVersionInRange(r semver.Range) OperatorPredicate {
 	return func(o *Operator) bool {
 		for _, p := range o.Properties() {

pkg/controller/registry/resolver/installabletypes.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/solver"
+	operatorregistry "github.com/operator-framework/operator-registry/pkg/registry"
 )
 
 type BundleInstallable struct {
@@ -54,6 +55,24 @@ func bundleId(bundle, channel string, catalog registry.CatalogKey) solver.Identi
 	return solver.IdentifierFromString(fmt.Sprintf("%s/%s/%s", catalog.String(), channel, bundle))
 }
 
+func NewBundleInstallableFromOperator(o *Operator) (BundleInstallable, error) {
+	src := o.SourceInfo()
+	if src == nil {
+		return BundleInstallable{}, fmt.Errorf("unable to resolve the source of bundle %s", o.Identifier())
+	}
+	var constraints []solver.Constraint
+	for _, p := range o.bundle.GetProperties() {
+		if p.GetType() == operatorregistry.DeprecatedType {
+			constraints = append(constraints, PrettyConstraint(
+				solver.Prohibited(),
+				fmt.Sprintf("bundle %s is deprecated", bundleId(o.Identifier(), o.Channel(), src.Catalog)),
+			))
+			break
+		}
+	}
+	return NewBundleInstallable(o.Identifier(), o.Channel(), src.Catalog, constraints...), nil
+}
+
 func NewBundleInstallable(bundle, channel string, catalog registry.CatalogKey, constraints ...solver.Constraint) BundleInstallable {
 	return BundleInstallable{
 		identifier:  bundleId(bundle, channel, catalog),

pkg/controller/registry/resolver/resolver.go

@@ -294,19 +294,17 @@ func (r *SatResolver) getBundleInstallables(catalog registry.CatalogKey, predica
 		bundle := bundleStack[len(bundleStack)-1]
 		bundleStack = bundleStack[:len(bundleStack)-1]
 
-		bundleSource := bundle.SourceInfo()
-		if bundleSource == nil {
-			err := fmt.Errorf("unable to resolve the source of bundle %s, invalid cache", bundle.Identifier())
-			errs = append(errs, err)
+		if b, ok := visited[bundle]; ok {
+			installables[b.identifier] = b
 			continue
 		}
 
-		if b, ok := visited[bundle]; ok {
-			installables[b.identifier] = b
+		bundleInstallable, err := NewBundleInstallableFromOperator(bundle)
+		if err != nil {
+			errs = append(errs, err)
 			continue
 		}
 
-		bundleInstallable := NewBundleInstallable(bundle.Identifier(), bundle.Channel(), bundleSource.Catalog)
 		visited[bundle] = &bundleInstallable
 
 		dependencyPredicates, err := bundle.DependencyPredicates()
@@ -348,14 +346,11 @@ func (r *SatResolver) getBundleInstallables(catalog registry.CatalogKey, predica
 			// (after sorting) to remove all bundles that
 			// don't satisfy the dependency.
 			for _, b := range Filter(sortedBundles, d) {
-				src := b.SourceInfo()
-				if src == nil {
-					err := fmt.Errorf("unable to resolve the source of bundle %s, invalid cache", bundle.Identifier())
+				i, err := NewBundleInstallableFromOperator(b)
+				if err != nil {
 					errs = append(errs, err)
 					continue
 				}
-
-				i := NewBundleInstallable(b.Identifier(), b.Channel(), src.Catalog)
 				installables[i.Identifier()] = &i
 				bundleDependencies = append(bundleDependencies, i.Identifier())
 				bundleStack = append(bundleStack, b)

pkg/controller/registry/resolver/resolver_test.go

@@ -1335,19 +1335,18 @@ func stripBundle(o *Operator) *Operator {
 }
 
 func TestSolveOperators_WithoutDeprecated(t *testing.T) {
-	namespace := "olm"
-	catalog := registry.CatalogKey{"community", namespace}
+	catalog := registry.CatalogKey{Name: "catalog", Namespace: "namespace"}
 
 	subs := []*v1alpha1.Subscription{
-		newSub(namespace, "packageA", "alpha", catalog),
+		newSub(catalog.Namespace, "packageA", "alpha", catalog),
 	}
 
 	fakeNamespacedOperatorCache := NamespacedOperatorCache{
 		snapshots: map[registry.CatalogKey]*CatalogSnapshot{
 			catalog: {
 				key: catalog,
 				operators: []*Operator{
-					genOperator("packageA.v1", "0.0.1", "packageA.v1", "packageA", "alpha", "community", "olm", nil, nil, nil, "", true),
+					genOperator("packageA.v1", "0.0.1", "packageA.v1", "packageA", "alpha", catalog.Name, catalog.Namespace, nil, nil, nil, "", true),
 				},
 			},
 		},
@@ -1357,11 +1356,40 @@ func TestSolveOperators_WithoutDeprecated(t *testing.T) {
 		log:   logrus.New(),
 	}
 
-	operators, err := satResolver.SolveOperators([]string{"olm"}, nil, subs)
+	operators, err := satResolver.SolveOperators([]string{catalog.Namespace}, nil, subs)
 	assert.Empty(t, operators)
 	assert.IsType(t, solver.NotSatisfiable{}, err)
 }
 
+func TestSolveOperatorsWithDeprecatedInnerChannelEntry(t *testing.T) {
+	catalog := registry.CatalogKey{Name: "catalog", Namespace: "namespace"}
+
+	subs := []*v1alpha1.Subscription{
+		newSub(catalog.Namespace, "a", "c", catalog),
+	}
+	logger, _ := test.NewNullLogger()
+	resolver := SatResolver{
+		cache: getFakeOperatorCache(NamespacedOperatorCache{
+			snapshots: map[registry.CatalogKey]*CatalogSnapshot{
+				catalog: {
+					key: catalog,
+					operators: []*Operator{
+						genOperator("a-1", "1.0.0", "", "a", "c", catalog.Name, catalog.Namespace, nil, nil, nil, "", false),
+						genOperator("a-2", "2.0.0", "a-1", "a", "c", catalog.Name, catalog.Namespace, nil, nil, nil, "", true),
+						genOperator("a-3", "3.0.0", "a-2", "a", "c", catalog.Name, catalog.Namespace, nil, nil, nil, "", false),
+					},
+				},
+			},
+		}),
+		log: logger,
+	}
+
+	operators, err := resolver.SolveOperators([]string{catalog.Namespace}, nil, subs)
+	assert.NoError(t, err)
+	assert.Len(t, operators, 1)
+	assert.Contains(t, operators, "a-3")
+}
+
 func TestSolveOperators_WithSkipsAndStartingCSV(t *testing.T) {
 	APISet := APISet{opregistry.APIKey{"g", "v", "k", "ks"}: struct{}{}}
 	Provides := APISet