New Resource: aws_guardduty_member #2911

bflad · 2018-01-09T17:27:29Z

References:

Reference specification: Add GuardDuty Support #2489
Go docs: https://docs.aws.amazon.com/sdk-for-go/api/service/guardduty/
API docs: https://docs.aws.amazon.com/guardduty/latest/ug/guardduty_api_ref.html

make testacc TEST=./aws TESTARGS='-run=TestAccAWSGuardDuty'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSGuardDuty -timeout 120m
=== RUN   TestAccAWSGuardDuty
=== RUN   TestAccAWSGuardDuty/Detector
=== RUN   TestAccAWSGuardDuty/Detector/basic
=== RUN   TestAccAWSGuardDuty/Detector/import
=== RUN   TestAccAWSGuardDuty/Member
=== RUN   TestAccAWSGuardDuty/Member/basic
=== RUN   TestAccAWSGuardDuty/Member/import
--- PASS: TestAccAWSGuardDuty (77.94s)
    --- PASS: TestAccAWSGuardDuty/Detector (47.27s)
        --- PASS: TestAccAWSGuardDuty/Detector/basic (33.38s)
        --- PASS: TestAccAWSGuardDuty/Detector/import (13.89s)
    --- PASS: TestAccAWSGuardDuty/Member (30.66s)
        --- PASS: TestAccAWSGuardDuty/Member/basic (15.09s)
        --- PASS: TestAccAWSGuardDuty/Member/import (15.58s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	77.977s

radeksimko

Looking pretty good.

I left you 2 major questions/suggestions about potential crashes and 1 minor about tests synchronicity.

radeksimko · 2018-01-11T12:42:17Z

aws/resource_aws_guardduty_member.go

+	log.Printf("[DEBUG] Reading GuardDuty Member: %s", input)
+	gmo, err := conn.GetMembers(&input)
+	if err != nil {
+		if isAWSErr(err, guardduty.ErrCodeBadRequestException, "The request is rejected because the input detectorId is not owned by the current account.") {


Yeah its like S3 NoSuchBucket in this regard. 😢

radeksimko · 2018-01-11T12:44:53Z

aws/resource_aws_guardduty_member.go

+	member := gmo.Members[0]
+	d.Set("account_id", *member.AccountId)
+	d.Set("detector_id", detectorID)
+	d.Set("email", *member.Email)


Dereferencing here is not necessary, or in fact undesired. Set() will perform safe dereferencing - meaning that it won't crash on nil. We had many crash reports caused by this in the past which led us to this thought.

Do you mind changing it accordingly? 😉

💯 I'll update this code and personally file this under "leave the stars to the night sky"!

radeksimko · 2018-01-11T12:47:32Z

aws/resource_aws_guardduty_member.go

+
+	idParts := strings.Split(d.Id(), ":")
+	accountID := idParts[1]
+	detectorID := idParts[0]


Q: What happens if the user passes only one ID? Isn't it going to crash here because Split() won't return [1]?

I think it would be nice to have a helper function for this, e.g. accId, detectorId, err := decodeGuardDutyMemberId(d.Id()) and keep the error message there.

Oi! This is a very good catch! I totally forgot to split it out into a decode function with the error checking after initially writing it.

radeksimko · 2018-01-11T12:49:31Z

aws/resource_aws_guardduty_test.go

+		"Member": {
+			"basic":  testAccAwsGuardDutyMember_basic,
+			"import": testAccAwsGuardDutyMember_import,
+		},


I assume there can be more than 1 member per AWS account, so we don't need to make these tests synchronous? Unless I'm mistaken...

This needs to be like this because its like AWS Config, where it can only be activated once in a region, so we're left with serially doing one test at a time since we turn the GuardDuty detector on and off each test.

I see, 👌

* Do not unsafely dereference member attributes for d.Set() * Separate ID parsing into decodeGuardDutyMemberID with error checking

bflad · 2018-01-11T16:04:53Z

Updated and verified locally! Thanks for those very important catches. 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccAWSGuardDuty'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSGuardDuty -timeout 120m
=== RUN   TestAccAWSGuardDuty
=== RUN   TestAccAWSGuardDuty/Detector
=== RUN   TestAccAWSGuardDuty/Detector/basic
=== RUN   TestAccAWSGuardDuty/Detector/import
=== RUN   TestAccAWSGuardDuty/Member
=== RUN   TestAccAWSGuardDuty/Member/basic
=== RUN   TestAccAWSGuardDuty/Member/import
--- PASS: TestAccAWSGuardDuty (53.58s)
    --- PASS: TestAccAWSGuardDuty/Detector (31.83s)
        --- PASS: TestAccAWSGuardDuty/Detector/basic (20.85s)
        --- PASS: TestAccAWSGuardDuty/Detector/import (10.97s)
    --- PASS: TestAccAWSGuardDuty/Member (21.75s)
        --- PASS: TestAccAWSGuardDuty/Member/basic (10.05s)
        --- PASS: TestAccAWSGuardDuty/Member/import (11.70s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	53.620s

radeksimko

LGTM.

radeksimko · 2018-01-11T18:17:39Z

aws/resource_aws_guardduty_member.go

+func decodeGuardDutyMemberID(id string) (accountID, detectorID string, err error) {
+	parts := strings.Split(id, ":")
+	if len(parts) != 2 {
+		err = fmt.Errorf("GuardDuty Member ID must be of the form <Detector ID>:<Member AWS Account ID>")


Nitpick: We could include the real ID in the error message here.

radeksimko · 2018-01-11T18:17:55Z

aws/resource_aws_guardduty_test.go

+		"Member": {
+			"basic":  testAccAwsGuardDutyMember_basic,
+			"import": testAccAwsGuardDutyMember_import,
+		},


I see, 👌

…ect format

bflad · 2018-01-12T14:49:29Z

This has been released in terraform-provider-aws version 1.7.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

* commit 'b9284490eff637460fef663794a496d363e19f10': (190 commits) v1.7.0 Update CHANGELOG.md d/aws_ssm_parameter: Support returning raw encrypted SecureString value. (hashicorp#2777) Bump aws-sdk-go to v1.12.60 Update CHANGELOG.md Add acceptance test for import + randomization Removed reference to Core fixes Add instructions for vendor updates Use AWS example instead of Azure Update CHANGELOG for hashicorp#2833 Update CHANGELOG.md r/lb_target_group: Fix validation rules for LB's healthcheck Update CHANGELOG for hashicorp#2911 r/aws_guardduty_member: Provide given ID in error message when incorrect format Update CHANGELOG.md Update CHANGELOG with hashicorp#2888 r/aws_guardduty_member: hashicorp#2911 PR review r/aws_cloudwatch_event_permission: hashicorp#2888 PR review Makefile: Fixed test outputs resource/aws_lb+aws_elb: Fix regression with undefined 'name' ... # Conflicts: # .gitignore # aws/data_source_aws_s3_bucket_object.go # aws/resource_aws_elasticsearch_domain.go

ghost · 2020-04-08T17:51:27Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

New Resource: aws_guardduty_member

13845f9

bflad mentioned this pull request Jan 9, 2018

Add GuardDuty Support #2489

Closed

Ninir added enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. labels Jan 10, 2018

bflad added the service/guardduty Issues and PRs that pertain to the guardduty service. label Jan 11, 2018

radeksimko suggested changes Jan 11, 2018

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 11, 2018

r/aws_guardduty_member: hashicorp#2911 PR review

33d551c

* Do not unsafely dereference member attributes for d.Set() * Separate ID parsing into decodeGuardDutyMemberID with error checking

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 11, 2018

bflad requested a review from radeksimko January 11, 2018 16:05

radeksimko approved these changes Jan 11, 2018

View reviewed changes

r/aws_guardduty_member: Provide given ID in error message when incorr…

c3ab8de

…ect format

bflad merged commit d811d63 into hashicorp:master Jan 11, 2018

bflad deleted the f-aws_guardduty_member branch January 11, 2018 18:41

bflad added a commit that referenced this pull request Jan 11, 2018

Update CHANGELOG for #2911

4fedd19

bflad added this to the v1.7.0 milestone Jan 12, 2018

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_guardduty_member #2911

New Resource: aws_guardduty_member #2911

bflad commented Jan 9, 2018

radeksimko left a comment

radeksimko Jan 11, 2018

bflad Jan 11, 2018

radeksimko Jan 11, 2018

bflad Jan 11, 2018

radeksimko Jan 11, 2018

bflad Jan 11, 2018

radeksimko Jan 11, 2018

bflad Jan 11, 2018

radeksimko Jan 11, 2018

bflad commented Jan 11, 2018

radeksimko left a comment

radeksimko Jan 11, 2018

radeksimko Jan 11, 2018

bflad commented Jan 12, 2018

ghost commented Apr 8, 2020

New Resource: aws_guardduty_member #2911

New Resource: aws_guardduty_member #2911

Conversation

bflad commented Jan 9, 2018

radeksimko 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

bflad commented Jan 11, 2018

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Jan 12, 2018

ghost commented Apr 8, 2020