Merge pull request #26011 from hashicorp/b-duplicate-s3-bucket

resource/aws_s3_bucket: Checks for duplicate bucket in `us-east-1`

.changelog/26011.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_s3_bucket: Prevents unexpected import of existing bucket in `us-east-1`.
+```

internal/service/s3/bucket.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log"
 	"net/http"
@@ -28,6 +29,11 @@ import (
 	tftags "github.com/hashicorp/terraform-provider-aws/internal/tags"
 	"github.com/hashicorp/terraform-provider-aws/internal/tfresource"
 	"github.com/hashicorp/terraform-provider-aws/internal/verify"
+	"github.com/hashicorp/terraform-provider-aws/names"
+)
+
+const (
+	resNameBucket = "Bucket"
 )
 
 func ResourceBucket() *schema.Resource {
@@ -711,6 +717,19 @@ func resourceBucketCreate(d *schema.ResourceData, meta interface{}) error {
 		bucket = resource.UniqueId()
 	}
 
+	awsRegion := meta.(*conns.AWSClient).Region
+
+	// Special case: us-east-1 does not return error if the bucket already exists and is owned by
+	// current account. It also resets the Bucket ACLs.
+	if awsRegion == endpoints.UsEast1RegionID {
+		_, err := conn.HeadBucket(&s3.HeadBucketInput{
+			Bucket: aws.String(bucket),
+		})
+		if err == nil {
+			return names.Error(names.S3, names.ErrActionCreating, resNameBucket, bucket, errors.New(ErrMessageBucketAlreadyExists))
+		}
+	}
+
 	log.Printf("[DEBUG] S3 bucket create: %s", bucket)
 
 	req := &s3.CreateBucketInput{
@@ -730,7 +749,6 @@ func resourceBucketCreate(d *schema.ResourceData, meta interface{}) error {
 		log.Printf("[DEBUG] S3 bucket %s has default canned ACL %s", bucket, s3.BucketCannedACLPrivate)
 	}
 
-	awsRegion := meta.(*conns.AWSClient).Region
 	log.Printf("[DEBUG] S3 bucket create: %s, using region: %s", bucket, awsRegion)
 
 	// Special case us-east-1 region and do not set the LocationConstraint.
@@ -760,7 +778,7 @@ func resourceBucketCreate(d *schema.ResourceData, meta interface{}) error {
 		_, err := conn.CreateBucket(req)
 
 		if tfawserr.ErrCodeEquals(err, ErrCodeOperationAborted) {
-			return resource.RetryableError(fmt.Errorf("error creating S3 Bucket (%s), retrying: %w", bucket, err))
+			return resource.RetryableError(err)
 		}
 
 		if err != nil {
@@ -773,7 +791,7 @@ func resourceBucketCreate(d *schema.ResourceData, meta interface{}) error {
 		_, err = conn.CreateBucket(req)
 	}
 	if err != nil {
-		return fmt.Errorf("error creating S3 Bucket (%s): %w", bucket, err)
+		return names.Error(names.S3, names.ErrActionCreating, resNameBucket, bucket, err)
 	}
 
 	// Assign the bucket name as the resource ID
@@ -935,7 +953,7 @@ func resourceBucketRead(d *schema.ResourceData, meta interface{}) error {
 	}
 
 	if err != nil {
-		return fmt.Errorf("error reading S3 Bucket (%s): %w", d.Id(), err)
+		return names.Error(names.S3, names.ErrActionReading, resNameBucket, d.Id(), err)
 	}
 
 	d.Set("bucket", d.Id())

internal/service/s3/bucket_test.go

@@ -356,6 +356,65 @@ func TestAccS3Bucket_disappears(t *testing.T) {
 	})
 }
 
+func TestAccS3Bucket_Duplicate_basic(t *testing.T) {
+	bucketName := sdkacctest.RandomWithPrefix("tf-test-bucket")
+	region := acctest.Region()
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck: func() {
+			acctest.PreCheck(t)
+			acctest.PreCheckRegionNot(t, endpoints.UsEast1RegionID)
+		},
+		ErrorCheck:               acctest.ErrorCheck(t, s3.EndpointsID),
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccCheckBucketDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config:      testAccBucketConfig_duplicate(region, bucketName),
+				ExpectError: regexp.MustCompile(s3.ErrCodeBucketAlreadyOwnedByYou),
+			},
+		},
+	})
+}
+
+func TestAccS3Bucket_Duplicate_UsEast1(t *testing.T) {
+	bucketName := sdkacctest.RandomWithPrefix("tf-test-bucket")
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck: func() {
+			acctest.PreCheck(t)
+			acctest.PreCheckPartition(t, endpoints.AwsPartitionID)
+		},
+		ErrorCheck:               acctest.ErrorCheck(t, s3.EndpointsID),
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccCheckBucketDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config:      testAccBucketConfig_duplicate(endpoints.UsEast1RegionID, bucketName),
+				ExpectError: regexp.MustCompile(tfs3.ErrMessageBucketAlreadyExists),
+			},
+		},
+	})
+}
+
+func TestAccS3Bucket_Duplicate_UsEast1AltAccount(t *testing.T) {
+	bucketName := sdkacctest.RandomWithPrefix("tf-test-bucket")
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck: func() {
+			acctest.PreCheck(t)
+			acctest.PreCheckPartition(t, endpoints.AwsPartitionID)
+			acctest.PreCheckAlternateAccount(t)
+		},
+		ErrorCheck:               acctest.ErrorCheck(t, s3.EndpointsID),
+		ProtoV5ProviderFactories: acctest.ProtoV5FactoriesAlternate(t),
+		CheckDestroy:             testAccCheckBucketDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config:      testAccBucketConfig_duplicateAltAccount(endpoints.UsEast1RegionID, bucketName),
+				ExpectError: regexp.MustCompile(s3.ErrCodeBucketAlreadyExists),
+			},
+		},
+	})
+}
+
 func TestAccS3Bucket_Tags_basic(t *testing.T) {
 	rInt := sdkacctest.RandInt()
 	resourceName := "aws_s3_bucket.bucket1"
@@ -4633,3 +4692,39 @@ resource "aws_s3_bucket" "test" {
   bucket_prefix = "tf-test-"
 }
 `
+
+func testAccBucketConfig_duplicate(region, bucketName string) string {
+	return acctest.ConfigCompose(
+		acctest.ConfigRegionalProvider(region),
+		fmt.Sprintf(`
+resource "aws_s3_bucket" "test" {
+  bucket = %[1]q
+
+  depends_on = [aws_s3_bucket.duplicate]
+}
+
+resource "aws_s3_bucket" "duplicate" {
+  bucket = %[1]q
+}
+  `, bucketName),
+	)
+}
+
+func testAccBucketConfig_duplicateAltAccount(region, bucketName string) string {
+	return acctest.ConfigCompose(
+		acctest.ConfigRegionalProvider(region),
+		acctest.ConfigAlternateAccountProvider(),
+		fmt.Sprintf(`
+resource "aws_s3_bucket" "test" {
+  bucket = %[1]q
+
+  depends_on = [aws_s3_bucket.duplicate]
+}
+
+resource "aws_s3_bucket" "duplicate" {
+  provider = "awsalternate"
+  bucket   = %[1]q
+}
+  `, bucketName),
+	)
+}

internal/service/s3/errors.go

@@ -28,3 +28,7 @@ const (
 	// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/14645
 	ErrCodeXNotImplemented = "XNotImplemented"
 )
+
+const (
+	ErrMessageBucketAlreadyExists = "bucket already exists"
+)