Repository navigation
fix(kubernetes): clear kamaji datastore-secret finalizer to unblock namespace deletion (#3062) - #3078
Conversation
github-actions
Bot
added
area/kubernetes
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pre-delete cleanup hook now hardens its pod settings, waits for ChangesKubernetes pre-delete cleanup hook
Possibly related issues
Suggested reviewers
Estimated review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
priority/critical-urgent
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 a critical issue where Kubernetes app/tenant deletion would hang in a 'Terminating' state due to a leftover Kamaji finalizer on the datastore-config Secret. By ensuring the TenantControlPlane is fully removed before attempting cleanup and introducing a targeted finalizer-clearing backstop, the PR ensures successful namespace garbage collection. Additionally, the cleanup Job has been significantly hardened with security best practices and verified through new unit tests. 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
|
![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
🧹 Nitpick comments (1)
packages/apps/kubernetes/tests/delete_hook_test.yaml (1)
44-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the 120s timeout in the wait regression test.
This asserts
--wait=truebut not--timeout=120s, so the hook could regress to an unbounded wait while the test still passes.Proposed test tightening
- pattern: 'delete tenantcontrolplanes\.kamaji\.clastix\.io test-k8s[\s\S]*--wait=true' + pattern: 'delete tenantcontrolplanes\.kamaji\.clastix\.io test-k8s[\s\S]*--wait=true --timeout=120s'🤖 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/apps/kubernetes/tests/delete_hook_test.yaml` around lines 44 - 46, The wait regression test only checks for --wait=true in the delete hook command, so it can miss a regression where timeout is removed or unbounded. Update the matchRegex in delete_hook_test.yaml for spec.template.spec.containers[0].command[2] to also require --timeout=120s alongside --wait=true, keeping the assertion anchored to the delete tenantcontrolplanes.kamaji.clastix.io test-k8s command.
🤖 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/apps/kubernetes/templates/delete.yaml`:
- Around line 73-97: The delete-and-patch flow in the TenantControlPlane cleanup
is too permissive because the `kubectl delete` in the `delete.yaml` template is
allowed to fail without stopping, so the later Secret finalizer removal can run
even if the `TenantControlPlane` still exists. Update the logic around the
`kubectl delete tenantcontrolplanes.kamaji.clastix.io` command and the
subsequent `secret/{{ .Release.Name }}-datastore-config` patch so the finalizer
is cleared only after explicitly confirming the `TenantControlPlane` is absent
(for example by checking with `kubectl get` before proceeding); keep the
existing `if kubectl -n ... get secret ...` gate, but add the owner-existence
verification before the patch step.
---
Nitpick comments:
In `@packages/apps/kubernetes/tests/delete_hook_test.yaml`:
- Around line 44-46: The wait regression test only checks for --wait=true in the
delete hook command, so it can miss a regression where timeout is removed or
unbounded. Update the matchRegex in delete_hook_test.yaml for
spec.template.spec.containers[0].command[2] to also require --timeout=120s
alongside --wait=true, keeping the assertion anchored to the delete
tenantcontrolplanes.kamaji.clastix.io test-k8s command.
🪄 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: 52fdb9d3-3a29-41bf-82b5-b62639b5b7e9
📒 Files selected for processing (2)
packages/apps/kubernetes/templates/delete.yamlpackages/apps/kubernetes/tests/delete_hook_test.yaml
![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 updates the pre-delete cleanup hook for the Kubernetes application package to resolve namespace deletion issues (issue #3062). It hardens the security posture of the cleanup job by running it as non-root, dropping capabilities, and using a read-only root filesystem with a temporary volume. Additionally, the deletion script now waits for the TenantControlPlane to be removed and includes a fallback step to strip the leftover datastore-secret finalizer from the datastore-config Secret. RBAC rules have been scoped down to allow patching only this specific Secret, and comprehensive unit tests have been added to verify the behavior. I have no feedback to provide as the implementation is robust and follows best practices.
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.
b223d36 to
708b3bb
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the finalizer-strip backstop runs in a path its own safety comment declares unsafe.
Business context: a leftover finalizer.kamaji.clastix.io/datastore-secret on the <release>-datastore-config Secret can block namespace deletion when a TenantControlPlane is force-deleted or Kamaji doesn't reconcile in time (#3062).
Blockers
B1: Step 4b strips the Secret finalizer even when the TenantControlPlane deletion timed out
File: packages/apps/kubernetes/templates/delete.yaml (Step 4 / Step 4b)
Issue: Step 4's kubectl delete tenantcontrolplanes ... --wait=true --timeout=120s || echo WARNING suppresses a timeout failure, and Step 4b then strips the Secret's finalizers gated only on the Secret existing. The Step 4b comment justifies the strip as safe because "by this point the TenantControlPlane is gone (Step 4 waited for it)" — but on timeout the TCP is not gone, so the documented precondition is violated.
Impact: (a) stripping finalizer.kamaji.clastix.io/datastore-secret while the owning TCP still exists bypasses Kamaji's datastore-secret lifecycle cleanup, risking orphaned tenant state in the shared datastore; (b) it doesn't even achieve the goal — if the TCP still exists, namespace deletion is blocked by the TCP itself, not by the Secret, so the premature strip gains nothing.
Fix: gate Step 4b on confirming the TCP is actually absent (e.g. only strip if kubectl get tenantcontrolplanes <name> returns not-found; otherwise log a warning and leave the finalizer). Keep the backstop for its legitimate case — TCP gone but Kamaji didn't strip the finalizer in time (force-delete) — just don't run it while the TCP is alive.
Note: the helm-unittest pins that --wait=true and the finalizer patch appear in the command string, but the runtime control-flow safety — the actual concern — can't be covered by helm-unittest, so it must be correct by construction.
Non-blocking
The rest is solid: the securityContext hardening, resource requests/limits, HOME=/tmp + tmp emptyDir for readOnlyRootFilesystem, and the resourceNames-scoped RBAC (get/patch on exactly <release>-datastore-config) are all correct.
…amespace deletion (#3062) After deleting a Kubernetes app/tenant, the <release>-datastore-config Secret kept the finalizer finalizer.kamaji.clastix.io/datastore-secret, blocking deletion of the whole namespace. Extend the pre-delete hook to wait for the TenantControlPlane to be removed (so Kamaji can strip the finalizer itself), then, as a backstop, clear the finalizer on the datastore-config Secret if it still lingers. RBAC is scoped via resourceNames to that one Secret (get/patch). The Job runs non-root with a read-only root fs, dropped capabilities and a writable /tmp for the kubectl cache; failures are logged. Signed-off-by: Алексей Артамонов <[email protected]>
708b3bb to
4b9d67f
Compare
|
Aleksei Sviridkin (@lexfrei) thanks for the careful review — B1 is addressed in Concretely:
PTAL when you get a chance. |
|
myasnikovdaniil thanks for the pointer to #3018 — I looked into it, and I think we're actually dealing with two distinct Kamaji finalizer bugs on two different objects, so the kamaji bump and this hook are complementary rather than alternatives:
I checked clastix/kamaji#1122 ("fix(ds): allow deletion for unused datastores", closes clastix/kamaji#1115): it changes only So my read is:
If you have signal that a later kamaji version also fixes the secret-finalizer path (or that the two are coupled), I'm very happy to drop/trim this hook in favour of the bump — could you point me at it? Otherwise I'd suggest keeping both, and I can add a TODO to revisit the hook once kamaji is bumped. (Separately: the timeout-gating concern Aleksei Sviridkin (@lexfrei) raised is now fixed in |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — B1 from the previous review is resolved.
The Step 4b finalizer strip is now gated on the TenantControlPlane being confirmed absent: if kubectl get tenantcontrolplanes <name> still finds it (Step 4 timed out), the finalizer is left untouched with an explanatory log, and the strip runs only in the elif branch once the TCP is gone — which preserves the force-delete backstop while honoring the safety precondition the comment documents. The added test pins the get-TCP gate ahead of the patch secret call.
No remaining blockers. The earlier non-blocking notes still hold (the securityContext hardening, resource limits, HOME=/tmp + tmp emptyDir, and the resourceNames-scoped RBAC are all correct).
|
scooby87 I think it is may be the same thing, this secret also managed by kamaji, and also wedged because of never cleared finalizer, did you test fix that i proposed? may be we can eliminate multiple issues at once. |
3 participants
What this PR does
After deleting a
Kubernetesapp CR / tenant, the secret<release>-datastore-configkept the finalizerfinalizer.kamaji.clastix.io/datastore-secret, which was never removed —blocking deletion of the entire namespace (observed stuck in
Terminatingfor 115 minutes). Kamaji adds this finalizer to the datastore connection Secret
and normally strips it as part of TenantControlPlane (TCP) deletion; if the TCP
is force-deleted or Kamaji does not reconcile in time, the finalizer hangs and
the Secret can never be garbage-collected, wedging the namespace.
This extends the existing
pre-deletecleanup hook(
packages/apps/kubernetes/templates/delete.yaml):--wait=true --timeout=120sso Kamaji's own finalizer-cleanup chain has a chance to run before we fall
through.
metadata.finalizers: nullon<release>-datastore-configonly if it still exists. By this point the TCP isgone, the Secret is owner-ref'd to the deleted TCP and only awaiting GC, so
clearing the finalizer merely lets GC reap it — it cannot strip a finalizer
while a live TCP still needs the datastore. The Secret holds only connection
config, not tenant data (which lives in the external etcd datastore), so no
data is lost.
secretsrule scoped viaresourceNamesto exactly<release>-datastore-config, verbsget/patchonly.The Job is hardened: non-root (uid 65534), read-only root fs, all caps dropped,
seccompProfile: RuntimeDefault, writable/tmpfor kubectl cache. Failures atboth steps are logged (the Step 4b warning explicitly mentions the namespace may
stay Terminating) but the hook still completes so it cannot itself block teardown.
This is the most impactful of the create→delete leak sweep findings: without it,
tenant teardown hangs and the create→delete cycle requires manual intervention.
Fixes #3062.
Release note
Summary by CodeRabbit
TenantControlPlaneand handle slow deletions with a bounded wait.finalizersfrom the relevant datastore Secret so namespace termination isn’t blocked.TenantControlPlaneis gone.