use legacy successors, support skips and skipRange (#743)

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

config/manager/manager.yaml

@@ -55,7 +55,6 @@ spec:
         - "--health-probe-bind-address=:8081"
         - "--metrics-bind-address=127.0.0.1:8080"
         - "--leader-elect"
-        - "--feature-gates=ForceSemverUpgradeConstraints=true"
         image: controller:latest
         imagePullPolicy: IfNotPresent
         name: manager

internal/catalogmetadata/filter/bundle_predicates.go

@@ -3,6 +3,7 @@ package filter
 import (
 	mmsemver "github.com/Masterminds/semver/v3"
 	bsemver "github.com/blang/semver/v4"
+	"github.com/operator-framework/operator-registry/alpha/declcfg"
 
 	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 )
@@ -58,11 +59,30 @@ func WithBundleImage(bundleImage string) Predicate[catalogmetadata.Bundle] {
 	}
 }
 
-func Replaces(bundleName string) Predicate[catalogmetadata.Bundle] {
-	return func(bundle *catalogmetadata.Bundle) bool {
-		for _, ch := range bundle.InChannels {
+func LegacySuccessor(installedBundle *catalogmetadata.Bundle) Predicate[catalogmetadata.Bundle] {
+	isSuccessor := func(candidateBundleEntry declcfg.ChannelEntry) bool {
+		if candidateBundleEntry.Replaces == installedBundle.Name {
+			return true
+		}
+		for _, skip := range candidateBundleEntry.Skips {
+			if skip == installedBundle.Name {
+				return true
+			}
+		}
+		if candidateBundleEntry.SkipRange != "" {
+			installedBundleVersion, _ := installedBundle.Version()
+			skipRange, _ := bsemver.ParseRange(candidateBundleEntry.SkipRange)
+			if installedBundleVersion != nil && skipRange != nil && skipRange(*installedBundleVersion) {
+				return true
+			}
+		}
+		return false
+	}
+
+	return func(candidateBundle *catalogmetadata.Bundle) bool {
+		for _, ch := range candidateBundle.InChannels {
 			for _, chEntry := range ch.Entries {
-				if bundle.Name == chEntry.Name && chEntry.Replaces == bundleName {
+				if candidateBundle.Name == chEntry.Name && isSuccessor(chEntry) {
 					return true
 				}
 			}

internal/catalogmetadata/filter/bundle_predicates_test.go

@@ -126,7 +126,7 @@ func TestWithBundleImage(t *testing.T) {
 	assert.False(t, f(b3))
 }
 
-func TestReplaces(t *testing.T) {
+func TestLegacySuccessor(t *testing.T) {
 	fakeChannel := &catalogmetadata.Channel{
 		Channel: declcfg.Channel{
 			Entries: []declcfg.ChannelEntry{
@@ -138,25 +138,51 @@ func TestReplaces(t *testing.T) {
 					Name:     "package1.v0.0.3",
 					Replaces: "package1.v0.0.2",
 				},
+				{
+					Name:  "package1.v0.0.4",
+					Skips: []string{"package1.v0.0.1"},
+				},
+				{
+					Name:      "package1.v0.0.5",
+					SkipRange: "<=0.0.1",
+				},
+			},
+		},
+	}
+	installedBundle := &catalogmetadata.Bundle{
+		Bundle: declcfg.Bundle{
+			Name: "package1.v0.0.1",
+			Properties: []property.Property{
+				{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "0.0.1"}`)},
 			},
 		},
 	}
 
-	b1 := &catalogmetadata.Bundle{
+	b2 := &catalogmetadata.Bundle{
 		Bundle:     declcfg.Bundle{Name: "package1.v0.0.2"},
 		InChannels: []*catalogmetadata.Channel{fakeChannel},
 	}
-	b2 := &catalogmetadata.Bundle{
+	b3 := &catalogmetadata.Bundle{
 		Bundle:     declcfg.Bundle{Name: "package1.v0.0.3"},
 		InChannels: []*catalogmetadata.Channel{fakeChannel},
 	}
-	b3 := &catalogmetadata.Bundle{}
+	b4 := &catalogmetadata.Bundle{
+		Bundle:     declcfg.Bundle{Name: "package1.v0.0.4"},
+		InChannels: []*catalogmetadata.Channel{fakeChannel},
+	}
+	b5 := &catalogmetadata.Bundle{
+		Bundle:     declcfg.Bundle{Name: "package1.v0.0.5"},
+		InChannels: []*catalogmetadata.Channel{fakeChannel},
+	}
+	emptyBundle := &catalogmetadata.Bundle{}
 
-	f := filter.Replaces("package1.v0.0.1")
+	f := filter.LegacySuccessor(installedBundle)
 
-	assert.True(t, f(b1))
-	assert.False(t, f(b2))
+	assert.True(t, f(b2))
 	assert.False(t, f(b3))
+	assert.True(t, f(b4))
+	assert.True(t, f(b5))
+	assert.False(t, f(emptyBundle))
 }
 
 func TestWithDeprecation(t *testing.T) {

internal/controllers/common_controller.go

@@ -22,9 +22,8 @@ import (
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
-
 	ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
+	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 )
 
 // BundleProvider provides the way to retrieve a list of Bundles from a source,
@@ -99,7 +98,7 @@ func setInstalledStatusConditionFailed(conditions *[]metav1.Condition, message s
 	})
 }
 
-// setDEprecationStatusesUnknown sets the deprecation status conditions to unknown.
+// setDeprecationStatusesUnknown sets the deprecation status conditions to unknown.
 func setDeprecationStatusesUnknown(conditions *[]metav1.Condition, message string, generation int64) {
 	conditionTypes := []string{
 		ocv1alpha1.TypeDeprecated,

internal/resolution/variablesources/installed_package.go

@@ -94,11 +94,10 @@ type successorsFunc func(allBundles []*catalogmetadata.Bundle, installedBundle *
 // legacySemanticsSuccessors returns successors based on legacy OLMv0 semantics
 // which rely on Replaces, Skips and skipRange.
 func legacySemanticsSuccessors(allBundles []*catalogmetadata.Bundle, installedBundle *catalogmetadata.Bundle) ([]*catalogmetadata.Bundle, error) {
-	// find the bundles that replace the bundle provided
-	// TODO: this algorithm does not yet consider skips and skipRange
+	// find the bundles that replace, skip, or skipRange the bundle provided
 	upgradeEdges := catalogfilter.Filter(allBundles, catalogfilter.And(
 		catalogfilter.WithPackageName(installedBundle.Package),
-		catalogfilter.Replaces(installedBundle.Name),
+		catalogfilter.LegacySuccessor(installedBundle),
 	))
 	sort.SliceStable(upgradeEdges, func(i, j int) bool {
 		return catalogsort.ByVersion(upgradeEdges[i], upgradeEdges[j])

internal/resolution/variablesources/installed_package_test.go

@@ -306,6 +306,20 @@ func TestMakeInstalledPackageVariablesWithForceSemverUpgradeConstraintsDisabled(
 				Name:     "test-package.v2.2.0",
 				Replaces: "test-package.v2.1.0",
 			},
+			{
+				Name: "test-package.v2.2.1",
+			},
+			{
+				Name:     "test-package.v2.3.0",
+				Replaces: "test-package.v2.2.0",
+				Skips: []string{
+					"test-package.v2.2.1",
+				},
+			},
+			{
+				Name:      "test-package.v2.4.0",
+				SkipRange: ">=2.3.0 <2.4.0",
+			},
 		},
 	}}
 	bundleSet := map[string]*catalogmetadata.Bundle{
@@ -342,6 +356,39 @@ func TestMakeInstalledPackageVariablesWithForceSemverUpgradeConstraintsDisabled(
 			},
 			InChannels: []*catalogmetadata.Channel{&testPackageChannel},
 		},
+		"test-package.v2.2.1": {
+			Bundle: declcfg.Bundle{
+				Name:    "test-package.v2.2.1",
+				Package: testPackageName,
+				Image:   "registry.io/repo/test-package@v2.2.1",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.2.1"}`)},
+				},
+			},
+			InChannels: []*catalogmetadata.Channel{&testPackageChannel},
+		},
+		"test-package.v2.3.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "test-package.v2.3.0",
+				Package: testPackageName,
+				Image:   "registry.io/repo/test-package@v2.3.0",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.3.0"}`)},
+				},
+			},
+			InChannels: []*catalogmetadata.Channel{&testPackageChannel},
+		},
+		"test-package.v2.4.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "test-package.v2.4.0",
+				Package: testPackageName,
+				Image:   "registry.io/repo/test-package@v2.4.0",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.4.0"}`)},
+				},
+			},
+			InChannels: []*catalogmetadata.Channel{&testPackageChannel},
+		},
 		// We need a bundle from different package to ensure that
 		// we filter out bundles certain bundle image
 		"some-other-package.v2.3.0": {
@@ -382,6 +429,34 @@ func TestMakeInstalledPackageVariablesWithForceSemverUpgradeConstraintsDisabled(
 				}),
 			},
 		},
