From 48d7c4c5fdd36d0fa8ee9268a43a4eae54ea7bd6 Mon Sep 17 00:00:00 2001 From: Factory Local Trial Date: Mon, 2 Nov 2026 15:31:16 +0000 Subject: [PATCH 1/3] [factory] Fix qualified DABs behavior --- .../apply_bundle_permissions.go | 18 ++++++- .../apply_bundle_permissions_test.go | 53 +++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go b/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go index 15493ae9d15..da9fcc75c22 100644 --- a/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go +++ b/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go @@ -108,6 +108,12 @@ func (m *bundlePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Di return diag.FromErr(err) } + // Target permissions follow root permissions when SelectTarget merges the lists. + targetPermissionsStart := len(b.Config.Permissions) + if b.Target != nil { + targetPermissionsStart -= len(b.Target.Permissions) + } + for _, group := range b.Config.Resources.AllResources() { key := group.Description.PluralName lm, ok := levelsMap[key] @@ -124,6 +130,7 @@ func (m *bundlePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Di permissions, key, lm, + targetPermissionsStart, )...) writePermissions(field, permissions) } @@ -187,9 +194,10 @@ func convertPermissions( resourcePermissions []resources.Permission, resourceName string, lm map[string]string, + targetPermissionsStart int, ) []resources.Permission { var permissions []resources.Permission - for _, p := range bundlePermissions { + for i, p := range bundlePermissions { level, ok := lm[string(p.Level)] // If there is no bundle permission level defined in the map, it means // it's not applicable for the resource, therefore skipping @@ -197,7 +205,13 @@ func convertPermissions( continue } - if notifyForPermissionOverlap(ctx, p, resourcePermissions, resourceName) { + if i >= targetPermissionsStart && resourceName == "jobs" && p.GroupName != "" { + // A job-level group permission takes precedence over the target-level one. + overlap, _ := isPermissionOverlap(p, resourcePermissions, resourceName) + if overlap { + continue + } + } else if notifyForPermissionOverlap(ctx, p, resourcePermissions, resourceName) { continue } diff --git a/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go b/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go index 610832c2b92..037f06332f2 100644 --- a/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go +++ b/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" @@ -223,6 +224,58 @@ func TestWarningOnOverlapPermission(t *testing.T) { require.Contains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.JobPermission{Level: "CAN_VIEW", GroupName: "TestGroup"}) } +func TestTargetGroupPermissionOverridesJobWithoutWarning(t *testing.T) { + for _, tc := range []struct { + name string + rootPermissions []resources.Permission + wantWarnings int + }{ + {name: "target only"}, + { + name: "root and target", + rootPermissions: []resources.Permission{ + {Level: permissions.CAN_VIEW, GroupName: "TestGroup"}, + }, + wantWarnings: 1, + }, + } { + t.Run(tc.name, func(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Permissions: tc.rootPermissions, + Targets: map[string]*config.Target{ + "dev-ci": { + Permissions: []resources.Permission{ + {Level: permissions.CAN_VIEW, GroupName: "TestGroup"}, + }, + Resources: &config.Resources{ + Jobs: map[string]*resources.Job{ + "job_1": { + JobSettings: jobs.JobSettings{Name: "job_1"}, + Permissions: []resources.JobPermission{ + {Level: "CAN_MANAGE_RUN", GroupName: "TestGroup"}, + }, + }, + }, + }, + }, + }, + }, + } + + diags := bundle.ApplySeq(t.Context(), b, mutator.SelectTarget("dev-ci"), ApplyBundlePermissions()) + require.Len(t, diags, tc.wantWarnings) + if tc.wantWarnings > 0 { + require.Equal(t, diag.Warning, diags[0].Severity) + require.Equal(t, "'jobs' already has permissions set for 'TestGroup' group", diags[0].Summary) + } + require.Equal(t, []resources.JobPermission{ + {Level: "CAN_MANAGE_RUN", GroupName: "TestGroup"}, + }, b.Config.Resources.Jobs["job_1"].Permissions) + }) + } +} + func TestAllResourcesExplicitlyDefinedForPermissionsSupport(t *testing.T) { r := config.Resources{} From 0cc8036f44893aa40755105969074d9021292ce8 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 6 Nov 2026 10:37:35 +0000 Subject: [PATCH 2/3] Fix target permission overlap handling across resource types --- .nextchanges/bundles/7291.md | 1 + .../validate/permissions_overlap/output.txt | 24 ++ .../validate/permissions_overlap/script | 2 + .../permissions_target/databricks.yml | 44 ++++ .../validate/permissions_target/out.test.toml | 2 + .../validate/permissions_target/output.txt | 216 ++++++++++++++++++ .../bundle/validate/permissions_target/script | 4 + .../validate/permissions_target/test.toml | 1 + .../apply_bundle_permissions.go | 35 +-- .../apply_bundle_permissions_test.go | 150 ++++++++---- bundle/config/root.go | 30 ++- bundle/config/target.go | 1 + 12 files changed, 438 insertions(+), 72 deletions(-) create mode 100644 .nextchanges/bundles/7291.md create mode 100644 acceptance/bundle/validate/permissions_target/databricks.yml create mode 100644 acceptance/bundle/validate/permissions_target/out.test.toml create mode 100644 acceptance/bundle/validate/permissions_target/output.txt create mode 100644 acceptance/bundle/validate/permissions_target/script create mode 100644 acceptance/bundle/validate/permissions_target/test.toml diff --git a/.nextchanges/bundles/7291.md b/.nextchanges/bundles/7291.md new file mode 100644 index 00000000000..eb270fc26ca --- /dev/null +++ b/.nextchanges/bundles/7291.md @@ -0,0 +1 @@ +* Allow resource permissions to override target-wide permissions without overlap warnings for all supported resources and principal types. ([#7291](https://github.com/databricks/cli/pull/7291)) diff --git a/acceptance/bundle/validate/permissions_overlap/output.txt b/acceptance/bundle/validate/permissions_overlap/output.txt index 43b95ddd440..6ae90c4bcd3 100644 --- a/acceptance/bundle/validate/permissions_overlap/output.txt +++ b/acceptance/bundle/validate/permissions_overlap/output.txt @@ -42,3 +42,27 @@ See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more } ] } + +>>> [CLI] bundle validate --strict +Warning: 'jobs' already has permissions set for 'overlap@example.com' user name + +Recommendation: permissions section should explicitly include the current deployment identity '[USERNAME]' or one of its groups +If it is not included, CAN_MANAGE permissions are only applied if the present identity is used to deploy. + +Consider using a adding a top-level permissions section such as the following: + + permissions: + - user_name: [USERNAME] + level: CAN_MANAGE + +See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration. + in databricks.yml:5:3 + +Name: test-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Found 1 warning and 1 recommendation +Error: 1 warning was found. Warnings are treated as errors in strict mode diff --git a/acceptance/bundle/validate/permissions_overlap/script b/acceptance/bundle/validate/permissions_overlap/script index da6c2e46463..af25ec4901b 100644 --- a/acceptance/bundle/validate/permissions_overlap/script +++ b/acceptance/bundle/validate/permissions_overlap/script @@ -1 +1,3 @@ $CLI bundle validate -o json | jq '.resources.jobs | map_values(.permissions)' + +musterr trace $CLI bundle validate --strict diff --git a/acceptance/bundle/validate/permissions_target/databricks.yml b/acceptance/bundle/validate/permissions_target/databricks.yml new file mode 100644 index 00000000000..77823f055c9 --- /dev/null +++ b/acceptance/bundle/validate/permissions_target/databricks.yml @@ -0,0 +1,44 @@ +bundle: + name: permissions-target + +variables: + group: + default: developers + acl: + type: complex + default: &target_permissions + - {group_name: "${var.group}", level: CAN_VIEW} + - {user_name: reader@example.test, level: CAN_VIEW} + - {service_principal_name: reader-sp, level: CAN_VIEW} + - {group_name: inherited, level: CAN_VIEW,} + +resources: + jobs: + root_job: &job + tasks: + - task_key: main + notebook_task: + notebook_path: /Workspace/notebook + source: WORKSPACE + permissions: + - {group_name: "${var.group}", level: CAN_MANAGE_RUN} + - {user_name: reader@example.test, level: CAN_MANAGE_RUN} + - {service_principal_name: reader-sp, level: CAN_MANAGE_RUN} + pipelines: + root_pipeline: &pipeline + permissions: + - {group_name: "${var.group}", level: CAN_RUN} + - {user_name: reader@example.test, level: CAN_RUN} + - {service_principal_name: reader-sp, level: CAN_RUN,} + +targets: + literal: + permissions: *target_permissions + resources: &target_resources + jobs: + target_job: *job + pipelines: + target_pipeline: *pipeline + complex: + permissions: ${var.acl} + resources: *target_resources diff --git a/acceptance/bundle/validate/permissions_target/out.test.toml b/acceptance/bundle/validate/permissions_target/out.test.toml new file mode 100644 index 00000000000..a927a5fbc06 --- /dev/null +++ b/acceptance/bundle/validate/permissions_target/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/validate/permissions_target/output.txt b/acceptance/bundle/validate/permissions_target/output.txt new file mode 100644 index 00000000000..04bd744753f --- /dev/null +++ b/acceptance/bundle/validate/permissions_target/output.txt @@ -0,0 +1,216 @@ + +>>> [CLI] bundle validate --strict -t literal -o json +Recommendation: permissions section should explicitly include the current deployment identity '[USERNAME]' or one of its groups +If it is not included, CAN_MANAGE permissions are only applied if the present identity is used to deploy. + +Consider using a adding a top-level permissions section such as the following: + + permissions: + - user_name: [USERNAME] + level: CAN_MANAGE + +See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration. + in databricks.yml:9:14 + +{ + "jobs": { + "root_job": [ + { + "group_name": "developers", + "level": "CAN_MANAGE_RUN" + }, + { + "level": "CAN_MANAGE_RUN", + "user_name": "reader@example.test" + }, + { + "level": "CAN_MANAGE_RUN", + "service_principal_name": "reader-sp" + }, + { + "group_name": "inherited", + "level": "CAN_VIEW" + }, + { + "level": "IS_OWNER", + "user_name": "[USERNAME]" + } + ], + "target_job": [ + { + "group_name": "developers", + "level": "CAN_MANAGE_RUN" + }, + { + "level": "CAN_MANAGE_RUN", + "user_name": "reader@example.test" + }, + { + "level": "CAN_MANAGE_RUN", + "service_principal_name": "reader-sp" + }, + { + "group_name": "inherited", + "level": "CAN_VIEW" + }, + { + "level": "IS_OWNER", + "user_name": "[USERNAME]" + } + ] + }, + "pipelines": { + "root_pipeline": [ + { + "group_name": "developers", + "level": "CAN_RUN" + }, + { + "level": "CAN_RUN", + "user_name": "reader@example.test" + }, + { + "level": "CAN_RUN", + "service_principal_name": "reader-sp" + }, + { + "group_name": "inherited", + "level": "CAN_VIEW" + }, + { + "level": "IS_OWNER", + "user_name": "[USERNAME]" + } + ], + "target_pipeline": [ + { + "group_name": "developers", + "level": "CAN_RUN" + }, + { + "level": "CAN_RUN", + "user_name": "reader@example.test" + }, + { + "level": "CAN_RUN", + "service_principal_name": "reader-sp" + }, + { + "group_name": "inherited", + "level": "CAN_VIEW" + }, + { + "level": "IS_OWNER", + "user_name": "[USERNAME]" + } + ] + } +} + +>>> [CLI] bundle validate --strict -t complex -o json +Recommendation: permissions section should explicitly include the current deployment identity '[USERNAME]' or one of its groups +If it is not included, CAN_MANAGE permissions are only applied if the present identity is used to deploy. + +Consider using a adding a top-level permissions section such as the following: + + permissions: + - user_name: [USERNAME] + level: CAN_MANAGE + +See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration. + in databricks.yml:43:18 + +{ + "jobs": { + "root_job": [ + { + "group_name": "developers", + "level": "CAN_MANAGE_RUN" + }, + { + "level": "CAN_MANAGE_RUN", + "user_name": "reader@example.test" + }, + { + "level": "CAN_MANAGE_RUN", + "service_principal_name": "reader-sp" + }, + { + "group_name": "inherited", + "level": "CAN_VIEW" + }, + { + "level": "IS_OWNER", + "user_name": "[USERNAME]" + } + ], + "target_job": [ + { + "group_name": "developers", + "level": "CAN_MANAGE_RUN" + }, + { + "level": "CAN_MANAGE_RUN", + "user_name": "reader@example.test" + }, + { + "level": "CAN_MANAGE_RUN", + "service_principal_name": "reader-sp" + }, + { + "group_name": "inherited", + "level": "CAN_VIEW" + }, + { + "level": "IS_OWNER", + "user_name": "[USERNAME]" + } + ] + }, + "pipelines": { + "root_pipeline": [ + { + "group_name": "developers", + "level": "CAN_RUN" + }, + { + "level": "CAN_RUN", + "user_name": "reader@example.test" + }, + { + "level": "CAN_RUN", + "service_principal_name": "reader-sp" + }, + { + "group_name": "inherited", + "level": "CAN_VIEW" + }, + { + "level": "IS_OWNER", + "user_name": "[USERNAME]" + } + ], + "target_pipeline": [ + { + "group_name": "developers", + "level": "CAN_RUN" + }, + { + "level": "CAN_RUN", + "user_name": "reader@example.test" + }, + { + "level": "CAN_RUN", + "service_principal_name": "reader-sp" + }, + { + "group_name": "inherited", + "level": "CAN_VIEW" + }, + { + "level": "IS_OWNER", + "user_name": "[USERNAME]" + } + ] + } +} diff --git a/acceptance/bundle/validate/permissions_target/script b/acceptance/bundle/validate/permissions_target/script new file mode 100644 index 00000000000..1a034fe035c --- /dev/null +++ b/acceptance/bundle/validate/permissions_target/script @@ -0,0 +1,4 @@ +for target in literal complex; do + trace $CLI bundle validate --strict -t "$target" -o json > config.$target.json + jq '.resources | map_values(map_values(.permissions))' config.$target.json +done diff --git a/acceptance/bundle/validate/permissions_target/test.toml b/acceptance/bundle/validate/permissions_target/test.toml new file mode 100644 index 00000000000..a49a24c2a58 --- /dev/null +++ b/acceptance/bundle/validate/permissions_target/test.toml @@ -0,0 +1 @@ +Ignore = ["config.literal.json", "config.complex.json"] diff --git a/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go b/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go index da9fcc75c22..3a689a7cbdb 100644 --- a/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go +++ b/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go @@ -108,12 +108,6 @@ func (m *bundlePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Di return diag.FromErr(err) } - // Target permissions follow root permissions when SelectTarget merges the lists. - targetPermissionsStart := len(b.Config.Permissions) - if b.Target != nil { - targetPermissionsStart -= len(b.Target.Permissions) - } - for _, group := range b.Config.Resources.AllResources() { key := group.Description.PluralName lm, ok := levelsMap[key] @@ -130,7 +124,7 @@ func (m *bundlePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Di permissions, key, lm, - targetPermissionsStart, + b.Config.TargetPermissionsStart(), )...) writePermissions(field, permissions) } @@ -205,13 +199,14 @@ func convertPermissions( continue } - if i >= targetPermissionsStart && resourceName == "jobs" && p.GroupName != "" { - // A job-level group permission takes precedence over the target-level one. - overlap, _ := isPermissionOverlap(p, resourcePermissions, resourceName) - if overlap { - continue + overlap, diagnostics := isPermissionOverlap(p, resourcePermissions, resourceName) + if i < targetPermissionsStart { + for _, d := range diagnostics { + logdiag.LogDiag(ctx, d) } - } else if notifyForPermissionOverlap(ctx, p, resourcePermissions, resourceName) { + } + if overlap { + // Resource permissions override target-wide defaults without a warning. continue } @@ -254,17 +249,3 @@ func isPermissionOverlap( return len(diagnostics) > 0, diagnostics } - -func notifyForPermissionOverlap( - ctx context.Context, - permission resources.Permission, - resourcePermissions []resources.Permission, - resourceName string, -) bool { - isOverlap, diagnostics := isPermissionOverlap(permission, resourcePermissions, resourceName) - for _, d := range diagnostics { - logdiag.LogDiag(ctx, d) - } - - return isOverlap -} diff --git a/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go b/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go index 037f06332f2..089a2701bc5 100644 --- a/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go +++ b/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go @@ -3,6 +3,7 @@ package resourcemutator import ( "fmt" "slices" + "strings" "testing" "github.com/databricks/cli/bundle/permissions" @@ -224,54 +225,119 @@ func TestWarningOnOverlapPermission(t *testing.T) { require.Contains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.JobPermission{Level: "CAN_VIEW", GroupName: "TestGroup"}) } -func TestTargetGroupPermissionOverridesJobWithoutWarning(t *testing.T) { +func TestTargetPermissionsOverrideWithoutWarning(t *testing.T) { + for resourceType, lm := range levelsMap { + for _, resourceLayer := range []string{"root", "target"} { + for _, withRootPermissions := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/%s/root_permissions=%t", resourceType, resourceLayer, withRootPermissions), func(t *testing.T) { + resourceConfig := fmt.Sprintf(` +resources: + %s: + test: + permissions: + - {group_name: principal, level: %s} + - {user_name: principal, level: %s} + - {service_principal_name: principal, level: %s} +`, resourceType, lm[permissions.CAN_MANAGE], lm[permissions.CAN_MANAGE], lm[permissions.CAN_MANAGE]) + rootConfig := "" + if withRootPermissions { + rootConfig = "permissions:\n - {group_name: principal, level: CAN_VIEW}\n" + } + targetConfig := `permissions: + - {group_name: principal, level: CAN_VIEW} + - {user_name: principal, level: CAN_VIEW} + - {service_principal_name: principal, level: CAN_VIEW} + - {group_name: inherited, level: CAN_VIEW} +` + if resourceLayer == "root" { + rootConfig += resourceConfig + } else { + targetConfig += resourceConfig + } + yaml := rootConfig + "\ntargets:\n test:\n" + " " + strings.ReplaceAll(strings.TrimSpace(targetConfig), "\n", "\n ") + "\n" + root, diags := config.LoadFromBytes("databricks.yml", []byte(yaml)) + require.Empty(t, diags) + b := &bundle.Bundle{Config: *root} + diags = bundle.ApplySeq(t.Context(), b, mutator.SelectTarget("test"), ApplyBundlePermissions()) + if withRootPermissions { + require.Equal(t, diag.Warningf("'%s' already has permissions set for 'principal' group", resourceType), diags) + } else { + require.Empty(t, diags) + } + actual := b.Config.Value().Get("resources").Get(resourceType).Get("test").Get("permissions").AsAny() + assert.Equal(t, []any{ + map[string]any{"group_name": "principal", "level": lm[permissions.CAN_MANAGE]}, + map[string]any{"user_name": "principal", "level": lm[permissions.CAN_MANAGE]}, + map[string]any{"service_principal_name": "principal", "level": lm[permissions.CAN_MANAGE]}, + map[string]any{"group_name": "inherited", "level": lm[permissions.CAN_VIEW]}, + }, actual) + }) + } + } + } +} + +func TestTargetPermissionsVariableResolution(t *testing.T) { for _, tc := range []struct { - name string - rootPermissions []resources.Permission - wantWarnings int + name string + root string + target string + warn bool }{ - {name: "target only"}, - { - name: "root and target", - rootPermissions: []resources.Permission{ - {Level: permissions.CAN_VIEW, GroupName: "TestGroup"}, - }, - wantWarnings: 1, - }, + {name: "target literal", target: `[{group_name: "${var.group}", level: CAN_VIEW}]`}, + {name: "target reference", target: "${var.acl}"}, + {name: "null root", root: "null", target: "${var.acl}"}, + {name: "resource narrows target", target: `[{group_name: developers, level: CAN_MANAGE}]`}, + {name: "target entry reference", target: "[\"${var.acl_entry}\"]"}, + {name: "root reference", root: "${var.acl}", warn: true}, + {name: "root literal", root: `[{group_name: "${var.group}", level: CAN_VIEW}]`, warn: true}, + {name: "both literals", root: `[{group_name: "${var.group}", level: CAN_VIEW}]`, target: `[{group_name: "${var.group}", level: CAN_VIEW}]`, warn: true}, + {name: "reference overrides reference", root: "${var.unused_acl}", target: "${var.acl}"}, + {name: "null target", root: "${var.acl}", target: "null", warn: true}, + {name: "empty target", root: `[{group_name: "${var.group}", level: CAN_VIEW}]`, target: "[]", warn: true}, } { t.Run(tc.name, func(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Permissions: tc.rootPermissions, - Targets: map[string]*config.Target{ - "dev-ci": { - Permissions: []resources.Permission{ - {Level: permissions.CAN_VIEW, GroupName: "TestGroup"}, - }, - Resources: &config.Resources{ - Jobs: map[string]*resources.Job{ - "job_1": { - JobSettings: jobs.JobSettings{Name: "job_1"}, - Permissions: []resources.JobPermission{ - {Level: "CAN_MANAGE_RUN", GroupName: "TestGroup"}, - }, - }, - }, - }, - }, - }, - }, + yaml := ` +variables: + group: + default: developers + acl: + type: complex + default: + - {group_name: "${var.group}", level: CAN_VIEW} + acl_entry: + type: complex + default: {group_name: "${var.group}", level: CAN_VIEW} + unused_acl: + type: complex + default: + - {group_name: overridden, level: CAN_MANAGE} +resources: + jobs: + test: + permissions: + - {group_name: developers, level: CAN_MANAGE_RUN} +` + if tc.root != "" { + yaml += "permissions: " + tc.root + "\n" } - - diags := bundle.ApplySeq(t.Context(), b, mutator.SelectTarget("dev-ci"), ApplyBundlePermissions()) - require.Len(t, diags, tc.wantWarnings) - if tc.wantWarnings > 0 { - require.Equal(t, diag.Warning, diags[0].Severity) - require.Equal(t, "'jobs' already has permissions set for 'TestGroup' group", diags[0].Summary) + yaml += "targets:\n test:\n default: true\n" + if tc.target != "" { + yaml += " permissions: " + tc.target + "\n" + } + root, diags := config.LoadFromBytes("databricks.yml", []byte(yaml)) + require.Empty(t, diags) + b := &bundle.Bundle{Config: *root} + diags = bundle.ApplySeq(t.Context(), b, + mutator.SelectTarget("test"), mutator.SetVariables(), + mutator.ResolveVariableReferencesWithoutResources(), ApplyBundlePermissions(), + ) + if tc.warn { + require.Equal(t, diag.Warningf("'jobs' already has permissions set for 'developers' group"), diags) + } else { + require.Empty(t, diags) } - require.Equal(t, []resources.JobPermission{ - {Level: "CAN_MANAGE_RUN", GroupName: "TestGroup"}, - }, b.Config.Resources.Jobs["job_1"].Permissions) + assert.Equal(t, []resources.JobPermission{{GroupName: "developers", Level: "CAN_MANAGE_RUN"}}, b.Config.Resources.Jobs["test"].Permissions) }) } } diff --git a/bundle/config/root.go b/bundle/config/root.go index a64b742630e..4a3942996d0 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -38,6 +38,9 @@ type Root struct { //nolint:recvcheck // value receivers for read-only accessors value dyn.Value depth int + // Nil when no target permissions were merged. Recorded before variable resolution. + targetPermissionsStart *int + // Contains user defined variables Variables map[string]*variable.Variable `json:"variables,omitempty"` @@ -160,12 +163,13 @@ func (r *Root) initializeDynamicValue() error { } func (r *Root) updateWithDynamicValue(nv dyn.Value) error { - // Hack: restore state; it may be cleared by [ToTyped] if - // the configuration equals nil (happens in tests). + // ToTyped clears all fields, including internal state. depth := r.depth + targetPermissionsStart := r.targetPermissionsStart defer func() { r.depth = depth + r.targetPermissionsStart = targetPermissionsStart }() // Convert normalized configuration tree to typed configuration. @@ -326,6 +330,14 @@ func mergeField(rv, ov dyn.Value, name string) (dyn.Value, error) { return dyn.SetByPath(rv, path, out) } +// TargetPermissionsStart returns the first target permission index, or the list length if none were merged. +func (r Root) TargetPermissionsStart() int { + if r.targetPermissionsStart == nil { + return len(r.Permissions) + } + return *r.targetPermissionsStart +} + func (r *Root) MergeTargetOverrides(name string) error { root := r.value target, err := dyn.GetByPath(root, dyn.NewPath(dyn.Key("targets"), dyn.Key(name))) @@ -339,6 +351,14 @@ func (r *Root) MergeTargetOverrides(name string) error { return err } + var targetPermissionsStart *int + if p := target.Get("permissions"); p.IsValid() && p.Kind() != dyn.KindNil { + // Lists concatenate; a complex-variable reference replaces another reference. + rootPermissions, _ := root.Get("permissions").AsSequence() + start := len(rootPermissions) + targetPermissionsStart = &start + } + // Merge fields that can be merged 1:1. for _, f := range []string{ "bundle", @@ -447,7 +467,11 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Convert normalized configuration tree to typed configuration. - return r.updateWithDynamicValue(root) + if err := r.updateWithDynamicValue(root); err != nil { + return err + } + r.targetPermissionsStart = targetPermissionsStart + return nil } var allowedVariableDefinitions = []([]string){ diff --git a/bundle/config/target.go b/bundle/config/target.go index fae9c940b3f..c9e535cb7a1 100644 --- a/bundle/config/target.go +++ b/bundle/config/target.go @@ -68,6 +68,7 @@ type Target struct { Sync *Sync `json:"sync,omitempty"` + // Permissions supplies target-wide defaults; explicit resource permissions take precedence. Permissions []resources.Permission `json:"permissions,omitempty"` } From 7cff8d114955971891073d8803c8a9f8fb47dbbc Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 6 Nov 2026 14:11:54 +0000 Subject: [PATCH 3/3] Keep target permission provenance in bundle runtime state --- bundle/bundle.go | 64 +++++++++++++++++++ bundle/bundle_test.go | 21 ++++++ .../apply_bundle_permissions.go | 2 +- .../apply_bundle_permissions_test.go | 38 +++++++++++ bundle/config/mutator/select_target.go | 39 +---------- bundle/config/root.go | 30 +-------- 6 files changed, 128 insertions(+), 66 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 6762b38b91e..1e3c3e5e0e2 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -8,9 +8,13 @@ package bundle import ( "context" + "errors" "fmt" + "maps" "os" "path/filepath" + "slices" + "strings" "sync" "time" "uuid" @@ -23,6 +27,7 @@ import ( "github.com/databricks/cli/bundle/statemgmt/resourcestate" "github.com/databricks/cli/libs/auth" "github.com/databricks/cli/libs/cache" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/fileset" "github.com/databricks/cli/libs/locker" "github.com/databricks/cli/libs/log" @@ -156,6 +161,9 @@ type Bundle struct { // Target stores a snapshot of the Root.Bundle.Target configuration when it was selected by SelectTarget. Target *config.Target `json:"target_config,omitempty" bundle:"internal"` + // Nil when no target permissions were merged. Recorded before variable resolution. + targetPermissionsStart *int + // RootPathIsNameTargetScoped reports whether workspace.root_path ends in the bundle // name and target. Recorded before variable resolution, so a path that only happens // to end in those two segments does not count. @@ -228,6 +236,62 @@ type Bundle struct { Metrics Metrics } +// SelectTarget merges the target overrides and retains their permission boundary. +// It must run in a mutator scope to keep typed and dynamic configuration synchronized. +func (b *Bundle) SelectTarget(name string) error { + if b.Config.Targets == nil { + return errors.New("no targets defined") + } + + // Get specified target + target, ok := b.Config.Targets[name] + if !ok { + return fmt.Errorf("%s: no such target. Available targets: %s", name, strings.Join(slices.Collect(maps.Keys(b.Config.Targets)), ", ")) + } + + var targetPermissionsStart *int + if p := b.Config.Value().Get("targets").Get(name).Get("permissions"); p.IsValid() && p.Kind() != dyn.KindNil { + // Lists concatenate; a complex-variable reference replaces another reference. + start := len(b.Config.Permissions) + targetPermissionsStart = &start + } + + // Merge specified target into root configuration structure. + err := b.Config.MergeTargetOverrides(name) + if err != nil { + return fmt.Errorf("failed to perform target override for target=%s: %w", name, err) + } + + // Store specified target in configuration for reference. + b.Target = target + b.targetPermissionsStart = targetPermissionsStart + b.Config.Bundle.Target = name + + // We do this for backward compatibility. + // TODO: remove when Environments section is not supported anymore. + b.Config.Bundle.Environment = b.Config.Bundle.Target + + // Record number of targets in the bundle. This is then used downstream during + // telemetry upload. This value is always >= 1 since if no targets are defined + // in YAML, we create a "default" placeholder target upstream. + b.Metrics.TargetCount = int64(len(b.Config.Targets)) + + // Cleanup the original targets and environments sections since they + // show up in the JSON output of the 'summary' and 'validate' commands. + b.Config.Targets = nil + b.Config.Environments = nil + + return nil +} + +// TargetPermissionsStart returns the first target permission index, or the list length if none were merged. +func (b *Bundle) TargetPermissionsStart() int { + if b.targetPermissionsStart == nil { + return len(b.Config.Permissions) + } + return *b.targetPermissionsStart +} + func Load(ctx context.Context, path string) (*Bundle, error) { b := &Bundle{ BundleRootPath: filepath.Clean(path), diff --git a/bundle/bundle_test.go b/bundle/bundle_test.go index 69c8fa5bdcb..7de2703c1c2 100644 --- a/bundle/bundle_test.go +++ b/bundle/bundle_test.go @@ -237,3 +237,24 @@ func TestBundleSyncMode(t *testing.T) { }) } } + +func TestSelectTargetMergeFailure(t *testing.T) { + root, diags := config.LoadFromBytes("databricks.yml", []byte(` +permissions: + - {group_name: developers, level: CAN_VIEW} +targets: + test: + permissions: ${var.acl} +`)) + require.Empty(t, diags) + b := &Bundle{Config: *root} + require.NoError(t, b.Config.MarkMutatorEntry(t.Context())) + t.Cleanup(func() { require.NoError(t, b.Config.MarkMutatorExit(t.Context())) }) + + err := b.SelectTarget("test") + require.ErrorContains(t, err, "cannot merge sequence with string") + assert.Nil(t, b.Target) + assert.Empty(t, b.Config.Bundle.Target) + assert.Contains(t, b.Config.Targets, "test") + assert.Equal(t, []resources.Permission{{GroupName: "developers", Level: "CAN_VIEW"}}, b.Config.Permissions) +} diff --git a/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go b/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go index 3a689a7cbdb..bf76ca67bf6 100644 --- a/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go +++ b/bundle/config/mutator/resourcemutator/apply_bundle_permissions.go @@ -124,7 +124,7 @@ func (m *bundlePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Di permissions, key, lm, - b.Config.TargetPermissionsStart(), + b.TargetPermissionsStart(), )...) writePermissions(field, permissions) } diff --git a/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go b/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go index 089a2701bc5..ed9f41a7d86 100644 --- a/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go +++ b/bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go @@ -14,6 +14,7 @@ import ( "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/databricks-sdk-go/service/iam" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -386,3 +387,40 @@ func TestApplyBundlePermissionsEmpty(t *testing.T) { assert.False(t, v.Get("permissions").IsValid(), name) } } + +func TestDuplicatePermissionsInOneSection(t *testing.T) { + for _, section := range []string{"root", "target", "resource"} { + t.Run(section, func(t *testing.T) { + b := &bundle.Bundle{Config: config.Root{ + Targets: map[string]*config.Target{"test": {}}, + Workspace: config.Workspace{CurrentUser: &config.User{ + User: &iam.User{UserName: "deployer@example.test"}, + }}, + Resources: config.Resources{Jobs: map[string]*resources.Job{"test": {}}}, + }} + permissions := []resources.Permission{ + {UserName: "reader@example.test", Level: "CAN_VIEW"}, + {UserName: "reader@example.test", Level: "CAN_MANAGE"}, + } + switch section { + case "root": + b.Config.Permissions = permissions + case "target": + b.Config.Targets["test"].Permissions = permissions + case "resource": + b.Config.Resources.Jobs["test"].Permissions = []resources.JobPermission{ + {UserName: "reader@example.test", Level: "CAN_VIEW"}, + {UserName: "reader@example.test", Level: "CAN_MANAGE"}, + } + } + + diags := bundle.ApplySeq(t.Context(), b, + mutator.SelectTarget("test"), ApplyBundlePermissions(), FixPermissions()) + require.Empty(t, diags) + assert.Equal(t, []resources.JobPermission{ + {UserName: "reader@example.test", Level: "CAN_MANAGE"}, + {UserName: "deployer@example.test", Level: "IS_OWNER"}, + }, b.Config.Resources.Jobs["test"].Permissions) + }) + } +} diff --git a/bundle/config/mutator/select_target.go b/bundle/config/mutator/select_target.go index 43764a9ee38..a6d0aa8b126 100644 --- a/bundle/config/mutator/select_target.go +++ b/bundle/config/mutator/select_target.go @@ -3,9 +3,6 @@ package mutator import ( "context" "fmt" - "maps" - "slices" - "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" @@ -28,39 +25,5 @@ func (m *selectTarget) Name() string { } func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics { - if b.Config.Targets == nil { - return diag.Errorf("no targets defined") - } - - // Get specified target - target, ok := b.Config.Targets[m.name] - if !ok { - return diag.Errorf("%s: no such target. Available targets: %s", m.name, strings.Join(slices.Collect(maps.Keys(b.Config.Targets)), ", ")) - } - - // Merge specified target into root configuration structure. - err := b.Config.MergeTargetOverrides(m.name) - if err != nil { - return diag.FromErr(fmt.Errorf("failed to perform target override for target=%s: %w", m.name, err)) - } - - // Store specified target in configuration for reference. - b.Target = target - b.Config.Bundle.Target = m.name - - // We do this for backward compatibility. - // TODO: remove when Environments section is not supported anymore. - b.Config.Bundle.Environment = b.Config.Bundle.Target - - // Record number of targets in the bundle. This is then used downstream during - // telemetry upload. This value is always >= 1 since if no targets are defined - // in YAML, we create a "default" placeholder target upstream. - b.Metrics.TargetCount = int64(len(b.Config.Targets)) - - // Cleanup the original targets and environments sections since they - // show up in the JSON output of the 'summary' and 'validate' commands. - b.Config.Targets = nil - b.Config.Environments = nil - - return nil + return diag.FromErr(b.SelectTarget(m.name)) } diff --git a/bundle/config/root.go b/bundle/config/root.go index 4a3942996d0..a64b742630e 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -38,9 +38,6 @@ type Root struct { //nolint:recvcheck // value receivers for read-only accessors value dyn.Value depth int - // Nil when no target permissions were merged. Recorded before variable resolution. - targetPermissionsStart *int - // Contains user defined variables Variables map[string]*variable.Variable `json:"variables,omitempty"` @@ -163,13 +160,12 @@ func (r *Root) initializeDynamicValue() error { } func (r *Root) updateWithDynamicValue(nv dyn.Value) error { - // ToTyped clears all fields, including internal state. + // Hack: restore state; it may be cleared by [ToTyped] if + // the configuration equals nil (happens in tests). depth := r.depth - targetPermissionsStart := r.targetPermissionsStart defer func() { r.depth = depth - r.targetPermissionsStart = targetPermissionsStart }() // Convert normalized configuration tree to typed configuration. @@ -330,14 +326,6 @@ func mergeField(rv, ov dyn.Value, name string) (dyn.Value, error) { return dyn.SetByPath(rv, path, out) } -// TargetPermissionsStart returns the first target permission index, or the list length if none were merged. -func (r Root) TargetPermissionsStart() int { - if r.targetPermissionsStart == nil { - return len(r.Permissions) - } - return *r.targetPermissionsStart -} - func (r *Root) MergeTargetOverrides(name string) error { root := r.value target, err := dyn.GetByPath(root, dyn.NewPath(dyn.Key("targets"), dyn.Key(name))) @@ -351,14 +339,6 @@ func (r *Root) MergeTargetOverrides(name string) error { return err } - var targetPermissionsStart *int - if p := target.Get("permissions"); p.IsValid() && p.Kind() != dyn.KindNil { - // Lists concatenate; a complex-variable reference replaces another reference. - rootPermissions, _ := root.Get("permissions").AsSequence() - start := len(rootPermissions) - targetPermissionsStart = &start - } - // Merge fields that can be merged 1:1. for _, f := range []string{ "bundle", @@ -467,11 +447,7 @@ func (r *Root) MergeTargetOverrides(name string) error { } // Convert normalized configuration tree to typed configuration. - if err := r.updateWithDynamicValue(root); err != nil { - return err - } - r.targetPermissionsStart = targetPermissionsStart - return nil + return r.updateWithDynamicValue(root) } var allowedVariableDefinitions = []([]string){