From 35cb45e8ebdc20afd3ed34c009369aaba8250dac Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Tue, 8 Mar 2022 21:04:43 -0500 Subject: [PATCH 1/2] r/s3_bucket_replication_configuration: correctly populate API requests with empty rule.filter and empty rule.filter.prefix --- .../bucket_replication_configuration_test.go | 143 ++++++++++++++++++ internal/service/s3/flex.go | 31 ++-- ...et_replication_configuration.html.markdown | 7 +- 3 files changed, 168 insertions(+), 13 deletions(-) diff --git a/internal/service/s3/bucket_replication_configuration_test.go b/internal/service/s3/bucket_replication_configuration_test.go index e08d4da203d7..f9fde4012efe 100644 --- a/internal/service/s3/bucket_replication_configuration_test.go +++ b/internal/service/s3/bucket_replication_configuration_test.go @@ -758,6 +758,91 @@ func TestAccS3BucketReplicationConfiguration_existingObjectReplication(t *testin }) } +// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/23487 +func TestAccS3BucketReplicationConfiguration_filter_emptyConfigurationBlock(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_emptyConfigurationBlock(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketReplicationConfigurationExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "role", iamRoleResourceName, "arn"), + resource.TestCheckResourceAttr(resourceName, "rule.#", "1"), + resource.TestCheckTypeSetElemNestedAttrs(resourceName, "rule.*", map[string]string{ + "id": "foobar", + "delete_marker_replication.#": "1", + "delete_marker_replication.0.status": s3.DeleteMarkerReplicationStatusDisabled, + "filter.#": "1", + "status": s3.ReplicationRuleStatusEnabled, + "destination.#": "1", + }), + resource.TestCheckTypeSetElemAttrPair(resourceName, "rule.*.destination.0.bucket", dstBucketResourceName, "arn"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/23487 +func TestAccS3BucketReplicationConfiguration_filter_emptyPrefix(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_emptyPrefix(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketReplicationConfigurationExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "role", iamRoleResourceName, "arn"), + resource.TestCheckResourceAttr(resourceName, "rule.#", "1"), + resource.TestCheckTypeSetElemNestedAttrs(resourceName, "rule.*", map[string]string{ + "id": "foobar", + "delete_marker_replication.#": "1", + "delete_marker_replication.0.status": s3.DeleteMarkerReplicationStatusDisabled, + "filter.#": "1", + "filter.0.prefix": "", + "status": s3.ReplicationRuleStatusEnabled, + "destination.#": "1", + }), + resource.TestCheckTypeSetElemAttrPair(resourceName, "rule.*.destination.0.bucket", dstBucketResourceName, "arn"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + func TestAccS3BucketReplicationConfiguration_filter_tagFilter(t *testing.T) { resourceName := "aws_s3_bucket_replication_configuration.test" rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -1707,6 +1792,64 @@ resource "aws_s3_bucket_replication_configuration" "test" { `, rName, rNameDestination) } +func testAccBucketReplicationConfiguration_filter_emptyConfigurationBlock(rName string) string { + return acctest.ConfigCompose( + testAccBucketReplicationConfigurationBase(rName), + ` +resource "aws_s3_bucket_replication_configuration" "test" { + depends_on = [aws_s3_bucket_versioning.source] + + bucket = aws_s3_bucket.source.id + role = aws_iam_role.test.arn + + rule { + id = "foobar" + + delete_marker_replication { + status = "Disabled" + } + + filter {} + + status = "Enabled" + + destination { + bucket = aws_s3_bucket.destination.arn + } + } +}`) +} + +func testAccBucketReplicationConfiguration_filter_emptyPrefix(rName string) string { + return acctest.ConfigCompose( + testAccBucketReplicationConfigurationBase(rName), ` +resource "aws_s3_bucket_replication_configuration" "test" { + depends_on = [aws_s3_bucket_versioning.source] + + bucket = aws_s3_bucket.source.id + role = aws_iam_role.test.arn + + rule { + id = "foobar" + + delete_marker_replication { + status = "Disabled" + } + + filter { + prefix = "" + } + + status = "Enabled" + + destination { + bucket = aws_s3_bucket.destination.arn + } + } +}`, + ) +} + func testAccBucketReplicationConfiguration_filter_tag(rName, key, value string) string { return acctest.ConfigCompose( testAccBucketReplicationConfigurationBase(rName), diff --git a/internal/service/s3/flex.go b/internal/service/s3/flex.go index 4731f4688ab8..9bb59b97cdfe 100644 --- a/internal/service/s3/flex.go +++ b/internal/service/s3/flex.go @@ -135,26 +135,25 @@ func ExpandExistingObjectReplication(l []interface{}) *s3.ExistingObjectReplicat } func ExpandFilter(l []interface{}) *s3.ReplicationRuleFilter { - if len(l) == 0 || l[0] == nil { + if len(l) == 0 { return nil } - tfMap, ok := l[0].(map[string]interface{}) + result := &s3.ReplicationRuleFilter{} - if !ok { - return nil + // Support the empty filter block in terraform i.e. 'filter {}', + // which is also supported by the API even though the docs note that + // one of Prefix, Tag, or And is required. + if l[0] == nil { + return result } - result := &s3.ReplicationRuleFilter{} + tfMap := l[0].(map[string]interface{}) if v, ok := tfMap["and"].([]interface{}); ok && len(v) > 0 && v[0] != nil { result.And = ExpandReplicationRuleAndOperator(v) } - if v, ok := tfMap["prefix"].(string); ok && v != "" { - result.Prefix = aws.String(v) - } - if v, ok := tfMap["tag"].([]interface{}); ok && len(v) > 0 && v[0] != nil { tags := Tags(tftags.New(v[0]).IgnoreAWS()) if len(tags) > 0 { @@ -162,6 +161,15 @@ func ExpandFilter(l []interface{}) *s3.ReplicationRuleFilter { } } + // Per AWS S3 API, "A Filter must have exactly one of Prefix, Tag, or And specified"; + // Specifying more than one of the listed parameters results in a MalformedXML error. + // If a filter is specified as filter { prefix = "" } in Terraform, we should send the prefix value + // in the API request even if it is an empty value, else Terraform will report non-empty plans. + // Reference: https://github.com/hashicorp/terraform-provider-aws/issues/23487 + if v, ok := tfMap["prefix"].(string); ok && result.And == nil && result.Tag == nil { + result.Prefix = aws.String(v) + } + return result } @@ -606,7 +614,10 @@ func ExpandRules(l []interface{}) []*s3.ReplicationRule { rule.Status = aws.String(v) } - if v, ok := tfMap["filter"].([]interface{}); ok && len(v) > 0 && v[0] != nil { + // Support the empty filter block in terraform i.e. 'filter {}', + // which implies the replication rule does not require a specific filter, + // by expanding the "filter" array even if the first element is nil. + if v, ok := tfMap["filter"].([]interface{}); ok && len(v) > 0 { // XML schema V2 rule.Filter = ExpandFilter(v) rule.Priority = aws.Int64(int64(tfMap["priority"].(int))) diff --git a/website/docs/r/s3_bucket_replication_configuration.html.markdown b/website/docs/r/s3_bucket_replication_configuration.html.markdown index d15e7d27a390..c7065f6afdc9 100644 --- a/website/docs/r/s3_bucket_replication_configuration.html.markdown +++ b/website/docs/r/s3_bucket_replication_configuration.html.markdown @@ -229,9 +229,9 @@ The `rule` configuration block supports the following arguments: * `delete_marker_replication` - (Optional) Whether delete markers are replicated. This argument is only valid with V2 replication configurations (i.e., when `filter` is used)[documented below](#delete_marker_replication). * `destination` - (Required) Specifies the destination for the rule [documented below](#destination). * `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). +* `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. +* `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. * `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". @@ -346,7 +346,8 @@ The `existing_object_replication` configuration block supports the following arg ### filter -~> **NOTE:** With the `filter` argument, you must specify exactly one of `prefix`, `tag`, or `and`. Replication configuration V1 supports filtering based on only the `prefix` attribute. For backwards compatibility, Amazon S3 continues to support the V1 configuration. +~> **NOTE:** The `filter` argument must be specified as either an empty configuration block (`filter {}`) to imply the rule requires no filter or with exactly one of `prefix`, `tag`, or `and`. +Replication configuration V1 supports filtering based on only the `prefix` attribute. For backwards compatibility, Amazon S3 continues to support the V1 configuration. The `filter` configuration block supports the following arguments: From 0c80e7ee9174866c7ddfd45deb609c7024461bdf Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Tue, 8 Mar 2022 21:26:58 -0500 Subject: [PATCH 2/2] Update CHANGELOG for #23586 --- .changelog/23586.txt | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changelog/23586.txt diff --git a/.changelog/23586.txt b/.changelog/23586.txt new file mode 100644 index 000000000000..ed6da2212775 --- /dev/null +++ b/.changelog/23586.txt @@ -0,0 +1,7 @@ +```release-note:bug +resource/aws_s3_bucket_replication_configuration: Prevent inconsistent final plan when `rule.filter.prefix` is an empty string +``` + +```release-note:bug +resource/aws_s3_bucket_replication_configuration: Correctly configure empty `rule.filter` configuration block in API requests +``` \ No newline at end of file