Merge pull request #316 from dmvolod/issue-315

🐛 HelmChartProxy is removed even when the release cannot be uninstalled

Author: k8s-ci-robot

controllers/controllers_test.go

@@ -17,8 +17,6 @@ limitations under the License.
 package controllers_test
 
 import (
-	"time"
-
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	helmrelease "helm.sh/helm/v3/pkg/release"
@@ -32,51 +30,62 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-var (
-	testNamespace = "test-namespace"
-	newVersion    = "new-version"
+const (
+	testNamespace1       = "test-namespace1"
+	testNamespace2       = "test-namespace2"
+	newVersion           = "new-version"
+	releaseFailedMessage = "unable to remove helm release"
+)
 
-	defaultProxy = &addonsv1alpha1.HelmChartProxy{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: addonsv1alpha1.GroupVersion.String(),
-			Kind:       "HelmChartProxy",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "test-hcp",
-			Namespace: testNamespace,
-		},
-		Spec: addonsv1alpha1.HelmChartProxySpec{
-			ClusterSelector: metav1.LabelSelector{
-				MatchLabels: map[string]string{
-					"test-label": "test-value",
+var (
+	namespaces          = []string{testNamespace1, testNamespace2}
+	failedHelmUninstall bool
+
+	newProxy = func(namespace string) *addonsv1alpha1.HelmChartProxy {
+		return &addonsv1alpha1.HelmChartProxy{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: addonsv1alpha1.GroupVersion.String(),
+				Kind:       "HelmChartProxy",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-hcp",
+				Namespace: namespace,
+			},
+			Spec: addonsv1alpha1.HelmChartProxySpec{
+				ClusterSelector: metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"test-label": "test-value",
+					},
 				},
+				ReleaseName:      "test-release-name",
+				ChartName:        "test-chart-name",
+				RepoURL:          "https://test-repo-url",
+				ReleaseNamespace: "test-release-namespace",
+				Version:          "test-version",
+				ValuesTemplate:   "apiServerPort: {{ .Cluster.spec.clusterNetwork.apiServerPort }}",
 			},
-			ReleaseName:      "test-release-name",
-			ChartName:        "test-chart-name",
-			RepoURL:          "https://test-repo-url",
-			ReleaseNamespace: "test-release-namespace",
-			Version:          "test-version",
-			ValuesTemplate:   "apiServerPort: {{ .Cluster.spec.clusterNetwork.apiServerPort }}",
-		},
+		}
 	}
 
-	cluster1 = &clusterv1.Cluster{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: clusterv1.GroupVersion.String(),
-			Kind:       "Cluster",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "test-cluster-1",
-			Namespace: testNamespace,
-			Labels: map[string]string{
-				"test-label": "test-value",
+	newCluster = func(namespace string) *clusterv1.Cluster {
+		return &clusterv1.Cluster{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: clusterv1.GroupVersion.String(),
+				Kind:       "Cluster",
 			},
-		},
-		Spec: clusterv1.ClusterSpec{
-			ClusterNetwork: &clusterv1.ClusterNetwork{
-				APIServerPort: ptr.To(int32(1234)),
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-cluster-1",
+				Namespace: namespace,
+				Labels: map[string]string{
+					"test-label": "test-value",
+				},
 			},
-		},
+			Spec: clusterv1.ClusterSpec{
+				ClusterNetwork: &clusterv1.ClusterNetwork{
+					APIServerPort: ptr.To(int32(1234)),
+				},
+			},
+		}
 	}
 
 	helmReleaseDeployed = &helmrelease.Release{
@@ -135,62 +144,91 @@ var _ = Describe("Testing HelmChartProxy and HelmReleaseProxy reconcile", func()
 				return condition != nil && condition(hrpList.Items)
 			}, timeout, interval).Should(BeTrue())
 		}
-	)
 
