Refactor legacy upgrade constraint semantics (#450)

* Make successors func swappable * Update test to not use Ginkgo & prepare the test to be able to switch between different states of the `ForceSemverUpgradeConstraints` feature flag Signed-off-by: Mikalai Radchuk <[email protected]>
operator-framework · Oct 10, 2023 · 205a487 · 205a487
1 parent 2b0984b
commit 205a487
Show file tree

Hide file tree

Showing 2 changed files with 141 additions and 98 deletions.
diff --git a/internal/resolution/variablesources/installed_package.go b/internal/resolution/variablesources/installed_package.go
@@ -8,6 +8,7 @@ import (
 	"github.com/operator-framework/deppy/pkg/deppy"
 	"github.com/operator-framework/deppy/pkg/deppy/input"
 
+	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 	catalogfilter "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter"
 	catalogsort "github.com/operator-framework/operator-controller/internal/catalogmetadata/sort"
 	"github.com/operator-framework/operator-controller/internal/resolution/variables"
@@ -17,6 +18,7 @@ var _ input.VariableSource = &InstalledPackageVariableSource{}
 
 type InstalledPackageVariableSource struct {
 	catalogClient BundleProvider
+	successors    successorsFunc
 	bundleImage   string
 }
 
@@ -40,13 +42,10 @@ func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]de
 	})
 	installedBundle := resultSet[0]
 
-	// now find the bundles that replace the installed bundle
-	// TODO: this algorithm does not yet consider skips and skipRange
-	//       we simplify the process here by just searching for the bundle that replaces the installed bundle
-	upgradeEdges := catalogfilter.Filter(allBundles, catalogfilter.Replaces(installedBundle.Name))
-	sort.SliceStable(upgradeEdges, func(i, j int) bool {
-		return catalogsort.ByVersion(upgradeEdges[i], upgradeEdges[j])
-	})
+	upgradeEdges, err := r.successors(allBundles, installedBundle)
+	if err != nil {
+		return nil, err
+	}
 
 	// you can always upgrade to yourself, i.e. not upgrade
 	upgradeEdges = append(upgradeEdges, installedBundle)
@@ -63,5 +62,24 @@ func NewInstalledPackageVariableSource(catalogClient BundleProvider, bundleImage
 	return &InstalledPackageVariableSource{
 		catalogClient: catalogClient,
 		bundleImage:   bundleImage,
+		successors:    legacySemanticsSuccessors,
 	}, nil
 }
+
+// successorsFunc must return successors of a currently installed bundle
+// from a list of all bundles provided to the function.
+// Must not return installed bundle as a successor
+type successorsFunc func(allBundles []*catalogmetadata.Bundle, installedBundle *catalogmetadata.Bundle) ([]*catalogmetadata.Bundle, error)
+
+// 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
+	upgradeEdges := catalogfilter.Filter(allBundles, catalogfilter.Replaces(installedBundle.Name))
+	sort.SliceStable(upgradeEdges, func(i, j int) bool {
+		return catalogsort.ByVersion(upgradeEdges[i], upgradeEdges[j])
+	})
+
+	return upgradeEdges, nil
+}
diff --git a/internal/resolution/variablesources/installed_package_test.go b/internal/resolution/variablesources/installed_package_test.go
@@ -3,116 +3,141 @@ package variablesources_test
 import (
 	"context"
 	"encoding/json"
+	"testing"
 
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
 	"github.com/operator-framework/deppy/pkg/deppy"
 	"github.com/operator-framework/operator-registry/alpha/declcfg"
 	"github.com/operator-framework/operator-registry/alpha/property"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 
 	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
 	olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables"
 	"github.com/operator-framework/operator-controller/internal/resolution/variablesources"
+	"github.com/operator-framework/operator-controller/pkg/features"
 	testutil "github.com/operator-framework/operator-controller/test/util"
 )
 
