Ambiguous Resolution (#1165)

Changes the behavior of catalog resolution by returning an error when multiple packages are available from multiple catalogs.

Signed-off-by: dtfranz <[email protected]>

Author: dtfranz

internal/resolve/catalog.go

@@ -51,8 +51,9 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1alpha1.ClusterEx
 	}
 
 	var (
-		resolvedBundle      *declcfg.Bundle
-		resolvedDeprecation *declcfg.Deprecation
+		resolvedBundles  []*declcfg.Bundle
+		matchedCatalogs  []string
+		priorDeprecation *declcfg.Deprecation
 	)
 
 	listOptions := []client.ListOption{
@@ -109,30 +110,39 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1alpha1.ClusterEx
 		})
 
 		thisBundle := packageFBC.Bundles[0]
-		if resolvedBundle != nil {
-			// Cases where we stick with `resolvedBundle`:
-			//  1. If `thisBundle` is deprecated and `resolvedBundle` is not
-			//  2. If `thisBundle` and `resolvedBundle` have the same deprecation status AND `resolvedBundle` is a higher version
-			if isDeprecated(thisBundle, thisDeprecation) && !isDeprecated(*resolvedBundle, resolvedDeprecation) {
-				return nil
-			}
-			if compare.ByVersion(*resolvedBundle, thisBundle) < 0 {
+
+		if len(resolvedBundles) != 0 {
+			// We've already found one or more package candidates
+			currentIsDeprecated := isDeprecated(thisBundle, thisDeprecation)
+			priorIsDeprecated := isDeprecated(*resolvedBundles[0], priorDeprecation) // Slice index doesn't matter; the whole slice is either deprecated or not
+			if currentIsDeprecated && !priorIsDeprecated {
+				// Skip this deprecated package and retain the non-deprecated package(s)
 				return nil
+			} else if !currentIsDeprecated && priorIsDeprecated {
+				// Our package candidates so far were deprecated and this one is not; clear the lists
+				resolvedBundles = []*declcfg.Bundle{}
+				matchedCatalogs = []string{}
 			}
 		}
-		resolvedBundle = &thisBundle
-		resolvedDeprecation = thisDeprecation
+		// The current bundle shares deprecation status with prior bundles or
+		// there are no prior bundles. Add it to the list.
+		resolvedBundles = append(resolvedBundles, &thisBundle)
+		matchedCatalogs = append(matchedCatalogs, cat.GetName())
+		priorDeprecation = thisDeprecation
 		return nil
 	}, listOptions...); err != nil {
 		return nil, nil, nil, fmt.Errorf("error walking catalogs: %w", err)
 	}
 
