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.

Sign up for GitHub

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

Jump to bottom

New resource: aws_macie_s3_bucket_association #5201

Merged

bflad merged 4 commits into hashicorp:master from ewbankkit:issue-4992

Jul 18, 2018

Contributor

ewbankkit commented Jul 16, 2018

Fixes #4992.
This is the initial Amazon Macie resource, associating S3 resources with Amazon Macie for monitoring and data classification.

Acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSMacieS3BucketAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSMacieS3BucketAssociation_ -timeout 120m
=== RUN   TestAccAWSMacieS3BucketAssociation_basic
--- PASS: TestAccAWSMacieS3BucketAssociation_basic (67.79s)
=== RUN   TestAccAWSMacieS3BucketAssociation_accountIdAndPrefix
--- PASS: TestAccAWSMacieS3BucketAssociation_accountIdAndPrefix (71.10s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	152.742s


          Add 'aws_macie_s3_bucket_association' resource.

c34d09e

ghost added the size/L label

ewbankkit changed the title ~~New Resource: aws_macie_s3_bucket_association~~ New resource: aws_macie_s3_bucket_association

ewbankkit mentioned this pull request

New resource: Amazon Macie account association #5202

Closed

bflad added new-resource service/macie labels

bflad reviewed

View reviewed changes

Contributor

bflad left a comment

Hi @ewbankkit 👋 Thanks for submitting this! I left some initial feedback below. Can you please take a look or let us know if you do not have time to implement it?

aws/resource_aws_macie_s3_bucket_association.go Outdated

+              	return nil
+              }
+              func macieS3BucketAssociationId(d *schema.ResourceData) string {

Contributor

bflad Jul 17, 2018

Nitpick: This indirection is unnecessary as its only used once for a one-liner. It seems like we should also include the member account ID so we can easily support passthrough import.

Contributor Author

ewbankkit Jul 17, 2018

OK, I'll fix.

aws/resource_aws_macie_s3_bucket_association.go Outdated

+              	}
+              	if oneTime {
+              		return macie.S3OneTimeClassificationTypeFull
+              	} else {

Contributor

bflad Jul 17, 2018

Nitpick: The else nesting here is extraneous as the if has a return statement.

Contributor Author

ewbankkit Jul 17, 2018

👍

aws/resource_aws_macie_s3_bucket_association.go Outdated

+              	prefix := d.Get("prefix")
+              	var res *macie.S3ResourceClassification
+              	for {
+              		resp, err := conn.ListS3Resources(req)

Contributor

bflad Jul 17, 2018

We should prefer to use the available SDK paginated function: https://docs.aws.amazon.com/sdk-for-go/api/service/macie/#Macie.ListS3ResourcesPages

Contributor Author

ewbankkit Jul 17, 2018

👍

aws/resource_aws_macie_s3_bucket_association.go Outdated

+              		return nil
+              	}
+              	m := map[string]interface{}{}

Contributor

bflad Jul 17, 2018

To prevent potential panics, we should perform a nil check for res.ClassificationType before dereferencing.

We also generally prefer to move this type of logic into a "flatten" function, e.g.

func flattenMacieClassificationType(classificationType *macie.ClassificationType) []map[string]interface{} {
  if classificationType == nil {
    return []map[string]interface{}{}
  }
  m := map[string]interface{}{
    "one_time": false,
  }
  if aws.StringValue(res.ClassificationType.OneTime) == macie.S3OneTimeClassificationTypeFull {
    m["one_time"] = true
  }
  return []map[string]interface{}{m}
}

We should also provide a helpful error message when d.Set() fails, e.g.

if err := d.Set("classification_type", flattenMacieClassificationType(res.ClassificationType)); err != nil {
  return fmt.Errorf("error setting classification_type: %s", err)
}

Contributor Author

ewbankkit Jul 17, 2018

👍

aws/resource_aws_macie_s3_bucket_association.go Outdated

+              				Elem: &schema.Resource{
+              					Schema: map[string]*schema.Schema{
+              						"one_time": {
+              							Type:     schema.TypeBool,

Contributor

bflad Jul 17, 2018

We might want to consider following the SDK and using a TypeString with the constant values in case they introduce additional types. This will also simplify the logic to remove the conditional handling.

Contributor Author

ewbankkit Jul 17, 2018

Agreed, trying to second guess what might come in the future is guaranteed compatibility pain.
I'll also add in the continuous attribute of TypeString.

aws/resource_aws_macie_s3_bucket_association_test.go

+              	"github.com/hashicorp/terraform/terraform"
+              )
+              func TestAccAWSMacieS3BucketAssociation_basic(t *testing.T) {

Contributor

bflad Jul 17, 2018

Is there a way to programatically enable/disable Macie?

=== RUN   TestAccAWSMacieS3BucketAssociation_basic
--- FAIL: TestAccAWSMacieS3BucketAssociation_basic (6.07s)
	testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:
		
		* aws_macie_s3_bucket_association.test: 1 error(s) occurred:
		
		* aws_macie_s3_bucket_association.test: Error creating Macie S3 bucket association: InvalidInputException: The request was rejected. Macie is not enabled for this AWS account *******.

Contributor Author

ewbankkit Jul 17, 2018

I'll see if AssociateMemberAccount with the current account ID does that.
I just manually enabled Macie via the console.

Contributor Author

ewbankkit Jul 17, 2018

No, it looks like Macie cannot programatically be enabled:

$ aws --region us-east-1 macie list-member-accounts

An error occurred (InvalidInputException) when calling the ListMemberAccounts operation: The request was rejected. Macie is not enabled for this AWS account 999999999999.
$ aws --region us-east-1 macie associate-member-account --member-account-id 999999999999

An error occurred (InvalidInputException) when calling the AssociateMemberAccount operation: The request was rejected. Macie is not enabled for this AWS account 999999999999.

I'll add a link to the manual setup instructions to the documentation.

aws/resource_aws_macie_s3_bucket_association_test.go Outdated

+              		}
+              		for {
+              			resp, err := conn.ListS3Resources(req)

Contributor

bflad Jul 17, 2018

We should prefer to also use the paginated method here 👍

Contributor Author

ewbankkit Jul 17, 2018

👍

aws/resource_aws_macie_s3_bucket_association_test.go

+              	return nil
+              }
+              func testAccCheckAWSMacieS3BucketAssociationExists(name string) resource.TestCheckFunc {

Contributor

bflad Jul 17, 2018

This function should perform similar logic as the destroy check above to ensure the S3 resources are appropriately allocated in Macie.

Contributor Author

ewbankkit Jul 17, 2018

👍

bflad added the waiting-response label

Contributor Author

ewbankkit commented Jul 17, 2018

Yes, should get time to address these today.

Kit Ewbank added 2 commits

July 17, 2018 14:08


          Changes after code review - Part I.

2b851ef


          Changes after code review - Part II.

606c330

ghost added size/XL and removed size/L labels

bflad removed the waiting-response label

Contributor Author

ewbankkit commented Jul 17, 2018

Acceptance tests after code review changes:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSMacieS3BucketAssociation_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSMacieS3BucketAssociation_ -timeout 120m
=== RUN   TestAccAWSMacieS3BucketAssociation_basic
--- PASS: TestAccAWSMacieS3BucketAssociation_basic (65.16s)
=== RUN   TestAccAWSMacieS3BucketAssociation_accountIdAndPrefix
--- PASS: TestAccAWSMacieS3BucketAssociation_accountIdAndPrefix (72.33s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	138.118s


          Add link to instructions for manually enabling Macie.

878a5ff

ghost added the size/XL label

bflad approved these changes

View reviewed changes

Contributor

bflad left a comment

Looks great, thanks @ewbankkit! 🚀

2 tests passed (all tests)
=== RUN   TestAccAWSMacieS3BucketAssociation_basic
--- PASS: TestAccAWSMacieS3BucketAssociation_basic (23.34s)
=== RUN   TestAccAWSMacieS3BucketAssociation_accountIdAndPrefix
--- PASS: TestAccAWSMacieS3BucketAssociation_accountIdAndPrefix (28.21s)

bflad added this to the v1.28.0 milestone

bflad merged commit c07d364 into hashicorp:master

bflad added a commit that referenced this pull request


          Update CHANGELOG for #5201

d9073ca

ewbankkit deleted the issue-4992 branch

July 18, 2018 13:42

Contributor

bflad commented Jul 18, 2018

This has been released in version 1.28.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

MKgridSec mentioned this pull request

aws_macie_s3_bucket_association plans mutually exclusive classification_type settings #12589

Closed

ghost commented Apr 4, 2020

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!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new-resource service/macie size/XL