From c250d2d36de34841cdc9461841dc5b4cff9b86d8 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Sat, 8 Jun 2024 13:20:25 -0400 Subject: [PATCH] Fix: installed bundle provider The installed bundle provider, which is necessary for safely handling package upgrades, was too strict. It required the currently installed bundle to exist in the catalog in order to "find" the installed bundle. This is problematic in several situations: - If a user receives a catalog update that no longer contains their installed bundle (perhaps its was pulled from the catalog for security or policy reasons) - If a user is trying to transition to a different catalog that provides that package - If a bundle was directly installed, and the user is trying to have a catalog-backed ClusterExtension adopt it. This change simply returns the name and version of the installed bundle, and makes some minor changes in the successors functions to handle the simplified installed bundle metadata. --- cmd/manager/main.go | 2 +- .../filter/bundle_predicates.go | 9 +- .../filter/bundle_predicates_test.go | 11 +- .../clusterextension_controller.go | 44 ++--- .../clusterextension_controller_test.go | 176 ++++++------------ ...terextension_registryv1_validation_test.go | 1 - internal/controllers/successors.go | 21 ++- internal/controllers/successors_test.go | 58 +++--- internal/controllers/suite_test.go | 9 +- 9 files changed, 125 insertions(+), 206 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index ab677dc38f..bc5f897249 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -188,7 +188,7 @@ func main() { Unpacker: unpacker, Storage: localStorage, Handler: handler.HandlerFunc(handler.HandleClusterExtension), - InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{}, + InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg}, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension") os.Exit(1) diff --git a/internal/catalogmetadata/filter/bundle_predicates.go b/internal/catalogmetadata/filter/bundle_predicates.go index 4dd7559fe4..8f16c6c5e4 100644 --- a/internal/catalogmetadata/filter/bundle_predicates.go +++ b/internal/catalogmetadata/filter/bundle_predicates.go @@ -6,6 +6,7 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" + ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" "github.com/operator-framework/operator-controller/internal/catalogmetadata" ) @@ -66,7 +67,7 @@ func WithBundleName(bundleName string) Predicate[catalogmetadata.Bundle] { } } -func LegacySuccessor(installedBundle *catalogmetadata.Bundle) Predicate[catalogmetadata.Bundle] { +func LegacySuccessor(installedBundle *ocv1alpha1.BundleMetadata) Predicate[catalogmetadata.Bundle] { isSuccessor := func(candidateBundleEntry declcfg.ChannelEntry) bool { if candidateBundleEntry.Replaces == installedBundle.Name { return true @@ -77,9 +78,9 @@ func LegacySuccessor(installedBundle *catalogmetadata.Bundle) Predicate[catalogm } } if candidateBundleEntry.SkipRange != "" { - installedBundleVersion, _ := installedBundle.Version() - skipRange, _ := bsemver.ParseRange(candidateBundleEntry.SkipRange) - if installedBundleVersion != nil && skipRange != nil && skipRange(*installedBundleVersion) { + installedBundleVersion, vErr := bsemver.Parse(installedBundle.Version) + skipRange, srErr := bsemver.ParseRange(candidateBundleEntry.SkipRange) + if vErr == nil && srErr == nil && skipRange(installedBundleVersion) { return true } } diff --git a/internal/catalogmetadata/filter/bundle_predicates_test.go b/internal/catalogmetadata/filter/bundle_predicates_test.go index 6392a01543..51bb5746a2 100644 --- a/internal/catalogmetadata/filter/bundle_predicates_test.go +++ b/internal/catalogmetadata/filter/bundle_predicates_test.go @@ -11,6 +11,7 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-registry/alpha/property" + ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" "github.com/operator-framework/operator-controller/internal/catalogmetadata" "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter" ) @@ -150,13 +151,9 @@ func TestLegacySuccessor(t *testing.T) { }, }, } - installedBundle := &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "package1.v0.0.1", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "0.0.1"}`)}, - }, - }, + installedBundle := &ocv1alpha1.BundleMetadata{ + Name: "package1.v0.0.1", + Version: "0.0.1", } b2 := &catalogmetadata.Bundle{ diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 7b9ff09967..f787157416 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -94,7 +94,7 @@ type ClusterExtensionReconciler struct { } type InstalledBundleGetter interface { - GetInstalledBundle(ctx context.Context, acg helmclient.ActionClientGetter, allBundles []*catalogmetadata.Bundle, ext *ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) + GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) } //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions,verbs=get;list;watch @@ -370,7 +370,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1 channelName := ext.Spec.Channel versionRange := ext.Spec.Version - installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, r.ActionClientGetter, allBundles, &ext) + installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, &ext) if err != nil { return nil, err } @@ -392,7 +392,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1 } if ext.Spec.UpgradeConstraintPolicy != ocv1alpha1.UpgradeConstraintPolicyIgnore && installedBundle != nil { - upgradePredicate, err := SuccessorsPredicate(installedBundle) + upgradePredicate, err := SuccessorsPredicate(ext.Spec.PackageName, installedBundle) if err != nil { return nil, err } @@ -404,7 +404,7 @@ func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext ocv1alpha1 var upgradeErrorPrefix string if installedBundle != nil { - installedBundleVersion, err := installedBundle.Version() + installedBundleVersion, err := mmsemver.NewVersion(installedBundle.Version) if err != nil { return nil, err } @@ -568,6 +568,7 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager) error { r.controller = controller r.cache = mgr.GetCache() r.dynamicWatchGVKs = map[schema.GroupVersionKind]struct{}{} + return nil } @@ -663,10 +664,12 @@ func (r *ClusterExtensionReconciler) getReleaseState(cl helmclient.ActionInterfa return currentRelease, stateUnchanged, nil } -type DefaultInstalledBundleGetter struct{} +type DefaultInstalledBundleGetter struct { + helmclient.ActionClientGetter +} -func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, acg helmclient.ActionClientGetter, allBundles []*catalogmetadata.Bundle, ext *ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) { - cl, err := acg.ActionClientFor(ctx, ext) +func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) { + cl, err := d.ActionClientFor(ctx, ext) if err != nil { return nil, err } @@ -679,27 +682,10 @@ func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, a return nil, nil } - // Bundle must match installed version exactly - vr, err := mmsemver.NewConstraint(release.Labels[labels.BundleVersionKey]) - if err != nil { - return nil, err - } - - // find corresponding bundle for the installed content - resultSet := catalogfilter.Filter(allBundles, catalogfilter.And( - catalogfilter.WithPackageName(release.Labels[labels.PackageNameKey]), - catalogfilter.WithBundleName(release.Labels[labels.BundleNameKey]), - catalogfilter.InMastermindsSemverRange(vr), - )) - if len(resultSet) == 0 { - return nil, fmt.Errorf("bundle %q for package %q not found in available catalogs but is currently installed in namespace %q", release.Labels[labels.BundleNameKey], ext.Spec.PackageName, release.Namespace) - } - - sort.SliceStable(resultSet, func(i, j int) bool { - return catalogsort.ByVersion(resultSet[i], resultSet[j]) - }) - - return resultSet[0], nil + return &ocv1alpha1.BundleMetadata{ + Name: release.Labels[labels.BundleNameKey], + Version: release.Labels[labels.BundleVersionKey], + }, nil } type postrenderer struct { @@ -749,7 +735,7 @@ func bundleMetadataFor(bundle *catalogmetadata.Bundle) *ocv1alpha1.BundleMetadat } func (r *ClusterExtensionReconciler) validateBundle(bundle *catalogmetadata.Bundle) error { - unsupportedProps := sets.New( + unsupportedProps := sets.New[string]( property.TypePackageRequired, property.TypeGVKRequired, property.TypeConstraint, diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index efa1191c8c..ebacd924bc 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -169,7 +169,7 @@ func TestClusterExtensionChannelVersionExists(t *testing.T) { require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) t.Log("By checking the status fields") - require.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) + require.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) require.Empty(t, clusterExtension.Status.InstalledBundle) t.Log("By checking the expected conditions") @@ -227,7 +227,7 @@ func TestClusterExtensionChannelExistsNoVersion(t *testing.T) { require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) t.Log("By checking the status fields") - require.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/2.0.0", Version: "2.0.0"}, clusterExtension.Status.ResolvedBundle) + require.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v2.0.0", Version: "2.0.0"}, clusterExtension.Status.ResolvedBundle) require.Empty(t, clusterExtension.Status.InstalledBundle) t.Log("By checking the expected conditions") @@ -418,22 +418,6 @@ func verifyConditionsInvariants(t *testing.T, ext *ocv1alpha1.ClusterExtension) } func TestClusterExtensionUpgrade(t *testing.T) { - bundle := &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/1.0.0", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake1.0.0", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName":"prometheus","version":"1.0.0"}`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[]`)}, - }, - }, - CatalogName: "fake-catalog", - InChannels: []*catalogmetadata.Channel{&prometheusBetaChannel}, - } - - cl, reconciler := newClientAndReconciler(t, bundle) - mockUnpacker := unpacker.(*MockUnpacker) // Set up the Unpack method to return a result with StateUnpackPending mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*v1alpha2.BundleDeployment")).Return(&source.Result{ @@ -442,6 +426,13 @@ func TestClusterExtensionUpgrade(t *testing.T) { ctx := context.Background() t.Run("semver upgrade constraints enforcement of upgrades within major version", func(t *testing.T) { + bundle := &ocv1alpha1.BundleMetadata{ + Name: "prometheus.v1.0.0", + Version: "1.0.0", + } + + cl, reconciler := newClientAndReconciler(t, bundle) + defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, true)() defer func() { require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) @@ -475,7 +466,7 @@ func TestClusterExtensionUpgrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -489,21 +480,6 @@ func TestClusterExtensionUpgrade(t *testing.T) { err = cl.Update(ctx, clusterExtension) require.NoError(t, err) - bundle := &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/1.0.0", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake1.0.0", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName":"prometheus","version":"1.0.0"}`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[]`)}, - }, - }, - CatalogName: "fake-catalog", - InChannels: []*catalogmetadata.Channel{&prometheusBetaChannel}, - } - - cl, reconciler := newClientAndReconciler(t, bundle) // Run reconcile again res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) require.Error(t, err) @@ -539,7 +515,7 @@ func TestClusterExtensionUpgrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/1.2.0", Version: "1.2.0"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.2.0", Version: "1.2.0"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -550,6 +526,13 @@ func TestClusterExtensionUpgrade(t *testing.T) { }) t.Run("legacy semantics upgrade constraints enforcement", func(t *testing.T) { + bundle := &ocv1alpha1.BundleMetadata{ + Name: "prometheus.v1.0.0", + Version: "1.0.0", + } + + cl, reconciler := newClientAndReconciler(t, bundle) + defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, false)() defer func() { require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) @@ -583,7 +566,7 @@ func TestClusterExtensionUpgrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -597,22 +580,6 @@ func TestClusterExtensionUpgrade(t *testing.T) { err = cl.Update(ctx, clusterExtension) require.NoError(t, err) - bundle := &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/1.0.0", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake1.0.0", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName":"prometheus","version":"1.0.0"}`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[]`)}, - }, - }, - CatalogName: "fake-catalog", - InChannels: []*catalogmetadata.Channel{&prometheusBetaChannel}, - } - - cl, reconciler := newClientAndReconciler(t, bundle) - // Run reconcile again res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) require.Error(t, err) @@ -648,7 +615,7 @@ func TestClusterExtensionUpgrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/1.0.1", Version: "1.0.1"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.1", Version: "1.0.1"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -673,6 +640,13 @@ func TestClusterExtensionUpgrade(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { + bundle := &ocv1alpha1.BundleMetadata{ + Name: "prometheus.v1.0.0", + Version: "1.0.0", + } + + cl, reconciler := newClientAndReconciler(t, bundle) + defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, tt.flagState)() defer func() { require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) @@ -704,7 +678,7 @@ func TestClusterExtensionUpgrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -720,22 +694,6 @@ func TestClusterExtensionUpgrade(t *testing.T) { err = cl.Update(ctx, clusterExtension) require.NoError(t, err) - bundle := &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/1.0.0", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake1.0.0", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName":"prometheus","version":"1.0.0"}`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[]`)}, - }, - }, - CatalogName: "fake-catalog", - InChannels: []*catalogmetadata.Channel{&prometheusBetaChannel}, - } - - cl, reconciler := newClientAndReconciler(t, bundle) - // Run reconcile again res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) require.NoError(t, err) @@ -746,7 +704,7 @@ func TestClusterExtensionUpgrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/2.0.0", Version: "2.0.0"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v2.0.0", Version: "2.0.0"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -760,7 +718,6 @@ func TestClusterExtensionUpgrade(t *testing.T) { } func TestClusterExtensionDowngrade(t *testing.T) { - cl, reconciler := newClientAndReconciler(t, nil) mockUnpacker := unpacker.(*MockUnpacker) // Set up the Unpack method to return a result with StateUnpacked mockUnpacker.On("Unpack", mock.Anything, mock.AnythingOfType("*v1alpha2.BundleDeployment")).Return(&source.Result{ @@ -783,6 +740,13 @@ func TestClusterExtensionDowngrade(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { + bundle := &ocv1alpha1.BundleMetadata{ + Name: "prometheus.v1.0.1", + Version: "1.0.1", + } + + cl, reconciler := newClientAndReconciler(t, bundle) + defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, tt.flagState)() defer func() { require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) @@ -813,7 +777,7 @@ func TestClusterExtensionDowngrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/1.0.1", Version: "1.0.1"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.1", Version: "1.0.1"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -827,22 +791,6 @@ func TestClusterExtensionDowngrade(t *testing.T) { err = cl.Update(ctx, clusterExtension) require.NoError(t, err) - bundle := &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/1.0.1", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake1.0.1", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName":"prometheus","version":"1.0.1"}`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[]`)}, - }, - }, - CatalogName: "fake-catalog", - InChannels: []*catalogmetadata.Channel{&prometheusBetaChannel}, - } - - cl, reconciler := newClientAndReconciler(t, bundle) - // Run reconcile again res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) require.Error(t, err) @@ -881,6 +829,12 @@ func TestClusterExtensionDowngrade(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { + bundle := &ocv1alpha1.BundleMetadata{ + Name: "prometheus.v2.0.0", + Version: "2.0.0", + } + + cl, reconciler := newClientAndReconciler(t, bundle) defer featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.ForceSemverUpgradeConstraints, tt.flagState)() defer func() { require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) @@ -912,7 +866,7 @@ func TestClusterExtensionDowngrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/2.0.0", Version: "2.0.0"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v2.0.0", Version: "2.0.0"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -926,22 +880,6 @@ func TestClusterExtensionDowngrade(t *testing.T) { err = cl.Update(ctx, clusterExtension) require.NoError(t, err) - bundle := &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/2.0.0", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake2.0.0", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName":"prometheus","version":"2.0.0"}`)}, - {Type: property.TypeGVK, Value: json.RawMessage(`[]`)}, - }, - }, - CatalogName: "fake-catalog", - InChannels: []*catalogmetadata.Channel{&prometheusBetaChannel}, - } - - cl, reconciler := newClientAndReconciler(t, bundle) - // Run reconcile again res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) require.NoError(t, err) @@ -952,7 +890,7 @@ func TestClusterExtensionDowngrade(t *testing.T) { require.NoError(t, err) // Checking the status fields - assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "operatorhub/prometheus/beta/1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) + assert.Equal(t, &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle) // checking the expected conditions cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved) @@ -1422,19 +1360,19 @@ var ( Package: "prometheus", Entries: []declcfg.ChannelEntry{ { - Name: "operatorhub/prometheus/beta/1.0.0", + Name: "prometheus.v1.0.0", }, { - Name: "operatorhub/prometheus/beta/1.0.1", - Replaces: "operatorhub/prometheus/beta/1.0.0", + Name: "prometheus.v1.0.1", + Replaces: "prometheus.v1.0.0", }, { - Name: "operatorhub/prometheus/beta/1.2.0", - Replaces: "operatorhub/prometheus/beta/1.0.1", + Name: "prometheus.v1.2.0", + Replaces: "prometheus.v1.0.1", }, { - Name: "operatorhub/prometheus/beta/2.0.0", - Replaces: "operatorhub/prometheus/beta/1.2.0", + Name: "prometheus.v2.0.0", + Replaces: "prometheus.v1.2.0", }, }, }, @@ -1444,7 +1382,7 @@ var ( var testBundleList = []*catalogmetadata.Bundle{ { Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/alpha/0.37.0", + Name: "prometheus.v0.37.0", Package: "prometheus", Image: "quay.io/operatorhubio/prometheus@sha256:3e281e587de3d03011440685fc4fb782672beab044c1ebadc42788ce05a21c35", Properties: []property.Property{ @@ -1457,7 +1395,7 @@ var testBundleList = []*catalogmetadata.Bundle{ }, { Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/1.0.0", + Name: "prometheus.v1.0.0", Package: "prometheus", Image: "quay.io/operatorhubio/prometheus@fake1.0.0", Properties: []property.Property{ @@ -1470,7 +1408,7 @@ var testBundleList = []*catalogmetadata.Bundle{ }, { Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/1.0.1", + Name: "prometheus.v1.0.1", Package: "prometheus", Image: "quay.io/operatorhubio/prometheus@fake1.0.1", Properties: []property.Property{ @@ -1483,7 +1421,7 @@ var testBundleList = []*catalogmetadata.Bundle{ }, { Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/1.2.0", + Name: "prometheus.v1.2.0", Package: "prometheus", Image: "quay.io/operatorhubio/prometheus@fake1.2.0", Properties: []property.Property{ @@ -1496,7 +1434,7 @@ var testBundleList = []*catalogmetadata.Bundle{ }, { Bundle: declcfg.Bundle{ - Name: "operatorhub/prometheus/beta/2.0.0", + Name: "prometheus.v2.0.0", Package: "prometheus", Image: "quay.io/operatorhubio/prometheus@fake2.0.0", Properties: []property.Property{ diff --git a/internal/controllers/clusterextension_registryv1_validation_test.go b/internal/controllers/clusterextension_registryv1_validation_test.go index a828a3e05b..298bd3def1 100644 --- a/internal/controllers/clusterextension_registryv1_validation_test.go +++ b/internal/controllers/clusterextension_registryv1_validation_test.go @@ -109,7 +109,6 @@ func TestClusterExtensionRegistryV1DisallowDependencies(t *testing.T) { }, nil) // Create and configure the mock InstalledBundleGetter mockInstalledBundleGetter := &MockInstalledBundleGetter{} - mockInstalledBundleGetter.SetBundle(tt.bundle) reconciler := &controllers.ClusterExtensionReconciler{ Client: cl, diff --git a/internal/controllers/successors.go b/internal/controllers/successors.go index 04283d70f9..0076b7c207 100644 --- a/internal/controllers/successors.go +++ b/internal/controllers/successors.go @@ -5,18 +5,19 @@ import ( mmsemver "github.com/Masterminds/semver/v3" + ocv1alpha1 "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" "github.com/operator-framework/operator-controller/pkg/features" ) -func SuccessorsPredicate(installedBundle *catalogmetadata.Bundle) (catalogfilter.Predicate[catalogmetadata.Bundle], error) { +func SuccessorsPredicate(packageName string, installedBundle *ocv1alpha1.BundleMetadata) (catalogfilter.Predicate[catalogmetadata.Bundle], error) { var successors successorsPredicateFunc = legacySemanticsSuccessorsPredicate if features.OperatorControllerFeatureGate.Enabled(features.ForceSemverUpgradeConstraints) { successors = semverSuccessorsPredicate } - installedBundleVersion, err := installedBundle.Version() + installedBundleVersion, err := mmsemver.NewVersion(installedBundle.Version) if err != nil { return nil, err } @@ -26,7 +27,7 @@ func SuccessorsPredicate(installedBundle *catalogmetadata.Bundle) (catalogfilter return nil, err } - successorsPredicate, err := successors(installedBundle) + successorsPredicate, err := successors(packageName, installedBundle) if err != nil { return nil, err } @@ -35,7 +36,7 @@ func SuccessorsPredicate(installedBundle *catalogmetadata.Bundle) (catalogfilter return catalogfilter.Or( successorsPredicate, catalogfilter.And( - catalogfilter.WithPackageName(installedBundle.Package), + catalogfilter.WithPackageName(packageName), catalogfilter.InMastermindsSemverRange(installedVersionConstraint), ), ), nil @@ -43,14 +44,14 @@ func SuccessorsPredicate(installedBundle *catalogmetadata.Bundle) (catalogfilter // successorsPredicateFunc returns a predicate to find successors // for a bundle. Predicate must not include the current version. -type successorsPredicateFunc func(bundle *catalogmetadata.Bundle) (catalogfilter.Predicate[catalogmetadata.Bundle], error) +type successorsPredicateFunc func(packageName string, bundle *ocv1alpha1.BundleMetadata) (catalogfilter.Predicate[catalogmetadata.Bundle], error) // legacySemanticsSuccessorsPredicate returns a predicate to find successors // based on legacy OLMv0 semantics which rely on Replaces, Skips and skipRange. -func legacySemanticsSuccessorsPredicate(bundle *catalogmetadata.Bundle) (catalogfilter.Predicate[catalogmetadata.Bundle], error) { +func legacySemanticsSuccessorsPredicate(packageName string, bundle *ocv1alpha1.BundleMetadata) (catalogfilter.Predicate[catalogmetadata.Bundle], error) { // find the bundles that replace, skip, or skipRange the bundle provided return catalogfilter.And( - catalogfilter.WithPackageName(bundle.Package), + catalogfilter.WithPackageName(packageName), catalogfilter.LegacySuccessor(bundle), ), nil } @@ -58,8 +59,8 @@ func legacySemanticsSuccessorsPredicate(bundle *catalogmetadata.Bundle) (catalog // semverSuccessorsPredicate returns a predicate to find successors based on Semver. // Successors will not include versions outside the major version of the // installed bundle as major version is intended to indicate breaking changes. -func semverSuccessorsPredicate(bundle *catalogmetadata.Bundle) (catalogfilter.Predicate[catalogmetadata.Bundle], error) { - currentVersion, err := bundle.Version() +func semverSuccessorsPredicate(packageName string, bundle *ocv1alpha1.BundleMetadata) (catalogfilter.Predicate[catalogmetadata.Bundle], error) { + currentVersion, err := mmsemver.NewVersion(bundle.Version) if err != nil { return nil, err } @@ -73,7 +74,7 @@ func semverSuccessorsPredicate(bundle *catalogmetadata.Bundle) (catalogfilter.Pr } return catalogfilter.And( - catalogfilter.WithPackageName(bundle.Package), + catalogfilter.WithPackageName(packageName), catalogfilter.InMastermindsSemverRange(wantedVersionRangeConstraint), ), nil } diff --git a/internal/controllers/successors_test.go b/internal/controllers/successors_test.go index cda5261add..946dc2a19e 100644 --- a/internal/controllers/successors_test.go +++ b/internal/controllers/successors_test.go @@ -14,6 +14,7 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-registry/alpha/property" + ocv1alpha1 "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" @@ -172,12 +173,12 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsEnabled(t *testing. for _, tt := range []struct { name string - installedBundle *catalogmetadata.Bundle + installedBundle *ocv1alpha1.BundleMetadata expectedResult []*catalogmetadata.Bundle }{ { name: "with non-zero major version", - installedBundle: bundleSet["test-package.v2.0.0"], + installedBundle: bundleMetadataFor(bundleSet["test-package.v2.0.0"]), expectedResult: []*catalogmetadata.Bundle{ // Updates are allowed within the major version bundleSet["test-package.v2.2.0"], @@ -187,7 +188,7 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsEnabled(t *testing. }, { name: "with zero major and zero minor version", - installedBundle: bundleSet["test-package.v0.0.1"], + installedBundle: bundleMetadataFor(bundleSet["test-package.v0.0.1"]), expectedResult: []*catalogmetadata.Bundle{ // No updates are allowed in major version zero when minor version is also zero bundleSet["test-package.v0.0.1"], @@ -195,7 +196,7 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsEnabled(t *testing. }, { name: "with zero major and non-zero minor version", - installedBundle: bundleSet["test-package.v0.1.0"], + installedBundle: bundleMetadataFor(bundleSet["test-package.v0.1.0"]), expectedResult: []*catalogmetadata.Bundle{ // Patch version updates are allowed within the minor version bundleSet["test-package.v0.1.2"], @@ -205,22 +206,15 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsEnabled(t *testing. }, { name: "installed bundle not found", - installedBundle: &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "test-package.v9.0.0", - Package: testPackageName, - Image: "registry.io/repo/test-package@v9.0.0", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "9.0.0"}`)}, - }, - }, - InChannels: []*catalogmetadata.Channel{&testPackageChannel}, + installedBundle: &ocv1alpha1.BundleMetadata{ + Name: "test-package.v9.0.0", + Version: "9.0.0", }, expectedResult: []*catalogmetadata.Bundle{}, }, } { t.Run(tt.name, func(t *testing.T) { - successors, err := controllers.SuccessorsPredicate(tt.installedBundle) + successors, err := controllers.SuccessorsPredicate("test-package", tt.installedBundle) assert.NoError(t, err) result := catalogfilter.Filter(allBundles, successors) @@ -369,12 +363,12 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsDisabled(t *testing for _, tt := range []struct { name string - installedBundle *catalogmetadata.Bundle + installedBundle *ocv1alpha1.BundleMetadata expectedResult []*catalogmetadata.Bundle }{ { name: "respect replaces directive from catalog", - installedBundle: bundleSet["test-package.v2.0.0"], + installedBundle: bundleMetadataFor(bundleSet["test-package.v2.0.0"]), expectedResult: []*catalogmetadata.Bundle{ // Must only have two bundle: // - the one which replaces the current version @@ -385,7 +379,7 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsDisabled(t *testing }, { name: "respect skips directive from catalog", - installedBundle: bundleSet["test-package.v2.2.1"], + installedBundle: bundleMetadataFor(bundleSet["test-package.v2.2.1"]), expectedResult: []*catalogmetadata.Bundle{ // Must only have two bundle: // - the one which skips the current version @@ -396,7 +390,7 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsDisabled(t *testing }, { name: "respect skipRange directive from catalog", - installedBundle: bundleSet["test-package.v2.3.0"], + installedBundle: bundleMetadataFor(bundleSet["test-package.v2.3.0"]), expectedResult: []*catalogmetadata.Bundle{ // Must only have two bundle: // - the one which is skipRanges the current version @@ -407,22 +401,15 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsDisabled(t *testing }, { name: "installed bundle not found", - installedBundle: &catalogmetadata.Bundle{ - Bundle: declcfg.Bundle{ - Name: "test-package.v9.0.0", - Package: testPackageName, - Image: "registry.io/repo/test-package@v9.0.0", - Properties: []property.Property{ - {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "9.0.0"}`)}, - }, - }, - InChannels: []*catalogmetadata.Channel{&testPackageChannel}, + installedBundle: &ocv1alpha1.BundleMetadata{ + Name: "test-package.v9.0.0", + Version: "9.0.0", }, expectedResult: []*catalogmetadata.Bundle{}, }, } { t.Run(tt.name, func(t *testing.T) { - successors, err := controllers.SuccessorsPredicate(tt.installedBundle) + successors, err := controllers.SuccessorsPredicate("test-package", tt.installedBundle) assert.NoError(t, err) result := catalogfilter.Filter(allBundles, successors) @@ -438,3 +425,14 @@ func TestSuccessorsPredicateWithForceSemverUpgradeConstraintsDisabled(t *testing }) } } + +func bundleMetadataFor(bundle *catalogmetadata.Bundle) *ocv1alpha1.BundleMetadata { + if bundle == nil { + return nil + } + v, _ := bundle.Version() + return &ocv1alpha1.BundleMetadata{ + Name: bundle.Name, + Version: v.String(), + } +} diff --git a/internal/controllers/suite_test.go b/internal/controllers/suite_test.go index 6de9560ef9..acb325acda 100644 --- a/internal/controllers/suite_test.go +++ b/internal/controllers/suite_test.go @@ -39,7 +39,6 @@ import ( "github.com/operator-framework/rukpak/pkg/storage" ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" - "github.com/operator-framework/operator-controller/internal/catalogmetadata" "github.com/operator-framework/operator-controller/internal/controllers" "github.com/operator-framework/operator-controller/pkg/scheme" testutil "github.com/operator-framework/operator-controller/test/util" @@ -97,18 +96,18 @@ func newClient(t *testing.T) client.Client { } type MockInstalledBundleGetter struct { - bundle *catalogmetadata.Bundle + bundle *ocv1alpha1.BundleMetadata } -func (m *MockInstalledBundleGetter) SetBundle(bundle *catalogmetadata.Bundle) { +func (m *MockInstalledBundleGetter) SetBundle(bundle *ocv1alpha1.BundleMetadata) { m.bundle = bundle } -func (m *MockInstalledBundleGetter) GetInstalledBundle(ctx context.Context, acg helmclient.ActionClientGetter, allBundles []*catalogmetadata.Bundle, ext *ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) { +func (m *MockInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) { return m.bundle, nil } -func newClientAndReconciler(t *testing.T, bundle *catalogmetadata.Bundle) (client.Client, *controllers.ClusterExtensionReconciler) { +func newClientAndReconciler(t *testing.T, bundle *ocv1alpha1.BundleMetadata) (client.Client, *controllers.ClusterExtensionReconciler) { cl := newClient(t) fakeCatalogClient := testutil.NewFakeCatalogClient(testBundleList)