fix(gateway): clean up orphaned ACME account secret on tenant module delete (#3089) - #3093
Conversation
…delete (#3089) When the gateway tenant module is disabled, cert-manager's ACME account private-key secret cozystack-acme-account was left orphaned (no ownerRef). Add a post-delete cleanup hook that deletes it by name. Verified the secret is referenced only by the gateway module's own cozystack-gateway Issuer, so removing it on gateway teardown is safe. The cleanup Job runs non-root with a read-only root fs and dropped capabilities; RBAC is scoped via resourceNames. Signed-off-by: Алексей Артамонов <[email protected]>
github-actions
Bot
added
area/networking
📝 WalkthroughWalkthroughAdds a post-delete Helm hook for the gateway chart that removes the release namespace's ChangesGateway ACME cleanup
Sequence Diagram(s)sequenceDiagram
participant Helm
participant gateway-cleanup Job
participant Kubernetes API
participant cozystack-acme-account Secret
Helm->>gateway-cleanup Job: create post-delete hook
gateway-cleanup Job->>Kubernetes API: delete cozystack-acme-account with --ignore-not-found
Kubernetes API->>cozystack-acme-account Secret: remove secret in release namespace
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
the
area/extra
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the 'cozystack-acme-account' secret remains in the tenant namespace after the gateway module is deleted. By implementing a hardened post-delete Helm hook, the system now ensures that this gateway-specific secret is cleaned up during teardown, preventing resource leakage while maintaining strict security standards. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a post-delete Helm hook to clean up the orphaned cozystack-acme-account secret when the gateway package is deleted, along with corresponding unit tests and a Makefile update. The review feedback recommends explicitly defining the namespace for the ServiceAccount subject in the RoleBinding to ensure proper RBAC resolution, and updating the unit tests to assert this namespace.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ .Release.Name }}-cleanup |
There was a problem hiding this comment.
For ServiceAccount subjects in a RoleBinding, it is highly recommended (and in some Kubernetes environments/validators, strictly required) to explicitly specify the namespace field. This ensures the RBAC controller can correctly resolve the ServiceAccount.
subjects:
- kind: ServiceAccount
name: {{ .Release.Name }}-cleanup
namespace: {{ .Release.Namespace }}| - equal: | ||
| path: subjects[0].kind | ||
| value: ServiceAccount | ||
| - equal: | ||
| path: subjects[0].name | ||
| value: gateway-cleanup |
There was a problem hiding this comment.
Add an assertion to verify that the namespace of the ServiceAccount subject in the RoleBinding is correctly set to the release namespace.
- equal:
path: subjects[0].kind
value: ServiceAccount
- equal:
path: subjects[0].name
value: gateway-cleanup
- equal:
path: subjects[0].namespace
value: tenant-root![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
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/extra/gateway/templates/hooks/cleanup-acme.yaml`:
- Around line 8-11: The Helm hook delete policy is missing failure cleanup,
which can leave orphaned resources behind when the post-delete hook fails.
Update the `helm.sh/hook-delete-policy` annotations in the `cleanup-acme.yaml`
template for the hook resources (including the Job, ServiceAccount, Role, and
RoleBinding) to add `hook-failed` alongside the existing policies. Use the
existing annotation blocks with the `helm.sh/hook-delete-policy` symbol to
ensure all cleanup resources are removed on both success and failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85e2ade1-b229-4c43-bada-5364038dc539
📒 Files selected for processing (3)
packages/extra/gateway/Makefilepackages/extra/gateway/templates/hooks/cleanup-acme.yamlpackages/extra/gateway/tests/cleanup_acme_test.yaml
| annotations: | ||
| "helm.sh/hook": post-delete | ||
| "helm.sh/hook-weight": "10" | ||
| "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In Helm 3 hook annotations, does helm.sh/hook-delete-policy: before-hook-creation,hook-succeededremove failedpost-deletehook resources automatically, or ishook-failed required to clean them up?
💡 Result:
The hook-succeeded policy does not remove a hook resource if the hook fails [1][2]. To automatically remove a hook resource when it fails, you must explicitly include hook-failed in your helm.sh/hook-delete-policy annotation [1][3][4]. If you define the annotation as helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded, Helm will only delete the resource if the hook execution completes successfully [1][3]. If the hook fails, that specific resource will remain in the cluster, which is often preferred to allow for debugging [3][4]. Important considerations regarding hook deletion: 1. Explicit Inclusion: If you want automatic cleanup of failed hooks, use helm.sh/hook-delete-policy: hook-failed (or combine it with others, such as hook-succeeded,hook-failed) [1][5][4]. 2. Cumulative Failures: In versions of Helm where specific issues with hook cleanup logic have been reported (e.g., when multiple hooks are defined), a failure in one hook may prevent the hook-succeeded policy from triggering for previous, successful hooks [6][7][8]. 3. Job TTL: Because Helm hooks are not tracked as part of the standard release lifecycle, many practitioners use the Kubernetes-native spec.ttlSecondsAfterFinished field on Job resources as a secondary or alternative method to ensure resources are cleaned up regardless of Helm's internal hook logic [1][9][3].
Citations:
- 1: https://helm.sh/docs/topics/charts_hooks/
- 2: https://github.com/helm/helm-www/blob/main/versioned_docs/version-3/topics/charts_hooks.md
- 3: https://kubernetes.recipes/recipes/helm/helm-hook-delete-policy/
- 4: https://devopsil.com/articles/2026-03-29-helm-hooks-and-tests
- 5: https://github.com/helm/helm/blob/dev-v2/docs/charts_hooks.md
- 6: fix: Process all hook deletions on failure helm/helm#30673
- 7: Hook delete policy not honored for preceding resource on a failure helm/helm#3714
- 8: Hook delete policy of "hook-failed" not honored for preceding resource on a failure helm/helm#10279
- 9: https://stackoverflow.com/questions/77748087/how-default-hook-delete-policy-works-in-helm-3
Add hook-failed to delete policies to prevent orphaned RBAC resources
The current helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded only removes hook resources if the job succeeds. If the post-delete hook fails (e.g., due to image pull errors, scheduling issues, or pre-flight failures), the Job, ServiceAccount, Role, and RoleBinding will remain in the namespace, leaving orphaned permissions behind.
Include hook-failed to ensure cleanup on any failure:
- Lines 11, 69, 80, and 96 need
hook-failedadded to theirhelm.sh/hook-delete-policyannotations.
Suggested fix
- "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
+ "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded,hook-failed
...
- "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
+ "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded,hook-failed
...
- "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
+ "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded,hook-failed
...
- "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
+ "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded,hook-failed🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/extra/gateway/templates/hooks/cleanup-acme.yaml` around lines 8 -
11, The Helm hook delete policy is missing failure cleanup, which can leave
orphaned resources behind when the post-delete hook fails. Update the
`helm.sh/hook-delete-policy` annotations in the `cleanup-acme.yaml` template for
the hook resources (including the Job, ServiceAccount, Role, and RoleBinding) to
add `hook-failed` alongside the existing policies. Use the existing annotation
blocks with the `helm.sh/hook-delete-policy` symbol to ensure all cleanup
resources are removed on both success and failure.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM.
Verified the deleted secret is genuinely per-tenant and orphaned, so the cleanup is safe:
- the gateway renders a namespaced
Issuerin the tenant namespace (internal/controller/tenantgateway/renderers.go:234), and its ACME account key is<TenantGateway>-acme-account; the TenantGateway is namedcozystack(packages/extra/gateway/templates/tenantgateway.yaml), so the target iscozystack-acme-accountin the tenant namespace — exactly what the hook deletes in.Release.Namespace. - it is not the shared platform account: the cluster-wide
letsencrypt-prod/letsencrypt-stageissuers use account secrets namedletsencrypt-prod/letsencrypt-stagein the cert-manager namespace — no collision. - the Issuer is owned by the TenantGateway and cascades; the cert-manager-created account key has no ownerReference and is the genuine orphan.
RBAC is tightly scoped (resourceNames: [cozystack-acme-account], get,delete), the pod is hardened, and the tests pin the hook shape, RBAC scope, and that the delete targets the per-tenant secret by name and namespace.
Non-blocking suggestions:
- Set
subjects[0].namespace: {{ .Release.Namespace }}on the RoleBinding to match the existing teardown convention and satisfy strict RBAC validators (it works without it, but explicit is the house style). - Consider adding
hook-failedto thehelm.sh/hook-delete-policyannotations so a failed hook does not leave the Job/SA/Role/RoleBinding behind until the next uninstall. - Out of scope here, but with
enableCertificateOwnerRef: falsethe per-listener TLS certificate secrets are also not garbage-collected on teardown — worth a follow-up. - Optional: assert the SA (weight 0) and Role/RoleBinding (weight 5) hook weights in the unit test, since the SA->RBAC->Job ordering is the correctness-critical part of the hook.
2 participants
What this PR does
When the
gatewaytenant module is disabled (its HelmRelease uninstalled), cert-manager's ACME account private-key secretcozystack-acme-accountwas left orphaned in the tenant namespace (labelapp.kubernetes.io/managed-by: cert-manager, noownerReferences).This adds a
post-deletecleanup hook (packages/extra/gateway/templates/hooks/cleanup-acme.yaml) that deletes the secret by name. Verified on a live cluster thatcozystack-acme-accountis referenced by exactly one Issuer — the gateway module's owncozystack-gatewayIssuer — so it is gateway-specific and safe to remove on gateway teardown (it is not shared with other modules, which use the cluster-wideletsencrypt-prodissuer).The cleanup Job is hardened: non-root (uid 65534), read-only root fs, all capabilities dropped,
seccompProfile: RuntimeDefault, resource requests/limits; RBAC scoped viaresourceNamesto exactlycozystack-acme-account(get/delete). A failure is logged but never blocks teardown.No user data is affected — only cert-manager's ACME account key (re-registered on next enable).
Fixes #3089.
Release note
Summary by CodeRabbit
New Features
Bug Fixes