Impement replacePermissions for databricks_grant (#1183)

Read existing permissions from platform before updating them, further improving drift detection. Existing permissions are merged onto diffs as removals.

Fix #1164

Author: nfx

CHANGELOG.md

@@ -8,6 +8,7 @@
 * Added diff suppression for `min_num_clusters` field in `databricks_sql_endpoint` ([#1172](https://github.com/databrickslabs/terraform-provider-databricks/pull/1172)).
 * Added special case for handling `Cannot access cluster that was terminated or unpinned more than 30 days ago` error in `databricks_cluster` as an indication of resource removed on the platform side ([#1177](https://github.com/databrickslabs/terraform-provider-databricks/issues/1177)).
 * Fixed updating of `databricks_table` resources ([#1175](https://github.com/databrickslabs/terraform-provider-databricks/issues/1175)).
+* Fixed configuration drift in `databricks_grant` by reading existing permissions and removing them in subsequent update calls ([#1164](https://github.com/databrickslabs/terraform-provider-databricks/issues/1164)).
 
 Updated dependency versions:
 

catalog/resource_grants.go

@@ -3,6 +3,7 @@ package catalog
 import (
 	"context"
 	"fmt"
+	"sort"
 	"strings"
 
 	"github.com/databrickslabs/terraform-provider-databricks/common"
@@ -14,6 +15,17 @@ type PermissionsAPI struct {
 	context context.Context
 }
 
+// permissionsDiff is the inner structure of updatePermissions RPC
+type permissionsDiff struct {
+	Changes []permissionsChange `json:"changes"`
+}
+
+type permissionsChange struct {
+	Principal string   `json:"principal"`
+	Add       []string `json:"add,omitempty"`
+	Remove    []string `json:"remove,omitempty"`
+}
+
 // PrivilegeAssignment reflects on `grant` block
 type PrivilegeAssignment struct {
 	Principal  string   `json:"principal"`
@@ -26,34 +38,59 @@ type PermissionsList struct {
 	Assignments []PrivilegeAssignment `json:"privilege_assignments" tf:"slice_set,alias:grant"`
 }
 
-func (pl PermissionsList) toCreate() (diff PermissionsDiff) {
+// diff returns permissionsDiff of this permissions list with `diff` privileges removed
+func (pl PermissionsList) diff(existing PermissionsList) (diff permissionsDiff) {
+	// diffs change sets
+	configured := map[string]*schema.Set{}
 	for _, v := range pl.Assignments {
-		diff.Changes = append(diff.Changes, PermissionsChange{
-			Principal: v.Principal,
-			Add:       v.Privileges,
+		configured[v.Principal] = newStringSet(v.Privileges)
+	}
+	// existing permissions that needs removal
+	remote := map[string]*schema.Set{}
+	for _, v := range existing.Assignments {
+		remote[v.Principal] = newStringSet(v.Privileges)
+	}
+	// STEP 1: detect overlaps
+	for principal, confPrivs := range configured {
+		remotePrivs, ok := remote[principal]
+		if !ok {
+			remotePrivs = newStringSet([]string{})
+		}
+		add := setToStrings(confPrivs.Difference(remotePrivs))
+		remove := setToStrings(remotePrivs.Difference(confPrivs))
+		if len(add) == 0 && len(remove) == 0 {
+			continue
+		}
+		diff.Changes = append(diff.Changes, permissionsChange{
+			Principal: principal,
+			Add:       add,
+			Remove:    remove,
 		})
 	}
-	return
-}
-
-func (pl PermissionsList) toDelete() (diff PermissionsDiff) {
-	for _, v := range pl.Assignments {
-		diff.Changes = append(diff.Changes, PermissionsChange{
-			Principal: v.Principal,
-			Remove:    v.Privileges,
+	// STEP 2: non overlap - simply remove
+	for principal, remove := range remote {
+		_, ok := configured[principal]
+		if ok { // already handled in STEP 1
+			continue
+		}
+		diff.Changes = append(diff.Changes, permissionsChange{
+			Principal: principal,
+			Remove:    setToStrings(remove),
 		})
 	}
-	return
-}
-
-type PermissionsDiff struct {
-	Changes []PermissionsChange `json:"changes"`
+	// so that we can deterministic tests
+	sort.Slice(diff.Changes, func(i, j int) bool {
+		return diff.Changes[i].Principal < diff.Changes[j].Principal
+	})
+	return diff
 }
 
-type PermissionsChange struct {
-	Principal string   `json:"principal"`
-	Add       []string `json:"add,omitempty"`
-	Remove    []string `json:"remove,omitempty"`
+func newStringSet(in []string) *schema.Set {
+	var out []interface{}
+	for _, v := range in {
+		out = append(out, v)
+	}
+	return schema.NewSet(schema.HashString, out)
 }
 
 func NewPermissionsAPI(ctx context.Context, m interface{}) PermissionsAPI {
@@ -65,10 +102,19 @@ func (a PermissionsAPI) getPermissions(securable, name string) (list Permissions
 	return
 }
 
-func (a PermissionsAPI) updatePermissions(securable, name string, diff PermissionsDiff) error {
+func (a PermissionsAPI) updatePermissions(securable, name string, diff permissionsDiff) error {
 	return a.client.Patch(a.context, fmt.Sprintf("/unity-catalog/permissions/%s/%s", securable, name), diff)
 }
 
+// replacePermissions merges removal diff of existing permissions on the platform
+func (a PermissionsAPI) replacePermissions(securable, name string, list PermissionsList) error {
+	existing, err := a.getPermissions(securable, name)
+	if err != nil {
+		return err
+	}
+	return a.updatePermissions(securable, name, list.diff(existing))
+}
+
 type securableMapping map[string]map[string]bool
 
 // reuse ResourceDiff and ResourceData
@@ -150,61 +196,6 @@ func setToStrings(set *schema.Set) (ss []string) {
 	return
 }
 
-func principalAndPrivsFromRaw(v interface{}) (string, *schema.Set) {
-	item := v.(map[string]interface{})
-	principal := item["principal"].(string)
-	privileges := item["privileges"].(*schema.Set)
-	return principal, privileges
-}
-
-func permissionDiffFromRaw(old, new interface{}) PermissionsDiff {
-	o := old.(*schema.Set)
-	n := new.(*schema.Set)
-	prev := map[string]*schema.Set{}
-	for _, v := range o.List() {
-		principal, privileges := principalAndPrivsFromRaw(v)
-		prev[principal] = privileges
-	}
-	diff := PermissionsDiff{Changes: []PermissionsChange{}}
-	mix := map[string]bool{}
-	for _, v := range n.List() {
-		principal, newPrivs := principalAndPrivsFromRaw(v)
-		oldPrivs, exist := prev[principal]
-		if !exist {
-			// add new principal privileges
-			diff.Changes = append(diff.Changes, PermissionsChange{
-				Principal: principal,
-				Add:       setToStrings(newPrivs),
-			})
-			continue
-		}
-		// add or remove principal privileges
-		change := PermissionsChange{
-			Principal: principal,
-			Remove:    setToStrings(oldPrivs.Difference(newPrivs)),
-			Add:       setToStrings(newPrivs.Difference(oldPrivs)),
-		}
-		if len(change.Add) == 0 && len(change.Remove) == 0 {
-			continue
-		}
-		diff.Changes = append(diff.Changes, change)
-		mix[principal] = true
-	}
-	for _, v := range o.Difference(n).List() {
-		// remove old principal privs
-		principal, oldPrivs := principalAndPrivsFromRaw(v)
-		if mix[principal] {
-			// skip if mixed removes/adds were detected
-			continue
-		}
-		diff.Changes = append(diff.Changes, PermissionsChange{
-			Principal: principal,
-			Remove:    setToStrings(oldPrivs),
-		})
-	}
-	return diff
-}
-
 func ResourceGrants() *schema.Resource {
 	s := common.StructToSchema(PermissionsList{},
 		func(s map[string]*schema.Schema) map[string]*schema.Schema {
@@ -237,7 +228,7 @@ func ResourceGrants() *schema.Resource {
 			var grants PermissionsList
 			common.DataToStructPointer(d, s, &grants)
 			securable, name := mapping.kv(d)
-			err := NewPermissionsAPI(ctx, c).updatePermissions(securable, name, grants.toCreate())
+			err := NewPermissionsAPI(ctx, c).replacePermissions(securable, name, grants)
 			if err != nil {
 				return err
 			}
@@ -253,18 +244,23 @@ func ResourceGrants() *schema.Resource {
 			if err != nil {
 				return err
 			}
+			if len(grants.Assignments) == 0 {
+				return common.NotFound("got empty permissions list")
+			}
 			return common.StructToData(grants, s, d)
 		},
 		Update: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
 			securable, name := mapping.kv(d)
-			diff := permissionDiffFromRaw(d.GetChange("grant"))
-			return NewPermissionsAPI(ctx, c).updatePermissions(securable, name, diff)
-		},
-		Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
 			var grants PermissionsList
 			common.DataToStructPointer(d, s, &grants)
-			securable, name := mapping.kv(d)
-			return NewPermissionsAPI(ctx, c).updatePermissions(securable, name, grants.toDelete())
+			return NewPermissionsAPI(ctx, c).replacePermissions(securable, name, grants)
+		},
+		Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
+			split := strings.SplitN(d.Id(), "/", 2)
+			if len(split) != 2 {
+				return fmt.Errorf("ID must be two elements split by `/`: %s", d.Id())
+			}
+			return NewPermissionsAPI(ctx, c).replacePermissions(split[0], split[1], PermissionsList{})
 		},
 	}.ToResource()
 }