Add meaningful string representations for resolution invariants.

During dependency resolution, invariants are enforced for
one-operator-per-package and one-provider-per-api. Their user-facing
string representation (shown when they prevent resolution from
succeeding) had re-used the same representation as constraints on
Subscriptions out of convenience.

Recently, the messaging of Subscription-related constraints was
changed to be more descriptive which inadvertently made the invariant
messaging misleading. This patch improves constraint messaging for all
dependency relationships except bundle dependencies.

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

Author: benluddy

pkg/controller/registry/resolver/installabletypes.go

@@ -5,7 +5,6 @@ import (
 	"strings"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
-
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/solver"
 )
 
@@ -37,7 +36,7 @@ func (i *BundleInstallable) AddDependency(dependencies []solver.Identifier) {
 }
 
 func (i *BundleInstallable) BundleSourceInfo() (string, string, registry.CatalogKey, error) {
-	info := strings.Split(string(i.identifier), "/")
+	info := strings.Split(i.identifier.String(), "/")
 	// This should be enforced by Kube naming constraints
 	if len(info) != 4 {
 		return "", "", registry.CatalogKey{}, fmt.Errorf("Unable to parse identifier %s for source info", i.identifier)
@@ -52,7 +51,7 @@ func (i *BundleInstallable) BundleSourceInfo() (string, string, registry.Catalog
 }
 
 func bundleId(bundle, channel string, catalog registry.CatalogKey) solver.Identifier {
-	return solver.Identifier(fmt.Sprintf("%s/%s/%s", catalog.String(), channel, bundle))
+	return solver.IdentifierFromString(fmt.Sprintf("%s/%s/%s", catalog.String(), channel, bundle))
 }
 
 func NewBundleInstallable(bundle, channel string, catalog registry.CatalogKey, constraints ...solver.Constraint) BundleInstallable {
@@ -62,58 +61,100 @@ func NewBundleInstallable(bundle, channel string, catalog registry.CatalogKey, c
 	}
 }
 
-func NewSubscriptionInstallable(pkg string) SubscriptionInstallable {
-	return SubscriptionInstallable{
-		identifier: solver.Identifier(pkg),
-		constraints: []solver.Constraint{
-			ConstraintSubscriptionExists,
-		},
-	}
-}
-
-type SubscriptionInstallable struct {
+type GenericInstallable struct {
 	identifier  solver.Identifier
 	constraints []solver.Constraint
 }
 
-func (r SubscriptionInstallable) Identifier() solver.Identifier {
-	return r.identifier
+func (i GenericInstallable) Identifier() solver.Identifier {
+	return i.identifier
 }
 
-func (r SubscriptionInstallable) Constraints() []solver.Constraint {
-	return r.constraints
+func (i GenericInstallable) Constraints() []solver.Constraint {
+	return i.constraints
 }
 
-func (r *SubscriptionInstallable) AddDependency(dependencies []solver.Identifier) {
+func NewSubscriptionInstallable(name string, dependencies []solver.Identifier) solver.Installable {
+	result := GenericInstallable{
+		identifier: solver.IdentifierFromString(fmt.Sprintf("subscription:%s", name)),
+		constraints: []solver.Constraint{
+			PrettyConstraint(solver.Mandatory(), fmt.Sprintf("subscription %s exists", name)),
+		},
+	}
+
 	if len(dependencies) == 0 {
-		r.constraints = append(r.constraints, ConstraintSubscriptionWithoutCandidates)
-		return
+		result.constraints = append(result.constraints, PrettyConstraint(solver.Dependency(), fmt.Sprintf("no operators found matching the criteria of subscription %s", name)))
+		return result
 	}
 
 	s := make([]string, len(dependencies))
 	for i, each := range dependencies {
-		s[i] = string(each)
+		s[i] = each.String()
+	}
+	var req string
+	if len(s) == 1 {
+		req = s[0]
+	} else {
+		req = fmt.Sprintf("at least one of %s or %s", strings.Join(s[:len(s)-1], ", "), s[len(s)-1])
+	}
+	result.constraints = append(result.constraints, PrettyConstraint(solver.Dependency(dependencies...), fmt.Sprintf("subscription %s requires %s", name, req)))
+
+	return result
+}
+
+func NewSingleAPIProviderInstallable(group, version, kind string, providers []solver.Identifier) solver.Installable {
+	gvk := fmt.Sprintf("%s (%s/%s)", kind, group, version)
+	result := GenericInstallable{
+		identifier: solver.IdentifierFromString(gvk),
+	}
+	if len(providers) <= 1 {
+		// The constraints are pointless without more than one provider.
+		return result
+	}
+	result.constraints = append(result.constraints, PrettyConstraint(solver.Mandatory(), fmt.Sprintf("there can be only one provider of %s", gvk)))
+
+	var s []string
+	for _, p := range providers {
+		s = append(s, p.String())
 	}
-	r.constraints = append(r.constraints, PrettyConstraint(solver.Dependency(dependencies...), fmt.Sprintf("subscription to %%s requires at least one of %s", strings.Join(s, ", "))))
+	msg := fmt.Sprintf("%s and %s provide %s", strings.Join(s[:len(s)-1], ", "), s[len(s)-1], gvk)
+	result.constraints = append(result.constraints, PrettyConstraint(solver.AtMost(1, providers...), msg))
+
+	return result
 }
 
-func PrettyConstraint(c solver.Constraint, format string) solver.Constraint {
+func NewSinglePackageInstanceInstallable(pkg string, providers []solver.Identifier) solver.Installable {
+	result := GenericInstallable{
+		identifier: solver.IdentifierFromString(pkg),
+	}
+	if len(providers) <= 1 {
+		// The constraints are pointless without more than one provider.
+		return result
+	}
+	result.constraints = append(result.constraints, PrettyConstraint(solver.Mandatory(), fmt.Sprintf("there can be only one operator from package %s", pkg)))
+
+	var s []string
+	for _, p := range providers {
+		s = append(s, p.String())
+	}
+	msg := fmt.Sprintf("%s and %s originate from package %s", strings.Join(s[:len(s)-1], ", "), s[len(s)-1], pkg)
+	result.constraints = append(result.constraints, PrettyConstraint(solver.AtMost(1, providers...), msg))
+
+	return result
+}
+
+func PrettyConstraint(c solver.Constraint, msg string) solver.Constraint {
 	return prettyConstraint{
 		Constraint: c,
-		format:     format,
+		msg:        msg,
 	}
 }
 
 type prettyConstraint struct {
 	solver.Constraint
-	format string
+	msg string
 }
 
-func (pc prettyConstraint) String(subject solver.Identifier) string {
-	return fmt.Sprintf(pc.format, subject)
+func (pc prettyConstraint) String(_ solver.Identifier) string {
+	return pc.msg
 }
-
-var (
-	ConstraintSubscriptionExists            = PrettyConstraint(solver.Mandatory(), "a subscription to package %s exists in the namespace")
-	ConstraintSubscriptionWithoutCandidates = PrettyConstraint(solver.Dependency(), "no operators found matching the subscription criteria for %s")
-)

pkg/controller/registry/resolver/resolver.go

@@ -102,7 +102,7 @@ func (r *SatResolver) SolveOperators(namespaces []string, csvs []*v1alpha1.Clust
 		}
 
 		// find operators, in channel order, that can skip from the current version or list the current in "replaces"
-		subInstallables, err := r.getSubscriptionInstallables(pkg, sub.Namespace, current, catalog, predicates, channelFilter, namespacedCache, visited)
+		subInstallables, err := r.getSubscriptionInstallables(sub.Name, pkg, sub.Namespace, current, catalog, predicates, channelFilter, namespacedCache, visited)
 		if err != nil {
 			errs = append(errs, err)
 			continue
@@ -180,13 +180,10 @@ func (r *SatResolver) SolveOperators(namespaces []string, csvs []*v1alpha1.Clust
 	return operators, nil
 }
 
-func (r *SatResolver) getSubscriptionInstallables(pkg, namespace string, current *Operator, catalog registry.CatalogKey, cachePredicates []OperatorPredicate, channelPredicates []OperatorPredicate, namespacedCache MultiCatalogOperatorFinder, visited map[OperatorSurface]*BundleInstallable) (map[solver.Identifier]solver.Installable, error) {
+func (r *SatResolver) getSubscriptionInstallables(name, pkg, namespace string, current *Operator, catalog registry.CatalogKey, cachePredicates []OperatorPredicate, channelPredicates []OperatorPredicate, namespacedCache MultiCatalogOperatorFinder, visited map[OperatorSurface]*BundleInstallable) (map[solver.Identifier]solver.Installable, error) {
 	installables := make(map[solver.Identifier]solver.Installable, 0)
 	candidates := make([]*BundleInstallable, 0)
 
-	subInstallable := NewSubscriptionInstallable(pkg)
-	installables[subInstallable.Identifier()] = &subInstallable
-
 	bundles := namespacedCache.Catalog(catalog).Find(cachePredicates...)
 
 	// bundles in the default channel appear first, then lexicographically order by channel name
@@ -263,7 +260,8 @@ func (r *SatResolver) getSubscriptionInstallables(pkg, namespace string, current
 	}
 
 	// all candidates added as options for this constraint
-	subInstallable.AddDependency(depIds)
+	subInstallable := NewSubscriptionInstallable(name, depIds)
+	installables[subInstallable.Identifier()] = subInstallable
 
 	return installables, nil
 }
@@ -423,7 +421,8 @@ func (r *SatResolver) newSnapshotForNamespace(namespace string, subs []*v1alpha1
 
 func (r *SatResolver) addInvariants(namespacedCache MultiCatalogOperatorFinder, installables map[solver.Identifier]solver.Installable) {
 	// no two operators may provide the same GVK or Package in a namespace
-	conflictToInstallable := make(map[string][]solver.Installable)
+	gvkConflictToInstallable := make(map[opregistry.GVKProperty][]solver.Identifier)
+	packageConflictToInstallable := make(map[string][]solver.Identifier)
 	for _, installable := range installables {
 		bundleInstallable, ok := installable.(*BundleInstallable)
 		if !ok {
@@ -449,12 +448,7 @@ func (r *SatResolver) addInvariants(namespacedCache MultiCatalogOperatorFinder,
 			if err != nil {
 				continue
 			}
-			key := fmt.Sprintf("gvkunique/%s/%s/%s", prop.Group, prop.Version, prop.Kind)
-			_, ok := conflictToInstallable[key]
-			if !ok {
-				conflictToInstallable[key] = make([]solver.Installable, 0)
-			}
-			conflictToInstallable[key] = append(conflictToInstallable[key], installable)
+			gvkConflictToInstallable[prop] = append(gvkConflictToInstallable[prop], installable.Identifier())
 		}
 
 		// cannot have the same package
@@ -467,27 +461,18 @@ func (r *SatResolver) addInvariants(namespacedCache MultiCatalogOperatorFinder,
 			if err != nil {
 				continue
 			}
-			key := fmt.Sprintf("pkgunique/%s", prop.PackageName)
-			_, ok := conflictToInstallable[key]
-			if !ok {
-				conflictToInstallable[key] = make([]solver.Installable, 0)
-			}
-			conflictToInstallable[key] = append(conflictToInstallable[key], installable)
+			packageConflictToInstallable[prop.PackageName] = append(packageConflictToInstallable[prop.PackageName], installable.Identifier())
 		}
 	}
 
-	for key, is := range conflictToInstallable {
-		if len(is) <= 1 {
-			continue
-		}
+	for gvk, is := range gvkConflictToInstallable {
+		s := NewSingleAPIProviderInstallable(gvk.Group, gvk.Version, gvk.Kind, is)
+		installables[s.Identifier()] = s
+	}
 
-		ids := make([]solver.Identifier, 0)
-		for _, d := range is {
-			ids = append(ids, d.Identifier())
-		}
-		s := NewSubscriptionInstallable(key)
-		s.constraints = append(s.constraints, solver.AtMost(1, ids...))
-		installables[s.Identifier()] = &s
+	for pkg, is := range packageConflictToInstallable {
+		s := NewSinglePackageInstanceInstallable(pkg, is)
+		installables[s.Identifier()] = s
 	}
 }
 

pkg/controller/registry/resolver/resolver_test.go

@@ -1,6 +1,7 @@
 package resolver
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/blang/semver/v4"
@@ -1241,34 +1242,36 @@ func TestSolveOperators_TransferApiOwnership(t *testing.T) {
 	}
 
 	var operators OperatorSet
-	for _, p := range phases {
-		fakeNamespacedOperatorCache := NamespacedOperatorCache{
-			snapshots: map[registry.CatalogKey]*CatalogSnapshot{
-				catalog: p.catalog,
-			},
-		}
-		satResolver := SatResolver{
-			cache: getFakeOperatorCache(fakeNamespacedOperatorCache),
-			log:   logrus.New(),
-		}
-		csvs := make([]*v1alpha1.ClusterServiceVersion, 0)
-		for _, o := range operators {
-			var pkg, channel string
-			if b := o.Bundle(); b != nil {
-				pkg = b.PackageName
-				channel = b.ChannelName
+	for i, p := range phases {
+		t.Run(fmt.Sprintf("phase %d", i+1), func(t *testing.T) {
+			fakeNamespacedOperatorCache := NamespacedOperatorCache{
+				snapshots: map[registry.CatalogKey]*CatalogSnapshot{
+					catalog: p.catalog,
+				},
+			}
+			satResolver := SatResolver{
+				cache: getFakeOperatorCache(fakeNamespacedOperatorCache),
+				log:   logrus.New(),
+			}
+			csvs := make([]*v1alpha1.ClusterServiceVersion, 0)
+			for _, o := range operators {
+				var pkg, channel string
+				if b := o.Bundle(); b != nil {
+					pkg = b.PackageName
+					channel = b.ChannelName
+				}
+				csvs = append(csvs, existingOperator(namespace, o.Identifier(), pkg, channel, o.Replaces(), o.ProvidedAPIs(), o.RequiredAPIs(), nil, nil))
 			}
-			csvs = append(csvs, existingOperator(namespace, o.Identifier(), pkg, channel, o.Replaces(), o.ProvidedAPIs(), o.RequiredAPIs(), nil, nil))
-		}
 
-		var err error
-		operators, err = satResolver.SolveOperators([]string{"olm"}, csvs, p.subs)
-		assert.NoError(t, err)
-		for k := range p.expected {
-			require.NotNil(t, operators[k])
-			assert.EqualValues(t, k, operators[k].Identifier())
-		}
-		assert.Equal(t, len(p.expected), len(operators))
+			var err error
+			operators, err = satResolver.SolveOperators([]string{"olm"}, csvs, p.subs)
+			assert.NoError(t, err)
+			for k := range p.expected {
+				require.NotNil(t, operators[k])
+				assert.EqualValues(t, k, operators[k].Identifier())
+			}
+			assert.Equal(t, len(p.expected), len(operators))
+		})
 	}
 }
 

pkg/controller/registry/resolver/solver/installable.go

@@ -8,6 +8,12 @@ func (id Identifier) String() string {
 	return string(id)
 }
 
+// IdentifierFromString returns an Identifier based on a provided
+// string.
+func IdentifierFromString(s string) Identifier {
+	return Identifier(s)
+}
+
 // Installable values are the basic unit of problems and solutions
 // understood by this package.
 type Installable interface {

pkg/controller/registry/resolver/step_resolver_test.go

@@ -98,24 +98,12 @@ func SharedResolverSpecs() []resolverTest {
 			out: resolverTestOut{
 				solverError: solver.NotSatisfiable{
 					{
-						Installable: &SubscriptionInstallable{
-							identifier: "a",
-							constraints: []solver.Constraint{
-								solver.Mandatory(),
-								solver.Dependency(),
-							},
-						},
-						Constraint: ConstraintSubscriptionExists,
+						Installable: NewSubscriptionInstallable("a", nil),
+						Constraint:  PrettyConstraint(solver.Mandatory(), "subscription a-alpha exists"),
 					},
 					{
-						Installable: &SubscriptionInstallable{
-							identifier: "a",
-							constraints: []solver.Constraint{
-								solver.Mandatory(),
-								solver.Dependency(),
-							},
-						},
-						Constraint: ConstraintSubscriptionWithoutCandidates,
+						Installable: NewSubscriptionInstallable("a", nil),
+						Constraint:  PrettyConstraint(solver.Dependency(), "no operators found matching the criteria of subscription a-alpha"),
 					},
 				},
 			},
@@ -267,14 +255,8 @@ func SharedResolverSpecs() []resolverTest {
 				subs:    []*v1alpha1.Subscription{},
 				solverError: solver.NotSatisfiable([]solver.AppliedConstraint{
 					{
-						Installable: &SubscriptionInstallable{
-							identifier: "a",
-							constraints: []solver.Constraint{
-								ConstraintSubscriptionExists,
-								PrettyConstraint(solver.Dependency("catsrc/catsrc-namespace/alpha/a.v1"), "subscription to %s requires at least one of catsrc/catsrc-namespace/alpha/a.v1"),
-							},
-						},
-						Constraint: PrettyConstraint(solver.Dependency("catsrc/catsrc-namespace/alpha/a.v1"), "subscription to %s requires at least one of catsrc/catsrc-namespace/alpha/a.v1"),
+						Installable: NewSubscriptionInstallable("a", []solver.Identifier{"catsrc/catsrc-namespace/alpha/a.v1"}),
+						Constraint:  PrettyConstraint(solver.Dependency("catsrc/catsrc-namespace/alpha/a.v1"), "subscription a-alpha requires catsrc/catsrc-namespace/alpha/a.v1"),
 					},
 					{
 						Installable: &BundleInstallable{
@@ -284,14 +266,8 @@ func SharedResolverSpecs() []resolverTest {
 						Constraint: solver.Dependency(),
 					},
 					{
-						Installable: &SubscriptionInstallable{
-							identifier: "a",
-							constraints: []solver.Constraint{
-								ConstraintSubscriptionExists,
-								PrettyConstraint(solver.Dependency("catsrc/catsrc-namespace/alpha/a.v1"), "subscription to %s requires at least one of catsrc/catsrc-namespace/alpha/a.v1"),
-							},
-						},
-						Constraint: ConstraintSubscriptionExists,
+						Installable: NewSubscriptionInstallable("a", []solver.Identifier{"catsrc/catsrc-namespace/alpha/a.v1"}),
+						Constraint:  PrettyConstraint(solver.Mandatory(), "subscription a-alpha exists"),
 					},
 				}),
 			},