Skip to content

Commit

Permalink
resource/aws_s3_bucket: Address PR #3577 feedback
Browse files Browse the repository at this point in the history 
* Switch replication rule destination access_control_translation from TypeSet to TypeList
* Add replication rule destination account validation
* Add replication rule destination access_control_translation owner validation
* Split replication rule destination access_control_translation acceptance testing into its own function and add non-encrypted configuration TestStep
* Ensure testAccCheckAWSS3BucketReplicationRules can translate HIL references in Rule.Destination.Account
  • Loading branch information
@bflad
bflad committed Oct 10, 2018
1 parent a5cc820 commit c7d1d5a
Show file tree
Hide file tree
Showing 2 changed files with 160 additions and 53 deletions.
25 changes: 15 additions & 10 deletions aws/resource_aws_s3_bucket.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,9 @@ func resourceAwsS3Bucket() *schema.Resource {
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"account": {
Type: schema.TypeString,
Optional: true,
Type: schema.TypeString,
Optional: true,
ValidateFunc: validateAwsAccountId,
},
"bucket": {
Type: schema.TypeString,
Expand All @@ -401,7 +402,7 @@ func resourceAwsS3Bucket() *schema.Resource {
Optional: true,
},
"access_control_translation": {
Type: schema.TypeSet,
Type: schema.TypeList,
Optional: true,
MinItems: 1,
MaxItems: 1,
Expand All @@ -410,6 +411,9 @@ func resourceAwsS3Bucket() *schema.Resource {
"owner": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validation.StringInSlice([]string{
s3.OwnerOverrideDestination,
}, false),
},
},
},
Expand Down Expand Up @@ -1778,8 +1782,8 @@ func resourceAwsS3BucketReplicationConfigurationUpdate(s3conn *s3.S3, d *schema.
ruleDestination.Account = aws.String(account.(string))
}

if aclTranslation, ok := bd["access_control_translation"].(*schema.Set); ok && aclTranslation.Len() > 0 {
aclTranslationValues := aclTranslation.List()[0].(map[string]interface{})
if aclTranslation, ok := bd["access_control_translation"].([]interface{}); ok && len(aclTranslation) > 0 {
aclTranslationValues := aclTranslation[0].(map[string]interface{})
ruleAclTranslation := &s3.AccessControlTranslation{}
ruleAclTranslation.Owner = aws.String(aclTranslationValues["owner"].(string))
ruleDestination.AccessControlTranslation = ruleAclTranslation
Expand Down Expand Up @@ -2034,9 +2038,10 @@ func flattenAwsS3BucketReplicationConfiguration(r *s3.ReplicationConfiguration)
rd["account"] = *v.Destination.Account
}
if v.Destination.AccessControlTranslation != nil {
rdt := make(map[string]interface{})
rdt["owner"] = *v.Destination.AccessControlTranslation.Owner
rd["access_control_translation"] = schema.NewSet(accessControlTranslationHash, []interface{}{rdt})
rdt := map[string]interface{}{
"owner": aws.StringValue(v.Destination.AccessControlTranslation.Owner),
}
rd["access_control_translation"] = []interface{}{rdt}
}
t["destination"] = schema.NewSet(destinationHash, []interface{}{rd})
}
Expand Down Expand Up @@ -2227,8 +2232,8 @@ func destinationHash(v interface{}) int {
if v, ok := m["account"]; ok {
buf.WriteString(fmt.Sprintf("%s-", v.(string)))
}
if v, ok := m["access_control_translation"].(*schema.Set); ok && v.Len() > 0 && v.List()[0] != nil {
buf.WriteString(fmt.Sprintf("%d-", accessControlTranslationHash(v.List()[0])))
if v, ok := m["access_control_translation"].([]interface{}); ok && len(v) > 0 && v[0] != nil {
buf.WriteString(fmt.Sprintf("%d-", accessControlTranslationHash(v[0])))
}
return hashcode.String(buf.String())
}
Expand Down
188 changes: 145 additions & 43 deletions aws/resource_aws_s3_bucket_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -731,42 +731,6 @@ func TestAccAWSS3Bucket_Cors_EmptyOrigin(t *testing.T) {
),
),
},
{
Config: testAccAWSS3BucketConfigReplicationWithSseKmsEncryptedObjectsAndAccessControlTranslation(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSS3BucketExistsWithProvider("aws_s3_bucket.bucket", testAccAwsRegionProviderFunc("us-west-2", &providers)),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "replication_configuration.#", "1"),
resource.TestMatchResourceAttr("aws_s3_bucket.bucket", "replication_configuration.0.role", regexp.MustCompile(fmt.Sprintf("^arn:aws:iam::[\\d+]+:role/tf-iam-role-replication-%d", rInt))),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "replication_configuration.0.rules.#", "1"),
testAccCheckAWSS3BucketReplicationRules(
"aws_s3_bucket.bucket",
testAccAwsRegionProviderFunc("us-west-2", &providers),
[]*s3.ReplicationRule{
{
ID: aws.String("foobar"),
Destination: &s3.Destination{
Account: aws.String("${data.aws_caller_identity.current.account_id}"),
Bucket: aws.String(fmt.Sprintf("arn:aws:s3:::tf-test-bucket-destination-%d", rInt)),
StorageClass: aws.String(s3.ObjectStorageClassStandard),
EncryptionConfiguration: &s3.EncryptionConfiguration{
ReplicaKmsKeyID: aws.String("${aws_kms_key.replica.arn}"),
},
AccessControlTranslation: &s3.AccessControlTranslation{
Owner: aws.String("Destination"),
},
},
Prefix: aws.String("foo"),
Status: aws.String(s3.ReplicationRuleStatusEnabled),
SourceSelectionCriteria: &s3.SourceSelectionCriteria{
SseKmsEncryptedObjects: &s3.SseKmsEncryptedObjects{
Status: aws.String(s3.SseKmsEncryptedObjectsStatusEnabled),
},
},
},
},
),
),
},
},
})
}
Expand Down Expand Up @@ -1021,6 +985,90 @@ func TestAccAWSS3Bucket_Replication(t *testing.T) {
})
}