-	if resolvedBundle == nil {
+	if len(resolvedBundles) != 1 {
 		errPrefix := ""
 		if installedBundle != nil {
 			errPrefix = fmt.Sprintf("error upgrading from currently installed version %q: ", installedBundle.Version)
 		}
 		switch {
+		case len(resolvedBundles) > 1:
+			slices.Sort(matchedCatalogs) // sort for consistent error message
+			return nil, nil, nil, fmt.Errorf("%smatching packages found in multiple catalogs: %v", errPrefix, matchedCatalogs)
 		case versionRange != "" && channelName != "":
 			return nil, nil, nil, fmt.Errorf("%sno package %q matching version %q in channel %q found", errPrefix, packageName, versionRange, channelName)
 		case versionRange != "":
@@ -143,7 +153,7 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1alpha1.ClusterEx
 			return nil, nil, nil, fmt.Errorf("%sno package %q found", errPrefix, packageName)
 		}
 	}
-
+	resolvedBundle := resolvedBundles[0]
 	resolvedBundleVersion, err := bundleutil.GetVersion(*resolvedBundle)
 	if err != nil {
 		return nil, nil, nil, fmt.Errorf("error getting resolved bundle version for bundle %q: %w", resolvedBundle.Name, err)
@@ -157,7 +167,7 @@ func (r *CatalogResolver) Resolve(ctx context.Context, ext *ocv1alpha1.ClusterEx
 		}
 	}
 
-	return resolvedBundle, resolvedBundleVersion, resolvedDeprecation, nil
+	return resolvedBundle, resolvedBundleVersion, priorDeprecation, nil
 }
 
 func isDeprecated(bundle declcfg.Bundle, deprecation *declcfg.Deprecation) bool {

internal/resolve/catalog_test.go

@@ -12,7 +12,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/rand"
-	"k8s.io/apimachinery/pkg/util/sets"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -262,60 +261,25 @@ func TestPackageVariationsBetweenCatalogs(t *testing.T) {
 	}
 	r := CatalogResolver{WalkCatalogsFunc: w.WalkCatalogs}
 
-	t.Run("always prefer non-deprecated when versions match", func(t *testing.T) {
-		for i := 0; i < 100; i++ {
-			// When the same version exists in both catalogs, we prefer the non-deprecated one.
-			ce := buildFooClusterExtension(pkgName, "", ">=1.0.0 <=1.0.1", ocv1alpha1.UpgradeConstraintPolicyEnforce)
-			gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
-			require.NoError(t, err)
-			assert.Equal(t, genBundle(pkgName, "1.0.1").Name, gotBundle.Name)
-			assert.Equal(t, bsemver.MustParse("1.0.1"), *gotVersion)
-			assert.Nil(t, gotDeprecation)
-		}
-	})
-
 	t.Run("when catalog b has a newer version that matches the range", func(t *testing.T) {
-		// When one version exists in one catalog but not the other, we prefer the one that exists.
 		ce := buildFooClusterExtension(pkgName, "", ">=1.0.0 <=1.0.3", ocv1alpha1.UpgradeConstraintPolicyEnforce)
 		gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
-		require.NoError(t, err)
-		assert.Equal(t, genBundle(pkgName, "1.0.3").Name, gotBundle.Name)
-		assert.Equal(t, genImgRef("catalog-b", gotBundle.Name), gotBundle.Image)
-		assert.Equal(t, bsemver.MustParse("1.0.3"), *gotVersion)
-		assert.Equal(t, ptr.To(packageDeprecation(pkgName)), gotDeprecation)
+		require.Error(t, err)
+		assert.ErrorContains(t, err, "found in multiple catalogs: [b c]")
+		assert.Nil(t, gotBundle)
+		assert.Nil(t, gotVersion)
+		assert.Nil(t, gotDeprecation)
 	})
 
 	t.Run("when catalog c has a newer version that matches the range", func(t *testing.T) {
 		ce := buildFooClusterExtension(pkgName, "", ">=0.1.0 <1.0.0", ocv1alpha1.UpgradeConstraintPolicyEnforce)
 		gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
-		require.NoError(t, err)
-		assert.Equal(t, genBundle(pkgName, "0.1.1").Name, gotBundle.Name)
-		assert.Equal(t, genImgRef("catalog-c", gotBundle.Name), gotBundle.Image)
-		assert.Equal(t, bsemver.MustParse("0.1.1"), *gotVersion)
+		require.Error(t, err)
+		assert.ErrorContains(t, err, "found in multiple catalogs: [b c]")
+		assert.Nil(t, gotBundle)
+		assert.Nil(t, gotVersion)
 		assert.Nil(t, gotDeprecation)
 	})
-
-	t.Run("when there is ambiguity between catalogs", func(t *testing.T) {
-		// When there is no way to disambiguate between two versions, the choice is undefined.
-		foundImages := sets.New[string]()
-		foundDeprecations := sets.New[*declcfg.Deprecation]()
-		for i := 0; i < 100; i++ {
-			ce := buildFooClusterExtension(pkgName, "", "0.1.0", ocv1alpha1.UpgradeConstraintPolicyEnforce)
-			gotBundle, gotVersion, gotDeprecation, err := r.Resolve(context.Background(), ce, nil)
-			require.NoError(t, err)
-			assert.Equal(t, genBundle(pkgName, "0.1.0").Name, gotBundle.Name)
-			assert.Equal(t, bsemver.MustParse("0.1.0"), *gotVersion)
-			foundImages.Insert(gotBundle.Image)
-			foundDeprecations.Insert(gotDeprecation)
-		}
-		assert.ElementsMatch(t, []string{
-			genImgRef("catalog-b", bundleName(pkgName, "0.1.0")),
-			genImgRef("catalog-c", bundleName(pkgName, "0.1.0")),
-		}, foundImages.UnsortedList())
-
-		assert.Contains(t, foundDeprecations, (*declcfg.Deprecation)(nil))
-		assert.Contains(t, foundDeprecations, ptr.To(packageDeprecation(pkgName)))
-	})
 }
 
 func TestUpgradeFoundLegacy(t *testing.T) {

test/e2e/cluster_extension_install_test.go

@@ -306,12 +306,10 @@ func TestClusterExtensionInstallRegistryMultipleBundles(t *testing.T) {
 		if !assert.NotNil(ct, cond) {
 			return
 		}
-		// TODO(tmshort/dtfranz): This should fail due to multiple bundles
-		assert.Equal(ct, metav1.ConditionTrue, cond.Status)
-		//assert.Equal(ct, metav1.ConditionFalse, cond.Status)
-		//assert.Equal(ct, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
-		//assert.Contains(ct, cond.Message, "TODO: matching bundles found in multiple catalogs")
-		//assert.Nil(ct, clusterExtension.Status.ResolvedBundle)
+		assert.Equal(ct, metav1.ConditionFalse, cond.Status)
+		assert.Equal(ct, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
+		assert.Contains(ct, cond.Message, "matching packages found in multiple catalogs")
+		assert.Nil(ct, clusterExtension.Status.ResolvedBundle)
 	}, pollDuration, pollInterval)
 }