provider/aws: Added S3 Bucket replication

[Ninir]

Reasonning

This adds the S3 Bucket replication.

Related Issues

Closes #7490

Acceptance tests

Sometimes, tests are failing and it is said that S3Bucket error: BucketRegionError: incorrect region, the bucket is not in 'eu-west-1' region, whereas buckets are created. I think it is related to the providerFactories stuff, but can't tell without any investigation.

However, I used this configuration (the one i added to the documentation) to check the compiled binary, and played with it (adding, removing, updating, etc etc.):

provider "aws" {
  alias  = "west"
  region = "eu-west-1"
}

provider "aws" {
  alias  = "central"
  region = "eu-central-1"
}

resource "aws_iam_role" "replication" {
  name               = "tf-iam-role-replication-12345"
  assume_role_policy = <<POLICY
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": "sts:AssumeRole",
      "Principal": {
        "Service": "s3.amazonaws.com"
      },
      "Effect": "Allow",
      "Sid": ""
    }
  ]
}
POLICY
}

resource "aws_iam_policy" "replication" {
    name = "tf-iam-role-policy-replication-12345"
    policy = <<POLICY
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "s3:GetReplicationConfiguration",
        "s3:ListBucket"
      ],
      "Effect": "Allow",
      "Resource": [
        "${aws_s3_bucket.bucket.arn}"
      ]
    },
    {
      "Action": [
        "s3:GetObjectVersion",
        "s3:GetObjectVersionAcl"
      ],
      "Effect": "Allow",
      "Resource": [
        "${aws_s3_bucket.bucket.arn}/*"
      ]
    },
    {
      "Action": [
        "s3:ReplicateObject",
        "s3:ReplicateDelete"
      ],
      "Effect": "Allow",
      "Resource": "${aws_s3_bucket.destination.arn}/*"
    }
  ]
}
POLICY
}

resource "aws_iam_policy_attachment" "replication" {
    name = "tf-iam-role-attachment-replication-12345"
    roles = ["${aws_iam_role.replication.name}"]
    policy_arn = "${aws_iam_policy.replication.arn}"
}

resource "aws_s3_bucket" "destination" {
    provider = "aws.west"
    bucket   = "tf-test-bucket-destination-123456"
    region   = "eu-west-1"

    versioning {
        enabled = true
    }
}

