From b6406342d41e723e70986279efaf41f9a0801b6d Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 27 Jul 2022 13:38:23 -0700 Subject: [PATCH 1/2] Checks for existence of S3 bucket in `us-east-1` --- internal/service/s3/acc_test.go | 22 ------- internal/service/s3/bucket.go | 26 ++++++-- internal/service/s3/bucket_test.go | 95 ++++++++++++++++++++++++++++++ internal/service/s3/errors.go | 4 ++ 4 files changed, 121 insertions(+), 26 deletions(-) delete mode 100644 internal/service/s3/acc_test.go diff --git a/internal/service/s3/acc_test.go b/internal/service/s3/acc_test.go deleted file mode 100644 index 030c1512672a..000000000000 --- a/internal/service/s3/acc_test.go +++ /dev/null @@ -1,22 +0,0 @@ -package s3_test - -import ( - "testing" - - "github.com/aws/aws-sdk-go/aws/endpoints" - tfs3 "github.com/hashicorp/terraform-provider-aws/internal/service/s3" -) - -func TestHostedZoneIDForRegion(t *testing.T) { - if r, _ := tfs3.HostedZoneIDForRegion(endpoints.UsEast1RegionID); r != "Z3AQBSTGFYJSTF" { - t.Fatalf("bad: %s", r) - } - if r, _ := tfs3.HostedZoneIDForRegion(endpoints.ApSoutheast2RegionID); r != "Z1WCIGYICN2BYD" { - t.Fatalf("bad: %s", r) - } - - // Bad input should be error - if r, err := tfs3.HostedZoneIDForRegion("not-a-region"); err == nil { - t.Fatalf("bad: %s", r) - } -} diff --git a/internal/service/s3/bucket.go b/internal/service/s3/bucket.go index e302762b1410..38ffe9be167e 100644 --- a/internal/service/s3/bucket.go +++ b/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()) diff --git a/internal/service/s3/bucket_test.go b/internal/service/s3/bucket_test.go index 327346be8857..d5af6e399e60 100644 --- a/internal/service/s3/bucket_test.go +++ b/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), + ) +} diff --git a/internal/service/s3/errors.go b/internal/service/s3/errors.go index 482285b469d5..47afc37adb59 100644 --- a/internal/service/s3/errors.go +++ b/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" +) From 2f38ac5425801b1b3978b2e096a7d68433669367 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 27 Jul 2022 13:45:09 -0700 Subject: [PATCH 2/2] Adds CHANGELOG entry --- .changelog/26011.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/26011.txt diff --git a/.changelog/26011.txt b/.changelog/26011.txt new file mode 100644 index 000000000000..d68c38d72da6 --- /dev/null +++ b/.changelog/26011.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_s3_bucket: Prevents unexpected import of existing bucket in `us-east-1`. +```