CR updates: align with API and update overall provider design patterns

.changelog/5132.txt

@@ -0,0 +1,3 @@
+```release-note:new-resource
+aws_s3_bucket_versioning
+```

internal/provider/provider.go

@@ -1575,7 +1575,7 @@ func Provider() *schema.Provider {
 			"aws_s3_bucket_policy":                            s3.ResourceBucketPolicy(),
 			"aws_s3_bucket_public_access_block":               s3.ResourceBucketPublicAccessBlock(),
 			"aws_s3_bucket_replication_configuration":         s3.ResourceBucketReplicationConfiguration(),
-			"aws_s3_bucket_versioning_configuration":          s3.ResourceBucketVersioning(),
+			"aws_s3_bucket_versioning":                        s3.ResourceBucketVersioning(),
 			"aws_s3_object_copy":                              s3.ResourceObjectCopy(),
 
 			"aws_s3_access_point":                             s3control.ResourceAccessPoint(),

internal/service/s3/bucket_versioning.go

@@ -1,167 +1,249 @@
 package s3
 
 import (
+	"context"
 	"fmt"
-	"time"
+	"log"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/s3"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/aws-sdk-go-base/tfawserr"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"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"
 	"github.com/hashicorp/terraform-provider-aws/internal/verify"
 )
 
-// the S3 API can be very inconsistent with the status of Versioning on a bucket
-// so we require at least this many confirmations that the value is actually set
-// before we move on
-const s3BucketVersioningConfirmationsRequired = 10
-
 func ResourceBucketVersioning() *schema.Resource {
 	return &schema.Resource{
-		Create: resourceBucketVersioningCreate,
-		Read:   resourceBucketVersioningRead,
-		Update: resourceBucketVersioningUpdate,
-		Delete: resourceBucketVersioningDelete,
+		CreateContext: resourceBucketVersioningCreate,
+		ReadContext:   resourceBucketVersioningRead,
+		UpdateContext: resourceBucketVersioningUpdate,
+		DeleteContext: resourceBucketVersioningDelete,
+		Importer: &schema.ResourceImporter{
+			StateContext: schema.ImportStatePassthroughContext,
+		},
+
 		Schema: map[string]*schema.Schema{
 			"bucket": {
+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validation.StringLenBetween(1, 63),
+			},
+			"expected_bucket_owner": {
+				Type:         schema.TypeString,
+				Optional:     true,
+				ForceNew:     true,
+				ValidateFunc: verify.ValidAccountID,
+			},
+			"mfa": {
 				Type:     schema.TypeString,
-				Required: true,
-				ForceNew: true,
+				Optional: true,
 			},
-			"enabled": {
-				Type:     schema.TypeBool,
+			"versioning_configuration": {
+				Type:     schema.TypeList,
 				Required: true,
+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"mfa_delete": {
+							Type:         schema.TypeString,
+							Optional:     true,
+							ValidateFunc: validation.StringInSlice(s3.MFADelete_Values(), false),
+						},
+						"status": {
+							Type:         schema.TypeString,
+							Required:     true,
+							ValidateFunc: validation.StringInSlice(s3.BucketVersioningStatus_Values(), false),
+						},
+					},
+				},
 			},
 		},
 	}
 }
 
