Fix perpetual permissions drift for a service principal declared under user_name

.nextchanges/bundles/6710.md

@@ -0,0 +1 @@
+* direct: Fix a spurious `permissions` update reported on every plan and deploy when a service principal is declared under `user_name` and the Permissions API returns it as `service_principal_name`. ([#6710](https://github.com/databricks/cli/pull/6710))

acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/databricks.yml.tmpl

@@ -0,0 +1,18 @@
+bundle:
+  name: dashboard-sp-perm-$UNIQUE_NAME
+
+resources:
+  dashboards:
+    foo:
+      display_name: test-dashboard-$UNIQUE_NAME
+      warehouse_id: $TEST_DEFAULT_WAREHOUSE_ID
+      serialized_dashboard: '{"pages":[{"name":"page1","displayName":"Page 1"}]}'
+      permissions:
+        # A service principal declared under user_name using its application-ID UUID.
+        # The Permissions API resolves the UUID to a service principal and returns it
+        # as service_principal_name on GET, so desired (user_name) and remote
+        # (service_principal_name) name the same principal under different fields. The
+        # plan matches the entry by value and treats that field difference as no
+        # change, so the deploy converges instead of reporting a perpetual update.
+        - level: CAN_MANAGE
+          user_name: aaaaaaaa-bbbb-4ccc-dddd-eeeeeeeeeeee

acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/output.txt

@@ -0,0 +1,39 @@
+
+=== Deploy the bundle
+>>> [CLI] bundle deploy
+Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/dashboard-sp-perm-[UNIQUE_NAME]/default/files...
+Created dashboards.foo
+Created dashboards.foo.permissions
+Files: 4 uploaded, 0 deleted
+Resources: 2 created, 0 changed, 0 deleted, 0 unchanged
+
+=== Re-plan after deploy (expected: no changes)
+>>> [CLI] bundle plan
+Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
+
+=== Permissions node of the JSON plan{
+  "action": "skip",
+  "changes": null
+}
+
+=== Lower the permission level: real drift is still detected
+>>> [CLI] bundle plan
+update dashboards.foo.permissions
+
+Plan: 0 to add, 1 to change, 0 to delete, 1 unchanged
+
+=== Permissions node of the JSON plan{
+  "action": "update",
+  "changes": {
+    "[service_principal_name='[UUID]'].level": {
+      "action": "update",
+      "new": "CAN_READ",
+      "remote": "CAN_MANAGE"
+    },
+    "[user_name='[UUID]'].level": {
+      "action": "update",
+      "old": "CAN_MANAGE",
+      "new": "CAN_READ"
+    }
+  }
+}

acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/script

@@ -0,0 +1,23 @@
+envsubst < databricks.yml.tmpl > databricks.yml
+
+title "Deploy the bundle"
+trace $CLI bundle deploy
+replace_ids.py
+
+# The service principal is declared under user_name but the API returns it as
+# service_principal_name. The entry is matched by value and the field difference is
+# relaxed, so a re-plan reports no changes rather than a perpetual no-op "update".
+title "Re-plan after deploy (expected: no changes)"
+trace $CLI bundle plan
+
+title "Permissions node of the JSON plan"
+$CLI bundle plan -o json | jq '.plan["resources.dashboards.foo.permissions"] | {action, changes}'
+
+# A genuine change to the same principal must still be detected: lowering the level
+# leaves the principal-field difference relaxed but reports the level update.
+title "Lower the permission level: real drift is still detected"
+update_file.py databricks.yml "CAN_MANAGE" "CAN_READ"
+trace $CLI bundle plan
+
+title "Permissions node of the JSON plan"
+$CLI bundle plan -o json | jq '.plan["resources.dashboards.foo.permissions"] | {action, changes}'

acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/test.toml

@@ -0,0 +1,6 @@
+# Reproduction of a false-positive permissions "update" after migrating to the
+# direct engine. Direct-only: the customer hit this on the direct engine and the
+# JSON plan format is direct-specific.
+EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"]
+
+Ignore = [".databricks", "databricks.yml"]

bundle/direct/bundle_plan.go

@@ -514,7 +514,7 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change
 			savedAction := ch.Action
 			savedReason := ch.Reason
 
-			err = adapter.OverrideChangeDesc(ctx, path, ch, remoteState)
+			err = adapter.OverrideChangeDesc(ctx, path, ch, newState, remoteState)
 			if err != nil {
 				return fmt.Errorf("internal error: failed to classify change: %w", err)
 			}

bundle/direct/dresources/adapter.go

@@ -70,7 +70,8 @@ type IResource interface {
 	DoDelete(ctx context.Context, id string, state any) error
 
 	// [Optional] OverrideChangeDesc can implement custom logic to update a given ChangeDesc; it is run last after built-in classifiers and field triggers.
-	OverrideChangeDesc(ctx context.Context, path *structpath.PathNode, changedesc *ChangeDesc, remoteState any) error
+	// newState is the desired (config) state and remoteState is the value read from the backend.
+	OverrideChangeDesc(ctx context.Context, path *structpath.PathNode, changedesc *ChangeDesc, newState, remoteState any) error
 
 	// DoCreate creates a new resource from the newState. Returns id of the resource and optionally remote state.
 	// If remote state is available as part of the operation, return it; otherwise return nil.
@@ -689,8 +690,8 @@ func (a *Adapter) HasOverrideChangeDesc() bool {
 }
 
 // OverrideChangeDesc allows custom logic to override change classification.
-func (a *Adapter) OverrideChangeDesc(ctx context.Context, path *structpath.PathNode, change *ChangeDesc, remoteState any) error {
-	_, err := a.overrideChangeDesc.Call(ctx, path, change, remoteState)
+func (a *Adapter) OverrideChangeDesc(ctx context.Context, path *structpath.PathNode, change *ChangeDesc, newState, remoteState any) error {
+	_, err := a.overrideChangeDesc.Call(ctx, path, change, newState, remoteState)
 	return err
 }
 

bundle/direct/dresources/all_test.go

@@ -1226,7 +1226,7 @@ func testCRUD(t *testing.T, group string, adapter *Adapter, client *databricks.W
 	require.NoError(t, err)
 
 	if adapter.HasOverrideChangeDesc() {
-		err = adapter.OverrideChangeDesc(ctx, p, &deployplan.ChangeDesc{}, nil)
+		err = adapter.OverrideChangeDesc(ctx, p, &deployplan.ChangeDesc{}, nil, nil)
 		require.NoError(t, err)
 	}
 

bundle/direct/dresources/app.go

@@ -258,7 +258,7 @@ func hasAppChanges(entry *PlanEntry) bool {
 // git_source) while the app has no active deployment. DoRead reads them only from the
 // active deployment, so before the first deploy (or once a stop clears it) the remote
 // side is empty and the diff is spurious; it applies on the next start (manageLifecycle).
-func (*ResourceApp) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, remote *AppRemote) error {
+func (*ResourceApp) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *AppState, remote *AppRemote) error {
 	// Prefix(1) so a nested diff (e.g. config.command) matches its top-level field.
 	switch path.Prefix(1).String() {
 	case "source_code_path", "config", "git_source":

bundle/direct/dresources/cluster.go

@@ -285,7 +285,7 @@ func (r *ResourceCluster) DoDelete(ctx context.Context, id string, _ *ClusterSta
 	return r.client.Clusters.PermanentDeleteByClusterId(ctx, id)
 }
 
-func (r *ResourceCluster) OverrideChangeDesc(ctx context.Context, p *structpath.PathNode, change *ChangeDesc, remoteState *ClusterRemote) error {
+func (r *ResourceCluster) OverrideChangeDesc(ctx context.Context, p *structpath.PathNode, change *ChangeDesc, _ *ClusterState, remoteState *ClusterRemote) error {
 	path := p.Prefix(1).String()
 
 	// Remaining overrides only apply to Update actions.

bundle/direct/dresources/dashboard.go

@@ -398,7 +398,7 @@ func (r *ResourceDashboard) DoDelete(ctx context.Context, id string, _ *Dashboar
 	})
 }
 
-func (r *ResourceDashboard) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *DashboardState) error {
+func (r *ResourceDashboard) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _, _ *DashboardState) error {
 	switch path.String() {
 	case "etag":
 		// change.New is always nil for etag because it's not present in the config

bundle/direct/dresources/experiment.go

@@ -83,7 +83,7 @@ func (r *ResourceExperiment) DoDelete(ctx context.Context, id string, _ *ml.Crea
 //
 // This matches the Terraform provider's experimentNameSuppressDiff behavior.
 // https://github.com/databricks/terraform-provider-databricks/blob/8945a7b2328659b1fc976d04e32457305860131f/mlflow/resource_mlflow_experiment.go#L13
-func (*ResourceExperiment) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *ml.Experiment) error {
+func (*ResourceExperiment) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *ml.CreateExperiment, _ *ml.Experiment) error {
 	if change.Action == deployplan.Skip {
 		return nil
 	}

bundle/direct/dresources/genie_space.go

@@ -214,7 +214,7 @@ func (r *ResourceGenieSpace) DoUpdate(ctx context.Context, id string, config *re
 // OverrideChangeDesc handles the etag field. The user never sets it directly;
 // we compare the stored etag against the remote one and Skip if they match.
 // This mirrors ResourceDashboard.OverrideChangeDesc.
-func (r *ResourceGenieSpace) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *resources.GenieSpaceConfig) error {
+func (r *ResourceGenieSpace) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _, _ *resources.GenieSpaceConfig) error {
 	switch path.String() {
 	case "etag":
 		// change.New is always nil for etag because it's not present in the

bundle/direct/dresources/genie_space_test.go

@@ -182,20 +182,20 @@ func TestGenieSpaceOverrideChangeDescEtag(t *testing.T) {
 
 	t.Run("Skip when stored matches remote", func(t *testing.T) {
 		change := &ChangeDesc{Old: "etag-7", Remote: "etag-7"}
-		require.NoError(t, r.OverrideChangeDesc(t.Context(), etagPath, change, nil))
+		require.NoError(t, r.OverrideChangeDesc(t.Context(), etagPath, change, nil, nil))
 		assert.Equal(t, deployplan.Skip, change.Action)
 	})
 
 	t.Run("Update when stored differs from remote", func(t *testing.T) {
 		change := &ChangeDesc{Old: "etag-7", Remote: "etag-8"}
-		require.NoError(t, r.OverrideChangeDesc(t.Context(), etagPath, change, nil))
+		require.NoError(t, r.OverrideChangeDesc(t.Context(), etagPath, change, nil, nil))
 		assert.Equal(t, deployplan.Update, change.Action)
 	})
 
 	t.Run("Other paths are untouched", func(t *testing.T) {
 		titlePath := structpath.MustParsePath("title")
 		change := &ChangeDesc{Action: deployplan.Update, Old: "a", Remote: "b"}
-		require.NoError(t, r.OverrideChangeDesc(t.Context(), titlePath, change, nil))
+		require.NoError(t, r.OverrideChangeDesc(t.Context(), titlePath, change, nil, nil))
 		assert.Equal(t, deployplan.Update, change.Action)
 	})
 }

bundle/direct/dresources/job_run.go

@@ -370,7 +370,7 @@ func reportRunLine(ctx context.Context, runID int64, msg string) {
 // Removing a trigger drops the change and leaves the last fingerprint in state,
 // so re-adding the trigger only re-fires when the watched files changed
 // meanwhile. All other trigger changes recreate the run.
-func (*ResourceJobRun) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, remote *JobRunRemote) error {
+func (*ResourceJobRun) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *JobRunState, remote *JobRunRemote) error {
 	switch path.String() {
 	case "lifecycle":
 		// Dropped when the trigger is removed (New nil) and when both sides are

bundle/direct/dresources/job_run_test.go

@@ -500,7 +500,7 @@ func TestJobRunOverrideChangeDescTriggerRemoved(t *testing.T) {
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			change := &ChangeDesc{Action: deployplan.Recreate, Old: tt.old, New: tt.new}
-			require.NoError(t, r.OverrideChangeDesc(t.Context(), structpath.MustParsePath(tt.path), change, nil))
+			require.NoError(t, r.OverrideChangeDesc(t.Context(), structpath.MustParsePath(tt.path), change, nil, nil))
 			assert.Equal(t, tt.action, change.Action)
 			assert.Equal(t, tt.reason, change.Reason)
 		})

bundle/direct/dresources/permissions.go

@@ -6,6 +6,8 @@ import (
 	"reflect"
 	"strings"
 
+	"github.com/databricks/cli/bundle/deployplan"
+	"github.com/databricks/cli/libs/structs/structpath"
 	"github.com/databricks/cli/libs/structs/structvar"
 	"github.com/databricks/databricks-sdk-go"
 	"github.com/databricks/databricks-sdk-go/service/iam"
@@ -167,6 +169,77 @@ func (*ResourcePermissions) KeyedSlices() map[string]any {
 	}
 }
 
+// hasPrincipal reports whether any entry names the given principal value under the
+// given field ("user_name" or "service_principal_name"). Nil-safe: missing state
+// (e.g. a resource not yet created) holds no principals.
+func (s *PermissionsState) hasPrincipal(field, value string) bool {
+	if s == nil {
+		return false
+	}
+	for _, p := range s.EmbeddedSlice {
+		switch field {
+		case "user_name":
+			if p.UserName == value {
+				return true
+			}
+		case "service_principal_name":
+			if p.ServicePrincipalName == value {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// OverrideChangeDesc suppresses false-positive drift when a principal is declared
+// under one field (e.g. user_name) but the Permissions API stores and returns it
+// under the other (service_principal_name) — which the backend does when a user_name
+// holds a service principal's application ID. The entry is matched by value (see
+// permissionKey), so the only diff is the field name; we drop the user_name /
+// service_principal_name leaf change whenever the same value is present under the
+// counterpart field on the other side. A level change on the same principal surfaces
+// via its own ".level" leaf and is left untouched.
+//
+// Matching purely on value means a user and a service principal sharing the exact
+// same name string would be treated as one principal; that collision does not occur
+// in practice (user names are emails, service principal names are application IDs).
+func (*ResourcePermissions) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, ch *ChangeDesc, newState, remoteState *PermissionsState) error {
+	field, ok := path.StringKey()
+	if !ok {
+		return nil
+	}
+
+	var counterpart string
+	switch field {
+	case "user_name":
+		counterpart = "service_principal_name"
+	case "service_principal_name":
+		counterpart = "user_name"
+	default:
+		return nil
+	}
+
+	// Match on value only, not level: level is a separate leaf that surfaces on its
+	// own. Gating these drops on a matching level would resurrect the field-swap diff
+	// whenever the level also changed.
+
+	// Config adds this field while remote holds the same value under the counterpart.
+	if newStr, ok := ch.New.(string); ok && newStr != "" && remoteState.hasPrincipal(counterpart, newStr) {
+		ch.Reason = deployplan.ReasonDrop
+		return nil
+	}
+
+	// Remote has this field (config does not) while config declares the same value
+	// under the counterpart field.
+	if ch.New == nil {
+		if remoteStr, ok := ch.Remote.(string); ok && remoteStr != "" && newState.hasPrincipal(counterpart, remoteStr) {
+			ch.Reason = deployplan.ReasonDrop
+		}
+	}
+
+	return nil
+}
+
 // parsePermissionsID extracts the object type and ID from a permissions ID string.
 // Handles both 3-part IDs ("/jobs/123") and 4-part IDs ("/sql/warehouses/uuid").
 func parsePermissionsID(id string) (extractedType, extractedID string, err error) {

bundle/direct/dresources/pipeline.go

@@ -233,7 +233,7 @@ func (r *ResourcePipeline) DoDelete(ctx context.Context, id string, state *Pipel
 // old=false, new=nil, remote=nil.
 // The problem is that classifier skips the change if remote and new are "empty". There is a bug
 // where "false" value for a boolean field is treated as empty. Hence, we set this override.
-func (*ResourcePipeline) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *PipelineRemote) error {
+func (*ResourcePipeline) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *PipelineState, _ *PipelineRemote) error {
 	if path.String() == "cascade_on_destroy" && !structdiff.IsEqual(change.Old, change.New) {
 		change.Action = deployplan.Update
 	}

bundle/direct/dresources/pipeline_test.go

@@ -30,26 +30,26 @@ func TestPipelineOverrideChangeDescCascadeOnDestroy(t *testing.T) {
 
 	t.Run("unset forces state-only update", func(t *testing.T) {
 		change := &ChangeDesc{Action: deployplan.Skip, Old: ptr(false), New: nil}
-		require.NoError(t, r.OverrideChangeDesc(t.Context(), cascadePath, change, nil))
+		require.NoError(t, r.OverrideChangeDesc(t.Context(), cascadePath, change, nil, nil))
 		assert.Equal(t, deployplan.Update, change.Action)
 	})
 
 	t.Run("changed value forces state-only update", func(t *testing.T) {
 		change := &ChangeDesc{Action: deployplan.Skip, Old: ptr(true), New: ptr(false)}
-		require.NoError(t, r.OverrideChangeDesc(t.Context(), cascadePath, change, nil))
+		require.NoError(t, r.OverrideChangeDesc(t.Context(), cascadePath, change, nil, nil))
 		assert.Equal(t, deployplan.Update, change.Action)
 	})
 
 	t.Run("unchanged value is left as skip", func(t *testing.T) {
 		change := &ChangeDesc{Action: deployplan.Skip, Old: ptr(false), New: ptr(false)}
-		require.NoError(t, r.OverrideChangeDesc(t.Context(), cascadePath, change, nil))
+		require.NoError(t, r.OverrideChangeDesc(t.Context(), cascadePath, change, nil, nil))
 		assert.Equal(t, deployplan.Skip, change.Action)
 	})
 
 	t.Run("other paths are untouched", func(t *testing.T) {
 		namePath := structpath.MustParsePath("name")
 		change := &ChangeDesc{Action: deployplan.Skip, Old: "a", New: nil}
-		require.NoError(t, r.OverrideChangeDesc(t.Context(), namePath, change, nil))
+		require.NoError(t, r.OverrideChangeDesc(t.Context(), namePath, change, nil, nil))
 		assert.Equal(t, deployplan.Skip, change.Action)
 	})
 }

bundle/direct/dresources/secret.go

@@ -125,7 +125,7 @@ func (r *ResourceSecret) DoDelete(ctx context.Context, id string, _ *catalog.Sec
 // the actual stored value. We compare new vs remote (via effective_value from DoRead)
 // to decide whether the secret actually changed: if they are equal, the user's config
 // already matches what is stored remotely and no update is needed.
-func (*ResourceSecret) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, ch *ChangeDesc, _ *catalog.Secret) error {
+func (*ResourceSecret) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, ch *ChangeDesc, _, _ *catalog.Secret) error {
 	if path.String() != "value" {
 		return nil
 	}

bundle/direct/dresources/vector_search_index.go

@@ -249,7 +249,7 @@ func (r *ResourceVectorSearchIndex) WaitAfterDelete(ctx context.Context, id stri
 // lookupEndpointUuid distinguishes this (404 -> "") from transient errors
 // (propagated through DoRead/DoCreate), so reaching this branch with empty
 // remoteUuid unambiguously means the endpoint is gone.
-func (*ResourceVectorSearchIndex) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, remote *VectorSearchIndexRemote) error {
+func (*ResourceVectorSearchIndex) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *VectorSearchIndexState, remote *VectorSearchIndexRemote) error {
 	if path.String() != "endpoint_uuid" {
 		return nil
 	}