diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index f55c79a5..329c4df0 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -94,16 +94,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..d011fa5f 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -34,6 +34,23 @@ import ( const testFinalizer = "testFinalizer" +func EnsureAnnotation(key, value string) UpdateFunc { + return func(obj *unstructured.Unstructured) bool { + var hasUpdate bool + if obj.GetAnnotations() == nil { + obj.SetAnnotations(map[string]string{}) + hasUpdate = true + } + var annotations = obj.GetAnnotations() + if annotations[key] != value { + annotations[key] = value + obj.SetAnnotations(annotations) + hasUpdate = true + } + return hasUpdate + } +} + var _ = Describe("Updater", func() { var ( client client.Client @@ -59,7 +76,7 @@ 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(EnsureAnnotation("foo", "bar")) err := u.Apply(context.TODO(), obj) Expect(err).NotTo(BeNil()) Expect(apierrors.IsNotFound(err)).To(BeTrue()) @@ -68,12 +85,12 @@ var _ = Describe("Updater", func() { When("an update is a change", func() { It("should apply an update function", func() { - u.Update(EnsureFinalizer(testFinalizer)) + u.Update(EnsureAnnotation("foo", "bar")) 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 +106,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 3fefad0d..de692dba 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" @@ -529,8 +530,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. log.V(1).Info("Adding uninstall finalizer") obj.SetFinalizers(append(obj.GetFinalizers(), uninstallFinalizer)) if err := r.client.Update(ctx, obj); err != nil { - log.Error(err, "Failed to add uninstall finalizer") - return ctrl.Result{}, err + return ctrl.Result{}, errs.Wrapf(err, "failed to add uninstall finalizer to %s/%s", req.NamespacedName.Namespace, req.NamespacedName.Name) } } @@ -551,14 +551,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 doesn't mean a release was deployed at some point in the past. - // This just means the CR was accepted by the reconcile method. The presence of the finalizer is required - // to deploy any resources. - // - 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.