From 58b2a3d77919fc0db2958e36ff08f7e801967926 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D0=BB=D0=B5=D0=BA=D1=81=D0=B5=D0=B9=20=D0=90=D1=80?= =?UTF-8?q?=D1=82=D0=B0=D0=BC=D0=BE=D0=BD=D0=BE=D0=B2?= Date: Fri, 26 Jun 2026 14:36:47 +0300 Subject: [PATCH] fix(bucket): harden ACME cleanup hook and fix cert-manager label selector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on the post-delete cleanup hook: - Target Orders/Challenges by cert-manager's own cert-manager.io/certificate-name=-ui-tls label instead of the Helm app.kubernetes.io/instance label, which cert-manager never sets on those resources (the old selector was a silent no-op). - Bound the Job with activeDeadlineSeconds=120 and backoffLimit=0 so a stuck finalizer or hung API call cannot block the uninstall. - Add --wait=false --request-timeout=30s to every kubectl delete/get so the hook never hangs on slow or finalizer-blocked deletes. - Surface list failures in the job log instead of redirecting to /dev/null. - Tests: assert the bounded execution and security-hardening contract, pin the rendered document count to 4, and update selector assertions. Assisted-By: Claude Signed-off-by: Алексей Артамонов --- .../bucket/templates/hooks/cleanup-acme.yaml | 24 ++++++++--- .../apps/bucket/tests/cleanup_acme_test.yaml | 40 +++++++++++++++++-- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/packages/apps/bucket/templates/hooks/cleanup-acme.yaml b/packages/apps/bucket/templates/hooks/cleanup-acme.yaml index e76b381eb..48d829d4f 100644 --- a/packages/apps/bucket/templates/hooks/cleanup-acme.yaml +++ b/packages/apps/bucket/templates/hooks/cleanup-acme.yaml @@ -10,6 +10,10 @@ metadata: "helm.sh/hook-weight": "10" "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded spec: + # Bound the whole cleanup: never let a stuck finalizer or a hung API call + # leave this post-delete hook running indefinitely and block the uninstall. + activeDeadlineSeconds: 120 + backoffLimit: 0 template: metadata: labels: @@ -59,9 +63,13 @@ spec: # then cascade-removes the cm-acme-http-solver-* solver objects it # owns. The release's TLS Certificate is named after the secret it # populates ({{ .Release.Name }}-ui-tls). - kubectl delete certificate {{ .Release.Name }}-ui-tls -n {{ .Release.Namespace }} --ignore-not-found \ + kubectl delete certificate {{ .Release.Name }}-ui-tls -n {{ .Release.Namespace }} --ignore-not-found --wait=false --request-timeout=30s \ || echo "WARNING: cert-manager cleanup failed for {{ .Release.Name }}; orphaned ACME resources may remain" >&2 - kubectl delete certificate,order,challenge -n {{ .Release.Namespace }} -l app.kubernetes.io/instance={{ .Release.Name }} --ignore-not-found \ + # Orders/Challenges do NOT carry the Helm app.kubernetes.io/instance + # label; cert-manager stamps them with its own + # cert-manager.io/certificate-name label, so target them by the + # Certificate name to reliably reap any that outlived the cascade. + kubectl delete order,challenge -n {{ .Release.Namespace }} -l cert-manager.io/certificate-name={{ .Release.Name }}-ui-tls --ignore-not-found --wait=false --request-timeout=30s \ || echo "WARNING: cert-manager cleanup failed for {{ .Release.Name }}; orphaned ACME resources may remain" >&2 # Release-scoped solver sweep. We deliberately do NOT delete # ingress/service/pod objects namespace-wide by the @@ -73,8 +81,10 @@ spec: # # Delete Challenges for our host (cascades the solver # Ingress/Service/Pod they own). - for ch in $(kubectl get challenge -n {{ .Release.Namespace }} -o jsonpath="{range .items[?(@.spec.dnsName=='$HOST')]}{.metadata.name} {end}" 2>/dev/null); do - kubectl delete challenge -n {{ .Release.Namespace }} "$ch" --ignore-not-found \ + challenges="$(kubectl get challenge -n {{ .Release.Namespace }} --request-timeout=30s -o jsonpath="{range .items[?(@.spec.dnsName=='$HOST')]}{.metadata.name} {end}")" \ + || { echo "WARNING: failed to list challenges for {{ .Release.Name }}; orphaned ACME resources may remain" >&2; challenges=""; } + for ch in $challenges; do + kubectl delete challenge -n {{ .Release.Namespace }} "$ch" --ignore-not-found --wait=false --request-timeout=30s \ || echo "WARNING: failed to delete challenge $ch for {{ .Release.Name }}; orphaned ACME resources may remain" >&2 done # Delete any orphaned solver Ingress for our host (no live owner @@ -82,8 +92,10 @@ spec: # Challenge and are cascade-deleted with it; truly owner-less # solver Service/Pod with no matching Challenge are not expected # and are not swept namespace-wide. - for ing in $(kubectl get ingress -n {{ .Release.Namespace }} -l acme.cert-manager.io/http01-solver=true -o jsonpath="{range .items[?(@.spec.rules[0].host=='$HOST')]}{.metadata.name} {end}" 2>/dev/null); do - kubectl delete ingress -n {{ .Release.Namespace }} "$ing" --ignore-not-found \ + ingresses="$(kubectl get ingress -n {{ .Release.Namespace }} -l acme.cert-manager.io/http01-solver=true --request-timeout=30s -o jsonpath="{range .items[?(@.spec.rules[0].host=='$HOST')]}{.metadata.name} {end}")" \ + || { echo "WARNING: failed to list solver ingresses for {{ .Release.Name }}; orphaned ACME resources may remain" >&2; ingresses=""; } + for ing in $ingresses; do + kubectl delete ingress -n {{ .Release.Namespace }} "$ing" --ignore-not-found --wait=false --request-timeout=30s \ || echo "WARNING: failed to delete solver ingress $ing for {{ .Release.Name }}; orphaned ACME resources may remain" >&2 done echo "ACME cleanup complete." diff --git a/packages/apps/bucket/tests/cleanup_acme_test.yaml b/packages/apps/bucket/tests/cleanup_acme_test.yaml index 388617de4..d6e7ae314 100644 --- a/packages/apps/bucket/tests/cleanup_acme_test.yaml +++ b/packages/apps/bucket/tests/cleanup_acme_test.yaml @@ -11,6 +11,11 @@ set: host: example.org tests: + - it: renders only the expected cleanup hook objects (Job, SA, Role, RoleBinding) + asserts: + - hasDocuments: + count: 4 + - it: renders a post-delete cleanup Job with the expected name and SA documentIndex: 0 asserts: @@ -32,6 +37,29 @@ tests: - equal: path: spec.template.metadata.labels["policy.cozystack.io/allow-to-apiserver"] value: "true" + # Bounded execution so a stuck hook can't block the uninstall. + - equal: + path: spec.activeDeadlineSeconds + value: 120 + - equal: + path: spec.backoffLimit + value: 0 + # Security hardening contract the PR relies on. + - equal: + path: spec.template.spec.securityContext.runAsNonRoot + value: true + - equal: + path: spec.template.spec.containers[0].securityContext.readOnlyRootFilesystem + value: true + - equal: + path: spec.template.spec.containers[0].securityContext.allowPrivilegeEscalation + value: false + - equal: + path: spec.template.spec.containers[0].securityContext.capabilities.drop[0] + value: ALL + - equal: + path: spec.template.spec.securityContext.seccompProfile.type + value: RuntimeDefault - it: deletes release-scoped cert-manager resources with a host-scoped solver sweep documentIndex: 0 @@ -39,9 +67,15 @@ tests: - matchRegex: path: spec.template.spec.containers[0].command[2] pattern: "kubectl delete certificate my-bucket-ui-tls -n tenant-root" + # Orders/Challenges are targeted by cert-manager's own + # cert-manager.io/certificate-name label, not the Helm release label. - matchRegex: path: spec.template.spec.containers[0].command[2] - pattern: "kubectl delete certificate,order,challenge -n tenant-root -l app.kubernetes.io/instance=my-bucket" + pattern: "kubectl delete order,challenge -n tenant-root -l cert-manager.io/certificate-name=my-bucket-ui-tls" + # The release label selector must NOT be used for cert-manager resources. + - notMatchRegex: + path: spec.template.spec.containers[0].command[2] + pattern: "delete .*-l app.kubernetes.io/instance=my-bucket" # Host derived from .<_namespace.host> == my-bucket.example.org - matchRegex: path: spec.template.spec.containers[0].command[2] @@ -49,14 +83,14 @@ tests: # Host-scoped challenge sweep filtering on .spec.dnsName == our host - matchRegex: path: spec.template.spec.containers[0].command[2] - pattern: "kubectl get challenge -n tenant-root -o jsonpath=.*@.spec.dnsName=='\\$HOST'" + pattern: "kubectl get challenge -n tenant-root --request-timeout=30s -o jsonpath=.*@.spec.dnsName=='\\$HOST'" - matchRegex: path: spec.template.spec.containers[0].command[2] pattern: 'kubectl delete challenge -n tenant-root "\$ch" --ignore-not-found' # Host-scoped orphaned solver Ingress sweep filtering on rules[0].host - matchRegex: path: spec.template.spec.containers[0].command[2] - pattern: "kubectl get ingress -n tenant-root -l acme.cert-manager.io/http01-solver=true -o jsonpath=.*@.spec.rules\\[0\\].host=='\\$HOST'" + pattern: "kubectl get ingress -n tenant-root -l acme.cert-manager.io/http01-solver=true --request-timeout=30s -o jsonpath=.*@.spec.rules\\[0\\].host=='\\$HOST'" - matchRegex: path: spec.template.spec.containers[0].command[2] pattern: 'kubectl delete ingress -n tenant-root "\$ing" --ignore-not-found'