-
Notifications
You must be signed in to change notification settings - Fork 722
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
Disable EC2 Instance Stop Protection #986
Disable EC2 Instance Stop Protection #986
Conversation
…tection code. Also fixed the feature flag name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware that you just amended the existing logic to also cater for stop protection, which is totally fine me. Though I was thinking if it would be better to add the two different attributes to the EC2Instance
struct and act accordingly.
With this I would see upsides:
1.) We do not need to check error messages
2.) If both things are enabled (is this even possible), both things can be deactivated and then the instance terminated. Maybe even before trying to delete them.
@bjoernhaeuser Thank you for your review! I'd considered rewriting this logic in a manner you described but similar to the original developer who implemented the termination protection logic, the problem I faced is both the termination and stop protection attributes are not returned by the regular Since I suspect these two attributes will not be a common issue for most users of AWS Nuke, I felt it was an unnecessary addition of API calls to every EC2 Instance. In an effort to keep AWS Nuke as light as possible for most users, my personal preference is to maintain the approach of calling these API endpoints only in the event of a termination error. I am, however, open to continued discussion and brainstorming! |
Thanks for the reminder, now I remember why it was implemented like this. |
Greetings @svenwltr @der-eismann , would you be okay getting this PR merged? Thank you! |
AWS recently launched the ability to enable "stop protection" on EC2 instance, similar to the existing "termination protection"
This PR implemented an error-state routine to disable stop protection and retry termination.
Open Question: A new feature flag was added called
DisableEC2InstanceStopProtection
at the top level since this doesn't exactly fit under theDisableDeletionProtection
hierarchy. I'd considered adding this flag under that group and just calling it something likeEC2InstanceStop
to differentiate it, but I felt that might be confusing. Thoughts?I went back and forth on how to handle the retry-routines from two possible error states, but what I came up with works and I tried to document it inline with some comments.
Testing
Create a set of EC2 instances with the following script:
Running AWS Nuke with
EC2Instance
enabled successfully terminates all instances.