Skip to content

Commit f6b5df9

Browse files
tanmay-dbtanmay-db
authored
Removed CustomizeDiff and Client Side Validation for databricks_grants (#3290)
* Remove CustomizeDiff and Client Side Validation for databricks_grants * - * - * - * - * -
1 parent b38e059 commit f6b5df9

File tree

5 files changed

+75
-235
lines changed

5 files changed

+75
-235
lines changed

‎catalog/permissions/permissions.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ func (sm SecurableMapping) KeyValue(d attributeGetter) (string, string) {
9292
}
9393
return field, v
9494
}
95+
log.Printf("[WARN] Unexpected resource or permissions. Please proceed at your own risk.")
9596
return "unknown", "unknown"
9697
}
9798
func (sm SecurableMapping) Id(d *schema.ResourceData) string {

‎catalog/resource_grants.go‎

Lines changed: 4 additions & 178 deletions
Original file line numberDiff line numberDiff line change
@@ -89,171 +89,6 @@ func replaceAllPermissions(a permissions.UnityCatalogPermissionsAPI, securable s
8989
})
9090
}
9191

92-
type securableMapping map[string]map[string]bool
93-
94-
// reuse ResourceDiff and ResourceData
95-
type attributeGetter interface {
96-
Get(key string) any
97-
}
98-
99-
func (sm securableMapping) kv(d attributeGetter) (string, string) {
100-
for field := range sm {
101-
v := d.Get(field).(string)
102-
if v == "" {
103-
continue
104-
}
105-
return field, v
106-
}
107-
return "unknown", "unknown"
108-
}
109-
110-
func (sm securableMapping) id(d *schema.ResourceData) string {
111-
securable, name := sm.kv(d)
112-
return fmt.Sprintf("%s/%s", securable, name)
113-
}
114-
115-
func (sm securableMapping) validate(d attributeGetter, pl PermissionsList) error {
116-
securable, _ := sm.kv(d)
117-
allowed, ok := sm[securable]
118-
if !ok {
119-
return fmt.Errorf(`%s is not fully supported yet`, securable)
120-
}
121-
for _, v := range pl.Assignments {
122-
for _, priv := range v.Privileges {
123-
if !allowed[strings.ToUpper(priv)] {
124-
// check if user uses spaces instead of underscores
125-
if allowed[strings.ReplaceAll(priv, " ", "_")] {
126-
return fmt.Errorf(`%s is not allowed on %s. Did you mean %s?`, priv, securable, strings.ReplaceAll(priv, " ", "_"))
127-
}
128-
return fmt.Errorf(`%s is not allowed on %s`, priv, securable)
129-
}
130-
}
131-
}
132-
return nil
133-
}
134-
135-
var mapping = securableMapping{
136-
// add other securable mappings once needed
137-
"table": {
138-
"MODIFY": true,
139-
"SELECT": true,
140-
141-
// v1.0
142-
"ALL_PRIVILEGES": true,
143-
"APPLY_TAG": true,
144-
"BROWSE": true,
145-
},
146-
"catalog": {
147-
"CREATE": true,
148-
"USAGE": true,
149-
150-
// v1.0
151-
"ALL_PRIVILEGES": true,
152-
"APPLY_TAG": true,
153-
"USE_CATALOG": true,
154-
"USE_SCHEMA": true,
155-
"CREATE_SCHEMA": true,
156-
"CREATE_TABLE": true,
157-
"CREATE_FUNCTION": true,
158-
"CREATE_MATERIALIZED_VIEW": true,
159-
"CREATE_MODEL": true,
160-
"CREATE_VOLUME": true,
161-
"READ_VOLUME": true,
162-
"WRITE_VOLUME": true,
163-
"EXECUTE": true,
164-
"MODIFY": true,
165-
"SELECT": true,
166-
"REFRESH": true,
167-
"BROWSE": true,
168-
},
169-
"schema": {
170-
"CREATE": true,
171-
"USAGE": true,
172-
173-
// v1.0
174-
"ALL_PRIVILEGES": true,
175-
"APPLY_TAG": true,
176-
"USE_SCHEMA": true,
177-
"CREATE_TABLE": true,
178-
"CREATE_FUNCTION": true,
179-
"CREATE_MATERIALIZED_VIEW": true,
180-
"CREATE_MODEL": true,
181-
"CREATE_VOLUME": true,
182-
"READ_VOLUME": true,
183-
"WRITE_VOLUME": true,
184-
"EXECUTE": true,
185-
"MODIFY": true,
186-
"SELECT": true,
187-
"REFRESH": true,
188-
"BROWSE": true,
189-
},
190-
"storage_credential": {
191-
"CREATE_TABLE": true,
192-
"READ_FILES": true,
193-
"WRITE_FILES": true,
194-
"CREATE_EXTERNAL_LOCATION": true,
195-
196-
// v1.0
197-
"ALL_PRIVILEGES": true,
198-
"CREATE_EXTERNAL_TABLE": true,
199-
},
200-
"external_location": {
201-
"CREATE_TABLE": true,
202-
"READ_FILES": true,
203-
"WRITE_FILES": true,
204-
205-
// v1.0
206-
"ALL_PRIVILEGES": true,
207-
"CREATE_EXTERNAL_TABLE": true,
208-
"CREATE_MANAGED_STORAGE": true,
209-
"CREATE_EXTERNAL_VOLUME": true,
210-
"BROWSE": true,
211-
},
212-
"metastore": {
213-
// v1.0
214-
"CREATE_CATALOG": true,
215-
"CREATE_CLEAN_ROOM": true,
216-
"CREATE_CONNECTION": true,
217-
"CREATE_EXTERNAL_LOCATION": true,
218-
"CREATE_STORAGE_CREDENTIAL": true,
219-
"CREATE_SHARE": true,
220-
"CREATE_RECIPIENT": true,
221-
"CREATE_PROVIDER": true,
222-
"MANAGE_ALLOWLIST": true,
223-
"USE_CONNECTION": true,
224-
"USE_PROVIDER": true,
225-
"USE_SHARE": true,
226-
"USE_RECIPIENT": true,
227-
"USE_MARKETPLACE_ASSETS": true,
228-
"SET_SHARE_PERMISSION": true,
229-
},
230-
"function": {
231-
"ALL_PRIVILEGES": true,
232-
"EXECUTE": true,
233-
},
234-
"model": {
235-
"ALL_PRIVILEGES": true,
236-
"APPLY_TAG": true,
237-
"EXECUTE": true,
238-
},
239-
"share": {
240-
"SELECT": true,
241-
},
242-
"volume": {
243-
"ALL_PRIVILEGES": true,
244-
"READ_VOLUME": true,
245-
"WRITE_VOLUME": true,
246-
},
247-
// avoid reserved field
248-
"foreign_connection": {
249-
"ALL_PRIVILEGES": true,
250-
"CREATE_FOREIGN_CATALOG": true,
251-
"CREATE_FOREIGN_SCHEMA": true,
252-
"CREATE_FOREIGN_TABLE": true,
253-
"USE_CONNECTION": true,
254-
},
255-
}
256-
25792
func (pl PermissionsList) toSdkPermissionsList() (out catalog.PermissionsList) {
25893
for _, v := range pl.Assignments {
25994
privileges := []catalog.Privilege{}
@@ -294,30 +129,21 @@ func ResourceGrants() common.Resource {
294129
s := common.StructToSchema(PermissionsList{},
295130
func(s map[string]*schema.Schema) map[string]*schema.Schema {
296131
alof := []string{}
297-
for field := range mapping {
132+
for field := range permissions.Mappings {
298133
s[field] = &schema.Schema{
299134
Type: schema.TypeString,
300135
ForceNew: true,
301136
Optional: true,
302137
}
303138
alof = append(alof, field)
304139
}
305-
for field := range mapping {
140+
for field := range permissions.Mappings {
306141
s[field].AtLeastOneOf = alof
307142
}
308143
return s
309144
})
310145
return common.Resource{
311146
Schema: s,
312-
CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
313-
if d.Id() == "" {
314-
// unfortunately we cannot do validation before dependent resources exist with tfsdkv2
315-
return nil
316-
}
317-
var grants PermissionsList
318-
common.DiffToStructPointer(d, s, &grants)
319-
return mapping.validate(d, grants)
320-
},
321147
Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
322148
w, err := c.WorkspaceClient()
323149
if err != nil {
@@ -329,13 +155,13 @@ func ResourceGrants() common.Resource {
329155
}
330156
var grants PermissionsList
331157
common.DataToStructPointer(d, s, &grants)
332-
securable, name := mapping.kv(d)
158+
securable, name := permissions.Mappings.KeyValue(d)
333159
unityCatalogPermissionsAPI := permissions.NewUnityCatalogPermissionsAPI(ctx, c)
334160
err = replaceAllPermissions(unityCatalogPermissionsAPI, securable, name, grants.toSdkPermissionsList())
335161
if err != nil {
336162
return err
337163
}
338-
d.SetId(mapping.id(d))
164+
d.SetId(permissions.Mappings.Id(d))
339165
return nil
340166
},
341167
Read: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {

‎catalog/resource_grants_test.go‎

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -358,31 +358,6 @@ func TestGrantReadMalformedId(t *testing.T) {
358358
}.ExpectError(t, "ID must be two elements split by `/`: foo.bar")
359359
}
360360

361-
type data map[string]string
362-
363-
func (a data) Get(k string) any {
364-
return a[k]
365-
}
366-
367-
func TestMappingUnsupported(t *testing.T) {
368-
d := data{"nothing": "here"}
369-
err := mapping.validate(d, PermissionsList{})
370-
assert.EqualError(t, err, "unknown is not fully supported yet")
371-
}
372-
373-
func TestInvalidPrivilege(t *testing.T) {
374-
d := data{"table": "me"}
375-
err := mapping.validate(d, PermissionsList{
376-
Assignments: []PrivilegeAssignment{
377-
{
378-
Principal: "me",
379-
Privileges: []string{"EVERYTHING"},
380-
},
381-
},
382-
})
383-
assert.EqualError(t, err, "EVERYTHING is not allowed on table")
384-
}
385-
386361
func TestPermissionsList_Diff_ExternallyAddedPrincipal(t *testing.T) {
387362
diff := diffPermissions(
388363
catalog.PermissionsList{ // config
@@ -600,30 +575,6 @@ func TestShareGrantUpdate(t *testing.T) {
600575
}.ApplyNoError(t)
601576
}
602577

603-
func TestPrivilegeWithSpace(t *testing.T) {
604-
d := data{"table": "me"}
605-
err := mapping.validate(d, PermissionsList{
606-
Assignments: []PrivilegeAssignment{
607-
{
608-
Principal: "me",
609-
Privileges: []string{"ALL PRIVILEGES"},
610-
},
611-
},
612-
})
613-
assert.EqualError(t, err, "ALL PRIVILEGES is not allowed on table. Did you mean ALL_PRIVILEGES?")
614-
615-
d = data{"external_location": "me"}
616-
err = mapping.validate(d, PermissionsList{
617-
Assignments: []PrivilegeAssignment{
618-
{
619-
Principal: "me",
620-
Privileges: []string{"CREATE TABLE"},
621-
},
622-
},
623-
})
624-
assert.EqualError(t, err, "CREATE TABLE is not allowed on external_location. Did you mean CREATE_TABLE?")
625-
}
626-
627578
func TestConnectionGrantCreate(t *testing.T) {
628579
qa.ResourceFixture{
629580
Fixtures: []qa.HTTPFixture{

‎internal/acceptance/grant_test.go‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package acceptance
22

33
import (
4+
"fmt"
5+
"regexp"
46
"strings"
57
"testing"
68
)
@@ -99,8 +101,36 @@ resource "databricks_grant" "some" {
99101
func TestUcAccGrant(t *testing.T) {
100102
unityWorkspaceLevel(t, step{
101103
Template: strings.ReplaceAll(grantTemplate, "%s", "{env.TEST_DATA_ENG_GROUP}"),
102-
},
103-
step{
104-
Template: strings.ReplaceAll(grantTemplate, "%s", "{env.TEST_DATA_SCI_GROUP}"),
105-
})
104+
}, step{
105+
Template: strings.ReplaceAll(grantTemplate, "%s", "{env.TEST_DATA_SCI_GROUP}"),
106+
})
107+
}
108+
109+
func grantTemplateForNamePermissionChange(suffix string, permission string) string {
110+
return fmt.Sprintf(`
111+
resource "databricks_storage_credential" "external" {
112+
name = "cred-{var.STICKY_RANDOM}%s"
113+
aws_iam_role {
114+
role_arn = "{env.TEST_METASTORE_DATA_ACCESS_ARN}"
115+
}
116+
comment = "Managed by TF"
117+
}
118+
119+
resource "databricks_grant" "cred" {
120+
storage_credential = databricks_storage_credential.external.id
121+
principal = "{env.TEST_DATA_ENG_GROUP}"
122+
privileges = ["%s"]
123+
}
124+
`, suffix, permission)
125+
}
126+
127+
func TestUcAccGrantForIdChange(t *testing.T) {
128+
unityWorkspaceLevel(t, step{
129+
Template: grantTemplateForNamePermissionChange("-old", "ALL_PRIVILEGES"),
130+
}, step{
131+
Template: grantTemplateForNamePermissionChange("-new", "ALL_PRIVILEGES"),
132+
}, step{
133+
Template: grantTemplateForNamePermissionChange("-fail", "abc"),
134+
ExpectError: regexp.MustCompile(`cannot create grant: Privilege abc is not applicable to this entity`),
135+
})
106136
}

‎internal/acceptance/grants_test.go‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package acceptance
22

33
import (
4+
"fmt"
5+
"regexp"
46
"strings"
57
"testing"
68
)
@@ -105,8 +107,38 @@ resource "databricks_grants" "some" {
105107
func TestUcAccGrants(t *testing.T) {
106108
unityWorkspaceLevel(t, step{
107109
Template: strings.ReplaceAll(grantsTemplate, "%s", "{env.TEST_DATA_ENG_GROUP}"),
108-
},
109-
step{
110-
Template: strings.ReplaceAll(grantsTemplate, "%s", "{env.TEST_DATA_SCI_GROUP}"),
111-
})
110+
}, step{
111+
Template: strings.ReplaceAll(grantsTemplate, "%s", "{env.TEST_DATA_SCI_GROUP}"),
112+
})
113+
}
114+
115+
func grantsTemplateForNamePermissionChange(suffix string, permission string) string {
116+
return fmt.Sprintf(`
117+
resource "databricks_storage_credential" "external" {
118+
name = "cred-{var.STICKY_RANDOM}%s"
119+
aws_iam_role {
120+
role_arn = "{env.TEST_METASTORE_DATA_ACCESS_ARN}"
121+
}
122+
comment = "Managed by TF"
123+
}
124+
125+
resource "databricks_grants" "cred" {
126+
storage_credential = databricks_storage_credential.external.id
127+
grant {
128+
principal = "{env.TEST_DATA_ENG_GROUP}"
129+
privileges = ["%s"]
130+
}
131+
}
132+
`, suffix, permission)
133+
}
134+
135+
func TestUcAccGrantsForIdChange(t *testing.T) {
136+
unityWorkspaceLevel(t, step{
137+
Template: grantsTemplateForNamePermissionChange("-old", "ALL_PRIVILEGES"),
138+
}, step{
139+
Template: grantsTemplateForNamePermissionChange("-new", "ALL_PRIVILEGES"),
140+
}, step{
141+
Template: grantsTemplateForNamePermissionChange("-fail", "abc"),
142+
ExpectError: regexp.MustCompile(`Error: cannot create grants: Privilege abc is not applicable to this entity`),
143+
})
112144
}

0 commit comments

Comments
 (0)