From f1083090f64c1264dab9af06b7f28e0ca9ffe09c Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Thu, 31 Mar 2022 23:27:35 -0400 Subject: [PATCH] r/s3_bucket: make 'policy' configurable --- internal/service/s3/bucket.go | 59 ++++++++- internal/service/s3/bucket_policy_test.go | 114 +++++++++++++++++ internal/service/s3/bucket_test.go | 143 ++++++++++++++++++++++ internal/service/s3/errors.go | 1 + website/docs/r/s3_bucket.html.markdown | 8 +- 5 files changed, 321 insertions(+), 4 deletions(-) diff --git a/internal/service/s3/bucket.go b/internal/service/s3/bucket.go index 155e81ef959..6e5c251ee82 100644 --- a/internal/service/s3/bucket.go +++ b/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{}) diff --git a/internal/service/s3/bucket_policy_test.go b/internal/service/s3/bucket_policy_test.go index f31c0f9e474..60f05ace8d7 100644 --- a/internal/service/s3/bucket_policy_test.go +++ b/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) +} diff --git a/internal/service/s3/bucket_test.go b/internal/service/s3/bucket_test.go index 30b2ed78904..ece924edfa3 100644 --- a/internal/service/s3/bucket_test.go +++ b/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), diff --git a/internal/service/s3/errors.go b/internal/service/s3/errors.go index 7669a63320c..40d27fd05bd 100644 --- a/internal/service/s3/errors.go +++ b/internal/service/s3/errors.go @@ -6,6 +6,7 @@ package s3 const ( ErrCodeInvalidBucketState = "InvalidBucketState" ErrCodeInvalidRequest = "InvalidRequest" + ErrCodeMalformedPolicy = "MalformedPolicy" ErrCodeMethodNotAllowed = "MethodNotAllowed" ErrCodeNoSuchBucketPolicy = "NoSuchBucketPolicy" ErrCodeNoSuchConfiguration = "NoSuchConfiguration" diff --git a/website/docs/r/s3_bucket.html.markdown b/website/docs/r/s3_bucket.html.markdown index dad86620d51..1e9afcec8ff 100644 --- a/website/docs/r/s3_bucket.html.markdown +++ b/website/docs/r/s3_bucket.html.markdown @@ -36,6 +36,10 @@ Configuring with both will cause inconsistencies and may overwrite configuration or with the deprecated parameter `logging` in the resource `aws_s3_bucket`. Configuring with both will cause inconsistencies and may overwrite configuration. +~> **NOTE on S3 Bucket Policy Configuration:** S3 Bucket Policy can be configured in either the standalone resource [`aws_s3_bucket_policy`](s3_bucket_policy.html) +or with the deprecated parameter `policy` in the resource `aws_s3_bucket`. +Configuring with both will cause inconsistencies and may overwrite configuration. + ~> **NOTE on S3 Bucket Replication Configuration:** S3 Bucket Replication can be configured in either the standalone resource [`aws_s3_bucket_replicaton_configuration`](s3_bucket_replication_configuration.html) or with the deprecated parameter `replication_configuration` in the resource `aws_s3_bucket`. Configuring with both will cause inconsistencies and may overwrite configuration. @@ -438,6 +442,9 @@ The following arguments are supported: Use the resource [`aws_s3_bucket_logging`](s3_bucket_logging.html.markdown) instead. * `object_lock_enabled` - (Optional, Default:`false`, Forces new resource) Indicates whether this bucket has an Object Lock configuration enabled. * `object_lock_configuration` - (Optional) A configuration of [S3 object locking](https://docs.aws.amazon.com/AmazonS3/latest/dev/object-lock.html). See [Object Lock Configuration](#object-lock-configuration) below. +* `policy` - (Optional, **Deprecated**) A valid [bucket policy](https://docs.aws.amazon.com/AmazonS3/latest/dev/example-bucket-policies.html) JSON document. Note that if the policy document is not specific enough (but still valid), Terraform may view the policy as constantly changing in a `terraform plan`. In this case, please make sure you use the verbose/specific version of the policy. For more information about building AWS IAM policy documents with Terraform, see the [AWS IAM Policy Document Guide](https://learn.hashicorp.com/terraform/aws/iam-policy). + Terraform will only perform drift detection if a configuration value is provided. + Use the resource [`aws_s3_bucket_policy`](s3_bucket_policy.html) instead. * `replication_configuration` - (Optional, **Deprecated**) A configuration of [replication configuration](http://docs.aws.amazon.com/AmazonS3/latest/dev/crr.html). See [Replication Configuration](#replication-configuration) below for details. Terraform will only perform drift detection if a configuration value is provided. Use the resource [`aws_s3_bucket_replication_configuration`](s3_bucket_replication_configuration.html) instead. * `server_side_encryption_configuration` - (Optional, **Deprecated**) A configuration of [server-side encryption configuration](http://docs.aws.amazon.com/AmazonS3/latest/dev/bucket-encryption.html). See [Server Side Encryption Configuration](#server-side-encryption-configuration) below for details. @@ -668,7 +675,6 @@ In addition to all arguments above, the following attributes are exported: * `mode` - The default Object Lock retention mode applied to new objects placed in this bucket. * `days` - The number of days specified for the default retention period. * `years` - The number of years specified for the default retention period. -* `policy` - The [bucket policy](https://docs.aws.amazon.com/AmazonS3/latest/dev/example-bucket-policies.html) JSON document. * `region` - The AWS region this bucket resides in. * `request_payer` - Either `BucketOwner` or `Requester` that pays for the download and request fees. * `tags_all` - A map of tags assigned to the resource, including those inherited from the provider [`default_tags` configuration block](/docs/providers/aws/index.html#default_tags-configuration-block).