r/s3_bucket: read-only policy argument (#22538)

* Add option to allow no results when querying for instances data * Revert "Add option to allow no results when querying for instances data" This reverts commit 9c81e9d. * feat: deprecate 'policy' argument * golanci-lint fix and update doc ref to policy * update S3 bucket policy usage in documentation * update S3 bucket policy usage in tst * tests/elb: add bucket policy dependencies * update docs policy usage * Update .changelog/22538.txt * set policy with nil if dne for bucket * add instructions for breaking change introduced in #22538 Co-authored-by: Matt Cooper <matthew.cooper@fairfaxmedia.co.nz> Co-authored-by: Kit Ewbank <Kit_Ewbank@hotmail.com>
hashicorp · Feb 7, 2022 · ad693b9 · ad693b9
1 parent 8a279c1
commit ad693b9
Show file tree

Hide file tree

Showing 11 changed files with 129 additions and 259 deletions.
diff --git a/.changelog/22538.txt b/.changelog/22538.txt
@@ -0,0 +1,3 @@
+```release-note:breaking-change
+resource/aws_s3_bucket: The `policy` argument has been deprecated and is now read-only. Use the `aws_s3_bucket_policy` resource instead.
+```
diff --git a/internal/service/elb/load_balancer_test.go b/internal/service/elb/load_balancer_test.go
@@ -1182,6 +1182,9 @@ resource "aws_elb" "test" {
 func testAccLoadBalancerAccessLogsOn(r string) string {
 	return `
 resource "aws_elb" "test" {
+  # Must have bucket policy attached first
+  depends_on = [aws_s3_bucket_policy.test]
+
   availability_zones = [data.aws_availability_zones.available.names[0], data.aws_availability_zones.available.names[1], data.aws_availability_zones.available.names[2]]
 
   listener {
@@ -1211,6 +1214,9 @@ data "aws_availability_zones" "available" {
 func testAccLoadBalancerAccessLogsDisabled(r string) string {
 	return `
 resource "aws_elb" "test" {
+  # Must have bucket policy attached first
+  depends_on = [aws_s3_bucket_policy.test]
+
   availability_zones = [data.aws_availability_zones.available.names[0], data.aws_availability_zones.available.names[1], data.aws_availability_zones.available.names[2]]
 
   listener {
@@ -1240,17 +1246,18 @@ data "aws_availability_zones" "available" {
 
 func testAccLoadBalancerAccessLogsCommon(r string) string {
 	return fmt.Sprintf(`
-data "aws_elb_service_account" "current" {
-}
+data "aws_elb_service_account" "current" {}
 
-data "aws_partition" "current" {
-}
+data "aws_partition" "current" {}
 
 resource "aws_s3_bucket" "accesslogs_bucket" {
   bucket        = "%[1]s"
   acl           = "private"
   force_destroy = true
+}
 
+resource "aws_s3_bucket_policy" "test" {
+  bucket = aws_s3_bucket.accesslogs_bucket.id
   policy = <<EOF
 {
   "Id": "Policy1446577137248",

diff --git a/internal/service/s3/bucket.go b/internal/service/s3/bucket.go
@@ -22,7 +22,6 @@ import (
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
-	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 	"github.com/hashicorp/terraform-provider-aws/internal/conns"
 	"github.com/hashicorp/terraform-provider-aws/internal/create"
@@ -122,10 +121,9 @@ func ResourceBucket() *schema.Resource {
 			},
 
 			"policy": {
-				Type:             schema.TypeString,
-				Optional:         true,
-				ValidateFunc:     validation.StringIsJSON,
-				DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs,
+				Type:       schema.TypeString,
+				Computed:   true,
+				Deprecated: "Use the aws_s3_bucket_policy resource instead",
 			},
 
 			"cors_rule": {
@@ -749,12 +747,6 @@ func resourceBucketUpdate(d *schema.ResourceData, meta interface{}) error {
 		}
 	}
 
-	if d.HasChange("policy") {
-		if err := resourceBucketPolicyUpdate(conn, d); err != nil {
-			return err
-		}
-	}
-
 	if d.HasChange("versioning") {
 		v := d.Get("versioning").([]interface{})
 
@@ -853,40 +845,21 @@ func resourceBucketRead(d *schema.ResourceData, meta interface{}) error {
 
 	d.Set("bucket_domain_name", meta.(*conns.AWSClient).PartitionHostname(fmt.Sprintf("%s.s3", d.Get("bucket").(string))))
 
-	// Read the policy
-	if _, ok := d.GetOk("policy"); ok {
-
-		pol, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
-			return conn.GetBucketPolicy(&s3.GetBucketPolicyInput{
-				Bucket: aws.String(d.Id()),
-			})
+	// Read the policy if configured outside this resource e.g. with aws_s3_bucket_policy resource
+	pol, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
+		return conn.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.(*s3.GetBucketPolicyOutput).Policy; v == nil {
-				if err := d.Set("policy", ""); err != nil {
-					return err
-				}
-			} else {
-				policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), aws.StringValue(v))
-
-				if err != nil {
-					return fmt.Errorf("while setting policy (%s), encountered: %w", aws.StringValue(v), err)
-				}
-
-				policyToSet, err = structure.NormalizeJsonString(policyToSet)
+	})
 
-				if err != nil {
-					return fmt.Errorf("policy (%s) contains invalid JSON: %w", d.Get("policy").(string), err)
-				}
+	if err != nil && !tfawserr.ErrCodeEquals(err, ErrCodeNoSuchBucketPolicy) {
+		return fmt.Errorf("error getting S3 bucket (%s) policy: %w", d.Id(), err)
+	}
 
-				d.Set("policy", policyToSet)
-			}
-		}
+	if output, ok := pol.(*s3.GetBucketPolicyOutput); ok {
+		d.Set("policy", output.Policy)
+	} else {
+		d.Set("policy", nil)
 	}
 
 	//Read the Grant ACL. Reset if `acl` (canned ACL) is set.
@@ -1344,55 +1317,6 @@ func resourceBucketDelete(d *schema.ResourceData, meta interface{}) error {
 	return nil
 }
 
-func resourceBucketPolicyUpdate(conn *s3.S3, d *schema.ResourceData) error {
-	bucket := d.Get("bucket").(string)
-
-	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 != "" {
-		log.Printf("[DEBUG] S3 bucket: %s, put policy: %s", bucket, policy)
-
-		params := &s3.PutBucketPolicyInput{
-			Bucket: aws.String(bucket),
-			Policy: aws.String(policy),
-		}
-
-		err := resource.Retry(1*time.Minute, func() *resource.RetryError {
-			_, err := conn.PutBucketPolicy(params)
-			if tfawserr.ErrMessageContains(err, "MalformedPolicy", "") || tfawserr.ErrMessageContains(err, s3.ErrCodeNoSuchBucket, "") {
-				return resource.RetryableError(err)
-			}
-			if err != nil {
-				return resource.NonRetryableError(err)
-			}
-			return nil
-		})
-		if tfresource.TimedOut(err) {
-			_, err = conn.PutBucketPolicy(params)
-		}
-		if err != nil {
-			return fmt.Errorf("Error putting S3 policy: %s", err)
-		}
-	} else {
-		log.Printf("[DEBUG] S3 bucket: %s, delete policy: %s", bucket, policy)
-		_, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
-			return conn.DeleteBucketPolicy(&s3.DeleteBucketPolicyInput{
-				Bucket: aws.String(bucket),
-			})
-		})
-
-		if err != nil {
-			return fmt.Errorf("Error deleting S3 policy: %s", err)
-		}
-	}
-
-	return nil
-}
-
 func resourceBucketGrantsUpdate(conn *s3.S3, d *schema.ResourceData) error {
 	bucket := d.Get("bucket").(string)
 	rawGrants := d.Get("grant").(*schema.Set).List()