From 0b031ece76a7dec4c365421ac9e15c66231daf93 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk Date: Tue, 10 Oct 2023 12:05:12 +0100 Subject: [PATCH] Refactor legacy upgrade constraint semantics * 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 --- .../variablesources/installed_package.go | 32 ++- .../variablesources/installed_package_test.go | 207 ++++++++++-------- 2 files changed, 141 insertions(+), 98 deletions(-) diff --git a/internal/resolution/variablesources/installed_package.go b/internal/resolution/variablesources/installed_package.go index 3687c04e5..03a7eeafa 100644 --- 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 index 1f435f908..287f476db 100644 --- 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/test-package@v1.0.0", - 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/test-package@v3.0.0", - 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/test-package@v2.0.0", - 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/test-package@v4.0.0", - 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/test-package@v5.0.0", - 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/test-package@v2.0.0" - 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/test-package@v1.0.0", + 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/test-package@v3.0.0", + 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/test-package@v2.0.0", + 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/test-package@v2.1.0", + 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/test-package@v2.2.0", + 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/test-package@v4.0.0", + 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/test-package@v5.0.0", + 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/test-package@v2.0.0" + 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) }) -}) +}