-	It("HelmChartProxy and HelmReleaseProxy lifecycle test", func() {
-		cluster := cluster1.DeepCopy()
-		err := k8sClient.Create(ctx, cluster)
-		Expect(err).ToNot(HaveOccurred())
-		err = k8sClient.Create(ctx, newKubeconfigSecretForCluster(cluster))
-		Expect(err).ToNot(HaveOccurred())
+		install = func(cluster *clusterv1.Cluster, proxy *addonsv1alpha1.HelmChartProxy) {
+			err := k8sClient.Create(ctx, cluster)
+			Expect(err).ToNot(HaveOccurred())
+			err = k8sClient.Create(ctx, newKubeconfigSecretForCluster(cluster))
+			Expect(err).ToNot(HaveOccurred())
 
-		patch := client.MergeFrom(cluster.DeepCopy())
-		cluster.Status.Conditions = clusterv1.Conditions{
-			{
-				Type:               clusterv1.ControlPlaneInitializedCondition,
-				Status:             corev1.ConditionTrue,
-				LastTransitionTime: metav1.NewTime(time.Now()),
-			},
+			patch := client.MergeFrom(cluster.DeepCopy())
+			conditions.MarkTrue(cluster, clusterv1.ControlPlaneInitializedCondition)
+			err = k8sClient.Status().Patch(ctx, cluster, patch)
+			Expect(err).ToNot(HaveOccurred())
+
+			err = k8sClient.Create(ctx, proxy)
+			Expect(err).ToNot(HaveOccurred())
+
+			waitForHelmChartProxyCondition(client.ObjectKeyFromObject(proxy), func(helmChartProxy *addonsv1alpha1.HelmChartProxy) bool {
+				return conditions.IsTrue(helmChartProxy, clusterv1.ReadyCondition)
+			})
+
+			waitForHelmReleaseProxyCondition(client.ObjectKeyFromObject(proxy), func(helmReleaseProxyList []addonsv1alpha1.HelmReleaseProxy) bool {
+				return len(helmReleaseProxyList) == 1 && conditions.IsTrue(&helmReleaseProxyList[0], clusterv1.ReadyCondition)
+			})
 		}
-		err = k8sClient.Status().Patch(ctx, cluster, patch)
-		Expect(err).ToNot(HaveOccurred())
 
-		err = k8sClient.Create(ctx, defaultProxy)
-		Expect(err).ToNot(HaveOccurred())
+		deleteAndWaitHelmChartProxy = func(proxy *addonsv1alpha1.HelmChartProxy) {
+			err := k8sClient.Delete(ctx, proxy)
+			Expect(err).ToNot(HaveOccurred())
 
-		waitForHelmChartProxyCondition(client.ObjectKeyFromObject(defaultProxy), func(helmChartProxy *addonsv1alpha1.HelmChartProxy) bool {
-			return conditions.IsTrue(helmChartProxy, clusterv1.ReadyCondition)
-		})
+			Eventually(func() bool {
+				if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(proxy), &addonsv1alpha1.HelmChartProxy{}); client.IgnoreNotFound(err) != nil {
+					return false
+				}
 
-		waitForHelmReleaseProxyCondition(client.ObjectKeyFromObject(defaultProxy), func(helmReleaseProxyList []addonsv1alpha1.HelmReleaseProxy) bool {
-			return len(helmReleaseProxyList) == 1 && conditions.IsTrue(&helmReleaseProxyList[0], clusterv1.ReadyCondition)
-		})
+				return true
+			}, timeout, interval).Should(BeTrue())
+
+			waitForHelmReleaseProxyCondition(client.ObjectKeyFromObject(proxy), func(helmReleaseProxyList []addonsv1alpha1.HelmReleaseProxy) bool {
+				return len(helmReleaseProxyList) == 0
+			})
+		}
+	)
+
+	It("HelmChartProxy and HelmReleaseProxy lifecycle happy path test", func() {
+		cluster := newCluster(testNamespace1)
+		helmChartProxy := newProxy(testNamespace1)
+		install(cluster, helmChartProxy)
 
-		hcp := &addonsv1alpha1.HelmChartProxy{}
-		err = k8sClient.Get(ctx, client.ObjectKeyFromObject(defaultProxy), hcp)
+		err := k8sClient.Get(ctx, client.ObjectKeyFromObject(helmChartProxy), helmChartProxy)
 		Expect(err).ToNot(HaveOccurred())
-		patch = client.MergeFrom(hcp.DeepCopy())
-		hcp.Spec.Version = newVersion
-		err = k8sClient.Patch(ctx, hcp, patch)
+		patch := client.MergeFrom(helmChartProxy.DeepCopy())
+		helmChartProxy.Spec.Version = newVersion
+		err = k8sClient.Patch(ctx, helmChartProxy, patch)
 		Expect(err).ToNot(HaveOccurred())
 
-		waitForHelmReleaseProxyCondition(client.ObjectKeyFromObject(defaultProxy), func(helmReleaseProxyList []addonsv1alpha1.HelmReleaseProxy) bool {
+		waitForHelmReleaseProxyCondition(client.ObjectKeyFromObject(helmChartProxy), func(helmReleaseProxyList []addonsv1alpha1.HelmReleaseProxy) bool {
 			return len(helmReleaseProxyList) == 1 && conditions.IsTrue(&helmReleaseProxyList[0], clusterv1.ReadyCondition) && helmReleaseProxyList[0].Spec.Version == "new-version"
 		})
 
-		err = k8sClient.Delete(ctx, hcp)
+		deleteAndWaitHelmChartProxy(helmChartProxy)
+	})
+
+	It("HelmChartProxy and HelmReleaseProxy test with failed Release uninstall", func() {
+		cluster := newCluster(testNamespace2)
+		helmChartProxy := newProxy(testNamespace2)
+		failedHelmUninstall = true
+		install(cluster, helmChartProxy)
+
+		err := k8sClient.Delete(ctx, helmChartProxy)
 		Expect(err).ToNot(HaveOccurred())
 
-		Eventually(func() bool {
-			if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(hcp), hcp); client.IgnoreNotFound(err) != nil {
+		Consistently(func() bool {
+			if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(helmChartProxy), helmChartProxy); err != nil {
 				return false
 			}
 
 			return true
 		}, timeout, interval).Should(BeTrue())
 
-		waitForHelmReleaseProxyCondition(client.ObjectKeyFromObject(defaultProxy), func(helmReleaseProxyList []addonsv1alpha1.HelmReleaseProxy) bool {
-			return len(helmReleaseProxyList) == 0
-		})
+		readyCondition := conditions.Get(helmChartProxy, clusterv1.ReadyCondition)
+		Expect(readyCondition).NotTo(BeNil())
+		Expect(readyCondition.Status).To(Equal(corev1.ConditionFalse))
+		Expect(readyCondition.Message).To(Equal(releaseFailedMessage))
+
+		By("Making HelmChartProxy uninstallable")
+		failedHelmUninstall = false
+		deleteAndWaitHelmChartProxy(helmChartProxy)
 	})
 })

controllers/helmchartproxy/helmchartproxy_controller.go

@@ -148,7 +148,7 @@ func (r *HelmChartProxyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		// The object is being deleted
 		if controllerutil.ContainsFinalizer(helmChartProxy, addonsv1alpha1.HelmChartProxyFinalizer) {
 			// our finalizer is present, so lets handle any external dependency
-			if err := r.reconcileDelete(ctx, helmChartProxy, releaseList.Items); err != nil {
+			if result, err := r.reconcileDelete(ctx, helmChartProxy, releaseList.Items); err != nil || !result.IsZero() {
 				// if fail to delete the external dependency here, return with error
 				// so that it can be retried
 				return ctrl.Result{}, err
@@ -213,22 +213,40 @@ func (r *HelmChartProxyReconciler) reconcileNormal(ctx context.Context, helmChar
 }
 
 // reconcileDelete handles the deletion of a HelmChartProxy. It takes a list of HelmReleaseProxies to uninstall the Helm chart from all selected Clusters.
-func (r *HelmChartProxyReconciler) reconcileDelete(ctx context.Context, helmChartProxy *addonsv1alpha1.HelmChartProxy, releases []addonsv1alpha1.HelmReleaseProxy) error {
+func (r *HelmChartProxyReconciler) reconcileDelete(ctx context.Context, helmChartProxy *addonsv1alpha1.HelmChartProxy, releases []addonsv1alpha1.HelmReleaseProxy) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
+	getters := make([]conditions.Getter, 0)
 
 	log.V(2).Info("Deleting all HelmReleaseProxies as part of HelmChartProxy deletion", "helmChartProxy", helmChartProxy.Name)
-
 	for i := range releases {
 		release := releases[i]
 
 		log.V(2).Info("Deleting release", "releaseName", release.Name, "cluster", release.Spec.ClusterRef.Name)
 		if err := r.deleteHelmReleaseProxy(ctx, &release); err != nil {
 			// TODO: will this fail if clusterRef is nil
-			return errors.Wrapf(err, "failed to delete release %s from cluster %s", release.Name, release.Spec.ClusterRef.Name)
+			return ctrl.Result{}, errors.Wrapf(err, "failed to delete release %s from cluster %s", release.Name, release.Spec.ClusterRef.Name)
+		}
+
+		log.V(2).Info("Validating release deletion", "releaseName", release.Name)
+		if err := r.Get(ctx, client.ObjectKeyFromObject(&release), &release); err != nil {
+			if apierrors.IsNotFound(err) {
+				continue
+			}
+
+			return ctrl.Result{}, errors.Wrapf(err, "failed to get HelmReleaseProxy %s", release.Name)
 		}
+
+		log.V(2).Info("The release has not been deleted yet, waiting for it to be removed", "releaseName", release.Name)
+		getters = append(getters, &release)
 	}
 
-	return nil
+	if len(getters) > 0 {
+		conditions.SetAggregate(helmChartProxy, addonsv1alpha1.HelmReleaseProxiesReadyCondition, getters, conditions.AddSourceRef(), conditions.WithStepCounterIf(false))
+
+		return ctrl.Result{Requeue: true}, nil
+	}
+
+	return ctrl.Result{}, nil
 }
 
 // listClustersWithLabels returns a list of Clusters that match the given label selector.

controllers/suite_test.go

@@ -18,6 +18,7 @@ package controllers_test
 
 import (
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 	"path/filepath"
@@ -114,7 +115,9 @@ var _ = BeforeSuite(func() {
 	Expect(err).NotTo(HaveOccurred())
 	Expect(k8sClient).NotTo(BeNil())
 
-	Expect(k8sClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNamespace}})).NotTo(HaveOccurred())
+	for _, namespace := range namespaces {
+		Expect(k8sClient.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}})).NotTo(HaveOccurred())
+	}
 
 	k8sManager, err = ctrl.NewManager(cfg, ctrl.Options{
 		Scheme: scheme.Scheme,
@@ -129,8 +132,15 @@ var _ = BeforeSuite(func() {
 	helmClient = mocks.NewMockClient(gomock.NewController(&TestReporter{}))
 
 	helmClient.EXPECT().InstallOrUpgradeHelmRelease(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(helmReleaseDeployed, nil).AnyTimes()
-	helmClient.EXPECT().UninstallHelmRelease(gomock.Any(), gomock.Any(), gomock.Any()).Return(&helmRelease.UninstallReleaseResponse{}, nil).AnyTimes()
 	helmClient.EXPECT().GetHelmRelease(gomock.Any(), gomock.Any(), gomock.Any()).Return(&helmRelease.Release{}, nil).AnyTimes()
+	helmClient.EXPECT().UninstallHelmRelease(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_, _, _ any) (*helmRelease.UninstallReleaseResponse, error) {
+		if failedHelmUninstall {
+			return nil, errors.New(releaseFailedMessage)
+		}
+
+		return &helmRelease.UninstallReleaseResponse{}, nil
+	},
+	).AnyTimes()
 
 	err = (&helmchartproxy.HelmChartProxyReconciler{
 		Client: k8sManager.GetClient(),