From 43217a710ba9ed04c211f556275e10574b09846f Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 16 Sep 2026 14:48:55 +0200 Subject: [PATCH 1/4] Fix perpetual permissions drift for a service principal declared under user_name A service principal listed in bundle permissions under `user_name` (its application ID) is stored and returned by the Permissions API as `service_principal_name`, so the direct engine's plan reported a no-op "update" of the ACL on every run that never converged. Permission entries are already matched by principal value, so treat the same value under `user_name` vs `service_principal_name` as one principal and drop the field-only change; a genuine level change still surfaces. The relaxation lives in ResourcePermissions.OverrideChangeDesc, which now also receives the desired state so the removal side of the swap can be recognized. The testserver models the backend's user_name(UUID) -> service_principal_name readback, and a new acceptance test asserts the plan converges and that a real level change is still reported. Co-authored-by: Isaac --- .../sp_declared_as_user/databricks.yml.tmpl | 18 +++++ .../sp_declared_as_user/out.test.toml | 3 + .../dashboards/sp_declared_as_user/output.txt | 39 +++++++++++ .../dashboards/sp_declared_as_user/script | 23 +++++++ .../dashboards/sp_declared_as_user/test.toml | 6 ++ bundle/direct/bundle_plan.go | 2 +- bundle/direct/dresources/adapter.go | 7 +- bundle/direct/dresources/all_test.go | 2 +- bundle/direct/dresources/app.go | 2 +- bundle/direct/dresources/cluster.go | 2 +- bundle/direct/dresources/dashboard.go | 2 +- bundle/direct/dresources/experiment.go | 2 +- bundle/direct/dresources/genie_space.go | 2 +- bundle/direct/dresources/genie_space_test.go | 6 +- bundle/direct/dresources/job_run.go | 2 +- bundle/direct/dresources/job_run_test.go | 2 +- bundle/direct/dresources/permissions.go | 69 +++++++++++++++++++ bundle/direct/dresources/pipeline.go | 2 +- bundle/direct/dresources/pipeline_test.go | 8 +-- bundle/direct/dresources/secret.go | 2 +- .../direct/dresources/vector_search_index.go | 2 +- libs/calladapt/calladapt.go | 5 +- libs/calladapt/calladapt_test.go | 2 +- libs/testserver/permissions.go | 28 ++++++-- 24 files changed, 209 insertions(+), 29 deletions(-) create mode 100644 acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/databricks.yml.tmpl create mode 100644 acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/out.test.toml create mode 100644 acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/output.txt create mode 100644 acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/script create mode 100644 acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/test.toml diff --git a/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/databricks.yml.tmpl b/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/databricks.yml.tmpl new file mode 100644 index 00000000000..20915edf952 --- /dev/null +++ b/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 diff --git a/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/out.test.toml b/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/out.test.toml new file mode 100644 index 00000000000..59b56a2037c --- /dev/null +++ b/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/output.txt b/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/output.txt new file mode 100644 index 00000000000..260fe47741d --- /dev/null +++ b/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" + } + } +} diff --git a/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/script b/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/script new file mode 100644 index 00000000000..fec69dab14e --- /dev/null +++ b/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}' diff --git a/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/test.toml b/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/test.toml new file mode 100644 index 00000000000..7c25f8ee2f3 --- /dev/null +++ b/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"] diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index baa5aa8d1eb..a4e0ceb6408 100644 --- a/bundle/direct/bundle_plan.go +++ b/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) } diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 6210a952112..9f887c66cf7 100644 --- a/bundle/direct/dresources/adapter.go +++ b/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 } diff --git a/bundle/direct/dresources/all_test.go b/bundle/direct/dresources/all_test.go index 28170a4c02d..a757b38eff8 100644 --- a/bundle/direct/dresources/all_test.go +++ b/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) } diff --git a/bundle/direct/dresources/app.go b/bundle/direct/dresources/app.go index 066669f8b87..06b4773ded5 100644 --- a/bundle/direct/dresources/app.go +++ b/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, _ any, 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": diff --git a/bundle/direct/dresources/cluster.go b/bundle/direct/dresources/cluster.go index 0605d50b86c..8afdbacff5d 100644 --- a/bundle/direct/dresources/cluster.go +++ b/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, _ any, remoteState *ClusterRemote) error { path := p.Prefix(1).String() // Remaining overrides only apply to Update actions. diff --git a/bundle/direct/dresources/dashboard.go b/bundle/direct/dresources/dashboard.go index cdfd57c5623..d8d2996c4ae 100644 --- a/bundle/direct/dresources/dashboard.go +++ b/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, _ any, _ *DashboardState) error { switch path.String() { case "etag": // change.New is always nil for etag because it's not present in the config diff --git a/bundle/direct/dresources/experiment.go b/bundle/direct/dresources/experiment.go index e4f2e8ebbd7..d1b2c825ec2 100644 --- a/bundle/direct/dresources/experiment.go +++ b/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, _ any, _ *ml.Experiment) error { if change.Action == deployplan.Skip { return nil } diff --git a/bundle/direct/dresources/genie_space.go b/bundle/direct/dresources/genie_space.go index 21d2c632409..a3b9680d036 100644 --- a/bundle/direct/dresources/genie_space.go +++ b/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, _ any, _ *resources.GenieSpaceConfig) error { switch path.String() { case "etag": // change.New is always nil for etag because it's not present in the diff --git a/bundle/direct/dresources/genie_space_test.go b/bundle/direct/dresources/genie_space_test.go index f77289083d3..23bedd98256 100644 --- a/bundle/direct/dresources/genie_space_test.go +++ b/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) }) } diff --git a/bundle/direct/dresources/job_run.go b/bundle/direct/dresources/job_run.go index 36031ec612e..e5b948403fe 100644 --- a/bundle/direct/dresources/job_run.go +++ b/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, _ any, remote *JobRunRemote) error { switch path.String() { case "lifecycle": // Dropped when the trigger is removed (New nil) and when both sides are diff --git a/bundle/direct/dresources/job_run_test.go b/bundle/direct/dresources/job_run_test.go index 5da8feb39e0..8799e1cbaf2 100644 --- a/bundle/direct/dresources/job_run_test.go +++ b/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) }) diff --git a/bundle/direct/dresources/permissions.go b/bundle/direct/dresources/permissions.go index c2aebb4d0ac..c086d56d1cd 100644 --- a/bundle/direct/dresources/permissions.go +++ b/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,73 @@ 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 + } + + // 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) { diff --git a/bundle/direct/dresources/pipeline.go b/bundle/direct/dresources/pipeline.go index aa9aae7ee33..75611faf267 100644 --- a/bundle/direct/dresources/pipeline.go +++ b/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, _ any, _ *PipelineRemote) error { if path.String() == "cascade_on_destroy" && !structdiff.IsEqual(change.Old, change.New) { change.Action = deployplan.Update } diff --git a/bundle/direct/dresources/pipeline_test.go b/bundle/direct/dresources/pipeline_test.go index 3629255a600..c768c17493a 100644 --- a/bundle/direct/dresources/pipeline_test.go +++ b/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) }) } diff --git a/bundle/direct/dresources/secret.go b/bundle/direct/dresources/secret.go index 122ea0440e9..a4b7c126299 100644 --- a/bundle/direct/dresources/secret.go +++ b/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, _ any, _ *catalog.Secret) error { if path.String() != "value" { return nil } diff --git a/bundle/direct/dresources/vector_search_index.go b/bundle/direct/dresources/vector_search_index.go index 39e29cb7b8e..91c5585147e 100644 --- a/bundle/direct/dresources/vector_search_index.go +++ b/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, _ any, remote *VectorSearchIndexRemote) error { if path.String() != "endpoint_uuid" { return nil } diff --git a/libs/calladapt/calladapt.go b/libs/calladapt/calladapt.go index b6fc3ce046e..812d96326ba 100644 --- a/libs/calladapt/calladapt.go +++ b/libs/calladapt/calladapt.go @@ -45,8 +45,9 @@ func (c *BoundCaller) call(args ...any) ([]reflect.Value, error) { for i, a := range args { it := c.InTypes[i] if a == nil { - // Allow untyped nil for pointer types, converting to typed nil - if it.Kind() == reflect.Pointer { + // Allow untyped nil for nilable types (pointer, interface), converting to a + // typed/nil-interface zero value. + if it.Kind() == reflect.Pointer || it.Kind() == reflect.Interface { in[i+1] = reflect.Zero(it) continue } diff --git a/libs/calladapt/calladapt_test.go b/libs/calladapt/calladapt_test.go index 9153533304e..4ed09503bdc 100644 --- a/libs/calladapt/calladapt_test.go +++ b/libs/calladapt/calladapt_test.go @@ -339,7 +339,7 @@ func TestCall(t *testing.T) { ifaceType: reflect.TypeFor[interface{ PMethodAcceptAny(v any) error }](), method: "PMethodAcceptAny", args: []any{nil}, - errMsg: "PMethodAcceptAny: arg 0 type mismatch: want interface {}, got nil", + expect: []any{}, }, { name: "nil slice param allowed", diff --git a/libs/testserver/permissions.go b/libs/testserver/permissions.go index a3584c1d49a..ab13d167721 100644 --- a/libs/testserver/permissions.go +++ b/libs/testserver/permissions.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/databricks/databricks-sdk-go/service/iam" + "github.com/google/uuid" ) // source: https://github.com/databricks/terraform-provider-databricks/blob/main/permissions/permission_definitions.go @@ -39,6 +40,13 @@ var requestObjectTypeToObjectType = map[string]string{ "alertsv2": "alertv2", } +// isServicePrincipalID reports whether name is a service principal application ID +// (a UUID). The Permissions API treats a UUID-valued user_name as a service principal. +func isServicePrincipalID(name string) bool { + _, err := uuid.Parse(name) + return err == nil +} + // aclPrincipalKey returns a unique key identifying the principal in an ACL entry. func aclPrincipalKey(acl iam.AccessControlResponse) string { switch { @@ -269,15 +277,27 @@ func (s *FakeWorkspace) SetPermissions(req Request) any { // Convert AccessControlRequest to AccessControlResponse and replace the ACL. existingPermissions.AccessControlList = nil for _, acl := range updateRequest.AccessControlList { - display := acl.UserName + userName := acl.UserName + servicePrincipalName := acl.ServicePrincipalName + + // The real Permissions API resolves a user_name that is actually a service + // principal's application ID (a UUID) to a service principal, and returns it + // as service_principal_name on GET. Model that here so a bundle that declares + // a service principal under user_name converges the same way it does on cloud. + if userName != "" && isServicePrincipalID(userName) { + servicePrincipalName = userName + userName = "" + } + + display := userName if display == "" { - display = acl.ServicePrincipalName + display = servicePrincipalName } response := iam.AccessControlResponse{ - UserName: acl.UserName, + UserName: userName, GroupName: acl.GroupName, - ServicePrincipalName: acl.ServicePrincipalName, + ServicePrincipalName: servicePrincipalName, DisplayName: display, } From 668b9a9728aababe17a801ccc37639c2df59bebe Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 16 Sep 2026 14:50:05 +0200 Subject: [PATCH 2/4] Add changelog fragment Co-authored-by: Isaac --- .nextchanges/bundles/6710.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 .nextchanges/bundles/6710.md diff --git a/.nextchanges/bundles/6710.md b/.nextchanges/bundles/6710.md new file mode 100644 index 00000000000..6668d3b0f48 --- /dev/null +++ b/.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)) From f31c20122bd00640e4ba0e626a92e5ba6ad09c17 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 16 Sep 2026 15:09:24 +0200 Subject: [PATCH 3/4] Type OverrideChangeDesc newState per resource; drop calladapt any-nil change Use each resource's concrete state type for the new OverrideChangeDesc newState parameter instead of `any`. A concrete pointer parameter already accepts an untyped nil through calladapt's existing pointer path, so the interface-nil handling added earlier is no longer needed and is reverted. Co-authored-by: Isaac --- bundle/direct/dresources/app.go | 2 +- bundle/direct/dresources/cluster.go | 2 +- bundle/direct/dresources/dashboard.go | 2 +- bundle/direct/dresources/experiment.go | 2 +- bundle/direct/dresources/genie_space.go | 2 +- bundle/direct/dresources/job_run.go | 2 +- bundle/direct/dresources/permissions.go | 4 ++++ bundle/direct/dresources/pipeline.go | 2 +- bundle/direct/dresources/secret.go | 2 +- bundle/direct/dresources/vector_search_index.go | 2 +- libs/calladapt/calladapt.go | 5 ++--- libs/calladapt/calladapt_test.go | 2 +- 12 files changed, 16 insertions(+), 13 deletions(-) diff --git a/bundle/direct/dresources/app.go b/bundle/direct/dresources/app.go index 06b4773ded5..1904c772e97 100644 --- a/bundle/direct/dresources/app.go +++ b/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, _ any, 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": diff --git a/bundle/direct/dresources/cluster.go b/bundle/direct/dresources/cluster.go index 8afdbacff5d..13530810ed2 100644 --- a/bundle/direct/dresources/cluster.go +++ b/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, _ any, 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. diff --git a/bundle/direct/dresources/dashboard.go b/bundle/direct/dresources/dashboard.go index d8d2996c4ae..e1fe8513dff 100644 --- a/bundle/direct/dresources/dashboard.go +++ b/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, _ any, _ *DashboardState) error { +func (r *ResourceDashboard) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *DashboardState, _ *DashboardState) error { switch path.String() { case "etag": // change.New is always nil for etag because it's not present in the config diff --git a/bundle/direct/dresources/experiment.go b/bundle/direct/dresources/experiment.go index d1b2c825ec2..ef46ce3d8da 100644 --- a/bundle/direct/dresources/experiment.go +++ b/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, _ any, _ *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 } diff --git a/bundle/direct/dresources/genie_space.go b/bundle/direct/dresources/genie_space.go index a3b9680d036..9b26fc6f582 100644 --- a/bundle/direct/dresources/genie_space.go +++ b/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, _ any, _ *resources.GenieSpaceConfig) error { +func (r *ResourceGenieSpace) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *resources.GenieSpaceConfig, _ *resources.GenieSpaceConfig) error { switch path.String() { case "etag": // change.New is always nil for etag because it's not present in the diff --git a/bundle/direct/dresources/job_run.go b/bundle/direct/dresources/job_run.go index e5b948403fe..86eec3fad6a 100644 --- a/bundle/direct/dresources/job_run.go +++ b/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, _ any, 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 diff --git a/bundle/direct/dresources/permissions.go b/bundle/direct/dresources/permissions.go index c086d56d1cd..127c7d49d36 100644 --- a/bundle/direct/dresources/permissions.go +++ b/bundle/direct/dresources/permissions.go @@ -219,6 +219,10 @@ func (*ResourcePermissions) OverrideChangeDesc(_ context.Context, path *structpa 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 diff --git a/bundle/direct/dresources/pipeline.go b/bundle/direct/dresources/pipeline.go index 75611faf267..7d3c8ae4eb6 100644 --- a/bundle/direct/dresources/pipeline.go +++ b/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, _ any, _ *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 } diff --git a/bundle/direct/dresources/secret.go b/bundle/direct/dresources/secret.go index a4b7c126299..e5b3dad1161 100644 --- a/bundle/direct/dresources/secret.go +++ b/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, _ any, _ *catalog.Secret) error { +func (*ResourceSecret) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, ch *ChangeDesc, _ *catalog.Secret, _ *catalog.Secret) error { if path.String() != "value" { return nil } diff --git a/bundle/direct/dresources/vector_search_index.go b/bundle/direct/dresources/vector_search_index.go index 91c5585147e..54eb65a1b5f 100644 --- a/bundle/direct/dresources/vector_search_index.go +++ b/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, _ any, remote *VectorSearchIndexRemote) error { +func (*ResourceVectorSearchIndex) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, change *ChangeDesc, _ *VectorSearchIndexState, remote *VectorSearchIndexRemote) error { if path.String() != "endpoint_uuid" { return nil } diff --git a/libs/calladapt/calladapt.go b/libs/calladapt/calladapt.go index 812d96326ba..b6fc3ce046e 100644 --- a/libs/calladapt/calladapt.go +++ b/libs/calladapt/calladapt.go @@ -45,9 +45,8 @@ func (c *BoundCaller) call(args ...any) ([]reflect.Value, error) { for i, a := range args { it := c.InTypes[i] if a == nil { - // Allow untyped nil for nilable types (pointer, interface), converting to a - // typed/nil-interface zero value. - if it.Kind() == reflect.Pointer || it.Kind() == reflect.Interface { + // Allow untyped nil for pointer types, converting to typed nil + if it.Kind() == reflect.Pointer { in[i+1] = reflect.Zero(it) continue } diff --git a/libs/calladapt/calladapt_test.go b/libs/calladapt/calladapt_test.go index 4ed09503bdc..9153533304e 100644 --- a/libs/calladapt/calladapt_test.go +++ b/libs/calladapt/calladapt_test.go @@ -339,7 +339,7 @@ func TestCall(t *testing.T) { ifaceType: reflect.TypeFor[interface{ PMethodAcceptAny(v any) error }](), method: "PMethodAcceptAny", args: []any{nil}, - expect: []any{}, + errMsg: "PMethodAcceptAny: arg 0 type mismatch: want interface {}, got nil", }, { name: "nil slice param allowed", From 3cd1c4fc7972b470e6afe5ad282ad9a38624bcb4 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 16 Sep 2026 15:43:01 +0200 Subject: [PATCH 4/4] Apply gofumpt to collapsed OverrideChangeDesc params Co-authored-by: Isaac --- bundle/direct/dresources/dashboard.go | 2 +- bundle/direct/dresources/genie_space.go | 2 +- bundle/direct/dresources/secret.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bundle/direct/dresources/dashboard.go b/bundle/direct/dresources/dashboard.go index e1fe8513dff..1edc4fe5bfb 100644 --- a/bundle/direct/dresources/dashboard.go +++ b/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, _ *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 diff --git a/bundle/direct/dresources/genie_space.go b/bundle/direct/dresources/genie_space.go index 9b26fc6f582..cd139a748e0 100644 --- a/bundle/direct/dresources/genie_space.go +++ b/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, _ *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 diff --git a/bundle/direct/dresources/secret.go b/bundle/direct/dresources/secret.go index e5b3dad1161..10677207d6f 100644 --- a/bundle/direct/dresources/secret.go +++ b/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, _ *catalog.Secret) error { +func (*ResourceSecret) OverrideChangeDesc(_ context.Context, path *structpath.PathNode, ch *ChangeDesc, _, _ *catalog.Secret) error { if path.String() != "value" { return nil }