-var _ = Describe("InstalledPackageVariableSource", func() {
-	var (
-		ipvs              *variablesources.InstalledPackageVariableSource
-		fakeCatalogClient testutil.FakeCatalogClient
-		bundleImage       string
-	)
-
-	BeforeEach(func() {
-		channel := catalogmetadata.Channel{Channel: declcfg.Channel{
-			Name: "stable",
-			Entries: []declcfg.ChannelEntry{
-				{
-					Name: "test-package.v1.0.0",
-				},
-				{
-					Name:     "test-package.v2.0.0",
-					Replaces: "test-package.v1.0.0",
-				},
-				{
-					Name:     "test-package.v3.0.0",
-					Replaces: "test-package.v2.0.0",
-				},
-				{
-					Name:     "test-package.v4.0.0",
-					Replaces: "test-package.v3.0.0",
-				},
-				{
-					Name:     "test-package.v5.0.0",
-					Replaces: "test-package.v4.0.0",
-				},
+func TestInstalledPackageVariableSource(t *testing.T) {
+	channel := catalogmetadata.Channel{Channel: declcfg.Channel{
+		Name: "stable",
+		Entries: []declcfg.ChannelEntry{
+			{
+				Name: "test-package.v1.0.0",
 			},
-		}}
-		bundleList := []*catalogmetadata.Bundle{
-			{Bundle: declcfg.Bundle{
-				Name:    "test-package.v1.0.0",
-				Package: "test-package",
-				Image:   "registry.io/repo/[email protected]",
-				Properties: []property.Property{
-					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "1.0.0"}`)},
-				}},
-				InChannels: []*catalogmetadata.Channel{&channel},
+			{
+				Name:     "test-package.v2.0.0",
+				Replaces: "test-package.v1.0.0",
 			},
-			{Bundle: declcfg.Bundle{
-				Name:    "test-package.v3.0.0",
-				Package: "test-package",
-				Image:   "registry.io/repo/[email protected]",
-				Properties: []property.Property{
-					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "3.0.0"}`)},
-				}},
-				InChannels: []*catalogmetadata.Channel{&channel},
+			{
+				Name:     "test-package.v2.1.0",
+				Replaces: "test-package.v2.0.0",
 			},
-			{Bundle: declcfg.Bundle{
-				Name:    "test-package.v2.0.0",
-				Package: "test-package",
-				Image:   "registry.io/repo/[email protected]",
-				Properties: []property.Property{
-					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)},
-				}},
-				InChannels: []*catalogmetadata.Channel{&channel},
+			{
+				Name:     "test-package.v2.2.0",
+				Replaces: "test-package.v2.1.0",
 			},
-			{Bundle: declcfg.Bundle{
-				Name:    "test-package.v4.0.0",
-				Package: "test-package",
-				Image:   "registry.io/repo/[email protected]",
-				Properties: []property.Property{
-					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "4.0.0"}`)},
-				}},
-				InChannels: []*catalogmetadata.Channel{&channel},
+			{
+				Name:     "test-package.v3.0.0",
+				Replaces: "test-package.v2.2.0",
 			},
-			{Bundle: declcfg.Bundle{
-				Name:    "test-package.v5.0.0",
-				Package: "test-package",
-				Image:   "registry.io/repo/[email protected]",
-				Properties: []property.Property{
-					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "5-0.0"}`)},
-				}},
-				InChannels: []*catalogmetadata.Channel{&channel},
+			{
+				Name:     "test-package.v4.0.0",
+				Replaces: "test-package.v3.0.0",
 			},
-		}
-		var err error
-		bundleImage = "registry.io/repo/[email protected]"
-		fakeCatalogClient = testutil.NewFakeCatalogClient(bundleList)
-		ipvs, err = variablesources.NewInstalledPackageVariableSource(&fakeCatalogClient, bundleImage)
-		Expect(err).NotTo(HaveOccurred())
-	})
+			{
+				Name:     "test-package.v5.0.0",
+				Replaces: "test-package.v4.0.0",
+			},
+		},
+	}}
+	bundleList := []*catalogmetadata.Bundle{
+		{Bundle: declcfg.Bundle{
+			Name:    "test-package.v1.0.0",
+			Package: "test-package",
+			Image:   "registry.io/repo/[email protected]",
+			Properties: []property.Property{
+				{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "1.0.0"}`)},
+			}},
+			InChannels: []*catalogmetadata.Channel{&channel},
+		},
+		{Bundle: declcfg.Bundle{
+			Name:    "test-package.v3.0.0",
+			Package: "test-package",
+			Image:   "registry.io/repo/[email protected]",
+			Properties: []property.Property{
+				{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "3.0.0"}`)},
+			}},
+			InChannels: []*catalogmetadata.Channel{&channel},
+		},
+		{Bundle: declcfg.Bundle{
+			Name:    "test-package.v2.0.0",
+			Package: "test-package",
+			Image:   "registry.io/repo/[email protected]",
+			Properties: []property.Property{
+				{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.0.0"}`)},
+			}},
+			InChannels: []*catalogmetadata.Channel{&channel},
+		},
+		{Bundle: declcfg.Bundle{
+			Name:    "test-package.v2.1.0",
+			Package: "test-package",
+			Image:   "registry.io/repo/[email protected]",
+			Properties: []property.Property{
+				{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.1.0"}`)},
+			}},
+			InChannels: []*catalogmetadata.Channel{&channel},
+		},
+		{Bundle: declcfg.Bundle{
+			Name:    "test-package.v2.2.0",
+			Package: "test-package",
+			Image:   "registry.io/repo/[email protected]",
+			Properties: []property.Property{
+				{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "2.2.0"}`)},
+			}},
+			InChannels: []*catalogmetadata.Channel{&channel},
+		},
+		{Bundle: declcfg.Bundle{
+			Name:    "test-package.v4.0.0",
+			Package: "test-package",
+			Image:   "registry.io/repo/[email protected]",
+			Properties: []property.Property{
+				{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "4.0.0"}`)},
+			}},
+			InChannels: []*catalogmetadata.Channel{&channel},
+		},
+		{Bundle: declcfg.Bundle{
+			Name:    "test-package.v5.0.0",
+			Package: "test-package",
+			Image:   "registry.io/repo/[email protected]",
+			Properties: []property.Property{
+				{Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "5-0.0"}`)},
+			}},
+			InChannels: []*catalogmetadata.Channel{&channel},
+		},
+	}
+
+	const bundleImage = "registry.io/repo/[email protected]"
+	fakeCatalogClient := testutil.NewFakeCatalogClient(bundleList)
+
+	t.Run("with ForceSemverUpgradeConstraints feature gate disabled", func(t *testing.T) {
+		defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, false)()
+
+		ipvs, err := variablesources.NewInstalledPackageVariableSource(&fakeCatalogClient, bundleImage)
+		require.NoError(t, err)
 
-	It("should return the correct package variable", func() {
 		variables, err := ipvs.GetVariables(context.TODO())
-		Expect(err).NotTo(HaveOccurred())
-		Expect(variables).To(HaveLen(1))
-		reqPackageVar, ok := variables[0].(*olmvariables.InstalledPackageVariable)
-		Expect(ok).To(BeTrue())
-		Expect(reqPackageVar.Identifier()).To(Equal(deppy.IdentifierFromString("installed package test-package")))
+		require.NoError(t, err)
+		require.Len(t, variables, 1)
+		packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable)
+		assert.True(t, ok)
+		assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier())
 
 		// ensure bundles are in version order (high to low)
-		Expect(reqPackageVar.Bundles()).To(HaveLen(2))
-		Expect(reqPackageVar.Bundles()[0].Name).To(Equal("test-package.v3.0.0"))
-		Expect(reqPackageVar.Bundles()[1].Name).To(Equal("test-package.v2.0.0"))
+		bundles := packageVariable.Bundles()
+		require.Len(t, bundles, 2)
+		assert.Equal(t, "test-package.v2.1.0", packageVariable.Bundles()[0].Name)
+		assert.Equal(t, "test-package.v2.0.0", packageVariable.Bundles()[1].Name)
 	})
-})
+}