Deterministic diff for databricks_permissions (#2059)

This PR removes SDK client as the argument to CustomizeDiff callback on the level of `common.Resource` framework. All customize diff validation must be hermetic and take only `schema.ResourceDiff` as the input. Any validation, that may require calls to the platform must be done in scope of Create or Update callbacks.

Fix #2052

Author: nfx

catalog/resource_grants.go

@@ -281,7 +281,7 @@ func ResourceGrants() *schema.Resource {
 		})
 	return common.Resource{
 		Schema: s,
-		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff, c any) error {
+		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

catalog/resource_table.go

@@ -85,7 +85,7 @@ func ResourceTable() *schema.Resource {
 		"view_definition", "comment", "properties"})
 	return common.Resource{
 		Schema: tableSchema,
-		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff, c any) error {
+		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
 			if d.Get("table_type") != "EXTERNAL" {
 				return nil
 			}

common/resource.go

@@ -20,7 +20,7 @@ type Resource struct {
 	Read           func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error
 	Update         func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error
 	Delete         func(ctx context.Context, d *schema.ResourceData, c *DatabricksClient) error
-	CustomizeDiff  func(ctx context.Context, d *schema.ResourceDiff, c any) error
+	CustomizeDiff  func(ctx context.Context, d *schema.ResourceDiff) error
 	StateUpgraders []schema.StateUpgrader
 	Schema         map[string]*schema.Schema
 	SchemaVersion  int
@@ -54,6 +54,32 @@ func recoverable(cb func(
 	}
 }
 
+func (r Resource) saferCustomizeDiff() schema.CustomizeDiffFunc {
+	if r.CustomizeDiff == nil {
+		return nil
+	}
+	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
+			// as an error and not a provider crash, which is way less
+			// of pleasant experience.
+			if panic := recover(); panic != nil {
+				err = nicerError(ctx, fmt.Errorf("panic: %v", panic),
+					"customize diff for")
+			}
+		}()
+		// we don't propagate instance of SDK client to the diff function, because
+		// authentication is not deterministic at this stage with the recent Terraform
+		// versions. Diff customization must be limited to hermetic checks only anyway.
+		err = r.CustomizeDiff(ctx, rd)
+		if err != nil {
+			err = nicerError(ctx, err, "customize diff for")
+		}
+		return
+	}
+}
+
 // ToResource converts to Terraform resource definition
 func (r Resource) ToResource() *schema.Resource {
 	var update func(ctx context.Context, d *schema.ResourceData,
@@ -117,7 +143,7 @@ func (r Resource) ToResource() *schema.Resource {
 		Schema:         r.Schema,
 		SchemaVersion:  r.SchemaVersion,
 		StateUpgraders: r.StateUpgraders,
-		CustomizeDiff:  r.CustomizeDiff,
+		CustomizeDiff:  r.saferCustomizeDiff(),
 		CreateContext: func(ctx context.Context, d *schema.ResourceData,
 			m any) diag.Diagnostics {
 			c := m.(*DatabricksClient)

common/resource_test.go

@@ -155,3 +155,26 @@ func TestRecoverableFromPanic(t *testing.T) {
 	assert.True(t, diags.HasError())
 	assert.Equal(t, "cannot read sample: panic: what to do?...", diags[0].Summary)
 }
+
+func TestCustomizeDiffRobustness(t *testing.T) {
+	r := Resource{
+		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
+			return fmt.Errorf("nope")
+		},
+	}.ToResource()
+
+	ctx := context.Background()
+	ctx = context.WithValue(ctx, ResourceName, "sample")
+
+	err := r.CustomizeDiff(ctx, nil, nil)
+	assert.EqualError(t, err, "cannot customize diff for sample: nope")
+
+	r = Resource{
+		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
+			panic("oops")
+		},
+	}.ToResource()
+
+	err = r.CustomizeDiff(ctx, nil, nil)
+	assert.EqualError(t, err, "cannot customize diff for sample: panic: oops")
+}

jobs/resource_job.go

@@ -595,7 +595,7 @@ func ResourceJob() *schema.Resource {
 			Create: schema.DefaultTimeout(clusters.DefaultProvisionTimeout),
 			Update: schema.DefaultTimeout(clusters.DefaultProvisionTimeout),
 		},
-		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff, m any) error {
+		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
 			var js JobSettings
 			common.DiffToStructPointer(d, jobSchema, &js)
 			alwaysRunning := d.Get("always_running").(bool)

mws/resource_mws_workspaces.go

@@ -570,7 +570,7 @@ func ResourceMwsWorkspaces() *schema.Resource {
 			}
 			return NewWorkspacesAPI(ctx, c).Delete(accountID, workspaceID)
 		},
-		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff, m any) error {
+		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
 			old, new := d.GetChange("private_access_settings_id")
 			if old != "" && new == "" {
 				return fmt.Errorf("cannot remove private access setting from workspace")

permissions/resource_permissions.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"log"
 	"path"
 	"strconv"
 	"strings"
@@ -381,21 +380,7 @@ func ResourcePermissions() *schema.Resource {
 	})
 	return common.Resource{
 		Schema: s,
-		CustomizeDiff: func(ctx context.Context, diff *schema.ResourceDiff, c any) error {
-			client := c.(*common.DatabricksClient)
-			log.Printf("[DEBUG] permissions id=%s, config_present=%v", diff.Id(), client.Config != nil)
-			if client.Config.Host == "" || client.DatabricksClient.Config.Host == "" {
-				log.Printf("[WARN] cannot validate permission levels, because host is not known yet")
-				return nil
-			}
-			w, err := client.WorkspaceClient()
-			if err != nil {
-				return err
-			}
-			me, err := w.CurrentUser.Me(ctx)
-			if err != nil {
-				return fmt.Errorf("customize diff: me: %w", err)
-			}
+		CustomizeDiff: func(ctx context.Context, diff *schema.ResourceDiff) error {
 			// Plan time validation for object permission levels
 			for _, mapping := range permissionsResourceIDFields() {
 				if _, ok := diff.GetOk(mapping.field); !ok {
@@ -406,10 +391,8 @@ func ResourcePermissions() *schema.Resource {
 					m := access_control.(map[string]any)
 					permission_level := m["permission_level"].(string)
 					if !stringInSlice(permission_level, mapping.allowedPermissionLevels) {
-						return fmt.Errorf(`permission_level %s is not supported with %s objects`, permission_level, mapping.field)
-					}
-					if m["user_name"].(string) == me.UserName {
-						return fmt.Errorf("it is not possible to decrease administrative permissions for the current user: %s", me.UserName)
+						return fmt.Errorf(`permission_level %s is not supported with %s objects`,
+							permission_level, mapping.field)
 					}
 				}
 			}
@@ -443,12 +426,25 @@ func ResourcePermissions() *schema.Resource {
 		Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
 			var entity PermissionsEntity
 			common.DataToStructPointer(d, s, &entity)
+			w, err := c.WorkspaceClient()
+			if err != nil {
+				return err
+			}
+			me, err := w.CurrentUser.Me(ctx)
+			if err != nil {
+				return err
+			}
+			// this logic was moved from CustomizeDiff because of undeterministic auth behavior
+			// in the corner-case scenarios.
+			// see https://github.com/databricks/terraform-provider-databricks/issues/2052
+			for _, v := range entity.AccessControlList {
+				if v.UserName == me.UserName {
+					format := "it is not possible to decrease administrative permissions for the current user: %s"
+					return fmt.Errorf(format, me.UserName)
+				}
+			}
 			for _, mapping := range permissionsResourceIDFields() {
 				if v, ok := d.GetOk(mapping.field); ok {
-					w, err := c.WorkspaceClient()
-					if err != nil {
-						return err
-					}
 					id, err := mapping.idRetriever(ctx, w, v.(string))
 					if err != nil {
 						return err

permissions/resource_permissions_test.go

@@ -409,51 +409,32 @@ func TestResourcePermissionsRead_some_error(t *testing.T) {
 	assert.Error(t, err)
 }
 
-func TestResourcePermissionsCustomizeDiff_ErrorOnScimMe(t *testing.T) {
+func TestResourcePermissionsCustomizeDiff_ErrorOnCreate(t *testing.T) {
+	qa.ResourceFixture{
+		Resource: ResourcePermissions(),
+		Create:   true,
+		HCL: `
+		cluster_id = "abc"
+		access_control {
+			permission_level = "WHATEVER"
+		}`,
+	}.ExpectError(t, "permission_level WHATEVER is not supported with cluster_id objects")
+}
+
+func TestResourcePermissionsCustomizeDiff_ErrorOnPermissionsDecreate(t *testing.T) {
 	qa.ResourceFixture{
 		Fixtures: []qa.HTTPFixture{
-			{
-				Method:   http.MethodGet,
-				Resource: "/api/2.0/permissions/clusters/abc",
-				Response: ObjectACL{
-					ObjectID:   "/clusters/abc",
-					ObjectType: "clusters",
-					AccessControlList: []AccessControl{
-						{
-							UserName: TestingUser,
-							AllPermissions: []Permission{
-								{
-									PermissionLevel: "CAN_READ",
-									Inherited:       false,
-								},
-							},
-						},
-						{
-							UserName: TestingAdminUser,
-							AllPermissions: []Permission{
-								{
-									PermissionLevel: "CAN_MANAGE",
-									Inherited:       false,
-								},
-							},
-						},
-					},
-				},
-			},
-			{
-				Method:   http.MethodGet,
-				Resource: "/api/2.0/preview/scim/v2/Me",
-				Response: apierr.APIErrorBody{
-					ErrorCode: "INVALID_REQUEST",
-					Message:   "Internal error happened",
-				},
-				Status: 400,
-			},
+			me,
 		},
 		Resource: ResourcePermissions(),
-		Read:     true,
-		ID:       "/clusters/abc",
-	}.ExpectError(t, "customize diff: me: Internal error happened")
+		Create:   true,
+		HCL: `
+		cluster_id = "abc"
+		access_control {
+			permission_level = "CAN_ATTACH_TO"
+			user_name = "admin"
+		}`,
+	}.ExpectError(t, "it is not possible to decrease administrative permissions for the current user: admin")
 }
 
 func TestResourcePermissionsRead_ErrorOnScimMe(t *testing.T) {