-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Add google_storage_default_object_acl resource #992
Changes from 6 commits
ba3c60c
19912ec
6810e53
2c80aee
58c7e0e
e7cebf7
5e35381
c702a73
bca2b1b
a89bc5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,199 @@ | ||
package google | ||
|
||
import ( | ||
"fmt" | ||
"log" | ||
|
||
"github.com/hashicorp/terraform/helper/schema" | ||
"google.golang.org/api/storage/v1" | ||
) | ||
|
||
func resourceStorageDefaultObjectAcl() *schema.Resource { | ||
return &schema.Resource{ | ||
Create: resourceStorageDefaultObjectAclCreate, | ||
Read: resourceStorageDefaultObjectAclRead, | ||
Update: resourceStorageDefaultObjectAclUpdate, | ||
Delete: resourceStorageDefaultObjectAclDelete, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"bucket": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
}, | ||
|
||
"role_entity": &schema.Schema{ | ||
Type: schema.TypeList, | ||
Required: true, | ||
Elem: &schema.Schema{Type: schema.TypeString}, | ||
}, | ||
}, | ||
} | ||
} | ||
|
||
func getDefaultObjectAclId(bucket string) string { | ||
return bucket + "-default-object-acl" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That suffix isn't really necessary |
||
} | ||
|
||
func resourceStorageDefaultObjectAclCreate(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*Config) | ||
|
||
bucket := d.Get("bucket").(string) | ||
role_entity := make([]interface{}, 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: use mixedCaps please for variables (https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll fix the variable naming in the next review, once I get your thoughts on the comment above |
||
|
||
if v, ok := d.GetOk("role_entity"); ok { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can just do |
||
role_entity = v.([]interface{}) | ||
} | ||
|
||
if len(role_entity) > 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's do MinItems: 1 on the schema element for this and then remove this check |
||
for _, v := range role_entity { | ||
pair, err := getRoleEntityPair(v.(string)) | ||
|
||
ObjectAccessControl := &storage.ObjectAccessControl{ | ||
Role: pair.Role, | ||
Entity: pair.Entity, | ||
} | ||
|
||
log.Printf("[DEBUG]: setting role = %s, entity = %s on bucket %s", pair.Role, pair.Entity, bucket) | ||
|
||
_, err = config.clientStorage.DefaultObjectAccessControls.Insert(bucket, ObjectAccessControl).Do() | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error setting Default Object ACL for %s on bucket %s: %v", pair.Entity, bucket, err) | ||
} | ||
} | ||
|
||
return resourceStorageDefaultObjectAclRead(d, meta) | ||
} | ||
return nil | ||
} | ||
|
||
func resourceStorageDefaultObjectAclRead(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*Config) | ||
|
||
bucket := d.Get("bucket").(string) | ||
|
||
if _, ok := d.GetOk("role_entity"); ok { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
role_entity := make([]interface{}, 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: roleEntities since it's a list? |
||
re_local := d.Get("role_entity").([]interface{}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and then by this point you'll have already d.gotten, so this var can be removed |
||
re_local_map := make(map[string]string) | ||
for _, v := range re_local { | ||
res, err := getRoleEntityPair(v.(string)) | ||
|
||
if err != nil { | ||
return fmt.Errorf( | ||
"Old state has malformed Role/Entity pair: %v", err) | ||
} | ||
|
||
re_local_map[res.Entity] = res.Role | ||
} | ||
|
||
res, err := config.clientStorage.DefaultObjectAccessControls.List(bucket).Do() | ||
|
||
if err != nil { | ||
return handleNotFoundError(err, d, fmt.Sprintf("Storage Default Object ACL for bucket %q", d.Get("bucket").(string))) | ||
} | ||
|
||
for _, v := range res.Items { | ||
role := v.Role | ||
entity := v.Entity | ||
// We only store updates to the locally defined access controls | ||
if _, in := re_local_map[entity]; in { | ||
role_entity = append(role_entity, fmt.Sprintf("%s:%s", role, entity)) | ||
log.Printf("[DEBUG]: saving re %s-%s", v.Role, v.Entity) | ||
} | ||
} | ||
|
||
d.Set("role_entity", role_entity) | ||
} | ||
|
||
d.SetId(getDefaultObjectAclId(bucket)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The id should be set at the end of create, since we want to make sure the state file has some information regardless of whether or not the read succeeded. |
||
return nil | ||
} | ||
|
||
func resourceStorageDefaultObjectAclUpdate(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*Config) | ||
|
||
bucket := d.Get("bucket").(string) | ||
|
||
if d.HasChange("role_entity") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since
and then the rest of the logic afterward? |
||
o, n := d.GetChange("role_entity") | ||
old_re := o.([]interface{}) | ||
new_re := n.([]interface{}) | ||
|
||
old_re_map := make(map[string]string) | ||
for _, v := range old_re { | ||
res, err := getRoleEntityPair(v.(string)) | ||
|
||
if err != nil { | ||
return fmt.Errorf( | ||
"Old state has malformed Role/Entity pair: %v", err) | ||
} | ||
|
||
old_re_map[res.Entity] = res.Role | ||
} | ||
|
||
for _, v := range new_re { | ||
pair, err := getRoleEntityPair(v.(string)) | ||
|
||
ObjectAccessControl := &storage.ObjectAccessControl{ | ||
Role: pair.Role, | ||
Entity: pair.Entity, | ||
} | ||
|
||
// If the old state is missing for this entity, it needs to | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This comment is written as create -> update, but the if statement is update -> create, can it be reworded to match? |
||
// be created. Otherwise it is updated | ||
if _, ok := old_re_map[pair.Entity]; ok { | ||
_, err = config.clientStorage.DefaultObjectAccessControls.Update( | ||
bucket, pair.Entity, ObjectAccessControl).Do() | ||
} else { | ||
_, err = config.clientStorage.DefaultObjectAccessControls.Insert( | ||
bucket, ObjectAccessControl).Do() | ||
} | ||
|
||
// Now we only store the keys that have to be removed | ||
delete(old_re_map, pair.Entity) | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error updating Storage Default Object ACL for bucket %s: %v", bucket, err) | ||
} | ||
} | ||
|
||
for entity, _ := range old_re_map { | ||
log.Printf("[DEBUG]: removing entity %s", entity) | ||
err := config.clientStorage.DefaultObjectAccessControls.Delete(bucket, entity).Do() | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error updating Storage Default Object ACL for bucket %s: %v", bucket, err) | ||
} | ||
} | ||
|
||
return resourceStorageDefaultObjectAclRead(d, meta) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func resourceStorageDefaultObjectAclDelete(d *schema.ResourceData, meta interface{}) error { | ||
config := meta.(*Config) | ||
|
||
bucket := d.Get("bucket").(string) | ||
|
||
re_local := d.Get("role_entity").([]interface{}) | ||
for _, v := range re_local { | ||
res, err := getRoleEntityPair(v.(string)) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
log.Printf("[DEBUG]: removing entity %s", res.Entity) | ||
|
||
err = config.clientStorage.DefaultObjectAccessControls.Delete(bucket, res.Entity).Do() | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error deleting entity %s ACL: %s", res.Entity, err) | ||
} | ||
} | ||
|
||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd highly recommend just having two separate fields here- one for the role, and the other for the entity. The id for the resource can then be
bucket/entity
, which would allow reading back the resource just from the id. If a Terraform user wants to specify multiple default acls, they can just have multiple resources. I'm going to hold off on reviewing the rest of this PR for now since it'll change a lot if you end up going with this approach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily think it is a good idea, because it is very likely that users will have multiple default acls. Default acls get applied to the objects only at object creation time, which means that the explicit dependency will need to be set for "google_storage_bucket_object" resource on "google_storage_default_object_acl" resource to ensure that if both of those resources are created simultaneously, the objects always gets created after the default acl is in place. Having multiple resources will make it more complicated to control for those explicit dependencies.
Besides, it seems to me that this resource should be similar in use with google_storage_object_acl, since it essentially does the same thing(sets up object permissions), but on the bucket level. So it seems to me that both should have similar resource declaration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, I can get behind having them be consistent with each other. That's fine then, I'll continue reviewing as-is :)