provider/aws: force_destroy argument for s3 buckets with objects

[m-s-austin]

This PR is for feedback / suggestions on a feature that would make it possible to destroy an S3 bucket even if it has objects in it when you are tearing it down.

resource "aws_s3_bucket" default {
   bucket = "my-tf-test-bucket"
   force_destroy = true
}

force_destroy defaults to false, so it won't change the current behavior (which is to error out if there are objects in the bucket), but if you set force_destroy to true, all of the items in the bucket would be deleted thereby allowing the entire bucket to be destroyed.

I have tested this code and it works well, but I am not sure of the best way to write an acceptance test for it, I could also use feedback on that.
Thanks!

[catsby]

go ahead and use err here instead of listErr

[catsby]

This is a good start! I noticed that the S3 API will by default only return the first 1,000 results with the ListBucket call (docs here). We'll need to check IsTruncated in the response and do follow up requests using NextMarker to go through them all.

[m-s-austin]

@catsby that was my original thought / plan as well, but as it is written it should recursively delete in groups of 1000 until all objects are gone or it returns an error. I'd be glad to revamp it into a loop if you prefer that approach. I thought this looked a little cleaner is why I went with it to start with.

I have added a comment in the code to make that intent more clear, but let me know if I should go with the other approach.

[catsby]

At one point I had a note here on formatting/style here, did I not post it?
I would rather see this style:

if err != nil {
    ec2err, ok := err.(aws.APIError)
    if ok && ec2err.Code == "BucketNotEmpty" {
        if d.Get("force_destroy").(bool) { 
            // bucket may have things delete them
          // attempt to delete things
        }
    } 
    return fmt.Errorf("Error deleting S3 Bucket: %s", err)
}

[m-s-austin]

I don't think I saw the note, but I will address it now. Thanks!

[catsby]

as it is written it should recursively delete in groups of 1000

Ah, clearly missed that. I'm curious how well that will work, more so on AWS ability to update and accurately re-list items as we need it in this recursive manner vs. us using the markers to page through. It's hard to say without a large bucket with various items in it handy to test with.

I'm OK with the recursive strategy as is then, thanks for pointing it out

[m-s-austin]

The testing comment actually leads into another question I have :)
I want to prepare an acceptance test for this but it doesn't seem like terraform currently has a way to "pre-load" a bucket with objects so currently we have to create the bucket then run some other scripts to upload the files. We were contemplating a method of doing this possibly something like (still rough obviously)

resource "aws_s3_bucket_object" default {
   bucket = "my-tf-test-bucket"
   key = "key"
   acl = "acl"
   body = file  
etc... 
}

so actually 2 questions 1) Is there already a way to do this I am missing? 2) does this seem like right approach for this?

[m-s-austin]

I have just finished a test for deleting ~3000 objects successfully. I planned to test in on 5000 objects, but the AWS uploader stopped working somewhere around 60%.

It seems to work quite well even with a full load

On May 19, 2015, at 12:49 PM, Clint [email protected] wrote:

'm curious how well that will work, more so on AWS ability to update and accurately re-list items as we need it in this recursive manner vs. us using the markers to page through. It's hard to say without a large bucket with various items in it handy to test with.

[catsby]

Excellent, thank you for testing that! Unfortunately master has progressed and introduced a change from upstream awslabs/aws-sdk-go, can you rebase? It's related to errors:

• aws/aws-sdk-go@be94f7c

I'm pretty sure this is the only change you need to make:

-ec2err, ok := err.(aws.APIError)
-if ok && ec2err.Code == "BucketNotEmpty" {
+ec2err, ok := err.(awserr.Error)
+if ok && ec2err.Code() == "BucketNotEmpty" {

You may also need to import awserr in the import block at the top:

"github.com/awslabs/aws-sdk-go/aws/awserr"

[m-s-austin]

I think it should be good now. Thanks!

—
Reply to this email directly or view it on GitHub #2007 (comment).

[catsby]

Sorry to keep you running around @m-s-austin , could you please add documentation?

• https://github.com/hashicorp/terraform/blob/master/website/source/docs/providers/aws/r/s3_bucket.html.markdown

Also, there are 23 commits here, could you possibly squash?

[m-s-austin]

No problem :) done and done

[catsby]

Thanks! Everything checked out here, pulling this in.
Thanks for the contribution!

[ozbillwang]

@m-s-austin & @catsby

Will the option force_destroy have conflicts with option versioning?

Second, why we have inconsistent setting as below?

    force_destroy = true

    versioning {
      enabled = true
    }

Should they be same?

    force_destroy = true
    versioning = true

or

     force_destroy {
      enabled = true
    }

    versioning {
      enabled = true
    }

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.