From 3f670f424c97416f73fef0d2d8e423ebbbfd24c7 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 17 Sep 2026 15:02:55 +0200 Subject: [PATCH 1/3] Match keyed slices by value in structdiff, ignoring the key field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keyed slices are matched by key value alone (the key field is only used to render the path). A diff on a matched element's own key field therefore means the two sides carry the same identity under a different field — e.g. a permission declared under user_name that the Permissions API stores and returns as service_principal_name — which is not a real change. Drop those field diffs; non-key fields still diff normally. This fixes a perpetual no-op "update" of dashboard/job/etc permissions when a service principal is declared under user_name, generically for any keyed slice, without per-resource logic. Alternative to the resource-level fix in #6710. The testserver models the backend's user_name(UUID) -> service_principal_name readback so the case reproduces locally. 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 ++ libs/structs/structdiff/diff.go | 34 +++++++++-- libs/structs/structdiff/diff_test.go | 61 +++++++++++++++++++ libs/testserver/permissions.go | 28 +++++++-- 8 files changed, 203 insertions(+), 9 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/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index 5e7a00cac4f..c55ad173fd0 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -70,8 +70,11 @@ type diffContext struct { // // The sliceKeys parameter maps path patterns to functions that extract // key field/value pairs from slice elements. When provided, slices at matching -// paths are compared as maps keyed by (keyField, keyValue) instead of by index. -// Path patterns use dot notation (e.g., "tasks" or "job.tasks"). +// paths are matched by key value instead of by index (the key field is used only +// to render the path). Because the value identifies the element, a diff on the +// element's own key field is not a real change and is dropped: this lets the same +// identity carried under different fields (e.g. user_name vs service_principal_name) +// compare equal. Path patterns use dot notation (e.g., "tasks" or "job.tasks"). // The [*] wildcard matches any slice index in the path. // Note, key wildcard is not supported yet ("a.*.c") // Pass nil if no slice key functions are needed. @@ -366,9 +369,23 @@ func validateKeyFuncElementType(seq reflect.Value, expected reflect.Type) error return nil } +// appendSkippingKeyFields appends pairChanges to changes, dropping any change on an +// element's own key field: a direct child of node whose field name is keyField1 or +// keyField2. See the call site for why such a diff is not a real change. +func appendSkippingKeyFields(changes *[]Change, pairChanges []Change, node *structpath.PathNode, keyField1, keyField2 string) { + for _, ch := range pairChanges { + if ch.Path.Parent() == node { + if field, ok := ch.Path.StringKey(); ok && (field == keyField1 || field == keyField2) { + continue + } + } + *changes = append(*changes, ch) + } +} + // diffSliceByKey compares two slices using the provided key function. -// Elements are matched by their (keyField, keyValue) pairs instead of by index. -// Duplicate keys are allowed and matched in order. +// Elements are matched by their key value instead of by index (keyField is only +// used to render the path). Duplicate keys are allowed and matched in order. func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Value, keyFunc KeyFunc, changes *[]Change) error { caller, err := newKeyFuncCaller(keyFunc) if err != nil { @@ -427,9 +444,16 @@ func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect. minLen := min(len(list1), len(list2)) for i := range minLen { node := structpath.NewKeyValue(path, keyField, keyValue) - if err := diffValues(ctx, node, list1[i].value, list2[i].value, changes); err != nil { + var pairChanges []Change + if err := diffValues(ctx, node, list1[i].value, list2[i].value, &pairChanges); err != nil { return err } + // Elements are matched by key value alone (keyField is only for display), so a + // diff on an element's own key field means the two sides carry the same identity + // under different fields (e.g. a principal returned as service_principal_name but + // declared as user_name). The identity is unchanged, so drop that field diff; + // non-key fields still diff normally. + appendSkippingKeyFields(changes, pairChanges, node, list1[i].keyField, list2[i].keyField) } // Handle extra elements in old (deleted) diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index dd1cf4bb5ad..9d04333d872 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -602,6 +602,67 @@ func TestGetStructDiffEmbedTagWithKeyFunc(t *testing.T) { } } +// principal has two interchangeable identity fields (like a permission's +// user_name / service_principal_name) plus a non-key field. +type principal struct { + UserName string `json:"user_name,omitempty"` + SpName string `json:"service_principal_name,omitempty"` + Level string `json:"level,omitempty"` +} + +type principalContainer struct { + ObjectID string `json:"object_id"` + EmbeddedSlice []principal `json:"items,omitempty"` +} + +// principalKey returns a varying key field depending on which identity field is set. +func principalKey(p principal) (string, string) { + if p.UserName != "" { + return "user_name", p.UserName + } + return "service_principal_name", p.SpName +} + +// TestGetStructDiffKeyFieldSwap covers a KeyFunc whose key field varies per element: +// the same identity value carried under a different field is not a change, but a +// non-key field still diffs. +func TestGetStructDiffKeyFieldSwap(t *testing.T) { + sliceKeys := map[string]KeyFunc{"": principalKey} + + tests := []struct { + name string + a, b principalContainer + want []ResolvedChange + }{ + { + name: "field swap only is not a change", + a: principalContainer{EmbeddedSlice: []principal{{SpName: "X", Level: "CAN_MANAGE"}}}, + b: principalContainer{EmbeddedSlice: []principal{{UserName: "X", Level: "CAN_MANAGE"}}}, + want: nil, + }, + { + name: "field swap with level change reports only the level", + a: principalContainer{EmbeddedSlice: []principal{{SpName: "X", Level: "CAN_MANAGE"}}}, + b: principalContainer{EmbeddedSlice: []principal{{UserName: "X", Level: "CAN_READ"}}}, + want: []ResolvedChange{{Field: "[service_principal_name='X'].level", Old: "CAN_MANAGE", New: "CAN_READ"}}, + }, + { + name: "same field, level change", + a: principalContainer{EmbeddedSlice: []principal{{UserName: "X", Level: "CAN_MANAGE"}}}, + b: principalContainer{EmbeddedSlice: []principal{{UserName: "X", Level: "CAN_READ"}}}, + want: []ResolvedChange{{Field: "[user_name='X'].level", Old: "CAN_MANAGE", New: "CAN_READ"}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := GetStructDiff(tt.a, tt.b, sliceKeys) + assert.NoError(t, err) + assert.Equal(t, tt.want, resolveChanges(got)) + }) + } +} + type Dep struct { TaskKey string `json:"task_key,omitempty"` Outcome string `json:"outcome,omitempty"` 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 03559b46bed88034ffa89ec82b413f6619a045d7 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 17 Sep 2026 15:04:13 +0200 Subject: [PATCH 2/3] Add changelog fragment Co-authored-by: Isaac --- .nextchanges/bundles/6732.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 .nextchanges/bundles/6732.md diff --git a/.nextchanges/bundles/6732.md b/.nextchanges/bundles/6732.md new file mode 100644 index 00000000000..58f3048ef6c --- /dev/null +++ b/.nextchanges/bundles/6732.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`. ([#6732](https://github.com/databricks/cli/pull/6732)) From fcb6e085c0dec4d7a1b5d976cc5c4e54f9c010f9 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 17 Sep 2026 15:27:47 +0200 Subject: [PATCH 3/3] Merge local and remote diffs of a keyed element by value prepareChanges indexed changes by the full path string, so a keyed element whose key field differs between the saved state and the backend (e.g. a principal saved under user_name but read back as service_principal_name) produced two half-populated change entries for one field. Index by the key-field-agnostic path (structpath.KeyValueAgnosticString) so the local and remote views merge into one entry; the displayed path stays the local one. Also document the value-match / key-field-drop semantics on structdiff.KeyFunc and cross-reference it from IResource.KeyedSlices. Co-authored-by: Isaac --- .../dashboards/sp_declared_as_user/output.txt | 8 +--- bundle/direct/bundle_plan.go | 44 ++++++++++++------- bundle/direct/dresources/adapter.go | 3 ++ libs/structs/structdiff/diff.go | 10 ++++- libs/structs/structpath/path.go | 16 ++++++- 5 files changed, 56 insertions(+), 25 deletions(-) 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 index 260fe47741d..3a95de38086 100644 --- a/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/output.txt +++ b/acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/output.txt @@ -25,15 +25,11 @@ 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" + "new": "CAN_READ", + "remote": "CAN_MANAGE" } } } diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 44b7061b6d6..d623a9cebe2 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -424,6 +424,14 @@ func getMaxAction(m map[string]*deployplan.ChangeDesc) deployplan.ActionType { func prepareChanges(ctx context.Context, adapter *dresources.Adapter, localDiff, remoteDiff []structdiff.Change, oldState, remoteState any) (deployplan.Changes, error) { m := make(deployplan.Changes) + // A keyed-slice element is matched by value, so the local diff (against saved + // state) and the remote diff (against the backend) can label the same element's + // key field differently — e.g. a principal saved under user_name but read back as + // service_principal_name. Index by the key-field-agnostic path so the two views + // merge into one change instead of appearing twice; the display key stays the + // first (local) path. + displayByKey := make(map[string]string) + for _, ch := range localDiff { e := deployplan.ChangeDesc{ Old: ch.Old, @@ -433,31 +441,35 @@ func prepareChanges(ctx context.Context, adapter *dresources.Adapter, localDiff, // We cannot assume e.Remote is the same as config: if the whole struct is missing, there might be diff entry for parent e.Remote, _ = structaccess.Get(remoteState, ch.Path) } - m[ch.Path.String()] = &e + display := ch.Path.String() + m[display] = &e + displayByKey[ch.Path.KeyValueAgnosticString()] = display } for _, ch := range remoteDiff { - entry := m[ch.Path.String()] - if entry == nil { - // we have difference for remoteState but not difference for localState - // from remoteDiff we can find out remote value (ch.Old) and new config value (ch.New) but we don't know oldState value - oldStateVal, err := structaccess.Get(oldState, ch.Path) - _, isNotFound := errors.AsType[*structaccess.NotFoundError](err) - if err != nil && !isNotFound { - log.Debugf(ctx, "Constructing diff: accessing %q on %T: %s", ch.Path, oldState, err) - } - m[ch.Path.String()] = &deployplan.ChangeDesc{ - Old: oldStateVal, - New: ch.New, - Remote: ch.Old, - } - } else { + if display, ok := displayByKey[ch.Path.KeyValueAgnosticString()]; ok { + entry := m[display] entry.Remote = ch.Old if !structdiff.IsEqual(entry.New, ch.New) { // this is not fatal (may result in unexpected drift or undetected change but not incorrect deploy), but good to log this log.Warnf(ctx, "unexpected local and remote diffs (%T, %T); entry=%v ch=%v", entry.New, ch.New, entry, ch) } + continue + } + // we have difference for remoteState but not difference for localState + // from remoteDiff we can find out remote value (ch.Old) and new config value (ch.New) but we don't know oldState value + oldStateVal, err := structaccess.Get(oldState, ch.Path) + _, isNotFound := errors.AsType[*structaccess.NotFoundError](err) + if err != nil && !isNotFound { + log.Debugf(ctx, "Constructing diff: accessing %q on %T: %s", ch.Path, oldState, err) + } + display := ch.Path.String() + m[display] = &deployplan.ChangeDesc{ + Old: oldStateVal, + New: ch.New, + Remote: ch.Old, } + displayByKey[ch.Path.KeyValueAgnosticString()] = display } return m, nil diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 6210a952112..d87fd66d764 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -109,6 +109,9 @@ type IResource interface { WaitAfterDelete(ctx context.Context, id string) error // [Optional] KeyedSlices returns a map from path patterns to KeyFunc for comparing slices by key instead of by index. + // Elements are matched by the KeyFunc's value, not its field, and a matched element's own key field is not + // diffed, so the same identity under a different field (e.g. user_name vs service_principal_name) compares + // equal. See structdiff.KeyFunc for the full semantics. // Example: func (*ResourcePermissions) KeyedSlices(state *PermissionsState) map[string]any KeyedSlices() map[string]any diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index c55ad173fd0..443ab3e9d03 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -23,8 +23,14 @@ type Change struct { // - func(T) (string, string) - typed function for specific element type T // - func(any) (string, string) - generic function accepting any element // -// The function returns (keyField, keyValue). The keyField is typically a field name -// like "task_key", and keyValue is the value that uniquely identifies the element. +// The function returns (keyField, keyValue). keyValue is the identity: elements are +// matched across the two sides by keyValue alone. keyField is only used to render +// the path (e.g. "task_key" -> [task_key='...']) and may vary between elements — a +// permission, for instance, keys on "user_name", "service_principal_name", or +// "group_name" depending on which is set. Because keyValue is the identity, a matched +// element's own key field is never diffed: the same value carried under a different +// key field (e.g. user_name vs service_principal_name for one principal) compares +// equal rather than reporting a spurious change. type KeyFunc = any // keyFuncCaller wraps a KeyFunc and provides a type-checked Call method. diff --git a/libs/structs/structpath/path.go b/libs/structs/structpath/path.go index 92715dc09d6..5ced3c64b37 100644 --- a/libs/structs/structpath/path.go +++ b/libs/structs/structpath/path.go @@ -181,6 +181,18 @@ func NewKeyValue(prev *PathNode, key, value string) *PathNode { // "resources.jobs.foo.tags['cost-center']": {} // } func (p *PathNode) String() string { + return p.render(false) +} + +// KeyValueAgnosticString renders the path like String, except key-value segments +// [field='value'] are rendered as [='value'] (the key field is omitted). Two paths +// that address the same keyed element under different key fields therefore render +// identically, which lets callers treat them as the same element. +func (p *PathNode) KeyValueAgnosticString() string { + return p.render(true) +} + +func (p *PathNode) render(omitKeyField bool) string { if p == nil { return "" } @@ -206,7 +218,9 @@ func (p *PathNode) String() string { result.WriteString("[*]") } else if node.index == tagKeyValue { result.WriteString("[") - result.WriteString(node.key) + if !omitKeyField { + result.WriteString(node.key) + } result.WriteString("=") result.WriteString(EncodeMapKey(node.value)) result.WriteString("]")