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

Removing a helm release incorrectly determines the namespace of resources #1558

Open
R-omk opened this issue May 16, 2022 · 8 comments
Open
Labels
bug Something isn't working question Further information is requested

Comments

@R-omk
Copy link

R-omk commented May 16, 2022




Describe the bug

If the metadata.namespace field is not explicitly specified in the manifest (this is valid when installing releases), then such a manifest will try to search in the default namespace instead of the namespace in which the helm release was installed.

To Reproduce

Install some chart in the namespace other than the default one.
It is mandatory that some (or all) manifests do not explicitly specify the namespace field during the templating process.

Expected behavior

The release must be removed as if it were removed by the original helm application. That is, all installed resources.

Versions (please complete the following information):

  • OS: ubuntu 18.04
  • K9s: v0.25.18
  • K8s: v1.22.8
@muffl0n
Copy link

muffl0n commented Oct 18, 2022

Just stumbled upon this one which caused us some major headaches. Imho this is a critical bug, cause it can lead to dataloss when deleting resources in the wrong namespace. This behavior is unnoticed to the user, cause the releases are removed but the resources are not. Or at least not in the right namespace.
When the resources k9s tries to delete are not found in the default namespace, there is no error message either. So there is no chance for the user to be aware of this behavior.

Imho, this needs a fix ASAP. Quickfix would be to prevent that helm releases can be deleted in k9s.

@derailed derailed added bug Something isn't working question Further information is requested labels Oct 18, 2022
@derailed
Copy link
Owner

@muffl0n Yikes! this is not good indeed. K9s relies on the helm api to delete the chart. So when you issue the delete (aka uninstall) we uninstall the chart using the namespace the chart was installed under. If the chart specifies no explicit namespace then guessing default would be used but that not a k9s issue as best as I can tell... That's all the api provides to us! Thus I wonder if this is a helm issue?? Need to research this a bit more. In the mean time if you could add more details here that will be useful to track this down especially the case you mention where the resources are deleted in the wrong namespace...

@muffl0n
Copy link

muffl0n commented Oct 18, 2022

It does not use the namespace default, but the namespace you set as a default before starting k9s. Which is even more dangerous.

@derailed
Copy link
Owner

That does not make any sense! The namespace the chart is installed in vs the namespace the chart template uses may not match. The chart must be uninstall in the namespace the chart is currently in, otherwise the uninstall call would fail.
Here is the repro step I've used:

  1. create a namespace fred
  2. launch k9s in the namespace k9s -n fred
  3. Install redis chart in namespace fred.
  4. K9s shows the redis chart in namespace 'fred' as expected
  5. K9s shows the redis pod in the default namespace as expected since no custom values are set
  6. Uninstall fred/redis chart
  7. The redis pods in the default namespace are deleted as expected

What am I missing?

@muffl0n
Copy link

muffl0n commented Oct 18, 2022

I know it sounds completely off. But this behavior matches with the one described in #1033.
I'm gonna have a look if I can reproduce this with a small testcase.

@muffl0n
Copy link

muffl0n commented Oct 19, 2022

I succeeded to reproduce this behavior: https://github.com/muffl0n/helm-k9s-testcase

Let me know if I can be of any further help!

@derailed
Copy link
Owner

derailed commented Feb 6, 2024

@muffl0n Sorry Sven I've missed your excellent repro scenario which helped dial this issue in.
Thank you so much for this!!

@muffl0n
Copy link

muffl0n commented Feb 6, 2024

No worries! I'm happy that I could do my part in tackling this behavior!
Thanks again for your marvellous work! ❤️‍🔥

derailed added a commit that referenced this issue Feb 7, 2024
derailed added a commit that referenced this issue Feb 7, 2024
derailed added a commit that referenced this issue Feb 7, 2024
* [Maint] Fix race condition issue

* [Bug] Fix #2501

* [Maint] Allow reference to resource aliases for plugins

* [Feat] Intro cp namespace command + misc cleanup

* [Maint] Rev k8s v0.29.1

* [Bug] Fix #1033, #1558

* [Bug] Fix #2527

* [Bug] Fix #2520

* rel v0.31.8
placintaalexandru pushed a commit to placintaalexandru/k9s that referenced this issue Apr 3, 2024
* [Maint] Fix race condition issue

* [Bug] Fix derailed#2501

* [Maint] Allow reference to resource aliases for plugins

* [Feat] Intro cp namespace command + misc cleanup

* [Maint] Rev k8s v0.29.1

* [Bug] Fix derailed#1033, derailed#1558

* [Bug] Fix derailed#2527

* [Bug] Fix derailed#2520

* rel v0.31.8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants