Merge pull request #8915 from TimeIncOSS/b-aws-s3-policy-fix

provider/aws: Make it possible to remove S3 bucket policy

builtin/providers/aws/import_aws_s3_bucket.go

@@ -0,0 +1,39 @@
+package aws
+
+import (
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
+	"github.com/aws/aws-sdk-go/service/s3"
+	"github.com/hashicorp/errwrap"
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func resourceAwsS3BucketImportState(
+	d *schema.ResourceData,
+	meta interface{}) ([]*schema.ResourceData, error) {
+
+	results := make([]*schema.ResourceData, 1, 1)
+	results[0] = d
+
+	conn := meta.(*AWSClient).s3conn
+	pol, err := conn.GetBucketPolicy(&s3.GetBucketPolicyInput{
+		Bucket: aws.String(d.Id()),
+	})
+	if err != nil {
+		if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "NoSuchBucketPolicy" {
+			// Bucket without policy
+			return results, nil
+		}
+		return nil, errwrap.Wrapf("Error importing AWS S3 bucket policy: {{err}}", err)
+	}
+
+	policy := resourceAwsS3BucketPolicy()
+	pData := policy.Data(nil)
+	pData.SetId(d.Id())
+	pData.SetType("aws_s3_bucket_policy")
+	pData.Set("bucket", d.Id())
+	pData.Set("policy", pol)
+	results = append(results, pData)
+
+	return results, nil
+}

builtin/providers/aws/import_aws_s3_bucket_test.go

@@ -1,10 +1,12 @@
 package aws
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/hashicorp/terraform/helper/acctest"
 	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/terraform"
 )
 
 func TestAccAWSS3Bucket_importBasic(t *testing.T) {
@@ -30,3 +32,46 @@ func TestAccAWSS3Bucket_importBasic(t *testing.T) {
 		},
 	})
 }
+
+func TestAccAWSS3Bucket_importWithPolicy(t *testing.T) {
+	rInt := acctest.RandInt()
+
+	checkFn := func(s []*terraform.InstanceState) error {
+		// Expect 2: bucket + policy
+		if len(s) != 2 {
+			return fmt.Errorf("expected 2 states: %#v", s)
+		}
+		bucketState, policyState := s[0], s[1]
+
+		expectedBucketId := fmt.Sprintf("tf-test-bucket-%d", rInt)
+
+		if bucketState.ID != expectedBucketId {
+			return fmt.Errorf("expected bucket of ID %s, %s received",
+				expectedBucketId, bucketState.ID)
+		}
+
+		if policyState.ID != expectedBucketId {
+			return fmt.Errorf("expected policy of ID %s, %s received",
+				expectedBucketId, bucketState.ID)
+		}
+
+		return nil
+	}
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSS3BucketDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSS3BucketConfigWithPolicy(rInt),
+			},
+
+			resource.TestStep{
+				ResourceName:     "aws_s3_bucket.bucket",
+				ImportState:      true,
+				ImportStateCheck: checkFn,
+			},
+		},
+	})
+}

builtin/providers/aws/resource_aws_s3_bucket.go

@@ -23,7 +23,7 @@ func resourceAwsS3Bucket() *schema.Resource {
 		Update: resourceAwsS3BucketUpdate,
 		Delete: resourceAwsS3BucketDelete,
 		Importer: &schema.ResourceImporter{
-			State: schema.ImportStatePassthrough,
+			State: resourceAwsS3BucketImportState,
 		},
 
 		Schema: map[string]*schema.Schema{
@@ -48,7 +48,6 @@ func resourceAwsS3Bucket() *schema.Resource {
 			"policy": &schema.Schema{
 				Type:             schema.TypeString,
 				Optional:         true,
-				Computed:         true,
 				DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs,
 			},
 
@@ -452,21 +451,23 @@ func resourceAwsS3BucketRead(d *schema.ResourceData, meta interface{}) error {
 	}
 
 	// Read the policy
-	pol, err := s3conn.GetBucketPolicy(&s3.GetBucketPolicyInput{
-		Bucket: aws.String(d.Id()),
-	})
-	log.Printf("[DEBUG] S3 bucket: %s, read policy: %v", d.Id(), pol)
-	if err != nil {
-		if err := d.Set("policy", ""); err != nil {
-			return err
-		}
-	} else {
-		if v := pol.Policy; v == nil {
+	if _, ok := d.GetOk("policy"); ok {
+		pol, err := s3conn.GetBucketPolicy(&s3.GetBucketPolicyInput{
+			Bucket: aws.String(d.Id()),
+		})
+		log.Printf("[DEBUG] S3 bucket: %s, read policy: %v", d.Id(), pol)
+		if err != nil {
 			if err := d.Set("policy", ""); err != nil {
 				return err
 			}
-		} else if err := d.Set("policy", normalizeJson(*v)); err != nil {
-			return err
+		} else {
+			if v := pol.Policy; v == nil {
+				if err := d.Set("policy", ""); err != nil {
+					return err
+				}
+			} else if err := d.Set("policy", normalizeJson(*v)); err != nil {
+				return err
+			}
 		}
 	}
 

builtin/providers/aws/resource_aws_s3_bucket_test.go

@@ -645,14 +645,20 @@ func testAccCheckAWSS3BucketPolicy(n string, policy string) resource.TestCheckFu
 			Bucket: aws.String(rs.Primary.ID),
 		})
 
-		if err != nil {
-			if policy == "" {
+		if policy == "" {
+			if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "NoSuchBucketPolicy" {
 				// expected
 				return nil
+			}
+			if err == nil {
+				return fmt.Errorf("Expected no policy, got: %#v", *out.Policy)
 			} else {
 				return fmt.Errorf("GetBucketPolicy error: %v, expected %s", err, policy)
 			}
 		}
+		if err != nil {
+			return fmt.Errorf("GetBucketPolicy error: %v, expected %s", err, policy)
+		}
 
 		if v := out.Policy; v == nil {
 			if policy != "" {