fix(keycloak-configure): patch HelmRelease in release namespace on teardown - #3372
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Keycloak configure chart’s pre-delete Job now patches the HelmRelease using the current Helm release name and namespace. New tests verify default and custom release metadata and reject mismatched namespaces. ChangesKeycloak teardown cleanup
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 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 |
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 resolves a deadlock issue occurring during the uninstallation of the keycloak-configure component. Previously, the teardown Job attempted to patch a HelmRelease in a hardcoded namespace, which caused permission errors and left the release in a 'Terminating' state. By parameterizing the patch command with Helm template variables, the Job now correctly targets the release's actual namespace, allowing for successful cleanup. 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
|
There was a problem hiding this comment.
Code Review
This pull request parameterizes the teardown job in keycloak-configure to patch the HelmRelease using the dynamic release name and namespace instead of hardcoded values, and adds a test suite to verify this behavior. The review feedback suggests making the patch command idempotent by checking if the HelmRelease exists before patching, which prevents the job from failing and retrying indefinitely if the resource has already been deleted.
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.
| done | ||
|
|
||
| kubectl patch hr keycloak-configure -n cozy-system --type=merge -p '{"metadata":{"finalizers":[]}}' | ||
| kubectl patch hr {{ .Release.Name }} -n {{ .Release.Namespace }} --type=merge -p '{"metadata":{"finalizers":[]}}' |
There was a problem hiding this comment.
Since this kubectl patch is the last command in the shell script, its exit status determines the exit status of the entire container/Job. If the HelmRelease has already been deleted (e.g., in a previous or concurrent teardown attempt, or if it was manually cleaned up), kubectl patch will fail with a NotFound error (exit code 1), causing the teardown Job to fail and retry indefinitely.
To make the teardown Job idempotent and robust, we should check if the HelmRelease exists before attempting to patch it. If it doesn't exist, the script can exit successfully (exit code 0).
if kubectl get hr {{ .Release.Name }} -n {{ .Release.Namespace }} >/dev/null 2>&1; then
kubectl patch hr {{ .Release.Name }} -n {{ .Release.Namespace }} --type=merge -p '{"metadata":{"finalizers":[]}}'
fi…ardown The pre-delete teardown Job clears the HelmRelease finalizers as its final step, but targeted a hardcoded namespace that does not match the namespace where the release and its RBAC live. The Job's Role and RoleBinding are created in the release namespace and grant patch on the HelmRelease by release name, so the ServiceAccount was Forbidden to patch the HelmRelease in the hardcoded namespace. The Job errored and retried forever, the HelmRelease stuck in Terminating, and the Helm release wedged in "uninstalling", blocking any teardown or reinstall. Target the release name and namespace so the patch matches the RBAC that grants it. Add a helm-unittest pinning the rendered namespace. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
f192234 to
fd568de
Compare
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM — verified. This fixes a real teardown deadlock: the pre-delete hook patched the HelmRelease in cozy-system, but the release installs into its own namespace (cozy-keycloak), so the patch was Forbidden and the uninstall hung. Patching -n {{ .Release.Namespace }} now matches the namespaced Role grant; the hook is pre-delete-only, idempotent (--type=merge clearing finalizers), and covered by render + anti-regression tests. The red E2E is the known node-join flake, and this pre-delete hook is not exercised by a normal install.
|
Successfully created backport PR for |
|
Successfully created backport PR for |
What this PR does
The
keycloak-configurepre-delete teardown Job clears the FluxHelmReleasefinalizers as its final step so the release can be uninstalled. That finalkubectl patchtargeted a hardcoded namespace that does not match where the release actually installs. The release installs intocozy-keycloak, and the teardown Job's own RBAC — aRole+RoleBindinggrantingpatchon theHelmReleaseby release name — is created in the release namespace, but the patch was aimed atcozy-system.The result is a deadlock: the Job's
ServiceAccountisForbiddento patch theHelmReleasein the mismatched namespace, so the Job errors and retries forever, theHelmReleasesticks inTerminating, and the Helm release wedges in statusuninstalling. This blocks any teardown or reinstall of the component.The fix templates both the release name and the namespace from
{{ .Release.Name }}/{{ .Release.Namespace }}, so the patch targets the namespace where theHelmReleaselives and where the teardown RBAC grants access. I also added a helm-unittest that pins the rendered target, guards against the hardcoded namespace regressing, and asserts the patch tracks the release name and namespace.Screenshots
N/A — no UI changes.
Release note
Summary by CodeRabbit