resource "aws_s3_bucket" "bucket" {
    provider = "aws.central"
    bucket   = "tf-test-bucket-123456"
    acl      = "private"
    region   = "eu-central-1"

    versioning {
        enabled = true
    }

    replication_configuration {
        role = "${aws_iam_role.replication.arn}"
        rules {
            id     = "foobar"
            prefix = ""
            status = "Enabled"

            destination {
                bucket        = "${aws_s3_bucket.destination.arn}"
                storage_class = "STANDARD"
            }
        }
    }
}

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3Bucket_Replication'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/06 23:04:30 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Replication -timeout 120m
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (61.29s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (28.35s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	89.673s

$ make test TEST=./builtin/providers/aws TESTARGS='-run=TestValidateS3BucketReplicationRuleId'
==> Checking that code complies with gofmt requirements...
==> Checking AWS provider for unchecked errors...
==> NOTE: at this time we only look for uncheck errors in the AWS package
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/07 22:53:33 Generated command/internal_plugin_list.go
TF_ACC= go test ./builtin/providers/aws -run=TestValidateS3BucketReplicationRuleId -timeout=30s -parallel=4
ok  	github.com/hashicorp/terraform/builtin/providers/aws	0.022s

$ make test TEST=./builtin/providers/aws TESTARGS='-run=TestValidateS3BucketReplicationRulePrefix'
==> Checking that code complies with gofmt requirements...
==> Checking AWS provider for unchecked errors...
==> NOTE: at this time we only look for uncheck errors in the AWS package
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/07 22:54:20 Generated command/internal_plugin_list.go
TF_ACC= go test ./builtin/providers/aws -run=TestValidateS3BucketReplicationRulePrefix -timeout=30s -parallel=4
ok  	github.com/hashicorp/terraform/builtin/providers/aws	0.026s

[kwilczynski]

@Ninir hi there!

Sir, keep being amazing! 🍰 ❤️

[stack72]

The only comment I have here is to extract this into a flatten function - this will allow the code to be a little more concise in this area

I suggest something like this:

if replication.ReplicationConfiguration != nil {
  if err := d.Set("replication_configuration", flattenAwsS3BucketReplicationConfiguration(replication.ReplicationConfiguration)); err != nil {
		log.Printf("[DEBUG] Error setting replication configuration: %s", err)
		return err
	}
}

[stack72]

Hi @Ninir

this is a great PR! I may 1 comment inline about extracting code to functions to keep the code concise in the Create / Read and Update areas

May be worth having a look at seeing what you think

It's not a huge blocker but would be a nice addition

Paul

[Ninir]

@kwilczynski 😳 thank you good sir!

@stack72 +1 for the decoupling, just updated it! thanks for the suggestion :)

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3Bucket_Replication'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/07 16:22:20 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Replication -timeout 120m
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (122.05s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (40.16s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	162.244s

[stack72]

Hi @Ninir

Not sure what the issue is here:

=== RUN   TestAccAWSS3Bucket_Replication
--- FAIL: TestAccAWSS3Bucket_Replication (33.14s)
	testing.go:265: Step 0 error: Check failed: Check 1/1 error: S3Bucket error: BucketRegionError: incorrect region, the bucket is not in 'us-west-2' region
			status code: 301, request id:
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (32.58s)

Paul

[Ninir]

@stack72 Ok found the issue! 😄
After trying to solve the issue from different perspectives... it is now crystal-clear.
In my case, I was running tests without any region set as env vars, so depending on the providers set in the config (which works fine in a real case).

Thus, if you use the callee region to create the source bucket (explicitly set to us-west-2 in this case), and use a provider for the other one, then it works fine. I ran tests 3 times to be sure, all with success. I suspect an issue with providerFactories, that I will investigate later on.

To sum-up: use us-west-2 as an env var for the source bucket, and eu-west-1 with a provider for the destination bucket.

Here is the output:

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3Bucket_Replication'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/07 23:19:50 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Replication -timeout 120m
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (113.14s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (37.65s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	150.817s

[stack72]

@Ninir that's an amazing find! Good work :) This now LGTM

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3Bucket_Replication'                 ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/08 11:46:28 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Replication -timeout 120m
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (192.16s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (76.39s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	268.610s

[Ninir]

Thanks to both of you for hints and quick reviews :)

[catsby]

Hello –

I'm still seeing failures here in our CI:

------- Stdout: -------
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- FAIL: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (23.29s)
    testing.go:265: Step 0 error: Check failed: Check 1/1 error: S3Bucket error: BucketRegionError: incorrect region, the bucket is not in 'eu-west-1' region
            status code: 301, request id: 

Local runs produce similarly inconsistent results:

TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Replication -timeout 120m
=== RUN   TestAccAWSS3Bucket_Replication
--- FAIL: TestAccAWSS3Bucket_Replication (30.52s)
        testing.go:265: Step 0 error: Check failed: Check 1/1 error: S3Bucket error: BucketRegionError: incorrect region, the bucket is not in 'eu-west-1' region
                        status code: 301, request id:
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (54.71s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (31.42s)
FAIL
exit status 1

[Ninir]

Hi @catsby

Are you having success & failures or simply failures?

This makes me think this is somehow related to the way provider are managed in tests. Never happened during the plan/apply phase, when doing it manually.

Also, I remember seeing issues with providers issues too. 'Will investigate as soon as possible.

[catsby]

The failures seem inconsistent. Sometimes it's TestAccAWSS3Bucket_ReplicationWithoutStorageClass, others it's TestAccAWSS3Bucket_Replication, as shown above

[Ninir]

Hi @catsby

While playing & investigating this S3 Replication issue related on testacc, I'm still really suspecting something done here: https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_s3_bucket_test.go#L609-L616

I can't reproduce any issue when using the compiled binary & playing with a flat tf file manually. However, it happens a lot when using the testing framework.

What are your thoughts on this?

[catsby]

That block of code seems to construct a slice of Providers and feeds it into the destroy method, as a way of saying "check these provider configs for the object in order to destroy it"

From I see, that block only creates a single provider, with no region specified, so I assume it's pulling that from the env of the machine running it (Either our CI, or a local machine, etc). It's kind of unclear to me what the intent is there.

If we know the regions we may just want to do this manually (change regions and try again) without the factories, sort of like I do here:

• https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_opsworks_stack.go#L373

Please let me know if this is something you can tackle, otherwise I'll try to get to it...

[Ninir]

Hi @catsby

I am off for 10 days :/ I'll be happy to let you handle it! Otherwise, I'll do it when i'll get back on a computer :)

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