-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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.
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
Adds the S3 Replication AccessControlTranslation and Account arguments #3577
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -377,6 +377,10 @@ func resourceAwsS3Bucket() *schema.Resource { | |
Set: destinationHash, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"account": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"bucket": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
|
@@ -396,6 +400,20 @@ func resourceAwsS3Bucket() *schema.Resource { | |
Type: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"access_control_translation": { | ||
Type: schema.TypeSet, | ||
Optional: true, | ||
MinItems: 1, | ||
MaxItems: 1, | ||
Elem: &schema.Resource{ | ||
bflad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Schema: map[string]*schema.Schema{ | ||
"owner": { | ||
bflad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Type: schema.TypeString, | ||
Required: true, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
|
@@ -1755,6 +1773,18 @@ func resourceAwsS3BucketReplicationConfigurationUpdate(s3conn *s3.S3, d *schema. | |
ReplicaKmsKeyID: aws.String(replicaKmsKeyId.(string)), | ||
} | ||
} | ||
|
||
if account, ok := bd["account"]; ok && account != "" { | ||
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{}) | ||
ruleAclTranslation := &s3.AccessControlTranslation{} | ||
ruleAclTranslation.Owner = aws.String(aclTranslationValues["owner"].(string)) | ||
ruleDestination.AccessControlTranslation = ruleAclTranslation | ||
} | ||
|
||
} | ||
rcRule.Destination = ruleDestination | ||
|
||
|
@@ -2000,6 +2030,14 @@ func flattenAwsS3BucketReplicationConfiguration(r *s3.ReplicationConfiguration) | |
rd["replica_kms_key_id"] = *v.Destination.EncryptionConfiguration.ReplicaKmsKeyID | ||
} | ||
} | ||
if v.Destination.Account != nil { | ||
rd["account"] = *v.Destination.Account | ||
} | ||
if v.Destination.AccessControlTranslation != nil { | ||
rdt := make(map[string]interface{}) | ||
rdt["owner"] = *v.Destination.AccessControlTranslation.Owner | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To prevent potential panics, we prefer to use the AWS Go SDK provided methods for dereferencing, e.g. |
||
rd["access_control_translation"] = schema.NewSet(accessControlTranslationHash, []interface{}{rdt}) | ||
} | ||
t["destination"] = schema.NewSet(destinationHash, []interface{}{rd}) | ||
} | ||
|
||
|
@@ -2186,6 +2224,26 @@ func destinationHash(v interface{}) int { | |
if v, ok := m["replica_kms_key_id"]; ok { | ||
buf.WriteString(fmt.Sprintf("%s-", v.(string))) | ||
} | ||
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]))) | ||
} | ||
return hashcode.String(buf.String()) | ||
} | ||
|
||
func accessControlTranslationHash(v interface{}) int { | ||
// v is nil if empty access_control_translation is given. | ||
if v == nil { | ||
return 0 | ||
} | ||
var buf bytes.Buffer | ||
m := v.(map[string]interface{}) | ||
|
||
if v, ok := m["owner"]; ok { | ||
buf.WriteString(fmt.Sprintf("%s-", v.(string))) | ||
} | ||
return hashcode.String(buf.String()) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -731,6 +731,42 @@ func TestAccAWSS3Bucket_Cors_EmptyOrigin(t *testing.T) { | |
), | ||
), | ||
}, | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We generally prefer to create new, separate acceptance test functions for new attributes 👍 |
||
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}"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how this was passing previously, but I needed to add the HIL translation for this parameter in 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)
} We should probably move this logic to its own function so it can be reused in other testing. 😅 |
||
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), | ||
}, | ||
}, | ||
}, | ||
}, | ||
), | ||
), | ||
}, | ||
}, | ||
}) | ||
} | ||
|
@@ -2207,6 +2243,64 @@ resource "aws_s3_bucket" "destination" { | |
`, randInt, randInt, randInt) | ||
} | ||
|
||
func testAccAWSS3BucketConfigReplicationWithSseKmsEncryptedObjectsAndAccessControlTranslation(randInt int) string { | ||
return fmt.Sprintf(testAccAWSS3BucketConfigReplicationBasic+` | ||
data "aws_caller_identity" "current" {} | ||
|
||
resource "aws_kms_key" "replica" { | ||
provider = "aws.euwest" | ||
description = "TF Acceptance Test S3 repl KMS key" | ||
deletion_window_in_days = 7 | ||
} | ||
|
||
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" | ||
replica_kms_key_id = "${aws_kms_key.replica.arn}" | ||
|
||
access_control_translation { | ||
owner = "Destination" | ||
} | ||
} | ||
|
||
source_selection_criteria { | ||
sse_kms_encrypted_objects { | ||
enabled = true | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
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 testAccAWSS3BucketConfigReplicationWithoutStorageClass(randInt int) string { | ||
return fmt.Sprintf(testAccAWSS3BucketConfigReplicationBasic+` | ||
resource "aws_s3_bucket" "bucket" { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest renaming this to
account_id
to match similar places in the code base.Also can add
ValidateFunc: validateAwsAccountId,
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both these seem reasonable and will be updated on merge.