Skip to content

Commit

Permalink
Respect .spec.upgradeConstraintPolicy
Browse files Browse the repository at this point in the history
This commit makes OLM respect `.spec.upgradeConstraintPolicy`
set on an `Operator` CR when chosing upgrade edges.

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
  • Loading branch information
m1kola committed Nov 6, 2023
1 parent cd784a0 commit 55e80d1
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 21 deletions.
2 changes: 1 addition & 1 deletion internal/controllers/variable_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions internal/resolution/variablesources/bundle_deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
}
Expand Down
45 changes: 39 additions & 6 deletions internal/resolution/variablesources/bundle_deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -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() {
Expand Down Expand Up @@ -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())

Expand All @@ -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"))
})
Expand Down
39 changes: 34 additions & 5 deletions internal/resolution/variablesources/installed_package.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -22,16 +24,26 @@ 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
}

result := make([]*olmvariables.InstalledPackageVariable, 0, len(bundleDeployments))
ownerIDToBundleDeployment := mapOwnerIDToBundleDeployment(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
}
Expand All @@ -58,9 +70,14 @@ func MakeInstalledPackageVariables(
})
installedBundle := resultSet[0]

upgradeEdges, err := successors(allBundles, installedBundle)
if err != nil {
return nil, err
// TODO: exclude self!
upgradeEdges := catalogfilter.Filter(allBundles, catalogfilter.WithPackageName(installedBundle.Package))
if operator.Spec.UpgradeConstraintPolicy == operatorsv1alpha1.UpgradeConstraintPolicyEnforce {
var err error
upgradeEdges, err = successors(upgradeEdges, installedBundle)
if err != nil {
return nil, err
}
}

// you can always upgrade to yourself, i.e. not upgrade
Expand Down Expand Up @@ -119,3 +136,15 @@ func semverSuccessors(allBundles []*catalogmetadata.Bundle, installedBundle *cat

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
}
77 changes: 70 additions & 7 deletions internal/resolution/variablesources/installed_package_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
},
Expand All @@ -218,17 +232,34 @@ 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) {
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"
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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -312,12 +349,38 @@ 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.v2.2.0", packageVariable.Bundles()[0].Name)
// assert.Equal(t, "test-package.v2.1.0", packageVariable.Bundles()[1].Name)
// assert.Equal(t, "test-package.v2.0.0", packageVariable.Bundles()[2].Name)
})

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)
Expand Down

0 comments on commit 55e80d1

Please sign in to comment.