Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

r/s3_bucket: read-only policy argument #22538

Merged
merged 11 commits into from
Feb 7, 2022
Merged

r/s3_bucket: read-only policy argument #22538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1dbc21e
Add option to allow no results when querying for instances data
Jul 2, 2018
35664f2
Revert "Add option to allow no results when querying for instances data"
ewbankkit Jan 25, 2022
7f869d9
feat: deprecate 'policy' argument
anGie44 Jan 14, 2022
0070bcd
golanci-lint fix and update doc ref to policy
anGie44 Jan 14, 2022
8d0d7dd
update S3 bucket policy usage in documentation
anGie44 Jan 26, 2022
a0724e7
update S3 bucket policy usage in tst
anGie44 Jan 27, 2022
09bbbfa
tests/elb: add bucket policy dependencies
anGie44 Jan 27, 2022
4d47e81
update docs policy usage
anGie44 Jan 27, 2022
2039be4
Update .changelog/22538.txt
anGie44 Feb 2, 2022
64d263d
set policy with nil if dne for bucket
anGie44 Feb 6, 2022
dfbac43
add instructions for breaking change introduced in #22538
anGie44 Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/22538.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
```
15 changes: 11 additions & 4 deletions internal/service/elb/load_balancer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand Down
106 changes: 15 additions & 91 deletions internal/service/s3/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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": {
Expand Down Expand Up @@ -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{})

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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()
Expand Down
Loading