pkg/{client,reconciler}: retry all uninstalls that result in error

pkg/client/actionclient.go

@@ -180,13 +180,50 @@ func (c *actionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m
 }
 
 func (c *actionClient) Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) {
-	uninstall := action.NewUninstall(c.conf)
+	uninstall, explicitKeepHistory, err := c.getUninstallAction(opts...)
+	if err != nil {
+		return nil, err
+	}
+
+	resp, err := uninstall.Run(name)
+	if err != nil {
+		// If there was an uninstall error, we kept the history, and the response contains a release,
+		// set the release to Uninstalling, and then return the response and error.
+		//
+		// This way, we can retry the uninstall during the next reconciliation. Without KeepHistory,
+		// the release secrets will be purged, and the helm-operator will interpret that to mean that
+		// the release was successfully uninstalled and thus remove the finalizer despite there being
+		// uninstall errors.
+		if uninstall.KeepHistory && resp != nil && resp.Release != nil && resp.Release.Info != nil {
+			resp.Release.Info.Status = release.StatusUninstalling
+			if updateErr := c.conf.Releases.Update(resp.Release); updateErr != nil {
+				return resp, fmt.Errorf("failed to set release status to uninstalling: %v; uninstall error: %w", updateErr, err)
+			}
+		}
+	} else if !explicitKeepHistory {
+		uninstall.KeepHistory = false
+		resp, err = uninstall.Run(name)
+	}
+	return resp, err
+}
+
+func (c *actionClient) getUninstallAction(opts ...UninstallOption) (*action.Uninstall, bool, error) {
+	u1 := action.NewUninstall(c.conf)
+	u1.KeepHistory = true
 	for _, o := range opts {
-		if err := o(uninstall); err != nil {
-			return nil, err
+		if err := o(u1); err != nil {
+			return nil, false, err
 		}
 	}
-	return uninstall.Run(name)
+	u2 := action.NewUninstall(c.conf)
+	u2.KeepHistory = false
+	for _, o := range opts {
+		if err := o(u2); err != nil {
+			return nil, false, err
+		}
+	}
+
+	return u1, !u1.KeepHistory || u2.KeepHistory, nil
 }
 
 func (c *actionClient) Reconcile(rel *release.Release) error {

pkg/client/actionclient_test.go

@@ -347,6 +347,36 @@ var _ = Describe("ActionClient", func() {
 				})
 			})
 		})
+
+		var _ = Describe("getUninstallAction", func() {
+			It("defaults to KeepHistory=true", func() {
+				ac := actionClient{}
+				uninstall, set, err := ac.getUninstallAction()
+				Expect(err).To(BeNil())
+				Expect(set).To(BeFalse())
+				Expect(uninstall.KeepHistory).To(BeTrue())
+			})
+			It("reports if KeepHistory was explicitly set to true", func() {
+				ac := actionClient{}
+				uninstall, set, err := ac.getUninstallAction(func(u *action.Uninstall) error {
+					u.KeepHistory = true
+					return nil
+				})
+				Expect(err).To(BeNil())
+				Expect(set).To(BeTrue())
+				Expect(uninstall.KeepHistory).To(BeTrue())
+			})
+			It("reports if KeepHistory was explicitly set to false", func() {
+				ac := actionClient{}
+				uninstall, set, err := ac.getUninstallAction(func(u *action.Uninstall) error {
+					u.KeepHistory = false
+					return nil
+				})
+				Expect(err).To(BeNil())
+				Expect(set).To(BeTrue())
+				Expect(uninstall.KeepHistory).To(BeFalse())
+			})
+		})
 	})
 
 	var _ = Describe("createPatch", func() {

pkg/reconciler/reconciler_test.go

@@ -41,6 +41,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -1081,15 +1082,27 @@ var _ = Describe("Reconciler", func() {
 						})
 					})
 					When("uninstall fails", func() {
+						var (
+							deployedManifest string
+							actionConf       *action.Configuration
+						)
 						BeforeEach(func() {
-							ac := helmfake.NewActionClient()
-							ac.HandleGet = func() (*release.Release, error) {
-								return &release.Release{Name: "test", Version: 1, Manifest: "manifest: 1"}, nil
-							}
-							ac.HandleUninstall = func() (*release.UninstallReleaseResponse, error) {
-								return nil, errors.New("uninstall failed: foobar")
-							}
-							r.actionClientGetter = helmfake.NewActionClientGetter(&ac, nil)
+							By("adding an invalid resource to the release manifest", func() {
+								var err error
+								acg := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), nil)
+								actionConf, err = acg.ActionConfigFor(obj)
+								Expect(err).To(BeNil())
+
+								deployedManifest = currentRelease.Manifest
+								currentRelease.Manifest = fmt.Sprintf("%s\n---\n%s", currentRelease.Manifest, nonExistentConfigMap(obj.GetNamespace()))
+								Expect(actionConf.Releases.Update(currentRelease)).To(Succeed())
+							})
+						})
+						AfterEach(func() {
+							By("removing the invalid resource from the release manifest", func() {
+								currentRelease.Manifest = deployedManifest
+								Expect(actionConf.Releases.Update(currentRelease)).To(Succeed())
+							})
 						})
 						It("handles the uninstall error", func() {
 							By("deleting the CR", func() {
@@ -1114,17 +1127,23 @@ var _ = Describe("Reconciler", func() {
 								Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeDeployed)).To(BeTrue())
 								Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeReleaseFailed)).To(BeTrue())
 								Expect(objStat.Status.DeployedRelease.Name).To(Equal("test"))
-								Expect(objStat.Status.DeployedRelease.Manifest).To(Equal("manifest: 1"))
+								Expect(objStat.Status.DeployedRelease.Manifest).To(Equal(currentRelease.Manifest))
 
 								c := objStat.Status.Conditions.GetCondition(conditions.TypeReleaseFailed)
 								Expect(c).NotTo(BeNil())
 								Expect(c.Reason).To(Equal(conditions.ReasonUninstallError))
-								Expect(c.Message).To(ContainSubstring("uninstall failed: foobar"))
+								Expect(c.Message).To(ContainSubstring("uninstallation completed with 1 error(s)"))
 
 								c = objStat.Status.Conditions.GetCondition(conditions.TypeIrreconcilable)
 								Expect(c).NotTo(BeNil())
 								Expect(c.Reason).To(Equal(conditions.ReasonReconcileError))
-								Expect(c.Message).To(ContainSubstring("uninstall failed: foobar"))
+								Expect(c.Message).To(ContainSubstring("uninstallation completed with 1 error(s)"))
+							})
+
+							By("ensuring the CR release is present and status is Uninstalling", func() {
+								rel, err := ac.Get(obj.GetName())
+								Expect(err).To(BeNil())
+								Expect(rel.Info.Status).To(Equal(release.StatusUninstalling))
 							})
 
 							By("ensuring the uninstall finalizer is present on the CR", func() {
@@ -1279,3 +1298,14 @@ func verifyEvent(ctx context.Context, cl client.Reader, obj metav1.Object, event
 	Reason: %q
 	Message: %q`, eventType, reason, message))
 }
+
+func nonExistentConfigMap(ns string) string {
+	return fmt.Sprintf(`apiVersion: v1
+kind: FakeConfigMap
+metadata:
+  name: non-existent-%s
+  namespace: %s
+data:
+  hello: world
+`, rand.String(4), ns)
+}