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

Implementation of acl grants #3728

Merged
merged 11 commits into from
Mar 4, 2020
Merged

Implementation of acl grants #3728

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6549e72
Implementation of acl grants and update in docs
Mar 9, 2018
62694d8
Merge branch 'master' into support-s3-bucket-acl-policies
Chhed13 Jan 29, 2019
844ef9b
Error check on fallback
Feb 1, 2019
8d95f64
Merge pull request #1 from terraform-providers/master
Chhed13 Mar 28, 2019
4c25528
Merge branch 'master' into support-s3-bucket-acl-policies
Mar 28, 2019
85cb44f
change grant to schema.HashSet type, re-evaluate grant logic, fix min…
Mar 31, 2019
10a70c4
fix docs
Mar 31, 2019
93ce16a
removed unused validator
Mar 31, 2019
51717d4
Merge remote-tracking branch 'upstream/master' into support-s3-bucket…
Chhed13 Feb 12, 2020
a5638e4
s3 bucket grant implementation: fix docs and tests
Chhed13 Feb 24, 2020
b0c2939
s3 bucket grant implementation: fix fmt
Chhed13 Mar 4, 2020
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
213 changes: 207 additions & 6 deletions aws/resource_aws_s3_bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,53 @@ func resourceAwsS3Bucket() *schema.Resource {
},

"acl": {
Type: schema.TypeString,
Default: "private",
Optional: true,
Type: schema.TypeString,
Default: "private",
Optional: true,
ConflictsWith: []string{"grant"},
},

"grant": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent issues with setting the state during Read and other unforeseen issues with the initial implementation, I would recommend adding Computed: true to this attribute for now. We can always adjust this in the future and add a note in the documentation for now, e.g. To enable drift detection for this configuration, it must be configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Computed: true leads to non-tacking in case of removal from TF resource, is it really needed?
It will work only if it was set explicitly, I don't see the way to track it always now (answered blow).
Of course I can miss some, but on tests Computed: true make the situation worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Day passed and I understand the idea of using Computed: true and always save the state, it will work. And if you'll say that it is better way - I change the PR to this version.
The only thing I miss - that it will no the way back from computed, because it will make a lot of changes in state files

Type: schema.TypeSet,
Optional: true,
Set: grantHash,
ConflictsWith: []string{"acl"},
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"id": {
Type: schema.TypeString,
Optional: true,
},
"type": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validation.StringInSlice([]string{
s3.TypeCanonicalUser,
s3.TypeGroup,
}, false),
},
"uri": {
Type: schema.TypeString,
Optional: true,
},

"permissions": {
Type: schema.TypeSet,
Required: true,
Set: schema.HashString,
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validation.StringInSlice([]string{
s3.PermissionFullControl,
s3.PermissionRead,
s3.PermissionReadAcp,
s3.PermissionWrite,
s3.PermissionWriteAcp,
}, false),
},
},
},
},
},

"policy": {
Expand Down Expand Up @@ -614,13 +658,17 @@ func resourceAwsS3BucketCreate(d *schema.ResourceData, meta interface{}) error {
bucket = resource.UniqueId()
}
d.Set("bucket", bucket)
acl := d.Get("acl").(string)

log.Printf("[DEBUG] S3 bucket create: %s, ACL: %s", bucket, acl)
log.Printf("[DEBUG] S3 bucket create: %s", bucket)

req := &s3.CreateBucketInput{
Bucket: aws.String(bucket),
ACL: aws.String(acl),
}

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)
}

var awsRegion string
Expand Down Expand Up @@ -717,6 +765,12 @@ func resourceAwsS3BucketUpdate(d *schema.ResourceData, meta interface{}) error {
}
}

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

