From 7779d4bcafcc7aa8867f995450204774311d98f8 Mon Sep 17 00:00:00 2001 From: Dmitry Volodin Date: Wed, 27 Nov 2024 20:38:42 +0300 Subject: [PATCH] :bug: HelmChartProxy is removed even when the release cannot be uninstalled --- controllers/controllers_test.go | 190 +++++++++++------- .../helmchartproxy_controller.go | 28 ++- controllers/suite_test.go | 14 +- 3 files changed, 149 insertions(+), 83 deletions(-) diff --git a/controllers/controllers_test.go b/controllers/controllers_test.go index 7eb1b93..83030c8 100644 --- a/controllers/controllers_test.go +++ b/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) }) }) diff --git a/controllers/helmchartproxy/helmchartproxy_controller.go b/controllers/helmchartproxy/helmchartproxy_controller.go index 0f61857..ce4d720 100644 --- a/controllers/helmchartproxy/helmchartproxy_controller.go +++ b/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. diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 3bc3524..c0c8d33 100644 --- a/controllers/suite_test.go +++ b/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(),