From 5b6df5ee934235d3e1f280efcc6f2bad90d0be84 Mon Sep 17 00:00:00 2001
From: Paddy <paddy@hashicorp.com>
Date: Thu, 24 Aug 2017 16:18:34 -0700
Subject: [PATCH] storage: make bucket ACLs control the entire resource.

Storage bucket ACLs inherited the behaviour of only updating the fields
that were set in the config file. Terraform should track all the fields
in the resource, whether the user has specified a value for them or not,
and correct any drift that may occur.

This has manifested in an issue and unexpected behaviour in #50, and
this PR restores the expected behaviour.
---
 google/resource_storage_bucket_acl.go | 66 ++++++++++-----------------
 1 file changed, 23 insertions(+), 43 deletions(-)

diff --git a/google/resource_storage_bucket_acl.go b/google/resource_storage_bucket_acl.go
index 428c1cec..78a2ec38 100644
--- a/google/resource_storage_bucket_acl.go
+++ b/google/resource_storage_bucket_acl.go
@@ -30,15 +30,17 @@ func resourceStorageBucketAcl() *schema.Resource {
 			},
 
 			"predefined_acl": &schema.Schema{
-				Type:     schema.TypeString,
-				Optional: true,
-				ForceNew: true,
+				Type:          schema.TypeString,
+				Optional:      true,
+				ForceNew:      true,
+				ConflictsWith: []string{"role_entity"},
 			},
 
 			"role_entity": &schema.Schema{
-				Type:     schema.TypeList,
-				Optional: true,
-				Elem:     &schema.Schema{Type: schema.TypeString},
+				Type:          schema.TypeList,
+				Optional:      true,
+				Elem:          &schema.Schema{Type: schema.TypeString},
+				ConflictsWith: []string{"predefined_acl"},
 			},
 		},
 	}
@@ -84,11 +86,6 @@ func resourceStorageBucketAclCreate(d *schema.ResourceData, meta interface{}) er
 	}
 
 	if len(predefined_acl) > 0 {
-		if len(role_entity) > 0 {
-			return fmt.Errorf("Error, you cannot specify both " +
-				"\"predefined_acl\" and \"role_entity\"")
-		}
-
 		res, err := config.clientStorage.Buckets.Get(bucket).Do()
 
 		if err != nil {
@@ -102,8 +99,8 @@ func resourceStorageBucketAclCreate(d *schema.ResourceData, meta interface{}) er
 			return fmt.Errorf("Error updating bucket %s: %v", bucket, err)
 		}
 
-		return resourceStorageBucketAclRead(d, meta)
-	} else if len(role_entity) > 0 {
+	}
+	if len(role_entity) > 0 {
 		for _, v := range role_entity {
 			pair, err := getRoleEntityPair(v.(string))
 
@@ -121,7 +118,6 @@ func resourceStorageBucketAclCreate(d *schema.ResourceData, meta interface{}) er
 			}
 		}
 
-		return resourceStorageBucketAclRead(d, meta)
 	}
 
 	if len(default_acl) > 0 {
@@ -138,10 +134,10 @@ func resourceStorageBucketAclCreate(d *schema.ResourceData, meta interface{}) er
 			return fmt.Errorf("Error updating bucket %s: %v", bucket, err)
 		}
 
-		return resourceStorageBucketAclRead(d, meta)
 	}
 
-	return nil
+	d.SetId(getBucketAclId(bucket))
+	return resourceStorageBucketAclRead(d, meta)
 }
 
 func resourceStorageBucketAclRead(d *schema.ResourceData, meta interface{}) error {
@@ -149,42 +145,26 @@ func resourceStorageBucketAclRead(d *schema.ResourceData, meta interface{}) erro
 
 	bucket := d.Get("bucket").(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))
-
-			if err != nil {
-				return fmt.Errorf(
-					"Old state has malformed Role/Entity pair: %v", err)
-			}
-
-			re_local_map[res.Entity] = res.Role
-		}
-
+	// The API offers no way to retrieve predefined ACLs,
+	// and we can't tell which access controls were created
+	// by the predefined roles, so...
+	//
+	// This is, needless to say, a bad state of affairs and
+	// should be fixed.
+	if _, ok := d.GetOk("role_entity"); ok {
 		res, err := config.clientStorage.BucketAccessControls.List(bucket).Do()
 
 		if err != nil {
 			return handleNotFoundError(err, d, fmt.Sprintf("Storage Bucket ACL for bucket %q", d.Get("bucket").(string)))
 		}
-
-		for _, v := range res.Items {
-			log.Printf("[DEBUG]: examining re %s-%s", v.Role, v.Entity)
-			// We only store updates to the locally defined access controls
-			if _, in := re_local_map[v.Entity]; in {
-				role_entity = append(role_entity, fmt.Sprintf("%s:%s", v.Role, v.Entity))
-				log.Printf("[DEBUG]: saving re %s-%s", v.Role, v.Entity)
-			}
+		entities := make([]string, 0, len(res.Items))
+		for _, item := range res.Items {
+			entities = append(entities, item.Role+":"+item.Entity)
 		}
 
-		d.Set("role_entity", role_entity)
+		d.Set("role_entity", entities)
 	}
 
-	d.SetId(getBucketAclId(bucket))
 	return nil
 }