From 2cce7f0fe1187c21e0950c2eb4879cdae52527c9 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Fri, 19 Apr 2024 07:05:17 -0700 Subject: [PATCH 1/8] Support IS_OWNER as a top-level permission --- bundle/config/mutator/process_target_mode.go | 19 +++++- .../mutator/process_target_mode_test.go | 20 +++++- bundle/config/mutator/run_as_test.go | 7 +- bundle/config/mutator/select_target.go | 4 -- ...ce_root.go => apply_folder_permissions.go} | 16 ++--- ...st.go => apply_folder_permissions_test.go} | 4 +- ...tator.go => apply_resource_permissions.go} | 66 ++++++++++++++----- ....go => apply_resource_permissions_test.go} | 36 +++++++++- bundle/phases/deploy.go | 2 +- bundle/phases/initialize.go | 2 +- bundle/tests/bundle_permissions_test.go | 4 +- .../{{.project_name}}/databricks.yml.tmpl | 17 ++--- .../{{.project_name}}/databricks.yml.tmpl | 36 +++------- .../{{.project_name}}/databricks.yml.tmpl | 39 ++++------- 14 files changed, 162 insertions(+), 110 deletions(-) rename bundle/permissions/{workspace_root.go => apply_folder_permissions.go} (80%) rename bundle/permissions/{workspace_root_test.go => apply_folder_permissions_test.go} (93%) rename bundle/permissions/{mutator.go => apply_resource_permissions.go} (61%) rename bundle/permissions/{mutator_test.go => apply_resource_permissions_test.go} (84%) diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index d3de5728c9..4828b5f269 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -138,8 +138,15 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs } } - if !isPrincipalUsed && !isRunAsSet(r) { - return diag.Errorf("'run_as' must be set for all jobs when using 'mode: production'") + // We need to verify that there is only a single deployment of the current target. + // A good way to enforce this is to explicitly set root_path or run_as. + if !isExplicitRootSet(b) && !isRunAsSet(r) { + // Only show a warning in case a principal was used for backward compatibility + // with projects from before the DABs GA. + if isPrincipalUsed { + return diag.Warningf("target with 'mode: production' should specify explicit 'workspace.root_path' to make sure only one copy is deployed") + } + return diag.Errorf("target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") } return nil } @@ -156,6 +163,14 @@ func isRunAsSet(r config.Resources) bool { return true } +func isExplicitRootSet(b *bundle.Bundle) bool { + targetConfig := b.Config.Targets[b.Config.Bundle.Target] + if targetConfig.Workspace == nil { + return false + } + return targetConfig.Workspace.RootPath != "" +} + func (m *processTargetMode) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { switch b.Config.Bundle.Mode { case config.Development: diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 17f8381608..3affd6005d 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -31,6 +31,9 @@ func mockBundle(mode config.Mode) *bundle.Bundle { Branch: "main", }, }, + Targets: map[string]*config.Target{ + "": {}, + }, Workspace: config.Workspace{ CurrentUser: &config.User{ ShortName: "lennart", @@ -206,7 +209,7 @@ func TestProcessTargetModeProduction(t *testing.T) { b := mockBundle(config.Production) diags := validateProductionMode(context.Background(), b, false) - require.ErrorContains(t, diags.Error(), "run_as") + require.ErrorContains(t, diags.Error(), "production") b.Config.Workspace.StatePath = "/Shared/.bundle/x/y/state" b.Config.Workspace.ArtifactPath = "/Shared/.bundle/x/y/artifacts" @@ -254,6 +257,21 @@ func TestProcessTargetModeProductionOkForPrincipal(t *testing.T) { require.NoError(t, diags.Error()) } +func TestProcessTargetModeProductionOkWithRootPath(t *testing.T) { + b := mockBundle(config.Production) + + // Our target has all kinds of problems when not using service principals ... + diags := validateProductionMode(context.Background(), b, false) + require.Error(t, diags.Error()) + + // ... but we're okay if we specify a root path + b.Config.Targets[""].Workspace = &config.Workspace{ + RootPath: "some-root-path", + } + diags = validateProductionMode(context.Background(), b, false) + require.NoError(t, diags.Error()) +} + // Make sure that we have test coverage for all resource types func TestAllResourcesMocked(t *testing.T) { b := mockBundle(config.Development) diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index d6fb2939f6..9805bd11c1 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -178,11 +178,6 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { Config: *r, } diags := bundle.Apply(context.Background(), b, SetRunAs()) - assert.Equal(t, diags.Error().Error(), errUnsupportedResourceTypeForRunAs{ - resourceType: rt, - resourceLocation: dyn.Location{}, - currentUser: "alice", - runAsUser: "bob", - }.Error(), "expected run_as with a different identity than the current deployment user to not supported for resources of type: %s", rt) + assert.ErrorContains(t, diags.Error(), "support") } } diff --git a/bundle/config/mutator/select_target.go b/bundle/config/mutator/select_target.go index 178686b6ed..2333c97dbc 100644 --- a/bundle/config/mutator/select_target.go +++ b/bundle/config/mutator/select_target.go @@ -49,9 +49,5 @@ func (m *selectTarget) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnosti // TODO: remove when Environments section is not supported anymore. b.Config.Bundle.Environment = b.Config.Bundle.Target - // Clear targets after loading. - b.Config.Targets = nil - b.Config.Environments = nil - return nil } diff --git a/bundle/permissions/workspace_root.go b/bundle/permissions/apply_folder_permissions.go similarity index 80% rename from bundle/permissions/workspace_root.go rename to bundle/permissions/apply_folder_permissions.go index a59a039f6f..b9baf0d446 100644 --- a/bundle/permissions/workspace_root.go +++ b/bundle/permissions/apply_folder_permissions.go @@ -9,15 +9,15 @@ import ( "github.com/databricks/databricks-sdk-go/service/workspace" ) -type workspaceRootPermissions struct { +type applyFolderPermissions struct { } -func ApplyWorkspaceRootPermissions() bundle.Mutator { - return &workspaceRootPermissions{} +func ApplyFolderPermissions() bundle.Mutator { + return &applyFolderPermissions{} } // Apply implements bundle.Mutator. -func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { +func (*applyFolderPermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { err := giveAccessForWorkspaceRoot(ctx, b) if err != nil { return diag.FromErr(err) @@ -26,8 +26,8 @@ func (*workspaceRootPermissions) Apply(ctx context.Context, b *bundle.Bundle) di return nil } -func (*workspaceRootPermissions) Name() string { - return "ApplyWorkspaceRootPermissions" +func (*applyFolderPermissions) Name() string { + return "ApplyFolderPermissions" } func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { @@ -57,7 +57,7 @@ func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { return err } - _, err = w.UpdatePermissions(ctx, workspace.WorkspaceObjectPermissionsRequest{ + _, err = w.SetPermissions(ctx, workspace.WorkspaceObjectPermissionsRequest{ WorkspaceObjectId: fmt.Sprint(obj.ObjectId), WorkspaceObjectType: "directories", AccessControlList: permissions, @@ -67,7 +67,7 @@ func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { func getWorkspaceObjectPermissionLevel(bundlePermission string) (workspace.WorkspaceObjectPermissionLevel, error) { switch bundlePermission { - case CAN_MANAGE: + case CAN_MANAGE, IS_OWNER: return workspace.WorkspaceObjectPermissionLevelCanManage, nil case CAN_RUN: return workspace.WorkspaceObjectPermissionLevelCanRun, nil diff --git a/bundle/permissions/workspace_root_test.go b/bundle/permissions/apply_folder_permissions_test.go similarity index 93% rename from bundle/permissions/workspace_root_test.go rename to bundle/permissions/apply_folder_permissions_test.go index 7dd97b62d2..95897a1520 100644 --- a/bundle/permissions/workspace_root_test.go +++ b/bundle/permissions/apply_folder_permissions_test.go @@ -59,7 +59,7 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) { workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/Users/foo@bar.com").Return(&workspace.ObjectInfo{ ObjectId: 1234, }, nil) - workspaceApi.EXPECT().UpdatePermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ + workspaceApi.EXPECT().SetPermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ {UserName: "TestUser", PermissionLevel: "CAN_MANAGE"}, {GroupName: "TestGroup", PermissionLevel: "CAN_READ"}, @@ -69,6 +69,6 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) { WorkspaceObjectType: "directories", }).Return(nil, nil) - diags := bundle.Apply(context.Background(), b, ApplyWorkspaceRootPermissions()) + diags := bundle.Apply(context.Background(), b, ApplyFolderPermissions()) require.NoError(t, diags.Error()) } diff --git a/bundle/permissions/mutator.go b/bundle/permissions/apply_resource_permissions.go similarity index 61% rename from bundle/permissions/mutator.go rename to bundle/permissions/apply_resource_permissions.go index 7787bc0481..91ca0dda6d 100644 --- a/bundle/permissions/mutator.go +++ b/bundle/permissions/apply_resource_permissions.go @@ -7,47 +7,54 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/diag" ) const CAN_MANAGE = "CAN_MANAGE" const CAN_VIEW = "CAN_VIEW" const CAN_RUN = "CAN_RUN" +const IS_OWNER = "IS_OWNER" -var allowedLevels = []string{CAN_MANAGE, CAN_VIEW, CAN_RUN} +var allowedLevels = []string{CAN_MANAGE, CAN_VIEW, CAN_RUN, IS_OWNER} var levelsMap = map[string](map[string]string){ "jobs": { + IS_OWNER: "IS_OWNER", CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_VIEW", CAN_RUN: "CAN_MANAGE_RUN", }, "pipelines": { + IS_OWNER: "IS_OWNER", CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_VIEW", CAN_RUN: "CAN_RUN", }, "mlflow_experiments": { + IS_OWNER: "IS_OWNER", CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_READ", }, "mlflow_models": { + IS_OWNER: "IS_OWNER", CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_READ", }, "model_serving_endpoints": { + IS_OWNER: "IS_OWNER", CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_VIEW", CAN_RUN: "CAN_QUERY", }, } -type bundlePermissions struct{} +type applyResourcePermissions struct{} -func ApplyBundlePermissions() bundle.Mutator { - return &bundlePermissions{} +func ApplyResourcePermissions() bundle.Mutator { + return &applyResourcePermissions{} } -func (m *bundlePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { +func (m *applyResourcePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { err := validate(b) if err != nil { return diag.FromErr(err) @@ -74,64 +81,89 @@ func validate(b *bundle.Bundle) error { func applyForJobs(ctx context.Context, b *bundle.Bundle) { for key, job := range b.Config.Resources.Jobs { - job.Permissions = append(job.Permissions, convert( + job.Permissions = extendPermissions(job.Permissions, convert( ctx, b.Config.Permissions, job.Permissions, key, levelsMap["jobs"], - )...) + )) } } func applyForPipelines(ctx context.Context, b *bundle.Bundle) { for key, pipeline := range b.Config.Resources.Pipelines { - pipeline.Permissions = append(pipeline.Permissions, convert( + pipeline.Permissions = extendPermissions(pipeline.Permissions, convert( ctx, b.Config.Permissions, pipeline.Permissions, key, levelsMap["pipelines"], - )...) + )) } } func applyForMlExperiments(ctx context.Context, b *bundle.Bundle) { for key, experiment := range b.Config.Resources.Experiments { - experiment.Permissions = append(experiment.Permissions, convert( + experiment.Permissions = extendPermissions(experiment.Permissions, convert( ctx, b.Config.Permissions, experiment.Permissions, key, levelsMap["mlflow_experiments"], - )...) + )) } } func applyForMlModels(ctx context.Context, b *bundle.Bundle) { for key, model := range b.Config.Resources.Models { - model.Permissions = append(model.Permissions, convert( + model.Permissions = extendPermissions(model.Permissions, convert( ctx, b.Config.Permissions, model.Permissions, key, levelsMap["mlflow_models"], - )...) + )) } } func applyForModelServiceEndpoints(ctx context.Context, b *bundle.Bundle) { for key, model := range b.Config.Resources.ModelServingEndpoints { - model.Permissions = append(model.Permissions, convert( + model.Permissions = extendPermissions(model.Permissions, convert( ctx, b.Config.Permissions, model.Permissions, key, levelsMap["model_serving_endpoints"], - )...) + )) } } -func (m *bundlePermissions) Name() string { - return "ApplyBundlePermissions" +func extendPermissions(permissions []resources.Permission, newPermissions []resources.Permission) []resources.Permission { + if !includesOwner(permissions) { + return append(permissions, newPermissions...) + } + + // Make sure we don't add a second owner + for _, p := range newPermissions { + if p.Level == IS_OWNER { + continue + } + permissions = append(permissions, p) + } + + return permissions +} + +func includesOwner(permissions []resources.Permission) bool { + for _, p := range permissions { + if p.Level == IS_OWNER { + return true + } + } + return false +} + +func (m *applyResourcePermissions) Name() string { + return "ApplyResourcePermissions" } diff --git a/bundle/permissions/mutator_test.go b/bundle/permissions/apply_resource_permissions_test.go similarity index 84% rename from bundle/permissions/mutator_test.go rename to bundle/permissions/apply_resource_permissions_test.go index 438a150615..2661315c95 100644 --- a/bundle/permissions/mutator_test.go +++ b/bundle/permissions/apply_resource_permissions_test.go @@ -46,7 +46,7 @@ func TestApplyBundlePermissions(t *testing.T) { }, } - diags := bundle.Apply(context.Background(), b, ApplyBundlePermissions()) + diags := bundle.Apply(context.Background(), b, ApplyResourcePermissions()) require.NoError(t, diags.Error()) require.Len(t, b.Config.Resources.Jobs["job_1"].Permissions, 3) @@ -123,7 +123,7 @@ func TestWarningOnOverlapPermission(t *testing.T) { }, } - diags := bundle.Apply(context.Background(), b, ApplyBundlePermissions()) + diags := bundle.Apply(context.Background(), b, ApplyResourcePermissions()) require.NoError(t, diags.Error()) require.Contains(t, b.Config.Resources.Jobs["job_1"].Permissions, resources.Permission{Level: "CAN_VIEW", UserName: "TestUser"}) @@ -133,3 +133,35 @@ func TestWarningOnOverlapPermission(t *testing.T) { require.Contains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.Permission{Level: "CAN_VIEW", GroupName: "TestGroup"}) } + +func TestOwnerLevel(t *testing.T) { + // Create a bundle with a run_as configuration + b := &bundle.Bundle{ + Config: config.Root{ + Permissions: []resources.Permission{ + {Level: IS_OWNER, UserName: "Alice"}, + }, + Resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "job_1": { + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, UserName: "Bob"}, + }, + }, + "job_2": { + Permissions: []resources.Permission{ + {Level: IS_OWNER, UserName: "Bob"}, + }, + }, + }, + }, + }, + } + + diags := bundle.Apply(context.Background(), b, ApplyResourcePermissions()) + require.NoError(t, diags.Error()) + require.Contains(t, b.Config.Resources.Jobs["job_1"].Permissions, resources.Permission{Level: "CAN_MANAGE", UserName: "Bob"}) + require.Contains(t, b.Config.Resources.Jobs["job_1"].Permissions, resources.Permission{Level: "IS_OWNER", UserName: "Alice"}) + require.Contains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.Permission{Level: "IS_OWNER", UserName: "Bob"}) + require.NotContains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.Permission{Level: "IS_OWNER", UserName: "Alice"}) +} diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index de94c5a0e7..8048f3d43a 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -33,7 +33,7 @@ func Deploy() bundle.Mutator { files.Upload(), deploy.StateUpdate(), deploy.StatePush(), - permissions.ApplyWorkspaceRootPermissions(), + permissions.ApplyFolderPermissions(), terraform.Interpolate(), terraform.Write(), deploy.CheckRunningResource(), diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index 6761ffabc6..d9ad0ed2be 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -34,13 +34,13 @@ func Initialize() bundle.Mutator { "workspace", "variables", ), + permissions.ApplyResourcePermissions(), mutator.SetRunAs(), mutator.OverrideCompute(), mutator.ProcessTargetMode(), mutator.ExpandPipelineGlobPaths(), mutator.TranslatePaths(), python.WrapperWarning(), - permissions.ApplyBundlePermissions(), permissions.FilterCurrentUser(), metadata.AnnotateJobs(), terraform.Initialize(), diff --git a/bundle/tests/bundle_permissions_test.go b/bundle/tests/bundle_permissions_test.go index b55cbdd2b2..3c9a07aa68 100644 --- a/bundle/tests/bundle_permissions_test.go +++ b/bundle/tests/bundle_permissions_test.go @@ -18,7 +18,7 @@ func TestBundlePermissions(t *testing.T) { assert.NotContains(t, b.Config.Permissions, resources.Permission{Level: "CAN_VIEW", ServicePrincipalName: "1234-abcd"}) assert.NotContains(t, b.Config.Permissions, resources.Permission{Level: "CAN_RUN", UserName: "bot@company.com"}) - diags := bundle.Apply(context.Background(), b, permissions.ApplyBundlePermissions()) + diags := bundle.Apply(context.Background(), b, permissions.ApplyResourcePermissions()) require.NoError(t, diags.Error()) pipelinePermissions := b.Config.Resources.Pipelines["nyc_taxi_pipeline"].Permissions @@ -41,7 +41,7 @@ func TestBundlePermissionsDevTarget(t *testing.T) { assert.Contains(t, b.Config.Permissions, resources.Permission{Level: "CAN_VIEW", ServicePrincipalName: "1234-abcd"}) assert.Contains(t, b.Config.Permissions, resources.Permission{Level: "CAN_RUN", UserName: "bot@company.com"}) - diags := bundle.Apply(context.Background(), b, permissions.ApplyBundlePermissions()) + diags := bundle.Apply(context.Background(), b, permissions.ApplyResourcePermissions()) require.NoError(t, diags.Error()) pipelinePermissions := b.Config.Resources.Pipelines["nyc_taxi_pipeline"].Permissions diff --git a/libs/template/templates/dbt-sql/template/{{.project_name}}/databricks.yml.tmpl b/libs/template/templates/dbt-sql/template/{{.project_name}}/databricks.yml.tmpl index fdda03c0d0..e82c3d1b5d 100644 --- a/libs/template/templates/dbt-sql/template/{{.project_name}}/databricks.yml.tmpl +++ b/libs/template/templates/dbt-sql/template/{{.project_name}}/databricks.yml.tmpl @@ -12,8 +12,10 @@ include: targets: dev: default: true - # We use 'mode: development' to indicate this is a personal development copy. - # Any job schedules and triggers are paused by default. + # The default target uses 'mode: development' to create a development copy. + # - Deployed resources get prefixed with '[dev my_user_name]' + # - Any job schedules and triggers are paused by default. + # See also https://docs.databricks.com/dev-tools/bundles/deployment-modes.html. mode: development workspace: host: {{workspace_host}} @@ -22,11 +24,10 @@ targets: mode: production workspace: host: {{workspace_host}} - # We always use /Users/{{user_name}} for all resources to make sure we only have a single copy. + # We explicitly specify /Users/{{user_name}} to make sure we only have a single copy. root_path: /Users/{{user_name}}/.bundle/${bundle.name}/${bundle.target} - {{- if not is_service_principal}} + permissions: + - {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}} + level: IS_OWNER run_as: - # This runs as {{user_name}} in production. We could also use a service principal here - # using service_principal_name (see the Databricks documentation). - user_name: {{user_name}} - {{- end}} + {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}} diff --git a/libs/template/templates/default-python/template/{{.project_name}}/databricks.yml.tmpl b/libs/template/templates/default-python/template/{{.project_name}}/databricks.yml.tmpl index e3572326b6..d44728e602 100644 --- a/libs/template/templates/default-python/template/{{.project_name}}/databricks.yml.tmpl +++ b/libs/template/templates/default-python/template/{{.project_name}}/databricks.yml.tmpl @@ -7,44 +7,24 @@ include: - resources/*.yml targets: - # The 'dev' target, for development purposes. This target is the default. dev: - # We use 'mode: development' to indicate this is a personal development copy: + # The default target uses 'mode: development' to create a development copy. # - Deployed resources get prefixed with '[dev my_user_name]' - # - Any job schedules and triggers are paused by default - # - The 'development' mode is used for Delta Live Tables pipelines + # - Any job schedules and triggers are paused by default. + # See also https://docs.databricks.com/dev-tools/bundles/deployment-modes.html. mode: development default: true workspace: host: {{workspace_host}} - ## Optionally, there could be a 'staging' target here. - ## (See Databricks docs on CI/CD at https://docs.databricks.com/dev-tools/bundles/ci-cd.html.) - # - # staging: - # workspace: - # host: {{workspace_host}} - - # The 'prod' target, used for production deployment. prod: - # We use 'mode: production' to indicate this is a production deployment. - # Doing so enables strict verification of the settings below. mode: production workspace: host: {{workspace_host}} - # We always use /Users/{{user_name}} for all resources to make sure we only have a single copy. - {{- /* - Internal note 2023-12: CLI versions v0.211.0 and before would show an error when using `mode: production` - with a path that doesn't say "/Shared". For now, we'll include an extra comment in the template - to explain that customers should update if they see this. - */}} - # If this path results in an error, please make sure you have a recent version of the CLI installed. + # We explicitly specify /Users/{{user_name}} to make sure we only have a single copy. root_path: /Users/{{user_name}}/.bundle/${bundle.name}/${bundle.target} + permissions: + - {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}} + level: IS_OWNER run_as: - {{- if is_service_principal}} - service_principal_name: {{user_name}} - {{- else}} - # This runs as {{user_name}} in production. We could also use a service principal here, - # see https://docs.databricks.com/dev-tools/bundles/permissions.html. - user_name: {{user_name}} - {{- end}} + {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}} diff --git a/libs/template/templates/default-sql/template/{{.project_name}}/databricks.yml.tmpl b/libs/template/templates/default-sql/template/{{.project_name}}/databricks.yml.tmpl index a47fb7c199..d2697185af 100644 --- a/libs/template/templates/default-sql/template/{{.project_name}}/databricks.yml.tmpl +++ b/libs/template/templates/default-sql/template/{{.project_name}}/databricks.yml.tmpl @@ -18,16 +18,16 @@ variables: {{- $dev_schema := .shared_schema }} {{- $prod_schema := .shared_schema }} {{- if (regexp "^yes").MatchString .personal_schemas}} -{{- $dev_schema = "${workspace.current_user.short_name}"}} -{{- $prod_schema = "default"}} + {{- $dev_schema = "${workspace.current_user.short_name}"}} + {{- $prod_schema = "default"}} {{- end}} -# Deployment targets. targets: - # The 'dev' target, for development purposes. This target is the default. dev: - # We use 'mode: development' to indicate this is a personal development copy. - # Any job schedules and triggers are paused by default + # The default target uses 'mode: development' to create a development copy. + # - Deployed resources get prefixed with '[dev my_user_name]' + # - Any job schedules and triggers are paused by default. + # See also https://docs.databricks.com/dev-tools/bundles/deployment-modes.html. mode: development default: true workspace: @@ -37,35 +37,18 @@ targets: catalog: {{.default_catalog}} schema: {{$dev_schema}} - ## Optionally, there could be a 'staging' target here. - ## (See Databricks docs on CI/CD at https://docs.databricks.com/dev-tools/bundles/ci-cd.html.) - # - # staging: - # workspace: - # host: {{workspace_host}} - - # The 'prod' target, used for production deployment. prod: - # We use 'mode: production' to indicate this is a production deployment. - # Doing so enables strict verification of the settings below. mode: production workspace: host: {{workspace_host}} - # We always use /Users/{{user_name}} for all resources to make sure we only have a single copy. - {{- /* - Internal note 2023-12: CLI versions v0.211.0 and before would show an error when using `mode: production` - with a path that doesn't say "/Shared". For now, we'll include an extra comment in the template - to explain that customers should update if they see this. - */}} - # If this path results in an error, please make sure you have a recent version of the CLI installed. + # We explicitly specify /Users/{{user_name}} to make sure we only have a single copy. root_path: /Users/{{user_name}}/.bundle/${bundle.name}/${bundle.target} variables: warehouse_id: {{index ((regexp "[^/]+$").FindStringSubmatch .http_path) 0}} catalog: {{.default_catalog}} schema: {{$prod_schema}} - {{- if not is_service_principal}} + permissions: + - {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}} + level: IS_OWNER run_as: - # This runs as {{user_name}} in production. We could also use a service principal here - # using service_principal_name (see https://docs.databricks.com/en/dev-tools/bundles/permissions.html). - user_name: {{user_name}} - {{end -}} + {{if is_service_principal}}service_principal{{else}}user{{end}}_name: {{user_name}} From 86b7a851bbbf60db159cbdcc690df42ce596816e Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 22 Apr 2024 15:42:35 +0200 Subject: [PATCH 2/8] WIP --- bundle/config/mutator/run_as.go | 1 + bundle/config/mutator/run_as_test.go | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 578591eb14..cd7dbce7cd 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -81,6 +81,7 @@ func validateRunAs(b *bundle.Bundle) error { } // DLT pipelines do not support run_as in the API. + // TODO: maybe oly make this check only fail when owner != runas if len(b.Config.Resources.Pipelines) > 0 { return errUnsupportedResourceTypeForRunAs{ resourceType: "pipelines", diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index 9805bd11c1..d6fb2939f6 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -178,6 +178,11 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { Config: *r, } diags := bundle.Apply(context.Background(), b, SetRunAs()) - assert.ErrorContains(t, diags.Error(), "support") + assert.Equal(t, diags.Error().Error(), errUnsupportedResourceTypeForRunAs{ + resourceType: rt, + resourceLocation: dyn.Location{}, + currentUser: "alice", + runAsUser: "bob", + }.Error(), "expected run_as with a different identity than the current deployment user to not supported for resources of type: %s", rt) } } From 6f0df284861ce25b078c0c37a0eab3756e13a373 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 27 May 2024 21:04:53 +0200 Subject: [PATCH 3/8] Fix permissions levels --- .../permissions/apply_resource_permissions.go | 6 +++--- .../apply_resource_permissions_test.go | 21 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/bundle/permissions/apply_resource_permissions.go b/bundle/permissions/apply_resource_permissions.go index 91ca0dda6d..ee0988f644 100644 --- a/bundle/permissions/apply_resource_permissions.go +++ b/bundle/permissions/apply_resource_permissions.go @@ -31,17 +31,17 @@ var levelsMap = map[string](map[string]string){ CAN_RUN: "CAN_RUN", }, "mlflow_experiments": { - IS_OWNER: "IS_OWNER", + IS_OWNER: "CAN_MANAGE", CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_READ", }, "mlflow_models": { - IS_OWNER: "IS_OWNER", + IS_OWNER: "CAN_MANAGE", CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_READ", }, "model_serving_endpoints": { - IS_OWNER: "IS_OWNER", + IS_OWNER: "CAN_MANAGE", CAN_MANAGE: "CAN_MANAGE", CAN_VIEW: "CAN_VIEW", CAN_RUN: "CAN_QUERY", diff --git a/bundle/permissions/apply_resource_permissions_test.go b/bundle/permissions/apply_resource_permissions_test.go index 2661315c95..06ebe75c1b 100644 --- a/bundle/permissions/apply_resource_permissions_test.go +++ b/bundle/permissions/apply_resource_permissions_test.go @@ -154,6 +154,22 @@ func TestOwnerLevel(t *testing.T) { }, }, }, + Pipelines: map[string]*resources.Pipeline{ + "pipeline_1": {}, + }, + Models: map[string]*resources.MlflowModel{ + "model_1": { + Permissions: []resources.Permission{ + {Level: CAN_MANAGE, UserName: "Bob"}, + }, + }, + }, + Experiments: map[string]*resources.MlflowExperiment{ + "experiment_1": {}, + }, + ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ + "endpoint_1": {}, + }, }, }, } @@ -164,4 +180,9 @@ func TestOwnerLevel(t *testing.T) { require.Contains(t, b.Config.Resources.Jobs["job_1"].Permissions, resources.Permission{Level: "IS_OWNER", UserName: "Alice"}) require.Contains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.Permission{Level: "IS_OWNER", UserName: "Bob"}) require.NotContains(t, b.Config.Resources.Jobs["job_2"].Permissions, resources.Permission{Level: "IS_OWNER", UserName: "Alice"}) + require.Contains(t, b.Config.Resources.Pipelines["pipeline_1"].Permissions, resources.Permission{Level: "IS_OWNER", UserName: "Alice"}) + require.Contains(t, b.Config.Resources.Experiments["experiment_1"].Permissions, resources.Permission{Level: "CAN_MANAGE", UserName: "Alice"}) + require.Contains(t, b.Config.Resources.Models["model_1"].Permissions, resources.Permission{Level: "CAN_MANAGE", UserName: "Alice"}) + require.Contains(t, b.Config.Resources.Models["model_1"].Permissions, resources.Permission{Level: "CAN_MANAGE", UserName: "Bob"}) + require.Contains(t, b.Config.Resources.ModelServingEndpoints["endpoint_1"].Permissions, resources.Permission{Level: "CAN_MANAGE", UserName: "Alice"}) } From e0fd22afaa6ec6a3d812be41d802f3c334a5cf43 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 27 May 2024 21:08:26 +0200 Subject: [PATCH 4/8] Restore original semantics for now --- bundle/permissions/apply_folder_permissions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/permissions/apply_folder_permissions.go b/bundle/permissions/apply_folder_permissions.go index b9baf0d446..ae285dc2cb 100644 --- a/bundle/permissions/apply_folder_permissions.go +++ b/bundle/permissions/apply_folder_permissions.go @@ -57,7 +57,7 @@ func giveAccessForWorkspaceRoot(ctx context.Context, b *bundle.Bundle) error { return err } - _, err = w.SetPermissions(ctx, workspace.WorkspaceObjectPermissionsRequest{ + _, err = w.UpdatePermissions(ctx, workspace.WorkspaceObjectPermissionsRequest{ WorkspaceObjectId: fmt.Sprint(obj.ObjectId), WorkspaceObjectType: "directories", AccessControlList: permissions, From ab04468a99326309662b96eabde27eb56eef5cb1 Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sun, 2 Jun 2024 15:09:58 +0200 Subject: [PATCH 5/8] Cleanup --- bundle/config/mutator/process_target_mode_test.go | 4 ++-- bundle/permissions/apply_folder_permissions_test.go | 2 +- bundle/permissions/apply_resource_permissions.go | 4 +++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/bundle/config/mutator/process_target_mode_test.go b/bundle/config/mutator/process_target_mode_test.go index 67ea22ce97..5ad7f6e585 100644 --- a/bundle/config/mutator/process_target_mode_test.go +++ b/bundle/config/mutator/process_target_mode_test.go @@ -216,14 +216,14 @@ func TestProcessTargetModeProduction(t *testing.T) { b := mockBundle(config.Production) diags := validateProductionMode(context.Background(), b, false) - require.ErrorContains(t, diags.Error(), "production") + require.ErrorContains(t, diags.Error(), "target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") b.Config.Workspace.StatePath = "/Shared/.bundle/x/y/state" b.Config.Workspace.ArtifactPath = "/Shared/.bundle/x/y/artifacts" b.Config.Workspace.FilePath = "/Shared/.bundle/x/y/files" diags = validateProductionMode(context.Background(), b, false) - require.ErrorContains(t, diags.Error(), "production") + require.ErrorContains(t, diags.Error(), "target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") permissions := []resources.Permission{ { diff --git a/bundle/permissions/apply_folder_permissions_test.go b/bundle/permissions/apply_folder_permissions_test.go index 29f696c267..08e7440e19 100644 --- a/bundle/permissions/apply_folder_permissions_test.go +++ b/bundle/permissions/apply_folder_permissions_test.go @@ -59,7 +59,7 @@ func TestApplyWorkspaceRootPermissions(t *testing.T) { workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/Users/foo@bar.com").Return(&workspace.ObjectInfo{ ObjectId: 1234, }, nil) - workspaceApi.EXPECT().SetPermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ + workspaceApi.EXPECT().UpdatePermissions(mock.Anything, workspace.WorkspaceObjectPermissionsRequest{ AccessControlList: []workspace.WorkspaceObjectAccessControlRequest{ {UserName: "TestUser", PermissionLevel: "CAN_MANAGE"}, {GroupName: "TestGroup", PermissionLevel: "CAN_READ"}, diff --git a/bundle/permissions/apply_resource_permissions.go b/bundle/permissions/apply_resource_permissions.go index ee0988f644..7f3a299c8c 100644 --- a/bundle/permissions/apply_resource_permissions.go +++ b/bundle/permissions/apply_resource_permissions.go @@ -141,10 +141,12 @@ func applyForModelServiceEndpoints(ctx context.Context, b *bundle.Bundle) { func extendPermissions(permissions []resources.Permission, newPermissions []resources.Permission) []resources.Permission { if !includesOwner(permissions) { + // Just merge the permissions, don't need to do anything special return append(permissions, newPermissions...) } - // Make sure we don't add a second owner + // Only apply the owner from top-level permissions if the local resource + // didn't have an owner. for _, p := range newPermissions { if p.Level == IS_OWNER { continue From e4df0b9fa89681b143eff3d6fd7638f97bd80bef Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 3 Jun 2024 08:59:26 +0200 Subject: [PATCH 6/8] Fix test failure caused by changes in main --- bundle/permissions/apply_resource_permissions_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bundle/permissions/apply_resource_permissions_test.go b/bundle/permissions/apply_resource_permissions_test.go index 315656c2ae..58ea1391e8 100644 --- a/bundle/permissions/apply_resource_permissions_test.go +++ b/bundle/permissions/apply_resource_permissions_test.go @@ -162,11 +162,17 @@ func TestOwnerLevel(t *testing.T) { Permissions: []resources.Permission{ {Level: CAN_MANAGE, UserName: "Bob"}, }, + JobSettings: &jobs.JobSettings{ + Name: "job_1", + }, }, "job_2": { Permissions: []resources.Permission{ {Level: IS_OWNER, UserName: "Bob"}, }, + JobSettings: &jobs.JobSettings{ + Name: "job_1", + }, }, }, Pipelines: map[string]*resources.Pipeline{ From ef6a43e6e5a1361b2e3433476dbffcfa54efef7a Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Sat, 27 Jul 2024 20:49:37 +0100 Subject: [PATCH 7/8] Process PR feedback --- bundle/config/mutator/cleanup_targets.go | 29 ++++++++++++++++++++ bundle/config/mutator/process_target_mode.go | 12 ++++---- bundle/config/mutator/run_as.go | 1 - cmd/bundle/summary.go | 4 +-- cmd/bundle/validate.go | 2 ++ 5 files changed, 40 insertions(+), 8 deletions(-) create mode 100644 bundle/config/mutator/cleanup_targets.go diff --git a/bundle/config/mutator/cleanup_targets.go b/bundle/config/mutator/cleanup_targets.go new file mode 100644 index 0000000000..870b5be90c --- /dev/null +++ b/bundle/config/mutator/cleanup_targets.go @@ -0,0 +1,29 @@ +package mutator + +import ( + "context" + "fmt" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/diag" +) + +type cleanupTargets struct { + name string +} + +// CleanupTargets cleans up configuration properties before the configuration +// is reported by the 'bundle summary' command. +func CleanupTargets() bundle.Mutator { + return &cleanupTargets{} +} + +func (m *cleanupTargets) Name() string { + return fmt.Sprintf("Cleanup(%s)", m.name) +} + +func (m *cleanupTargets) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + b.Config.Targets = nil + b.Config.Environments = nil + return nil +} diff --git a/bundle/config/mutator/process_target_mode.go b/bundle/config/mutator/process_target_mode.go index 89a5929e27..ee4b6abaa2 100644 --- a/bundle/config/mutator/process_target_mode.go +++ b/bundle/config/mutator/process_target_mode.go @@ -154,11 +154,13 @@ func validateProductionMode(ctx context.Context, b *bundle.Bundle, isPrincipalUs } // We need to verify that there is only a single deployment of the current target. - // A good way to enforce this is to explicitly set root_path or run_as. - if !isExplicitRootSet(b) && !isRunAsSet(r) { - // Only show a warning in case a principal was used for backward compatibility - // with projects from before the DABs GA. - if isPrincipalUsed { + // The best way to enforce this is to explicitly set root_path. + if !isExplicitRootSet(b) { + if isRunAsSet(r) || isPrincipalUsed { + // Just setting run_as is not enough to guarantee a single deployment, + // and neither is setting a principal. + // We only show a warning for these cases since we didn't historically + // report an error for them. return diag.Warningf("target with 'mode: production' should specify explicit 'workspace.root_path' to make sure only one copy is deployed") } return diag.Errorf("target with 'mode: production' must specify explicit 'workspace.root_path' to make sure only one copy is deployed") diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 14bf629faf..d344a988ae 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -87,7 +87,6 @@ func validateRunAs(b *bundle.Bundle) error { } // DLT pipelines do not support run_as in the API. - // TODO: maybe oly make this check only fail when owner != runas if len(b.Config.Resources.Pipelines) > 0 { return errUnsupportedResourceTypeForRunAs{ resourceType: "pipelines", diff --git a/cmd/bundle/summary.go b/cmd/bundle/summary.go index 5a64b46c0a..c8a16b8680 100644 --- a/cmd/bundle/summary.go +++ b/cmd/bundle/summary.go @@ -8,6 +8,7 @@ import ( "path/filepath" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/deploy/terraform" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" @@ -60,7 +61,7 @@ func newSummaryCommand() *cobra.Command { } } - diags = bundle.Apply(ctx, b, terraform.Load()) + diags = bundle.Apply(ctx, b, bundle.Seq(terraform.Load(), mutator.CleanupTargets())) if err := diags.Error(); err != nil { return err } @@ -68,7 +69,6 @@ func newSummaryCommand() *cobra.Command { switch root.OutputType(cmd) { case flags.OutputText: return fmt.Errorf("%w, only json output is supported", errors.ErrUnsupported) - case flags.OutputJSON: buf, err := json.MarshalIndent(b.Config, "", " ") if err != nil { return err diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 59a9770476..f193a073cc 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/config/validate" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/bundle/render" @@ -64,6 +65,7 @@ func newValidateCommand() *cobra.Command { return nil case flags.OutputJSON: + bundle.Apply(ctx, b, mutator.CleanupTargets()) return renderJsonOutput(cmd, b, diags) default: return fmt.Errorf("unknown output type %s", root.OutputType(cmd)) From 4df6fdae5c16b1cbbe0ca73044a0828f6fc184be Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Mon, 29 Jul 2024 20:37:14 +0100 Subject: [PATCH 8/8] Clarify code --- bundle/permissions/apply_resource_permissions.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bundle/permissions/apply_resource_permissions.go b/bundle/permissions/apply_resource_permissions.go index 7f3a299c8c..9ece3706c9 100644 --- a/bundle/permissions/apply_resource_permissions.go +++ b/bundle/permissions/apply_resource_permissions.go @@ -139,16 +139,15 @@ func applyForModelServiceEndpoints(ctx context.Context, b *bundle.Bundle) { } } -func extendPermissions(permissions []resources.Permission, newPermissions []resources.Permission) []resources.Permission { +func extendPermissions(permissions []resources.Permission, topLevelPermissions []resources.Permission) []resources.Permission { if !includesOwner(permissions) { // Just merge the permissions, don't need to do anything special - return append(permissions, newPermissions...) + return append(permissions, topLevelPermissions...) } - // Only apply the owner from top-level permissions if the local resource - // didn't have an owner. - for _, p := range newPermissions { + for _, p := range topLevelPermissions { if p.Level == IS_OWNER { + // Prefer the owner from the local permission set continue } permissions = append(permissions, p)