From 18084be34528bbd8c483fe6d54a2fff0d54d9236 Mon Sep 17 00:00:00 2001 From: The Magician Date: Fri, 26 Oct 2018 11:27:41 -0700 Subject: [PATCH] Make google_storage_object_acl authoritative (#2316) /cc @rileykarson --- google/resource_storage_object_acl.go | 384 +++++++++++------- google/resource_storage_object_acl_test.go | 66 +++ .../docs/r/storage_object_acl.html.markdown | 23 +- 3 files changed, 323 insertions(+), 150 deletions(-) diff --git a/google/resource_storage_object_acl.go b/google/resource_storage_object_acl.go index 15e03a51..039499a1 100644 --- a/google/resource_storage_object_acl.go +++ b/google/resource_storage_object_acl.go @@ -2,11 +2,10 @@ package google import ( "fmt" - "log" - "github.com/hashicorp/terraform/helper/schema" - + "github.com/hashicorp/terraform/helper/validation" "google.golang.org/api/storage/v1" + "strings" ) func resourceStorageObjectAcl() *schema.Resource { @@ -15,37 +14,85 @@ func resourceStorageObjectAcl() *schema.Resource { Read: resourceStorageObjectAclRead, Update: resourceStorageObjectAclUpdate, Delete: resourceStorageObjectAclDelete, - CustomizeDiff: resourceStorageRoleEntityCustomizeDiff, + CustomizeDiff: resourceStorageObjectAclDiff, Schema: map[string]*schema.Schema{ - "bucket": &schema.Schema{ + "bucket": { Type: schema.TypeString, Required: true, ForceNew: true, }, - "object": &schema.Schema{ + "object": { Type: schema.TypeString, Required: true, ForceNew: true, }, - "predefined_acl": &schema.Schema{ - Type: schema.TypeString, - Optional: true, - ForceNew: true, + "predefined_acl": { + Type: schema.TypeString, + Optional: true, + ConflictsWith: []string{"role_entity"}, + ValidateFunc: validation.StringInSlice([]string{"private", "bucketOwnerRead", "bucketOwnerFullControl", "projectPrivate", "authenticatedRead", "publicRead", ""}, false), }, - "role_entity": &schema.Schema{ - Type: schema.TypeList, + "role_entity": { + Type: schema.TypeSet, Optional: true, Computed: true, - Elem: &schema.Schema{Type: schema.TypeString}, + Elem: &schema.Schema{ + Type: schema.TypeString, + ValidateFunc: validateRoleEntityPair, + }, + ConflictsWith: []string{"predefined_acl"}, }, }, } } +// We can't edit the object owner (at risk of 403 errors), and users will always see a diff if they +// don't explicitly specify that it has OWNER permissions. +// Suppressing it means their configs won't be *strictly* correct as they will be missing the object +// owner having OWNER. It's impossible to remove that permission though, so this custom diff +// makes configs with or without that line indistinguishable. +func resourceStorageObjectAclDiff(diff *schema.ResourceDiff, meta interface{}) error { + config := meta.(*Config) + bucket := diff.Get("bucket").(string) + object := diff.Get("object").(string) + + sObject, err := config.clientStorage.Objects.Get(bucket, object).Projection("full").Do() + if err != nil { + // Failing here is OK! Generally, it means we are at Create although it could mean the resource is gone. + // Create won't show the object owner being given + return nil + } + + objectOwner := sObject.Owner.Entity + ownerRole := fmt.Sprintf("%s:%s", "OWNER", objectOwner) + oldRoleEntity, newRoleEntity := diff.GetChange("role_entity") + + // We can fail at plan time if the object owner/creator is being set to + // a reader + for _, v := range newRoleEntity.(*schema.Set).List() { + res := getValidatedRoleEntityPair(v.(string)) + + if res.Entity == objectOwner && res.Role != "OWNER" { + return fmt.Errorf("New state tried setting object owner entity (%s) to non-'OWNER' role", objectOwner) + } + } + + // Diffs won't match in Plan and Apply pre-create if we naively add the RE + // every time. So instead, we check to see if the old state (upstream/gcp + // because we will have just done a refresh) contains it first. + if oldRoleEntity.(*schema.Set).Contains(ownerRole) && + !newRoleEntity.(*schema.Set).Contains(ownerRole) { + newRoleEntity.(*schema.Set).Add(ownerRole) + return diff.SetNew("role_entity", newRoleEntity) + } + + return nil +} + func getObjectAclId(object string) string { return object + "-acl" } @@ -56,61 +103,45 @@ func resourceStorageObjectAclCreate(d *schema.ResourceData, meta interface{}) er bucket := d.Get("bucket").(string) object := d.Get("object").(string) - predefined_acl := "" - role_entity := make([]interface{}, 0) - - if v, ok := d.GetOk("predefined_acl"); ok { - predefined_acl = v.(string) - } - - if v, ok := d.GetOk("role_entity"); ok { - role_entity = v.([]interface{}) - } - - if len(predefined_acl) > 0 { - if len(role_entity) > 0 { - return fmt.Errorf("Error, you cannot specify both " + - "\"predefined_acl\" and \"role_entity\"") - } - + // If we're using a predefined acl we just use the canned api. + if predefinedAcl, ok := d.GetOk("predefined_acl"); ok { res, err := config.clientStorage.Objects.Get(bucket, object).Do() - if err != nil { - return fmt.Errorf("Error reading object %s: %v", bucket, err) + return fmt.Errorf("Error reading object %s in %s: %v", object, bucket, err) } - res, err = config.clientStorage.Objects.Update(bucket, object, - res).PredefinedAcl(predefined_acl).Do() - + res, err = config.clientStorage.Objects.Update(bucket, object, res).PredefinedAcl(predefinedAcl.(string)).Do() if err != nil { - return fmt.Errorf("Error updating object %s: %v", bucket, err) + return fmt.Errorf("Error updating object %s in %s: %v", object, bucket, err) } return resourceStorageObjectAclRead(d, meta) - } else if len(role_entity) > 0 { - for _, v := range role_entity { - pair, err := getRoleEntityPair(v.(string)) + } else if reMap := d.Get("role_entity").(*schema.Set); reMap.Len() > 0 { + sObject, err := config.clientStorage.Objects.Get(bucket, object).Projection("full").Do() + if err != nil { + return fmt.Errorf("Error reading object %s in %s: %v", object, bucket, err) + } - objectAccessControl := &storage.ObjectAccessControl{ - Role: pair.Role, - Entity: pair.Entity, - } + objectOwner := sObject.Owner.Entity + roleEntitiesUpstream, err := getRoleEntitiesAsStringsFromApi(config, bucket, object) + if err != nil { + return fmt.Errorf("Error reading object %s in %s: %v", object, bucket, err) + } - log.Printf("[DEBUG]: setting role = %s, entity = %s", pair.Role, pair.Entity) + create, update, remove, err := getRoleEntityChange(roleEntitiesUpstream, convertStringSet(reMap), objectOwner) + if err != nil { + return fmt.Errorf("Error reading object %s in %s. Invalid schema: %v", object, bucket, err) + } - _, err = config.clientStorage.ObjectAccessControls.Insert(bucket, - object, objectAccessControl).Do() - - if err != nil { - return fmt.Errorf("Error setting ACL for %s on object %s: %v", pair.Entity, object, err) - } + err = performStorageObjectRoleEntityOperations(create, update, remove, config, bucket, object) + if err != nil { + return fmt.Errorf("Error creating object %s in %s: %v", object, bucket, err) } return resourceStorageObjectAclRead(d, meta) } - return fmt.Errorf("Error, you must specify either " + - "\"predefined_acl\" or \"role_entity\"") + return fmt.Errorf("Error, you must specify either \"predefined_acl\" or \"role_entity\"") } func resourceStorageObjectAclRead(d *schema.ResourceData, meta interface{}) error { @@ -119,41 +150,14 @@ func resourceStorageObjectAclRead(d *schema.ResourceData, meta interface{}) erro bucket := d.Get("bucket").(string) object := d.Get("object").(string) - // Predefined ACLs cannot easily be parsed once they have been processed - // by the GCP server - if _, ok := d.GetOk("predefined_acl"); !ok { - role_entity := make([]interface{}, 0) - re_local := d.Get("role_entity").([]interface{}) - re_local_map := make(map[string]string) - for _, v := range re_local { - res, err := getRoleEntityPair(v.(string)) + roleEntities, err := getRoleEntitiesAsStringsFromApi(config, bucket, object) + if err != nil { + return handleNotFoundError(err, d, fmt.Sprintf("Storage Object ACL for Bucket %q", d.Get("bucket").(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.ObjectAccessControls.List(bucket, object).Do() - - if err != nil { - return handleNotFoundError(err, d, fmt.Sprintf("Storage Object ACL for Bucket %q", d.Get("bucket").(string))) - } - - for _, v := range res.Items { - role := v.Role - entity := v.Entity - 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", role, entity) - } - } - - d.Set("role_entity", role_entity) - } else { - d.Set("role_entity", nil) + err = d.Set("role_entity", roleEntities) + if err != nil { + return err } d.SetId(getObjectAclId(object)) @@ -166,61 +170,42 @@ func resourceStorageObjectAclUpdate(d *schema.ResourceData, meta interface{}) er bucket := d.Get("bucket").(string) object := d.Get("object").(string) - if d.HasChange("role_entity") { + if _, ok := d.GetOk("predefined_acl"); d.HasChange("predefined_acl") && ok { + res, err := config.clientStorage.Objects.Get(bucket, object).Do() + if err != nil { + return fmt.Errorf("Error reading object %s in %s: %v", object, bucket, err) + } + + res, err = config.clientStorage.Objects.Update(bucket, object, res).PredefinedAcl(d.Get("predefined_acl").(string)).Do() + if err != nil { + return fmt.Errorf("Error updating object %s in %s: %v", object, bucket, err) + } + + return resourceStorageObjectAclRead(d, meta) + } else { + sObject, err := config.clientStorage.Objects.Get(bucket, object).Projection("full").Do() + if err != nil { + return fmt.Errorf("Error reading object %s in %s: %v", object, bucket, err) + } + + objectOwner := sObject.Owner.Entity + o, n := d.GetChange("role_entity") - old_re, new_re := o.([]interface{}), 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 + create, update, remove, err := getRoleEntityChange( + convertStringSet(o.(*schema.Set)), + convertStringSet(n.(*schema.Set)), + objectOwner) + if err != nil { + return fmt.Errorf("Error reading object %s in %s. Invalid schema: %v", object, bucket, err) } - 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 this entity, it needs to - // be created. Otherwise it is updated - if _, ok := old_re_map[pair.Entity]; ok { - _, err = config.clientStorage.ObjectAccessControls.Update( - bucket, object, pair.Entity, objectAccessControl).Do() - } else { - _, err = config.clientStorage.ObjectAccessControls.Insert( - bucket, object, 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 ACL for object %s: %v", bucket, err) - } - } - - for entity, _ := range old_re_map { - log.Printf("[DEBUG]: removing entity %s", entity) - err := config.clientStorage.ObjectAccessControls.Delete(bucket, object, entity).Do() - - if err != nil { - return fmt.Errorf("Error updating ACL for object %s: %v", bucket, err) - } + err = performStorageObjectRoleEntityOperations(create, update, remove, config, bucket, object) + if err != nil { + return fmt.Errorf("Error updating object %s in %s: %v", object, bucket, err) } return resourceStorageObjectAclRead(d, meta) } - - return nil } func resourceStorageObjectAclDelete(d *schema.ResourceData, meta interface{}) error { @@ -229,25 +214,134 @@ func resourceStorageObjectAclDelete(d *schema.ResourceData, meta interface{}) er bucket := d.Get("bucket").(string) object := d.Get("object").(string) - re_local := d.Get("role_entity").([]interface{}) - for _, v := range re_local { - res, err := getRoleEntityPair(v.(string)) - if err != nil { - return err + res, err := config.clientStorage.Objects.Get(bucket, object).Do() + if err != nil { + return fmt.Errorf("Error reading object %s in %s: %v", object, bucket, err) + } + + res, err = config.clientStorage.Objects.Update(bucket, object, res).PredefinedAcl("private").Do() + if err != nil { + return fmt.Errorf("Error updating object %s in %s: %v", object, bucket, err) + } + + return nil +} + +func getRoleEntitiesAsStringsFromApi(config *Config, bucket string, object string) ([]string, error) { + var roleEntities []string + res, err := config.clientStorage.ObjectAccessControls.List(bucket, object).Do() + if err != nil { + return nil, err + } + + for _, roleEntity := range res.Items { + role := roleEntity.Role + entity := roleEntity.Entity + roleEntities = append(roleEntities, fmt.Sprintf("%s:%s", role, entity)) + } + + return roleEntities, nil +} + +// Creates 3 lists of changes we need to make to go from one set of entities to another- which entities need to be created, update, and deleted +// Not resource specific +func getRoleEntityChange(old []string, new []string, owner string) (create, update, remove []*RoleEntity, err error) { + newEntitiesUsed := make(map[string]struct{}) + for _, v := range new { + res := getValidatedRoleEntityPair(v) + if _, ok := newEntitiesUsed[res.Entity]; ok { + return nil, nil, nil, fmt.Errorf("New state has duplicate Entity: %v", res.Entity) } - entity := res.Entity + newEntitiesUsed[res.Entity] = struct{}{} + } - log.Printf("[DEBUG]: removing entity %s", entity) + oldEntitiesUsed := make(map[string]string) + for _, v := range old { + res := getValidatedRoleEntityPair(v) - err = config.clientStorage.ObjectAccessControls.Delete(bucket, object, - entity).Do() + // Updating the owner will error out, so let's avoid it. + if res.Entity == owner { + continue + } + oldEntitiesUsed[res.Entity] = res.Role + } + + for _, re := range new { + res := getValidatedRoleEntityPair(re) + + // Updating the owner will error out, so let's never do it. + if res.Entity == owner { + continue + } + + if v, ok := oldEntitiesUsed[res.Entity]; ok { + if res.Role != v { + update = append(update, res) + } + + delete(oldEntitiesUsed, res.Entity) + } else { + create = append(create, res) + } + } + + for _, v := range old { + res := getValidatedRoleEntityPair(v) + + if _, ok := oldEntitiesUsed[res.Entity]; ok { + remove = append(remove, res) + } + } + + return create, update, remove, nil +} + +// Takes in lists of changes to make to a Storage Object's ACL and makes those changes +func performStorageObjectRoleEntityOperations(create []*RoleEntity, update []*RoleEntity, remove []*RoleEntity, config *Config, bucket string, object string) error { + for _, v := range create { + objectAccessControl := &storage.ObjectAccessControl{ + Role: v.Role, + Entity: v.Entity, + } + _, err := config.clientStorage.ObjectAccessControls.Insert(bucket, object, objectAccessControl).Do() if err != nil { - return fmt.Errorf("Error deleting entity %s ACL: %s", - entity, err) + return fmt.Errorf("Error inserting ACL item %s for object %s in %s: %v", v.Entity, object, bucket, err) + } + } + + for _, v := range update { + objectAccessControl := &storage.ObjectAccessControl{ + Role: v.Role, + Entity: v.Entity, + } + _, err := config.clientStorage.ObjectAccessControls.Update(bucket, object, v.Entity, objectAccessControl).Do() + if err != nil { + return fmt.Errorf("Error updating ACL item %s for object %s in %s: %v", v.Entity, object, bucket, err) + } + } + + for _, v := range remove { + err := config.clientStorage.ObjectAccessControls.Delete(bucket, object, v.Entity).Do() + if err != nil { + return fmt.Errorf("Error deleting ACL item %s for object %s in %s: %v", v.Entity, object, bucket, err) } } return nil } + +func validateRoleEntityPair(v interface{}, k string) (ws []string, errors []error) { + split := strings.Split(v.(string), ":") + if len(split) != 2 { + errors = append(errors, fmt.Errorf("Role entity pairs must be formatted as 'ROLE:entity': %s", v)) + } + + return +} + +func getValidatedRoleEntityPair(roleEntity string) *RoleEntity { + split := strings.Split(roleEntity, ":") + return &RoleEntity{Role: split[0], Entity: split[1]} +} diff --git a/google/resource_storage_object_acl_test.go b/google/resource_storage_object_acl_test.go index 3b10b6b0..a9199b10 100644 --- a/google/resource_storage_object_acl_test.go +++ b/google/resource_storage_object_acl_test.go @@ -176,6 +176,72 @@ func TestAccStorageObjectAcl_predefined(t *testing.T) { }) } +func TestAccStorageObjectAcl_predefinedToExplicit(t *testing.T) { + t.Parallel() + + bucketName := testBucketName() + objectName := testAclObjectName() + objectData := []byte("data data data") + ioutil.WriteFile(tfObjectAcl.Name(), objectData, 0644) + resource.Test(t, resource.TestCase{ + PreCheck: func() { + if errObjectAcl != nil { + panic(errObjectAcl) + } + testAccPreCheck(t) + }, + Providers: testAccProviders, + CheckDestroy: testAccStorageObjectAclDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testGoogleStorageObjectsAclPredefined(bucketName, objectName), + }, + resource.TestStep{ + Config: testGoogleStorageObjectsAclBasic1(bucketName, objectName), + Check: resource.ComposeTestCheckFunc( + testAccCheckGoogleStorageObjectAcl(bucketName, + objectName, roleEntityBasic1), + testAccCheckGoogleStorageObjectAcl(bucketName, + objectName, roleEntityBasic2), + ), + }, + }, + }) +} + +func TestAccStorageObjectAcl_explicitToPredefined(t *testing.T) { + t.Parallel() + + bucketName := testBucketName() + objectName := testAclObjectName() + objectData := []byte("data data data") + ioutil.WriteFile(tfObjectAcl.Name(), objectData, 0644) + resource.Test(t, resource.TestCase{ + PreCheck: func() { + if errObjectAcl != nil { + panic(errObjectAcl) + } + testAccPreCheck(t) + }, + Providers: testAccProviders, + CheckDestroy: testAccStorageObjectAclDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testGoogleStorageObjectsAclBasic1(bucketName, objectName), + Check: resource.ComposeTestCheckFunc( + testAccCheckGoogleStorageObjectAcl(bucketName, + objectName, roleEntityBasic1), + testAccCheckGoogleStorageObjectAcl(bucketName, + objectName, roleEntityBasic2), + ), + }, + resource.TestStep{ + Config: testGoogleStorageObjectsAclPredefined(bucketName, objectName), + }, + }, + }) +} + // Test that we allow the API to reorder our role entities without perma-diffing. func TestAccStorageObjectAcl_unordered(t *testing.T) { t.Parallel() diff --git a/website/docs/r/storage_object_acl.html.markdown b/website/docs/r/storage_object_acl.html.markdown index 42dfb645..386a30e3 100644 --- a/website/docs/r/storage_object_acl.html.markdown +++ b/website/docs/r/storage_object_acl.html.markdown @@ -8,11 +8,18 @@ description: |- # google\_storage\_object\_acl -Creates a new object ACL in Google cloud storage service (GCS). For more information see +Authoritatively manages the access control list (ACL) for an object in a Google +Cloud Storage (GCS) bucket. Removing a `google_storage_object_acl` sets the +acl to the `private` [predefined ACL](https://cloud.google.com/storage/docs/access-control#predefined-acl). + +For more information see [the official documentation](https://cloud.google.com/storage/docs/access-control/lists) and [API](https://cloud.google.com/storage/docs/json_api/v1/objectAccessControls). +-> Want fine-grained control over object ACLs? Use `google_storage_object_access_control` to control individual +role entity pairs. + ## Example Usage Create an object ACL with one owner and one reader. @@ -42,15 +49,21 @@ resource "google_storage_object_acl" "image-store-acl" { ## Argument Reference -* `bucket` - (Required) The name of the bucket it applies to. +* `bucket` - (Required) The name of the bucket the object is stored in. -* `object` - (Required) The name of the object it applies to. +* `object` - (Required) The name of the object to apply the acl to. - - - -* `predefined_acl` - (Optional) The [canned GCS ACL](https://cloud.google.com/storage/docs/access-control#predefined-acl) to apply. Must be set if `role_entity` is not. +* `predefined_acl` - (Optional) The "canned" [predefined ACL](https://cloud.google.com/storage/docs/access-control#predefined-acl) to apply. Must be set if `role_entity` is not. + +* `role_entity` - (Optional) List of role/entity pairs in the form `ROLE:entity`. See [GCS Object ACL documentation](https://cloud.google.com/storage/docs/json_api/v1/objectAccessControls) for more details. +Must be set if `predefined_acl` is not. + +-> The object's creator will always have `OWNER` permissions for their object, and any attempt to modify that permission would return an error. Instead, Terraform automatically +adds that role/entity pair to your `terraform plan` results when it is omitted in your config; `terraform plan` will show the correct final state at every point except for at +`Create` time, where the object role/entity pair is omitted if not explicitly set. -* `role_entity` - (Optional) List of role/entity pairs in the form `ROLE:entity`. See [GCS Object ACL documentation](https://cloud.google.com/storage/docs/json_api/v1/objectAccessControls) for more details. Must be set if `predefined_acl` is not. ## Attributes Reference