fix(bucket): clean up orphaned cert-manager ACME solver resources on delete (#3060) - #3076

Code Review

This pull request introduces a post-delete Helm hook to clean up orphaned cert-manager ACME resources for the bucket application, along with corresponding unit tests and a Makefile test target. The reviewer feedback highlights a critical issue where a blanket sweep of all http01-solver resources in the namespace could disrupt active ACME challenges for other applications sharing the same namespace. To address this, the reviewer suggests removing the unsafe sweep, narrowing the RBAC permissions to adhere to the principle of least privilege, and updating the unit tests accordingly.

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/bucket/templates/hooks/cleanup-acme.yaml`:
- Around line 47-50: The cleanup command in cleanup-acme.yaml is using the Helm
release label selector in kubectl delete, which won’t match cert-manager Order
and Challenge resources. Update the kubectl delete target in the cert-manager
cleanup block to select those resources by the certificate name using the
cert-manager.io/certificate label, keeping the existing delete of the
certificate itself in place. Refer to the cleanup shell block around the kubectl
delete certificate,order,challenge command and preserve the warning behavior.

LGTM — adds a post-delete Job that removes the release-scoped cert-manager Certificate, letting cert-manager cascade-delete the orphaned ACME Order/Challenge/solver objects; deliberately avoids a namespace-wide solver sweep that would kill sibling apps' in-flight challenges.

Business context: deleting a bucket left orphaned cert-manager ACME solver resources behind (#3060).

Verified: the targeted Certificate name is correct — packages/system/bucket/templates/ingress.yaml sets tls.secretName=<bucketName>-ui-tls and bucketName == .Release.Name, so cert-manager's ingress-shim creates Certificate/<release>-ui-tls; deleting it cascades to CertificateRequest -> Order -> Challenge -> solver via ownerReferences. RBAC is scoped to cert-manager/acme resources only, hook ordering (SA -> Role/RoleBinding -> Job) is correct, and the no-blanket-sweep decision is sound and well-commented.

Non-blocking follow-up

1. The secondary kubectl delete certificate,order,challenge -l app.kubernetes.io/instance=<release> is effectively a no-op: cert-manager doesn't put app.kubernetes.io/instance on Certificate/Order/Challenge (Orders/Challenges carry cert-manager.io/certificate-name). Cleanup still works via the name-based Certificate delete + cascade, so it's harmless — but either switch the selector to cert-manager.io/certificate-name=<release>-ui-tls or drop the redundant line so it doesn't imply coverage it doesn't provide.

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/bucket/templates/hooks/cleanup-acme.yaml`:
- Around line 12-88: The cleanup Job in cleanup-acme.yaml needs runtime bounds
and safer kubectl behavior because the current delete/list flow can hang on
finalizers and hide list failures. Update the Job spec to enforce an execution
timeout with activeDeadlineSeconds, and adjust the kubectl calls in the cleanup
command so deletions use --wait=false and all kubectl get lookups set a request
timeout instead of relying on default waits. Also remove the /dev/null
redirection from the Challenge and Ingress list commands so failures are
surfaced in the job logs, using the existing cleanup command block and kubectl
delete/get loops as the places to change.

---

Nitpick comments:
In `@packages/apps/bucket/tests/cleanup_acme_test.yaml`:
- Around line 13-147: The cleanup-acme test suite only checks documents 0
through 3, so extra rendered manifests could be added without failing. Update
the cleanup-acme YAML test to assert the total rendered document count with a
hasDocuments count of 4, so templates like cleanup-acme.yaml cannot emit an
unexpected additional hook or RBAC object. Use the existing
cleanup_acme_test.yaml document assertions as the anchor for where to add the
count guard.
- Around line 14-85: The cleanup Job test coverage is missing the security
hardening assertions, so add checks in the bucket cleanup Helm test to verify
the Job pod template preserves the non-root, read-only, dropped-capability, and
RuntimeDefault seccomp settings. Update the existing cleanup Job assertions
around spec.template.spec and the container securityContext in
cleanup_acme_test.yaml so future changes to the cleanup template still enforce
these protections.

NOT LGTM — the orphaned-solver cleanup itself is correct and safe, but one line is a no-op carried by an inaccurate comment and pinned by a test, which should be cleaned up before merge.

The fix resolves the orphaned cert-manager ACME solver objects through three mechanisms I verified: the name-based kubectl delete certificate <release>-ui-tls plus owner cascade; a host-scoped Challenge sweep filtered on .spec.dnsName; and a host-scoped solver-Ingress sweep filtered on .spec.rules[0].host. The host value matches the Challenge dnsName and the solver Ingress rule host, and there is no namespace-wide blanket delete over the shared acme.cert-manager.io/http01-solver label — the label-wide get is filtered by host and only matching objects are deleted by name, so sibling apps are untouched. The bounded execution (activeDeadlineSeconds, backoffLimit: 0, --request-timeout=30s --wait=false), RBAC scope, and hardening are all correct, and the negative no-blanket-sweep test assertions are the right call.

Blocker (small, but it's a false-coverage + inaccurate-comment issue):

• templates/hooks/cleanup-acme.yaml:72 — kubectl delete order,challenge -n <ns> -l cert-manager.io/certificate-name=<release>-ui-tls is a no-op. Challenge objects carry no labels at all, and on an Order the cert-manager.io/certificate-name value is an annotation (copied from the CertificateRequest), not a label — so a label selector matches neither. The accompanying comment (:69-70, "cert-manager stamps them with its own cert-manager.io/certificate-name label") is therefore factually wrong, and tests/cleanup_acme_test.yaml:74 pins this no-op as if it were meaningful coverage. Orders are already reaped by the Certificate cascade and Challenges by the working dnsName sweep, so please drop this line along with its test assertion and the inaccurate comment, rather than keeping a selector that deletes nothing and a comment that misstates how cert-manager labels these objects.

LGTM — the previously-raised blocker is resolved; the orphaned-solver cleanup is correct and release-scoped.

The no-op label-based order/challenge delete (-l cert-manager.io/certificate-name=<release>-ui-tls, which matched nothing — Challenges are unlabeled and that value is an annotation on Orders, not a label) has been dropped, together with its inaccurate comment and the test assertion that pinned it. The replacement comment accurately explains why there is no label selector for Order/Challenge.

Re-verified against packages/system/bucket/templates/ingress.yaml:

• Certificate name <release>-ui-tls matches the ingress-shim secretName.
• HOST <release>.<_namespace.host> matches both the Challenge .spec.dnsName and the solver Ingress .spec.rules[0].host, by exact equality (not a prefix), so a sibling like <release>-2.<host> is never affected.
• The if not $wildcardSecret guard already suppresses per-host ACME, so wildcard mode has nothing to clean.
• RBAC stays least-privilege (no services/pods — solver Service/Pod cascade with their Challenge), execution is bounded (activeDeadlineSeconds 120, backoffLimit 0, per-call --request-timeout/--wait=false), and the script always exits 0 so a failure never blocks uninstall.

Tests cover the contract, including the negative assertions (no namespace-wide solver sweep, no services/pods RBAC).