From a6a6b9d756c5e74e2525d2987aaa8df07b8d55c8 Mon Sep 17 00:00:00 2001 From: OpenShift Cherrypick Robot Date: Wed, 25 Aug 2021 10:45:14 -0700 Subject: [PATCH] Fix for resolution error on channels with deprecated inner entries. (#2317) 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 Co-authored-by: Ben Luddy --- pkg/controller/registry/resolver/cache.go | 14 ------- .../registry/resolver/installabletypes.go | 19 ++++++++++ pkg/controller/registry/resolver/resolver.go | 19 ++++------ .../registry/resolver/resolver_test.go | 38 ++++++++++++++++--- 4 files changed, 59 insertions(+), 31 deletions(-) diff --git a/pkg/controller/registry/resolver/cache.go b/pkg/controller/registry/resolver/cache.go index e8c0d2d6e0..522af0dfa3 100644 --- a/pkg/controller/registry/resolver/cache.go +++ b/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() { diff --git a/pkg/controller/registry/resolver/installabletypes.go b/pkg/controller/registry/resolver/installabletypes.go index 83e2712b3b..39f2a570af 100644 --- a/pkg/controller/registry/resolver/installabletypes.go +++ b/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), diff --git a/pkg/controller/registry/resolver/resolver.go b/pkg/controller/registry/resolver/resolver.go index 30f9878aac..285cf10596 100644 --- a/pkg/controller/registry/resolver/resolver.go +++ b/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) diff --git a/pkg/controller/registry/resolver/resolver_test.go b/pkg/controller/registry/resolver/resolver_test.go index fde65f1094..52400e4599 100644 --- a/pkg/controller/registry/resolver/resolver_test.go +++ b/pkg/controller/registry/resolver/resolver_test.go @@ -1335,11 +1335,10 @@ 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{ @@ -1347,7 +1346,7 @@ func TestSolveOperators_WithoutDeprecated(t *testing.T) { 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