Skip to content

Commit

Permalink
Merge pull request #23731 from hashicorp/backport-bucket-versioning-fix
Browse files Browse the repository at this point in the history 
r/s3_bucket_versioning: backport #23723; support management of resource with disabled versioning
  • Loading branch information
@anGie44
anGie44 authored Mar 17, 2022
2 parents c09f3c4 + b969302 commit 3dc5e08
Show file tree
Hide file tree
Showing 7 changed files with 323 additions and 24 deletions.
3 changes: 3 additions & 0 deletions .changelog/23731.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:enhancement
resource/aws_s3_bucket_versioning: Add missing support for `Disabled` bucket versioning
```
79 changes: 59 additions & 20 deletions internal/service/s3/bucket_versioning.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/aws/aws-sdk-go/service/s3"
"github.com/hashicorp/aws-sdk-go-base/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"
Expand Down Expand Up @@ -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
},
),
}
}

Expand All @@ -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))
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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}
Expand Down
214 changes: 213 additions & 1 deletion internal/service/s3/bucket_versioning_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package s3_test

import (
"fmt"
"regexp"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -134,6 +135,68 @@ func TestAccS3BucketVersioning_MFADelete(t *testing.T) {
})
}

func TestAccS3BucketVersioning_migrate_versioningDisabledNoChange(t *testing.T) {
bucketName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
bucketResourceName := "aws_s3_bucket.bucket"
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: testAccCheckBucketDestroy,
Steps: []resource.TestStep{
{
Config: testAccBucketWithVersioningConfig(bucketName, false),
Check: resource.ComposeTestCheckFunc(
testAccCheckBucketExists(bucketResourceName),
resource.TestCheckResourceAttr(bucketResourceName, "versioning.#", "1"),
resource.TestCheckResourceAttr(bucketResourceName, "versioning.0.enabled", "false"),
),
},
{
Config: testAccBucketVersioning_Migrate_VersioningEnabledConfig(bucketName, tfs3.BucketVersioningStatusDisabled),
Check: resource.ComposeTestCheckFunc(
testAccCheckBucketVersioningExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"),
resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", tfs3.BucketVersioningStatusDisabled),
),
},
},
})
}

func TestAccS3BucketVersioning_migrate_versioningDisabledWithChange(t *testing.T) {
bucketName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
bucketResourceName := "aws_s3_bucket.bucket"
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: testAccCheckBucketDestroy,
Steps: []resource.TestStep{
{
Config: testAccBucketWithVersioningConfig(bucketName, false),
Check: resource.ComposeTestCheckFunc(
testAccCheckBucketExists(bucketResourceName),
resource.TestCheckResourceAttr(bucketResourceName, "versioning.#", "1"),
resource.TestCheckResourceAttr(bucketResourceName, "versioning.0.enabled", "false"),
),
},
{
Config: testAccBucketVersioning_Migrate_VersioningEnabledConfig(bucketName, s3.BucketVersioningStatusEnabled),
Check: resource.ComposeTestCheckFunc(
testAccCheckBucketVersioningExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "versioning_configuration.#", "1"),
resource.TestCheckResourceAttr(resourceName, "versioning_configuration.0.status", s3.BucketVersioningStatusEnabled),
),
},
},
})
}

func TestAccS3BucketVersioning_migrate_versioningEnabledNoChange(t *testing.T) {
bucketName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
bucketResourceName := "aws_s3_bucket.bucket"
Expand Down Expand Up @@ -229,6 +292,155 @@ func TestAccS3BucketVersioning_migrate_mfaDeleteNoChange(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

Expand All @@ -251,7 +463,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)
}
}
Expand Down
8 changes: 8 additions & 0 deletions internal/service/s3/enum.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const (
BucketCannedACLExecRead = "aws-exec-read"
BucketCannedACLLogDeliveryWrite = "log-delivery-write"

BucketVersioningStatusDisabled = "Disabled"

LifecycleRuleStatusEnabled = "Enabled"
LifecycleRuleStatusDisabled = "Disabled"
)
Expand All @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions internal/service/s3/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Loading

0 comments on commit 3dc5e08

Please sign in to comment.