diff --git a/acceptance/bundle/apps/job_permissions/app/app.py b/acceptance/bundle/apps/job_permissions/app/app.py new file mode 100644 index 0000000000..11b15b1a45 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions/app/app.py @@ -0,0 +1 @@ +print("hello") diff --git a/acceptance/bundle/apps/job_permissions/databricks.yml b/acceptance/bundle/apps/job_permissions/databricks.yml new file mode 100644 index 0000000000..b16634f86e --- /dev/null +++ b/acceptance/bundle/apps/job_permissions/databricks.yml @@ -0,0 +1,30 @@ +bundle: + name: test-bundle + +permissions: + - level: CAN_MANAGE + user_name: ${workspace.current_user.userName} + +resources: + jobs: + my_job: + name: my-job + tasks: + - task_key: hello + environment_key: default + spark_python_task: + python_file: ./hello.py + environments: + - environment_key: default + spec: + client: "1" + + apps: + my_app: + name: my-app + source_code_path: ./app + resources: + - name: my-job + job: + id: ${resources.jobs.my_job.id} + permission: CAN_MANAGE_RUN diff --git a/acceptance/bundle/apps/job_permissions/fix.patch b/acceptance/bundle/apps/job_permissions/fix.patch new file mode 100644 index 0000000000..ee49b41295 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions/fix.patch @@ -0,0 +1,11 @@ +--- a/databricks.yml ++++ b/databricks.yml +@@ -10,6 +10,9 @@ + my_job: + name: my-job ++ permissions: ++ - level: CAN_MANAGE_RUN ++ service_principal_name: ${resources.apps.my_app.service_principal_client_id} + tasks: + - task_key: hello + environment_key: default diff --git a/acceptance/bundle/apps/job_permissions/hello.py b/acceptance/bundle/apps/job_permissions/hello.py new file mode 100644 index 0000000000..11b15b1a45 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions/hello.py @@ -0,0 +1 @@ +print("hello") diff --git a/acceptance/bundle/apps/job_permissions/out.test.toml b/acceptance/bundle/apps/job_permissions/out.test.toml new file mode 100644 index 0000000000..a9f28de48a --- /dev/null +++ b/acceptance/bundle/apps/job_permissions/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = true + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform"] diff --git a/acceptance/bundle/apps/job_permissions/output.txt b/acceptance/bundle/apps/job_permissions/output.txt new file mode 100644 index 0000000000..7a8c3e9691 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions/output.txt @@ -0,0 +1,40 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== After first deploy +>>> has_manage_run +true + +=== After second deploy +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> has_manage_run +false + +=== Apply fix and redeploy +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> has_manage_run +true + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.apps.my_app + delete resources.jobs.my_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/apps/job_permissions/script b/acceptance/bundle/apps/job_permissions/script new file mode 100644 index 0000000000..5b404560ac --- /dev/null +++ b/acceptance/bundle/apps/job_permissions/script @@ -0,0 +1,26 @@ + +# Test that app-granted permissions on a job survive a second deploy. +# Issue: https://github.com/databricks/cli/issues/4309 + +trace $CLI bundle deploy + +job_id=$(read_id.py my_job) + +has_manage_run() { + MSYS_NO_PATHCONV=1 $CLI api get /api/2.0/permissions/jobs/$job_id \ + | jq 'any(.access_control_list[].all_permissions[]; .permission_level == "CAN_MANAGE_RUN")' +} + +title "After first deploy" +trace has_manage_run + +title "After second deploy" +trace $CLI bundle deploy +trace has_manage_run + +title "Apply fix and redeploy" +patch -s --no-backup-if-mismatch databricks.yml < fix.patch +trace $CLI bundle deploy +trace has_manage_run + +trace $CLI bundle destroy --auto-approve diff --git a/acceptance/bundle/apps/job_permissions/test.toml b/acceptance/bundle/apps/job_permissions/test.toml new file mode 100644 index 0000000000..4935cf6732 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions/test.toml @@ -0,0 +1,11 @@ +# Direct engine error: cannot plan resources.jobs.my_job.permissions: cannot update +# [0].service_principal_name: failed to navigate to parent [0]: [0]: cannot index struct. +# This is a bug in structaccess.Set() where it fails to index into a struct when +# setting permissions with service_principal_name. +# See https://github.com/databricks/cli/pull/4644 +Badness = "Direct engine fails to plan permissions with service_principal_name on jobs" +Cloud = true +RecordRequests = false + +[EnvMatrix] +DATABRICKS_BUNDLE_ENGINE = ["terraform"] diff --git a/acceptance/bundle/generate/app_not_yet_deployed/output.txt b/acceptance/bundle/generate/app_not_yet_deployed/output.txt index 3b693fb2c7..feb98e9108 100644 --- a/acceptance/bundle/generate/app_not_yet_deployed/output.txt +++ b/acceptance/bundle/generate/app_not_yet_deployed/output.txt @@ -11,6 +11,9 @@ }, "id":"1000", "name":"my-app", + "service_principal_client_id":"[UUID]", + "service_principal_id":[NUMID], + "service_principal_name":"app-my-app", "url":"my-app-123.cloud.databricksapps.com" } diff --git a/acceptance/bundle/resources/apps/create_already_exists/output.txt b/acceptance/bundle/resources/apps/create_already_exists/output.txt index de5ccd70ac..19af292ead 100644 --- a/acceptance/bundle/resources/apps/create_already_exists/output.txt +++ b/acceptance/bundle/resources/apps/create_already_exists/output.txt @@ -11,6 +11,9 @@ }, "id":"1000", "name":"test-app-already-exists", + "service_principal_client_id":"[UUID]", + "service_principal_id":[NUMID], + "service_principal_name":"app-test-app-already-exists", "url":"test-app-already-exists-123.cloud.databricksapps.com" } diff --git a/acceptance/bundle/resources/apps/update/out.requests.terraform.json b/acceptance/bundle/resources/apps/update/out.requests.terraform.json index f277c7dfe4..98dfdcccdc 100644 --- a/acceptance/bundle/resources/apps/update/out.requests.terraform.json +++ b/acceptance/bundle/resources/apps/update/out.requests.terraform.json @@ -12,7 +12,10 @@ { "body": { "description": "MY_APP_DESCRIPTION", - "name": "myappname" + "name": "myappname", + "service_principal_client_id": "[UUID]", + "service_principal_id": [NUMID], + "service_principal_name": "app-myappname" }, "method": "PATCH", "path": "/api/2.0/apps/myappname" diff --git a/acceptance/cmd/workspace/apps/output.txt b/acceptance/cmd/workspace/apps/output.txt index 8d7c0ee659..618942df4c 100644 --- a/acceptance/cmd/workspace/apps/output.txt +++ b/acceptance/cmd/workspace/apps/output.txt @@ -24,6 +24,9 @@ } } ], + "service_principal_client_id":"[UUID]", + "service_principal_id":[NUMID], + "service_principal_name":"app-test-name", "url":"test-name-123.cloud.databricksapps.com" } @@ -52,6 +55,9 @@ } } ], + "service_principal_client_id":"[UUID]", + "service_principal_id":[NUMID], + "service_principal_name":"app-test-name", "url":"test-name-123.cloud.databricksapps.com" } diff --git a/libs/testserver/apps.go b/libs/testserver/apps.go index 3fb513eee5..8abd4cd416 100644 --- a/libs/testserver/apps.go +++ b/libs/testserver/apps.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/databricks/databricks-sdk-go/service/apps" + "github.com/databricks/databricks-sdk-go/service/iam" ) func (s *FakeWorkspace) AppsCreateUpdate(req Request, name string) Response { @@ -140,6 +141,29 @@ func (s *FakeWorkspace) AppsUpsert(req Request, name string) Response { app.Url = name + "-123.cloud.databricksapps.com" app.Id = strconv.Itoa(len(s.Apps) + 1000) + // Assign a service principal to the app, mimicking the real platform. + if app.ServicePrincipalClientId == "" { + app.ServicePrincipalClientId = nextUUID() + app.ServicePrincipalId = nextID() + app.ServicePrincipalName = "app-" + name + } + + // Simulate the apps platform side effect: when an app references a job + // with a permission, the platform grants that permission to the app's + // service principal on the referenced resource. + for _, res := range app.Resources { + if res.Job == nil { + continue + } + s.upsertPermission("/jobs/"+res.Job.Id, iam.AccessControlResponse{ + ServicePrincipalName: app.ServicePrincipalName, + AllPermissions: []iam.Permission{{ + PermissionLevel: iam.PermissionLevel(res.Job.Permission), + ForceSendFields: []string{"Inherited"}, + }}, + }) + } + s.Apps[name] = app return Response{ Body: app, diff --git a/libs/testserver/permissions.go b/libs/testserver/permissions.go index 12483429af..eb5d1c1d0f 100644 --- a/libs/testserver/permissions.go +++ b/libs/testserver/permissions.go @@ -34,6 +34,44 @@ var requestObjectTypeToObjectType = map[string]string{ "alertsv2": "alertv2", } +// aclPrincipalKey returns a unique key identifying the principal in an ACL entry. +func aclPrincipalKey(acl iam.AccessControlResponse) string { + switch { + case acl.UserName != "": + return "user:" + acl.UserName + case acl.GroupName != "": + return "group:" + acl.GroupName + case acl.ServicePrincipalName != "": + return "sp:" + acl.ServicePrincipalName + default: + return "" + } +} + +// upsertACL adds or replaces an ACL entry by principal key. +// Entries with no principal key are ignored. +func upsertACL(perms *iam.ObjectPermissions, entry iam.AccessControlResponse) { + key := aclPrincipalKey(entry) + if key == "" { + return + } + for i, acl := range perms.AccessControlList { + if aclPrincipalKey(acl) == key { + perms.AccessControlList[i] = entry + return + } + } + perms.AccessControlList = append(perms.AccessControlList, entry) +} + +// upsertPermission adds or replaces an ACL entry on the given object. +// Must be called with s.mu held. +func (s *FakeWorkspace) upsertPermission(objectKey string, entry iam.AccessControlResponse) { + perms := s.Permissions[objectKey] + upsertACL(&perms, entry) + s.Permissions[objectKey] = perms +} + // GetPermissions retrieves permissions for a given object type and ID func (s *FakeWorkspace) GetPermissions(req Request) any { defer s.LockUnlock()() @@ -136,26 +174,9 @@ func (s *FakeWorkspace) SetPermissions(req Request) any { } } - // Convert AccessControlRequest to AccessControlResponse - // Use map to track principal indices and slice to preserve order - principalIndices := make(map[string]int) - var newAccessControlList []iam.AccessControlResponse - + // Convert AccessControlRequest to AccessControlResponse and replace the ACL. + existingPermissions.AccessControlList = nil for _, acl := range updateRequest.AccessControlList { - // Determine principal key - use the non-empty field as the unique identifier - var principalKey string - if acl.UserName != "" { - principalKey = "user:" + acl.UserName - } else if acl.GroupName != "" { - principalKey = "group:" + acl.GroupName - } else if acl.ServicePrincipalName != "" { - principalKey = "sp:" + acl.ServicePrincipalName - } - - if principalKey == "" { - continue // Skip invalid entries - } - display := acl.UserName if display == "" { display = acl.ServicePrincipalName @@ -166,32 +187,19 @@ func (s *FakeWorkspace) SetPermissions(req Request) any { GroupName: acl.GroupName, ServicePrincipalName: acl.ServicePrincipalName, DisplayName: display, - AllPermissions: []iam.Permission{}, } - // Convert PermissionLevel to Permission if acl.PermissionLevel != "" { - response.AllPermissions = append(response.AllPermissions, iam.Permission{ + response.AllPermissions = []iam.Permission{{ Inherited: false, PermissionLevel: acl.PermissionLevel, ForceSendFields: []string{"Inherited"}, - }) + }} } - // Check if principal already exists in our list - if index, exists := principalIndices[principalKey]; exists { - // Update existing entry (last entry for same principal wins) - newAccessControlList[index] = response - } else { - // Add new entry and track its index - principalIndices[principalKey] = len(newAccessControlList) - newAccessControlList = append(newAccessControlList, response) - } + upsertACL(&existingPermissions, response) } - // Update the permissions - existingPermissions.AccessControlList = newAccessControlList - // Apply cloud environment fixups - better match cloud env if requestObjectType == "jobs" { existingPermissions.AccessControlList = append(existingPermissions.AccessControlList, iam.AccessControlResponse{