[tenant][kubernetes] Introduce better cleanup logic - #1661

packages/apps/kubernetes/templates/delete.yaml (2)

32-40: Verify safety of the HelmRelease suspension loop.

The loop at lines 33-40 iterates over kubectl get ... -o name output. The inline check if [ -n "$hr" ]; then is redundant because get -o name only outputs names (or nothing if no matches). However, if the kubectl command itself fails, the script will exit (due to set -e) before any errors are logged. Add explicit error handling for the kubectl call itself.

- for hr in $(kubectl -n {{ .Release.Namespace }} get helmreleases.helm.toolkit.fluxcd.io -l "cozystack.io/target-cluster-name={{ .Release.Name }}" -o name 2>/dev/null || true); do
-   if [ -n "$hr" ]; then           
-     echo "  Suspending $hr"
-     kubectl -n {{ .Release.Namespace }} patch "$hr" \
-       -p '{"spec": {"suspend": true}}' \
-       --type=merge --field-manager=flux-client-side-apply
-   fi
- done
+ for hr in $(kubectl -n {{ .Release.Namespace }} get helmreleases.helm.toolkit.fluxcd.io -l "cozystack.io/target-cluster-name={{ .Release.Name }}" -o name 2>/dev/null || echo ""); do
+   echo "  Suspending $hr"
+   kubectl -n {{ .Release.Namespace }} patch "$hr" \
+     -p '{"spec": {"suspend": true}}' \
+     --type=merge --field-manager=flux-client-side-apply
+ done

Removes the redundant [ -n "$hr" ] check and simplifies the logic.

---

88-116: Verify that the 'watch' verb is necessary for HelmRelease patch operations.

The Role grants watch verb for datavolumes and services (newly added), and the HelmReleases rule includes patch verb (line 96). Typically, watch is needed for controller-managed operations, but for direct imperative patches, it may not be required. Confirm whether the watch verb is necessary, or if it should be removed to follow least-privilege principles.

Also, verify that patch verb with --type=merge --field-manager=flux-client-side-apply is the correct approach for Flux objects. This uses server-side apply semantics; ensure the field-manager alignment is intentional.

If watch is not needed for patch operations, consider removing it:

  - apiGroups:
      - "helm.toolkit.fluxcd.io"
    resources:
      - helmreleases
    verbs:
      - get
      - list
-     - watch
      - patch
      - delete

packages/apps/tenant/templates/cleanup-job.yaml (2)

70-85: Improve error handling and clarify the two-phase HelmRelease deletion strategy.

The cleanup script lacks explicit error handling and logging similar to the concern in file 1. Additionally, the script deletes HelmReleases in two cohorts (Applications and Tenant Modules). Clarify:

1. Why are they split into two cohorts? (e.g., ordering dependency, different deletion semantics)
2. Is there any risk of the second cohort failing if the first succeeds?
3. Should both cohorts run even if the first fails, or should the script halt on first failure?

Consider adding error handling:

  - /bin/bash
  - -c
  - |
    set -e
    NAMESPACE="{{ include "tenant.name" . }}"
    
    echo "Cleaning up HelmReleases in namespace: $NAMESPACE"
    
    echo "Deleting Applications"
-   kubectl delete helmreleases.helm.toolkit.fluxcd.io -n "$NAMESPACE" \
+   kubectl delete helmreleases.helm.toolkit.fluxcd.io -n "$NAMESPACE" \
      -l 'cozystack.io/ui=true,internal.cozystack.io/tenantmodule!=true' \
-     --ignore-not-found=true --wait=true
+     --ignore-not-found=true --wait=true || echo "Warning: Failed to delete Applications"
    
    echo "Deleting Tenant Modules"
-   kubectl delete helmreleases.helm.toolkit.fluxcd.io -n "$NAMESPACE" \
+   kubectl delete helmreleases.helm.toolkit.fluxcd.io -n "$NAMESPACE" \
      -l 'cozystack.io/ui=true,internal.cozystack.io/tenantmodule=true' \
-     --ignore-not-found=true --wait=true
+     --ignore-not-found=true --wait=true || echo "Warning: Failed to delete Tenant Modules"
    
    echo "Cleanup completed successfully"

This logs warnings if either cohort deletion fails while still completing cleanup.

---

43-52: Verify pre-delete hook execution ordering across kubernetes and tenant cleanup jobs.

File 1 (kubernetes/delete.yaml) defines a pre-delete Job with weights: ServiceAccount (0), Role (5), RoleBinding (5), Job (10).

File 2 (tenant/cleanup-job.yaml) defines a pre-delete Job with weights: ServiceAccount/Role/RoleBinding (-5), Job (0).

Lower weights execute first in Helm hooks, so tenant cleanup (weight -5 Job) runs before kubernetes cleanup (weight 10 Job). Verify this ordering is intentional:

• If tenant cleanup must complete before cluster cleanup, this is correct.
• If cluster cleanup should occur first, adjust tenant Job weight to be higher than 10.
• If there's no strict dependency, document this to prevent future confusion.

Also confirm that the tenant namespace is still accessible when kubernetes-level cleanup (which deletes control planes) runs.

Document the hook weight rationale in comments or a separate ADR explaining why tenant cleanup (weight 0) runs before kubernetes cleanup (weight 10).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro