diff --git a/cmd/manager/main.go b/cmd/manager/main.go index d5da75c32..72465b4dd 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -54,6 +54,7 @@ import ( "github.com/operator-framework/operator-controller/internal/contentmanager" "github.com/operator-framework/operator-controller/internal/controllers" "github.com/operator-framework/operator-controller/internal/features" + "github.com/operator-framework/operator-controller/internal/finalizers" "github.com/operator-framework/operator-controller/internal/httputil" "github.com/operator-framework/operator-controller/internal/labels" "github.com/operator-framework/operator-controller/internal/resolve" @@ -207,7 +208,7 @@ func main() { } clusterExtensionFinalizers := crfinalizer.NewFinalizers() - if err := clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupUnpackCacheFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + if err := clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupUnpackCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { return crfinalizer.Result{}, unpacker.Cleanup(ctx, &source.BundleSource{Name: obj.GetName()}) })); err != nil { setupLog.Error(err, "unable to register finalizer", "finalizerKey", controllers.ClusterExtensionCleanupUnpackCacheFinalizer) @@ -258,7 +259,7 @@ func main() { } cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper()) - err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { ext := obj.(*ocv1alpha1.ClusterExtension) err := cm.Delete(ext) return crfinalizer.Result{}, err @@ -308,12 +309,6 @@ func main() { } } -type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) - -func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { - return f(ctx, obj) -} - func authFilePathIfPresent(logger logr.Logger) string { _, err := os.Stat(authFilePath) if os.IsNotExist(err) { diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 4272e05e5..4f30f3aa0 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -192,15 +192,9 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp l.Info("handling finalizers") finalizeResult, err := r.Finalizers.Finalize(ctx, ext) if err != nil { - // TODO: For now, this error handling follows the pattern of other error handling. - // Namely: zero just about everything out, throw our hands up, and return an error. - // This is not ideal, and we should consider a more nuanced approach that resolves - // as much status as possible before returning, or at least keeps previous state if - // it is properly labeled with its observed generation. - setInstallStatus(ext, nil) setResolutionStatus(ext, nil) setStatusProgressing(ext, err) - ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, err.Error()) + return ctrl.Result{}, err } if finalizeResult.Updated || finalizeResult.StatusUpdated { @@ -297,8 +291,11 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp // The only way to eventually recover from permission errors is to keep retrying). managedObjs, _, err := r.Applier.Apply(ctx, unpackResult.Bundle, ext, objLbls, storeLbls) if err != nil { - setInstalledStatusConditionFailed(ext, err.Error()) setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedBundleMetadata, err)) + // If bundle is not already installed, set Installed status condition to False + if installedBundle == nil { + setInstalledStatusConditionFailed(ext, err.Error()) + } return ctrl.Result{}, err } diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index b80ca59b0..bf0aeaa19 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -18,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/util/rand" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/operator-framework/operator-registry/alpha/declcfg" @@ -25,6 +26,7 @@ import ( ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" "github.com/operator-framework/operator-controller/internal/conditionsets" "github.com/operator-framework/operator-controller/internal/controllers" + "github.com/operator-framework/operator-controller/internal/finalizers" "github.com/operator-framework/operator-controller/internal/resolve" "github.com/operator-framework/operator-controller/internal/rukpak/source" ) @@ -336,6 +338,105 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) } +func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) { + cl, reconciler := newClientAndReconciler(t) + reconciler.Unpacker = &MockUnpacker{ + result: &source.Result{ + State: source.StateUnpacked, + Bundle: fstest.MapFS{}, + }, + } + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + t.Log("When the cluster extension specifies a channel with version that exist") + t.Log("By initializing cluster state") + pkgName := "prometheus" + pkgVer := "1.0.0" + pkgChan := "beta" + namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) + serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) + + clusterExtension := &ocv1alpha1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1alpha1.ClusterExtensionSpec{ + Source: ocv1alpha1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1alpha1.CatalogSource{ + PackageName: pkgName, + Version: pkgVer, + Channels: []string{pkgChan}, + }, + }, + Install: ocv1alpha1.ClusterExtensionInstallConfig{ + Namespace: namespace, + ServiceAccount: ocv1alpha1.ServiceAccountReference{ + Name: serviceAccount, + }, + }, + }, + } + err := cl.Create(ctx, clusterExtension) + require.NoError(t, err) + + t.Log("It sets resolution success status") + t.Log("By running reconcile") + reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1alpha1.ClusterExtension, _ *ocv1alpha1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { + v := 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 + }) + + reconciler.Manager = &MockManagedContentCacheManager{ + cache: &MockManagedContentCache{}, + } + reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{ + bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, + } + reconciler.Applier = &MockApplier{ + objs: []client.Object{}, + } + + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.NoError(t, err) + + reconciler.Applier = &MockApplier{ + err: errors.New("apply failure"), + } + + res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.Error(t, err) + + t.Log("By fetching updated cluster extension after reconcile") + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + + t.Log("By checking the status fields") + expectedBundleMetadata := ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"} + require.Equal(t, expectedBundleMetadata, clusterExtension.Status.Resolution.Bundle) + require.Equal(t, expectedBundleMetadata, clusterExtension.Status.Install.Bundle) + + t.Log("By checking the expected installed conditions") + installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) + require.NotNil(t, installedCond) + require.Equal(t, metav1.ConditionTrue, installedCond.Status) + require.Equal(t, ocv1alpha1.ReasonSuccess, installedCond.Reason) + + t.Log("By checking the expected progressing conditions") + progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeProgressing) + require.NotNil(t, progressingCond) + require.Equal(t, metav1.ConditionTrue, progressingCond.Status) + require.Equal(t, ocv1alpha1.ReasonRetrying, progressingCond.Reason) + require.Contains(t, progressingCond.Message, fmt.Sprintf("for resolved bundle %q with version %q", expectedBundleMetadata.Name, expectedBundleMetadata.Version)) + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) +} + func TestClusterExtensionManagerFailed(t *testing.T) { cl, reconciler := newClientAndReconciler(t) reconciler.Unpacker = &MockUnpacker{ @@ -591,6 +692,107 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) { require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) } +func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { + cl, reconciler := newClientAndReconciler(t) + reconciler.Unpacker = &MockUnpacker{ + result: &source.Result{ + State: source.StateUnpacked, + Bundle: fstest.MapFS{}, + }, + } + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + t.Log("When the cluster extension specifies a channel with version that exist") + t.Log("By initializing cluster state") + pkgName := "prometheus" + pkgVer := "1.0.0" + pkgChan := "beta" + namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) + serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) + + clusterExtension := &ocv1alpha1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1alpha1.ClusterExtensionSpec{ + Source: ocv1alpha1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1alpha1.CatalogSource{ + PackageName: pkgName, + Version: pkgVer, + Channels: []string{pkgChan}, + }, + }, + Install: ocv1alpha1.ClusterExtensionInstallConfig{ + Namespace: namespace, + ServiceAccount: ocv1alpha1.ServiceAccountReference{ + Name: serviceAccount, + }, + }, + }, + } + err := cl.Create(ctx, clusterExtension) + require.NoError(t, err) + t.Log("It sets resolution success status") + t.Log("By running reconcile") + reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1alpha1.ClusterExtension, _ *ocv1alpha1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { + v := 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 + }) + fakeFinalizer := "fake.testfinalizer.io" + finalizersMessage := "still have finalizers" + reconciler.Applier = &MockApplier{ + objs: []client.Object{}, + } + reconciler.Manager = &MockManagedContentCacheManager{ + cache: &MockManagedContentCache{}, + } + reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{ + bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, + } + err = reconciler.Finalizers.Register(fakeFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + return crfinalizer.Result{}, errors.New(finalizersMessage) + })) + + require.NoError(t, err) + + // Reconcile twice to simulate installing the ClusterExtension and loading in the finalizers + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.NoError(t, err) + res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.NoError(t, err) + + t.Log("By fetching updated cluster extension after first reconcile") + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) + expectedBundleMetadata := ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"} + require.Equal(t, expectedBundleMetadata, clusterExtension.Status.Install.Bundle) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionTrue, cond.Status) + + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{})) + res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Error(t, err, res) + + t.Log("By fetching updated cluster extension after second reconcile") + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled) + require.Equal(t, expectedBundleMetadata, clusterExtension.Status.Install.Bundle) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionTrue, cond.Status) + require.Equal(t, fakeFinalizer, clusterExtension.Finalizers[0]) + cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeProgressing) + require.NotNil(t, cond) + require.Equal(t, metav1.ConditionTrue, cond.Status) + require.Contains(t, cond.Message, finalizersMessage) +} + func verifyInvariants(ctx context.Context, t *testing.T, c client.Client, ext *ocv1alpha1.ClusterExtension) { key := client.ObjectKeyFromObject(ext) require.NoError(t, c.Get(ctx, key, ext)) diff --git a/internal/finalizers/finalizers.go b/internal/finalizers/finalizers.go new file mode 100644 index 000000000..b04635a9e --- /dev/null +++ b/internal/finalizers/finalizers.go @@ -0,0 +1,14 @@ +package finalizers + +import ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/client" + crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" +) + +type FinalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) + +func (f FinalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + return f(ctx, obj) +}