fix(harbor): clean up orphaned jobservice PVC and ACME solver resources on delete (#3061) - #3077

Code Review

This pull request introduces a post-delete cleanup hook for the Harbor package to remove orphaned PVCs and cert-manager ACME resources upon uninstallation, along with corresponding unit tests and a Makefile update. Feedback on these changes highlights that the cleanup job requires the deletecollection RBAC verb to delete resources by label selector, and the cleanup container needs resource requests and limits defined in accordance with the repository style guide. Additionally, the unit tests should be updated to reflect the RBAC changes.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/apps/harbor/templates/hooks/cleanup.yaml`:
- Around line 54-63: The cleanup hook only deletes cert-manager CRs and a
label-selected subset, so orphaned cm-acme-http-solver-* workload/network
objects can remain. Update the cleanup logic in the hook that runs around the
kubectl delete calls to explicitly delete solver Ingress, Service, and Pod
resources associated with the release, and adjust the RBAC used by the cleanup
job to grant delete access for those resource types. Keep the existing
certificate/order/challenge cleanup, and make sure the additional deletions
target the same release/solver scope used by the current cleanup flow.

---

Nitpick comments:
In `@packages/apps/harbor/tests/cleanup_hook_test.yaml`:
- Around line 26-41: The cleanup hook test currently only asserts CR cleanup,
but it should also verify solver resource teardown and RBAC coverage. Update the
cleanup test in cleanup_hook_test.yaml so the command built by the cleanup hook
includes delete handling for the ACME solver Ingress, Service, and Pod
resources, using the existing cleanup command assertions around the target
symbols in the test. Also add assertions on the generated Role rules to confirm
the needed API groups/resources are granted for those solver objects, so the
teardown contract stays protected.

Verdict

NOT LGTM

Operational risks remain: the post-delete hook fires synchronously while the inner <release>-system HR is still being uninstalled by Flux, leaving the user-visible behaviour of Terminating PVCs unverified on a live cluster; the PR body also still advertises a label-selector ACME sweep that was deliberately removed before merge.

Findings

[MINOR] packages/apps/harbor/templates/hooks/cleanup.yaml:55-67 — PR body claims the hook "sweeps leftover solver objects by acme.cert-manager.io/http01-solver=true", but the implementation deliberately does NOT do this and the unit test (tests/cleanup_hook_test.yaml:36-43) asserts notMatchRegex against that label.

Update the "What this PR does" section so the release-note reader sees the actual scope: deterministic <release>-ingress-tls Certificate deletion with reliance on cert-manager owner-ref cascade for the solver-side objects. The current PR body advertises behaviour that was removed before merge.

[MINOR] packages/apps/harbor/templates/hooks/cleanup.yaml:25 — Cleanup image docker.io/clastix/kubectl:v1.32 is a moving tag, not pinned by digest, and is not routed through cozy-lib.images-registry.

Air-gapped / mirrored installs that rely on the cozystack images registry will fail to pull this image at the worst possible moment (during app uninstall). The same gap exists in packages/apps/mariadb/templates/hooks/cleanup-pvc.yaml:23, so this is pre-existing fleet debt rather than a new regression — but it is now duplicated. Recommend digest-pinning and routing through cozy-lib.images-registry in both before this is fanned out further.

Claim mismatches

• "sweeps leftover solver objects by acme.cert-manager.io/http01-solver=true" — [MISSING] Removed from the implementation; tests forbid it.

Operational risks

• Hook vs inner-HR uninstall race (packages/apps/harbor/templates/hooks/cleanup.yaml:50-58 + packages/apps/harbor/templates/harbor.yaml): when the outer harbor release is uninstalled, Helm's post-delete hook runs synchronously, but the inner <release>-system HR (the upstream Harbor subchart) is reconciled and uninstalled asynchronously by Flux helm-controller. The hook's kubectl delete pvc -l app.kubernetes.io/instance=<release>-system may fire while jobservice / trivy pods are still running. Kubernetes will hold each PVC in Terminating until consumer pods exit, so the outcome is eventually-correct, but operators get no diagnosable failure mode if the inner uninstall stalls — the Job's "PVC deletion: completed" log line is misleading when the PVC is actually Terminating, not deleted. Either gate the PVC sweep behind a wait-until-<release>-system-HR-is-gone check, or document this clearly in the Job's log + operator docs.

• Out-of-scope orphans remain: the hook handles jobservice / trivy PVCs from the upstream Harbor subchart, plus this release's <release>-ingress-tls Certificate. CNPG Cluster PVCs (<release>-db-N) follow CNPG's own reclaim policy and are NOT touched. Re-installing Harbor with a different DB size into the same namespace will leak the old DB PVCs. If this PR is positioned as "cleanup orphans on Harbor app delete", the scope boundary needs to be either widened to CNPG PVCs (with explicit risk discussion, since DB data is on those) or called out in the PR body so operators know what is and is not cleaned up.

• PR-body ACME claim divergence (see Finding 1): the published claim shapes what operators expect to be cleaned. Shipping with the current PR body teaches operators they don't need to manually sweep cm-acme-http-solver-* pods, which is true today only because cert-manager's owner-ref cascade is doing the actual work — not because of any label-selector sweep the hook performs. If cert-manager's behaviour shifts (e.g. a future Challenge created without an owner-ref), the silent loss of the documented sweep will surface as new orphans with no in-repo signal that the cleanup ever covered that path.

Caveats

• I did not exercise the full uninstall path on a live cluster; the race-window analysis is from reading helm.toolkit.fluxcd.io/v2 semantics and Helm hook ordering, not from a repro. Smoke-testing on a dev cluster with a real Harbor instance that has issued at least one Certificate (ideally during an in-flight HTTP-01 challenge) is needed to confirm or refute the operational-risk severity.
• helm template against the modified chart fails on dashboard-resourcemap.yaml when rendered with --namespace default because of the tenant.name guard; with --namespace tenant-root the chart renders cleanly. helm unittest passes all 8 tests (2 suites). Confirmed locally.
• make generate was not required: no changes to values.yaml, values.schema.json, Chart.yaml, or README.md.
• I did not deeply verify whether kubectl delete pvc -l <selector> requires the deletecollection RBAC verb on this cluster's cozystack-api-driven RBAC layer. kubectl normally does LIST + DELETE-per-object, which only needs list + delete, but if any admission proxy in the cluster intercepts and rewrites to deletecollection, the current Role would be insufficient. Worth a one-off kubectl auth can-i test as part of the smoke test above.

Recommended follow-ups

1. Update the PR description to remove the "sweeps … http01-solver=true" claim, mirroring the in-code comment, before merge.
2. Open a follow-up to digest-pin docker.io/clastix/kubectl:v1.32 and route it through cozy-lib.images-registry in both packages/apps/harbor/templates/hooks/cleanup.yaml and packages/apps/mariadb/templates/hooks/cleanup-pvc.yaml.
3. Decide and document the cleanup scope boundary re CNPG PVCs (either widen the hook or call out the limitation in PR body and operator docs).
4. Add a real-cluster smoke test (port-forward / kind / dev cluster) covering: install Harbor → issue at least one cert → uninstall → assert no jobservice PVC, no Certificate, no cm-acme-http-solver-* resources in the namespace within a bounded timeout.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/apps/harbor/templates/hooks/cleanup.yaml`:
- Around line 58-60: The PVC cleanup in the cleanup hook is too broad because it
targets every PVC for the `{{ .Release.Name }}-system` instance instead of only
jobservice storage. Update the deletion logic in the cleanup template to scope
the `kubectl delete pvc` command to the jobservice PVC specifically, using the
relevant component label or known PVC name from the hook rather than the generic
instance label. Keep the warning fallback, but ensure the match only affects
jobservice-related PVCs.

---

Nitpick comments:
In `@packages/apps/harbor/tests/cleanup_hook_test.yaml`:
- Around line 86-157: Add assertions in cleanup_hook_test.yaml for the hook
annotations on the ServiceAccount, Role, and RoleBinding rendered by the cleanup
hook. Use the existing document checks for the ServiceAccount, Role, and
RoleBinding to verify the `helm.sh/hook` lifecycle metadata (including
`post-delete`) and the delete-policy annotation so these helper resources stay
tied to uninstall. Focus the new assertions near the existing `ServiceAccount`,
`Role`, and `RoleBinding` tests to keep the RBAC objects covered by the hook
contract.

NOT LGTM — the PVC cleanup silently misses the trivy PVC, so it does not fully fix the orphan it claims to.

The post-delete hook deletes PVCs with -l app.kubernetes.io/instance=<release>-system (templates/hooks/cleanup.yaml:59). That label is only set by the upstream subchart's harbor.labels helper, which the jobservice PVC uses — so jobservice is handled correctly. The trivy PVC is different: it is a StatefulSet volumeClaimTemplate whose selector uses harbor.matchLabels (release, app — no app.kubernetes.io/instance), and the StatefulSet controller only merges spec.selector.matchLabels onto the PVC, so data-<release>-trivy-0 never carries app.kubernetes.io/instance and is not matched by the sweep. There is no persistentVolumeClaimRetentionPolicy on the trivy StatefulSet, so it is retained on uninstall. Trivy is on by default, so a default Harbor delete leaves the trivy PVC orphaned — which contradicts the PR body, the release note, and the code comment ("intentionally covers both the jobservice and trivy PVCs").

Suggested fix: select by the label both PVCs share — -l release=<release>-system matches jobservice (harbor.labels) and trivy (harbor.matchLabels), while CNPG/Redis PVCs use different label schemes and the registry is on S3, so nothing extra is caught. The unit test that pins the app.kubernetes.io/instance=<release>-system selector must be updated to assert the corrected selector.

Non-blocking:

• kubectl delete certificate,order,challenge <release>-ingress-tls only resolves a real object for certificate (Orders/Challenges are hash-suffixed), so the Order/Challenge kinds are no-ops here — the real cleanup is the Certificate owner-ref cascade plus the host-scoped sweep. Consider dropping order,challenge from the by-name delete.
• In Gateway mode no per-app Ingress or <release>-ingress-tls Certificate is rendered, so the Certificate delete is a no-op there; the dnsName-scoped challenge sweep still covers http01. Worth a one-line comment.

The ACME solver handling (release-scoped Certificate delete + host-scoped Challenge/Ingress sweep, no blanket label sweep), the hardened Job, the hook ordering, and the RBAC scoping all look correct.

LGTM — the prior blocker (the PVC sweep missed the trivy PVC) is resolved; -l release=<release>-system correctly matches both kept PVCs, verified against the chart's label helpers.

The trivy miss is fixed. Verified: harbor.labels (jobservice PVC) and harbor.legacy.labels (trivy volumeClaimTemplate) both set release: .Release.Name, and the trivy StatefulSet's matchLabels (release, app, component, no app.kubernetes.io/instance) are stamped onto its PVC — so the old instance selector missed trivy (enabled by default) while release=<release>-system catches both. CNPG (cnpg.io/cluster) and redis-operator PVCs use different label schemes, so nothing extra is matched; the release value is an exact match, so there is no cross-release collision. The ACME handling (by-name <release>-ingress-tls Certificate delete + host-scoped Challenge/solver-Ingress sweep, with no blanket http01-solver label sweep) is correct. helm-unittest passes (9/9) and pins the full contract including the no-instance-label and no-blanket-sweep guards.

Non-blocking follow-ups

1. The PR description is now inaccurate: it states PVCs are selected by app.kubernetes.io/instance=<release>-system and that they carry that label "not <release>" — the shipped selector is release=<release>-system (the opposite), which is precisely what the final commit changed. The RBAC paragraph also lists orders, which the hook deliberately does not grant. Worth correcting before merge so the release note and merge record match the code.
2. The PVC delete runs with the default --wait=true and the Job has no activeDeadlineSeconds. Because the inner <release>-system HelmRelease is uninstalled asynchronously by Flux, the hook can fire while jobservice/trivy pods are still running and block on pvc-protection; if that uninstall stalls, the hook has no bounded failure mode. The sibling SeaweedFS cleanup hook uses --wait=false + activeDeadlineSeconds: 120 — applying the same here would bound the failure and make the body's "deletion request always succeeds" note accurate.
3. No e2e coverage: harbor.bats exists but is not extended to assert the PVCs/Certificate/solver objects are gone after delete. helm-unittest only pins the rendered command, so real deletion is unverified by automated tests.

Verdict

LGTM with non-blocking notes

The cleanup hook is correctly scoped, PSS-restricted compliant, follows the existing mariadb cleanup-hook pattern, and unittest passes (9/9). One stale paragraph in the PR description contradicts the code, and the documented force-delete of previously kept PVCs is a behaviour change worth a follow-up audit.

Findings

[MINOR] packages/apps/harbor/templates/hooks/cleanup.yaml:66 — PR body description contradicts the actual selector

The PR body states the hook selects by app.kubernetes.io/instance=<release>-system, but the code uses -l release=<release>-system. The in-template comment (lines 51-63) correctly explains the reason for picking release= (the trivy PVC, populated via harbor.legacy.labels, does not carry app.kubernetes.io/instance), and tests/cleanup_hook_test.yaml explicitly guards against regressing to the instance label. The PR description is stale; tests and code are correct. Please update the description so reviewers and changelog readers do not have to reconcile the discrepancy.

[MINOR] packages/apps/harbor/templates/hooks/cleanup.yaml:30 — docker.io/clastix/kubectl:v1.32 not digest-pinned and not routed through cozy-lib.images-registry

Pre-existing debt shared with packages/apps/mariadb/templates/hooks/cleanup-pvc.yaml:23 and acknowledged in the PR body as follow-up #3088. Air-gapped customers must mirror this image themselves. Out of scope for this PR but worth tracking.

[MINOR] packages/apps/harbor/templates/hooks/cleanup.yaml:13 — no backoffLimit or activeDeadlineSeconds on the cleanup Job

The script catches every kubectl failure with || echo WARNING, so the Job will normally exit 0. However, on ImagePullBackOff or a stuck API call the Job inherits Kubernetes defaults (backoffLimit: 6, no deadline) and a post-delete hook will block helm/Flux until it eventually fails. Consider backoffLimit: 1 and activeDeadlineSeconds: 300 to bound teardown time. Same shape as mariadb cleanup, so introducing it here is the right place to set the pattern.

Caveats

• Behaviour change for existing tenants: tenants that have relied on persistence.resourcePolicy: keep to retain jobservice file logs or the trivy vulnerability-DB cache across a helm uninstall will now see those PVCs force-deleted on next teardown. The release-note flags this; CNPG and Redis PVCs are verified untouched (CNPG uses cnpg.io/cluster labels, Redis is managed by redis-operator with its own labels — neither carries release=<release>-system).
• Eventually-consistent PVC reclaim while the inner <release>-system HelmRelease is still being uninstalled by Flux: PVCs may stay in Terminating until jobservice/trivy Pods exit. Acknowledged in the PR body NOTE; the Job's kubectl delete pvc itself always succeeds.
• Gateway mode (where no per-app Ingress or <release>-ingress-tls Certificate is rendered): the kubectl delete certificate is a no-op (covered by --ignore-not-found), and the dnsName-scoped Challenge sweep still applies. Verified against templates/httproute.yaml and templates/ingress.yaml.
• Job runs in a tenant-restricted PSS namespace: pod-level runAsNonRoot/runAsUser=65534/seccompProfile=RuntimeDefault and container-level allowPrivilegeEscalation=false/readOnlyRootFilesystem=true/capabilities.drop=[ALL] satisfy restricted PSS.

Recommended follow-ups

• Update the PR description to match the actual selector (release=<release>-system, not app.kubernetes.io/instance=<release>-system).
• In #3088, in addition to digest-pinning, consider adding backoffLimit / activeDeadlineSeconds to both the harbor and mariadb cleanup Jobs so a hook failure cannot indefinitely stall app deletion.