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

Removed CustomizeDiff and Client Side Validation for databricks_grants #3290

Merged
merged 6 commits into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions catalog/permissions/permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ func (sm SecurableMapping) KeyValue(d attributeGetter) (string, string) {
}
return field, v
}
log.Printf("[WARN] Unexpected resource or permissions. Please proceed at your own risk.")
return "unknown", "unknown"
}
func (sm SecurableMapping) Id(d *schema.ResourceData) string {
Expand Down
182 changes: 4 additions & 178 deletions catalog/resource_grants.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,171 +89,6 @@ func replaceAllPermissions(a permissions.UnityCatalogPermissionsAPI, securable s
})
}

type securableMapping map[string]map[string]bool

// reuse ResourceDiff and ResourceData
type attributeGetter interface {
Get(key string) any
}

func (sm securableMapping) kv(d attributeGetter) (string, string) {
for field := range sm {
v := d.Get(field).(string)
if v == "" {
continue
}
return field, v
}
return "unknown", "unknown"
}

func (sm securableMapping) id(d *schema.ResourceData) string {
securable, name := sm.kv(d)
return fmt.Sprintf("%s/%s", securable, name)
}

func (sm securableMapping) validate(d attributeGetter, pl PermissionsList) error {
securable, _ := sm.kv(d)
allowed, ok := sm[securable]
if !ok {
return fmt.Errorf(`%s is not fully supported yet`, securable)
}
for _, v := range pl.Assignments {
for _, priv := range v.Privileges {
if !allowed[strings.ToUpper(priv)] {
// check if user uses spaces instead of underscores
if allowed[strings.ReplaceAll(priv, " ", "_")] {
return fmt.Errorf(`%s is not allowed on %s. Did you mean %s?`, priv, securable, strings.ReplaceAll(priv, " ", "_"))
}
return fmt.Errorf(`%s is not allowed on %s`, priv, securable)
}
}
}
return nil
}

var mapping = securableMapping{
// add other securable mappings once needed
"table": {
"MODIFY": true,
"SELECT": true,

// v1.0
"ALL_PRIVILEGES": true,
"APPLY_TAG": true,
"BROWSE": true,
},
"catalog": {
"CREATE": true,
"USAGE": true,

// v1.0
"ALL_PRIVILEGES": true,
"APPLY_TAG": true,
"USE_CATALOG": true,
"USE_SCHEMA": true,
"CREATE_SCHEMA": true,
"CREATE_TABLE": true,
"CREATE_FUNCTION": true,
"CREATE_MATERIALIZED_VIEW": true,
"CREATE_MODEL": true,
"CREATE_VOLUME": true,
"READ_VOLUME": true,
"WRITE_VOLUME": true,
"EXECUTE": true,
"MODIFY": true,
"SELECT": true,
"REFRESH": true,
"BROWSE": true,
},
"schema": {
"CREATE": true,
"USAGE": true,

// v1.0
"ALL_PRIVILEGES": true,
"APPLY_TAG": true,
"USE_SCHEMA": true,
"CREATE_TABLE": true,
"CREATE_FUNCTION": true,
"CREATE_MATERIALIZED_VIEW": true,
"CREATE_MODEL": true,
"CREATE_VOLUME": true,
"READ_VOLUME": true,
"WRITE_VOLUME": true,
"EXECUTE": true,
"MODIFY": true,
"SELECT": true,
"REFRESH": true,
"BROWSE": true,
},
"storage_credential": {
"CREATE_TABLE": true,
"READ_FILES": true,
"WRITE_FILES": true,
"CREATE_EXTERNAL_LOCATION": true,

// v1.0
"ALL_PRIVILEGES": true,
"CREATE_EXTERNAL_TABLE": true,
},
"external_location": {
"CREATE_TABLE": true,
"READ_FILES": true,
"WRITE_FILES": true,

// v1.0
"ALL_PRIVILEGES": true,
"CREATE_EXTERNAL_TABLE": true,
"CREATE_MANAGED_STORAGE": true,
"CREATE_EXTERNAL_VOLUME": true,
"BROWSE": true,
},
"metastore": {
// v1.0
"CREATE_CATALOG": true,
"CREATE_CLEAN_ROOM": true,
"CREATE_CONNECTION": true,
"CREATE_EXTERNAL_LOCATION": true,
"CREATE_STORAGE_CREDENTIAL": true,
"CREATE_SHARE": true,
"CREATE_RECIPIENT": true,
"CREATE_PROVIDER": true,
"MANAGE_ALLOWLIST": true,
"USE_CONNECTION": true,
"USE_PROVIDER": true,
"USE_SHARE": true,
"USE_RECIPIENT": true,
"USE_MARKETPLACE_ASSETS": true,
"SET_SHARE_PERMISSION": true,
},
"function": {
"ALL_PRIVILEGES": true,
"EXECUTE": true,
},
"model": {
"ALL_PRIVILEGES": true,
"APPLY_TAG": true,
"EXECUTE": true,
},
"share": {
"SELECT": true,
},
"volume": {
"ALL_PRIVILEGES": true,
"READ_VOLUME": true,
"WRITE_VOLUME": true,
},
// avoid reserved field
"foreign_connection": {
"ALL_PRIVILEGES": true,
"CREATE_FOREIGN_CATALOG": true,
"CREATE_FOREIGN_SCHEMA": true,
"CREATE_FOREIGN_TABLE": true,
"USE_CONNECTION": true,
},
}

