Add support for more resources #5

tonerdo · 2018-02-05T11:23:57Z

Adds support for nuking:

Auto Scale Groups
Elastic Load Balancers (Classic and v2)
EBS Volumes

brikis98

This looks great. Clear structure, obvious how to add more features, and nice tests 👍

brikis98 · 2018-02-05T11:33:23Z

aws/asg_test.go

+	}
+}
+
+func TestListAutoScalingGroups(t *testing.T) {


Love the tests!

One issue: do the tests clean up after themselves? Or do they leave the instances / ASGs running? You should probably add a defer to each test that runs the appropriate clean up functions you already have in this codebase.

Typically I run aws-nuke to clean up the resources the tests leave around. I feel it's a good exercise of the functionality of the tool

Well, we'll be hooking up the tests to run automatically after every commit to the aws-nuke repo, so we can't rely on manual cleanup. I'd rather have the tests make an effort to clean up after themselves and we'll have a scheduled run of aws-nuke in our AWS accounts.

List tests now cleanup after themselves

brikis98 · 2018-02-05T11:37:14Z

aws/asg_test.go

+		assert.Fail(t, errors.WithStackTrace(err).Error())
+	}
+
+	err = svc.WaitUntilGroupNotExists(&autoscaling.DescribeAutoScalingGroupsInput{


I wonder if the nukeAllAutoScalingGroups should do this wait call itself? Otherwise, steps that run after it may not realize the ASGs are still in progress of being deleted, and therefore not work correctly. E.g., If we try to delete EBS volumes immediately after, the ASGs still using those volumes may not yet have terminated.

brikis98 · 2018-02-05T11:38:21Z

aws/ebs.go

+
+		_, err := svc.DeleteVolume(params)
+		if err != nil {
+			// Ignore not found errors, some volumes are deleted along with EC2 Instances


Probably still worth logging that "Volume XXX has already been deleted."

This is done as well

It's probably also a good idea to add a log statement with the ID/name of each EBS volume, instance, ASG, etc. that gets nuked. That'll be very useful for debugging and seeing the progress of the code.

Logging has been updated to reflect this

brikis98 · 2018-02-05T12:05:02Z

aws/ebs_test.go

+func createTestEBSVolume(t *testing.T, session *session.Session, name string) ec2.Volume {
+	svc := ec2.New(session)
+	volume, err := svc.CreateVolume(&ec2.CreateVolumeInput{
+		AvailabilityZone: awsgo.String("us-west-2a"),


I just noticed that these tests all hard-code the region. The downside to this is if we have some region-specific bug, we won't catch it.

You can use this method to pick a random one.

brikis98 · 2018-02-05T12:08:29Z

aws/ebs_test.go

+		// Retrive only IDs of instances with the unique test tag
+		for _, tag := range volume.Tags {
+			if *tag.Key == "Name" {
+				if *tag.Value == name {


Instead of a second level of nesting, you could combine the two if-statements: if *tag.Key == "Name" && *tag.Value == name.

Also, I wonder if there is a chance tag.Key or tag.Value could be a nil pointer? Might be safer to use aws.StringValue.

brikis98 · 2018-02-05T12:09:43Z

aws/ebs_test.go

+
+	volumeIds := findEBSVolumesByNameTag(output, uniqueTestID)
+
+	if err := nukeAllEbsVolumes(session, volumeIds); err != nil {


Does this method wait for all volumes to be deleted? Or is it possible for it to return while deletion is still in progress, causing the next check to fail?

It does now

brikis98 · 2018-02-05T12:10:23Z

aws/ec2.go

@@ -85,6 +85,20 @@ func nukeAllEc2Instances(session *session.Session, instanceIds []*string) error
 		return errors.WithStackTrace(err)
 	}

+	err = svc.WaitUntilInstanceTerminated(&ec2.DescribeInstancesInput{


Ah, yea, this seems like a good addition. I'd recommend adding equivalent "wait until XXX deleted" to the EBS, ASG, and ELB code.

This has been done for everything except classic ELB which doesn't have this functionality

If there's no built-in WaitUntilXXX method, you can create your own! You just create a retry loop (see DoWithRetry), looking up the ELB by name until it no longer exists or max retries is exceeded.

brikis98 · 2018-02-05T12:12:05Z

aws/elb_test.go

+	assert.Contains(t, awsgo.StringValueSlice(elbNames), elbName)
+}
+
+func TestNukeELBs(t *testing.T) {


How long do the tests take to run? It seems like it would take a while.

You should probably add:

t.Parallel()

As the first line of each test. This will get them all to run in parallel. It will also flush out any concurrency bugs.

Tests now run and pass concurrently

brikis98 · 2018-02-05T12:12:54Z

aws/elbv2_test.go

+		assert.Failf(t, "Could not create test ELBv2: %s", errors.WithStackTrace(err).Error())
+	}
+
+	balancer := *result.LoadBalancers[0]


Could this list be empty? Prob worth checking and returning an error if it is.

brikis98 · 2018-02-06T13:57:52Z

README.md

@@ -19,6 +22,14 @@ The currently supported functionality includes:

 Simply running `aws-nuke` will start the process of cleaning up your AWS account. You'll be shown a list of resources that'll be deleted as well as a prompt to confirm before any deletion actually takes place.

+### Excluding Regions
+
+You can use the `--exclude` or `-e` flag to exclude resources in certain regions from being deleted. For example the following command does not nuke resources in `ap-south-1` and `ap-south-2` regions:


We might want to call it --exclude-region, as I suspect we'll have other types of excludes in the future (e.g., --exclude-ec2-instances).

brikis98 · 2018-02-06T13:58:49Z

aws/asg_test.go

+	instance := createTestEC2Instance(t, session, name)
+
+	// EC2 Instance must be in a running state before ASG can be created
+	err := ec2.New(session).WaitUntilInstanceRunning(&ec2.DescribeInstancesInput{


Should the createTestEC2Instance method do the waiting?

Ah missed this. Thanks

brikis98 · 2018-02-06T13:59:55Z

aws/aws.go

 	"github.com/gruntwork-io/gruntwork-cli/errors"
 )

 // Returns a list of all AWS regions
 func getAllRegions() []string {
+	// chinese and government regions are not accessible with regular accounts
+	reservedRegions := []string{
+		"cn-north-1", "cn-northwest-1", "us-gov-west-1",


Ah, good catch

brikis98 · 2018-02-06T14:00:50Z

aws/aws.go

 	account := AwsAccountResources{
 		Resources: make(map[string]AwsRegionResource),
 	}

 	for _, region := range getAllRegions() {
+		// Ignore all cli excluded regions
+		if collections.ListContainsElement(excludedRegions, region) {
+			continue


Might be a good idea to log either "Processing region XXX" or "Skipping region XXX".

brikis98 · 2018-02-06T14:01:32Z

aws/aws.go

@@ -39,6 +62,46 @@ func GetAllResources() (*AwsAccountResources, error) {

 		resourcesInRegion := AwsRegionResource{}

+		// ASG Names
+		groupNames, err := getAllAutoScalingGroups(session, region)


You might want to add a comment near the top of this method that the order of these resources matters due to dependencies between them: e.g., ASGs first, then EC2 instances, then EBS volumes.

brikis98 · 2018-02-06T14:03:00Z

aws/ebs.go

+
+		_, err := svc.DeleteVolume(params)
+		if err != nil {
+			// Ignore not found errors, some volumes are deleted along with EC2 Instances


It's probably also a good idea to add a log statement with the ID/name of each EBS volume, instance, ASG, etc. that gets nuked. That'll be very useful for debugging and seeing the progress of the code.

brikis98 · 2018-02-06T14:03:33Z

aws/ebs_test.go

+func findEBSVolumesByNameTag(output *ec2.DescribeVolumesOutput, name string) []*string {
+	var volumeIds []*string
+	for _, volume := range output.Volumes {
+		// Retrive only IDs of instances with the unique test tag


s/Retrive/Retrieve/

brikis98 · 2018-02-06T14:05:02Z

aws/ec2_test.go

-	rand := rand.New(rand.NewSource(time.Now().UnixNano()))
-	for i := 0; i < UNIQUE_ID_LENGTH; i++ {
-		out.WriteByte(BASE_62_CHARS[rand.Intn(len(BASE_62_CHARS))])
+func getLinuxAmiIDForRegion(region string) string {


This will be a pain to maintain as regions are added or AMIs removed. You should be able to find the AMI automatically using Amazon's APIs, similar to Packer's source_ami_filter.

brikis98 · 2018-02-06T14:06:59Z

commands/cli.go

@@ -29,7 +35,7 @@ func CreateCli(version string) *cli.App {
 func awsNuke(c *cli.Context) error {
 	logging.Logger.Infoln("Retrieving all active AWS resources")

-	account, err := aws.GetAllResources()
+	account, err := aws.GetAllResources(c.StringSlice("exclude"))


You should validate the list of regions passed in to ensure each one corresponds to a valid AWS region. We don't want to blow away a region because someone made a typo (e.g., us-wes-1).

…unning state

brikis98 · 2018-02-07T13:53:39Z

aws/asg_test.go

+	assert.Contains(t, awsgo.StringValueSlice(groupNames), groupName)
+
+	// clean up after this test
+	defer nukeAllAutoScalingGroups(session, []*string{&groupName})


This should go immediately after the createTestAutoScalingGroup call. Otherwise, one of the assert calls may fail and prevent the code from ever executing this cleanup step.

brikis98 · 2018-02-07T13:56:00Z

aws/ebs_test.go

+	assert.Contains(t, awsgo.StringValueSlice(volumeIds), awsgo.StringValue(volume.VolumeId))
+
+	// clean up after this test
+	defer nukeAllEbsVolumes(session, []*string{volume.VolumeId})


Same comment about defer here and any other test cases.

brikis98 · 2018-02-07T13:57:40Z

aws/ec2_test.go

-	if err != nil {
-		assert.Failf(t, "Could not create test EC2 instance: %s", errors.WithStackTrace(err).Error())
+	if err != nil || len(runResult.Instances) == 0 {
+		assert.Fail(t, "Could not create test EC2 instance")


You may want to include err and runResult.Instances in the log statement so we know what the issue was.

brikis98 · 2018-02-07T13:59:41Z

aws/elb.go

+	for i := 0; i < 30; i++ {
+		_, err := svc.DescribeLoadBalancers(input)
+		if err != nil {
+			// an error is returned when ELB no longer exists


You need to check which error! This could be a 500 error or 404 or 403 or a dozen other things, most of which don't mean the ELB is gone. See here for how to check AWS errors.

brikis98 · 2018-02-07T14:00:50Z

aws/elb.go

+			return nil
+		}
+
+		time.Sleep(1 * time.Second)


Log here (debug) that you're waiting for the EBS volume to be deleted so it's clear what's going on.

brikis98 · 2018-02-07T14:00:59Z

aws/elb.go

+		time.Sleep(1 * time.Second)
+	}
+
+	panic("ELBs failed to delete")


Return an error. Don't panic.

brikis98 · 2018-02-07T14:05:15Z

commands/cli.go

+	for _, excludedRegion := range excludedRegions {
+		if !collections.ListContainsElement(regions, excludedRegion) {
+			fmt.Println(excludedRegion + "is not a valid AWS Region")
+			return InvalidFlagError{}


Instead of the error message in a println and a separate error with no info, update InvalidFlagError to have the flag name and value as fields and to have an Error() method that clearly shows them to the user.

brikis98 · 2018-02-07T14:05:32Z

commands/cli.go


-	account, err := aws.GetAllResources()
+	fmt.Println("Retrieving all active AWS resources")


Use the logger rather than fmt.Println. Applies everywhere in this code.

…ancerNotFound error

brikis98

Fantastic :-D

tonerdo added 14 commits January 29, 2018 11:25

Move EC2Instances type to ec2 specific types file

e84d0e4

Add functionality to nuke Auto scaling groups

9649bae

Include ASGs in list of supported AWS resources

feebc33

Add tests for Auto Scaling Group nuking functionality

56101fa

add functionality to nuke elastic load balancers

4d683b1

add functionality to nuke V2 elastic load balancers

307d97b

add information about deleting load balancers

8bf39d3

add functionality to nuke elastic block store volumes

163170d

add information about deleting EBS volumes

7fb51a3

add tests for nuking elb

a29434e

ensure ec2 instance is terminated before proceeding

dcdc88d

add tests for nuking of EBS volumes

eb82cf8

ignore volume not found errors during nuking of ebs volumes

05f79e4

add test for nuking elbv2

99c91a5

brikis98 requested changes Feb 5, 2018

View reviewed changes

tonerdo added 12 commits February 5, 2018 15:51

ensure all ASGs have been nuked before nuke function returns

1d5c534

update ebs test to guard against nil pointer

b51e604

ensure elbv2 load balancer is successfully created

c95bad0

ensure ebs volume is deleted before returning from nuke method

fa9ad56

ensure elbv2 is deleted before returning from nuke function

d7cd7ec

Run all tests in parallel

e8d7c5a

add funtion to return a random region and ignore reserved regions

69b32f3

use random regions for tests

ab693b0

add function to return a linux ami for a specific region

a4048ef

Move unique id function to separate go package file

e913e17

clean up resources created in list tests

90dd2c7

add ability to exclude resources in certain regions from being deleted

2ceecca

brikis98 mentioned this pull request Feb 6, 2018

Add support for deleting Auto Scaling Groups. #3

Closed

brikis98 requested changes Feb 6, 2018

View reviewed changes

change name of exclude region flag

5d1af22

tonerdo added 6 commits February 6, 2018 15:45

fix typo

a3c38be

validate excluded regions

88b5c0f

createTestEC2Instance should wait for created instance to be in the r…

6187bab

…unning state

improve logging and overall cli UX

8bf36f9

wait until elb is deleted before returning from nuke function

f18e711

use dynamically selected images to create ec2 instance

d6ce82c

brikis98 requested changes Feb 7, 2018

View reviewed changes

tonerdo added 5 commits February 7, 2018 15:29

re-order cleanup defer calls

146c760

use the logger package to print log all info

e368ad4

update InvalidFlagError to contain more info

b010c07

distinguish between ec2 creation errors

9169791

update waitUntilElbDeleted function to only return success on LoadBal…

fa51aa5

…ancerNotFound error

brikis98 approved these changes Feb 7, 2018

View reviewed changes

brikis98 merged commit 069d76f into master Feb 7, 2018

tonerdo changed the title ~~aws-nuke v0.0.2~~ Add support for more resources Feb 11, 2018

tonerdo deleted the v0.0.2 branch February 12, 2018 12:17


		volumeIds := findEBSVolumesByNameTag(output, uniqueTestID)

		if err := nukeAllEbsVolumes(session, volumeIds); err != nil {


		account, err := aws.GetAllResources()
		fmt.Println("Retrieving all active AWS resources")

Add support for more resources #5

Add support for more resources #5

Conversation

tonerdo commented Feb 5, 2018

brikis98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brikis98 left a comment

Choose a reason for hiding this comment