Proposal: Refactoring AWS waiters (and examples for Lambda event source mapping and S3 bucket)

[vancluever]

The following PR (and the aws_waiters branch) is the result of the work that I did on waiters today.

After several hours of diving into the waiters in the AWS Go SDK this morning, I found that Terraform already has a pretty competent retry waiter in helper/resource/wait.go, that is actually in use with several of the AWS provider resources already. However, it looked like there was a lot of code duplication, so I refactored the existing pattern into a new aws.retryWaiter function within the provider itself.

The method is contained and documented in builtin/providers/aws/wait.go. I have also re-factored the policy application function in aws_s3_bucket and the event source mapping creation in aws_lambda_event_source_mapping to use it. The latter addresses the issue that I mentioned in my last PR #4093.

If this looks good and/or pending other comment I might go about checking out fixing #3557, if this is still an issue, and re-factoring the other instances of its use in the AWS provider. This can then be used easily to address other issues of the like should they come up.

Also not too sure if test coverage is going to be necessary or if just picking it up thru the consuming resources will be enough.

PS: This PR will probably have unrelated commits on it until #4093 is merged.
PPS: After looking at the AWS Go SDK's own waiter I actually think Terraform's is currently in better shape, as it offers a way to get return data and also has backoff baked in already.

[jen20]

This is probably one for @catsby to review!

[vancluever]

Rebased to bring in line with my master's rebase for #4093

[vancluever]

One of the errors that this resolves:

Error applying plan:

1 error(s) occurred:

* aws_s3_bucket.test_s3_bucket: Error putting S3 policy: MalformedPolicy: Invalid principal in policy
    status code: 400, request id:

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

With the following:

variable "aws_account_id" {}
variable "region" {
  default = "us-east-1"
}
provider "aws" {
    region = "${var.region}"
}
resource "aws_s3_bucket" "test_s3_bucket" {
    bucket = "test_s3_bucket.${var.aws_account_id}.s3.example.com"
    acl = "private"
    policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [ "s3:GetObject", "s3:PutObject", "s3:DeleteObject" ],
      "Effect": "Allow",
      "Resource": "arn:aws:s3:::test_s3_bucket.${var.aws_account_id}.s3.example.com/*",
      "Principal": { "AWS": "${aws_iam_role.test_iam_role.arn}" }
    },
    {
      "Action": [ "s3:ListBucket" ],
      "Effect": "Allow",
      "Resource": "arn:aws:s3:::test_s3_bucket.${var.aws_account_id}.s3.example.com",
      "Principal": { "AWS": "${aws_iam_role.test_iam_role.arn}" }
    }
  ]
}
EOF
}
resource "aws_iam_policy" "test_iam_policy" {
    name = "test_iam_policy"
    path = "/"
    policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
      {
          "Effect": "Allow",
          "Action": [ "s3:*" ],
          "Resource": "${aws_s3_bucket.test_s3_bucket.arn}/*"
      },
      {
          "Effect": "Allow",
          "Action": [
            "kinesis:GetRecords",
            "kinesis:GetShardIterator",
            "kinesis:DescribeStream"
          ],
          "Resource": "*"
      },
      {
          "Effect": "Allow",
          "Action": [
            "kinesis:ListStreams"
          ],
          "Resource": "*"
      }
  ]
}
EOF
}
resource "aws_iam_role" "test_iam_role" {
    name = "test_iam_role"
    assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": "sts:AssumeRole",
      "Principal": {
        "Service": "lambda.amazonaws.com"
      },
      "Effect": "Allow"
    }
  ]
}
EOF
}

Also, this is coming up on Lambda-related stuff, namely with my new resource that I've got in PR #4093.

[vancluever]

Hey guys any idea on the status of this? If there's any suggestions on changes or anything that needs to be done to get this merged, let me know.

[catsby]

I have not had a chance to review in detail. I'll do so this week. Sorry for the delay

[vancluever]

Thanks @catsby, again if something is needed to help complete this by all means let me know!

[catsby]

I think we need return err here, otherwise if err != nil but is not an awserr.Error, we return nil, right?

[catsby]

Hey @vancluever – thank you for the contribution here, I appreciate the work to make Terraform easier to work with and more consistent 😄

We're going to pass on this for now though. As we've discussed on aws/aws-sdk-go#454 , I think first-class support for the waiters is something worth waiting for. I have faith the Amazon folks will get us the hooks we need if we just give them sometime, at which point hopefully we'll have lots of code to rip out here 😄 Until then, I think we'll just limp along with the duplication.

Thanks again!

[vancluever]

OK well in that case, do you mind if I submit the PR for these uses with current pattern? The lack of retry features on the S3 policies and the Lambda event source mapping stuff is still burning us. I'll just follow the pattern in the other resources.

[catsby]

@vancluever by all means, please do! I'll do my best to get them reviewed and merged promptly

[vancluever]

Thanks @catsby! I've referenced the PR's back to this issue.

[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.