From 207dda6d838c061044d84b9276f1e61d599a0457 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Thu, 22 Feb 2024 12:17:41 -0600 Subject: [PATCH 01/16] Add Tests to Demonstrate Problem --- catalog/resource_grants_test.go | 46 +++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/catalog/resource_grants_test.go b/catalog/resource_grants_test.go index 01660863b0..fa2a52d2b7 100644 --- a/catalog/resource_grants_test.go +++ b/catalog/resource_grants_test.go @@ -741,3 +741,49 @@ func TestModelGrantCreate(t *testing.T) { }`, }.ApplyNoError(t) } + +func TestGrantCreateSpaces(t *testing.T) { + qa.ResourceFixture{ + Resource: ResourceGrants(), + Create: true, + HCL: ` + foreign_connection = "myconn" + + grant { + principal = "me" + privileges = ["USE CONNECTION"] + }`, + }.ExpectError(t, "USE CONNECTION is not allowed on foreign_connection. Did you mean USE_CONNECTION?") +} + +func TestGrantUpdateSpaces(t *testing.T) { + qa.ResourceFixture{ + Fixtures: []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.1/unity-catalog/volumes/myvolume/permissions?", + Response: catalog.PermissionsList{ + PrivilegeAssignments: []catalog.PrivilegeAssignment{ + { + Principal: "me", + Privileges: []catalog.Privilege{"ALL_PRIVILEGES"}, + }, + }, + }, + }, + }, + Resource: ResourceGrants(), + Update: true, + ID: "volumes/myvolume", + InstanceState: map[string]string{ + "volume": "myvolume", + }, + HCL: ` + volume = "myvolume" + + grant { + principal = "me" + privileges = ["ALL PRIVILEGES"] + }`, + }.ExpectError(t, "ALL PRIVILEGES is not allowed on volume. Did you mean ALL_PRIVILEGES?") +} From 49e2d1e9b7fae2b1658cfb27464d6df8e9866ac0 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Thu, 22 Feb 2024 13:20:36 -0600 Subject: [PATCH 02/16] Validate Privs for Create/Update --- catalog/resource_grants.go | 8 ++++++++ catalog/resource_grants_test.go | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/catalog/resource_grants.go b/catalog/resource_grants.go index 8834e609c9..f2c799b430 100644 --- a/catalog/resource_grants.go +++ b/catalog/resource_grants.go @@ -330,6 +330,10 @@ func ResourceGrants() common.Resource { var grants PermissionsList common.DataToStructPointer(d, s, &grants) securable, name := mapping.kv(d) + err = mapping.validate(d, grants) + if err != nil { + return err + } unityCatalogPermissionsAPI := permissions.NewUnityCatalogPermissionsAPI(ctx, c) err = replaceAllPermissions(unityCatalogPermissionsAPI, securable, name, grants.toSdkPermissionsList()) if err != nil { @@ -373,6 +377,10 @@ func ResourceGrants() common.Resource { } var grants PermissionsList common.DataToStructPointer(d, s, &grants) + err = mapping.validate(d, grants) + if err != nil { + return err + } unityCatalogPermissionsAPI := permissions.NewUnityCatalogPermissionsAPI(ctx, c) return replaceAllPermissions(unityCatalogPermissionsAPI, securable, name, grants.toSdkPermissionsList()) }, diff --git a/catalog/resource_grants_test.go b/catalog/resource_grants_test.go index fa2a52d2b7..35a132f6b1 100644 --- a/catalog/resource_grants_test.go +++ b/catalog/resource_grants_test.go @@ -9,7 +9,7 @@ import ( ) func TestPermissionsCornerCases(t *testing.T) { - qa.ResourceCornerCases(t, ResourceGrants(), qa.CornerCaseID("schema/sandbox")) + qa.ResourceCornerCases(t, ResourceGrants(), qa.CornerCaseSkipCRUD("Validation prevents CRUD")) } func TestGrantCreate(t *testing.T) { From aabb111f377997bc81d07e4e27869ac5c6595379 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Thu, 22 Feb 2024 14:38:24 -0600 Subject: [PATCH 03/16] Fix format --- catalog/resource_grants.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/catalog/resource_grants.go b/catalog/resource_grants.go index 58e9c69700..e953619bdb 100644 --- a/catalog/resource_grants.go +++ b/catalog/resource_grants.go @@ -155,7 +155,7 @@ func ResourceGrants() common.Resource { } var grants PermissionsList common.DataToStructPointer(d, s, &grants) - err = mapping.validate(d, grants) + err = mapping.validate(d, grants) if err != nil { return err } From 7111b110e489c15d938f1239654cb78e00ebaca6 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Thu, 22 Feb 2024 16:35:30 -0600 Subject: [PATCH 04/16] Switch to normalizing privs --- catalog/resource_grants.go | 17 ++++---- catalog/resource_grants_test.go | 70 +++++++++++---------------------- 2 files changed, 31 insertions(+), 56 deletions(-) diff --git a/catalog/resource_grants.go b/catalog/resource_grants.go index e953619bdb..45897dcef2 100644 --- a/catalog/resource_grants.go +++ b/catalog/resource_grants.go @@ -30,8 +30,14 @@ type PermissionsList struct { func diffPermissions(pl catalog.PermissionsList, existing catalog.PermissionsList) (diff []catalog.PermissionsChange) { // diffs change sets configured := map[string]*schema.Set{} + // normalize the configured privileges to match API behavior by treating spaces as underscores for _, v := range pl.PrivilegeAssignments { - configured[v.Principal] = permissions.SliceToSet(v.Privileges) + normalizedPrivileges := []catalog.Privilege{} + for _, p := range v.Privileges { + normalizedPriv := strings.ReplaceAll(p.String(), " ", "_") + normalizedPrivileges = append(normalizedPrivileges, catalog.Privilege(normalizedPriv)) + } + configured[v.Principal] = permissions.SliceToSet(normalizedPrivileges) } // existing permissions that needs removal remote := map[string]*schema.Set{} @@ -80,6 +86,7 @@ func replaceAllPermissions(a permissions.UnityCatalogPermissionsAPI, securable s if err != nil { return err } + err = a.UpdatePermissions(securableType, name, diffPermissions(list, *existing)) if err != nil { return err @@ -155,10 +162,6 @@ func ResourceGrants() common.Resource { } var grants PermissionsList common.DataToStructPointer(d, s, &grants) - err = mapping.validate(d, grants) - if err != nil { - return err - } securable, name := permissions.Mappings.KeyValue(d) unityCatalogPermissionsAPI := permissions.NewUnityCatalogPermissionsAPI(ctx, c) err = replaceAllPermissions(unityCatalogPermissionsAPI, securable, name, grants.toSdkPermissionsList()) @@ -203,10 +206,6 @@ func ResourceGrants() common.Resource { } var grants PermissionsList common.DataToStructPointer(d, s, &grants) - err = mapping.validate(d, grants) - if err != nil { - return err - } unityCatalogPermissionsAPI := permissions.NewUnityCatalogPermissionsAPI(ctx, c) return replaceAllPermissions(unityCatalogPermissionsAPI, securable, name, grants.toSdkPermissionsList()) }, diff --git a/catalog/resource_grants_test.go b/catalog/resource_grants_test.go index 56359c60b4..99f9e26e66 100644 --- a/catalog/resource_grants_test.go +++ b/catalog/resource_grants_test.go @@ -9,7 +9,7 @@ import ( ) func TestPermissionsCornerCases(t *testing.T) { - qa.ResourceCornerCases(t, ResourceGrants(), qa.CornerCaseSkipCRUD("Validation prevents CRUD")) + qa.ResourceCornerCases(t, ResourceGrants(), qa.CornerCaseID("schema/sandbox")) } func TestGrantCreate(t *testing.T) { @@ -444,6 +444,28 @@ func TestPermissionsList_Diff_LocalRemoteDiff(t *testing.T) { assert.Equal(t, catalog.Privilege("c"), diff[0].Remove[0]) } +func TestPermissionsList_Diff_Spaces(t *testing.T) { + diff := diffPermissions( + catalog.PermissionsList{ // config + PrivilegeAssignments: []catalog.PrivilegeAssignment{ + { + Principal: "a", + Privileges: []catalog.Privilege{"a b"}, + }, + }, + }, + catalog.PermissionsList{ + PrivilegeAssignments: []catalog.PrivilegeAssignment{ // platform + { + Principal: "a", + Privileges: []catalog.Privilege{"a_b"}, + }, + }, + }, + ) + assert.Len(t, diff, 0) +} + func TestShareGrantCreate(t *testing.T) { qa.ResourceFixture{ Fixtures: []qa.HTTPFixture{ @@ -692,49 +714,3 @@ func TestModelGrantCreate(t *testing.T) { }`, }.ApplyNoError(t) } - -func TestGrantCreateSpaces(t *testing.T) { - qa.ResourceFixture{ - Resource: ResourceGrants(), - Create: true, - HCL: ` - foreign_connection = "myconn" - - grant { - principal = "me" - privileges = ["USE CONNECTION"] - }`, - }.ExpectError(t, "USE CONNECTION is not allowed on foreign_connection. Did you mean USE_CONNECTION?") -} - -func TestGrantUpdateSpaces(t *testing.T) { - qa.ResourceFixture{ - Fixtures: []qa.HTTPFixture{ - { - Method: "GET", - Resource: "/api/2.1/unity-catalog/volumes/myvolume/permissions?", - Response: catalog.PermissionsList{ - PrivilegeAssignments: []catalog.PrivilegeAssignment{ - { - Principal: "me", - Privileges: []catalog.Privilege{"ALL_PRIVILEGES"}, - }, - }, - }, - }, - }, - Resource: ResourceGrants(), - Update: true, - ID: "volumes/myvolume", - InstanceState: map[string]string{ - "volume": "myvolume", - }, - HCL: ` - volume = "myvolume" - - grant { - principal = "me" - privileges = ["ALL PRIVILEGES"] - }`, - }.ExpectError(t, "ALL PRIVILEGES is not allowed on volume. Did you mean ALL_PRIVILEGES?") -} From e346ea89c785ff3eb062181ffb3416e142596ae2 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Wed, 28 Feb 2024 08:31:42 -0600 Subject: [PATCH 05/16] Add Test for resource_grant (same scenario) --- catalog/resource_grant_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/catalog/resource_grant_test.go b/catalog/resource_grant_test.go index 43a0eeb6b9..838f327106 100644 --- a/catalog/resource_grant_test.go +++ b/catalog/resource_grant_test.go @@ -585,6 +585,29 @@ func TestResourceGrantPermissionsList_Diff_LocalRemoteDiff(t *testing.T) { assert.Equal(t, catalog.Privilege("c"), diff[0].Remove[0]) } +func TestResourceGrantPermissionsList_Diff_Spaces(t *testing.T) { + diff := diffPermissionsForPrincipal( + "a", + catalog.PermissionsList{ // config + PrivilegeAssignments: []catalog.PrivilegeAssignment{ + { + Principal: "a", + Privileges: []catalog.Privilege{"a b"}, + }, + }, + }, + catalog.PermissionsList{ + PrivilegeAssignments: []catalog.PrivilegeAssignment{ // platform + { + Principal: "a", + Privileges: []catalog.Privilege{"a_b"}, + }, + }, + }, + ) + assert.Len(t, diff, 0) +} + func TestResourceGrantShareGrantCreate(t *testing.T) { qa.ResourceFixture{ Fixtures: []qa.HTTPFixture{ From c99e9ac893a928d493d130b8bd94e9f63eb82bce Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Wed, 28 Feb 2024 08:40:59 -0600 Subject: [PATCH 06/16] Port fix for resource_grant --- catalog/resource_grant.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/catalog/resource_grant.go b/catalog/resource_grant.go index 2c8cd174cd..f844922567 100644 --- a/catalog/resource_grant.go +++ b/catalog/resource_grant.go @@ -21,7 +21,12 @@ func diffPermissionsForPrincipal(principal string, desired catalog.PermissionsLi configured := map[string]*schema.Set{} for _, v := range desired.PrivilegeAssignments { if v.Principal == principal { - configured[v.Principal] = permissions.SliceToSet(v.Privileges) + normalizedPrivileges := []catalog.Privilege{} + for _, p := range v.Privileges { + normalizedPriv := strings.ReplaceAll(p.String(), " ", "_") + normalizedPrivileges = append(normalizedPrivileges, catalog.Privilege(normalizedPriv)) + } + configured[v.Principal] = permissions.SliceToSet(normalizedPrivileges) } } // existing permissions that needs removal for principal From 600c2881c519b100179c39c82a11f27320220e12 Mon Sep 17 00:00:00 2001 From: Sven Grosen <> Date: Mon, 18 Mar 2024 22:32:18 -0500 Subject: [PATCH 07/16] Point check-in: Figuring Out CustomizeDiff Testing --- .devcontainer/devcontainer.json | 2 +- catalog/resource_grant.go | 35 ++++++++++++++++++++++++++++----- catalog/resource_grant_test.go | 35 +++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 6 deletions(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 1d955cb3f6..0e9c134d8c 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -8,7 +8,7 @@ // Update the VARIANT arg to pick a version of Go: 1, 1.18, 1.17 // Append -bullseye or -buster to pin to an OS version. // Use -bullseye variants on local arm64/Apple Silicon. - "VARIANT": "1.19-bullseye", + "VARIANT": "1.21-bullseye", // Options "NODE_VERSION": "none" } diff --git a/catalog/resource_grant.go b/catalog/resource_grant.go index f844922567..4d27b0443d 100644 --- a/catalog/resource_grant.go +++ b/catalog/resource_grant.go @@ -15,17 +15,23 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) +// normalizePrivileges return an array of catalog.Privilege with privilege values normalized to match API behavior +func normalizePrivileges(privileges []catalog.Privilege) []catalog.Privilege { + normalizedPrivileges := []catalog.Privilege{} + for _, p := range privileges { + normalizedPriv := strings.ReplaceAll(p.String(), " ", "_") + normalizedPrivileges = append(normalizedPrivileges, catalog.Privilege(normalizedPriv)) + } + return normalizedPrivileges +} + // diffPermissionsForPrincipal returns an array of catalog.PermissionsChange of this permissions list with `diff` privileges removed func diffPermissionsForPrincipal(principal string, desired catalog.PermissionsList, existing catalog.PermissionsList) (diff []catalog.PermissionsChange) { // diffs change sets for principal configured := map[string]*schema.Set{} for _, v := range desired.PrivilegeAssignments { if v.Principal == principal { - normalizedPrivileges := []catalog.Privilege{} - for _, p := range v.Privileges { - normalizedPriv := strings.ReplaceAll(p.String(), " ", "_") - normalizedPrivileges = append(normalizedPrivileges, catalog.Privilege(normalizedPriv)) - } + normalizedPrivileges := normalizePrivileges(v.Privileges) configured[v.Principal] = permissions.SliceToSet(normalizedPrivileges) } } @@ -236,5 +242,24 @@ func ResourceGrant() common.Resource { unityCatalogPermissionsAPI := permissions.NewUnityCatalogPermissionsAPI(ctx, c) return replacePermissionsForPrincipal(unityCatalogPermissionsAPI, securable, name, principal, catalog.PermissionsList{}) }, + CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error { + if d.HasChange("privileges") { + // validate that changes are not just differences between spaces and underscores + // between words within each privilege + old, new := d.GetChange("privileges") + oldPrivs := permissions.SetToSlice(old.(*schema.Set)) + newPrivs := permissions.SetToSlice(new.(*schema.Set)) + normalizedOld := permissions.SliceToSet(normalizePrivileges(oldPrivs)) + normalizedNew := permissions.SliceToSet(normalizePrivileges(newPrivs)) + + add := permissions.SetToSlice(normalizedOld.Difference(normalizedNew)) + remove := permissions.SetToSlice(normalizedNew.Difference(normalizedOld)) + + if len(add) == 0 && len(remove) == 0 { + d.Clear("privileges") + } + } + return nil + }, } } diff --git a/catalog/resource_grant_test.go b/catalog/resource_grant_test.go index 838f327106..7fd1a409b2 100644 --- a/catalog/resource_grant_test.go +++ b/catalog/resource_grant_test.go @@ -857,3 +857,38 @@ func TestResourceGrantModelGrantCreate(t *testing.T) { `, }.ApplyNoError(t) } + +func TestResourceGrantUpdateSpacesNoUpdate(t *testing.T) { + qa.ResourceFixture{ + Fixtures: []qa.HTTPFixture{ + { + Method: "GET", + Resource: "/api/2.1/unity-catalog/permissions/table/foo.bar.baz?", + Response: catalog.PermissionsList{ + PrivilegeAssignments: []catalog.PrivilegeAssignment{ + { + Principal: "me", + Privileges: []catalog.Privilege{"ALL_PRIVILEGES"}, + }, + }, + }, + }, + }, + Resource: ResourceGrant(), + Update: true, + //RequiresNew: false, + ID: "table/foo.bar.baz/me", + InstanceState: map[string]string{ + "table": "foo.bar.baz", + "principal": "me", + "privileges": "[ALL_PRIVILEGES]", + }, + //ExpectedDiff: map[string]*terraform.ResourceAttrDiff{}, + HCL: ` + table = "foo.bar.baz" + + principal = "me" + privileges = ["ALL PRIVILEGES"] + `, + }.ApplyNoError(t) +} From ebfa348a7d7925810ca431b0bf5db73d8cd958a2 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Tue, 19 Mar 2024 11:51:41 -0500 Subject: [PATCH 08/16] Implement CustomizeDiff with Error --- CONTRIBUTING.md | 2 +- catalog/resource_grant.go | 35 ++++++++++++++++++++++------------ catalog/resource_grant_test.go | 29 +++++++--------------------- common/resource.go | 2 +- qa/testing.go | 2 +- 5 files changed, 33 insertions(+), 37 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index caae05a063..858de6cf5b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -353,7 +353,7 @@ So please run `make lint` instead. NOTE: This use of devcontainers for terraform-provider-databricks development is **experimental** and not officially supported by Databricks -This project has configuration for working with [Visual Studio Code Devcontainers](https://code.visualstudio.com/docs/remote/containers) - this allows you to containerise your development rerequisites (e.g. golang, terraform). To use this you will need [Visual Studio Code](https://code.visualstudio.com/) and [Docker](https://www.docker.com/products/docker-desktop). +This project has configuration for working with [Visual Studio Code Devcontainers](https://code.visualstudio.com/docs/remote/containers) - this allows you to containerize your development prerequisites (e.g. golang, terraform). To use this you will need [Visual Studio Code](https://code.visualstudio.com/) and [Docker](https://www.docker.com/products/docker-desktop). To get started, clone this repo and open the folder with Visual Studio Code. If you don't have the [Remote Development extension](https://marketplace.visualstudio.com/items?itemName=ms-vscode-remote.vscode-remote-extensionpack) then you should be prompted to install it. diff --git a/catalog/resource_grant.go b/catalog/resource_grant.go index 4d27b0443d..70ea69ace5 100644 --- a/catalog/resource_grant.go +++ b/catalog/resource_grant.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log" "sort" "strings" "time" @@ -243,20 +244,30 @@ func ResourceGrant() common.Resource { return replacePermissionsForPrincipal(unityCatalogPermissionsAPI, securable, name, principal, catalog.PermissionsList{}) }, CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error { - if d.HasChange("privileges") { - // validate that changes are not just differences between spaces and underscores - // between words within each privilege - old, new := d.GetChange("privileges") - oldPrivs := permissions.SetToSlice(old.(*schema.Set)) - newPrivs := permissions.SetToSlice(new.(*schema.Set)) - normalizedOld := permissions.SliceToSet(normalizePrivileges(oldPrivs)) - normalizedNew := permissions.SliceToSet(normalizePrivileges(newPrivs)) + changedPrivileges := d.GetChangedKeysPrefix("privileges") - add := permissions.SetToSlice(normalizedOld.Difference(normalizedNew)) - remove := permissions.SetToSlice(normalizedNew.Difference(normalizedOld)) + if len(changedPrivileges) > 0 { - if len(add) == 0 && len(remove) == 0 { - d.Clear("privileges") + log.Printf("[DEBUG] CustomizeDiff: changedPrivileges: %v", changedPrivileges) + + for _, k := range changedPrivileges { + old, new := d.GetChange(k) + + oldString := old.(string) + newString := new.(string) + + log.Printf("[DEBUG] CustomizeDiff: oldString: %s newString: %s", oldString, newString) + + // if the only difference is spaces, remove the change + if newString != oldString && strings.ReplaceAll(newString, " ", "_") == oldString { + log.Printf("[DEBUG] CustomizeDiff: removing change: %s", k) + // this doesn't work since these privileges are not computed... + err := d.Clear(k) + if err != nil { + log.Printf("[DEBUG] CustomizeDiff: error clearing change: %s with error: %v", k, err) + return fmt.Errorf("privilege %s only differs from current privilege %s by spaces, please update to match current", newString, oldString) + } + } } } return nil diff --git a/catalog/resource_grant_test.go b/catalog/resource_grant_test.go index 7fd1a409b2..53e1e6bb43 100644 --- a/catalog/resource_grant_test.go +++ b/catalog/resource_grant_test.go @@ -860,35 +860,20 @@ func TestResourceGrantModelGrantCreate(t *testing.T) { func TestResourceGrantUpdateSpacesNoUpdate(t *testing.T) { qa.ResourceFixture{ - Fixtures: []qa.HTTPFixture{ - { - Method: "GET", - Resource: "/api/2.1/unity-catalog/permissions/table/foo.bar.baz?", - Response: catalog.PermissionsList{ - PrivilegeAssignments: []catalog.PrivilegeAssignment{ - { - Principal: "me", - Privileges: []catalog.Privilege{"ALL_PRIVILEGES"}, - }, - }, - }, - }, - }, Resource: ResourceGrant(), - Update: true, - //RequiresNew: false, - ID: "table/foo.bar.baz/me", + ID: "table/foo.bar.baz/me", InstanceState: map[string]string{ - "table": "foo.bar.baz", - "principal": "me", - "privileges": "[ALL_PRIVILEGES]", + "table": "foo.bar.baz", + "principal": "me", + "privileges.#": "1", + "privileges.3182106578": "ALL_PRIVILEGES", // this indexer seems like a huge hack }, - //ExpectedDiff: map[string]*terraform.ResourceAttrDiff{}, + Update: true, HCL: ` table = "foo.bar.baz" principal = "me" privileges = ["ALL PRIVILEGES"] `, - }.ApplyNoError(t) + }.ExpectError(t, "privilege ALL PRIVILEGES only differs from current privilege ALL_PRIVILEGES by spaces, please update to match current") } diff --git a/common/resource.go b/common/resource.go index 4d5de0b10f..fc491c84c5 100644 --- a/common/resource.go +++ b/common/resource.go @@ -63,7 +63,7 @@ func (r Resource) saferCustomizeDiff() schema.CustomizeDiffFunc { return func(ctx context.Context, rd *schema.ResourceDiff, _ any) (err error) { defer func() { // this is deliberate decision to convert a panic into error, - // so that any unforeseen bug would we visible to end-user + // so that any unforeseen bug would be visible to end-user // as an error and not a provider crash, which is way less // of pleasant experience. if panic := recover(); panic != nil { diff --git a/qa/testing.go b/qa/testing.go index ee7adf60cf..4c16b25084 100644 --- a/qa/testing.go +++ b/qa/testing.go @@ -124,7 +124,7 @@ type ResourceFixture struct { New bool } -// wrapper type for calling resource methords +// wrapper type for calling resource methods type resourceCRUD func(context.Context, *schema.ResourceData, any) diag.Diagnostics func (cb resourceCRUD) before(before func(d *schema.ResourceData)) resourceCRUD { From dfaa2520243e11891255b00c9a82b6947d636a5d Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Tue, 26 Mar 2024 11:04:30 -0500 Subject: [PATCH 09/16] Account for non-string diffs for now --- catalog/resource_grant.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/catalog/resource_grant.go b/catalog/resource_grant.go index 70ea69ace5..6f7ad803a3 100644 --- a/catalog/resource_grant.go +++ b/catalog/resource_grant.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log" + "reflect" "sort" "strings" "time" @@ -253,13 +254,22 @@ func ResourceGrant() common.Resource { for _, k := range changedPrivileges { old, new := d.GetChange(k) + log.Printf("[DEBUG] CustomizeDiff: old: %v new: %v", old, new) + + oldType := reflect.TypeOf(old) + newType := reflect.TypeOf(new) + if oldType.String() != "string" || newType.String() != "string" { + log.Printf("[DEBUG] CustomizeDiff: oldType: %s newType: %s", oldType, newType) + continue + } + oldString := old.(string) newString := new.(string) log.Printf("[DEBUG] CustomizeDiff: oldString: %s newString: %s", oldString, newString) // if the only difference is spaces, remove the change - if newString != oldString && strings.ReplaceAll(newString, " ", "_") == oldString { + if oldString != newString && strings.ReplaceAll(newString, " ", "_") == oldString { log.Printf("[DEBUG] CustomizeDiff: removing change: %s", k) // this doesn't work since these privileges are not computed... err := d.Clear(k) From 79d952023cb68d661747660149980abb0837556e Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Fri, 29 Mar 2024 15:20:14 -0500 Subject: [PATCH 10/16] Use DiffSuppressFunc Instead --- catalog/permissions/permissions.go | 8 +++++ catalog/resource_grant.go | 49 +++++------------------------- catalog/resource_grant_test.go | 20 ------------ catalog/resource_grants.go | 7 +++++ 4 files changed, 23 insertions(+), 61 deletions(-) diff --git a/catalog/permissions/permissions.go b/catalog/permissions/permissions.go index c0c21b45e9..6e74f67580 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" @@ -144,3 +145,10 @@ func SliceWithoutString(in []string, without string) (out []string) { } return } + +// If a privilege/permission only differs with underscores replacing spaces +// then we can suppress the change +func PrivilegesSuppressWhitespaceChange(k, old, new string, _ *schema.ResourceData) bool { + log.Printf("[INFO] privilegesSuppressWhitespaceChange: k: %s old: %s new: %s", k, old, new) + return strings.ReplaceAll(old, " ", "_") == strings.ReplaceAll(new, " ", "_") +} diff --git a/catalog/resource_grant.go b/catalog/resource_grant.go index 6f7ad803a3..be6017fd13 100644 --- a/catalog/resource_grant.go +++ b/catalog/resource_grant.go @@ -4,8 +4,6 @@ import ( "context" "errors" "fmt" - "log" - "reflect" "sort" "strings" "time" @@ -132,10 +130,14 @@ func parseSecurableId(d *schema.ResourceData) (string, string, string, error) { return split[0], split[1], split[2], nil } +// func privilegesSuppressWhitespaceChange(k, old, new string, d *schema.ResourceData) bool { +// log.Printf("[INFO] privilegesSuppressWhitespaceChange: k: %s old: %s new: %s", k, old, new) +// return strings.ReplaceAll(old, " ", "_") == strings.ReplaceAll(new, " ", "_") +// } + func ResourceGrant() common.Resource { s := common.StructToSchema(permissions.UnityCatalogPrivilegeAssignment{}, func(m map[string]*schema.Schema) map[string]*schema.Schema { - m["principal"].ForceNew = true allFields := []string{} @@ -151,6 +153,9 @@ func ResourceGrant() common.Resource { ConflictsWith: permissions.SliceWithoutString(allFields, field), } } + + m["privileges"].DiffSuppressFunc = permissions.PrivilegesSuppressWhitespaceChange + return m }) @@ -244,43 +249,5 @@ func ResourceGrant() common.Resource { unityCatalogPermissionsAPI := permissions.NewUnityCatalogPermissionsAPI(ctx, c) return replacePermissionsForPrincipal(unityCatalogPermissionsAPI, securable, name, principal, catalog.PermissionsList{}) }, - CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error { - changedPrivileges := d.GetChangedKeysPrefix("privileges") - - if len(changedPrivileges) > 0 { - - log.Printf("[DEBUG] CustomizeDiff: changedPrivileges: %v", changedPrivileges) - - for _, k := range changedPrivileges { - old, new := d.GetChange(k) - - log.Printf("[DEBUG] CustomizeDiff: old: %v new: %v", old, new) - - oldType := reflect.TypeOf(old) - newType := reflect.TypeOf(new) - if oldType.String() != "string" || newType.String() != "string" { - log.Printf("[DEBUG] CustomizeDiff: oldType: %s newType: %s", oldType, newType) - continue - } - - oldString := old.(string) - newString := new.(string) - - log.Printf("[DEBUG] CustomizeDiff: oldString: %s newString: %s", oldString, newString) - - // if the only difference is spaces, remove the change - if oldString != newString && strings.ReplaceAll(newString, " ", "_") == oldString { - log.Printf("[DEBUG] CustomizeDiff: removing change: %s", k) - // this doesn't work since these privileges are not computed... - err := d.Clear(k) - if err != nil { - log.Printf("[DEBUG] CustomizeDiff: error clearing change: %s with error: %v", k, err) - return fmt.Errorf("privilege %s only differs from current privilege %s by spaces, please update to match current", newString, oldString) - } - } - } - } - return nil - }, } } diff --git a/catalog/resource_grant_test.go b/catalog/resource_grant_test.go index 53e1e6bb43..838f327106 100644 --- a/catalog/resource_grant_test.go +++ b/catalog/resource_grant_test.go @@ -857,23 +857,3 @@ func TestResourceGrantModelGrantCreate(t *testing.T) { `, }.ApplyNoError(t) } - -func TestResourceGrantUpdateSpacesNoUpdate(t *testing.T) { - qa.ResourceFixture{ - Resource: ResourceGrant(), - ID: "table/foo.bar.baz/me", - InstanceState: map[string]string{ - "table": "foo.bar.baz", - "principal": "me", - "privileges.#": "1", - "privileges.3182106578": "ALL_PRIVILEGES", // this indexer seems like a huge hack - }, - Update: true, - HCL: ` - table = "foo.bar.baz" - - principal = "me" - privileges = ["ALL PRIVILEGES"] - `, - }.ExpectError(t, "privilege ALL PRIVILEGES only differs from current privilege ALL_PRIVILEGES by spaces, please update to match current") -} diff --git a/catalog/resource_grants.go b/catalog/resource_grants.go index 45897dcef2..331bd7c3c6 100644 --- a/catalog/resource_grants.go +++ b/catalog/resource_grants.go @@ -3,6 +3,7 @@ package catalog import ( "context" "fmt" + "log" "sort" "strings" "time" @@ -135,6 +136,11 @@ 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 { + + log.Printf("[INFO] ResourceGrants schema: %v", s) + + common.CustomizeSchemaPath(s, "grant", "privileges").SetCustomSuppressDiff(permissions.PrivilegesSuppressWhitespaceChange) + alof := []string{} for field := range permissions.Mappings { s[field] = &schema.Schema{ @@ -144,6 +150,7 @@ func ResourceGrants() common.Resource { } alof = append(alof, field) } + for field := range permissions.Mappings { s[field].AtLeastOneOf = alof } From 18dfb099579650796850b010e0d755073b85d1de Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Fri, 29 Mar 2024 15:47:08 -0500 Subject: [PATCH 11/16] Clean up --- catalog/permissions/permissions.go | 1 - catalog/resource_grants.go | 3 --- 2 files changed, 4 deletions(-) diff --git a/catalog/permissions/permissions.go b/catalog/permissions/permissions.go index 6e74f67580..83e6f3c184 100644 --- a/catalog/permissions/permissions.go +++ b/catalog/permissions/permissions.go @@ -149,6 +149,5 @@ func SliceWithoutString(in []string, without string) (out []string) { // If a privilege/permission only differs with underscores replacing spaces // then we can suppress the change func PrivilegesSuppressWhitespaceChange(k, old, new string, _ *schema.ResourceData) bool { - log.Printf("[INFO] privilegesSuppressWhitespaceChange: k: %s old: %s new: %s", k, old, new) return strings.ReplaceAll(old, " ", "_") == strings.ReplaceAll(new, " ", "_") } diff --git a/catalog/resource_grants.go b/catalog/resource_grants.go index 331bd7c3c6..4f1fd65be6 100644 --- a/catalog/resource_grants.go +++ b/catalog/resource_grants.go @@ -3,7 +3,6 @@ package catalog import ( "context" "fmt" - "log" "sort" "strings" "time" @@ -137,8 +136,6 @@ func ResourceGrants() common.Resource { s := common.StructToSchema(PermissionsList{}, func(s map[string]*schema.Schema) map[string]*schema.Schema { - log.Printf("[INFO] ResourceGrants schema: %v", s) - common.CustomizeSchemaPath(s, "grant", "privileges").SetCustomSuppressDiff(permissions.PrivilegesSuppressWhitespaceChange) alof := []string{} From ef02c083dcd8b5e3541721a382085cc7bbf64aaf Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Mon, 1 Apr 2024 14:47:09 -0500 Subject: [PATCH 12/16] Switch to debug log when suppressing --- catalog/permissions/permissions.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/catalog/permissions/permissions.go b/catalog/permissions/permissions.go index 83e6f3c184..2359c5c25c 100644 --- a/catalog/permissions/permissions.go +++ b/catalog/permissions/permissions.go @@ -149,5 +149,9 @@ func SliceWithoutString(in []string, without string) (out []string) { // If a privilege/permission only differs with underscores replacing spaces // then we can suppress the change func PrivilegesSuppressWhitespaceChange(k, old, new string, _ *schema.ResourceData) bool { - return strings.ReplaceAll(old, " ", "_") == strings.ReplaceAll(new, " ", "_") + if (new == strings.ReplaceAll(old, " ", "_")) || (old == strings.ReplaceAll(new, " ", "_")) { + log.Printf("[DEBUG] Ignoring configuration drift from %s to %s", old, new) + return true + } + return false } From 0c523a0f70042c78ff1a61cd1fa91c8aa55f7861 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Wed, 3 Apr 2024 17:06:09 -0500 Subject: [PATCH 13/16] Add unit test and speculative acceptance test --- catalog/resource_grant.go | 5 ----- catalog/resource_grant_test.go | 8 +++++++ internal/acceptance/grant_test.go | 33 ++++++++++++++++++++++++++++ internal/acceptance/grants_test.go | 35 ++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 5 deletions(-) diff --git a/catalog/resource_grant.go b/catalog/resource_grant.go index be6017fd13..a87633c35e 100644 --- a/catalog/resource_grant.go +++ b/catalog/resource_grant.go @@ -130,11 +130,6 @@ func parseSecurableId(d *schema.ResourceData) (string, string, string, error) { return split[0], split[1], split[2], nil } -// func privilegesSuppressWhitespaceChange(k, old, new string, d *schema.ResourceData) bool { -// log.Printf("[INFO] privilegesSuppressWhitespaceChange: k: %s old: %s new: %s", k, old, new) -// return strings.ReplaceAll(old, " ", "_") == strings.ReplaceAll(new, " ", "_") -// } - func ResourceGrant() common.Resource { s := common.StructToSchema(permissions.UnityCatalogPrivilegeAssignment{}, func(m map[string]*schema.Schema) map[string]*schema.Schema { diff --git a/catalog/resource_grant_test.go b/catalog/resource_grant_test.go index 838f327106..e510acbfd7 100644 --- a/catalog/resource_grant_test.go +++ b/catalog/resource_grant_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/databricks/databricks-sdk-go/service/catalog" + "github.com/databricks/terraform-provider-databricks/catalog/permissions" "github.com/databricks/terraform-provider-databricks/qa" "github.com/stretchr/testify/assert" ) @@ -857,3 +858,10 @@ func TestResourceGrantModelGrantCreate(t *testing.T) { `, }.ApplyNoError(t) } + +func TestPrivilegesSuppressWhitespaceChange(t *testing.T) { + assert.True(t, permissions.PrivilegesSuppressWhitespaceChange("", "a", "a", nil)) + assert.True(t, permissions.PrivilegesSuppressWhitespaceChange("", "a_b", "a b", nil)) + assert.True(t, permissions.PrivilegesSuppressWhitespaceChange("", "a b", "a_b", nil)) + assert.False(t, permissions.PrivilegesSuppressWhitespaceChange("", "a", "b", nil)) +} diff --git a/internal/acceptance/grant_test.go b/internal/acceptance/grant_test.go index 19898e477d..f738e7f4ea 100644 --- a/internal/acceptance/grant_test.go +++ b/internal/acceptance/grant_test.go @@ -5,6 +5,9 @@ import ( "regexp" "strings" "testing" + + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/stretchr/testify/assert" ) var grantTemplate = ` @@ -134,3 +137,33 @@ func TestUcAccGrantForIdChange(t *testing.T) { ExpectError: regexp.MustCompile(`cannot create grant: Privilege abc is not applicable to this entity`), }) } + +func grantTemplateForUnderscoreChange(permission string) string { + return fmt.Sprintf(` + resource "databricks_storage_credential" "external" { + name = "cred-{var.STICKY_RANDOM}-hyphens" + 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"] + } + `, permission) +} + +func TestUcAccGrantForUnderscoreChange(t *testing.T) { + unityWorkspaceLevel(t, step{ + Template: grantTemplateForUnderscoreChange("ALL_PRIVILEGES"), + }, step{ + Template: grantTemplateForUnderscoreChange("ALL PRIVILEGES"), + Check: func(s *terraform.State) error { + assert.Equal(t, s.Serial, int64(1), "Expected serial to not be updated") + return nil + }, + }) +} diff --git a/internal/acceptance/grants_test.go b/internal/acceptance/grants_test.go index 23beb2fe3b..e78428fdba 100644 --- a/internal/acceptance/grants_test.go +++ b/internal/acceptance/grants_test.go @@ -5,6 +5,9 @@ import ( "regexp" "strings" "testing" + + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/stretchr/testify/assert" ) var grantsTemplate = ` @@ -142,3 +145,35 @@ func TestUcAccGrantsForIdChange(t *testing.T) { ExpectError: regexp.MustCompile(`Error: cannot create grants: Privilege abc is not applicable to this entity`), }) } + +func grantsTemplateForNamePermissionChange(permission string) string { + return fmt.Sprintf(` + resource "databricks_storage_credential" "external" { + name = "cred-{var.STICKY_RANDOM}-underscores" + 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"] + } + } + `, permission) +} + +func TestUcAccGrantsForUnderscoreChange(t *testing.T) { + unityWorkspaceLevel(t, step{ + Template: grantsTemplateForUnderscoreChange("ALL_PRIVILEGES"), + }, step{ + Template: grantsTemplateForUnderscoreChange("ALL PRIVILEGES"), + Check: func(s *terraform.State) error { + assert.Equal(t, s.Serial, int64(1), "Expected serial to not be updated") + return nil + }, + }) +} From 4a39e67067105ef0ee7c2857a2dfa726fdd890cf Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Wed, 3 Apr 2024 17:12:06 -0500 Subject: [PATCH 14/16] Saving is hard --- internal/acceptance/grants_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/acceptance/grants_test.go b/internal/acceptance/grants_test.go index e78428fdba..c1419f3248 100644 --- a/internal/acceptance/grants_test.go +++ b/internal/acceptance/grants_test.go @@ -146,7 +146,7 @@ func TestUcAccGrantsForIdChange(t *testing.T) { }) } -func grantsTemplateForNamePermissionChange(permission string) string { +func grantsTemplateForUnderscoreChange(permission string) string { return fmt.Sprintf(` resource "databricks_storage_credential" "external" { name = "cred-{var.STICKY_RANDOM}-underscores" From 8b079a19363eae8f98fb3b4ad5d5cbfd315cfa81 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Fri, 5 Apr 2024 10:37:50 -0500 Subject: [PATCH 15/16] Simplify integration tests from feedback --- internal/acceptance/grant_test.go | 9 --------- internal/acceptance/grants_test.go | 9 --------- 2 files changed, 18 deletions(-) diff --git a/internal/acceptance/grant_test.go b/internal/acceptance/grant_test.go index f738e7f4ea..c3fcc1760b 100644 --- a/internal/acceptance/grant_test.go +++ b/internal/acceptance/grant_test.go @@ -5,9 +5,6 @@ import ( "regexp" "strings" "testing" - - "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" - "github.com/stretchr/testify/assert" ) var grantTemplate = ` @@ -158,12 +155,6 @@ func grantTemplateForUnderscoreChange(permission string) string { func TestUcAccGrantForUnderscoreChange(t *testing.T) { unityWorkspaceLevel(t, step{ - Template: grantTemplateForUnderscoreChange("ALL_PRIVILEGES"), - }, step{ Template: grantTemplateForUnderscoreChange("ALL PRIVILEGES"), - Check: func(s *terraform.State) error { - assert.Equal(t, s.Serial, int64(1), "Expected serial to not be updated") - return nil - }, }) } diff --git a/internal/acceptance/grants_test.go b/internal/acceptance/grants_test.go index c1419f3248..86fa27413a 100644 --- a/internal/acceptance/grants_test.go +++ b/internal/acceptance/grants_test.go @@ -5,9 +5,6 @@ import ( "regexp" "strings" "testing" - - "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" - "github.com/stretchr/testify/assert" ) var grantsTemplate = ` @@ -168,12 +165,6 @@ func grantsTemplateForUnderscoreChange(permission string) string { func TestUcAccGrantsForUnderscoreChange(t *testing.T) { unityWorkspaceLevel(t, step{ - Template: grantsTemplateForUnderscoreChange("ALL_PRIVILEGES"), - }, step{ Template: grantsTemplateForUnderscoreChange("ALL PRIVILEGES"), - Check: func(s *terraform.State) error { - assert.Equal(t, s.Serial, int64(1), "Expected serial to not be updated") - return nil - }, }) } From 71bd3ae958aefa8d20ecd771d2cf9086ed690bb6 Mon Sep 17 00:00:00 2001 From: Sven Grosen Date: Thu, 25 Apr 2024 10:48:41 -0500 Subject: [PATCH 16/16] Roll back unrelated changes --- .devcontainer/devcontainer.json | 2 +- common/resource.go | 3 +-- qa/testing.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index 0e9c134d8c..1d955cb3f6 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -8,7 +8,7 @@ // Update the VARIANT arg to pick a version of Go: 1, 1.18, 1.17 // Append -bullseye or -buster to pin to an OS version. // Use -bullseye variants on local arm64/Apple Silicon. - "VARIANT": "1.21-bullseye", + "VARIANT": "1.19-bullseye", // Options "NODE_VERSION": "none" } diff --git a/common/resource.go b/common/resource.go index fc491c84c5..a0717f461c 100644 --- a/common/resource.go +++ b/common/resource.go @@ -63,7 +63,7 @@ func (r Resource) saferCustomizeDiff() schema.CustomizeDiffFunc { return func(ctx context.Context, rd *schema.ResourceDiff, _ any) (err error) { defer func() { // this is deliberate decision to convert a panic into error, - // so that any unforeseen bug would be visible to end-user + // so that any unforeseen bug would we visible to end-user // as an error and not a provider crash, which is way less // of pleasant experience. if panic := recover(); panic != nil { @@ -443,7 +443,6 @@ var NoAuth string = "default auth: cannot configure default credentials, " + func AddAccountIdField(s map[string]*schema.Schema) map[string]*schema.Schema { s["account_id"] = &schema.Schema{ Type: schema.TypeString, - Computed: true, Optional: true, Deprecated: "Configuring `account_id` at the resource-level is deprecated; please specify it in the `provider {}` configuration block instead", } diff --git a/qa/testing.go b/qa/testing.go index 4c16b25084..ee7adf60cf 100644 --- a/qa/testing.go +++ b/qa/testing.go @@ -124,7 +124,7 @@ type ResourceFixture struct { New bool } -// wrapper type for calling resource methods +// wrapper type for calling resource methords type resourceCRUD func(context.Context, *schema.ResourceData, any) diag.Diagnostics func (cb resourceCRUD) before(before func(d *schema.ResourceData)) resourceCRUD {