-func BucketVersioningStatusToBool(status *string) bool {
-	if status == nil {
-		return false
-	}
+func resourceBucketVersioningCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
+	conn := meta.(*conns.AWSClient).S3Conn
+
+	bucket := d.Get("bucket").(string)
+	expectedBucketOwner := d.Get("expected_bucket_owner").(string)
 
-	if *status == s3.BucketVersioningStatusEnabled {
-		return true
+	input := &s3.PutBucketVersioningInput{
+		Bucket:                  aws.String(bucket),
+		VersioningConfiguration: expandBucketVersioningConfiguration(d.Get("versioning_configuration").([]interface{})),
 	}
 
-	return false
-}
+	if expectedBucketOwner != "" {
+		input.ExpectedBucketOwner = aws.String(expectedBucketOwner)
+	}
 
-// updates s3 bucket versioning and waits for the change to be confirmed
-func putBucketVersioning(s3conn *s3.S3, bucket string, enabled bool) error {
-	vc := &s3.VersioningConfiguration{}
-	if enabled {
-		vc.SetStatus(s3.BucketVersioningStatusEnabled)
-	} else {
-		vc.SetStatus(s3.BucketVersioningStatusSuspended)
+	if v, ok := d.GetOk("mfa"); ok {
+		input.MFA = aws.String(v.(string))
 	}
 
-	_, err := s3conn.PutBucketVersioning(&s3.PutBucketVersioningInput{
-		Bucket:                  aws.String(bucket),
-		VersioningConfiguration: vc,
+	_, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
+		return conn.PutBucketVersioningWithContext(ctx, input)
 	})
+
 	if err != nil {
-		return err
+		return diag.FromErr(fmt.Errorf("error creating S3 bucket versioning for %s: %w", bucket, err))
 	}
 
-	// wait up to 30 seconds for the change to be reflected
-	updateConfirmations := 0
-	for i := 0; i < 120; i++ {
-		bucketVersioningStatus, err := s3conn.GetBucketVersioning(&s3.GetBucketVersioningInput{
-			Bucket: aws.String(bucket),
-		})
-		if err != nil {
-			return fmt.Errorf("Error getting versioning config for %s: %s", bucket, err)
-		}
+	d.SetId(CreateResourceID(bucket, expectedBucketOwner))
 
-		if bucketVersioningStatus != nil && BucketVersioningStatusToBool(bucketVersioningStatus.Status) == enabled {
-			updateConfirmations++
-			if updateConfirmations >= s3BucketVersioningConfirmationsRequired {
-				return nil
-			}
+	return resourceBucketVersioningRead(ctx, d, meta)
+}
+
+func resourceBucketVersioningRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
+	conn := meta.(*conns.AWSClient).S3Conn
+
+	bucket, expectedBucketOwner, err := ParseResourceID(d.Id())
+	if err != nil {
+		return diag.FromErr(err)
+	}
+
+	input := &s3.GetBucketVersioningInput{
+		Bucket: aws.String(bucket),
+	}
+
+	if expectedBucketOwner != "" {
+		input.ExpectedBucketOwner = aws.String(expectedBucketOwner)
+	}
+
+	output, err := conn.GetBucketVersioningWithContext(ctx, input)
+
+	if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
+		log.Printf("[WARN] S3 Bucket Versioning (%s) not found, removing from state", d.Id())
+		d.SetId("")
+		return nil
+	}
+
+	if err != nil {
+		return diag.FromErr(fmt.Errorf("error getting S3 bucket versioning (%s): %w", d.Id(), err))
+	}
+
+	if output == nil {
+		if d.IsNewResource() {
+			return diag.FromErr(fmt.Errorf("error getting S3 bucket versioning (%s): empty output", d.Id()))
 		}
+		log.Printf("[WARN] S3 Bucket Versioning (%s) not found, removing from state", d.Id())
+		d.SetId("")
+		return nil
+	}
 
-		time.Sleep(250 * time.Millisecond)
+	d.Set("bucket", bucket)
+	d.Set("expected_bucket_owner", expectedBucketOwner)
+	if err := d.Set("versioning_configuration", flattenBucketVersioningConfiguration(output)); err != nil {
+		return diag.FromErr(fmt.Errorf("error setting versioning_configuration: %w", err))
 	}
 
-	return fmt.Errorf("Timed out waiting for confirmation")
+	return nil
 }
 
-func resourceBucketVersioningCreate(d *schema.ResourceData, meta interface{}) error {
+func resourceBucketVersioningUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	conn := meta.(*conns.AWSClient).S3Conn
 
-	bucket := d.Get("bucket").(string)
-	versioningEnabled := d.Get("enabled").(bool)
-
-	_, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
-		return nil, putBucketVersioning(conn, bucket, versioningEnabled)
-	})
+	bucket, expectedBucketOwner, err := ParseResourceID(d.Id())
 	if err != nil {
-		return fmt.Errorf("Error putting bucket versioning for %s: %s", bucket, err)
+		return diag.FromErr(err)
 	}
 
-	d.SetId(resource.UniqueId())
+	input := &s3.PutBucketVersioningInput{
+		Bucket:                  aws.String(bucket),
+		VersioningConfiguration: expandBucketVersioningConfiguration(d.Get("versioning_configuration").([]interface{})),
+	}
 
-	return nil
+	if expectedBucketOwner != "" {
+		input.ExpectedBucketOwner = aws.String(expectedBucketOwner)
+	}
+
+	if v, ok := d.GetOk("mfa"); ok {
+		input.MFA = aws.String(v.(string))
+	}
+
+	_, err = conn.PutBucketVersioningWithContext(ctx, input)
+
+	if err != nil {
+		return diag.FromErr(fmt.Errorf("error updating S3 bucket versioning (%s): %w", d.Id(), err))
+	}
+
+	return resourceBucketVersioningRead(ctx, d, meta)
 }
 
