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

High risk: uninstall velero will delete the namespace #5433

Open
OPSTime opened this issue Oct 11, 2022 · 12 comments · May be fixed by #6155
Open

High risk: uninstall velero will delete the namespace #5433

OPSTime opened this issue Oct 11, 2022 · 12 comments · May be fixed by #6155
Labels
Icebox We see the value, but it is not slated for the next couple releases. kind/requirement

Comments

@OPSTime
Copy link

OPSTime commented Oct 11, 2022

$ velero -n devops uninstall velero

You are about to uninstall Velero.
Are you sure you want to continue (Y/N)? y
Waiting for velero namespace "devops" to be deleted
...........................................................................................................................................................................
@sseago
Copy link
Collaborator

sseago commented Oct 11, 2022

By default, velero is installed in the "velero" namespace which is created as part of the velero install. It sounds like for your particular use case, you've installed velero into an existing namespace that also contains non-velero content, and you'd rather not remove the namespace, just all the velero content.

Currently the uninstall command does the following:

  1. Deletes the velero namespace (including velero deployment, restic daemonset, secrets, pods, all velero CRs, etc.
  2. Deletes velero clusterrolebinding
  3. Deletes velero CRDs

If you want to uninstall velero without removing unrelated things in the velero namespace then we'd need to implement something like a --preserve-velero-namespace flag which, when set to true causes velero to substitute the delete namespace call with a more complicated function which would need to explicitly delete velero deployment, daemonset, each type of velero CR, secrets, etc.

@OPSTime
Copy link
Author

OPSTime commented Oct 12, 2022

I think we should add a check whether the namespace is velero, or add a prompt for the user to confirm, or do not delete the namespace by default.

@reasonerjt
Copy link
Contributor

There is already a confirmation message, IMO we may just update the message to clarify that the namespace will be removed if the user chooses to proceed.
I think we can make this change if there is a good number of thumb-ups

@sseago
Copy link
Collaborator

sseago commented Oct 12, 2022

We can't "not delete the namespace by default" with the current code, since namespace deletion is the way velero cleans up all of its resources. If there's a need for "leave the namespace" functionality, that will require an alternate (non-default) implementation that has to explicitly delete all of the things that Velero install (and operation) might create -- deployment, daemonste, secrets, backups, restores, bsls, vsls, etc.

@reasonerjt
Copy link
Contributor

I also think it's the convention that velero has a dedicated namespace.
Therefore, I'm putting this issue in icebox.

@reasonerjt reasonerjt added the Icebox We see the value, but it is not slated for the next couple releases. label Oct 17, 2022
@sseago
Copy link
Collaborator

sseago commented Oct 17, 2022

@reasonerjt Yes, default installation gives velero a dedicated namespace with the assumption that everything there is velero-related. So if we did decide to implement an option to not delete the namespace on uninstall, it shouldn't be the default. This feels like an edge case, although I'm not sure how important the use case is for having velero in a shared namespace.

@VickyWinner
Copy link

I am having the same issue. Namespace was created separately to maintain infra as code. Now velero deleted the namespace too. I would say not to touch any k8s resource which was not created by velero. does it makes sense?

@sseago
Copy link
Collaborator

sseago commented Nov 30, 2022

The issue is that Velero doesn't know it didn't create it, since the normal install procedure is to create the namespace and then create the velero resources. As mentioned above, this is a possible future enhancement, but it's not a trivial change, since it significantly complicates the uninstall procedure. Instead of just deleting the namespace, the command would have to individually uninstall each specific resource that velero created on install and post-install -- Deployment, DaemonSet, Secrets, BackupStorageLocations, VolumeSnapshotLocations, BackupRespositories, Backups, Restores, PodVolumeBackups, PodVolumeRestores, etc.

@kaovilai
Copy link
Contributor

#6152 should make it easier on the user to script uninstall velero created resources except namespace.

@subudhiroshan
Copy link

This is indeed high risk, and I would like to upvote this issue.
We tried to circumvent this by recommending the usage of helm to install/uninstall Velero, but unfortunately, the CLI is critical to view logs and describe backups/restores etc. And we dont want to maintain a forked version of the CLI since that can be a maintenance overhead.

@GennadiyD
Copy link

GennadiyD commented Oct 25, 2023

Namespace does not belong to Velero, even if we created it specifically for this installation. Namespace is a wider and more fundamental thing than everything that Velero can install in it.

I would suggest, to change the message. Please use something like "WARNING: During uninstall, the namespace <The_Name_of_Namespace> will be removed with all resources in it.". And don't do anything without additional key. The key should be something like "--I_UNDERSTAND_WHAT_I_AM_DOING".

We don't use "uninstall" every day. I believe that is not so difficult to provide that complicated key once in a while. Many people will appreciate you when you care about them.

@kaovilai
Copy link
Contributor

@subudhiroshan don't maintain a fork, make a pull request with your intended change/design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Icebox We see the value, but it is not slated for the next couple releases. kind/requirement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants