fix(migration): suspend cozy-proxy if it conflicts with installer release#2128
fix(migration): suspend cozy-proxy if it conflicts with installer release#2128
Conversation
📝 WalkthroughWalkthroughAdds a new initial step to the migration script that detects a conflicting cozy-proxy HelmRelease with releaseName "cozystack", interactively prompts the user to suspend it, and patches the resource if agreed, or aborts the migration if refused. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 enhances the robustness of the system's Helm deployments and migration process. It introduces measures to safeguard critical Kubernetes resources from unintended deletion during Helm uninstallations and addresses a specific conflict in the migration script that could lead to data loss or operational issues during upgrades. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Changelog
Activity
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 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 counter productive. You can react with 👍 and 👎 on @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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
kvaps
changed the title
![gemini-code-assist[bot]](proxy.php?url=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 important fixes for the v1.0 migration. It adds a helm.sh/resource-policy: keep annotation to the cozy-system namespace to prevent its deletion by Helm, and it enhances the migration script to handle a potential conflict with the cozy-proxy HelmRelease. The logic in the migration script is sound, but I have one suggestion to improve its robustness by using the full Kubernetes resource name instead of a short name.
| # In v0.41.x, cozy-proxy was incorrectly configured with releaseName "cozystack", | ||
| # which conflicts with the installer helm release name. If not suspended, cozy-proxy | ||
| # HelmRelease will overwrite the installer release and delete cozystack-operator. | ||
| COZY_PROXY_RELEASE_NAME=$(kubectl get hr -n "$NAMESPACE" cozy-proxy -o jsonpath='{.spec.releaseName}' 2>/dev/null || true) |
There was a problem hiding this comment.
For better robustness in this migration script, it's recommended to use the full resource name helmreleases instead of the short name hr. Short names are not guaranteed to be available on all clusters, and using the full name makes the script more reliable. This change should also be applied on lines 72 and 77 where hr is also used.
| COZY_PROXY_RELEASE_NAME=$(kubectl get hr -n "$NAMESPACE" cozy-proxy -o jsonpath='{.spec.releaseName}' 2>/dev/null || true) | |
| COZY_PROXY_RELEASE_NAME=$(kubectl get helmreleases -n "$NAMESPACE" cozy-proxy -o jsonpath='{.spec.releaseName}' 2>/dev/null || true) |
…ease In v0.41.x, cozy-proxy HelmRelease was configured with releaseName: cozystack, which collides with the installer helm release. If not suspended before upgrade, the cozy-proxy HR reconciles and overwrites the installer release, deleting cozystack-operator. Add a check in the migration script that detects this conflict and suspends the cozy-proxy HelmRelease before proceeding. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
kvaps
force-pushed
the
fix/remove-namespace-from-installer
branch
from
da00ceb to
14a9017
Compare
kvaps
requested review from
IvanHunters,
androndo,
lexfrei,
lllamnyp and
sircthulhu
as code owners
dosubot
bot
added
size:S
![coderabbitai[bot]](proxy.php?url=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 the current code and only fix it if needed.
Inline comments:
In `@hack/migrate-to-version-1.0.sh`:
- Around line 64-79: The script currently aborts if the user answers "N" even
when the HelmRelease cozy-proxy is already suspended; modify the block that
checks COZY_PROXY_RELEASE_NAME so it first queries the HelmRelease .spec.suspend
(e.g., using kubectl get hr cozy-proxy -n "$NAMESPACE" -o
jsonpath='{.spec.suspend}') and if that value is true skip prompting and
continue, otherwise prompt as before and only abort if the user declines and
.spec.suspend is false; update the logic around the REPLY check and the kubectl
patch hr cozy-proxy command to reflect this new pre-check so answering "N" does
not cause an exit when .spec.suspend is already true.
| if [ "$COZY_PROXY_RELEASE_NAME" = "cozystack" ]; then | ||
| echo "WARNING: HelmRelease cozy-proxy has releaseName 'cozystack', which conflicts" | ||
| echo "with the installer release. It must be suspended before proceeding, otherwise" | ||
| echo "it will overwrite the installer and delete cozystack-operator." | ||
| echo "" | ||
| read -p "Suspend HelmRelease cozy-proxy? (y/N) " -n 1 -r | ||
| echo "" | ||
| if [[ $REPLY =~ ^[Yy]$ ]]; then | ||
| kubectl -n "$NAMESPACE" patch hr cozy-proxy --type=merge --field-manager=flux-client-side-apply -p '{"spec":{"suspend":true}}' | ||
| echo "HelmRelease cozy-proxy suspended." | ||
| else | ||
| echo "ERROR: Cannot proceed with conflicting cozy-proxy HelmRelease active." | ||
| echo "Please suspend it manually:" | ||
| echo " kubectl -n $NAMESPACE patch hr cozy-proxy --type=merge -p '{\"spec\":{\"suspend\":true}}'" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Avoid aborting when cozy-proxy is already suspended.
At Line 64, the branch is based only on releaseName. If .spec.suspend is already true, answering N at Line 69 still aborts at Line 78 even though the conflict is already mitigated.
💡 Suggested fix
COZY_PROXY_RELEASE_NAME=$(kubectl get hr -n "$NAMESPACE" cozy-proxy -o jsonpath='{.spec.releaseName}' 2>/dev/null || true)
+COZY_PROXY_SUSPENDED=$(kubectl get hr -n "$NAMESPACE" cozy-proxy -o jsonpath='{.spec.suspend}' 2>/dev/null || true)
if [ "$COZY_PROXY_RELEASE_NAME" = "cozystack" ]; then
+ if [ "$COZY_PROXY_SUSPENDED" = "true" ]; then
+ echo "HelmRelease cozy-proxy is already suspended."
+ echo ""
+ else
echo "WARNING: HelmRelease cozy-proxy has releaseName 'cozystack', which conflicts"
echo "with the installer release. It must be suspended before proceeding, otherwise"
echo "it will overwrite the installer and delete cozystack-operator."
echo ""
read -p "Suspend HelmRelease cozy-proxy? (y/N) " -n 1 -r
echo ""
if [[ $REPLY =~ ^[Yy]$ ]]; then
kubectl -n "$NAMESPACE" patch hr cozy-proxy --type=merge --field-manager=flux-client-side-apply -p '{"spec":{"suspend":true}}'
echo "HelmRelease cozy-proxy suspended."
else
echo "ERROR: Cannot proceed with conflicting cozy-proxy HelmRelease active."
echo "Please suspend it manually:"
echo " kubectl -n $NAMESPACE patch hr cozy-proxy --type=merge -p '{\"spec\":{\"suspend\":true}}'"
exit 1
fi
echo ""
+ fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$COZY_PROXY_RELEASE_NAME" = "cozystack" ]; then | |
| echo "WARNING: HelmRelease cozy-proxy has releaseName 'cozystack', which conflicts" | |
| echo "with the installer release. It must be suspended before proceeding, otherwise" | |
| echo "it will overwrite the installer and delete cozystack-operator." | |
| echo "" | |
| read -p "Suspend HelmRelease cozy-proxy? (y/N) " -n 1 -r | |
| echo "" | |
| if [[ $REPLY =~ ^[Yy]$ ]]; then | |
| kubectl -n "$NAMESPACE" patch hr cozy-proxy --type=merge --field-manager=flux-client-side-apply -p '{"spec":{"suspend":true}}' | |
| echo "HelmRelease cozy-proxy suspended." | |
| else | |
| echo "ERROR: Cannot proceed with conflicting cozy-proxy HelmRelease active." | |
| echo "Please suspend it manually:" | |
| echo " kubectl -n $NAMESPACE patch hr cozy-proxy --type=merge -p '{\"spec\":{\"suspend\":true}}'" | |
| exit 1 | |
| fi | |
| COZY_PROXY_RELEASE_NAME=$(kubectl get hr -n "$NAMESPACE" cozy-proxy -o jsonpath='{.spec.releaseName}' 2>/dev/null || true) | |
| COZY_PROXY_SUSPENDED=$(kubectl get hr -n "$NAMESPACE" cozy-proxy -o jsonpath='{.spec.suspend}' 2>/dev/null || true) | |
| if [ "$COZY_PROXY_RELEASE_NAME" = "cozystack" ]; then | |
| if [ "$COZY_PROXY_SUSPENDED" = "true" ]; then | |
| echo "HelmRelease cozy-proxy is already suspended." | |
| echo "" | |
| else | |
| echo "WARNING: HelmRelease cozy-proxy has releaseName 'cozystack', which conflicts" | |
| echo "with the installer release. It must be suspended before proceeding, otherwise" | |
| echo "it will overwrite the installer and delete cozystack-operator." | |
| echo "" | |
| read -p "Suspend HelmRelease cozy-proxy? (y/N) " -n 1 -r | |
| echo "" | |
| if [[ $REPLY =~ ^[Yy]$ ]]; then | |
| kubectl -n "$NAMESPACE" patch hr cozy-proxy --type=merge --field-manager=flux-client-side-apply -p '{"spec":{"suspend":true}}' | |
| echo "HelmRelease cozy-proxy suspended." | |
| else | |
| echo "ERROR: Cannot proceed with conflicting cozy-proxy HelmRelease active." | |
| echo "Please suspend it manually:" | |
| echo " kubectl -n $NAMESPACE patch hr cozy-proxy --type=merge -p '{\"spec\":{\"suspend\":true}}'" | |
| exit 1 | |
| fi | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/migrate-to-version-1.0.sh` around lines 64 - 79, The script currently
aborts if the user answers "N" even when the HelmRelease cozy-proxy is already
suspended; modify the block that checks COZY_PROXY_RELEASE_NAME so it first
queries the HelmRelease .spec.suspend (e.g., using kubectl get hr cozy-proxy -n
"$NAMESPACE" -o jsonpath='{.spec.suspend}') and if that value is true skip
prompting and continue, otherwise prompt as before and only abort if the user
declines and .spec.suspend is false; update the logic around the REPLY check and
the kubectl patch hr cozy-proxy command to reflect this new pre-check so
answering "N" does not cause an exit when .spec.suspend is already true.
|
Successfully created backport PR for |
1 participant
What this PR does
Adds a check in the migration script to detect and suspend the
cozy-proxyHelmRelease if it has
releaseName: cozystack, which conflicts with the installerrelease and causes cozystack-operator deletion during upgrade from v0.41 to v1.0.
Release note
Summary by CodeRabbit