r/s3_bucket: make 'policy' configurable

internal/service/s3/bucket.go

@@ -122,9 +122,12 @@ func ResourceBucket() *schema.Resource {
 			},
 
 			"policy": {
-				Type:       schema.TypeString,
-				Computed:   true,
-				Deprecated: "Use the aws_s3_bucket_policy resource instead",
+				Type:             schema.TypeString,
+				Optional:         true,
+				Computed:         true,
+				Deprecated:       "Use the aws_s3_bucket_policy resource instead",
+				ValidateFunc:     validation.StringIsJSON,
+				DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs,
 			},
 
 			"cors_rule": {
@@ -782,6 +785,12 @@ func resourceBucketUpdate(d *schema.ResourceData, meta interface{}) error {
 
 	// Note: Order of argument updates below is important
 
+	if d.HasChange("policy") {
+		if err := resourceBucketInternalPolicyUpdate(conn, d); err != nil {
+			return fmt.Errorf("error updating S3 Bucket (%s) Policy: %w", d.Id(), err)
+		}
+	}
+
 	if d.HasChange("cors_rule") {
 		if err := resourceBucketInternalCorsUpdate(conn, d); err != nil {
 			return fmt.Errorf("error updating S3 Bucket (%s) CORS Rules: %w", d.Id(), err)
@@ -1871,6 +1880,50 @@ func resourceBucketInternalObjectLockConfigurationUpdate(conn *s3.S3, d *schema.
 	return err
 }
 
+func resourceBucketInternalPolicyUpdate(conn *s3.S3, d *schema.ResourceData) error {
+	policy, err := structure.NormalizeJsonString(d.Get("policy").(string))
+
+	if err != nil {
+		return fmt.Errorf("policy (%s) is an invalid JSON: %w", policy, err)
+	}
+
+	if policy == "" {
+		_, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
+			return conn.DeleteBucketPolicy(&s3.DeleteBucketPolicyInput{
+				Bucket: aws.String(d.Id()),
+			})
+		})
+
+		if err != nil {
+			return fmt.Errorf("error deleting S3 Bucket (%s) policy: %w", d.Id(), err)
+		}
+
+		return nil
+	}
+
+	params := &s3.PutBucketPolicyInput{
+		Bucket: aws.String(d.Id()),
+		Policy: aws.String(policy),
+	}
+
+	err = resource.Retry(1*time.Minute, func() *resource.RetryError {
+		_, err := conn.PutBucketPolicy(params)
+		if tfawserr.ErrCodeEquals(err, ErrCodeMalformedPolicy, s3.ErrCodeNoSuchBucket) {
+			return resource.RetryableError(err)
+		}
+		if err != nil {
+			return resource.NonRetryableError(err)
+		}
+		return nil
+	})
+
+	if tfresource.TimedOut(err) {
+		_, err = conn.PutBucketPolicy(params)
+	}
+
+	return err
+}
+
 func resourceBucketInternalReplicationConfigurationUpdate(conn *s3.S3, d *schema.ResourceData) error {
 	replicationConfiguration := d.Get("replication_configuration").([]interface{})
 

internal/service/s3/bucket_policy_test.go

@@ -2,6 +2,7 @@ package s3_test
 
 import (
 	"fmt"
+	"strconv"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -249,6 +250,66 @@ func TestAccS3BucketPolicy_IAMRoleOrder_jsonEncode(t *testing.T) {
 	})
 }
 
+func TestAccS3BucketPolicy_migrate_noChange(t *testing.T) {
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+	resourceName := "aws_s3_bucket_policy.test"
+	bucketResourceName := "aws_s3_bucket.test"
+	partition := acctest.Partition()
+
+	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: testAccBucketConfig_withPolicy(rName, partition),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckBucketExists(bucketResourceName),
+					testAccCheckBucketPolicy(bucketResourceName, testAccBucketPolicy(rName, partition)),
+				),
+			},
+			{
+				Config: testAccBucketPolicy_Migrate_NoChangeConfig(rName, partition),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckBucketExists(bucketResourceName),
+					testAccCheckBucketPolicy(resourceName, testAccBucketPolicy(rName, partition)),
+				),
+			},
+		},
+	})
+}
+
+func TestAccS3BucketPolicy_migrate_withChange(t *testing.T) {
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+	resourceName := "aws_s3_bucket_policy.test"
+	bucketResourceName := "aws_s3_bucket.test"
+	partition := acctest.Partition()
+
+	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: testAccBucketConfig_withPolicy(rName, partition),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckBucketExists(bucketResourceName),
+					testAccCheckBucketPolicy(bucketResourceName, testAccBucketPolicy(rName, partition)),
+				),
+			},
+			{
+				Config: testAccBucketPolicy_Migrate_WithChangeConfig(rName, partition),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckBucketExists(resourceName),
+					testAccCheckBucketPolicy(resourceName, testAccBucketPolicyUpdated(rName, partition)),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckBucketHasPolicy(n string, expectedPolicyText string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		rs, ok := s.RootModule().Resources[n]
@@ -640,3 +701,56 @@ resource "aws_s3_bucket_policy" "bucket" {
 }
 `)
 }
+
+func testAccBucketPolicy_Migrate_NoChangeConfig(bucketName, partition string) string {
+	return fmt.Sprintf(`
+resource "aws_s3_bucket" "test" {
+  bucket = %[1]q
+}
+
+resource "aws_s3_bucket_acl" "test" {
+  bucket = aws_s3_bucket.test.id
+  acl    = "private"
+}
+
+resource "aws_s3_bucket_policy" "test" {
+  bucket = aws_s3_bucket.test.id
+  policy = %[2]s
+}
+`, bucketName, strconv.Quote(testAccBucketPolicy(bucketName, partition)))
+}
+
+func testAccBucketPolicy_Migrate_WithChangeConfig(bucketName, partition string) string {
+	return fmt.Sprintf(`
+resource "aws_s3_bucket" "test" {
+  bucket = %[1]q
+}
+
+resource "aws_s3_bucket_acl" "test" {
+  bucket = aws_s3_bucket.test.id
+  acl    = "private"
+}
+
+resource "aws_s3_bucket_policy" "test" {
+  bucket = aws_s3_bucket.test.id
+  policy = %[2]s
+}
+`, bucketName, strconv.Quote(testAccBucketPolicyUpdated(bucketName, partition)))
+}
+
+func testAccBucketPolicyUpdated(bucketName, partition string) string {
+	return fmt.Sprintf(`{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "*"
+      },
+      "Action": "s3:PutObject",
+      "Resource": "arn:%[1]s:s3:::%[2]s/*"
+    }
+  ]
+}`, partition, bucketName)
+}

internal/service/s3/bucket_test.go

@@ -1,15 +1,18 @@
 package s3_test
 
 import (
+	"encoding/json"
 	"fmt"
 	"log"
 	"reflect"
 	"regexp"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/aws/endpoints"
 	"github.com/aws/aws-sdk-go/service/cloudformation"
 	"github.com/aws/aws-sdk-go/service/cloudfront"
@@ -2101,6 +2104,62 @@ func TestAccS3Bucket_Security_disableDefaultEncryptionWhenDefaultEncryptionIsEna
 	})
 }
 
+func TestAccS3Bucket_Security_policy(t *testing.T) {
+	bucketName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+	partition := acctest.Partition()
+	resourceName := "aws_s3_bucket.test"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { acctest.PreCheck(t) },
+		ErrorCheck:   acctest.ErrorCheck(t, s3.EndpointsID),
+		Providers:    acctest.Providers,
+		CheckDestroy: testAccCheckBucketDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccBucketConfig_withPolicy(bucketName, partition),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBucketExists(resourceName),
+					testAccCheckBucketPolicy(resourceName, testAccBucketPolicy(bucketName, partition)),
+				),
+			},
+			{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"acl",
+					"force_destroy",
+					"grant",
+					// NOTE: Prior to Terraform AWS Provider 3.0, this attribute did not import correctly either.
+					//       The Read function does not require GetBucketPolicy, if the argument is not configured.
+					//       Rather than introduce that breaking change as well with 3.0, instead we leave the
+					//       current Read behavior and note this will be deprecated in a later 3.x release along
+					//       with other inline policy attributes across the provider.
+					"policy",
+				},
+			},
+			{
+				// As Policy is a Computed field, removing it from terraform will not
+				// trigger an update to remove it from the S3 bucket.
+				Config: testAccBucketConfig_Basic(bucketName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBucketExists(resourceName),
+					testAccCheckBucketPolicy(resourceName, testAccBucketPolicy(bucketName, partition)),
+				),
+			},
+			{
+				// As Policy is a Computed field, setting it to the empty String will not
+				// trigger an update to remove it from the S3 bucket.
+				Config: testAccBucketConfig_withEmptyPolicy(bucketName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckBucketExists(resourceName),
+					testAccCheckBucketPolicy(resourceName, testAccBucketPolicy(bucketName, partition)),
+				),
+			},
+		},
+	})
+}
+
 func TestAccS3Bucket_Web_simple(t *testing.T) {
 	bucketName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
 	region := acctest.Region()
@@ -2719,6 +2778,53 @@ func testAccCheckS3BucketDomainName(resourceName string, attributeName string, b
 	}
 }
 
+func testAccCheckBucketPolicy(n string, policy string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		rs := s.RootModule().Resources[n]
+		conn := acctest.Provider.Meta().(*conns.AWSClient).S3Conn
+
+		out, err := conn.GetBucketPolicy(&s3.GetBucketPolicyInput{
+			Bucket: aws.String(rs.Primary.ID),
+		})
+
+		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 != "" {
+				return fmt.Errorf("bad policy, found nil, expected: %s", policy)
+			}
+		} else {
+			expected := make(map[string]interface{})
+			if err := json.Unmarshal([]byte(policy), &expected); err != nil {
+				return err
+			}
+			actual := make(map[string]interface{})
+			if err := json.Unmarshal([]byte(*v), &actual); err != nil {
+				return err
+			}
+
+			if !reflect.DeepEqual(expected, actual) {
+				return fmt.Errorf("bad policy, expected: %#v, got %#v", expected, actual)
+			}
+		}
+
+		return nil
+	}
+}
+
 func testAccBucketRegionalDomainName(bucket, region string) string {
 	regionalEndpoint, err := tfs3.BucketRegionalDomainName(bucket, region)
 	if err != nil {
@@ -2764,6 +2870,23 @@ func testAccCheckBucketCheckTags(n string, expectedTags map[string]string) resou
 	}
 }
 
+func testAccBucketPolicy(bucketName, partition string) string {
+	return fmt.Sprintf(`{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "*"
+      },
+      "Action": "s3:GetObject",
+      "Resource": "arn:%[1]s:s3:::%[2]s/*"
+    }
+  ]
+}`, partition, bucketName)
+}
+
 func testAccBucketConfig_Basic(bucketName string) string {
 	return fmt.Sprintf(`
 resource "aws_s3_bucket" "test" {
@@ -3110,6 +3233,26 @@ resource "aws_s3_bucket" "test" {
 `, bucketName)
 }
 
+func testAccBucketConfig_withEmptyPolicy(bucketName string) string {
+	return fmt.Sprintf(`
+resource "aws_s3_bucket" "test" {
+  bucket = %[1]q
+  acl    = "private"
+  policy = ""
+}
+`, bucketName)
+}
+
+func testAccBucketConfig_withPolicy(bucketName, partition string) string {
+	return fmt.Sprintf(`
+resource "aws_s3_bucket" "test" {
+  bucket = %[1]q
+  acl    = "private"
+  policy = %[2]s
+}
+`, bucketName, strconv.Quote(testAccBucketPolicy(bucketName, partition)))
+}
+
 func testAccBucketConfig_ReplicationBase(bucketName string) string {
 	return acctest.ConfigCompose(
 		acctest.ConfigMultipleRegionProvider(2),