Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make flux uninstall warning verbiage much stronger #884

Closed
wants to merge 1 commit into from

Conversation

kingdonb
Copy link
Member

@kingdonb kingdonb commented Feb 8, 2021

I am nearly sure it is not intended to delete helmreleases.helm.fluxcd.io HelmReleases when users run flux uninstall, but the effect is there currently, it does delete helm releases from the old API version. So until a behavior change can be implemented (if one makes sense), a more strongly worded warning might be appropriate such as this. (#811 first reported this.)

I would expect flux uninstall to only remove resources that come from Flux v2 CRDs, not Helm Operator HRs, since a person may try to upgrade their cluster to Flux v2, run into some issue, and then abort, thinking it's safe to just run flux uninstall.

I think the risk may be slightly overblown, so far only developer clusters have been impacted by this that we know of, at least. But the worst case scenario modes of failure are pretty bleak. Now that we both know it does this, we won't make this mistake again! It was discussed at another Flux team meeting that some migrations might not take place at once, so we should take some extra care around scenarios that may impact users who have only partially migrated their cluster from Flux v1 to Flux v2.

Does it make sense to merge in a change to the warning message such as this?

Is it possible to change the uninstall routine to refer to a blessed list of CRDs (maybe one exists) or at least regex filter the fully qualified names so that helm.fluxcd.io HelmReleases are not uninstalled too when this code runs? I think we have other CRDs with names that overlap other well-known types that could be affected by this, like Kustomization.

Nearly sure it is not intended to delete helmreleases.helm.fluxcd.io
HelmReleases when users run `helm uninstall`, but the effect is there
currently, so until the behavior change needed is made clear, a more
strongly worded warning might be appropriate such as this. (See fluxcd#811)

Is there any way this could be fixed? I would expect `flux uninstall` to
only remove resources that come from Flux v2 CRDs, not Helm Operator HRs
@hiddeco
Copy link
Member

hiddeco commented Feb 8, 2021

Wouldn't it be a better solution to use the FQDN of the Custom Resources here:

namespacedKinds := []string{
sourcev1.GitRepositoryKind,
sourcev1.HelmRepositoryKind,
sourcev1.BucketKind,
}
namespacedKinds = append(namespacedKinds, helmv2.HelmReleaseKind)

to simply solve the whole issue of removing the wrong resources?

@yebyen
Copy link
Contributor

yebyen commented Feb 8, 2021

Yes, absolutely. I wasn't sure how to suggest that myself, but I agree, if this is not an intentional thing then it should be fixed.

I still think a text change is warranted because the default value --resources true will remove resources that are not mentioned in the warning (all the custom resources, which could be in any namespace) but the third clause in my suggested text can go as long as this is an error and can be fixed.

@stefanprodan
Copy link
Member

Closing in favour of #891

@kingdonb kingdonb deleted the warn-strongly branch April 13, 2021 15:43
ybelleguic pushed a commit to ybelleguic/flux2 that referenced this pull request Jan 9, 2023
[OCI] Static credentials should take precedence over the OIDC provider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants