r/s3_bucket_versioning: support disabled versioning i.e. unversioned buckets

.changelog/23723.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_s3_bucket_versioning: Add missing support for `Disabled` bucket versioning
+```

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/v2/awsv1shim/v2/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}

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,155 @@ func TestAccS3BucketVersioning_MFADelete(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
 
@@ -156,7 +306,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)
 		}
 	}

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 {

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
 	}
 }

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,