func TestAccAWSS3Bucket_ReplicationConfiguration_Rule_Destination_AccessControlTranslation(t *testing.T) {
rInt := acctest.RandInt()
region := testAccGetRegion()
partition := testAccGetPartition()

// record the initialized providers so that we can use them to check for the instances in each region
var providers []*schema.Provider

resource.Test(t, resource.TestCase{
PreCheck: func() {
testAccPreCheck(t)
testAccMultipleRegionsPreCheck(t)
},
ProviderFactories: testAccProviderFactories(&providers),
CheckDestroy: testAccCheckWithProviders(testAccCheckAWSS3BucketDestroyWithProvider, &providers),
Steps: []resource.TestStep{
{
Config: testAccAWSS3BucketConfigReplicationWithAccessControlTranslation(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSS3BucketExistsWithProvider("aws_s3_bucket.bucket", testAccAwsRegionProviderFunc(region, &providers)),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "replication_configuration.#", "1"),
resource.TestMatchResourceAttr("aws_s3_bucket.bucket", "replication_configuration.0.role", regexp.MustCompile(fmt.Sprintf("^arn:%s:iam::[\\d+]+:role/tf-iam-role-replication-%d", partition, rInt))),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "replication_configuration.0.rules.#", "1"),
testAccCheckAWSS3BucketReplicationRules(
"aws_s3_bucket.bucket",
testAccAwsRegionProviderFunc(region, &providers),
[]*s3.ReplicationRule{
{
ID: aws.String("foobar"),
Destination: &s3.Destination{
Account: aws.String("${data.aws_caller_identity.current.account_id}"),
Bucket: aws.String(fmt.Sprintf("arn:%s:s3:::tf-test-bucket-destination-%d", partition, rInt)),
StorageClass: aws.String(s3.ObjectStorageClassStandard),
AccessControlTranslation: &s3.AccessControlTranslation{
Owner: aws.String("Destination"),
},
},
Prefix: aws.String("foo"),
Status: aws.String(s3.ReplicationRuleStatusEnabled),
},
},
),
),
},
{
Config: testAccAWSS3BucketConfigReplicationWithSseKmsEncryptedObjectsAndAccessControlTranslation(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSS3BucketExistsWithProvider("aws_s3_bucket.bucket", testAccAwsRegionProviderFunc(region, &providers)),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "replication_configuration.#", "1"),
resource.TestMatchResourceAttr("aws_s3_bucket.bucket", "replication_configuration.0.role", regexp.MustCompile(fmt.Sprintf("^arn:%s:iam::[\\d+]+:role/tf-iam-role-replication-%d", partition, rInt))),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "replication_configuration.0.rules.#", "1"),
testAccCheckAWSS3BucketReplicationRules(
"aws_s3_bucket.bucket",
testAccAwsRegionProviderFunc(region, &providers),
[]*s3.ReplicationRule{
{
ID: aws.String("foobar"),
Destination: &s3.Destination{
Account: aws.String("${data.aws_caller_identity.current.account_id}"),
Bucket: aws.String(fmt.Sprintf("arn:%s:s3:::tf-test-bucket-destination-%d", partition, rInt)),
StorageClass: aws.String(s3.ObjectStorageClassStandard),
EncryptionConfiguration: &s3.EncryptionConfiguration{
ReplicaKmsKeyID: aws.String("${aws_kms_key.replica.arn}"),
},
AccessControlTranslation: &s3.AccessControlTranslation{
Owner: aws.String("Destination"),
},
},
Prefix: aws.String("foo"),
Status: aws.String(s3.ReplicationRuleStatusEnabled),
SourceSelectionCriteria: &s3.SourceSelectionCriteria{
SseKmsEncryptedObjects: &s3.SseKmsEncryptedObjects{
Status: aws.String(s3.SseKmsEncryptedObjectsStatusEnabled),
},
},
},
},
),
),
},
},
})
}

