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

r/s3_bucket: read-only acl and grant arguments #22537

Merged
merged 11 commits into from
Feb 7, 2022
Merged

r/s3_bucket: read-only acl and grant arguments #22537

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d8a010d
r/s3_bucket: deprecate 'acl' and 'grant' arguments
anGie44 Feb 4, 2022
48a6b95
Update CHANGELOG for #22537
anGie44 Feb 4, 2022
75c9586
update s3 bucket tests
anGie44 Feb 4, 2022
8a04b26
Update website/docs/r/s3_bucket.html.markdown
anGie44 Feb 5, 2022
61cbb7f
remove TODO's
anGie44 Feb 5, 2022
07139cc
remove lifecycle.ignore_changes
anGie44 Feb 5, 2022
e3ef95d
remove lifecycle.ignore_changes from s3_bucket_acl tests
anGie44 Feb 5, 2022
965d307
add instructions for breaking change introduced in #22537
anGie44 Feb 7, 2022
7ff2f3d
remove one more 'acl' from import step in test file
anGie44 Feb 7, 2022
30f7e80
Fix 'testAccBucketConfig_objectLockEnabledWithVersioning' to use 'aws…
ewbankkit Feb 7, 2022
07a57b6
add not-found handling after Get calls during read of non-new buckets
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/22537.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 `acl` and `grant` arguments have been deprecated and are now read-only. Use the `aws_s3_bucket_acl` resource instead.
```
197 changes: 48 additions & 149 deletions internal/service/s3/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,46 +75,38 @@ func ResourceBucket() *schema.Resource {
},

"acl": {
Type: schema.TypeString,
Default: "private",
Optional: true,
ConflictsWith: []string{"grant"},
ValidateFunc: validation.StringInSlice(BucketCannedACL_Values(), false),
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_acl resource instead",
},

"grant": {
Type: schema.TypeSet,
Optional: true,
Set: grantHash,
ConflictsWith: []string{"acl"},
Type: schema.TypeSet,
Computed: true,
Deprecated: "Use the aws_s3_bucket_acl resource instead",
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"id": {
Type: schema.TypeString,
Optional: true,
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_acl resource instead",
},
"type": {
Type: schema.TypeString,
Required: true,
// TypeAmazonCustomerByEmail is not currently supported
ValidateFunc: validation.StringInSlice([]string{
s3.TypeCanonicalUser,
s3.TypeGroup,
}, false),
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_acl resource instead",
},
"uri": {
Type: schema.TypeString,
Optional: true,
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_acl resource instead",
},

"permissions": {
Type: schema.TypeSet,
Required: true,
Set: schema.HashString,
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validation.StringInSlice(s3.Permission_Values(), false),
},
Type: schema.TypeSet,
Computed: true,
Deprecated: "Use the aws_s3_bucket_acl resource instead",
Elem: &schema.Schema{Type: schema.TypeString},
},
},
},
Expand Down Expand Up @@ -168,31 +160,31 @@ func ResourceBucket() *schema.Resource {
"website": {
Type: schema.TypeList,
Computed: true,
Deprecated: "Use the aws_s3_bucket_website_configuration resource instead when available in a future minor version",
Deprecated: "Use the aws_s3_bucket_website_configuration resource",
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"index_document": {
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_website_configuration resource instead when available in a future minor version",
Deprecated: "Use the aws_s3_bucket_website_configuration resource",
},

"error_document": {
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_website_configuration resource instead when available in a future minor version",
Deprecated: "Use the aws_s3_bucket_website_configuration resource",
},

"redirect_all_requests_to": {
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_website_configuration resource instead when available in a future minor version",
Deprecated: "Use the aws_s3_bucket_website_configuration resource",
},

"routing_rules": {
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_website_configuration resource instead when available in a future minor version",
Deprecated: "Use the aws_s3_bucket_website_configuration resource",
},
},
},
Expand All @@ -211,12 +203,12 @@ func ResourceBucket() *schema.Resource {
"website_endpoint": {
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_website_configuration resource instead when available in a future minor version",
Deprecated: "Use the aws_s3_bucket_website_configuration resource",
},
"website_domain": {
Type: schema.TypeString,
Computed: true,
Deprecated: "Use the aws_s3_bucket_website_configuration resource instead when available in a future minor version",
Deprecated: "Use the aws_s3_bucket_website_configuration resource",
},

"versioning": {
Expand Down Expand Up @@ -681,12 +673,6 @@ func resourceBucketCreate(d *schema.ResourceData, meta interface{}) error {
Bucket: aws.String(bucket),
}

if acl, ok := d.GetOk("acl"); ok {
acl := acl.(string)
req.ACL = aws.String(acl)
log.Printf("[DEBUG] S3 bucket %s has canned ACL %s", bucket, acl)
}

awsRegion := meta.(*conns.AWSClient).Region
log.Printf("[DEBUG] S3 bucket create: %s, using region: %s", bucket, awsRegion)

Expand Down Expand Up @@ -750,18 +736,6 @@ func resourceBucketUpdate(d *schema.ResourceData, meta interface{}) error {
}
}

if d.HasChange("acl") && !d.IsNewResource() {
if err := resourceBucketACLUpdate(conn, d); err != nil {
return err
}
}

if d.HasChange("grant") {
if err := resourceBucketGrantsUpdate(conn, d); err != nil {
return err
}
}

if d.HasChange("object_lock_configuration") {
if err := resourceBucketInternalObjectLockConfigurationUpdate(conn, d); err != nil {
return err
Expand Down Expand Up @@ -842,24 +816,34 @@ func resourceBucketRead(d *schema.ResourceData, meta interface{}) error {
d.Set("policy", nil)
}

//Read the Grant ACL. Reset if `acl` (canned ACL) is set.
if acl, ok := d.GetOk("acl"); ok && acl.(string) != "private" {
// Read the Grant ACL if configured outside this resource;
// In the event grants are not configured on the bucket, the API returns an empty array

// Reset `grant` if `acl` (canned ACL) is set.
if acl, ok := d.GetOk("acl"); ok && acl.(string) != s3.BucketCannedACLPrivate {
if err := d.Set("grant", nil); err != nil {
return fmt.Errorf("error resetting grant %s", err)
return fmt.Errorf("error resetting grant %w", err)
}
} else {
apResponse, err := verify.RetryOnAWSCode("NoSuchBucket", func() (interface{}, error) {
// Set the ACL to its default i.e. "private" (to mimic pre-v4.0 schema)
d.Set("acl", s3.BucketCannedACLPrivate)

apResponse, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
return conn.GetBucketAcl(&s3.GetBucketAclInput{
Bucket: aws.String(d.Id()),
})
})

if err != nil {
return fmt.Errorf("error getting S3 Bucket (%s) ACL: %s", d.Id(), err)
return fmt.Errorf("error getting S3 Bucket (%s) ACL: %w", d.Id(), err)
}
log.Printf("[DEBUG] S3 bucket: %s, read ACL grants policy: %+v", d.Id(), apResponse)
grants := flattenGrants(apResponse.(*s3.GetBucketAclOutput))
if err := d.Set("grant", schema.NewSet(grantHash, grants)); err != nil {
return fmt.Errorf("error setting grant %s", err)

if aclOutput, ok := apResponse.(*s3.GetBucketAclOutput); ok {
if err := d.Set("grant", flattenGrants(aclOutput)); err != nil {
return fmt.Errorf("error setting grant %s", err)
}
} else {
d.Set("grant", nil)
}
}

Expand Down Expand Up @@ -1195,74 +1179,6 @@ func resourceBucketDelete(d *schema.ResourceData, meta interface{}) error {
return nil
}

func resourceBucketGrantsUpdate(conn *s3.S3, d *schema.ResourceData) error {
bucket := d.Get("bucket").(string)
rawGrants := d.Get("grant").(*schema.Set).List()

if len(rawGrants) == 0 {
log.Printf("[DEBUG] S3 bucket: %s, Grants fallback to canned ACL", bucket)
if err := resourceBucketACLUpdate(conn, d); err != nil {
return fmt.Errorf("Error fallback to canned ACL, %s", err)
}
} else {
apResponse, err := verify.RetryOnAWSCode("NoSuchBucket", func() (interface{}, error) {
return conn.GetBucketAcl(&s3.GetBucketAclInput{
Bucket: aws.String(d.Id()),
})
})

if err != nil {
return fmt.Errorf("error getting S3 Bucket (%s) ACL: %s", d.Id(), err)
}

ap := apResponse.(*s3.GetBucketAclOutput)
log.Printf("[DEBUG] S3 bucket: %s, read ACL grants policy: %+v", d.Id(), ap)

grants := make([]*s3.Grant, 0, len(rawGrants))
for _, rawGrant := range rawGrants {
log.Printf("[DEBUG] S3 bucket: %s, put grant: %#v", bucket, rawGrant)
grantMap := rawGrant.(map[string]interface{})
for _, rawPermission := range grantMap["permissions"].(*schema.Set).List() {
ge := &s3.Grantee{}
if i, ok := grantMap["id"].(string); ok && i != "" {
ge.SetID(i)
}
if t, ok := grantMap["type"].(string); ok && t != "" {
ge.SetType(t)
}
if u, ok := grantMap["uri"].(string); ok && u != "" {
ge.SetURI(u)
}

g := &s3.Grant{
Grantee: ge,
Permission: aws.String(rawPermission.(string)),
}
grants = append(grants, g)
}
}

grantsInput := &s3.PutBucketAclInput{
Bucket: aws.String(bucket),
AccessControlPolicy: &s3.AccessControlPolicy{
Grants: grants,
Owner: ap.Owner,
},
}

log.Printf("[DEBUG] S3 bucket: %s, put Grants: %#v", bucket, grantsInput)

_, err = verify.RetryOnAWSCode("NoSuchBucket", func() (interface{}, error) {
return conn.PutBucketAcl(grantsInput)
})

if err != nil {
return fmt.Errorf("Error putting S3 Grants: %s", err)
}
}
return nil
}

func websiteEndpoint(client *conns.AWSClient, d *schema.ResourceData) (*S3Website, error) {
// If the bucket doesn't have a website configuration, return an empty
// endpoint
Expand Down Expand Up @@ -1344,26 +1260,6 @@ func isOldRegion(region string) bool {
return false
}

func resourceBucketACLUpdate(conn *s3.S3, d *schema.ResourceData) error {
acl := d.Get("acl").(string)
bucket := d.Get("bucket").(string)

i := &s3.PutBucketAclInput{
Bucket: aws.String(bucket),
ACL: aws.String(acl),
}
log.Printf("[DEBUG] S3 put bucket ACL: %#v", i)

_, err := verify.RetryOnAWSCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
return conn.PutBucketAcl(i)
})
if err != nil {
return fmt.Errorf("Error putting S3 ACL: %s", err)
}

return nil
}

func resourceBucketInternalObjectLockConfigurationUpdate(conn *s3.S3, d *schema.ResourceData) error {
// S3 Object Lock configuration cannot be deleted, only updated.
req := &s3.PutObjectLockConfigurationInput{
Expand Down Expand Up @@ -2212,6 +2108,9 @@ func flattenS3ObjectLockConfiguration(conf *s3.ObjectLockConfiguration) []interf
}

func flattenGrants(ap *s3.GetBucketAclOutput) []interface{} {
if len(ap.Grants) == 0 {
return []interface{}{}
}
//if ACL grants contains bucket owner FULL_CONTROL only - it is default "private" acl
if len(ap.Grants) == 1 && aws.StringValue(ap.Grants[0].Grantee.ID) == aws.StringValue(ap.Owner.ID) &&
aws.StringValue(ap.Grants[0].Permission) == s3.PermissionFullControl {
Expand Down
18 changes: 0 additions & 18 deletions internal/service/s3/bucket_acl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,12 +435,6 @@ func testAccBucketAclBasicConfig(rName, acl string) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "test" {
bucket = %[1]q

lifecycle {
ignore_changes = [
grant,
]
}
}

resource "aws_s3_bucket_acl" "test" {
Expand All @@ -456,12 +450,6 @@ data "aws_canonical_user_id" "current" {}

resource "aws_s3_bucket" "test" {
bucket = %[1]q

lifecycle {
ignore_changes = [
grant,
]
}
}

resource "aws_s3_bucket_acl" "test" {
Expand Down Expand Up @@ -499,12 +487,6 @@ data "aws_partition" "current" {}

resource "aws_s3_bucket" "test" {
bucket = %[1]q

lifecycle {
ignore_changes = [
grant,
]
}
}

resource "aws_s3_bucket_acl" "test" {
Expand Down
Loading