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

Need confirmation for eksctl cli that modify the cluster #601

Closed
arunmk opened this issue Mar 2, 2019 · 7 comments
Closed

Need confirmation for eksctl cli that modify the cluster #601

arunmk opened this issue Mar 2, 2019 · 7 comments

Comments

@arunmk
Copy link

arunmk commented Mar 2, 2019

Before creating a feature request, please search existing feature requests to see if you find a similar one. If there is a similar feature request please up-vote it and/or add your comments to it instead

Why do you want this feature?
The eksctl delete, eksctl create and eksctl create operations take effect immediately without any sort of confirmation. It would be useful to have a basic confirmation prompt and a --force flag to override the option so that end-users don't delete the cluster in error.

What feature/behavior/change do you want?
I would like to see behavior similar to the following for the commands listed above:

eksctl delete cluster
This will delete the cluster. Are you sure (yes/no)[no]? ^C

eksctl delete cluster --force
<cluster is deleted>

y | eksctl delete cluster
<cluster is deleted>

Note: This will break existing scripts and automation.

@arunmk
Copy link
Author

arunmk commented Mar 2, 2019

If this behavior is acceptable, I have a PR in progress.

@errordeveloper
Copy link
Contributor

errordeveloper commented Mar 2, 2019

Thanks for your suggestion! I would rather consider adding --dry-run and enabling it by default (which would be inline with how upgrades are going to be handled), but we have a number of things that need to be reworked around deletions, namely deletion of any resources that were crested by the cluster (see #103, #274, #536, #602).
I would entertain a PR, but considering all of those issues, I'd rather avoid adding new functionality in cluster deletion code path - a lot of that needs reworking to begin with.

On the other hand, I also rather wouldn't introduce a change that breaks automation (which would be still the case with --dry-run enabled by default). Perhaps we can provide an environment variable that lets you protect yourself from accidental deletion. We can discuss that, but again I wouldn't like to change the code before we addressed other issue and improved how deletions are handled.
Perhaps we'd be looking for --dry-run settable via an environment variable basically. Anyhow, must work needs to be done before we can introduce this. I assume you already read the code as you have attempted making a change - it's in a need of some major improvements.

@errordeveloper
Copy link
Contributor

FYI: if you interrupt deletion process early only your nodegroups will get deleted and can recover the cluster by re-adding some nodegroups.

@arunmk
Copy link
Author

arunmk commented Mar 4, 2019

@errordeveloper thanks for the explanation and historic details. Based on the rework needed, it does seem like a change that breaks existing automation is likely to occur at some point due to the fixes. Do you think that would be the case?

In any case, this should affect only the CLI parsing path. My thought was to have this as a PreRunE option (for example https://github.com/weaveworks/eksctl/blob/master/pkg/ctl/delete/cluster.go#L32). That will not affect any of the functionality and can remain unchanged even when the other issues are fixed.

@errordeveloper
Copy link
Contributor

xref #682

@TBeijen
Copy link

TBeijen commented Oct 18, 2019

Deletion protection could be opt in (cli arg or entry in yaml config file). If present it would need to be removed first (similar to some other AWS resources, e.g. RDS).

It could be stored in a tag on the EKS cluster, for example alpha.eksctl.io/delete-protection-enabled: true

@michaelbeaumont
Copy link
Contributor

Duplicate of #476

@michaelbeaumont michaelbeaumont marked this as a duplicate of #476 Oct 27, 2020
torredil pushed a commit to torredil/eksctl that referenced this issue May 20, 2022
Fix the name of the snapshot controller leader election RoleBinding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants