provider/aws: Support S3 bucket notification

[kjmkznr]

Add S3 bucket notification resource.

The reason for creating a new resource is to avoid the circular reference.

resource "aws_sns_topic" "topic" {
    name = "terraform-test-topic"
        policy = <<POLICY
{
        "Version":"2012-10-17",
        "Statement":[{
                "Effect": "Allow",
                "Principal": {"AWS":"*"},
                "Action": "SNS:Publish",
                "Resource": "arn:aws:sns:*:*:terraform-test-topic",
                "Condition":{
                        "ArnLike":{"aws:SourceArn":"${aws_s3_bucket.bucket.arn}"}
                }
        }]
}
POLICY
}

resource "aws_s3_bucket" "bucket" {
        bucket = "tf-test-bucket-1928392189481395839258"
}

resource "aws_s3_bucket_notification" "bucket_notification" {
    bucket = "${aws_s3_bucket.bucket.id}"
    topic {
        topic_arn = "${aws_sns_topic.topic.arn}"
        events = ["s3:ObjectCreated:*"]
        filter_suffix = ".log"
    }
    queue {
        ...
    }
    lambda_function {
        ...
    }
}

Acceptance test

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3Bucket_Notification'                                                         
==> Checking that code complies with gofmt requirements...
/home/kjmkznr/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Notification -timeout 120m
=== RUN   TestAccAWSS3Bucket_Notification
--- PASS: TestAccAWSS3Bucket_Notification (66.94s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    66.945s

[stack72]

This may #4120

[catsby]

These Set functions do not account for all the elements in the set, so changing anything outside of filter_prefix or filter_suffix will not be recognized, e.g. changing to another SNS Topic.

I believe it's safe to simply omit the Set func entirely here.

[catsby]

Poking at this further, it seems that if omitted, the id will get populated by the API. This is troublesome because we'll then see a diff in future plans if we don't have that populated.

[kjmkznr]

@catsby
I think id parameter is good to change to auto generate by terraform when empty like AutoScaling Group name.
What do you think?

[kjmkznr]

I changed to auto generate id when not specific id parameter.

[catsby]

This looks really promising, I'm excited to have it in! We need to touch up some things first though as I noted above, please let me know if you have any questions about them

[akerl]

+1 to this, just found it while trying to set up S3 -> lambda notifications

[akerl]

I'm digging into it a bit now, but when I use this with:

resource "aws_s3_bucket_notification" "cloudtrail_bucket_notification" {
    bucket = "${aws_s3_bucket.cloudtrail_bucket.id}"
    lambda_function {
        id = "notifylambda"
        lambda_function = "${aws_lambda_function.cloudtrail_alert_lambda.arn}"
        events = ["s3:ObjectCreated:*"]
    }
}

I get this:

* aws_s3_bucket_notification.cloudtrail_bucket_notification: Error putting S3 notification configuration: MalformedXML: The XML you provided was not well-formed or did not validate against our published schema
    status code: 400, request id: BLAH

[akerl]

Found the issue: if I add a filter_prefix of "*", it works:

resource "aws_s3_bucket_notification" "cloudtrail_bucket_notification" {
    bucket = "${aws_s3_bucket.cloudtrail_bucket.id}"
    lambda_function {
        id = "notifylambda"
        lambda_function = "${aws_lambda_function.cloudtrail_alert_lambda.arn}"
        events = ["s3:ObjectCreated:*"]
        filter_prefix = "*"
    }
}

I double checked what CloudTrail was catching for the error before:

    "requestParameters": {
        "bucketName": "BUCKETl",
        "NotificationConfiguration": {
            "CloudFunctionConfiguration": {
                "Filter": {
                    "S3Key": ""
                },
                "CloudFunction": "arn:aws:lambda:us-east-1:REDACTED:function:cloudtrail_alert_lambda",
                "Event": "s3:ObjectCreated:*",
                "Id": "notifylambda"
            }
        }
    },

So it looks like it should be tweaked to not provide a filter key to the API if no filter is provided to the resource definition

[akerl]

It turns out that "*" is taken as the literal *, not a wildcard. In my specific case, I just updated it to be AWSLogs, but that's not quite the global workaround I'd thought it would be.

[kjmkznr]

@akerl Thanks find bug.
Fixed failed when not specific both filter_prefix and filter_suffix parameters.
Please try again.

[curena]

This is looking really nice. I hope it makes it into 0.6.14.

[curena]

What's the status on this PR? Will this make it to the next release?

[stack72]

This looks now good that the initial fixes were made. I am going to merge and squash the commits

test results are as follows:

TF_LOG=1 make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3Bucket_Notification' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Notification -timeout 120m
=== RUN   TestAccAWSS3Bucket_Notification
--- PASS: TestAccAWSS3Bucket_Notification (67.51s)
=== RUN   TestAccAWSS3Bucket_NotificationWithoutFilter
--- PASS: TestAccAWSS3Bucket_NotificationWithoutFilter (28.43s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    95.958s

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.