+		{
+			name:                    "respect skips directive from catalog",
+			upgradeConstraintPolicy: ocv1alpha1.UpgradeConstraintPolicyEnforce,
+			installedBundle:         bundleSet["test-package.v2.2.1"],
+			expectedResult: []*olmvariables.InstalledPackageVariable{
+				olmvariables.NewInstalledPackageVariable(testPackageName, []*catalogmetadata.Bundle{
+					// Must only have two bundle:
+					// - the one which skips the current version
+					// - the current version (to allow to stay on the current version)
+					bundleSet["test-package.v2.3.0"],
+					bundleSet["test-package.v2.2.1"],
+				}),
+			},
+		},
+		{
+			name:                    "respect skipRange directive from catalog",
+			upgradeConstraintPolicy: ocv1alpha1.UpgradeConstraintPolicyEnforce,
+			installedBundle:         bundleSet["test-package.v2.3.0"],
+			expectedResult: []*olmvariables.InstalledPackageVariable{
+				olmvariables.NewInstalledPackageVariable(testPackageName, []*catalogmetadata.Bundle{
+					// Must only have two bundle:
+					// - the one which is skipRanges the current version
+					// - the current version (to allow to stay on the current version)
+					bundleSet["test-package.v2.4.0"],
+					bundleSet["test-package.v2.3.0"],
+				}),
+			},
+		},
 		{
 			name:                    "UpgradeConstraintPolicy is set to Ignore",
 			upgradeConstraintPolicy: ocv1alpha1.UpgradeConstraintPolicyIgnore,
@@ -409,6 +484,22 @@ func TestMakeInstalledPackageVariablesWithForceSemverUpgradeConstraintsDisabled(
 			},
 			expectedError: `bundle with image "registry.io/repo/test-package@v9.0.0" for package "test-package" not found in available catalogs but is currently installed via BundleDeployment "test-package-bd"`,
 		},
