From 22732d11f9ad982b5df3b028a7e6070a9af62cbe Mon Sep 17 00:00:00 2001 From: vuong-nguyen <44292934+nkvuong@users.noreply.github.com> Date: Fri, 7 Jun 2024 13:44:31 +0100 Subject: [PATCH] Add customize diff for `databricks_grant` and `databricks_grants` for case insensitivity & spaces in grants (#3657) * relax principal checks to be case insensitive * fix test * revert acceptance test * add DiffSuppressFunc * strings.EqualFold * normalizePrivilege * normalize more * fix tests * fix int test * custom hash for set * fix test * clean up --- catalog/permissions/permissions.go | 10 ++++++-- catalog/resource_grant.go | 17 +++++++++----- catalog/resource_grant_test.go | 37 +++++++++++++++++++++++++++--- catalog/resource_grants.go | 19 +++++++++++++-- catalog/resource_grants_test.go | 30 ++++++++++++++++++++---- internal/acceptance/grant_test.go | 4 +++- internal/acceptance/grants_test.go | 4 +++- 7 files changed, 102 insertions(+), 19 deletions(-) diff --git a/catalog/permissions/permissions.go b/catalog/permissions/permissions.go index c0c21b45e9..4f37fc8a73 100644 --- a/catalog/permissions/permissions.go +++ b/catalog/permissions/permissions.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log" + "strings" "time" "github.com/databricks/databricks-sdk-go" @@ -119,18 +120,23 @@ var Mappings = SecurableMapping{ "volume": catalog.SecurableType("volume"), } +// Unity Catalog accepts privileges with spaces, but will automatically convert them to underscores +func NormalizePrivilege(privilege string) string { + return strings.ToUpper(strings.Replace(privilege, " ", "_", -1)) +} + // Utils for Slice and Set func SliceToSet(in []catalog.Privilege) *schema.Set { var out []any for _, v := range in { - out = append(out, v.String()) + out = append(out, NormalizePrivilege(v.String())) } return schema.NewSet(schema.HashString, out) } func SetToSlice(set *schema.Set) (ss []catalog.Privilege) { for _, v := range set.List() { - ss = append(ss, catalog.Privilege(v.(string))) + ss = append(ss, catalog.Privilege(NormalizePrivilege(v.(string)))) } return } diff --git a/catalog/resource_grant.go b/catalog/resource_grant.go index 2c8cd174cd..ddd3abb08c 100644 --- a/catalog/resource_grant.go +++ b/catalog/resource_grant.go @@ -20,15 +20,15 @@ func diffPermissionsForPrincipal(principal string, desired catalog.PermissionsLi // diffs change sets for principal configured := map[string]*schema.Set{} for _, v := range desired.PrivilegeAssignments { - if v.Principal == principal { - configured[v.Principal] = permissions.SliceToSet(v.Privileges) + if strings.EqualFold(v.Principal, principal) { + configured[strings.ToLower(v.Principal)] = permissions.SliceToSet(v.Privileges) } } // existing permissions that needs removal for principal remote := map[string]*schema.Set{} for _, v := range existing.PrivilegeAssignments { - if v.Principal == principal { - remote[v.Principal] = permissions.SliceToSet(v.Privileges) + if strings.EqualFold(v.Principal, principal) { + remote[strings.ToLower(v.Principal)] = permissions.SliceToSet(v.Privileges) } } // STEP 1: detect overlaps @@ -87,7 +87,7 @@ func filterPermissionsForPrincipal(in catalog.PermissionsList, principal string) grantsForPrincipal := []permissions.UnityCatalogPrivilegeAssignment{} for _, v := range in.PrivilegeAssignments { privileges := []string{} - if v.Principal == principal { + if strings.EqualFold(v.Principal, principal) { for _, p := range v.Privileges { privileges = append(privileges, p.String()) } @@ -122,8 +122,13 @@ func parseSecurableId(d *schema.ResourceData) (string, string, string, error) { func ResourceGrant() common.Resource { s := common.StructToSchema(permissions.UnityCatalogPrivilegeAssignment{}, func(m map[string]*schema.Schema) map[string]*schema.Schema { + common.CustomizeSchemaPath(m, "principal").SetForceNew().SetCustomSuppressDiff(common.EqualFoldDiffSuppress) - m["principal"].ForceNew = true + // set custom hash function for privileges + common.MustSchemaPath(m, "privileges").Set = func(i any) int { + privilege := i.(string) + return schema.HashString(permissions.NormalizePrivilege(privilege)) + } allFields := []string{} for field := range permissions.Mappings { diff --git a/catalog/resource_grant_test.go b/catalog/resource_grant_test.go index 43a0eeb6b9..a51d0b8737 100644 --- a/catalog/resource_grant_test.go +++ b/catalog/resource_grant_test.go @@ -532,6 +532,37 @@ func TestResourceGrantPermissionsList_Diff_ExternallyAddedPrincipal(t *testing.T assert.Len(t, diff, 0) } +func TestResourceGrantPermissionsList_Diff_CaseSensitive(t *testing.T) { + diff := diffPermissionsForPrincipal( + "a", + catalog.PermissionsList{ // config + PrivilegeAssignments: []catalog.PrivilegeAssignment{ + { + Principal: "a", + Privileges: []catalog.Privilege{"a"}, + }, + { + Principal: "c", + Privileges: []catalog.Privilege{"a"}, + }, + }, + }, + catalog.PermissionsList{ + PrivilegeAssignments: []catalog.PrivilegeAssignment{ // platform + { + Principal: "A", + Privileges: []catalog.Privilege{"a"}, + }, + { + Principal: "b", + Privileges: []catalog.Privilege{"a"}, + }, + }, + }, + ) + assert.Len(t, diff, 0) +} + func TestResourceGrantPermissionsList_Diff_ExternallyAddedPriv(t *testing.T) { diff := diffPermissionsForPrincipal( "a", @@ -555,7 +586,7 @@ func TestResourceGrantPermissionsList_Diff_ExternallyAddedPriv(t *testing.T) { assert.Len(t, diff, 1) assert.Len(t, diff[0].Add, 0) assert.Len(t, diff[0].Remove, 1) - assert.Equal(t, catalog.Privilege("b"), diff[0].Remove[0]) + assert.Equal(t, catalog.Privilege("B"), diff[0].Remove[0]) } func TestResourceGrantPermissionsList_Diff_LocalRemoteDiff(t *testing.T) { @@ -581,8 +612,8 @@ func TestResourceGrantPermissionsList_Diff_LocalRemoteDiff(t *testing.T) { assert.Len(t, diff, 1) assert.Len(t, diff[0].Add, 1) assert.Len(t, diff[0].Remove, 1) - assert.Equal(t, catalog.Privilege("a"), diff[0].Add[0]) - assert.Equal(t, catalog.Privilege("c"), diff[0].Remove[0]) + assert.Equal(t, catalog.Privilege("A"), diff[0].Add[0]) + assert.Equal(t, catalog.Privilege("C"), diff[0].Remove[0]) } func TestResourceGrantShareGrantCreate(t *testing.T) { diff --git a/catalog/resource_grants.go b/catalog/resource_grants.go index a33fac2535..9831728d40 100644 --- a/catalog/resource_grants.go +++ b/catalog/resource_grants.go @@ -31,12 +31,12 @@ func diffPermissions(pl catalog.PermissionsList, existing catalog.PermissionsLis // diffs change sets configured := map[string]*schema.Set{} for _, v := range pl.PrivilegeAssignments { - configured[v.Principal] = permissions.SliceToSet(v.Privileges) + configured[strings.ToLower(v.Principal)] = permissions.SliceToSet(v.Privileges) } // existing permissions that needs removal remote := map[string]*schema.Set{} for _, v := range existing.PrivilegeAssignments { - remote[v.Principal] = permissions.SliceToSet(v.Privileges) + remote[strings.ToLower(v.Principal)] = permissions.SliceToSet(v.Privileges) } // STEP 1: detect overlaps for principal, confPrivs := range configured { @@ -128,6 +128,21 @@ func parseId(d *schema.ResourceData) (string, string, error) { func ResourceGrants() common.Resource { s := common.StructToSchema(PermissionsList{}, func(s map[string]*schema.Schema) map[string]*schema.Schema { + // set custom hash function for principal and privileges + common.MustSchemaPath(s, "grant", "privileges").Set = func(i any) int { + privilege := i.(string) + return schema.HashString(permissions.NormalizePrivilege(privilege)) + } + common.MustSchemaPath(s, "grant").Set = func(i any) int { + objectStruct := i.(map[string]any) + principal := objectStruct["principal"].(string) + privileges := objectStruct["privileges"].(*schema.Set) + hashString := strings.ToLower(principal) + for _, privilege := range privileges.List() { + hashString += "|" + permissions.NormalizePrivilege(privilege.(string)) + } + return schema.HashString(hashString) + } alof := []string{} for field := range permissions.Mappings { s[field] = &schema.Schema{ diff --git a/catalog/resource_grants_test.go b/catalog/resource_grants_test.go index 81fa93f88f..88b002018e 100644 --- a/catalog/resource_grants_test.go +++ b/catalog/resource_grants_test.go @@ -389,7 +389,7 @@ func TestPermissionsList_Diff_ExternallyAddedPrincipal(t *testing.T) { assert.Len(t, diff[0].Add, 0) assert.Len(t, diff[0].Remove, 1) assert.Equal(t, "b", diff[0].Principal) - assert.Equal(t, catalog.Privilege("a"), diff[0].Remove[0]) + assert.Equal(t, catalog.Privilege("A"), diff[0].Remove[0]) assert.Equal(t, "c", diff[1].Principal) } @@ -415,7 +415,29 @@ func TestPermissionsList_Diff_ExternallyAddedPriv(t *testing.T) { assert.Len(t, diff, 1) assert.Len(t, diff[0].Add, 0) assert.Len(t, diff[0].Remove, 1) - assert.Equal(t, catalog.Privilege("b"), diff[0].Remove[0]) + assert.Equal(t, catalog.Privilege("B"), diff[0].Remove[0]) +} + +func TestPermissionsList_Diff_CaseSensitivePrincipal(t *testing.T) { + diff := diffPermissions( + catalog.PermissionsList{ // config + PrivilegeAssignments: []catalog.PrivilegeAssignment{ + { + Principal: "A", + Privileges: []catalog.Privilege{"a"}, + }, + }, + }, + catalog.PermissionsList{ + PrivilegeAssignments: []catalog.PrivilegeAssignment{ // platform + { + Principal: "a", + Privileges: []catalog.Privilege{"a"}, + }, + }, + }, + ) + assert.Len(t, diff, 0) } func TestPermissionsList_Diff_LocalRemoteDiff(t *testing.T) { @@ -440,8 +462,8 @@ func TestPermissionsList_Diff_LocalRemoteDiff(t *testing.T) { assert.Len(t, diff, 1) assert.Len(t, diff[0].Add, 1) assert.Len(t, diff[0].Remove, 1) - assert.Equal(t, catalog.Privilege("a"), diff[0].Add[0]) - assert.Equal(t, catalog.Privilege("c"), diff[0].Remove[0]) + assert.Equal(t, catalog.Privilege("A"), diff[0].Add[0]) + assert.Equal(t, catalog.Privilege("C"), diff[0].Remove[0]) } func TestShareGrantCreate(t *testing.T) { diff --git a/internal/acceptance/grant_test.go b/internal/acceptance/grant_test.go index 19898e477d..f0c26575d4 100644 --- a/internal/acceptance/grant_test.go +++ b/internal/acceptance/grant_test.go @@ -103,6 +103,8 @@ func TestUcAccGrant(t *testing.T) { Template: strings.ReplaceAll(grantTemplate, "%s", "{env.TEST_DATA_ENG_GROUP}"), }, step{ Template: strings.ReplaceAll(grantTemplate, "%s", "{env.TEST_DATA_SCI_GROUP}"), + }, step{ + Template: strings.ReplaceAll(strings.ReplaceAll(grantTemplate, "ALL_PRIVILEGES", "ALL PRIVILEGES"), `"%s"`, `upper("{env.TEST_DATA_SCI_GROUP}")`), }) } @@ -131,6 +133,6 @@ func TestUcAccGrantForIdChange(t *testing.T) { Template: grantTemplateForNamePermissionChange("-new", "ALL_PRIVILEGES"), }, step{ Template: grantTemplateForNamePermissionChange("-fail", "abc"), - ExpectError: regexp.MustCompile(`cannot create grant: Privilege abc is not applicable to this entity`), + ExpectError: regexp.MustCompile(`cannot create grant: Privilege ABC is not applicable to this entity`), }) } diff --git a/internal/acceptance/grants_test.go b/internal/acceptance/grants_test.go index 23beb2fe3b..4f32dcb8df 100644 --- a/internal/acceptance/grants_test.go +++ b/internal/acceptance/grants_test.go @@ -109,6 +109,8 @@ func TestUcAccGrants(t *testing.T) { Template: strings.ReplaceAll(grantsTemplate, "%s", "{env.TEST_DATA_ENG_GROUP}"), }, step{ Template: strings.ReplaceAll(grantsTemplate, "%s", "{env.TEST_DATA_SCI_GROUP}"), + }, step{ + Template: strings.ReplaceAll(strings.ReplaceAll(grantsTemplate, "ALL_PRIVILEGES", "ALL PRIVILEGES"), `"%s"`, `upper("{env.TEST_DATA_SCI_GROUP}")`), }) } @@ -139,6 +141,6 @@ func TestUcAccGrantsForIdChange(t *testing.T) { Template: grantsTemplateForNamePermissionChange("-new", "ALL_PRIVILEGES"), }, step{ Template: grantsTemplateForNamePermissionChange("-fail", "abc"), - ExpectError: regexp.MustCompile(`Error: cannot create grants: Privilege abc is not applicable to this entity`), + ExpectError: regexp.MustCompile(`Error: cannot create grants: Privilege ABC is not applicable to this entity`), }) }