-func resourceBucketVersioningRead(d *schema.ResourceData, meta interface{}) error {
+func resourceBucketVersioningDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	conn := meta.(*conns.AWSClient).S3Conn
 
-	bucket := d.Get("bucket").(string)
+	bucket, expectedBucketOwner, err := ParseResourceID(d.Id())
+	if err != nil {
+		return diag.FromErr(err)
+	}
 
-	vo, err := conn.GetBucketVersioning(&s3.GetBucketVersioningInput{
+	input := &s3.PutBucketVersioningInput{
 		Bucket: aws.String(bucket),
-	})
-	if err != nil {
-		return fmt.Errorf("Error getting bucket versioning for %s: %s", bucket, err)
+		VersioningConfiguration: &s3.VersioningConfiguration{
+			// Status must be provided thus to "remove" this resource,
+			// we suspend versioning
+			Status: aws.String(s3.BucketVersioningStatusSuspended),
+		},
 	}
 
-	var versioningEnabled bool
-	if vo == nil {
-		versioningEnabled = false
-	} else {
-		versioningEnabled = BucketVersioningStatusToBool(vo.Status)
+	if expectedBucketOwner != "" {
+		input.ExpectedBucketOwner = aws.String(expectedBucketOwner)
 	}
 
-	if err := d.Set("enabled", versioningEnabled); err != nil {
-		return fmt.Errorf("[WARN] Error setting versioning status from S3 (%s / %t), error: %s", bucket, versioningEnabled, err)
+	_, err = conn.PutBucketVersioningWithContext(ctx, input)
+
+	if tfawserr.ErrCodeEquals(err, s3.ErrCodeNoSuchBucket) {
+		return nil
+	}
+
+	if err != nil {
+		return diag.FromErr(fmt.Errorf("error deleting S3 bucket versioning (%s): %w", d.Id(), err))
 	}
 
 	return nil
 }
 
-func resourceBucketVersioningUpdate(d *schema.ResourceData, meta interface{}) error {
-	conn := meta.(*conns.AWSClient).S3Conn
-	bucket := d.Get("bucket").(string)
+func expandBucketVersioningConfiguration(l []interface{}) *s3.VersioningConfiguration {
+	if len(l) == 0 || l[0] == nil {
+		return nil
+	}
 
-	if d.HasChange("enabled") {
-		_, versioningEnabled := d.GetChange("enabled")
+	tfMap, ok := l[0].(map[string]interface{})
+	if !ok {
+		return nil
+	}
 
-		err := putBucketVersioning(conn, bucket, versioningEnabled.(bool))
-		if err != nil {
-			return fmt.Errorf("Error setting versioning = %t on %s: %s", versioningEnabled, bucket, err)
-		}
+	result := &s3.VersioningConfiguration{}
+
+	if v, ok := tfMap["mfa_delete"].(string); ok && v != "" {
+		result.MFADelete = aws.String(v)
 	}
 
-	return nil
+	if v, ok := tfMap["status"].(string); ok && v != "" {
+		result.Status = aws.String(v)
+	}
+
+	return result
 }
 
-func resourceBucketVersioningDelete(d *schema.ResourceData, meta interface{}) error {
-	conn := meta.(*conns.AWSClient).S3Conn
-	bucket := d.Get("bucket").(string)
+func flattenBucketVersioningConfiguration(config *s3.GetBucketVersioningOutput) []interface{} {
+	if config == nil {
+		return []interface{}{}
+	}
 
-	// disable versioning if it was enabled
-	if d.Get("enabled").(bool) {
-		err := putBucketVersioning(conn, bucket, false)
-		if err != nil {
-			return fmt.Errorf("Error deleting aws_s3_bucket_versioning: could not disable versioning on bucket %s: %s", bucket, err)
-		}
+	m := make(map[string]interface{})
+
+	if config.MFADelete != nil {
+		m["mfa_delete"] = aws.StringValue(config.MFADelete)
 	}
 
-	d.SetId("")
+	if config.Status != nil {
+		m["status"] = aws.StringValue(config.Status)
+	}
 
-	return nil
+	return []interface{}{m}
 }