From 4f959c755d3c303b119ac27b993e2083def5a6e5 Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Tue, 22 Mar 2022 21:04:00 -0400 Subject: [PATCH 1/2] r/s3_bucket: make 'acceleration_status' configurable --- internal/service/s3/bucket.go | 31 ++++++++-- .../bucket_accelerate_configuration_test.go | 60 +++++++++++++++++++ internal/service/s3/bucket_test.go | 47 +++++++++++++++ website/docs/r/s3_bucket.html.markdown | 7 ++- 4 files changed, 140 insertions(+), 5 deletions(-) diff --git a/internal/service/s3/bucket.go b/internal/service/s3/bucket.go index 837e6c5eb2e..92399b50b96 100644 --- a/internal/service/s3/bucket.go +++ b/internal/service/s3/bucket.go @@ -378,9 +378,11 @@ func ResourceBucket() *schema.Resource { }, "acceleration_status": { - Type: schema.TypeString, - Computed: true, - Deprecated: "Use the aws_s3_bucket_accelerate_configuration resource instead", + Type: schema.TypeString, + Optional: true, + Computed: true, + Deprecated: "Use the aws_s3_bucket_accelerate_configuration resource instead", + ValidateFunc: validation.StringInSlice(s3.BucketAccelerateStatus_Values(), false), }, "request_payer": { @@ -762,6 +764,12 @@ func resourceBucketUpdate(d *schema.ResourceData, meta interface{}) error { } } + if d.HasChange("acceleration_status") { + if err := resourceBucketInternalAccelerationUpdate(conn, d); err != nil { + return fmt.Errorf("error updating S3 Bucket (%s) Acceleration Status: %w", d.Id(), err) + } + } + if d.HasChange("acl") && !d.IsNewResource() { if err := resourceBucketInternalACLUpdate(conn, d); err != nil { return fmt.Errorf("error updating S3 Bucket (%s) ACL: %w", d.Id(), err) @@ -1000,7 +1008,7 @@ func resourceBucketRead(d *schema.ResourceData, meta interface{}) error { // Amazon S3 Transfer Acceleration might not be supported in the region if err != nil && !tfawserr.ErrCodeEquals(err, ErrCodeMethodNotAllowed, ErrCodeUnsupportedArgument, ErrCodeNotImplemented) { - return fmt.Errorf("error getting S3 Bucket acceleration configuration: %w", err) + return fmt.Errorf("error getting S3 Bucket (%s) accelerate configuration: %w", d.Id(), err) } if accelerate, ok := accelerateResponse.(*s3.GetBucketAccelerateConfigurationOutput); ok { @@ -1482,6 +1490,21 @@ func normalizeRegion(region string) string { ////////////////////////////////////////// Argument-Specific Update Functions ////////////////////////////////////////// +func resourceBucketInternalAccelerationUpdate(conn *s3.S3, d *schema.ResourceData) error { + input := &s3.PutBucketAccelerateConfigurationInput{ + Bucket: aws.String(d.Id()), + AccelerateConfiguration: &s3.AccelerateConfiguration{ + Status: aws.String(d.Get("acceleration_status").(string)), + }, + } + + _, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) { + return conn.PutBucketAccelerateConfiguration(input) + }) + + return err +} + func resourceBucketInternalACLUpdate(conn *s3.S3, d *schema.ResourceData) error { acl := d.Get("acl").(string) if acl == "" { diff --git a/internal/service/s3/bucket_accelerate_configuration_test.go b/internal/service/s3/bucket_accelerate_configuration_test.go index aacc2f8c345..223c3c5047d 100644 --- a/internal/service/s3/bucket_accelerate_configuration_test.go +++ b/internal/service/s3/bucket_accelerate_configuration_test.go @@ -109,6 +109,66 @@ func TestAccS3BucketAccelerateConfiguration_disappears(t *testing.T) { }) } +func TestAccS3BucketAccelerateConfiguration_migrate_noChange(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket_accelerate_configuration.test" + bucketResourceName := "aws_s3_bucket.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketAccelerateConfigurationDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketConfig_withAcceleration(rName, s3.BucketAccelerateStatusEnabled), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckBucketExists(bucketResourceName), + resource.TestCheckResourceAttr(bucketResourceName, "acceleration_status", s3.BucketAccelerateStatusEnabled), + ), + }, + { + Config: testAccBucketAccelerateConfigurationBasicConfig(rName, s3.BucketAccelerateStatusEnabled), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckBucketAccelerateConfigurationExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "bucket", bucketResourceName, "id"), + resource.TestCheckResourceAttr(resourceName, "status", s3.BucketAccelerateStatusEnabled), + ), + }, + }, + }) +} + +func TestAccS3BucketAccelerateConfiguration_migrate_withChange(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_bucket_accelerate_configuration.test" + bucketResourceName := "aws_s3_bucket.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + ProviderFactories: acctest.ProviderFactories, + CheckDestroy: testAccCheckBucketAccelerateConfigurationDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketConfig_withAcceleration(rName, s3.BucketAccelerateStatusEnabled), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckBucketExists(bucketResourceName), + resource.TestCheckResourceAttr(bucketResourceName, "acceleration_status", s3.BucketAccelerateStatusEnabled), + ), + }, + { + Config: testAccBucketAccelerateConfigurationBasicConfig(rName, s3.BucketAccelerateStatusSuspended), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckBucketAccelerateConfigurationExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "bucket", bucketResourceName, "id"), + resource.TestCheckResourceAttr(resourceName, "status", s3.BucketAccelerateStatusSuspended), + ), + }, + }, + }) +} + func testAccCheckBucketAccelerateConfigurationDestroy(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).S3Conn diff --git a/internal/service/s3/bucket_test.go b/internal/service/s3/bucket_test.go index b49919a8e8d..db39cc31a79 100644 --- a/internal/service/s3/bucket_test.go +++ b/internal/service/s3/bucket_test.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/endpoints" "github.com/aws/aws-sdk-go/service/cloudformation" + "github.com/aws/aws-sdk-go/service/cloudfront" "github.com/aws/aws-sdk-go/service/s3" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" @@ -225,6 +226,43 @@ func TestAccS3Bucket_Basic_forceDestroyWithObjectLockEnabled(t *testing.T) { }) } +func TestAccS3Bucket_Basic_acceleration(t *testing.T) { + bucketName := sdkacctest.RandomWithPrefix("tf-test-bucket") + resourceName := "aws_s3_bucket.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(t) + acctest.PreCheckPartitionHasService(cloudfront.EndpointsID, t) + }, + ErrorCheck: acctest.ErrorCheck(t, s3.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckBucketDestroy, + Steps: []resource.TestStep{ + { + Config: testAccBucketConfig_withAcceleration(bucketName, s3.BucketAccelerateStatusEnabled), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "acceleration_status", s3.BucketAccelerateStatusEnabled), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"force_destroy"}, + }, + { + Config: testAccBucketConfig_withAcceleration(bucketName, s3.BucketAccelerateStatusSuspended), + Check: resource.ComposeTestCheckFunc( + testAccCheckBucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "acceleration_status", s3.BucketAccelerateStatusSuspended), + ), + }, + }, + }) +} + // Test TestAccS3Bucket_disappears is designed to fail with a "plan // not empty" error in Terraform, to check against regressions. // See https://github.com/hashicorp/terraform/pull/2925 @@ -1275,6 +1313,15 @@ resource "aws_s3_bucket" "bucket" { `, bucketName) } +func testAccBucketConfig_withAcceleration(bucketName, acceleration string) string { + return fmt.Sprintf(` +resource "aws_s3_bucket" "test" { + bucket = %[1]q + acceleration_status = %[2]q +} +`, bucketName, acceleration) +} + func testAccBucketConfig_withACL(bucketName, acl string) string { return fmt.Sprintf(` resource "aws_s3_bucket" "bucket" { diff --git a/website/docs/r/s3_bucket.html.markdown b/website/docs/r/s3_bucket.html.markdown index db6066aa6c1..4048ae5b871 100644 --- a/website/docs/r/s3_bucket.html.markdown +++ b/website/docs/r/s3_bucket.html.markdown @@ -12,6 +12,10 @@ Provides a S3 bucket resource. -> This functionality is for managing S3 in an AWS Partition. To manage [S3 on Outposts](https://docs.aws.amazon.com/AmazonS3/latest/dev/S3onOutposts.html), see the [`aws_s3control_bucket`](/docs/providers/aws/r/s3control_bucket.html) resource. +~> **NOTE on S3 Bucket Accelerate Configuration:** S3 Bucket Accelerate can be configured in either the standalone resource [`aws_s3_bucket_accelerate_configuration`](s3_bucket_accelerate_configuration.html) +or with the deprecated parameter `acceleration_status` in the resource `aws_s3_bucket`. +Configuring with both will cause inconsistencies and may overwrite configuration. + ~> **NOTE on S3 Bucket canned ACL Configuration:** S3 Bucket canned ACL can be configured in either the standalone resource [`aws_s3_bucket_acl`](s3_bucket_acl.html.markdown) or with the deprecated parameter `acl` in the resource `aws_s3_bucket`. Configuring with both will cause inconsistencies and may overwrite configuration. @@ -113,6 +117,8 @@ The following arguments are supported: * `bucket` - (Optional, Forces new resource) The name of the bucket. If omitted, Terraform will assign a random, unique name. Must be lowercase and less than or equal to 63 characters in length. A full list of bucket naming rules [may be found here](https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html). * `bucket_prefix` - (Optional, Forces new resource) Creates a unique bucket name beginning with the specified prefix. Conflicts with `bucket`. Must be lowercase and less than or equal to 37 characters in length. A full list of bucket naming rules [may be found here](https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html). +* `acceleration_status` - (Optional, **Deprecated**) Sets the accelerate configuration of an existing bucket. Can be `Enabled` or `Suspended`. Cannot be used in `cn-north-1` or `us-gov-west-1`. Terraform will only perform drift detection if a configuration value is provided. + Use the resource [`aws_s3_bucket_accelerate_configuration`](s3_bucket_accelerate_configuration.html) instead. * `acl` - (Optional, **Deprecated**) The [canned ACL](https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl) to apply. Valid values are `private`, `public-read`, `public-read-write`, `aws-exec-read`, `authenticated-read`, and `log-delivery-write`. Defaults to `private`. Conflicts with `grant`. Terraform will only perform drift detection if a configuration value is provided. Use the resource [`aws_s3_bucket_acl`](s3_bucket_acl.html.markdown) instead. * `grant` - (Optional, **Deprecated**) An [ACL policy grant](https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#sample-acl). See [Grant](#grant) below for details. Conflicts with `acl`. Terraform will only perform drift detection if a configuration value is provided. Use the resource [`aws_s3_bucket_acl`](s3_bucket_acl.html.markdown) instead. * `force_destroy` - (Optional, Default:`false`) A boolean that indicates all objects (including any [locked objects](https://docs.aws.amazon.com/AmazonS3/latest/dev/object-lock-overview.html)) should be deleted from the bucket so that the bucket can be destroyed without error. These objects are *not* recoverable. @@ -147,7 +153,6 @@ The `object_lock_configuration` configuration block supports the following argum In addition to all arguments above, the following attributes are exported: * `id` - The name of the bucket. -* `acceleration_status` - (Optional) The accelerate configuration status of the bucket. Not available in `cn-north-1` or `us-gov-west-1`. * `arn` - The ARN of the bucket. Will be of format `arn:aws:s3:::bucketname`. * `bucket_domain_name` - The bucket domain name. Will be of format `bucketname.s3.amazonaws.com`. * `bucket_regional_domain_name` - The bucket region-specific domain name. The bucket domain name including the region name, please refer [here](https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region) for format. Note: The AWS CloudFront allows specifying S3 region-specific endpoint when creating S3 origin, it will prevent [redirect issues](https://forums.aws.amazon.com/thread.jspa?threadID=216814) from CloudFront to S3 Origin URL. From 7b84f21150ff03d341bd875c8eba17afb89a8e37 Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Tue, 22 Mar 2022 21:06:37 -0400 Subject: [PATCH 2/2] Update CHANGELOG for #23816 --- .changelog/23816.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/23816.txt diff --git a/.changelog/23816.txt b/.changelog/23816.txt new file mode 100644 index 00000000000..ccec53a7cd9 --- /dev/null +++ b/.changelog/23816.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +resource/aws_s3_bucket: Update `acceleration_status` parameter to be configurable. Please refer to the documentation for details on drift detection and potential conflicts when configuring this parameter with the standalone `aws_s3_bucket_accelerate_configuration` resource. +``` \ No newline at end of file