Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add customize diff for databricks_grant and databricks_grants for case insensitivity & spaces in grants #3657

Merged
merged 12 commits into from
Jun 7, 2024
10 changes: 8 additions & 2 deletions catalog/permissions/permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"log"
"strings"
"time"

"github.com/databricks/databricks-sdk-go"
Expand Down Expand Up @@ -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
}
Expand Down
17 changes: 11 additions & 6 deletions catalog/resource_grant.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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())
}
Expand Down Expand Up @@ -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 {
Expand Down
37 changes: 34 additions & 3 deletions catalog/resource_grant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
19 changes: 17 additions & 2 deletions catalog/resource_grants.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down
30 changes: 26 additions & 4 deletions catalog/resource_grants_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
4 changes: 3 additions & 1 deletion internal/acceptance/grant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}")`),
})
}

Expand Down Expand Up @@ -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`),
})
}
4 changes: 3 additions & 1 deletion internal/acceptance/grants_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}")`),
})
}

Expand Down Expand Up @@ -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`),
})
}
Loading