Skip to content

Commit

Permalink
Merge pull request #23737 from hashicorp/backport-bucket-replication-…
Browse files Browse the repository at this point in the history 
…configuration-type-fix

r/s3_bucket_replication_configuration: backport  update rule to a TypeList and mark `rule.id` as Computed and `rule.prefix` as Deprecated
  • Loading branch information
@anGie44
anGie44 authored Mar 17, 2022
2 parents a31ce4d + 6244cce commit f551f5b
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 16 deletions.
11 changes: 11 additions & 0 deletions .changelog/23737.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
```release-note:bug
resource/aws_s3_bucket_replication_configuration: Set `rule.id` as Computed to prevent drift when the value is not configured
```

```release-note:bug
resource/aws_s3_bucket_replication_configuration: Change `rule` configuration block to list instead of set
```

```release-note:note:
resource/aws_s3_bucket_replication_configuration: The `rule.prefix` parameter has been deprecated. Use the `rule.filter` parameter instead.
```
11 changes: 6 additions & 5 deletions internal/service/s3/bucket_replication_configuration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,8 @@ func ResourceBucketReplicationConfiguration() *schema.Resource {
Sensitive: true,
},
"rule": {
Type: schema.TypeSet,
Type: schema.TypeList,
Required: true,
Set: rulesHash,
MaxItems: 1000,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
Expand Down Expand Up @@ -241,12 +240,14 @@ func ResourceBucketReplicationConfiguration() *schema.Resource {
"id": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ValidateFunc: validation.StringLenBetween(0, 255),
},
"prefix": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringLenBetween(0, 1024),
Deprecated: "Use filter instead",
},
"priority": {
Type: schema.TypeInt,
Expand Down Expand Up @@ -308,7 +309,7 @@ func resourceBucketReplicationConfigurationCreate(d *schema.ResourceData, meta i

rc := &s3.ReplicationConfiguration{
Role: aws.String(d.Get("role").(string)),
Rules: ExpandReplicationRules(d.Get("rule").(*schema.Set).List()),
Rules: ExpandReplicationRules(d.Get("rule").([]interface{})),
}

input := &s3.PutBucketReplicationInput{
Expand Down Expand Up @@ -376,7 +377,7 @@ func resourceBucketReplicationConfigurationRead(d *schema.ResourceData, meta int

d.Set("bucket", d.Id())
d.Set("role", r.Role)
if err := d.Set("rule", schema.NewSet(rulesHash, FlattenReplicationRules(r.Rules))); err != nil {
if err := d.Set("rule", FlattenReplicationRules(r.Rules)); err != nil {
return fmt.Errorf("error setting rule: %w", err)
}

Expand All @@ -388,7 +389,7 @@ func resourceBucketReplicationConfigurationUpdate(d *schema.ResourceData, meta i

rc := &s3.ReplicationConfiguration{
Role: aws.String(d.Get("role").(string)),
Rules: ExpandReplicationRules(d.Get("rule").(*schema.Set).List()),
Rules: ExpandReplicationRules(d.Get("rule").([]interface{})),
}

input := &s3.PutBucketReplicationInput{
Expand Down
133 changes: 130 additions & 3 deletions internal/service/s3/bucket_replication_configuration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,46 @@ func TestAccS3BucketReplicationConfiguration_replicaModifications(t *testing.T)
})
}

// TestAccS3BucketReplicationConfiguration_withoutId ensures a configuration with a Computed
// rule.id does not result in a non-empty plan
// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/23690
func TestAccS3BucketReplicationConfiguration_withoutId(t *testing.T) {
resourceName := "aws_s3_bucket_replication_configuration.test"
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
dstBucketResourceName := "aws_s3_bucket.destination"
iamRoleResourceName := "aws_iam_role.test"

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

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(t) },
ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID),
ProviderFactories: acctest.FactoriesAlternate(&providers),
CheckDestroy: acctest.CheckWithProviders(testAccCheckBucketReplicationConfigurationDestroy, &providers),
Steps: []resource.TestStep{
{
Config: testAccBucketReplicationConfiguration_prefix_withoutIdConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckBucketReplicationConfigurationExists(resourceName),
resource.TestCheckResourceAttrPair(resourceName, "role", iamRoleResourceName, "arn"),
resource.TestCheckResourceAttr(resourceName, "rule.#", "1"),
resource.TestCheckResourceAttrSet(resourceName, "rule.0.id"),
resource.TestCheckResourceAttr(resourceName, "rule.0.prefix", "foo"),
resource.TestCheckResourceAttr(resourceName, "rule.0.status", s3.ReplicationRuleStatusEnabled),
resource.TestCheckResourceAttr(resourceName, "rule.0.destination.#", "1"),
resource.TestCheckResourceAttrPair(resourceName, "rule.0.destination.0.bucket", dstBucketResourceName, "arn"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

// StorageClass issue: https://github.com/hashicorp/terraform/issues/10909
func TestAccS3BucketReplicationConfiguration_withoutStorageClass(t *testing.T) {
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
Expand Down Expand Up @@ -835,9 +875,12 @@ func TestAccS3BucketReplicationConfiguration_filter_emptyPrefix(t *testing.T) {
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ResourceName: resourceName,
ImportState: true,
// The "rule" parameter as a TypeList will have a nil value
// if prefix was specified as an empty string, which was used as workaround
// when the parameter was a TypeSet
ImportStateVerifyIgnore: []string{"rule.0.filter.0.prefix"},
},
},
})
Expand Down Expand Up @@ -959,6 +1002,47 @@ func TestAccS3BucketReplicationConfiguration_filter_andOperator(t *testing.T) {
})
}

// TestAccS3BucketReplicationConfiguration_filter_withoutId ensures a configuration with a Computed
// rule.id does not result in a non-empty plan.
// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/23690
func TestAccS3BucketReplicationConfiguration_filter_withoutId(t *testing.T) {
resourceName := "aws_s3_bucket_replication_configuration.test"
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
dstBucketResourceName := "aws_s3_bucket.destination"
iamRoleResourceName := "aws_iam_role.test"

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

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(t) },
ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID),
ProviderFactories: acctest.FactoriesAlternate(&providers),
CheckDestroy: acctest.CheckWithProviders(testAccCheckBucketReplicationConfigurationDestroy, &providers),
Steps: []resource.TestStep{
{
Config: testAccBucketReplicationConfiguration_filter_withoutIdConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckBucketReplicationConfigurationExists(resourceName),
resource.TestCheckResourceAttrPair(resourceName, "role", iamRoleResourceName, "arn"),
resource.TestCheckResourceAttr(resourceName, "rule.#", "1"),
resource.TestCheckResourceAttrSet(resourceName, "rule.0.id"),
resource.TestCheckResourceAttr(resourceName, "rule.0.filter.#", "1"),
resource.TestCheckResourceAttr(resourceName, "rule.0.status", s3.ReplicationRuleStatusEnabled),
resource.TestCheckResourceAttr(resourceName, "rule.0.delete_marker_replication.#", "1"),
resource.TestCheckResourceAttr(resourceName, "rule.0.destination.#", "1"),
resource.TestCheckResourceAttrPair(resourceName, "rule.0.destination.0.bucket", dstBucketResourceName, "arn"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/21961
func TestAccS3BucketReplicationConfiguration_withoutPrefix(t *testing.T) {
resourceName := "aws_s3_bucket_replication_configuration.test"
Expand Down Expand Up @@ -1125,6 +1209,49 @@ resource "aws_s3_bucket_replication_configuration" "test" {
}`, storageClass)
}

func testAccBucketReplicationConfiguration_prefix_withoutIdConfig(rName string) string {
return acctest.ConfigCompose(
testAccBucketReplicationConfigurationBase(rName), `
resource "aws_s3_bucket_replication_configuration" "test" {
depends_on = [
aws_s3_bucket_versioning.source,
aws_s3_bucket_versioning.destination
]
bucket = aws_s3_bucket.source.id
role = aws_iam_role.test.arn
rule {
prefix = "foo"
status = "Enabled"
destination {
bucket = aws_s3_bucket.destination.arn
}
}
}`)
}

func testAccBucketReplicationConfiguration_filter_withoutIdConfig(rName string) string {
return acctest.ConfigCompose(
testAccBucketReplicationConfigurationBase(rName), `
resource "aws_s3_bucket_replication_configuration" "test" {
depends_on = [
aws_s3_bucket_versioning.source,
aws_s3_bucket_versioning.destination
]
bucket = aws_s3_bucket.source.id
role = aws_iam_role.test.arn
rule {
filter {}
status = "Enabled"
delete_marker_replication {
status = "Disabled"
}
destination {
bucket = aws_s3_bucket.destination.arn
}
}
}`)
}

func testAccBucketReplicationConfigurationRTC(rName string) string {
return acctest.ConfigCompose(
testAccBucketReplicationConfigurationBase(rName),
Expand Down
28 changes: 20 additions & 8 deletions website/docs/r/s3_bucket_replication_configuration.html.markdown
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,12 @@ resource "aws_s3_bucket_replication_configuration" "replication" {
bucket = aws_s3_bucket.source.id
rule {
id = "foobar"
prefix = "foo"
id = "foobar"
filter {
prefix = "foo"
}
status = "Enabled"
destination {
Expand Down Expand Up @@ -203,8 +207,12 @@ resource "aws_s3_bucket_replication_configuration" "east_to_west" {
bucket = aws_s3_bucket.east.id
rule {
id = "foobar"
prefix = "foo"
id = "foobar"
filter {
prefix = "foo"
}
status = "Enabled"
destination {
Expand All @@ -224,8 +232,12 @@ resource "aws_s3_bucket_replication_configuration" "west_to_east" {
bucket = aws_s3_bucket.west.id
rule {
id = "foobar"
prefix = "foo"
id = "foobar"
filter {
prefix = "foo"
}
status = "Enabled"
destination {
Expand Down Expand Up @@ -265,7 +277,7 @@ The following arguments are supported:

* `bucket` - (Required) The name of the source S3 bucket you want Amazon S3 to monitor.
* `role` - (Required) The ARN of the IAM role for Amazon S3 to assume when replicating the objects.
* `rule` - (Required) Set of configuration blocks describing the rules managing the replication [documented below](#rule).
* `rule` - (Required) List of configuration blocks describing the rules managing the replication [documented below](#rule).
* `token` - (Optional) A token to allow replication to be enabled on an Object Lock-enabled bucket. You must contact AWS support for the bucket's "Object Lock token".
For more details, see [Using S3 Object Lock with replication](https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-lock-managing.html#object-lock-managing-replication).

Expand All @@ -282,7 +294,7 @@ The `rule` configuration block supports the following arguments:
* `existing_object_replication` - (Optional) Replicate existing objects in the source bucket according to the rule configurations [documented below](#existing_object_replication).
* `filter` - (Optional, Conflicts with `prefix`) Filter that identifies subset of objects to which the replication rule applies [documented below](#filter). If not specified, the `rule` will default to using `prefix`.
* `id` - (Optional) Unique identifier for the rule. Must be less than or equal to 255 characters in length.
* `prefix` - (Optional, Conflicts with `filter`) Object key name prefix identifying one or more objects to which the rule applies. Must be less than or equal to 1024 characters in length. Defaults to an empty string (`""`) if `filter` is not specified.
* `prefix` - (Optional, Conflicts with `filter`, **Deprecated**) Object key name prefix identifying one or more objects to which the rule applies. Must be less than or equal to 1024 characters in length. Defaults to an empty string (`""`) if `filter` is not specified.
* `priority` - (Optional) The priority associated with the rule. Priority should only be set if `filter` is configured. If not provided, defaults to `0`. Priority must be unique between multiple rules.
* `source_selection_criteria` - (Optional) Specifies special object selection criteria [documented below](#source_selection_criteria).
* `status` - (Required) The status of the rule. Either `"Enabled"` or `"Disabled"`. The rule is ignored if status is not "Enabled".
Expand Down

0 comments on commit f551f5b

Please sign in to comment.