diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index b4b9c6b0a3..4b8ed3d9c6 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -10,6 +10,7 @@ ### Bundles * Add new Lakeflow Pipelines support for bundle generate ([#3568](https://github.com/databricks/cli/pull/3568)) +* Fix bundle deploy to not update permissions or grants for unbound resources ([#3642](https://github.com/databricks/cli/pull/3642)) * Introduce new bundle variable: `${workspace.current_user.domain_friendly_name}` ([#3623](https://github.com/databricks/cli/pull/3623)) ### API Changes diff --git a/acceptance/bundle/deploy/experimental-python/output.txt b/acceptance/bundle/deploy/experimental-python/output.txt index 590d7cd350..9dad22cf1d 100644 --- a/acceptance/bundle/deploy/experimental-python/output.txt +++ b/acceptance/bundle/deploy/experimental-python/output.txt @@ -8,6 +8,7 @@ Deployment complete! >>> [CLI] jobs list --output json [ { + "creator_user_name": "[USERNAME]", "job_id": [NUMID], "settings": { "deployment": { diff --git a/acceptance/bundle/deploy/python-notebook/output.txt b/acceptance/bundle/deploy/python-notebook/output.txt index 6e2f2f8de7..704430ada4 100644 --- a/acceptance/bundle/deploy/python-notebook/output.txt +++ b/acceptance/bundle/deploy/python-notebook/output.txt @@ -8,6 +8,7 @@ Deployment complete! >>> [CLI] jobs list --output json [ { + "creator_user_name": "[USERNAME]", "job_id": [NUMID], "settings": { "deployment": { diff --git a/acceptance/bundle/deployment/unbind/grants/databricks.yml.tmpl b/acceptance/bundle/deployment/unbind/grants/databricks.yml.tmpl new file mode 100644 index 0000000000..ca0346b412 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/grants/databricks.yml.tmpl @@ -0,0 +1,19 @@ +bundle: + name: unbind_grants-$UNIQUE_NAME + +workspace: + root_path: ~/.bundle/$UNIQUE_NAME + +variables: + suffix: + default: "" + description: "Suffix for the schema name" + +resources: + schemas: + schema_1: + name: "test-schema-$UNIQUE_NAME${var.suffix}" + catalog_name: "main" + grants: + - principal: "account users" + privileges: ["CREATE_VOLUME", "SELECT"] diff --git a/acceptance/bundle/deployment/unbind/grants/out.test.toml b/acceptance/bundle/deployment/unbind/grants/out.test.toml new file mode 100644 index 0000000000..8409922737 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/grants/out.test.toml @@ -0,0 +1,7 @@ +Local = true +Cloud = true +RequiresUnityCatalog = true + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform"] + Ignore = [".databricks"] diff --git a/acceptance/bundle/deployment/unbind/grants/output.txt b/acceptance/bundle/deployment/unbind/grants/output.txt new file mode 100644 index 0000000000..df6ed2d492 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/grants/output.txt @@ -0,0 +1,49 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] grants get schema main.test-schema-[UNIQUE_NAME] --output json +{ + "principal": "account users", + "privileges": [ + "CREATE_VOLUME", + "SELECT" + ] +} + +>>> [CLI] bundle deployment unbind schema_1 +Updating deployment state... + +>>> [CLI] bundle deploy --var suffix=another +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Grants should be the same as before unbind +>>> [CLI] grants get schema main.test-schema-[UNIQUE_NAME] --output json +{ + "principal": "account users", + "privileges": [ + "CREATE_VOLUME", + "SELECT" + ] +} + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete schema schema_1 + +This action will result in the deletion of the following UC schemas. Any underlying data may be lost: + delete schema schema_1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME] + +Deleting files... +Destroy complete! + +>>> [CLI] schemas delete main.test-schema-[UNIQUE_NAME] +0 diff --git a/acceptance/bundle/deployment/unbind/grants/script b/acceptance/bundle/deployment/unbind/grants/script new file mode 100644 index 0000000000..f6da92a7e6 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/grants/script @@ -0,0 +1,20 @@ +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve + if [[ -n "$schema_id" ]]; then + trace $CLI schemas delete $schema_id + fi + echo $? +} +trap cleanup EXIT + +trace $CLI bundle deploy +schema_id=$($CLI bundle summary --output json | jq -r '.resources.schemas.schema_1.id') +trace $CLI grants get schema $schema_id --output json | jq '.privilege_assignments[] | select(.principal == "account users")' + +trace $CLI bundle deployment unbind schema_1 +trace $CLI bundle deploy --var "suffix=another" + +title "Grants should be the same as before unbind" +trace $CLI grants get schema $schema_id --output json | jq '.privilege_assignments[] | select(.principal == "account users")' diff --git a/acceptance/bundle/deployment/unbind/grants/test.toml b/acceptance/bundle/deployment/unbind/grants/test.toml new file mode 100644 index 0000000000..ca6de773cd --- /dev/null +++ b/acceptance/bundle/deployment/unbind/grants/test.toml @@ -0,0 +1,8 @@ +RequiresUnityCatalog = true + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform"] + +Ignore = [ + ".databricks", +] diff --git a/acceptance/bundle/deployment/unbind/job/output.txt b/acceptance/bundle/deployment/unbind/job/output.txt index f3b5963b53..482431cae7 100644 --- a/acceptance/bundle/deployment/unbind/job/output.txt +++ b/acceptance/bundle/deployment/unbind/job/output.txt @@ -18,6 +18,7 @@ Deployment complete! >>> [CLI] jobs get [NUMID] --output json { + "creator_user_name":"[USERNAME]", "job_id":[NUMID], "settings": { "deployment": { diff --git a/acceptance/bundle/deployment/unbind/permissions/databricks.yml.tmpl b/acceptance/bundle/deployment/unbind/permissions/databricks.yml.tmpl new file mode 100644 index 0000000000..206eee9d19 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/permissions/databricks.yml.tmpl @@ -0,0 +1,13 @@ +bundle: + name: unbind_permissions + +workspace: + root_path: "~/.bundle/$UNIQUE_NAME" + +resources: + jobs: + job_1: + name: "Job name" + permissions: + - group_name: users + level: CAN_MANAGE diff --git a/acceptance/bundle/deployment/unbind/permissions/out.test.toml b/acceptance/bundle/deployment/unbind/permissions/out.test.toml new file mode 100644 index 0000000000..4dccab7df1 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/permissions/out.test.toml @@ -0,0 +1,6 @@ +Local = true +Cloud = true + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform"] + Ignore = [".databricks"] diff --git a/acceptance/bundle/deployment/unbind/permissions/output.txt b/acceptance/bundle/deployment/unbind/permissions/output.txt new file mode 100644 index 0000000000..d43b51fdd5 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/permissions/output.txt @@ -0,0 +1,52 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] bundle summary --output json + +>>> [CLI] jobs get-permissions [NUMID] --output json +{ + "all_permissions": [ + { + "inherited": false, + "permission_level": "CAN_MANAGE" + } + ], + "group_name": "users" +} + +>>> [CLI] bundle deployment unbind job_1 +Updating deployment state... + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Permissions should be the same as before unbind +>>> [CLI] jobs get-permissions [NUMID] --output json +{ + "all_permissions": [ + { + "inherited": false, + "permission_level": "CAN_MANAGE" + } + ], + "group_name": "users" +} + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete job job_1 + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME] + +Deleting files... +Destroy complete! + +>>> [CLI] jobs delete [NUMID] +0 diff --git a/acceptance/bundle/deployment/unbind/permissions/script b/acceptance/bundle/deployment/unbind/permissions/script new file mode 100644 index 0000000000..078e3c3af6 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/permissions/script @@ -0,0 +1,18 @@ +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve + trace $CLI jobs delete $job_id + echo $? +} +trap cleanup EXIT + +trace $CLI bundle deploy +job_id=$(trace $CLI bundle summary --output json | jq -r '.resources.jobs.job_1.id') +trace $CLI jobs get-permissions $job_id --output json | jq '.access_control_list[] | select(.group_name == "users")' + +trace $CLI bundle deployment unbind job_1 +trace $CLI bundle deploy + +title "Permissions should be the same as before unbind" +trace $CLI jobs get-permissions $job_id --output json | jq '.access_control_list[] | select(.group_name == "users")' diff --git a/acceptance/bundle/deployment/unbind/permissions/test.toml b/acceptance/bundle/deployment/unbind/permissions/test.toml new file mode 100644 index 0000000000..0708e106d0 --- /dev/null +++ b/acceptance/bundle/deployment/unbind/permissions/test.toml @@ -0,0 +1,6 @@ +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform"] + +Ignore = [ + ".databricks", +] diff --git a/acceptance/bundle/deployment/unbind/python-job/output.txt b/acceptance/bundle/deployment/unbind/python-job/output.txt index 95ef280a81..acfdafa930 100644 --- a/acceptance/bundle/deployment/unbind/python-job/output.txt +++ b/acceptance/bundle/deployment/unbind/python-job/output.txt @@ -18,6 +18,7 @@ Deployment complete! >>> [CLI] jobs get [NUMID] --output json { + "creator_user_name":"[USERNAME]", "job_id":[NUMID], "settings": { "deployment": { diff --git a/acceptance/bundle/resource_deps/jobs_update/output.txt b/acceptance/bundle/resource_deps/jobs_update/output.txt index 26ab67029b..10cffda960 100644 --- a/acceptance/bundle/resource_deps/jobs_update/output.txt +++ b/acceptance/bundle/resource_deps/jobs_update/output.txt @@ -153,6 +153,7 @@ Deployment complete! >>> [CLI] jobs get [BAR_ID] { + "creator_user_name":"[USERNAME]", "job_id":[BAR_ID], "settings": { "deployment": { diff --git a/bundle/deploy/terraform/unbind.go b/bundle/deploy/terraform/unbind.go index 494cb7ef1f..6eeb323f16 100644 --- a/bundle/deploy/terraform/unbind.go +++ b/bundle/deploy/terraform/unbind.go @@ -10,8 +10,9 @@ import ( ) type unbind struct { - resourceType string - resourceKey string + bundleType string + tfResourceType string + resourceKey string } func (m *unbind) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { @@ -25,11 +26,32 @@ func (m *unbind) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return diag.Errorf("terraform init: %v", err) } - err = tf.StateRm(ctx, fmt.Sprintf("%s.%s", m.resourceType, m.resourceKey)) + err = tf.StateRm(ctx, fmt.Sprintf("%s.%s", m.tfResourceType, m.resourceKey)) if err != nil { return diag.Errorf("terraform state rm: %v", err) } + // Also remove the permission if it exists + // First check terraform state list to see if the permission exists + state, err := tf.Show(ctx) + if err != nil { + return diag.Errorf("terraform show: %v", err) + } + + if state.Values == nil || state.Values.RootModule == nil || state.Values.RootModule.Resources == nil { + return nil + } + + for _, resource := range state.Values.RootModule.Resources { + if resource.Address == fmt.Sprintf("databricks_permissions.%s_%s", m.bundleType, m.resourceKey) || + resource.Address == fmt.Sprintf("databricks_grants.%s_%s", m.bundleType, m.resourceKey) { + err = tf.StateRm(ctx, resource.Address) + if err != nil { + return diag.Errorf("terraform state rm: %v", err) + } + } + } + return nil } @@ -37,6 +59,6 @@ func (*unbind) Name() string { return "terraform.Unbind" } -func Unbind(resourceType, resourceKey string) bundle.Mutator { - return &unbind{resourceType: resourceType, resourceKey: resourceKey} +func Unbind(bundleType, tfResourceType, resourceKey string) bundle.Mutator { + return &unbind{bundleType: bundleType, tfResourceType: tfResourceType, resourceKey: resourceKey} } diff --git a/bundle/phases/bind.go b/bundle/phases/bind.go index afc42a639c..8d8f5ff7a1 100644 --- a/bundle/phases/bind.go +++ b/bundle/phases/bind.go @@ -32,7 +32,7 @@ func Bind(ctx context.Context, b *bundle.Bundle, opts *terraform.BindOptions) { ) } -func Unbind(ctx context.Context, b *bundle.Bundle, resourceType, resourceKey string) { +func Unbind(ctx context.Context, b *bundle.Bundle, bundleType, tfResourceType, resourceKey string) { log.Info(ctx, "Phase: unbind") bundle.ApplyContext(ctx, b, lock.Acquire()) @@ -48,7 +48,7 @@ func Unbind(ctx context.Context, b *bundle.Bundle, resourceType, resourceKey str statemgmt.StatePull(), terraform.Interpolate(), terraform.Write(), - terraform.Unbind(resourceType, resourceKey), + terraform.Unbind(bundleType, tfResourceType, resourceKey), statemgmt.StatePush(), ) } diff --git a/cmd/bundle/deployment/unbind.go b/cmd/bundle/deployment/unbind.go index c0ebe52661..0c9d399c9d 100644 --- a/cmd/bundle/deployment/unbind.go +++ b/cmd/bundle/deployment/unbind.go @@ -76,8 +76,9 @@ To re-bind the resource later, use: b.Config.Bundle.Deployment.Lock.Force = forceLock }) - tfName := terraform.GroupToTerraformName[resource.ResourceDescription().PluralName] - phases.Unbind(ctx, b, tfName, args[0]) + rd := resource.ResourceDescription() + tfName := terraform.GroupToTerraformName[rd.PluralName] + phases.Unbind(ctx, b, rd.SingularName, tfName, args[0]) if logdiag.HasError(ctx) { return root.ErrAlreadyPrinted } diff --git a/libs/testserver/fake_workspace.go b/libs/testserver/fake_workspace.go index 00e85d3453..9918cc1888 100644 --- a/libs/testserver/fake_workspace.go +++ b/libs/testserver/fake_workspace.go @@ -60,16 +60,17 @@ type FakeWorkspace struct { repoIdByPath map[string]int64 // normally, ids are not sequential, but we make them sequential for deterministic diff - nextJobId int64 - nextJobRunId int64 - Jobs map[int64]jobs.Job - JobRuns map[int64]jobs.Run - + nextJobId int64 + nextJobRunId int64 + Jobs map[int64]jobs.Job + JobRuns map[int64]jobs.Run + JobPermissions map[string][]jobs.JobAccessControlRequest Pipelines map[string]pipelines.GetPipelineResponse PipelineUpdates map[string]bool Monitors map[string]catalog.MonitorInfo Apps map[string]apps.App Schemas map[string]catalog.SchemaInfo + SchemasGrants map[string][]catalog.PrivilegeAssignment Volumes map[string]catalog.VolumeInfo Dashboards map[string]dashboards.Dashboard SqlWarehouses map[string]sql.GetWarehouseResponse @@ -153,6 +154,8 @@ func NewFakeWorkspace(url, token string) *FakeWorkspace { Jobs: map[int64]jobs.Job{}, JobRuns: map[int64]jobs.Run{}, + JobPermissions: map[string][]jobs.JobAccessControlRequest{}, + SchemasGrants: map[string][]catalog.PrivilegeAssignment{}, nextJobId: TestJobID, nextJobRunId: TestRunID, Pipelines: map[string]pipelines.GetPipelineResponse{}, diff --git a/libs/testserver/handlers.go b/libs/testserver/handlers.go index 256e3766e9..a0ab2070eb 100644 --- a/libs/testserver/handlers.go +++ b/libs/testserver/handlers.go @@ -349,6 +349,14 @@ func AddDefaultHandlers(server *Server) { return MapDelete(req.Workspace, req.Workspace.Schemas, req.Vars["full_name"]) }) + server.Handle("PATCH", "/api/2.1/unity-catalog/permissions/schema/{full_name}", func(req Request) any { + return req.Workspace.SchemasUpdateGrants(req, req.Vars["full_name"]) + }) + + server.Handle("GET", "/api/2.1/unity-catalog/permissions/schema/{full_name}", func(req Request) any { + return req.Workspace.SchemasGetGrants(req, req.Vars["full_name"]) + }) + // Volumes: server.Handle("GET", "/api/2.1/unity-catalog/volumes/{full_name}", func(req Request) any { @@ -505,4 +513,12 @@ func AddDefaultHandlers(server *Server) { server.Handle("DELETE", "/api/2.0/database/synced_tables/{name}", func(req Request) any { return MapDelete(req.Workspace, req.Workspace.SyncedDatabaseTables, req.Vars["name"]) }) + + server.Handle("PUT", "/api/2.0/permissions/jobs/{job_id}", func(req Request) any { + return req.Workspace.JobsUpdatePermissions(req, req.Vars["job_id"]) + }) + + server.Handle("GET", "/api/2.0/permissions/jobs/{job_id}", func(req Request) any { + return req.Workspace.JobsGetPermissions(req, req.Vars["job_id"]) + }) } diff --git a/libs/testserver/jobs.go b/libs/testserver/jobs.go index 334898a306..45c368b9e2 100644 --- a/libs/testserver/jobs.go +++ b/libs/testserver/jobs.go @@ -31,7 +31,9 @@ func (s *FakeWorkspace) JobsCreate(req Request) Response { } } - s.Jobs[jobId] = jobs.Job{JobId: jobId, Settings: &jobSettings} + // CreatorUserName field is used by TF to check if the resource exists or not. CreatorUserName should be non-empty for the resource to be considered as "exists" + // https://github.com/databricks/terraform-provider-databricks/blob/main/permissions/permission_definitions.go#L108 + s.Jobs[jobId] = jobs.Job{JobId: jobId, Settings: &jobSettings, CreatorUserName: TestUser.UserName} return Response{Body: jobs.CreateResponse{JobId: jobId}} } @@ -145,6 +147,64 @@ func (s *FakeWorkspace) JobsGetRun(req Request) Response { return Response{Body: run} } +func (s *FakeWorkspace) JobsUpdatePermissions(req Request, jobId string) Response { + var request jobs.JobPermissionsRequest + if err := json.Unmarshal(req.Body, &request); err != nil { + return Response{ + StatusCode: 400, + Body: fmt.Sprintf("request parsing error: %s", err), + } + } + + defer s.LockUnlock()() + + s.JobPermissions[jobId] = request.AccessControlList + var acl []jobs.JobAccessControlResponse + for _, accessControlList := range request.AccessControlList { + acl = append(acl, jobs.JobAccessControlResponse{ + UserName: accessControlList.UserName, + ServicePrincipalName: accessControlList.ServicePrincipalName, + GroupName: accessControlList.GroupName, + AllPermissions: []jobs.JobPermission{ + { + PermissionLevel: accessControlList.PermissionLevel, + }, + }, + }) + } + + return Response{Body: jobs.JobPermissions{ + AccessControlList: acl, + ObjectType: "job", + ObjectId: jobId, + }} +} + +func (s *FakeWorkspace) JobsGetPermissions(req Request, jobId string) Response { + jobPermissions := s.JobPermissions[jobId] + var acl []jobs.JobAccessControlResponse + for _, accessControlList := range jobPermissions { + acl = append(acl, jobs.JobAccessControlResponse{ + UserName: accessControlList.UserName, + DisplayName: accessControlList.UserName, + ServicePrincipalName: accessControlList.ServicePrincipalName, + GroupName: accessControlList.GroupName, + AllPermissions: []jobs.JobPermission{ + { + PermissionLevel: accessControlList.PermissionLevel, + ForceSendFields: []string{"Inherited"}, + }, + }, + }) + } + + return Response{Body: jobs.JobPermissions{ + AccessControlList: acl, + ObjectType: "job", + ObjectId: "/jobs/" + jobId, + }} +} + func setSourceIfNotSet(job jobs.Job) jobs.Job { if job.Settings != nil { source := "WORKSPACE" diff --git a/libs/testserver/schemas.go b/libs/testserver/schemas.go index 94c63b0627..4dace522be 100644 --- a/libs/testserver/schemas.go +++ b/libs/testserver/schemas.go @@ -60,3 +60,43 @@ func (s *FakeWorkspace) SchemasUpdate(req Request, name string) Response { Body: existing, } } + +func (s *FakeWorkspace) SchemasUpdateGrants(req Request, fullName string) Response { + var request catalog.UpdatePermissions + if err := json.Unmarshal(req.Body, &request); err != nil { + return Response{ + StatusCode: http.StatusBadRequest, + Body: fmt.Sprintf("request parsing error: %s", err), + } + } + defer s.LockUnlock()() + + // For simplicity, we'll just replace all grants (similar to how job permissions work) + var grants []catalog.PrivilegeAssignment + for _, change := range request.Changes { + if len(change.Add) > 0 { + grants = append(grants, catalog.PrivilegeAssignment{ + Principal: change.Principal, + Privileges: change.Add, + }) + } + } + s.SchemasGrants[fullName] = grants + + return Response{ + Body: catalog.GetPermissionsResponse{ + PrivilegeAssignments: grants, + }, + } +} + +func (s *FakeWorkspace) SchemasGetGrants(req Request, fullName string) Response { + defer s.LockUnlock()() + + grants := s.SchemasGrants[fullName] + return Response{ + Body: catalog.GetPermissionsResponse{ + PrivilegeAssignments: grants, + }, + } +}