From 5dbdbd129948c3b79e6135020b4faaaf7b5d5677 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Wed, 1 Mar 2023 18:12:51 +0530 Subject: [PATCH] check if service account exists before uninstalling release Check if the service account to be impersonated actually exists and proceed with uninstalling the Helm release only if it does. Otherwise, skip uninstalling the release and carry on with finalization. Add an e2e test to check if deleting a namespace with the RBAC and HelmRelease succeeds with the namespace being fully deleted. Signed-off-by: Sanskar Jaiswal --- .github/workflows/e2e.yaml | 12 +++++ config/testdata/delete-ns/test.yaml | 68 +++++++++++++++++++++++++++ controllers/helmrelease_controller.go | 44 ++++++++++++----- 3 files changed, 112 insertions(+), 12 deletions(-) create mode 100644 config/testdata/delete-ns/test.yaml diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index b8cfe38..6fe12b1 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -536,6 +536,18 @@ jobs: fi done echo ' done' + - name: Run delete-ns tests + run: | + kubectl apply -f config/testdata/delete-ns + kubectl -n delete-ns wait helmreleases/podinfo --for=condition=ready --timeout=2m + kubectl delete ns delete-ns 1>/dev/null 2>&1 & + echo -n ">> Waiting for namespace to be deleted" + if kubectl wait --for=delete namespace delete-ns --timeout=2m; then + echo 'Namespace deleted successfully' + else + echo 'Timed out waiting for namespace to be deleted' + exit 1 + fi - name: Run post-renderer-kustomize test run: | kubectl -n helm-system apply -f config/testdata/post-renderer-kustomize diff --git a/config/testdata/delete-ns/test.yaml b/config/testdata/delete-ns/test.yaml new file mode 100644 index 0000000..1e57837 --- /dev/null +++ b/config/testdata/delete-ns/test.yaml @@ -0,0 +1,68 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: delete-ns +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: gotk-reconciler + namespace: delete-ns +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: gotk-reconciler + namespace: delete-ns +rules: + - apiGroups: + - "" + resources: + - '*' + verbs: + - '*' + - apiGroups: + - apps + resources: + - '*' + verbs: + - '*' +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: gotk-reconciler + namespace: delete-ns +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: gotk-reconciler +subjects: + - kind: ServiceAccount + name: gotk-reconciler + namespace: delete-ns +--- +apiVersion: source.toolkit.fluxcd.io/v1beta1 +kind: HelmRepository +metadata: + name: podinfo + namespace: delete-ns +spec: + interval: 1m + url: https://stefanprodan.github.io/podinfo +--- +apiVersion: helm.toolkit.fluxcd.io/v2beta1 +kind: HelmRelease +metadata: + name: podinfo + namespace: delete-ns +spec: + serviceAccountName: gotk-reconciler + interval: 5m + chart: + spec: + chart: podinfo + version: 5.0.3 + sourceRef: + kind: HelmRepository + name: podinfo diff --git a/controllers/helmrelease_controller.go b/controllers/helmrelease_controller.go index 52d788a..1889fc7 100644 --- a/controllers/helmrelease_controller.go +++ b/controllers/helmrelease_controller.go @@ -660,8 +660,11 @@ func (r *HelmReleaseReconciler) composeValues(ctx context.Context, hr v2.HelmRel // reconcileDelete deletes the v1beta2.HelmChart of the v2beta1.HelmRelease, // and uninstalls the Helm release if the resource has not been suspended. +// It only performs a Helm uninstall if the ServiceAccount to be impersonated +// exists. func (r *HelmReleaseReconciler) reconcileDelete(ctx context.Context, hr v2.HelmRelease) (ctrl.Result, error) { r.recordReadiness(ctx, hr) + log := ctrl.LoggerFrom(ctx) // Delete the HelmChart that belongs to this resource. if err := r.deleteHelmChart(ctx, &hr); err != nil { @@ -670,19 +673,36 @@ func (r *HelmReleaseReconciler) reconcileDelete(ctx context.Context, hr v2.HelmR // Only uninstall the Helm Release if the resource is not suspended. if !hr.Spec.Suspend { - getter, err := r.buildRESTClientGetter(ctx, hr) - if err != nil { - return ctrl.Result{}, err - } - run, err := runner.NewRunner(getter, hr.GetStorageNamespace(), ctrl.LoggerFrom(ctx)) - if err != nil { - return ctrl.Result{}, err - } - if err := run.Uninstall(hr); err != nil && !errors.Is(err, driver.ErrReleaseNotFound) { - return ctrl.Result{}, err - } - ctrl.LoggerFrom(ctx).Info("uninstalled Helm release for deleted resource") + impersonator := runtimeClient.NewImpersonator( + r.Client, + r.StatusPoller, + r.PollingOpts, + hr.Spec.KubeConfig, + r.KubeConfigOpts, + kube.DefaultServiceAccountName, + hr.Spec.ServiceAccountName, + hr.GetNamespace(), + ) + if impersonator.CanImpersonate(ctx) { + getter, err := r.buildRESTClientGetter(ctx, hr) + if err != nil { + return ctrl.Result{}, err + } + run, err := runner.NewRunner(getter, hr.GetStorageNamespace(), ctrl.LoggerFrom(ctx)) + if err != nil { + return ctrl.Result{}, err + } + if err := run.Uninstall(hr); err != nil && !errors.Is(err, driver.ErrReleaseNotFound) { + return ctrl.Result{}, err + } + log.Info("uninstalled Helm release for deleted resource") + } else { + err := fmt.Errorf("failed to find service account to impersonate") + msg := "skipping Helm uninstall" + log.Error(err, msg) + r.event(ctx, hr, hr.Status.LastAppliedRevision, eventv1.EventSeverityError, fmt.Sprintf("%s: %s", msg, err.Error())) + } } else { ctrl.LoggerFrom(ctx).Info("skipping Helm uninstall for suspended resource") }