diff --git a/.changelog/23731.txt b/.changelog/23731.txt new file mode 100644 index 000000000000..ead3ad6c2447 --- /dev/null +++ b/.changelog/23731.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +resource/aws_s3_bucket_versioning: Add missing support for `Disabled` bucket versioning +``` \ No newline at end of file diff --git a/internal/service/s3/bucket_versioning.go b/internal/service/s3/bucket_versioning.go index bbd926aa48cc..e5af62897676 100644 --- a/internal/service/s3/bucket_versioning.go +++ b/internal/service/s3/bucket_versioning.go @@ -9,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go/service/s3" "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/hashicorp/terraform-provider-aws/internal/conns" @@ -58,12 +59,32 @@ func ResourceBucketVersioning() *schema.Resource { "status": { Type: schema.TypeString, Required: true, - ValidateFunc: validation.StringInSlice(s3.BucketVersioningStatus_Values(), false), + ValidateFunc: validation.StringInSlice(BucketVersioningStatus_Values(), false), }, }, }, }, }, + + CustomizeDiff: customdiff.Sequence( + func(_ context.Context, diff *schema.ResourceDiff, v interface{}) error { + // This CustomizeDiff acts as a plan-time validation to prevent MalformedXML errors + // when updating bucket versioning to "Disabled" on existing resources + // as it's not supported by the AWS S3 API. + if diff.Id() == "" || !diff.HasChange("versioning_configuration.0.status") { + return nil + } + + oldStatusRaw, newStatusRaw := diff.GetChange("versioning_configuration.0.status") + oldStatus, newStatus := oldStatusRaw.(string), newStatusRaw.(string) + + if newStatus == BucketVersioningStatusDisabled && (oldStatus == s3.BucketVersioningStatusEnabled || oldStatus == s3.BucketVersioningStatusSuspended) { + return fmt.Errorf("versioning_configuration.status cannot be updated from '%s' to '%s'", oldStatus, newStatus) + } + + return nil + }, + ), } } @@ -73,25 +94,35 @@ func resourceBucketVersioningCreate(ctx context.Context, d *schema.ResourceData, bucket := d.Get("bucket").(string) expectedBucketOwner := d.Get("expected_bucket_owner").(string) - input := &s3.PutBucketVersioningInput{ - Bucket: aws.String(bucket), - VersioningConfiguration: expandBucketVersioningConfiguration(d.Get("versioning_configuration").([]interface{})), - } - - if expectedBucketOwner != "" { - input.ExpectedBucketOwner = aws.String(expectedBucketOwner) - } - - if v, ok := d.GetOk("mfa"); ok { - input.MFA = aws.String(v.(string)) - } - - _, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) { - return conn.PutBucketVersioningWithContext(ctx, input) - }) - - if err != nil { - return diag.FromErr(fmt.Errorf("error creating S3 bucket versioning for %s: %w", bucket, err)) + versioningConfiguration := expandBucketVersioningConfiguration(d.Get("versioning_configuration").([]interface{})) + + // To support migration from v3 to v4 of the provider, we need to support + // versioning resources that represent unversioned S3 buckets as was previously + // supported within the aws_s3_bucket resource of the 3.x version of the provider. + // Thus, we essentially bring existing bucket versioning into adoption. + if aws.StringValue(versioningConfiguration.Status) != BucketVersioningStatusDisabled { + input := &s3.PutBucketVersioningInput{ + Bucket: aws.String(bucket), + VersioningConfiguration: versioningConfiguration, + } + + if expectedBucketOwner != "" { + input.ExpectedBucketOwner = aws.String(expectedBucketOwner) + } + + if v, ok := d.GetOk("mfa"); ok { + input.MFA = aws.String(v.(string)) + } + + _, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) { + return conn.PutBucketVersioningWithContext(ctx, input) + }) + + if err != nil { + return diag.FromErr(fmt.Errorf("error creating S3 bucket versioning for %s: %w", bucket, err)) + } + } else { + log.Printf("[DEBUG] Creating S3 bucket versioning for unversioned bucket: %s", bucket) } d.SetId(CreateResourceID(bucket, expectedBucketOwner)) @@ -166,6 +197,11 @@ func resourceBucketVersioningUpdate(ctx context.Context, d *schema.ResourceData, func resourceBucketVersioningDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { conn := meta.(*conns.AWSClient).S3Conn + if v := expandBucketVersioningConfiguration(d.Get("versioning_configuration").([]interface{})); v != nil && aws.StringValue(v.Status) == BucketVersioningStatusDisabled { + log.Printf("[DEBUG] Removing S3 bucket versioning for unversioned bucket (%s) from state", d.Id()) + return nil + } + bucket, expectedBucketOwner, err := ParseResourceID(d.Id()) if err != nil { return diag.FromErr(err) @@ -238,6 +274,9 @@ func flattenBucketVersioningConfiguration(config *s3.GetBucketVersioningOutput) if config.Status != nil { m["status"] = aws.StringValue(config.Status) + } else { + // Bucket Versioning by default is disabled but not set in the config struct's Status field + m["status"] = BucketVersioningStatusDisabled } return []interface{}{m} diff --git a/internal/service/s3/bucket_versioning_test.go b/internal/service/s3/bucket_versioning_test.go index afd6905b8e02..d418413c7c76 100644 --- a/internal/service/s3/bucket_versioning_test.go +++ b/internal/service/s3/bucket_versioning_test.go @@ -2,6 +2,7 @@ package s3_test import ( "fmt" + "regexp" "testing" "github.com/aws/aws-sdk-go/aws" @@ -134,6 +135,68 @@ func TestAccS3BucketVersioning_MFADelete(t *testing.T) { }) } +func TestAccS3BucketVersioning_migrate_versioningDisabledNoChange(t *testing.T) { + bucketName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + bucketResourceName := "aws_s3_bucket.bucket" + resourceName := "aws_s3_bucket_versioning.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketWithVersioningConfig(bucketName, false), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(bucketResourceName), + resource.TestCheckResourceAttr(bucketResourceName, "versioning.#", "1"), + resource.TestCheckResourceAttr(bucketResourceName, "versioning.0.enabled", "false"), + ), + }, + { + Config: testAccBucketVersioning_Migrate_VersioningEnabledConfig(bucketName, tfs3.BucketVersioningStatusDisabled), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", tfs3.BucketVersioningStatusDisabled), + ), + }, + }, + }) +} + +func TestAccS3BucketVersioning_migrate_versioningDisabledWithChange(t *testing.T) { + bucketName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + bucketResourceName := "aws_s3_bucket.bucket" + resourceName := "aws_s3_bucket_versioning.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketWithVersioningConfig(bucketName, false), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(bucketResourceName), + resource.TestCheckResourceAttr(bucketResourceName, "versioning.#", "1"), + resource.TestCheckResourceAttr(bucketResourceName, "versioning.0.enabled", "false"), + ), + }, + { + Config: testAccBucketVersioning_Migrate_VersioningEnabledConfig(bucketName, s3.BucketVersioningStatusEnabled), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", s3.BucketVersioningStatusEnabled), + ), + }, + }, + }) +} + func TestAccS3BucketVersioning_migrate_versioningEnabledNoChange(t *testing.T) { bucketName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) bucketResourceName := "aws_s3_bucket.bucket" @@ -229,6 +292,155 @@ func TestAccS3BucketVersioning_migrate_mfaDeleteNoChange(t *testing.T) { }) } +func TestAccS3BucketVersioning_Status_disabled(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket_versioning.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketVersioningDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketVersioningBasicConfig(rName, tfs3.BucketVersioningStatusDisabled), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", tfs3.BucketVersioningStatusDisabled), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccS3BucketVersioning_Status_disabledToEnabled(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket_versioning.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketVersioningDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketVersioningBasicConfig(rName, tfs3.BucketVersioningStatusDisabled), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", tfs3.BucketVersioningStatusDisabled), + ), + }, + { + Config: testAccBucketVersioningBasicConfig(rName, s3.BucketVersioningStatusEnabled), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", s3.BucketVersioningStatusEnabled), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccS3BucketVersioning_Status_disabledToSuspended(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket_versioning.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketVersioningDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketVersioningBasicConfig(rName, tfs3.BucketVersioningStatusDisabled), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", tfs3.BucketVersioningStatusDisabled), + ), + }, + { + Config: testAccBucketVersioningBasicConfig(rName, s3.BucketVersioningStatusSuspended), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", s3.BucketVersioningStatusSuspended), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccS3BucketVersioning_Status_enabledToDisabled(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket_versioning.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketVersioningDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketVersioningBasicConfig(rName, s3.BucketVersioningStatusEnabled), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", s3.BucketVersioningStatusEnabled), + ), + }, + { + Config: testAccBucketVersioningBasicConfig(rName, tfs3.BucketVersioningStatusDisabled), + ExpectError: regexp.MustCompile(`versioning_configuration.status cannot be updated from 'Enabled' to 'Disabled'`), + }, + }, + }) +} + +func TestAccS3BucketVersioning_Status_suspendedToDisabled(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket_versioning.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketVersioningDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketVersioningBasicConfig(rName, s3.BucketVersioningStatusSuspended), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketVersioningExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"), + resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", s3.BucketVersioningStatusSuspended), + ), + }, + { + Config: testAccBucketVersioningBasicConfig(rName, tfs3.BucketVersioningStatusDisabled), + ExpectError: regexp.MustCompile(`versioning_configuration.status cannot be updated from 'Suspended' to 'Disabled'`), + }, + }, + }) +} + func testAccCheckBucketVersioningDestroy(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).S3Conn @@ -251,7 +463,7 @@ func testAccCheckBucketVersioningDestroy(s *terraform.State) error { return fmt.Errorf("error getting S3 bucket versioning (%s): %w", rs.Primary.ID, err) } - if output != nil && aws.StringValue(output.Status) != s3.BucketVersioningStatusSuspended { + if output != nil && aws.StringValue(output.Status) == s3.ReplicationRuleStatusEnabled { return fmt.Errorf("S3 bucket versioning (%s) still exists", rs.Primary.ID) } } diff --git a/internal/service/s3/enum.go b/internal/service/s3/enum.go index 4efe38c2d2f2..c3e52454f2c5 100644 --- a/internal/service/s3/enum.go +++ b/internal/service/s3/enum.go @@ -11,6 +11,8 @@ const ( BucketCannedACLExecRead = "aws-exec-read" BucketCannedACLLogDeliveryWrite = "log-delivery-write" + BucketVersioningStatusDisabled = "Disabled" + LifecycleRuleStatusEnabled = "Enabled" LifecycleRuleStatusDisabled = "Disabled" ) @@ -22,6 +24,12 @@ func BucketCannedACL_Values() []string { return result } +func BucketVersioningStatus_Values() []string { + result := s3.BucketVersioningStatus_Values() + result = appendUniqueString(result, BucketVersioningStatusDisabled) + return result +} + func appendUniqueString(slice []string, elem string) []string { for _, e := range slice { if e == elem { diff --git a/internal/service/s3/status.go b/internal/service/s3/status.go index d611e17a0be7..d7f9e1781af4 100644 --- a/internal/service/s3/status.go +++ b/internal/service/s3/status.go @@ -85,6 +85,10 @@ func bucketVersioningStatus(ctx context.Context, conn *s3.S3, bucket, expectedBu } } + if output.Status == nil { + return output, BucketVersioningStatusDisabled, nil + } + return output, aws.StringValue(output.Status), nil } } diff --git a/internal/service/s3/wait.go b/internal/service/s3/wait.go index 77e9e20e8ee7..14e177c99218 100644 --- a/internal/service/s3/wait.go +++ b/internal/service/s3/wait.go @@ -46,7 +46,7 @@ func waitForLifecycleConfigurationRulesStatus(ctx context.Context, conn *s3.S3, func waitForBucketVersioningStatus(ctx context.Context, conn *s3.S3, bucket, expectedBucketOwner string) (*s3.GetBucketVersioningOutput, error) { stateConf := &resource.StateChangeConf{ Pending: []string{""}, - Target: []string{s3.BucketVersioningStatusEnabled, s3.BucketVersioningStatusSuspended}, + Target: []string{s3.BucketVersioningStatusEnabled, s3.BucketVersioningStatusSuspended, BucketVersioningStatusDisabled}, Refresh: bucketVersioningStatus(ctx, conn, bucket, expectedBucketOwner), Timeout: bucketVersioningStableTimeout, ContinuousTargetOccurence: 3, diff --git a/website/docs/r/s3_bucket_versioning.html.markdown b/website/docs/r/s3_bucket_versioning.html.markdown index ef7ead48ccb5..ceacb5ba667c 100644 --- a/website/docs/r/s3_bucket_versioning.html.markdown +++ b/website/docs/r/s3_bucket_versioning.html.markdown @@ -9,13 +9,17 @@ description: |- # Resource: aws_s3_bucket_versioning Provides a resource for controlling versioning on an S3 bucket. -Deleting this resource will suspend versioning on the associated S3 bucket. +Deleting this resource will either suspend versioning on the associated S3 bucket or +simply remove the resource from Terraform state if the associated S3 bucket is unversioned. + For more information, see [How S3 versioning works](https://docs.aws.amazon.com/AmazonS3/latest/userguide/manage-versioning-examples.html). ~> **NOTE:** If you are enabling versioning on the bucket for the first time, AWS recommends that you wait for 15 minutes after enabling versioning before issuing write operations (PUT or DELETE) on objects in the bucket. ## Example Usage +### With Versioning Enabled + ```terraform resource "aws_s3_bucket" "example" { bucket = "example-bucket" @@ -40,6 +44,32 @@ resource "aws_s3_bucket_versioning" "versioning_example" { } ``` +### With Versioning Disabled + +```terraform +resource "aws_s3_bucket" "example" { + bucket = "example-bucket" + + lifecycle { + ignore_changes = [ + grant + ] + } +} + +resource "aws_s3_bucket_acl" "example" { + bucket = aws_s3_bucket.example.id + acl = "private" +} + +resource "aws_s3_bucket_versioning" "versioning_example" { + bucket = aws_s3_bucket.example.id + versioning_configuration { + status = "Disabled" + } +} +``` + ### Object Dependency On Versioning When you create an object whose `version_id` you need and an `aws_s3_bucket_versioning` resource in the same configuration, you are more likely to have success by ensuring the `s3_object` depends either implicitly (see below) or explicitly (i.e., using `depends_on = [aws_s3_bucket_versioning.example]`) on the `aws_s3_bucket_versioning` resource. @@ -79,9 +109,12 @@ The following arguments are supported: ### versioning_configuration +~> **Note:** While the `versioning_configuration.status` parameter supports `Disabled`, this value is only intended for _creating_ or _importing_ resources that correspond to unversioned S3 buckets. +Updating the value from `Enabled` or `Suspended` to `Disabled` will result in errors as the AWS S3 API does not support returning buckets to an unversioned state. + The `versioning_configuration` configuration block supports the following arguments: -* `status` - (Required) The versioning state of the bucket. Valid values: `Enabled` or `Suspended`. +* `status` - (Required) The versioning state of the bucket. Valid values: `Enabled`, `Suspended`, or `Disabled`. `Disabled` should only be used when creating or importing resources that correspond to unversioned S3 buckets. * `mfa_delete` - (Optional) Specifies whether MFA delete is enabled in the bucket versioning configuration. Valid values: `Enabled` or `Disabled`. ## Attributes Reference