aws-nuke-v0.0.1 #4

tonerdo · 2018-01-22T19:36:03Z

This PR contains a reimplementation of the original aws-nuke bash shell script in golang.

Changes Include:

Complete rewrite of cli utility in golang
Support for nuking only EC2 instances
Unit tests

brikis98

Woohoo! First PR :)

Overall: the code looks great. The structure is perfect and I'm very happy to see solid docs & tests.

I went through and left lots of detailed comments :)

brikis98 · 2018-01-22T20:08:31Z

README.md

+# aws-nuke
+
+This repo contains a CLI tool to delete all AWS resources in an account. The currently supported functionality includes:


It's probably a good idea to add two more things to this intro section:

Motivation: explain why aws-nuke exists and you might want to use it. E.g., you have an account you use for testing and need to clean up left over resources so AWS doesn't charge you for them.

Warning: big, bold text explaining that this tool is destructive, irreversibly deletes things in your account, and that you must be very careful with its use.

brikis98 · 2018-01-22T20:08:48Z

README.md

+
+1. Download the latest binary for your OS on the [releases page](https://github.com/gruntwork-io/aws-nuke/releases).
+2. Move the binary to a folder on your `PATH`. E.g.: `mv gruntwork_darwin_amd64 /usr/local/bin/gruntwork`.


Update name of the binary here.

brikis98 · 2018-01-22T20:10:19Z

README.md

+
+```shell
+go test -v ./...


Could you add an MIT license to this repo? See the license section in fetch for an example.

brikis98 · 2018-01-22T20:13:21Z

aws/aws.go

+		"us-east-1", "us-east-2", "us-west-1", "us-west-2", "ca-central-1",
+		"eu-west-1", "eu-central-1", "eu-west-2", "ap-southeast-1", "ap-southeast-2",
+		"ap-northeast-2", "ap-northeast-1", "ap-south-1", "sa-east-1",
+	}


I think you can get the list of regions programmatically from this API: https://docs.aws.amazon.com/sdk-for-go/api/aws/endpoints/

brikis98 · 2018-01-22T20:15:06Z

aws/aws.go

+
+	var resources []string
+	for _, region := range regions {
+		session, _ := session.NewSession(&awsgo.Config{


I'm guessing the second parameter is an error object. If so, do not ignore it. Although it's verbose, the idiomatic code style in Go is for every function (including this GetAllResources function) to return a value (e.g., []string and an error):

func GetAllResources() ([]string, error) { // ... session, err := session.NewSession(&awsgo.Config{ ... }) if err != nil { return nil, errors.WithStackTrace(err) } }

brikis98 · 2018-01-22T20:24:37Z

aws/ec2_test.go

+		assert.Fail(t, "Unable to fetch list of EC2 Instances")
+	}
+
+	assert.NotEqual(t, 0, len(instanceEntries))


We often run tests in parallel, and I'm afraid if some other tests happens to fire up an EC2 instance in the same account, this test will pass, even if the code isn't actually working. Consider tagging the instances you launch from the aws-nuke test and make sure the instance you find with getAllEc2Instances includes the tag you expect.

In fact, it's probably a good idea to tag ALL resources created by aws-nuke tests with "aws-nuke-test-<UNIQUE_ID>" so if aws-nuke's own tests leave resources around, we know what the cause is. We typically use this UniqueId function to generate unique IDs for all our tests.

This particular test line simply checks that the creation of an instance succeeded and the next line ensures that the instance created by the test is present in the returned list. If in the TestNukeInstances we ensure we only delete appropriately tagged instances then, I feel this line will be fine

Consider the following:

We have two automated tests running in the same AWS account at the same time. The first test, foo, creates an EC2 instance. The second test, this one for aws-nuke, tries to create the EC2 Instance, but due to some bug in the test, fails.

Now you do your check, which is merely seeing that there are more than 0 instances created. The check will pass, as foo created an EC2 Instance, but it should fail, as the instance created by aws-nuke didn't actually succeed.

Ah makes sense, thanks for the clarification. I've removed this line and I now specifically check for the presence of the instance that was created

brikis98 · 2018-01-22T20:24:46Z

aws/ec2_test.go

+}
+
+func TestNukeInstances(t *testing.T) {
+	session, _ := session.NewSession(&awsgo.Config{


error handling

brikis98 · 2018-01-22T20:25:12Z

aws/ec2_test.go

+		assert.Fail(t, "Unable to fetch list of EC2 Instances")
+	}
+
+	assert.Equal(t, 0, len(instanceEntries))


This won't work if other things are deploying into the same account or this test is running multiple times in parallel. See my note on tagging above.

brikis98 · 2018-01-22T20:25:33Z

commands/cli.go

+	app.HelpName = app.Name
+	app.Author = "Gruntwork <www.gruntwork.io>"
+	app.Version = version
+	app.Usage = "A CLI tool to cleanup AWS resources"


We should definitely have a big warning text in the usage description!

As well as a list of what resources are currently supported

brikis98 · 2018-01-22T20:26:41Z

commands/cli.go

+		logging.Logger.Infoln(resource)
+	}
+
+	prompt := "\nAre you sure you want to nuke all listed resources"


Add a big, huge WARNING here. Yell at the user to tell them this is irreversible! Make the warning red (example of using colors on the CLI here).

tonerdo · 2018-01-24T16:14:40Z

Add a big, huge WARNING here. Yell at the user to tell them this is irreversible! Make the warning red (example of using colors on the CLI here).

The text is printed by the PromptUserForYesNo function which defaults to BRIGHT_GREEN with no way of specifying a custom color

brikis98 · 2018-01-24T17:07:19Z

The text is printed by the PromptUserForYesNo function which defaults to BRIGHT_GREEN with no way of specifying a custom color

Sure, but you can add a separate logging statement just before that in red! This is a hugely dangerous tools, so we need to very seriously warn users. In fact, even "yes/no" might not be the right prompt here. We might want to force users to type "nuke" or something where you can't possibly do it by accident.

tonerdo · 2018-01-24T17:34:44Z

We might want to force users to type "nuke" or something where you can't possibly do it by accident.

Good idea!

brikis98

OK, almost there. Just some nitpicky code maintainability stuff to clean up 👍

brikis98 · 2018-01-24T17:37:20Z

README.md

+* Deleting all unprotected EC2 instances in an AWS account
+
+### WARNING: THIS TOOL IS HIGHLY DESTRUCTIVE, ALL SUPPORTED RESOURCES WILL BE DELETED. ITS EFFECTS ARE IRREVERSIBLE AND SHOULD NEVER BE USED IN A PRODUCTION ENVIRONMENT


brikis98 · 2018-01-24T17:37:43Z

README.md

+
+1. Download the latest binary for your OS on the [releases page](https://github.com/gruntwork-io/aws-nuke/releases).
+2. Move the binary to a folder on your `PATH`. E.g.: `mv gruntwork_darwin_amd64 /usr/local/bin/aws-nuke`.


s/gruntwork_darwin_amd64/aws-nuke_darwin_amd64/

brikis98 · 2018-01-24T17:43:38Z

aws/aws.go

+}
+
+// GetAllResources - Lists all aws resources
+func GetAllResources() (map[string]map[string][]*string, error) {


Hm, that's quite a nested return value. Very hard to know what'll be in it! I had to read through all the code to figure out that it's a map of resource type -> map of region -> resource unique ID. I think.

This is a perfect opportunity to create some custom struct types with named parameters. Example:

type AwsAccountResources { ResourcesInRegion map[string]AwsRegionResources } type AwsRegionResources struct { Instances []Ec2Instance } type Ec2Instance struct { InstanceId string } func GetAllResources() (*AwsAccountResources, error) { }

Now I can read the function signature and types and know exactly what is being returned.

brikis98 · 2018-01-24T17:45:04Z

aws/aws.go

+}
+
+// NukeAllResources - Nukes all aws resources
+func NukeAllResources(resourceMaps map[string]map[string][]*string) error {


Same thing here. To someone who isn't intimately familiar with the code, there's no way to guess what this map of maps should contain. See my comment above on using types to make this easier to understand.

brikis98 · 2018-01-24T17:45:48Z

aws/aws.go

+		}
+
+		err = nukeAllEc2Instances(session, resourceMaps["ec2"][region])
+		if err != nil {


In go, you can combine these into one statement:

if err := nukeAllEc2Instances(session, resourceMaps["ec2"][region]); err != nil { }

brikis98 · 2018-01-24T17:53:03Z

aws/ec2_test.go

+
+	// Add test tag to the created instance
+	_, err = svc.CreateTags(&ec2.CreateTagsInput{
+		Resources: []*string{runResult.Instances[0].InstanceId},


I wonder if, due to AWS eventual consistency, this API call can fail, as the information about this instance ID has not yet propagated... You may need to call WaitUntilInstanceExists.

brikis98 · 2018-01-24T17:53:46Z

aws/ec2_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+var uniqueTestID = "aws-nuke-test-" + uniqueID()


Create this variable within each test function and not out here, as multiple test functions may run in parallel, and should each have their own unique IDs.

In fact, as a general rule, avoid any sort of global state by default!

brikis98 · 2018-01-24T17:55:13Z

aws/ec2_test.go

+		assert.Fail(t, errors.WithStackTrace(err).Error())
+	}
+
+	output, err := ec2.New(session).DescribeInstances(&ec2.DescribeInstancesInput{})


This code here, plus the for-loop below, should be extracted into a method called findEC2InstancesByNameTag(name string) to make it easier to read this code.

brikis98 · 2018-01-24T17:55:29Z

aws/ec2_test.go

+		}
+	}
+
+	nukeAllEc2Instances(session, instanceIds)


Check for an error returned by this method!

brikis98 · 2018-01-24T17:56:07Z

commands/cli.go

+		}
+	}
+
+	prompt := "\nAre you sure you want to nuke all listed resources? Enter 'nuke' to confirm: "


I would still add a log line before this with a big red warning. It's very important to get the user's attention here.

brikis98

OK, one last change and we're there :)

brikis98 · 2018-01-25T14:53:40Z

aws/aws.go

+			ResourceIdentifiers: awsgo.StringValueSlice(instanceIds),
+		}
+
+		account.Resources["ec2"] = append(account.Resources["ec2"], regionResources)


I'm not sure I like having a map with arbitrary resource strings in it. It won't be obvious to a maintainer later what kind of resources may be in the map and it's prone to typos ("EC2" vs "ec2").

I'd recommend using the type system here and having lists of specific resources types, such as EC2Instance and (in the future), ASG, ELB, etc.

Ah, in fact, given your need to loop over all the resources and nuke them, it probably makes sense to do something like this:

interface AwsResources { Nuke(session *aws.Session) error } type AwsRegionResources struct { RegionName string Resources []AwsResources } type EC2Instances struct { InstanceIds []string } // Implement the AwsResources interface func (instance EC2Instance) Nuke(session *aws.Session) error { // This is what's currently in the nukeAllEc2Instances method }

Now NukeAllResources can just loop over all the resources:

for _, region := range getAllRegions() { resourcesInRegion := account.Resources[region] for _, resources := resourcesInRegion.Resources { if err := resources.Nuke(session); err != nil { // ... } } }

brikis98

LGTM. Merging! 👍

tonerdo added 13 commits January 18, 2018 16:07

Purge bash script

2d16e14

Add simple golang CLI app

622268b

Add AWS sdk dependency

d0c22db

Add functionality to list all resources to be nuked

3e0fe02

Add function to nuke all active AWS resources

659fe76

Use inbuilt gruntwork-cli prompt

89a535f

Only attempt to delete ec2 instances in regions that have

45b1cdb

Use plugable logger instead of printing directly to stdout/stderr

a9a13c4

Add test for listing and nuking ec2 instances

7556b2a

Add function to build resource array entry name for ec2 instance

1914c14

Exclude protected EC2 instances from being nuked

28bc174

update README.md

cfbd9a2

Make some code enhancements to better handle errors

cddcd44

brikis98 requested changes Jan 22, 2018

View reviewed changes

tonerdo added 5 commits January 23, 2018 14:18

Dynamically get all aws regions

1157fd1

Add better error handling

4721280

Add license file

d575a81

Remove use of stateful variables and update data structures

eec8e7b

update README.md

7345bad

Update tests to ensure they work well when run in parallel

2840e70

tonerdo added 4 commits January 24, 2018 18:10

Expand scope of ec2 instance filters

8f38fd4

Fix variable name typo

bbb374c

Update CLI description

da8c523

Update CLI confirmation prompt

b4341c7

brikis98 requested changes Jan 24, 2018

View reviewed changes

tonerdo added 2 commits January 25, 2018 11:45

Move code to filter unprotected ec2 instances to separate function

1aa2611

Add custom types to hold aws resources info

d392c89

tonerdo added 5 commits January 25, 2018 15:18

Move code to retrieve ec2 instances by name tag to func

2c9c868

Create a unique test id within each test function

762fac8

Add bold red warning before nuke prompt

8873d10

Ensure ec2 instance exists before tagging it

1276771

update README.md

51a1c41

brikis98 requested changes Jan 25, 2018

View reviewed changes

Update types for storing AWS resource info

5c6b193

brikis98 approved these changes Jan 25, 2018

View reviewed changes

brikis98 merged commit 698207e into master Jan 25, 2018

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

ghost referenced this pull request Jul 13, 2020

Initial commit

d35d928

		# aws-nuke

		This repo contains a CLI tool to delete all AWS resources in an account. The currently supported functionality includes:


		1. Download the latest binary for your OS on the [releases page](https://github.com/gruntwork-io/aws-nuke/releases).
		2. Move the binary to a folder on your `PATH`. E.g.: `mv gruntwork_darwin_amd64 /usr/local/bin/gruntwork`.

		* Deleting all unprotected EC2 instances in an AWS account

		### WARNING: THIS TOOL IS HIGHLY DESTRUCTIVE, ALL SUPPORTED RESOURCES WILL BE DELETED. ITS EFFECTS ARE IRREVERSIBLE AND SHOULD NEVER BE USED IN A PRODUCTION ENVIRONMENT

aws-nuke-v0.0.1 #4

aws-nuke-v0.0.1 #4

Conversation

tonerdo commented Jan 22, 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

tonerdo Jan 24, 2018 • edited Loading

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

tonerdo commented Jan 24, 2018

brikis98 commented Jan 24, 2018

tonerdo commented Jan 24, 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

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

brikis98 left a comment

Choose a reason for hiding this comment

tonerdo Jan 24, 2018 •

edited

Loading