diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index f340520fc..593e5482e 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go @@ -62,10 +62,11 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e // is fairly decoupled from this code where we get the annotations back out. We may want to co-locate // the set/get logic a bit better to make it more maintainable and less likely to get out of sync. rm := &RevisionMetadata{ - RevisionName: rev.Name, - Package: rev.Annotations[labels.PackageNameKey], - Image: rev.Annotations[labels.BundleReferenceKey], - Conditions: rev.Status.Conditions, + RevisionName: rev.Name, + Package: rev.Annotations[labels.PackageNameKey], + Image: rev.Annotations[labels.BundleReferenceKey], + CatalogSpecDigest: rev.Annotations[labels.CatalogSpecDigestKey], + Conditions: rev.Status.Conditions, BundleMetadata: ocv1.BundleMetadata{ Name: rev.Annotations[labels.BundleNameKey], Version: rev.Annotations[labels.BundleVersionKey], @@ -104,10 +105,11 @@ func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, e return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) { l := log.FromContext(ctx) revisionAnnotations := map[string]string{ - labels.BundleNameKey: state.resolvedRevisionMetadata.Name, - labels.PackageNameKey: state.resolvedRevisionMetadata.Package, - labels.BundleVersionKey: state.resolvedRevisionMetadata.Version, - labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image, + labels.BundleNameKey: state.resolvedRevisionMetadata.Name, + labels.PackageNameKey: state.resolvedRevisionMetadata.Package, + labels.BundleVersionKey: state.resolvedRevisionMetadata.Version, + labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image, + labels.CatalogSpecDigestKey: CatalogSpecDigest(ext), } if state.resolvedRevisionMetadata.Release != nil { revisionAnnotations[labels.BundleReleaseKey] = *state.resolvedRevisionMetadata.Release diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 6645d392f..360f70cab 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -499,9 +499,10 @@ func clusterExtensionRequestsForCatalog(c client.Reader, logger logr.Logger) crh } type RevisionMetadata struct { - RevisionName string - Package string - Image string + RevisionName string + Package string + Image string + CatalogSpecDigest string ocv1.BundleMetadata Conditions []metav1.Condition } diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index 2726fe7c8..3cea001f6 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -781,28 +781,6 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) }) - cl, reconciler := newClientAndReconciler(t, func(d *deps) { - // Boxcutter keeps a rolling revision when apply fails. We mirror that state so the test uses - // the same inputs the runtime would see. - d.RevisionStatesGetter = &MockRevisionStatesGetter{ - RevisionStates: &controllers.RevisionStates{ - RollingOut: []*controllers.RevisionMetadata{{}}, - }, - } - d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { - v := bundle.VersionRelease{ - Version: bsemver.MustParse("1.0.0"), - } - return &declcfg.Bundle{ - Name: "prometheus.v1.0.0", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake1.0.0", - }, &v, nil, nil - }) - d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} - d.Applier = &MockApplier{err: errors.New("boxcutter apply failure")} - }) - ctx := context.Background() extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} @@ -824,6 +802,38 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te }, }, } + specDigest := catalogSpecDigestFor(clusterExtension.Spec.Source.Catalog) + + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + // Boxcutter keeps a rolling revision when apply fails. We mirror that state so the test uses + // the same inputs the runtime would see. The revision's CatalogSpecDigest must match the spec so + // the controller recognizes this as a still-valid rollout (not a spec change). + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + RollingOut: []*controllers.RevisionMetadata{{ + Package: "prometheus", + Image: "quay.io/operatorhubio/prometheus@fake1.0.0", + CatalogSpecDigest: specDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "prometheus.v1.0.0", + Version: "1.0.0", + }, + }}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + v := bundle.VersionRelease{ + Version: bsemver.MustParse("1.0.0"), + } + return &declcfg.Bundle{ + Name: "prometheus.v1.0.0", + Package: "prometheus", + Image: "quay.io/operatorhubio/prometheus@fake1.0.0", + }, &v, nil, nil + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{err: errors.New("boxcutter apply failure")} + }) require.NoError(t, cl.Create(ctx, clusterExtension)) res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) @@ -871,6 +881,643 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) } +// TestClusterExtensionSpecMismatchWhileRollingOutReResolvesBundle verifies that when a +// ClusterExtension's spec requests a different version than the revision currently rolling out, +// the controller re-resolves the bundle from the catalog instead of continuing to use the stale revision. +// +// Scenario: +// - A revision for v1.0.2 is rolling out (e.g., stuck due to deployment probe failure) +// - The ClusterExtension spec already requests v1.0.3 at reconcile time (e.g., previously updated by the user) +// - The controller detects the spec/revision mismatch and re-resolves from the catalog +// - The resolver returns v1.0.3, which is used for the new rollout +func TestClusterExtensionSpecMismatchWhileRollingOutReResolvesBundle(t *testing.T) { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.BoxcutterRuntime))) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) + }) + + oldSpecDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package", Version: "1.0.2"}) + + resolverCalled := false + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + Installed: &controllers.RevisionMetadata{ + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.1-nginxolm88138", + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.1", + Version: "1.0.1", + }, + }, + RollingOut: []*controllers.RevisionMetadata{{ + RevisionName: "extension-88138-2", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-test-package", + CatalogSpecDigest: oldSpecDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.2", + Version: "1.0.2", + }, + }}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + resolverCalled = true + v := bundle.VersionRelease{ + Version: bsemver.MustParse("1.0.3"), + } + return &declcfg.Bundle{ + Name: "test-package.v1.0.3", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.3-nginxolm88138", + }, &v, nil, nil + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{installCompleted: true} + }) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Version: "1.0.3", + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.NoError(t, err) + + require.True(t, resolverCalled, + "resolver should be called because the rolling out revision (v1.0.2) does not match the spec (v1.0.3)") + + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + + installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) + require.NotNil(t, installedCond) + require.Equal(t, metav1.ConditionTrue, installedCond.Status) + require.Contains(t, installedCond.Message, "1.0.3") + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} + +// TestClusterExtensionRollingOutRevisionMatchesSpecNoReResolve verifies that when a +// revision is rolling out and the spec hasn't changed, the controller uses the existing +// rolling out revision without re-resolving from the catalog. +func TestClusterExtensionRollingOutRevisionMatchesSpecNoReResolve(t *testing.T) { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.BoxcutterRuntime))) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) + }) + + specDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package", Version: "1.0.2"}) + + resolverCalled := false + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + RollingOut: []*controllers.RevisionMetadata{{ + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-test-package", + CatalogSpecDigest: specDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.2", + Version: "1.0.2", + }, + }}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + resolverCalled = true + v := bundle.VersionRelease{ + Version: bsemver.MustParse("1.0.2"), + } + return &declcfg.Bundle{ + Name: "test-package.v1.0.2", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-test-package", + }, &v, nil, nil + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{installCompleted: true} + }) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Version: "1.0.2", + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.NoError(t, err) + + require.False(t, resolverCalled, + "resolver should NOT be called because the rolling out revision (v1.0.2) matches the spec (v1.0.2)") + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} + +// TestClusterExtensionMultipleRollingOutRevisionsUsesLatestMatch verifies that when multiple +// revisions are rolling out (e.g., a stuck revision and a new one created after re-resolution), +// the controller picks the latest revision that matches the spec. +// +// This covers the lifecycle after re-resolution: the stuck revision (v1.0.2) is still active, +// a new revision (v1.0.1) was created. The revision controller will archive the old one once +// the new one succeeds, but in the meantime both are in the RollingOut list. +func TestClusterExtensionMultipleRollingOutRevisionsUsesLatestMatch(t *testing.T) { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.BoxcutterRuntime))) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) + }) + + oldSpecDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package", Version: "1.0.2"}) + currentSpecDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package", Version: "1.0.1"}) + + resolverCalled := false + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + Installed: &controllers.RevisionMetadata{ + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.0-nginxolm88138", + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.0", + Version: "1.0.0", + }, + }, + RollingOut: []*controllers.RevisionMetadata{ + { + RevisionName: "extension-88138-2", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-test-package", + CatalogSpecDigest: oldSpecDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.2", + Version: "1.0.2", + }, + }, + { + RevisionName: "extension-88138-3", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.1-nginxolm88138", + CatalogSpecDigest: currentSpecDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.1", + Version: "1.0.1", + }, + }, + }, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + resolverCalled = true + return nil, nil, nil, fmt.Errorf("should not be called") + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{installCompleted: true} + }) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Version: "1.0.1", + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.NoError(t, err) + + require.False(t, resolverCalled, + "resolver should NOT be called: the latest rolling-out revision (v1.0.1) matches the spec, "+ + "even though an older stuck revision (v1.0.2) does not match") + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} + +// TestClusterExtensionReResolvesWithInstalledAsFromVersion verifies that when re-resolving +// during a stuck rollout, the resolver receives the Installed revision (A) as the +// installedBundle parameter — not the rolling-out revision (B). This ensures upgrade +// constraints are checked against the last known-good state. +func TestClusterExtensionReResolvesWithInstalledAsFromVersion(t *testing.T) { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.BoxcutterRuntime))) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) + }) + + oldSpecDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package", Version: "1.0.2"}) + + var capturedInstalledBundle *ocv1.BundleMetadata + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + Installed: &controllers.RevisionMetadata{ + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.0-nginxolm88138", + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.0", + Version: "1.0.0", + }, + }, + RollingOut: []*controllers.RevisionMetadata{{ + RevisionName: "extension-88138-2", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-test-package", + CatalogSpecDigest: oldSpecDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.2", + Version: "1.0.2", + }, + }}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + capturedInstalledBundle = installedBundle + v := bundle.VersionRelease{ + Version: bsemver.MustParse("1.0.1"), + } + return &declcfg.Bundle{ + Name: "test-package.v1.0.1", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.1-nginxolm88138", + }, &v, nil, nil + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{installCompleted: true} + }) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Version: "1.0.1", + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.NoError(t, err) + + require.NotNil(t, capturedInstalledBundle, + "resolver should receive an installedBundle") + require.Equal(t, "test-package.v1.0.0", capturedInstalledBundle.Name, + "resolver should receive the Installed version (v1.0.0), not the rolling-out version (v1.0.2)") + require.Equal(t, "1.0.0", capturedInstalledBundle.Version) + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} + +// TestClusterExtensionNoInstalledRevisionSpecMismatchReResolves verifies that when there +// is no installed revision (first install attempt got stuck) and the spec changes, +// the controller re-resolves with nil as the installedBundle. +func TestClusterExtensionNoInstalledRevisionSpecMismatchReResolves(t *testing.T) { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.BoxcutterRuntime))) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) + }) + + oldSpecDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package", Version: "1.0.2"}) + + resolverCalled := false + var capturedInstalledBundle *ocv1.BundleMetadata + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + RollingOut: []*controllers.RevisionMetadata{{ + RevisionName: "extension-1", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-test-package", + CatalogSpecDigest: oldSpecDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.2", + Version: "1.0.2", + }, + }}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + resolverCalled = true + capturedInstalledBundle = installedBundle + v := bundle.VersionRelease{ + Version: bsemver.MustParse("1.0.1"), + } + return &declcfg.Bundle{ + Name: "test-package.v1.0.1", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.1-nginxolm88138", + }, &v, nil, nil + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{installCompleted: true} + }) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Version: "1.0.1", + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.NoError(t, err) + + require.True(t, resolverCalled, + "resolver should be called: first install stuck (v1.0.2) and spec requests v1.0.1") + require.Nil(t, capturedInstalledBundle, + "installedBundle should be nil since no revision has been successfully installed yet") + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} + +// TestClusterExtensionRollingOutSameSpecHashReuses verifies that when a revision's +// source spec hash matches the current spec (i.e., the spec hasn't changed since +// the revision was resolved), the controller reuses the revision without re-resolving. +func TestClusterExtensionRollingOutSameSpecHashReuses(t *testing.T) { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.BoxcutterRuntime))) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) + }) + + specDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package"}) + + resolverCalled := false + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + RollingOut: []*controllers.RevisionMetadata{{ + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-test-package", + CatalogSpecDigest: specDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.2", + Version: "1.0.2", + }, + }}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + resolverCalled = true + return nil, nil, nil, fmt.Errorf("should not be called") + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{installCompleted: true} + }) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.NoError(t, err) + + require.False(t, resolverCalled, + "resolver should NOT be called: revision's source spec hash matches the current spec") + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} + +// TestClusterExtensionRollingOutVersionRangeMatchReuses verifies that when the spec has +// a version range constraint and the rolling-out revision was resolved from the same spec, +// the controller reuses the revision without re-resolving. +func TestClusterExtensionRollingOutVersionRangeMatchReuses(t *testing.T) { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.BoxcutterRuntime))) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) + }) + + specDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package", Version: ">=1.0.0, <2.0.0"}) + + resolverCalled := false + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + RollingOut: []*controllers.RevisionMetadata{{ + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-test-package", + CatalogSpecDigest: specDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.2", + Version: "1.0.2", + }, + }}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + resolverCalled = true + return nil, nil, nil, fmt.Errorf("should not be called") + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{installCompleted: true} + }) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Version: ">=1.0.0, <2.0.0", + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.NoError(t, err) + + require.False(t, resolverCalled, + "resolver should NOT be called: revision's source spec hash matches the current spec (range unchanged)") + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} + +func TestCatalogSpecDigestSelectorNormalization(t *testing.T) { + makeExt := func(sel *metav1.LabelSelector) *ocv1.ClusterExtension { + return &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "pkg", + UpgradeConstraintPolicy: ocv1.UpgradeConstraintPolicyCatalogProvided, + Selector: sel, + }, + }, + }, + } + } + + t.Run("reordered MatchExpressions produce the same hash", func(t *testing.T) { + sel1 := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpIn, Values: []string{"prod", "staging"}}, + {Key: "arch", Operator: metav1.LabelSelectorOpIn, Values: []string{"amd64"}}, + }, + } + sel2 := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "arch", Operator: metav1.LabelSelectorOpIn, Values: []string{"amd64"}}, + {Key: "env", Operator: metav1.LabelSelectorOpIn, Values: []string{"prod", "staging"}}, + }, + } + require.Equal(t, controllers.CatalogSpecDigest(makeExt(sel1)), controllers.CatalogSpecDigest(makeExt(sel2))) + }) + + t.Run("reordered Values within MatchExpression produce the same hash", func(t *testing.T) { + sel1 := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpIn, Values: []string{"staging", "prod"}}, + }, + } + sel2 := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "env", Operator: metav1.LabelSelectorOpIn, Values: []string{"prod", "staging"}}, + }, + } + require.Equal(t, controllers.CatalogSpecDigest(makeExt(sel1)), controllers.CatalogSpecDigest(makeExt(sel2))) + }) + + t.Run("different selectors produce different hashes", func(t *testing.T) { + sel1 := &metav1.LabelSelector{ + MatchLabels: map[string]string{"env": "prod"}, + } + sel2 := &metav1.LabelSelector{ + MatchLabels: map[string]string{"env": "staging"}, + } + require.NotEqual(t, controllers.CatalogSpecDigest(makeExt(sel1)), controllers.CatalogSpecDigest(makeExt(sel2))) + }) + + t.Run("nil selector is stable", func(t *testing.T) { + h1 := controllers.CatalogSpecDigest(makeExt(nil)) + h2 := controllers.CatalogSpecDigest(makeExt(nil)) + require.Equal(t, h1, h2) + require.NotEmpty(t, h1) + }) + + t.Run("nil and empty selector produce the same hash", func(t *testing.T) { + hNil := controllers.CatalogSpecDigest(makeExt(nil)) + hEmpty := controllers.CatalogSpecDigest(makeExt(&metav1.LabelSelector{})) + require.Equal(t, hNil, hEmpty, "nil and empty selectors are semantically equivalent and must hash identically") + }) + + t.Run("reordered channels produce the same hash", func(t *testing.T) { + ext1 := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Channels: []string{"stable", "beta", "alpha"}, + }, + }, + }, + } + ext2 := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Channels: []string{"alpha", "stable", "beta"}, + }, + }, + }, + } + require.Equal(t, controllers.CatalogSpecDigest(ext1), controllers.CatalogSpecDigest(ext2)) + }) +} + func TestValidateClusterExtension(t *testing.T) { tests := []struct { name string @@ -3109,3 +3756,83 @@ func serviceAccountValidatorWithFakeClient(serviceAccount *corev1.ServiceAccount } return controllers.ServiceAccountValidator(fake.NewClientset(serviceAccount).CoreV1()) } + +// TestClusterExtensionVersionMismatchDuringRolloutReResolves verifies that when a +// ClusterExtension is rolling out version 1.0.2 (BadImage), and the user changes the spec +// to request version 1.2.0, the controller re-resolves the bundle instead of staying stuck. +func TestClusterExtensionVersionMismatchDuringRolloutReResolves(t *testing.T) { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=true", features.BoxcutterRuntime))) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("%s=false", features.BoxcutterRuntime))) + }) + + oldSpecDigest := catalogSpecDigestFor(&ocv1.CatalogFilter{PackageName: "test-package", Version: "1.0.2"}) + + resolverCalled := false + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{ + RollingOut: []*controllers.RevisionMetadata{{ + RevisionName: "extension-stuck-2", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.0.2-badimage", + CatalogSpecDigest: oldSpecDigest, + BundleMetadata: ocv1.BundleMetadata{ + Name: "test-package.v1.0.2", + Version: "1.0.2", + }, + }}, + }, + } + d.Resolver = resolve.Func(func(ctx context.Context, ext *ocv1.ClusterExtension, installedBundle *ocv1.BundleMetadata) (*declcfg.Bundle, *bundle.VersionRelease, *declcfg.Deprecation, error) { + resolverCalled = true + v := bundle.VersionRelease{ + Version: bsemver.MustParse("1.2.0"), + } + return &declcfg.Bundle{ + Name: "test-package.v1.2.0", + Package: "test-package", + Image: "registry.example.com/test-bundle:v1.2.0", + }, &v, nil, nil + }) + d.ImagePuller = &imageutil.MockPuller{ImageFS: fstest.MapFS{}} + d.Applier = &MockApplier{installCompleted: true} + }) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + Version: "1.2.0", + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.NoError(t, err) + + require.True(t, resolverCalled, + "resolver should be called because the rolling-out revision (v1.0.2) does not match the spec (v1.2.0)") + + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + + installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) + require.NotNil(t, installedCond) + require.Equal(t, metav1.ConditionTrue, installedCond.Status) + require.Contains(t, installedCond.Message, "1.2.0") + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} diff --git a/internal/operator-controller/controllers/clusterextension_digest_test.go b/internal/operator-controller/controllers/clusterextension_digest_test.go new file mode 100644 index 000000000..a81fef056 --- /dev/null +++ b/internal/operator-controller/controllers/clusterextension_digest_test.go @@ -0,0 +1,149 @@ +package controllers_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" +) + +func TestCatalogSpecDigest(t *testing.T) { + tests := []struct { + name string + ext1 *ocv1.ClusterExtension + ext2 *ocv1.ClusterExtension + wantSame bool + }{ + { + name: "same spec produces same digest", + ext1: &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: ocv1.SourceTypeCatalog, + Catalog: &ocv1.CatalogFilter{ + PackageName: "test", + Version: "1.0.0", + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "olm.operatorframework.io/metadata.name": "test-catalog", + }, + }, + UpgradeConstraintPolicy: ocv1.UpgradeConstraintPolicySelfCertified, + }, + }, + }, + }, + ext2: &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: ocv1.SourceTypeCatalog, + Catalog: &ocv1.CatalogFilter{ + PackageName: "test", + Version: "1.0.0", + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "olm.operatorframework.io/metadata.name": "test-catalog", + }, + }, + UpgradeConstraintPolicy: ocv1.UpgradeConstraintPolicySelfCertified, + }, + }, + }, + }, + wantSame: true, + }, + { + name: "different version produces different digest", + ext1: &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: ocv1.SourceTypeCatalog, + Catalog: &ocv1.CatalogFilter{ + PackageName: "test", + Version: "1.0.0", + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "olm.operatorframework.io/metadata.name": "test-catalog", + }, + }, + UpgradeConstraintPolicy: ocv1.UpgradeConstraintPolicySelfCertified, + }, + }, + }, + }, + ext2: &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: ocv1.SourceTypeCatalog, + Catalog: &ocv1.CatalogFilter{ + PackageName: "test", + Version: "1.0.2", + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "olm.operatorframework.io/metadata.name": "test-catalog", + }, + }, + UpgradeConstraintPolicy: ocv1.UpgradeConstraintPolicySelfCertified, + }, + }, + }, + }, + wantSame: false, + }, + { + name: "different channels produces different digest", + ext1: &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: ocv1.SourceTypeCatalog, + Catalog: &ocv1.CatalogFilter{ + PackageName: "test", + Channels: []string{"alpha"}, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "olm.operatorframework.io/metadata.name": "test-catalog", + }, + }, + }, + }, + }, + }, + ext2: &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: ocv1.SourceTypeCatalog, + Catalog: &ocv1.CatalogFilter{ + PackageName: "test", + Channels: []string{"beta"}, + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "olm.operatorframework.io/metadata.name": "test-catalog", + }, + }, + }, + }, + }, + }, + wantSame: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + digest1 := controllers.CatalogSpecDigest(tt.ext1) + digest2 := controllers.CatalogSpecDigest(tt.ext2) + + assert.NotEmpty(t, digest1, "digest1 should not be empty") + assert.NotEmpty(t, digest2, "digest2 should not be empty") + + if tt.wantSame { + assert.Equal(t, digest1, digest2, "digests should be equal") + } else { + assert.NotEqual(t, digest1, digest2, "digests should be different") + } + }) + } +} diff --git a/internal/operator-controller/controllers/clusterextension_reconcile_steps.go b/internal/operator-controller/controllers/clusterextension_reconcile_steps.go index 1c6bfcb29..3d40bcd76 100644 --- a/internal/operator-controller/controllers/clusterextension_reconcile_steps.go +++ b/internal/operator-controller/controllers/clusterextension_reconcile_steps.go @@ -18,9 +18,15 @@ package controllers import ( "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" "errors" "fmt" + "slices" + "sort" + bsemver "github.com/blang/semver/v4" apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -32,6 +38,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" + "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/compare" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" "github.com/operator-framework/operator-controller/internal/operator-controller/resolve" imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image" @@ -140,14 +147,7 @@ func ResolveBundle(r resolve.Resolver, c client.Client) ReconcileStepFunc { return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) { l := log.FromContext(ctx) - // If already rolling out, use existing revision and set deprecation to Unknown (no catalog check) - if len(state.revisionStates.RollingOut) > 0 { - installedBundleName := "" - if state.revisionStates.Installed != nil { - installedBundleName = state.revisionStates.Installed.Name - } - SetDeprecationStatus(ext, installedBundleName, nil, false) - state.resolvedRevisionMetadata = state.revisionStates.RollingOut[0] + if reuseRollingOutRevision(ctx, state, ext) { return nil, nil } @@ -203,6 +203,138 @@ func ResolveBundle(r resolve.Resolver, c client.Client) ReconcileStepFunc { } } +func setRollingOutRevisionAsResolved(state *reconcileState, ext *ocv1.ClusterExtension, revision *RevisionMetadata) { + installedBundleName := "" + if state.revisionStates.Installed != nil { + installedBundleName = state.revisionStates.Installed.Name + } + SetDeprecationStatus(ext, installedBundleName, nil, false) + state.resolvedRevisionMetadata = revision +} + +// Re-resolution is triggered when no rolling-out revision was resolved from the same +// catalog spec, detecting changes to resolution-relevant fields (packageName, version, channels, selector, upgradeConstraintPolicy). +func reuseRollingOutRevision(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) bool { + if len(state.revisionStates.RollingOut) == 0 { + return false + } + + l := log.FromContext(ctx) + + // RollingOut is ordered by revision number ascending (oldest first, newest last). + latestRollingOut := state.revisionStates.RollingOut[len(state.revisionStates.RollingOut)-1] + + currentDigest := CatalogSpecDigest(ext) + + if currentDigest == "" { + setRollingOutRevisionAsResolved(state, ext, latestRollingOut) + return true + } + + for i := len(state.revisionStates.RollingOut) - 1; i >= 0; i-- { + rollingOut := state.revisionStates.RollingOut[i] + if rollingOut.CatalogSpecDigest != "" { + if rollingOut.CatalogSpecDigest == currentDigest { + setRollingOutRevisionAsResolved(state, ext, rollingOut) + return true + } + } + } + + l.Info("no rolling-out revision matches current catalog spec hash, re-resolving bundle", + "rollingOutCount", len(state.revisionStates.RollingOut), + "currentSpecHash", currentDigest, + ) + return false +} + +func versionMatchesSpec(version string, ext *ocv1.ClusterExtension) bool { + if ext.Spec.Source.Catalog == nil { + return true + } + specVersion := ext.Spec.Source.Catalog.Version + if specVersion == "" { + return true + } + if version == "" { + return false + } + versionRange, err := compare.NewVersionRange(specVersion) + if err != nil { + return false + } + v, err := bsemver.Parse(version) + if err != nil { + return false + } + return versionRange(v) +} + +func normalizeLabelSelector(sel *metav1.LabelSelector) *metav1.LabelSelector { + if sel == nil { + return nil + } + // A selector with no MatchLabels and no MatchExpressions is semantically + // equivalent to nil (both match everything). Canonicalize to nil so they + // hash identically. + if len(sel.MatchLabels) == 0 && len(sel.MatchExpressions) == 0 { + return nil + } + n := sel.DeepCopy() + for i := range n.MatchExpressions { + sort.Strings(n.MatchExpressions[i].Values) + } + sort.Slice(n.MatchExpressions, func(i, j int) bool { + a, b := n.MatchExpressions[i], n.MatchExpressions[j] + if a.Key != b.Key { + return a.Key < b.Key + } + if a.Operator != b.Operator { + return string(a.Operator) < string(b.Operator) + } + if len(a.Values) != len(b.Values) { + return len(a.Values) < len(b.Values) + } + for k := range a.Values { + if a.Values[k] != b.Values[k] { + return a.Values[k] < b.Values[k] + } + } + return false + }) + return n +} + +func CatalogSpecDigest(ext *ocv1.ClusterExtension) string { + if ext.Spec.Source.Catalog == nil { + return "" + } + cat := ext.Spec.Source.Catalog + + channels := slices.Clone(cat.Channels) + sort.Strings(channels) + + data := struct { + PackageName string `json:"p"` + Version string `json:"v,omitempty"` + Channels []string `json:"ch,omitempty"` + Selector *metav1.LabelSelector `json:"s,omitempty"` + UpgradeConstraintPolicy ocv1.UpgradeConstraintPolicy `json:"u,omitempty"` + }{ + PackageName: cat.PackageName, + Version: cat.Version, + Channels: channels, + Selector: normalizeLabelSelector(cat.Selector), + UpgradeConstraintPolicy: cat.UpgradeConstraintPolicy, + } + raw, err := json.Marshal(data) + if err != nil { + return "" + } + h := sha256.Sum256(raw) + return hex.EncodeToString(h[:]) +} + // handleResolutionError handles the case when bundle resolution fails. // // Decision logic (evaluated in order): @@ -229,14 +361,13 @@ func handleResolutionError(ctx context.Context, c client.Client, state *reconcil } // Check if the spec is requesting a specific version that differs from installed - specVersion := "" - if ext.Spec.Source.Catalog != nil { - specVersion = ext.Spec.Source.Catalog.Version - } - installedVersion := state.revisionStates.Installed.Version - // If spec requests a different version, we cannot fall back - must fail and retry - if specVersion != "" && specVersion != installedVersion { + if !versionMatchesSpec(state.revisionStates.Installed.Version, ext) { + specVersion := "" + if ext.Spec.Source.Catalog != nil { + specVersion = ext.Spec.Source.Catalog.Version + } + installedVersion := state.revisionStates.Installed.Version msg := fmt.Sprintf("unable to upgrade to version %s: %v (currently installed: %s)", specVersion, err, installedVersion) l.Error(err, "resolution failed and spec requests version change - cannot fall back", "requestedVersion", specVersion, diff --git a/internal/operator-controller/controllers/suite_test.go b/internal/operator-controller/controllers/suite_test.go index 109f0b66c..2b4caa48a 100644 --- a/internal/operator-controller/controllers/suite_test.go +++ b/internal/operator-controller/controllers/suite_test.go @@ -91,6 +91,17 @@ type deps struct { Validators []controllers.ClusterExtensionValidator } +// Computes catalog spec digest with CRD defaults applied (UpgradeConstraintPolicy defaults to CatalogProvided). +func catalogSpecDigestFor(cat *ocv1.CatalogFilter) string { + cf := *cat + if cf.UpgradeConstraintPolicy == "" { + cf.UpgradeConstraintPolicy = ocv1.UpgradeConstraintPolicyCatalogProvided + } + return controllers.CatalogSpecDigest(&ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{Source: ocv1.SourceConfig{Catalog: &cf}}, + }) +} + func newClientAndReconciler(t *testing.T, opts ...reconcilerOption) (client.Client, *controllers.ClusterExtensionReconciler) { cl := newClient(t) diff --git a/internal/operator-controller/labels/labels.go b/internal/operator-controller/labels/labels.go index 02234dce4..3d4d58c74 100644 --- a/internal/operator-controller/labels/labels.go +++ b/internal/operator-controller/labels/labels.go @@ -65,4 +65,11 @@ const ( // that were created during migration from Helm releases. This label is used // to distinguish migrated revisions from those created by normal Boxcutter operation. MigratedFromHelmKey = "olm.operatorframework.io/migrated-from-helm" + + // CatalogSpecDigestKey is the label key used to record a SHA256 hash digest + // of the catalog spec fields that affect bundle resolution (packageName, version, + // channels, selector, upgradeConstraintPolicy). This digest is used to detect when + // re-resolution is needed by comparing the current spec digest with the digest + // stored on rolling-out ClusterObjectSet revisions. + CatalogSpecDigestKey = "olm.operatorframework.io/catalog-spec-digest" ) diff --git a/test/e2e/features/update.feature b/test/e2e/features/update.feature index 7815ba873..bc11a17d2 100644 --- a/test/e2e/features/update.feature +++ b/test/e2e/features/update.feature @@ -338,3 +338,37 @@ Feature: Update ClusterExtension Then ClusterExtension reports "${NAME}-1, ${NAME}-2" as active revisions And ClusterObjectSet "${NAME}-2" reports Progressing as True with Reason RollingOut And ClusterObjectSet "${NAME}-2" reports Available as False with Reason ProbeFailure + + @BoxcutterRuntime + Scenario: Changing version during stuck rollout triggers new resolution + Given ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: olm-sa + source: + sourceType: Catalog + catalog: + packageName: ${PACKAGE:test} + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": ${CATALOG:test} + version: 1.0.0 + upgradeConstraintPolicy: SelfCertified + """ + And ClusterExtension is rolled out + And ClusterExtension is available + When ClusterExtension is updated to version "1.0.2" + Then ClusterObjectSet "${NAME}-2" reports Progressing as True with Reason RollingOut + And ClusterObjectSet "${NAME}-2" reports Available as False with Reason ProbeFailure + And ClusterObjectSet "${NAME}-2" has annotation "olm.operatorframework.io/catalog-spec-digest" with a non-empty value + When ClusterExtension is updated to version "1.2.0" + Then ClusterExtension is rolled out + And ClusterExtension is available + And bundle "test-operator.1.2.0" is installed in version "1.2.0" + And ClusterObjectSet "${NAME}-3" annotation "olm.operatorframework.io/catalog-spec-digest" differs from ClusterObjectSet "${NAME}-2" diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index a91bfbacb..5f607cd30 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -120,6 +120,8 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has observed phase "([^"]+)" with a non-empty digest$`, ClusterObjectSetHasObservedPhase) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" is archived$`, ClusterObjectSetIsArchived) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" contains annotation "([^"]+)" with value$`, ClusterObjectSetHasAnnotationWithValue) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has annotation "([^"]+)" with a non-empty value$`, ClusterObjectSetHasAnnotationWithNonEmptyValue) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" annotation "([^"]+)" differs from ClusterObjectSet "([^"]+)"$`, ClusterObjectSetAnnotationDiffersFrom) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has label "([^"]+)" with value "([^"]+)"$`, ClusterObjectSetHasLabelWithValue) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" phase objects are not found or not owned by the revision$`, ClusterObjectSetObjectsNotFoundOrNotOwned) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" phase objects are managed in Kubernetes secrets$`, ClusterObjectSetPhaseObjectsManagedInSecrets) @@ -791,6 +793,69 @@ func ClusterObjectSetHasLabelWithValue(ctx context.Context, revisionName, labelK return nil } +// ClusterObjectSetHasAnnotationWithNonEmptyValue verifies an annotation exists and has a non-empty value. +// Waits for the condition to be met with timeout. +func ClusterObjectSetHasAnnotationWithNonEmptyValue(ctx context.Context, name, annotationKey string) error { + sc := scenarioCtx(ctx) + name = substituteScenarioVars(name, sc) + + waitFor(ctx, func() bool { + obj, err := getResource("clusterobjectset", name, "") + if err != nil { + return false + } + annotations := obj.GetAnnotations() + if annotations == nil { + return false + } + + value, ok := annotations[annotationKey] + if !ok { + return false + } + if value == "" { + return false + } + return true + }) + return nil +} + +// ClusterObjectSetAnnotationDiffersFrom compares annotation values between two ClusterObjectSets. +// Waits for both ClusterObjectSets to have the annotation with different values. +func ClusterObjectSetAnnotationDiffersFrom(ctx context.Context, name1, annotationKey, name2 string) error { + sc := scenarioCtx(ctx) + name1 = substituteScenarioVars(name1, sc) + name2 = substituteScenarioVars(name2, sc) + + waitFor(ctx, func() bool { + obj1, err := getResource("clusterobjectset", name1, "") + if err != nil { + return false + } + + obj2, err := getResource("clusterobjectset", name2, "") + if err != nil { + return false + } + + annotations1 := obj1.GetAnnotations() + annotations2 := obj2.GetAnnotations() + + value1, ok1 := annotations1[annotationKey] + value2, ok2 := annotations2[annotationKey] + + if !ok1 || !ok2 { + return false + } + if value1 == value2 { + return false + } + return true + }) + return nil +} + // ClusterObjectSetObjectsNotFoundOrNotOwned waits for all objects described in the named // ClusterObjectSet's phases to either not exist on the cluster or not contain the revision // in their ownerReferences. Polls with timeout.