Make google_storage_object_acl authoritative (#2316)

<!-- This change is generated by MagicModules. -->
/cc @rileykarson

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]}
+}

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

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