func (pl PermissionsList) toSdkPermissionsList() (out catalog.PermissionsList) {
for _, v := range pl.Assignments {
privileges := []catalog.Privilege{}
Expand Down Expand Up @@ -294,30 +129,21 @@ func ResourceGrants() common.Resource {
s := common.StructToSchema(PermissionsList{},
func(s map[string]*schema.Schema) map[string]*schema.Schema {
alof := []string{}
for field := range mapping {
for field := range permissions.Mappings {
s[field] = &schema.Schema{
Type: schema.TypeString,
ForceNew: true,
Optional: true,
}
alof = append(alof, field)
}
for field := range mapping {
for field := range permissions.Mappings {
s[field].AtLeastOneOf = alof
}
return s
})
return common.Resource{
Schema: s,
CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
if d.Id() == "" {
// unfortunately we cannot do validation before dependent resources exist with tfsdkv2
return nil
}
var grants PermissionsList
common.DiffToStructPointer(d, s, &grants)
return mapping.validate(d, grants)
},
Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
w, err := c.WorkspaceClient()
if err != nil {
Expand All @@ -329,13 +155,13 @@ func ResourceGrants() common.Resource {
}
var grants PermissionsList
common.DataToStructPointer(d, s, &grants)
securable, name := mapping.kv(d)
securable, name := permissions.Mappings.KeyValue(d)
unityCatalogPermissionsAPI := permissions.NewUnityCatalogPermissionsAPI(ctx, c)
err = replaceAllPermissions(unityCatalogPermissionsAPI, securable, name, grants.toSdkPermissionsList())
if err != nil {
return err
}
d.SetId(mapping.id(d))
d.SetId(permissions.Mappings.Id(d))
return nil
},
Read: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
Expand Down
49 changes: 0 additions & 49 deletions catalog/resource_grants_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,31 +358,6 @@ func TestGrantReadMalformedId(t *testing.T) {
}.ExpectError(t, "ID must be two elements split by `/`: foo.bar")
}

type data map[string]string

func (a data) Get(k string) any {
return a[k]
}

func TestMappingUnsupported(t *testing.T) {
d := data{"nothing": "here"}
err := mapping.validate(d, PermissionsList{})
assert.EqualError(t, err, "unknown is not fully supported yet")
}

func TestInvalidPrivilege(t *testing.T) {
d := data{"table": "me"}
err := mapping.validate(d, PermissionsList{
Assignments: []PrivilegeAssignment{
{
Principal: "me",
Privileges: []string{"EVERYTHING"},
},
},
})
assert.EqualError(t, err, "EVERYTHING is not allowed on table")
}

func TestPermissionsList_Diff_ExternallyAddedPrincipal(t *testing.T) {
diff := diffPermissions(
catalog.PermissionsList{ // config
Expand Down Expand Up @@ -600,30 +575,6 @@ func TestShareGrantUpdate(t *testing.T) {
}.ApplyNoError(t)
}

func TestPrivilegeWithSpace(t *testing.T) {
d := data{"table": "me"}
err := mapping.validate(d, PermissionsList{
Assignments: []PrivilegeAssignment{
{
Principal: "me",
Privileges: []string{"ALL PRIVILEGES"},
},
},
})
assert.EqualError(t, err, "ALL PRIVILEGES is not allowed on table. Did you mean ALL_PRIVILEGES?")

d = data{"external_location": "me"}
err = mapping.validate(d, PermissionsList{
Assignments: []PrivilegeAssignment{
{
Principal: "me",
Privileges: []string{"CREATE TABLE"},
},
},
})
assert.EqualError(t, err, "CREATE TABLE is not allowed on external_location. Did you mean CREATE_TABLE?")
}

func TestConnectionGrantCreate(t *testing.T) {
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
Expand Down
38 changes: 34 additions & 4 deletions internal/acceptance/grant_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package acceptance

import (
"fmt"
"regexp"
"strings"
"testing"
)
Expand Down Expand Up @@ -99,8 +101,36 @@ resource "databricks_grant" "some" {
func TestUcAccGrant(t *testing.T) {
unityWorkspaceLevel(t, step{
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(grantTemplate, "%s", "{env.TEST_DATA_SCI_GROUP}"),
})
}

func grantTemplateForNamePermissionChange(suffix string, permission string) string {
return fmt.Sprintf(`
resource "databricks_storage_credential" "external" {
name = "cred-{var.STICKY_RANDOM}%s"
aws_iam_role {
role_arn = "{env.TEST_METASTORE_DATA_ACCESS_ARN}"
}
comment = "Managed by TF"
}

resource "databricks_grant" "cred" {
storage_credential = databricks_storage_credential.external.id
principal = "{env.TEST_DATA_ENG_GROUP}"
privileges = ["%s"]
}
`, suffix, permission)
}

func TestUcAccGrantForIdChange(t *testing.T) {
unityWorkspaceLevel(t, step{
Template: grantTemplateForNamePermissionChange("-old", "ALL_PRIVILEGES"),
}, step{
Template: grantTemplateForNamePermissionChange("-new", "ALL_PRIVILEGES"),
}, step{
Template: grantTemplateForNamePermissionChange("-fail", "abc"),
ExpectError: regexp.MustCompile(`cannot create grant: Privilege abc is not applicable to this entity`),
})
}
40 changes: 36 additions & 4 deletions internal/acceptance/grants_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package acceptance

import (
"fmt"
"regexp"
"strings"
"testing"
)
Expand Down Expand Up @@ -105,8 +107,38 @@ resource "databricks_grants" "some" {
func TestUcAccGrants(t *testing.T) {
unityWorkspaceLevel(t, step{
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(grantsTemplate, "%s", "{env.TEST_DATA_SCI_GROUP}"),
})
}

func grantsTemplateForNamePermissionChange(suffix string, permission string) string {
return fmt.Sprintf(`
resource "databricks_storage_credential" "external" {
name = "cred-{var.STICKY_RANDOM}%s"
aws_iam_role {
role_arn = "{env.TEST_METASTORE_DATA_ACCESS_ARN}"
}
comment = "Managed by TF"
}

resource "databricks_grants" "cred" {
storage_credential = databricks_storage_credential.external.id
grant {
principal = "{env.TEST_DATA_ENG_GROUP}"
privileges = ["%s"]
}
}
`, suffix, permission)
}

func TestUcAccGrantsForIdChange(t *testing.T) {
unityWorkspaceLevel(t, step{
Template: grantsTemplateForNamePermissionChange("-old", "ALL_PRIVILEGES"),
}, step{
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`),
})
}
Loading