// StorageClass issue: https://github.com/hashicorp/terraform/issues/10909
func TestAccAWSS3Bucket_ReplicationWithoutStorageClass(t *testing.T) {
rInt := acctest.RandInt()
Expand Down Expand Up @@ -1513,6 +1561,15 @@ func testAccCheckAWSS3BucketReplicationRules(n string, providerF func() *schema.
rs, _ := s.RootModule().Resources[n]
for _, rule := range rules {
if dest := rule.Destination; dest != nil {
if account := dest.Account; account != nil && strings.HasPrefix(aws.StringValue(dest.Account), "${") {
resourceReference := strings.Replace(aws.StringValue(dest.Account), "${", "", 1)
resourceReference = strings.Replace(resourceReference, "}", "", 1)
resourceReferenceParts := strings.Split(resourceReference, ".")
resourceAttribute := resourceReferenceParts[len(resourceReferenceParts)-1]
resourceName := strings.Join(resourceReferenceParts[:len(resourceReferenceParts)-1], ".")
value := s.RootModule().Resources[resourceName].Primary.Attributes[resourceAttribute]
dest.Account = aws.String(value)
}
if ec := dest.EncryptionConfiguration; ec != nil {
if ec.ReplicaKmsKeyID != nil {
key_arn := s.RootModule().Resources["aws_kms_key.replica"].Primary.Attributes["arn"]
Expand Down Expand Up @@ -2243,6 +2300,51 @@ resource "aws_s3_bucket" "destination" {
`, randInt, randInt, randInt)
}

func testAccAWSS3BucketConfigReplicationWithAccessControlTranslation(randInt int) string {
return fmt.Sprintf(testAccAWSS3BucketConfigReplicationBasic+`
data "aws_caller_identity" "current" {}
resource "aws_s3_bucket" "bucket" {
provider = "aws.uswest2"
bucket = "tf-test-bucket-%d"
acl = "private"
versioning {
enabled = true
}
replication_configuration {
role = "${aws_iam_role.role.arn}"
rules {
id = "foobar"
prefix = "foo"
status = "Enabled"
destination {
account = "${data.aws_caller_identity.current.account_id}"
bucket = "${aws_s3_bucket.destination.arn}"
storage_class = "STANDARD"
access_control_translation {
owner = "Destination"
}
}
}
}
}
resource "aws_s3_bucket" "destination" {
provider = "aws.euwest"
bucket = "tf-test-bucket-destination-%d"
region = "eu-west-1"
versioning {
enabled = true
}
}
`, randInt, randInt, randInt)
}

func testAccAWSS3BucketConfigReplicationWithSseKmsEncryptedObjectsAndAccessControlTranslation(randInt int) string {
return fmt.Sprintf(testAccAWSS3BucketConfigReplicationBasic+`
data "aws_caller_identity" "current" {}
Expand Down Expand Up @@ -2270,14 +2372,14 @@ resource "aws_s3_bucket" "bucket" {
status = "Enabled"
destination {
account = "${data.aws_caller_identity.current.account_id}"
bucket = "${aws_s3_bucket.destination.arn}"
storage_class = "STANDARD"
replica_kms_key_id = "${aws_kms_key.replica.arn}"
account = "${data.aws_caller_identity.current.account_id}"
bucket = "${aws_s3_bucket.destination.arn}"
storage_class = "STANDARD"
replica_kms_key_id = "${aws_kms_key.replica.arn}"
access_control_translation {
owner = "Destination"
}
access_control_translation {
owner = "Destination"
}
}
source_selection_criteria {
Expand Down

0 comments on commit c7d1d5a

Please sign in to comment.