if d.HasChange("logging") {
if err := resourceAwsS3BucketLoggingUpdate(s3conn, d); err != nil {
return err
Expand Down Expand Up @@ -836,6 +890,27 @@ func resourceAwsS3BucketRead(d *schema.ResourceData, meta interface{}) error {
}
}

//Read the Grant ACL. Reset if `acl` (canned ACL) is set.
if acl, ok := d.GetOk("acl"); ok && acl.(string) != "private" {
if err := d.Set("grant", nil); err != nil {
return fmt.Errorf("error resetting grant %s", err)
}
} else {
apResponse, err := retryOnAwsCode("NoSuchBucket", func() (interface{}, error) {
return s3conn.GetBucketAcl(&s3.GetBucketAclInput{
Bucket: aws.String(d.Id()),
})
})
if err != nil {
return fmt.Errorf("error getting S3 Bucket (%s) ACL: %s", 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)
}
}

// Read the CORS
corsResponse, err := retryOnAwsCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) {
return s3conn.GetBucketCors(&s3.GetBucketCorsInput{
Expand Down Expand Up @@ -1365,6 +1440,74 @@ func resourceAwsS3BucketPolicyUpdate(s3conn *s3.S3, d *schema.ResourceData) erro
return nil
}

func resourceAwsS3BucketGrantsUpdate(s3conn *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 := resourceAwsS3BucketAclUpdate(s3conn, d); err != nil {
return fmt.Errorf("Error fallback to canned ACL, %s", err)
}
} else {
apResponse, err := retryOnAwsCode("NoSuchBucket", func() (interface{}, error) {
return s3conn.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 = retryOnAwsCode("NoSuchBucket", func() (interface{}, error) {
return s3conn.PutBucketAcl(grantsInput)
})

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

func resourceAwsS3BucketCorsUpdate(s3conn *s3.S3, d *schema.ResourceData) error {
bucket := d.Get("bucket").(string)
rawCors := d.Get("cors_rule").([]interface{})
Expand Down Expand Up @@ -2339,6 +2482,24 @@ func validateS3BucketName(value string, region string) error {
return nil
}

func grantHash(v interface{}) int {
var buf bytes.Buffer
m := v.(map[string]interface{})
if v, ok := m["id"]; ok {
buf.WriteString(fmt.Sprintf("%s-", v.(string)))
}
if v, ok := m["type"]; ok {
buf.WriteString(fmt.Sprintf("%s-", v.(string)))
}
if v, ok := m["uri"]; ok {
buf.WriteString(fmt.Sprintf("%s-", v.(string)))
}
if p, ok := m["permissions"]; ok {
buf.WriteString(fmt.Sprintf("%v-", p.(*schema.Set).List()))
}
return hashcode.String(buf.String())
}

func expirationHash(v interface{}) int {
var buf bytes.Buffer
m := v.(map[string]interface{})
Expand Down Expand Up @@ -2561,3 +2722,43 @@ func flattenS3ObjectLockConfiguration(conf *s3.ObjectLockConfiguration) []interf

return []interface{}{mConf}
}

func flattenGrants(ap *s3.GetBucketAclOutput) []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 {
return nil
}

getGrant := func(grants []interface{}, grantee map[string]interface{}) (interface{}, bool) {
for _, pg := range grants {
pgt := pg.(map[string]interface{})
if pgt["type"] == grantee["type"] && pgt["id"] == grantee["id"] && pgt["uri"] == grantee["uri"] &&
pgt["permissions"].(*schema.Set).Len() > 0 {
return pg, true
}
}
return nil, false
}

grants := make([]interface{}, 0, len(ap.Grants))
for _, granteeObject := range ap.Grants {
grantee := make(map[string]interface{})
grantee["type"] = aws.StringValue(granteeObject.Grantee.Type)

if granteeObject.Grantee.ID != nil {
grantee["id"] = aws.StringValue(granteeObject.Grantee.ID)
}
if granteeObject.Grantee.URI != nil {
grantee["uri"] = aws.StringValue(granteeObject.Grantee.URI)
}
if pg, ok := getGrant(grants, grantee); ok {
pg.(map[string]interface{})["permissions"].(*schema.Set).Add(aws.StringValue(granteeObject.Permission))
} else {
grantee["permissions"] = schema.NewSet(schema.HashString, []interface{}{aws.StringValue(granteeObject.Permission)})
grants = append(grants, grantee)
}
}

return grants
}
84 changes: 84 additions & 0 deletions aws/resource_aws_s3_bucket_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,49 @@ func TestAccAWSS3Bucket_UpdateAcl(t *testing.T) {
})
}

func TestAccAWSS3Bucket_UpdateGrant(t *testing.T) {
Chhed13 marked this conversation as resolved.
Show resolved Hide resolved
ri := acctest.RandInt()
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSS3BucketDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSS3BucketConfigWithGrants(ri),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSS3BucketExists("aws_s3_bucket.bucket"),
Chhed13 marked this conversation as resolved.
Show resolved Hide resolved
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.#", "1"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.2524447860.permissions.#", "2"),
Chhed13 marked this conversation as resolved.
Show resolved Hide resolved
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.2524447860.permissions.3535167073", "FULL_CONTROL"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.2524447860.permissions.2319431919", "WRITE"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.2524447860.type", "CanonicalUser"),
),
},
Chhed13 marked this conversation as resolved.
Show resolved Hide resolved
{
Config: testAccAWSS3BucketConfigWithGrantsUpdate(ri),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSS3BucketExists("aws_s3_bucket.bucket"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.#", "2"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.2344010500.permissions.#", "1"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.2344010500.permissions.2931993811", "READ"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.2344010500.type", "CanonicalUser"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.986027168.permissions.#", "1"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.986027168.permissions.1600971645", "READ_ACP"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.986027168.type", "Group"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.986027168.uri", "http://acs.amazonaws.com/groups/s3/LogDelivery"),
),
},
{
Config: testAccAWSS3BucketBasic(ri),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSS3BucketExists("aws_s3_bucket.bucket"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "grant.#", "0"),
),
},
},
})
}

func TestAccAWSS3Bucket_Website_Simple(t *testing.T) {
rInt := acctest.RandInt()
region := testAccGetRegion()
Expand Down Expand Up @@ -2651,6 +2694,14 @@ resource "aws_s3_bucket" "bucket" {
`, randInt)
}

func testAccAWSS3BucketBasic(randInt int) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "bucket" {
bucket = "tf-test-bucket-%d"
}
`, randInt)
}

func testAccAWSS3BucketConfig_withNoTags(bucketName string) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "bucket" {
Expand Down Expand Up @@ -3060,6 +3111,39 @@ resource "aws_s3_bucket" "bucket" {
}
`

func testAccAWSS3BucketConfigWithGrants(randInt int) string {
return fmt.Sprintf(`
data "aws_canonical_user_id" "current" {}
resource "aws_s3_bucket" "bucket" {
bucket = "tf-test-bucket-%d"
grant {
id = "${data.aws_canonical_user_id.current.id}"
type = "CanonicalUser"
permissions = ["FULL_CONTROL", "WRITE"]
}
}
`, randInt)
}

func testAccAWSS3BucketConfigWithGrantsUpdate(randInt int) string {
return fmt.Sprintf(`
data "aws_canonical_user_id" "current" {}
resource "aws_s3_bucket" "bucket" {
bucket = "tf-test-bucket-%d"
grant {
id = "${data.aws_canonical_user_id.current.id}"
type = "CanonicalUser"
permissions = ["READ"]
}
grant {
type = "Group"
permissions = ["READ_ACP"]
uri = "http://acs.amazonaws.com/groups/s3/LogDelivery"
}
}
`, randInt)
}

func testAccAWSS3BucketConfigWithLogging(randInt int) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "log_bucket" {
Expand Down
Loading