diff --git a/.nextchanges/bundles/snapshot-can-manage-deployer-duplicate.md b/.nextchanges/bundles/snapshot-can-manage-deployer-duplicate.md new file mode 100644 index 00000000000..a3a96d72889 --- /dev/null +++ b/.nextchanges/bundles/snapshot-can-manage-deployer-duplicate.md @@ -0,0 +1 @@ +* Immutable-folder snapshots no longer list the deploying identity twice in `can_manage_principals` when the top-level `permissions` section grants it `CAN_MANAGE`. Affected bundles upload their snapshot to a new path once on the next deploy. ([#7332](https://github.com/databricks/cli/pull/7332)) diff --git a/acceptance/bundle/deploy/immutable/can-manage/databricks.yml.tmpl b/acceptance/bundle/deploy/immutable/can-manage/databricks.yml.tmpl index 3247180b9e9..43c834e898c 100644 --- a/acceptance/bundle/deploy/immutable/can-manage/databricks.yml.tmpl +++ b/acceptance/bundle/deploy/immutable/can-manage/databricks.yml.tmpl @@ -5,6 +5,8 @@ workspace: sync_mode: immutable permissions: + - level: CAN_MANAGE + service_principal_name: ${workspace.current_user.userName} - level: CAN_MANAGE user_name: $TEST_USER_EMAIL diff --git a/acceptance/bundle/deploy/immutable/can-manage/output.txt b/acceptance/bundle/deploy/immutable/can-manage/output.txt index cdcca39708d..3fa11a0fcd1 100644 --- a/acceptance/bundle/deploy/immutable/can-manage/output.txt +++ b/acceptance/bundle/deploy/immutable/can-manage/output.txt @@ -1,18 +1,6 @@ === Deploy with a CAN_MANAGE permission entry >>> [CLI] bundle deploy -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: - - service_principal_name: [USERNAME] - level: CAN_MANAGE - -See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration. - in databricks.yml:8:3 - Created internal_immutable_snapshots.immutable Created jobs.my_job Created jobs.my_job.permissions @@ -31,18 +19,6 @@ Resources: 3 created, 0 changed, 0 deleted, 0 unchanged ] >>> [CLI] bundle destroy --auto-approve -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: - - service_principal_name: [USERNAME] - level: CAN_MANAGE - -See https://docs.databricks.com/dev-tools/bundles/permissions.html to learn more about permission configuration. - in databricks.yml:8:3 - The following resources will be deleted: delete resources.jobs.my_job diff --git a/bundle/deploy/snapshot/upload.go b/bundle/deploy/snapshot/upload.go index cdfb6d538d3..a4b01c190e2 100644 --- a/bundle/deploy/snapshot/upload.go +++ b/bundle/deploy/snapshot/upload.go @@ -204,28 +204,34 @@ func BuildACL(b *bundle.Bundle) []snapshot.ACLEntry { // CAN_MANAGE in the top-level permissions section. The deploying identity is always included // because the API rejects an empty list, and it is the one principal always available. The // recommended permissions section names it with CAN_MANAGE as well, so skip it in the loop -// rather than sending it twice. +// rather than sending it twice. Other principals listed more than once are sent once. func BuildCanManage(b *bundle.Bundle) []snapshot.ManagePrincipal { currentUser := b.Config.Workspace.CurrentUser.User - var canManage []snapshot.ManagePrincipal + deployer := snapshot.ManagePrincipal{UserName: currentUser.UserName} if iamutil.IsServicePrincipal(currentUser) { - canManage = append(canManage, snapshot.ManagePrincipal{ - ServicePrincipalName: currentUser.UserName, - }) - } else { - canManage = append(canManage, snapshot.ManagePrincipal{ - UserName: currentUser.UserName, - }) + deployer = snapshot.ManagePrincipal{ServicePrincipalName: currentUser.UserName} } + canManage := []snapshot.ManagePrincipal{deployer} + seen := map[snapshot.ManagePrincipal]bool{} for _, p := range b.Config.Permissions { if p.Level != "CAN_MANAGE" { continue } - canManage = append(canManage, snapshot.ManagePrincipal{ + // Match the deployer by name in either field, as the permissions diagnostics do, so a + // config that satisfies their recommendation never sends the deployer twice. + if p.UserName == currentUser.UserName || p.ServicePrincipalName == currentUser.UserName { + continue + } + principal := snapshot.ManagePrincipal{ UserName: p.UserName, GroupName: p.GroupName, ServicePrincipalName: p.ServicePrincipalName, - }) + } + if seen[principal] { + continue + } + seen[principal] = true + canManage = append(canManage, principal) } return canManage } diff --git a/bundle/deploy/snapshot/upload_warning_test.go b/bundle/deploy/snapshot/upload_warning_test.go index 254d7a70aa6..4567964bb49 100644 --- a/bundle/deploy/snapshot/upload_warning_test.go +++ b/bundle/deploy/snapshot/upload_warning_test.go @@ -109,13 +109,14 @@ func canManageBundle(permissions []resources.Permission) *bundle.Bundle { func TestBuildCanManage(t *testing.T) { b := canManageBundle([]resources.Permission{ + {Level: "CAN_MANAGE", UserName: "deployer@example.test"}, {Level: "CAN_MANAGE", UserName: "manager@example.test"}, {Level: "CAN_VIEW", UserName: "viewer@example.test"}, {Level: "CAN_MANAGE", GroupName: "admins"}, {Level: "CAN_MANAGE", ServicePrincipalName: "sp-1"}, }) - // The deploying identity, then only the CAN_MANAGE principals. + // The deploying identity once, then only the other CAN_MANAGE principals. assert.Equal(t, []snapshot.ManagePrincipal{ {UserName: "deployer@example.test"}, {UserName: "manager@example.test"}, @@ -124,6 +125,21 @@ func TestBuildCanManage(t *testing.T) { }, BuildCanManage(b)) } +func TestBuildCanManageSkipsDeployerListedInOtherField(t *testing.T) { + // The permissions diagnostics accept the deployer under either name field, so a user + // deployer listed as service_principal_name is still the deployer. + b := canManageBundle([]resources.Permission{ + {Level: "CAN_MANAGE", ServicePrincipalName: "deployer@example.test"}, + {Level: "CAN_MANAGE", GroupName: "admins"}, + {Level: "CAN_MANAGE", GroupName: "admins"}, + }) + + assert.Equal(t, []snapshot.ManagePrincipal{ + {UserName: "deployer@example.test"}, + {GroupName: "admins"}, + }, BuildCanManage(b)) +} + func TestBuildCanManageWithoutManagers(t *testing.T) { b := canManageBundle([]resources.Permission{ {Level: "CAN_VIEW", UserName: "viewer@example.test"},