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 GuardDuty Detectors #320

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Conversation

zackproser
Copy link
Contributor

@zackproser zackproser commented Jul 1, 2022

Description

Implement support for inspecting and destroying GuardDuty Detectors.

Closes #321
Closes #175

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 support for inspecting and nuking GuardDuty detectors

Migration Guide

@zackproser zackproser force-pushed the feature/guardduty branch 2 times, most recently from ec9436b to 1fcf792 Compare July 5, 2022 21:00
@zackproser zackproser changed the title [WIP] - begin implementing support for GuardDuty Detectors Implement support for GuardDuty Detectors Jul 5, 2022
@zackproser zackproser force-pushed the feature/guardduty branch 2 times, most recently from 23ae18f to 503e893 Compare July 5, 2022 23:24
@@ -217,7 +217,7 @@ func GetAllResources(targetRegions []string, excludeAfter time.Time, resourceTyp

count := 1
totalRegions := len(targetRegions)
var resourcesCache = map[string]map[string][]*string{}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to https://github.com/mvdan/gofumpt for auto-formatting, so that's why some of these vars and extra lines are getting cleaned up.

@@ -3,7 +3,7 @@ module github.com/gruntwork-io/cloud-nuke
go 1.16

require (
github.com/aws/aws-sdk-go v1.42.4
github.com/aws/aws-sdk-go v1.44.46
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this upgrade, some of the latest GuardDuty methods are not available.

@zackproser
Copy link
Contributor Author

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.

Mostly LGTM! Had one nit about performance.

Comment on lines +106 to +119
for _, detectorId := range detectorIds {
params := &guardduty.DeleteDetectorInput{
DetectorId: aws.String(detectorId),
}

_, err := svc.DeleteDetector(params)

if err != nil {
logging.Logger.Errorf("[Failed] %s: %s", detectorId, err)
} else {
deletedIds = append(deletedIds, detectorId)
logging.Logger.Infof("Deleted GuardDuty detector: %s", detectorId)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably ok since AFAIK there can only be a limited number of detectors per region, but recommend using the same pattern as NAT Gateway (https://github.com/gruntwork-io/cloud-nuke/blob/master/aws/nat_gateway.go#L63) to implement concurrent deletion of the detectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, also saw this pattern in the cloudwatch_loggroup that you added! I'll file a ticket to do this in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #324 to track.

@yorinasub17 yorinasub17 mentioned this pull request Jul 7, 2022
3 tasks
@zackproser
Copy link
Contributor Author

Thanks for reviews! Going to merge this in now.

@zackproser zackproser merged commit 42f24e3 into master Jul 7, 2022
@zackproser zackproser deleted the feature/guardduty branch July 7, 2022 14:53
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.

Feature: Add support for GuardDuty Detectors New feature: remove GuardDuty
2 participants