+		{
+			name:                    "installed bundle not found, but UpgradeConstraintPolicy is set to Ignore",
+			upgradeConstraintPolicy: ocv1alpha1.UpgradeConstraintPolicyIgnore,
+			installedBundle: &catalogmetadata.Bundle{
+				Bundle: declcfg.Bundle{
+					Name:    "test-package.v9.0.0",
+					Package: testPackageName,
+					Image:   "registry.io/repo/test-package@v9.0.0",
+					Properties: []property.Property{
+						{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "9.0.0"}`)},
+					},
+				},
+				InChannels: []*catalogmetadata.Channel{&testPackageChannel},
+			},
+			expectedResult: []*olmvariables.InstalledPackageVariable{},
+		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			fakeOwnerClusterExtension := fakeClusterExtension("test-extension-legacy", testPackageName, tt.upgradeConstraintPolicy)

test/e2e/cluster_extension_install_test.go

@@ -236,7 +236,7 @@ func TestClusterExtensionInstallReResolvesWhenNewCatalog(t *testing.T) {
 	}, pollDuration, pollInterval)
 }
 
-func TestClusterExtensionInstallNonSuccessorVersion(t *testing.T) {
+func TestClusterExtensionBlockInstallNonSuccessorVersion(t *testing.T) {
 	t.Log("When a cluster extension is installed from a catalog")
 	t.Log("When resolving upgrade edges")
 
@@ -264,8 +264,8 @@ func TestClusterExtensionInstallNonSuccessorVersion(t *testing.T) {
 
 	t.Log("It does not allow to upgrade the ClusterExtension to a non-successor version")
 	t.Log("By updating the ClusterExtension resource to a non-successor version")
-	// Semver only allows upgrades within major version at the moment.
-	clusterExtension.Spec.Version = "2.0.0"
+	// 1.2.0 does not replace/skip/skipRange 1.0.0.
+	clusterExtension.Spec.Version = "1.2.0"
 	require.NoError(t, c.Update(context.Background(), clusterExtension))
 	t.Log("By eventually reporting an unsatisfiable resolution")
 	require.EventuallyWithT(t, func(ct *assert.CollectT) {
@@ -276,11 +276,56 @@ func TestClusterExtensionInstallNonSuccessorVersion(t *testing.T) {
 		}
 		assert.Equal(ct, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
 		assert.Contains(ct, cond.Message, "constraints not satisfiable")
-		assert.Contains(ct, cond.Message, "installed package prometheus requires at least one of test-catalog-prometheus-prometheus-operator.1.2.0, test-catalog-prometheus-prometheus-operator.1.0.1, test-catalog-prometheus-prometheus-operator.1.0.0")
+		assert.Contains(ct, cond.Message, "installed package prometheus requires at least one of test-catalog-prometheus-prometheus-operator.1.0.1, test-catalog-prometheus-prometheus-operator.1.0.0")
 		assert.Empty(ct, clusterExtension.Status.ResolvedBundle)
 	}, pollDuration, pollInterval)
 }
 
+func TestClusterExtensionForceInstallNonSuccessorVersion(t *testing.T) {
+	t.Log("When a cluster extension is installed from a catalog")
+	t.Log("When resolving upgrade edges")
+
+	clusterExtension, _, extensionCatalog := testInit(t)
+	defer testCleanup(t, extensionCatalog, clusterExtension)
+	defer getArtifactsOutput(t)
+
+	t.Log("By creating an ClusterExtension at a specified version")
+	clusterExtension.Spec = ocv1alpha1.ClusterExtensionSpec{
+		PackageName: "prometheus",
+		Version:     "1.0.0",
+	}
+	require.NoError(t, c.Create(context.Background(), clusterExtension))
+	t.Log("By eventually reporting a successful resolution")
+	require.EventuallyWithT(t, func(ct *assert.CollectT) {
+		assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
+		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved)
+		if !assert.NotNil(ct, cond) {
+			return
+		}
+		assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
+		assert.Contains(ct, cond.Message, "resolved to")
+		assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle)
+	}, pollDuration, pollInterval)
+
+	t.Log("It does not allow to upgrade the ClusterExtension to a non-successor version")
+	t.Log("By updating the ClusterExtension resource to a non-successor version")
+	// 1.2.0 does not replace/skip/skipRange 1.0.0.
+	clusterExtension.Spec.Version = "1.2.0"
+	clusterExtension.Spec.UpgradeConstraintPolicy = ocv1alpha1.UpgradeConstraintPolicyIgnore
+	require.NoError(t, c.Update(context.Background(), clusterExtension))
+	t.Log("By eventually reporting an unsatisfiable resolution")
+	require.EventuallyWithT(t, func(ct *assert.CollectT) {
+		assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
+		cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved)
+		if !assert.NotNil(ct, cond) {
+			return
+		}
+		assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
+		assert.Contains(ct, cond.Message, "resolved to")
+		assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.2.0", Version: "1.2.0"}, clusterExtension.Status.ResolvedBundle)
+	}, pollDuration, pollInterval)
+}
+
 func TestClusterExtensionInstallSuccessorVersion(t *testing.T) {
 	t.Log("When a cluster extension is installed from a catalog")
 	t.Log("When resolving upgrade edges")
@@ -308,8 +353,8 @@ func TestClusterExtensionInstallSuccessorVersion(t *testing.T) {
 
 	t.Log("It does allow to upgrade the ClusterExtension to any of the successor versions within non-zero major version")
 	t.Log("By updating the ClusterExtension resource by skipping versions")
-	// Test catalog has versions between the initial version and new version
-	clusterExtension.Spec.Version = "1.2.0"
+	// 1.0.1 replaces 1.0.0 in the test catalog
+	clusterExtension.Spec.Version = "1.0.1"
 	require.NoError(t, c.Update(context.Background(), clusterExtension))
 	t.Log("By eventually reporting a successful resolution and bundle path")
 	require.EventuallyWithT(t, func(ct *assert.CollectT) {
@@ -320,7 +365,7 @@ func TestClusterExtensionInstallSuccessorVersion(t *testing.T) {
 		}
 		assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
 		assert.Contains(ct, cond.Message, "resolved to")
-		assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.2.0", Version: "1.2.0"}, clusterExtension.Status.ResolvedBundle)
+		assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.0.1", Version: "1.0.1"}, clusterExtension.Status.ResolvedBundle)
 	}, pollDuration, pollInterval)
 }