From 9038198dea4bfa895c58a48d825d6a80e87fb7c5 Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Wed, 2 Aug 2023 16:06:54 +0200 Subject: [PATCH] Ensure finalizer is present before reconciling (#218) * Ensure finalizer is present before reconciling * Fix gomod --- go.mod | 2 +- pkg/reconciler/internal/updater/updater.go | 10 ------ .../internal/updater/updater_test.go | 31 ++++++------------- pkg/reconciler/reconciler.go | 24 ++++++++------ pkg/reconciler/reconciler_test.go | 20 ++++++------ 5 files changed, 35 insertions(+), 52 deletions(-) diff --git a/go.mod b/go.mod index 6315377e..778c7ae9 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/onsi/ginkgo/v2 v2.11.0 github.com/onsi/gomega v1.27.8 github.com/operator-framework/operator-lib v0.11.1-0.20230607132417-ecb9be488378 + github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.15.1 github.com/sergi/go-diff v1.2.0 github.com/sirupsen/logrus v1.9.3 @@ -129,7 +130,6 @@ require ( github.com/opencontainers/image-spec v1.1.0-rc2.0.20221005185240-3a7f492d3f1b // indirect github.com/peterbourgon/diskv v2.0.1+incompatible // indirect github.com/phayes/freeport v0.0.0-20220201140144-74d24b5ae9f5 // indirect - github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_model v0.4.0 // indirect github.com/prometheus/common v0.44.0 // indirect diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 482e3fb4..f931c36b 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -122,16 +122,6 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err return nil } -func EnsureFinalizer(finalizer string) UpdateFunc { - return func(obj *unstructured.Unstructured) bool { - if controllerutil.ContainsFinalizer(obj, finalizer) { - return false - } - controllerutil.AddFinalizer(obj, finalizer) - return true - } -} - func RemoveFinalizer(finalizer string) UpdateFunc { return func(obj *unstructured.Unstructured) bool { if !controllerutil.ContainsFinalizer(obj, finalizer) { diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index 74569cd6..b952d9a1 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -59,7 +59,10 @@ var _ = Describe("Updater", func() { When("the object does not exist", func() { It("should fail", func() { Expect(client.Delete(context.TODO(), obj)).To(Succeed()) - u.Update(EnsureFinalizer(testFinalizer)) + u.Update(func(u *unstructured.Unstructured) bool { + u.SetAnnotations(map[string]string{"foo": "bar"}) + return true + }) err := u.Apply(context.TODO(), obj) Expect(err).NotTo(BeNil()) Expect(apierrors.IsNotFound(err)).To(BeTrue()) @@ -68,12 +71,15 @@ var _ = Describe("Updater", func() { When("an update is a change", func() { It("should apply an update function", func() { - u.Update(EnsureFinalizer(testFinalizer)) + u.Update(func(u *unstructured.Unstructured) bool { + u.SetAnnotations(map[string]string{"foo": "bar"}) + return true + }) resourceVersion := obj.GetResourceVersion() Expect(u.Apply(context.TODO(), obj)).To(Succeed()) Expect(client.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) - Expect(obj.GetFinalizers()).To(Equal([]string{testFinalizer})) + Expect(obj.GetAnnotations()["foo"]).To(Equal("bar")) Expect(obj.GetResourceVersion()).NotTo(Equal(resourceVersion)) }) @@ -89,25 +95,6 @@ var _ = Describe("Updater", func() { }) }) -var _ = Describe("EnsureFinalizer", func() { - var obj *unstructured.Unstructured - - BeforeEach(func() { - obj = &unstructured.Unstructured{} - }) - - It("should add finalizer if not present", func() { - Expect(EnsureFinalizer(testFinalizer)(obj)).To(BeTrue()) - Expect(obj.GetFinalizers()).To(Equal([]string{testFinalizer})) - }) - - It("should not add duplicate finalizer", func() { - obj.SetFinalizers([]string{testFinalizer}) - Expect(EnsureFinalizer(testFinalizer)(obj)).To(BeFalse()) - Expect(obj.GetFinalizers()).To(Equal([]string{testFinalizer})) - }) -}) - var _ = Describe("RemoveFinalizer", func() { var obj *unstructured.Unstructured diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 097581ff..a661cd38 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + errs "github.com/pkg/errors" "strings" "sync" "time" @@ -553,6 +554,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. return ctrl.Result{}, err } + // The finalizer must be present on the CR before we can do anything. Otherwise, if the reconciliation fails, + // there might be resources created by the chart that will not be garbage-collected + // (cluster-scoped resources or resources in other namespaces, which are not bound by an owner reference). + // This is a safety measure to ensure that the chart is fully uninstalled before the CR is deleted. + if obj.GetDeletionTimestamp() == nil && !controllerutil.ContainsFinalizer(obj, uninstallFinalizer) { + log.V(1).Info("Adding uninstall finalizer.") + obj.SetFinalizers(append(obj.GetFinalizers(), uninstallFinalizer)) + if err := r.client.Update(ctx, obj); err != nil { + return ctrl.Result{}, errs.Wrapf(err, "failed to add uninstall finalizer to %s/%s", req.NamespacedName.Namespace, req.NamespacedName.Name) + } + } + u := updater.New(r.client) defer func() { applyErr := u.Apply(ctx, obj) @@ -570,14 +583,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. updater.EnsureConditionUnknown(conditions.TypeReleaseFailed), updater.EnsureDeployedRelease(nil), ) - // NOTE: If obj has the uninstall finalizer, that means a release WAS deployed at some point - // in the past, but we don't know if it still is because we don't have an actionClient to check. - // So the question is, what do we do with the finalizer? We could: - // - Leave it in place. This would make the CR impossible to delete without either resolving this error, or - // manually uninstalling the release, deleting the finalizer, and deleting the CR. - // - Remove the finalizer. This would make it possible to delete the CR, but it would leave around any - // release resources that are not owned by the CR (those in the cluster scope or in other namespaces). - // + // When it is impossible to obtain an actionClient, we cannot proceed with the reconciliation. Question is + // what to do with the finalizer? // The decision made for now is to leave the finalizer in place, so that the user can intervene and try to // resolve the issue, instead of the operator silently leaving some dangling resources hanging around after the // CR is deleted. @@ -984,7 +991,6 @@ func ensureDeployedRelease(u *updater.Updater, rel *release.Release) { if rel.Info != nil && len(rel.Info.Notes) > 0 { message = rel.Info.Notes } - u.Update(updater.EnsureFinalizer(uninstallFinalizer)) u.UpdateStatus( updater.EnsureCondition(conditions.Deployed(corev1.ConditionTrue, reason, message)), updater.EnsureDeployedRelease(rel), diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 9efffe68..31167fb5 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -607,8 +607,8 @@ var _ = Describe("Reconciler", func() { Expect(c.Message).To(Equal(acgErr.Error())) }) - By("verifying the uninstall finalizer is not present on the CR", func() { - Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeFalse()) + By("verifying the uninstall finalizer is present on the CR", func() { + Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue()) }) }) It("returns an error getting the release", func() { @@ -644,8 +644,8 @@ var _ = Describe("Reconciler", func() { Expect(c.Message).To(Equal("get not implemented")) }) - By("verifying the uninstall finalizer is not present on the CR", func() { - Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeFalse()) + By("verifying the uninstall finalizer is present on the CR", func() { + Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue()) }) }) }) @@ -680,8 +680,8 @@ var _ = Describe("Reconciler", func() { Expect(c.Message).To(ContainSubstring("error parsing index")) }) - By("verifying the uninstall finalizer is not present on the CR", func() { - Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeFalse()) + By("verifying the uninstall finalizer is present on the CR", func() { + Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue()) }) }) }) @@ -747,8 +747,8 @@ var _ = Describe("Reconciler", func() { Expect(c.Message).To(ContainSubstring("install failed: foobar")) }) - By("ensuring the uninstall finalizer is not present on the CR", func() { - Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeFalse()) + By("verifying the uninstall finalizer is present on the CR", func() { + Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue()) }) }) }) @@ -928,7 +928,7 @@ var _ = Describe("Reconciler", func() { Expect(objStat.Status.DeployedRelease.Manifest).To(Equal(currentRelease.Manifest)) }) - By("verifying the uninstall finalizer is not present on the CR", func() { + By("verifying the uninstall finalizer is present on the CR", func() { Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue()) }) }) @@ -967,7 +967,7 @@ var _ = Describe("Reconciler", func() { Expect(objStat.Status.DeployedRelease.Manifest).To(Equal(currentRelease.Manifest)) }) - By("verifying the uninstall finalizer is not present on the CR", func() { + By("verifying the uninstall finalizer is present on the CR", func() { Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue()) }) })