From f1d4d8e6a34f711c3cc8e8ca13455e2dbfc72f60 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk Date: Mon, 6 Nov 2023 13:51:53 +0000 Subject: [PATCH] Respect `.spec.upgradeConstraintPolicy` This commit makes OLM respect `.spec.upgradeConstraintPolicy` set on an `Operator` CR when chosing upgrade edges. Signed-off-by: Mikalai Radchuk --- internal/controllers/variable_source.go | 2 +- .../variablesources/bundle_deployment.go | 7 +- .../variablesources/bundle_deployment_test.go | 45 +++++++-- .../variablesources/installed_package.go | 68 +++++++++++-- .../variablesources/installed_package_test.go | 97 +++++++++++++++++-- 5 files changed, 197 insertions(+), 22 deletions(-) diff --git a/internal/controllers/variable_source.go b/internal/controllers/variable_source.go index c1976a1cc..75d647a9b 100644 --- a/internal/controllers/variable_source.go +++ b/internal/controllers/variable_source.go @@ -71,7 +71,7 @@ func (v *VariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, er return variablesources.NewOperatorVariableSource(operatorList.Items, allBundles, inputVariableSource), nil }, func(inputVariableSource input.VariableSource) (input.VariableSource, error) { - return variablesources.NewBundleDeploymentVariableSource(bundleDeploymentList.Items, allBundles, inputVariableSource), nil + return variablesources.NewBundleDeploymentVariableSource(operatorList.Items, bundleDeploymentList.Items, allBundles, inputVariableSource), nil }, func(inputVariableSource input.VariableSource) (input.VariableSource, error) { return variablesources.NewBundlesAndDepsVariableSource(allBundles, inputVariableSource), nil diff --git a/internal/resolution/variablesources/bundle_deployment.go b/internal/resolution/variablesources/bundle_deployment.go index 8b994c25c..c4ceafb18 100644 --- a/internal/resolution/variablesources/bundle_deployment.go +++ b/internal/resolution/variablesources/bundle_deployment.go @@ -7,19 +7,22 @@ import ( "github.com/operator-framework/deppy/pkg/deppy/input" rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" + operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" "github.com/operator-framework/operator-controller/internal/catalogmetadata" ) var _ input.VariableSource = &BundleDeploymentVariableSource{} type BundleDeploymentVariableSource struct { + operators []operatorsv1alpha1.Operator bundleDeployments []rukpakv1alpha1.BundleDeployment allBundles []*catalogmetadata.Bundle inputVariableSource input.VariableSource } -func NewBundleDeploymentVariableSource(bundleDeployments []rukpakv1alpha1.BundleDeployment, allBundles []*catalogmetadata.Bundle, inputVariableSource input.VariableSource) *BundleDeploymentVariableSource { +func NewBundleDeploymentVariableSource(operators []operatorsv1alpha1.Operator, bundleDeployments []rukpakv1alpha1.BundleDeployment, allBundles []*catalogmetadata.Bundle, inputVariableSource input.VariableSource) *BundleDeploymentVariableSource { return &BundleDeploymentVariableSource{ + operators: operators, bundleDeployments: bundleDeployments, allBundles: allBundles, inputVariableSource: inputVariableSource, @@ -37,7 +40,7 @@ func (o *BundleDeploymentVariableSource) GetVariables(ctx context.Context) ([]de return nil, err } - installedPackages, err := MakeInstalledPackageVariables(o.allBundles, o.bundleDeployments) + installedPackages, err := MakeInstalledPackageVariables(o.allBundles, o.operators, o.bundleDeployments) if err != nil { return nil, err } diff --git a/internal/resolution/variablesources/bundle_deployment_test.go b/internal/resolution/variablesources/bundle_deployment_test.go index 1010d0743..a5e356cf1 100644 --- a/internal/resolution/variablesources/bundle_deployment_test.go +++ b/internal/resolution/variablesources/bundle_deployment_test.go @@ -10,18 +10,32 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-registry/alpha/property" + operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" "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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" "github.com/operator-framework/deppy/pkg/deppy" rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" ) -func bundleDeployment(name, image string) rukpakv1alpha1.BundleDeployment { - return rukpakv1alpha1.BundleDeployment{ +func fakeOperator(name, packageName string, upgradeConstraintPolicy operatorsv1alpha1.UpgradeConstraintPolicy) operatorsv1alpha1.Operator { + return operatorsv1alpha1.Operator{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: operatorsv1alpha1.OperatorSpec{ + PackageName: packageName, + UpgradeConstraintPolicy: upgradeConstraintPolicy, + }, + } +} + +func bundleDeployment(name, image string, owner *operatorsv1alpha1.Operator) rukpakv1alpha1.BundleDeployment { + bd := rukpakv1alpha1.BundleDeployment{ ObjectMeta: metav1.ObjectMeta{ Name: name, }, @@ -39,6 +53,21 @@ func bundleDeployment(name, image string) rukpakv1alpha1.BundleDeployment { }, }, } + + if owner != nil { + bd.SetOwnerReferences([]metav1.OwnerReference{ + { + APIVersion: operatorsv1alpha1.GroupVersion.String(), + Kind: "Operator", + Name: owner.Name, + UID: owner.UID, + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), + }, + }) + } + + return bd } var _ = Describe("BundleDeploymentVariableSource", func() { @@ -102,11 +131,13 @@ var _ = Describe("BundleDeploymentVariableSource", func() { }) It("should produce RequiredPackage variables", func() { + fakeOperator := fakeOperator("test-operator", "test-prometheus", operatorsv1alpha1.UpgradeConstraintPolicyEnforce) + operators := []operatorsv1alpha1.Operator{fakeOperator} bundleDeployments := []rukpakv1alpha1.BundleDeployment{ - bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35"), + bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35", &fakeOperator), } - bdVariableSource := variablesources.NewBundleDeploymentVariableSource(bundleDeployments, testBundleList, &MockRequiredPackageSource{}) + bdVariableSource := variablesources.NewBundleDeploymentVariableSource(operators, bundleDeployments, testBundleList, &MockRequiredPackageSource{}) variables, err := bdVariableSource.GetVariables(context.Background()) Expect(err).ToNot(HaveOccurred()) @@ -125,11 +156,13 @@ var _ = Describe("BundleDeploymentVariableSource", func() { }))) }) It("should return an error if the bundleDeployment image doesn't match any operator resource", func() { + fakeOperator := fakeOperator("test-operator", "test-prometheus", operatorsv1alpha1.UpgradeConstraintPolicyEnforce) + operators := []operatorsv1alpha1.Operator{fakeOperator} bundleDeployments := []rukpakv1alpha1.BundleDeployment{ - bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:nonexistent"), + bundleDeployment("prometheus", "quay.io/operatorhubio/prometheus@sha256:nonexistent", &fakeOperator), } - bdVariableSource := variablesources.NewBundleDeploymentVariableSource(bundleDeployments, testBundleList, &MockRequiredPackageSource{}) + bdVariableSource := variablesources.NewBundleDeploymentVariableSource(operators, bundleDeployments, testBundleList, &MockRequiredPackageSource{}) _, err := bdVariableSource.GetVariables(context.Background()) Expect(err.Error()).To(Equal("bundleImage \"quay.io/operatorhubio/prometheus@sha256:nonexistent\" not found")) }) diff --git a/internal/resolution/variablesources/installed_package.go b/internal/resolution/variablesources/installed_package.go index 1f1f3792e..aa09297f0 100644 --- a/internal/resolution/variablesources/installed_package.go +++ b/internal/resolution/variablesources/installed_package.go @@ -5,10 +5,12 @@ import ( "sort" mmsemver "github.com/Masterminds/semver/v3" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" + operatorsv1alpha1 "github.com/operator-framework/operator-controller/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" @@ -22,16 +24,21 @@ import ( // has own variable. func MakeInstalledPackageVariables( allBundles []*catalogmetadata.Bundle, + operators []operatorsv1alpha1.Operator, bundleDeployments []rukpakv1alpha1.BundleDeployment, ) ([]*olmvariables.InstalledPackageVariable, error) { - var successors successorsFunc = legacySemanticsSuccessors - if features.OperatorControllerFeatureGate.Enabled(features.ForceSemverUpgradeConstraints) { - successors = semverSuccessors - } + ownerIDToBundleDeployment := mapOwnerIDToBundleDeployment(bundleDeployments) - result := make([]*olmvariables.InstalledPackageVariable, 0, len(bundleDeployments)) + result := make([]*olmvariables.InstalledPackageVariable, 0, len(operators)) processed := sets.Set[string]{} - for _, bundleDeployment := range bundleDeployments { + for _, operator := range operators { + bundleDeployment, ok := ownerIDToBundleDeployment[operator.UID] + if !ok { + // This can happen when an Operator is requested, + // but not yet installed (e.g. no BundleDeployment created for it) + continue + } + if bundleDeployment.Spec.Template == nil { continue } @@ -58,6 +65,7 @@ func MakeInstalledPackageVariables( }) installedBundle := resultSet[0] + successors := successorsFuncForOperator(operator) upgradeEdges, err := successors(allBundles, installedBundle) if err != nil { return nil, err @@ -76,6 +84,18 @@ func MakeInstalledPackageVariables( // Must not return installed bundle as a successor type successorsFunc func(allBundles []*catalogmetadata.Bundle, installedBundle *catalogmetadata.Bundle) ([]*catalogmetadata.Bundle, error) +func successorsFuncForOperator(operator operatorsv1alpha1.Operator) successorsFunc { + if operator.Spec.UpgradeConstraintPolicy == operatorsv1alpha1.UpgradeConstraintPolicyIgnore { + return ignoreConstraints + } + + if features.OperatorControllerFeatureGate.Enabled(features.ForceSemverUpgradeConstraints) { + return semverSuccessors + } + + return legacySemanticsSuccessors +} + // 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) { @@ -119,3 +139,39 @@ func semverSuccessors(allBundles []*catalogmetadata.Bundle, installedBundle *cat return upgradeEdges, nil } + +// ignoreConstraints returns all available bundles for a package +// as successors of the currently installed bundle. This allows downgrades. +func ignoreConstraints(allBundles []*catalogmetadata.Bundle, installedBundle *catalogmetadata.Bundle) ([]*catalogmetadata.Bundle, error) { + currentVersion, err := installedBundle.Version() + if err != nil { + return nil, err + } + + excludeSelfConstraint, err := mmsemver.NewConstraint(fmt.Sprintf("!= %s", currentVersion.String())) + if err != nil { + return nil, err + } + + upgradeEdges := catalogfilter.Filter(allBundles, catalogfilter.And( + catalogfilter.WithPackageName(installedBundle.Package), + catalogfilter.InMastermindsSemverRange(excludeSelfConstraint), + )) + sort.SliceStable(upgradeEdges, func(i, j int) bool { + return catalogsort.ByVersion(upgradeEdges[i], upgradeEdges[j]) + }) + + return upgradeEdges, nil +} + +func mapOwnerIDToBundleDeployment(bundleDeployments []rukpakv1alpha1.BundleDeployment) map[types.UID]*rukpakv1alpha1.BundleDeployment { + result := map[types.UID]*rukpakv1alpha1.BundleDeployment{} + + for idx := range bundleDeployments { + for _, ref := range bundleDeployments[idx].OwnerReferences { + result[ref.UID] = &bundleDeployments[idx] + } + } + + return result +} diff --git a/internal/resolution/variablesources/installed_package_test.go b/internal/resolution/variablesources/installed_package_test.go index 15937681d..8675dcfe9 100644 --- a/internal/resolution/variablesources/installed_package_test.go +++ b/internal/resolution/variablesources/installed_package_test.go @@ -12,7 +12,9 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/utils/pointer" + operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" "github.com/operator-framework/operator-controller/internal/catalogmetadata" "github.com/operator-framework/operator-controller/internal/resolution/variablesources" "github.com/operator-framework/operator-controller/pkg/features" @@ -201,8 +203,20 @@ func TestMakeInstalledPackageVariables(t *testing.T) { }, } - fakeBundleDeployment := func(name, bundleImage string) rukpakv1alpha1.BundleDeployment { - return rukpakv1alpha1.BundleDeployment{ + fakeOperator := func(name, packageName string, upgradeConstraintPolicy operatorsv1alpha1.UpgradeConstraintPolicy) operatorsv1alpha1.Operator { + return operatorsv1alpha1.Operator{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: operatorsv1alpha1.OperatorSpec{ + PackageName: packageName, + UpgradeConstraintPolicy: upgradeConstraintPolicy, + }, + } + } + + fakeBundleDeployment := func(name, bundleImage string, owner *operatorsv1alpha1.Operator) rukpakv1alpha1.BundleDeployment { + bd := rukpakv1alpha1.BundleDeployment{ ObjectMeta: metav1.ObjectMeta{ Name: name, }, @@ -218,6 +232,21 @@ func TestMakeInstalledPackageVariables(t *testing.T) { }, }, } + + if owner != nil { + bd.SetOwnerReferences([]metav1.OwnerReference{ + { + APIVersion: operatorsv1alpha1.GroupVersion.String(), + Kind: "Operator", + Name: owner.Name, + UID: owner.UID, + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), + }, + }) + } + + return bd } t.Run("with ForceSemverUpgradeConstraints feature gate enabled", func(t *testing.T) { @@ -225,10 +254,12 @@ func TestMakeInstalledPackageVariables(t *testing.T) { t.Run("with non-zero major version", func(t *testing.T) { const bundleImage = "registry.io/repo/test-package@v2.0.0" + fakeOperator := fakeOperator("test-operator", "test-package", operatorsv1alpha1.UpgradeConstraintPolicyEnforce) installedPackages, err := variablesources.MakeInstalledPackageVariables( allBundles, + []operatorsv1alpha1.Operator{fakeOperator}, []rukpakv1alpha1.BundleDeployment{ - fakeBundleDeployment("test-bd", bundleImage), + fakeBundleDeployment("test-package-bd", bundleImage, &fakeOperator), }, ) require.NoError(t, err) @@ -248,10 +279,12 @@ func TestMakeInstalledPackageVariables(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" + fakeOperator := fakeOperator("test-operator", "test-package", operatorsv1alpha1.UpgradeConstraintPolicyEnforce) installedPackages, err := variablesources.MakeInstalledPackageVariables( allBundles, + []operatorsv1alpha1.Operator{fakeOperator}, []rukpakv1alpha1.BundleDeployment{ - fakeBundleDeployment("test-bd", bundleImage), + fakeBundleDeployment("test-package-bd", bundleImage, &fakeOperator), }, ) require.NoError(t, err) @@ -268,10 +301,12 @@ func TestMakeInstalledPackageVariables(t *testing.T) { t.Run("with non-zero minor version", func(t *testing.T) { const bundleImage = "registry.io/repo/test-package@v0.1.0" + fakeOperator := fakeOperator("test-operator", "test-package", operatorsv1alpha1.UpgradeConstraintPolicyEnforce) installedPackages, err := variablesources.MakeInstalledPackageVariables( allBundles, + []operatorsv1alpha1.Operator{fakeOperator}, []rukpakv1alpha1.BundleDeployment{ - fakeBundleDeployment("test-bd", bundleImage), + fakeBundleDeployment("test-package-bd", bundleImage, &fakeOperator), }, ) require.NoError(t, err) @@ -293,10 +328,12 @@ func TestMakeInstalledPackageVariables(t *testing.T) { defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, false)() const bundleImage = "registry.io/repo/test-package@v2.0.0" + fakeOperator := fakeOperator("test-operator", "test-package", operatorsv1alpha1.UpgradeConstraintPolicyEnforce) installedPackages, err := variablesources.MakeInstalledPackageVariables( allBundles, + []operatorsv1alpha1.Operator{fakeOperator}, []rukpakv1alpha1.BundleDeployment{ - fakeBundleDeployment("test-bd", bundleImage), + fakeBundleDeployment("test-package-bd", bundleImage, &fakeOperator), }, ) require.NoError(t, err) @@ -312,12 +349,58 @@ func TestMakeInstalledPackageVariables(t *testing.T) { assert.Equal(t, "test-package.v2.0.0", packageVariable.Bundles()[1].Name) }) + t.Run("UpgradeConstraintPolicy is set to Ignore", func(t *testing.T) { + const bundleImage = "registry.io/repo/test-package@v2.0.0" + fakeOperator := fakeOperator("test-operator", "test-package", operatorsv1alpha1.UpgradeConstraintPolicyIgnore) + installedPackages, err := variablesources.MakeInstalledPackageVariables( + allBundles, + []operatorsv1alpha1.Operator{fakeOperator}, + []rukpakv1alpha1.BundleDeployment{ + fakeBundleDeployment("test-package-bd", bundleImage, &fakeOperator), + }, + ) + require.NoError(t, err) + + 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) + bundles := packageVariable.Bundles() + require.Len(t, bundles, 12) + assert.Equal(t, "test-package.v5.0.0", bundles[0].Name) + assert.Equal(t, "test-package.v4.0.0", bundles[1].Name) + assert.Equal(t, "test-package.v3.0.0", bundles[2].Name) + assert.Equal(t, "test-package.v2.2.0", bundles[3].Name) + assert.Equal(t, "test-package.v2.1.0", bundles[4].Name) + assert.Equal(t, "test-package.v1.0.0", bundles[5].Name) + assert.Equal(t, "test-package.v0.2.0", bundles[6].Name) + assert.Equal(t, "test-package.v0.1.1", bundles[7].Name) + assert.Equal(t, "test-package.v0.1.0", bundles[8].Name) + assert.Equal(t, "test-package.v0.0.2", bundles[9].Name) + assert.Equal(t, "test-package.v0.0.1", bundles[10].Name) + assert.Equal(t, "test-package.v2.0.0", bundles[11].Name) + }) + + t.Run("no BundleDeployment for an Operator", func(t *testing.T) { + fakeOperator := fakeOperator("test-operator", "test-package", operatorsv1alpha1.UpgradeConstraintPolicyEnforce) + installedPackages, err := variablesources.MakeInstalledPackageVariables( + allBundles, + []operatorsv1alpha1.Operator{fakeOperator}, + []rukpakv1alpha1.BundleDeployment{}, + ) + assert.NoError(t, err) + assert.Empty(t, installedPackages) + }) + t.Run("installed bundle not found", func(t *testing.T) { const bundleImage = "registry.io/repo/test-package@v9.0.0" + fakeOperator := fakeOperator("test-operator", "test-package", operatorsv1alpha1.UpgradeConstraintPolicyEnforce) installedPackages, err := variablesources.MakeInstalledPackageVariables( allBundles, + []operatorsv1alpha1.Operator{fakeOperator}, []rukpakv1alpha1.BundleDeployment{ - fakeBundleDeployment("test-bd", bundleImage), + fakeBundleDeployment("test-package-bd", bundleImage, &fakeOperator), }, ) assert.Nil(t, installedPackages)