From 5dabe8f3e91ad59824a2704ab303d156571b51f0 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk Date: Tue, 31 Oct 2023 15:58:46 +0000 Subject: [PATCH] Turn installed package variable source into a func Signed-off-by: Mikalai Radchuk --- .../variablesources/bundle_deployment.go | 30 +++--- .../variablesources/installed_package.go | 100 +++++++++--------- .../variablesources/installed_package_test.go | 77 +++++++++----- 3 files changed, 112 insertions(+), 95 deletions(-) diff --git a/internal/resolution/variablesources/bundle_deployment.go b/internal/resolution/variablesources/bundle_deployment.go index 34e26ee4f..811c1f53d 100644 --- a/internal/resolution/variablesources/bundle_deployment.go +++ b/internal/resolution/variablesources/bundle_deployment.go @@ -3,8 +3,6 @@ package variablesources import ( "context" - "k8s.io/apimachinery/pkg/util/sets" - "github.com/operator-framework/deppy/pkg/deppy" "github.com/operator-framework/deppy/pkg/deppy/input" rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" @@ -34,21 +32,19 @@ func (o *BundleDeploymentVariableSource) GetVariables(ctx context.Context) ([]de variableSources = append(variableSources, o.inputVariableSource) } - processed := sets.Set[string]{} - for _, bundleDeployment := range o.bundleDeployments { - sourceImage := bundleDeployment.Spec.Template.Spec.Source.Image - if sourceImage != nil && sourceImage.Ref != "" { - if processed.Has(sourceImage.Ref) { - continue - } - processed.Insert(sourceImage.Ref) - ips, err := NewInstalledPackageVariableSource(o.allBundles, bundleDeployment.Spec.Template.Spec.Source.Image.Ref) - if err != nil { - return nil, err - } - variableSources = append(variableSources, ips) - } + variables, err := variableSources.GetVariables(ctx) + if err != nil { + return nil, err + } + + // This must go after + installedPackages, err := MakeInstalledPackageVariables(o.allBundles, o.bundleDeployments) + if err != nil { + return nil, err } - return variableSources.GetVariables(ctx) + for _, v := range installedPackages { + variables = append(variables, v) + } + return variables, nil } diff --git a/internal/resolution/variablesources/installed_package.go b/internal/resolution/variablesources/installed_package.go index c545ddb9a..74df66e01 100644 --- a/internal/resolution/variablesources/installed_package.go +++ b/internal/resolution/variablesources/installed_package.go @@ -1,71 +1,71 @@ package variablesources import ( - "context" "fmt" "sort" mmsemver "github.com/Masterminds/semver/v3" - "github.com/operator-framework/deppy/pkg/deppy" - "github.com/operator-framework/deppy/pkg/deppy/input" + "k8s.io/apimachinery/pkg/util/sets" + + rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" "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" + olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables" "github.com/operator-framework/operator-controller/pkg/features" ) -var _ input.VariableSource = &InstalledPackageVariableSource{} - -type InstalledPackageVariableSource struct { - allBundles []*catalogmetadata.Bundle - successors successorsFunc - bundleImage string -} - -func (r *InstalledPackageVariableSource) GetVariables(_ context.Context) ([]deppy.Variable, error) { - // find corresponding bundle for the installed content - resultSet := catalogfilter.Filter(r.allBundles, catalogfilter.WithBundleImage(r.bundleImage)) - if len(resultSet) == 0 { - return nil, r.notFoundError() - } - - // TODO: fast follow - we should check whether we are already supporting the channel attribute in the operator spec. - // if so, we should take the value from spec of the operator CR in the owner ref of the bundle deployment. - // If that channel is set, we need to update the filter above to filter by channel as well. - sort.SliceStable(resultSet, func(i, j int) bool { - return catalogsort.ByVersion(resultSet[i], resultSet[j]) - }) - installedBundle := resultSet[0] - - upgradeEdges, err := r.successors(r.allBundles, installedBundle) - if err != nil { - return nil, err - } - - // you can always upgrade to yourself, i.e. not upgrade - upgradeEdges = append(upgradeEdges, installedBundle) - return []deppy.Variable{ - variables.NewInstalledPackageVariable(installedBundle.Package, upgradeEdges), - }, nil -} - -func (r *InstalledPackageVariableSource) notFoundError() error { - return fmt.Errorf("bundleImage %q not found", r.bundleImage) -} - -func NewInstalledPackageVariableSource(allBundles []*catalogmetadata.Bundle, bundleImage string) (*InstalledPackageVariableSource, error) { - successors := legacySemanticsSuccessors +// MakeInstalledPackageVariables returns variables representing packages +// already installed in the system. +// Meaning that each BundleDeployment managed by operator-controller +// has own variable. +func MakeInstalledPackageVariables( + allBundles []*catalogmetadata.Bundle, + bundleDeployments []rukpakv1alpha1.BundleDeployment, +) ([]*olmvariables.InstalledPackageVariable, error) { + var successors successorsFunc = legacySemanticsSuccessors if features.OperatorControllerFeatureGate.Enabled(features.ForceSemverUpgradeConstraints) { successors = semverSuccessors } - return &InstalledPackageVariableSource{ - allBundles: allBundles, - bundleImage: bundleImage, - successors: successors, - }, nil + result := make([]*olmvariables.InstalledPackageVariable, 0, len(bundleDeployments)) + processed := sets.Set[string]{} + for _, bundleDeployment := range bundleDeployments { + sourceImage := bundleDeployment.Spec.Template.Spec.Source.Image + if sourceImage == nil || sourceImage.Ref == "" { + continue + } + + if processed.Has(sourceImage.Ref) { + continue + } + processed.Insert(sourceImage.Ref) + + bundleImage := sourceImage.Ref + + // find corresponding bundle for the installed content + resultSet := catalogfilter.Filter(allBundles, catalogfilter.WithBundleImage(bundleImage)) + if len(resultSet) == 0 { + return nil, fmt.Errorf("bundleImage %q not found", bundleImage) + } + + sort.SliceStable(resultSet, func(i, j int) bool { + return catalogsort.ByVersion(resultSet[i], resultSet[j]) + }) + installedBundle := resultSet[0] + + upgradeEdges, err := successors(allBundles, installedBundle) + if err != nil { + return nil, err + } + + // you can always upgrade to yourself, i.e. not upgrade + upgradeEdges = append(upgradeEdges, installedBundle) + result = append(result, olmvariables.NewInstalledPackageVariable(installedBundle.Package, upgradeEdges)) + } + + return result, nil } // successorsFunc must return successors of a currently installed bundle diff --git a/internal/resolution/variablesources/installed_package_test.go b/internal/resolution/variablesources/installed_package_test.go index 77e76e59f..dc7679a0d 100644 --- a/internal/resolution/variablesources/installed_package_test.go +++ b/internal/resolution/variablesources/installed_package_test.go @@ -1,24 +1,25 @@ package variablesources_test import ( - "context" "encoding/json" + "fmt" "testing" "github.com/operator-framework/deppy/pkg/deppy" "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-registry/alpha/property" + rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" 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" ) -func TestInstalledPackageVariableSource(t *testing.T) { +func TestMakeInstalledPackageVariables(t *testing.T) { someOtherPackageChannel := catalogmetadata.Channel{Channel: declcfg.Channel{ Name: "stable", Package: "some-other-package", @@ -81,7 +82,7 @@ func TestInstalledPackageVariableSource(t *testing.T) { }, }, }} - bundleList := []*catalogmetadata.Bundle{ + allBundles := []*catalogmetadata.Bundle{ {Bundle: declcfg.Bundle{ Name: "test-package.v0.0.1", Package: "test-package", @@ -201,19 +202,41 @@ func TestInstalledPackageVariableSource(t *testing.T) { }, } + fakeBundleDeployments := func(bundleImages ...string) []rukpakv1alpha1.BundleDeployment { + bundleDeployments := []rukpakv1alpha1.BundleDeployment{} + for idx, bundleImage := range bundleImages { + bd := rukpakv1alpha1.BundleDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("bd-%d", idx), + }, + Spec: rukpakv1alpha1.BundleDeploymentSpec{ + Template: &rukpakv1alpha1.BundleTemplate{ + Spec: rukpakv1alpha1.BundleSpec{ + Source: rukpakv1alpha1.BundleSource{ + Image: &rukpakv1alpha1.ImageSource{ + Ref: bundleImage, + }, + }, + }, + }, + }, + } + bundleDeployments = append(bundleDeployments, bd) + } + + return bundleDeployments + } + t.Run("with ForceSemverUpgradeConstraints feature gate enabled", func(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, true)() t.Run("with non-zero major version", func(t *testing.T) { const bundleImage = "registry.io/repo/test-package@v2.0.0" - ipvs, err := variablesources.NewInstalledPackageVariableSource(bundleList, bundleImage) + installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage)) require.NoError(t, err) - variables, err := ipvs.GetVariables(context.TODO()) - require.NoError(t, err) - require.Len(t, variables, 1) - packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable) - assert.True(t, ok) + require.Len(t, installedPackages, 1) + packageVariable := installedPackages[0] assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier()) // ensure bundles are in version order (high to low) @@ -227,14 +250,11 @@ func TestInstalledPackageVariableSource(t *testing.T) { t.Run("with zero major version", func(t *testing.T) { t.Run("with zero minor version", func(t *testing.T) { const bundleImage = "registry.io/repo/test-package@v0.0.1" - ipvs, err := variablesources.NewInstalledPackageVariableSource(bundleList, bundleImage) + installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage)) require.NoError(t, err) - variables, err := ipvs.GetVariables(context.TODO()) - require.NoError(t, err) - require.Len(t, variables, 1) - packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable) - assert.True(t, ok) + require.Len(t, installedPackages, 1) + packageVariable := installedPackages[0] assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier()) // No upgrades are allowed in major version zero when minor version is also zero @@ -245,14 +265,11 @@ func TestInstalledPackageVariableSource(t *testing.T) { t.Run("with non-zero minor version", func(t *testing.T) { const bundleImage = "registry.io/repo/test-package@v0.1.0" - ipvs, err := variablesources.NewInstalledPackageVariableSource(bundleList, bundleImage) + installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage)) require.NoError(t, err) - variables, err := ipvs.GetVariables(context.TODO()) - require.NoError(t, err) - require.Len(t, variables, 1) - packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable) - assert.True(t, ok) + require.Len(t, installedPackages, 1) + packageVariable := installedPackages[0] assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier()) // Patch version upgrades are allowed, but not minor upgrades @@ -268,14 +285,11 @@ func TestInstalledPackageVariableSource(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, false)() const bundleImage = "registry.io/repo/test-package@v2.0.0" - ipvs, err := variablesources.NewInstalledPackageVariableSource(bundleList, bundleImage) + installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage)) require.NoError(t, err) - variables, err := ipvs.GetVariables(context.TODO()) - require.NoError(t, err) - require.Len(t, variables, 1) - packageVariable, ok := variables[0].(*olmvariables.InstalledPackageVariable) - assert.True(t, ok) + require.Len(t, installedPackages, 1) + packageVariable := installedPackages[0] assert.Equal(t, deppy.IdentifierFromString("installed package test-package"), packageVariable.Identifier()) // ensure bundles are in version order (high to low) @@ -284,4 +298,11 @@ func TestInstalledPackageVariableSource(t *testing.T) { assert.Equal(t, "test-package.v2.1.0", packageVariable.Bundles()[0].Name) assert.Equal(t, "test-package.v2.0.0", packageVariable.Bundles()[1].Name) }) + + t.Run("installed bundle not found", func(t *testing.T) { + const bundleImage = "registry.io/repo/test-package@v9.0.0" + installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, fakeBundleDeployments(bundleImage)) + assert.Nil(t, installedPackages) + assert.ErrorContains(t, err, `bundleImage "registry.io/repo/test-package@v9.0.0" not found`) + }) }