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

Implement support for IAM Roles #330

Merged
merged 11 commits into from
Jul 19, 2022
Merged

Implement support for IAM Roles #330

merged 11 commits into from
Jul 19, 2022

Conversation

zackproser
Copy link
Contributor

Description

These changes are based on #251, but also introduce concurrent deletion of roles for speed. They introduce support for inspecting and nuking IAM Roles.

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added / Removed / Updated [X].

Migration Guide

@@ -106,3 +106,7 @@ IAMUsers:
names_regex:
# We want to make sure the main circle-ci-test user is never deleted
- "^circle-ci-test$"
IAMRoles:
include:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yorinasub17 I looked through sandbox and phxdevops and it's diffcult to see what is actually important from a role perspective, as you know.

I know you suggested using an exclude for the critical roles, but I did notice that the vast majority of roles seemed to be created for tests - they're used for those test runs and then hang out forever, so I thought one thing that might make sense initially is to explicitly delete those roles that match the test names and are older than some number of hours.

FWIW, this cloud-nuke feature intentionally ignores all AWS-managed roles, which we also seem to have a good number of in play in both accounts.

LMK if there's a common format of names we use for the critical roles in both accounts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's start with this, and then later on we can identify which ones we want to keep during a ticket jam. FWIW, I believe the critical IAM roles are actually the cross account ones referenced in dogfood-infrastructure-live. We can probably recover with relative ease if any of the others are deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger - sounds good!

aws/iam_role.go Outdated Show resolved Hide resolved
@@ -106,3 +106,7 @@ IAMUsers:
names_regex:
# We want to make sure the main circle-ci-test user is never deleted
- "^circle-ci-test$"
IAMRoles:
include:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's start with this, and then later on we can identify which ones we want to keep during a ticket jam. FWIW, I believe the critical IAM roles are actually the cross account ones referenced in dogfood-infrastructure-live. We can probably recover with relative ease if any of the others are deleted.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code mostly LGTM! Given the criticality of IAM Roles, I'd like to do a quick sanity check run with go run before approving, but I ran out of time for today. Will do that check tomorrow.

Also, looks like there are build failures for unit tests in CircleCI that should be fixed.

@zackproser
Copy link
Contributor Author

Code mostly LGTM! Given the criticality of IAM Roles, I'd like to do a quick sanity check run with go run before approving, but I ran out of time for today. Will do that check tomorrow.

Also, looks like there are build failures for unit tests in CircleCI that should be fixed.

Tests are fixed. Looks like the two failing now are:

  • TestNukeTransitGateway
  • TestGetAllTransitGatewayVpcAttachment

which are both marked as Flaky

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I also ran it locally and it worked as expected.

@zackproser
Copy link
Contributor Author

LGTM! I also ran it locally and it worked as expected.

Thanks for reviews! It also behaved as expected for me locally, so I'm going to merge this in now.

@zackproser zackproser merged commit 044f1f8 into master Jul 19, 2022
@zackproser zackproser deleted the iam-roles branch July 19, 2022 15:05
@zackproser zackproser mentioned this pull request Jul 19, 2022
3 tasks
@arsci arsci mentioned this pull request Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants