diff --git a/acceptance/bundle/apps/job_permissions/output.txt b/acceptance/bundle/apps/job_permissions/output.txt index 5e678040d1..d64fd16fd0 100644 --- a/acceptance/bundle/apps/job_permissions/output.txt +++ b/acceptance/bundle/apps/job_permissions/output.txt @@ -1,5 +1,19 @@ >>> [CLI] bundle deploy +Warning: app "my_app" references jobs "my_job" which has permissions set. To prevent permission override after deploying the app, please add the app service principal to the jobs permissions + at resources.apps.my_app + in databricks.yml:24:7 + +Add the following section to the jobs permissions: + + resources: + jobs: + my_job: + permissions: + - level: CAN_MANAGE_RUN + service_principal_name: ${resources.apps.my_app.service_principal_client_id} + + Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... Deploying resources... Updating deployment state... @@ -10,6 +24,20 @@ Deployment complete! === After second deploy >>> [CLI] bundle deploy +Warning: app "my_app" references jobs "my_job" which has permissions set. To prevent permission override after deploying the app, please add the app service principal to the jobs permissions + at resources.apps.my_app + in databricks.yml:24:7 + +Add the following section to the jobs permissions: + + resources: + jobs: + my_job: + permissions: + - level: CAN_MANAGE_RUN + service_principal_name: ${resources.apps.my_app.service_principal_client_id} + + Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/files... Deploying resources... Updating deployment state... diff --git a/acceptance/bundle/apps/job_permissions_warning/app/app.py b/acceptance/bundle/apps/job_permissions_warning/app/app.py new file mode 100644 index 0000000000..11b15b1a45 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions_warning/app/app.py @@ -0,0 +1 @@ +print("hello") diff --git a/acceptance/bundle/apps/job_permissions_warning/databricks.yml b/acceptance/bundle/apps/job_permissions_warning/databricks.yml new file mode 100644 index 0000000000..df8f4191df --- /dev/null +++ b/acceptance/bundle/apps/job_permissions_warning/databricks.yml @@ -0,0 +1,28 @@ +bundle: + name: test-bundle + +resources: + jobs: + my_job: + name: my-job + permissions: + - level: CAN_MANAGE + user_name: ${workspace.current_user.userName} + tasks: + - task_key: hello + spark_python_task: + python_file: ./hello.py + new_cluster: + num_workers: 1 + spark_version: 14.0.x-scala2.13 + node_type_id: i3.xlarge + + apps: + my_app: + name: myapp + 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_warning/hello.py b/acceptance/bundle/apps/job_permissions_warning/hello.py new file mode 100644 index 0000000000..11b15b1a45 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions_warning/hello.py @@ -0,0 +1 @@ +print("hello") diff --git a/acceptance/bundle/apps/job_permissions_warning/out.test.toml b/acceptance/bundle/apps/job_permissions_warning/out.test.toml new file mode 100644 index 0000000000..d560f1de04 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions_warning/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/apps/job_permissions_warning/output.txt b/acceptance/bundle/apps/job_permissions_warning/output.txt new file mode 100644 index 0000000000..e440a6c736 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions_warning/output.txt @@ -0,0 +1,23 @@ + +>>> [CLI] bundle validate +Warning: app "my_app" references jobs "my_job" which has permissions set. To prevent permission override after deploying the app, please add the app service principal to the jobs permissions + at resources.apps.my_app + in databricks.yml:22:7 + +Add the following section to the jobs permissions: + + resources: + jobs: + my_job: + permissions: + - level: CAN_MANAGE_RUN + service_principal_name: ${resources.apps.my_app.service_principal_client_id} + + +Name: test-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Found 1 warning diff --git a/acceptance/bundle/apps/job_permissions_warning/script b/acceptance/bundle/apps/job_permissions_warning/script new file mode 100644 index 0000000000..5350876150 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions_warning/script @@ -0,0 +1 @@ +trace $CLI bundle validate diff --git a/acceptance/bundle/apps/job_permissions_warning/test.toml b/acceptance/bundle/apps/job_permissions_warning/test.toml new file mode 100644 index 0000000000..a5b2fe2819 --- /dev/null +++ b/acceptance/bundle/apps/job_permissions_warning/test.toml @@ -0,0 +1,5 @@ +RecordRequests = false + +Ignore = [ + '.databricks', +] diff --git a/bundle/apps/validate.go b/bundle/apps/validate.go index 0bdced6161..6c6403e06f 100644 --- a/bundle/apps/validate.go +++ b/bundle/apps/validate.go @@ -3,11 +3,18 @@ package apps import ( "context" "fmt" + "regexp" "github.com/databricks/cli/bundle" + "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/apps" ) +// resourceReferencePattern matches ${resources...} variable references. +var resourceReferencePattern = regexp.MustCompile(`\$\{resources\.(\w+)\.([^.]+)\.\w+\}`) + type validate struct{} func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { @@ -44,6 +51,130 @@ func (v *validate) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics }) } usedSourceCodePaths[app.SourceCodePath] = key + + diags = diags.Extend(warnForAppResourcePermissions(b, key, app)) + } + + return diags +} + +// appResourceRef extracts resource references from an app resource entry. +// When resourceType is empty the reference matches any bundle resource type +// extracted from the variable reference pattern. +func appResourceRef(r apps.AppResource) (appResourceReference, bool) { + switch { + case r.Job != nil: + return appResourceReference{"jobs", r.Job.Id, string(r.Job.Permission)}, true + case r.SqlWarehouse != nil: + return appResourceReference{"sql_warehouses", r.SqlWarehouse.Id, string(r.SqlWarehouse.Permission)}, true + case r.ServingEndpoint != nil: + return appResourceReference{"model_serving_endpoints", r.ServingEndpoint.Name, string(r.ServingEndpoint.Permission)}, true + case r.Experiment != nil: + return appResourceReference{"experiments", r.Experiment.ExperimentId, string(r.Experiment.Permission)}, true + case r.Postgres != nil: + if r.Postgres.Branch != "" { + return appResourceReference{"postgres_projects", r.Postgres.Branch, string(r.Postgres.Permission)}, true + } + return appResourceReference{ + "database_instances", r.Postgres.Database, string(r.Postgres.Permission), + }, true + case r.UcSecurable != nil: + return appResourceReference{string(r.UcSecurable.SecurableType), r.UcSecurable.SecurableFullName, string(r.UcSecurable.Permission)}, true + default: + return appResourceReference{}, false + } +} + +type appResourceReference struct { + resourceType string + refValue string + permission string +} + +// hasPermissions checks if a bundle resource at the given dyn path has a non-empty permissions list. +func hasPermissions(b *bundle.Bundle, resourcePath string) bool { + pv, err := dyn.Get(b.Config.Value(), resourcePath+".permissions") + if err != nil { + return false + } + s, ok := pv.AsSequence() + return ok && len(s) > 0 +} + +// hasAppSPInPermissions checks if any permission entry for the given resource +// references the app's service principal via variable interpolation. +func hasAppSPInPermissions(b *bundle.Bundle, resourcePath, appKey string) bool { + appSPRef := fmt.Sprintf("${resources.apps.%s.service_principal_client_id}", appKey) + pv, err := dyn.Get(b.Config.Value(), resourcePath+".permissions") + if err != nil { + return false + } + s, ok := pv.AsSequence() + if !ok { + return false + } + for _, entry := range s { + spn, err := dyn.Get(entry, "service_principal_name") + if err != nil { + continue + } + if str, ok := spn.AsString(); ok && str == appSPRef { + return true + } + } + return false +} + +// warnForAppResourcePermissions warns when an app references a bundle resource +// that has explicit permissions but doesn't include the app's service principal. +// Without the SP in the permission list, the second deploy will overwrite the +// app-granted permission on the resource. +// See https://github.com/databricks/cli/issues/4309 +func warnForAppResourcePermissions(b *bundle.Bundle, appKey string, app *resources.App) diag.Diagnostics { + var diags diag.Diagnostics + + for _, ar := range app.Resources { + ref, ok := appResourceRef(ar) + if !ok { + continue + } + + matches := resourceReferencePattern.FindStringSubmatch(ref.refValue) + if len(matches) < 3 { + continue + } + refType, resourceKey := matches[1], matches[2] + + resourcePath := fmt.Sprintf("resources.%s.%s", refType, resourceKey) + if !hasPermissions(b, resourcePath) { + continue + } + + if hasAppSPInPermissions(b, resourcePath, appKey) { + continue + } + + appPath := "resources.apps." + appKey + diags = append(diags, diag.Diagnostic{ + Severity: diag.Warning, + Summary: fmt.Sprintf("app %q references %s %q which has permissions set. To prevent permission override after deploying the app, please add the app service principal to the %s permissions", appKey, refType, resourceKey, refType), + Detail: fmt.Sprintf( + "Add the following section to the %s permissions:\n\n"+ + " resources:\n"+ + " %s:\n"+ + " %s:\n"+ + " permissions:\n"+ + " - level: %s\n"+ + " service_principal_name: ${resources.apps.%s.service_principal_client_id}\n", + refType, + refType, + resourceKey, + ref.permission, + appKey, + ), + Paths: []dyn.Path{dyn.MustPathFromString(appPath)}, + Locations: b.Config.GetLocations(appPath), + }) } return diags diff --git a/bundle/apps/validate_test.go b/bundle/apps/validate_test.go index 5e89de1f1d..ffc4fde45e 100644 --- a/bundle/apps/validate_test.go +++ b/bundle/apps/validate_test.go @@ -10,6 +10,7 @@ import ( "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/bundle/internal/bundletest" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/vfs" "github.com/databricks/databricks-sdk-go/service/apps" @@ -55,6 +56,171 @@ func TestAppsValidateSameSourcePath(t *testing.T) { require.Contains(t, diags[0].Detail, "has the same source code path as app resource") } +func TestAppsValidateResourcePermissionsWarning(t *testing.T) { + testCases := []struct { + name string + appResources []apps.AppResource + resources config.Resources + wantWarning bool + wantSummary string + }{ + { + name: "job with permissions warns", + appResources: []apps.AppResource{ + {Name: "my-job", Job: &apps.AppResourceJob{Id: "${resources.jobs.my_job.id}", Permission: "CAN_MANAGE_RUN"}}, + }, + resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "my_job": {Permissions: []resources.JobPermission{{Level: "CAN_MANAGE", UserName: "someone@example.com"}}}, + }, + }, + wantWarning: true, + wantSummary: `app "my_app" references jobs "my_job"`, + }, + { + name: "job with SP already included no warning", + appResources: []apps.AppResource{ + {Name: "my-job", Job: &apps.AppResourceJob{Id: "${resources.jobs.my_job.id}", Permission: "CAN_MANAGE_RUN"}}, + }, + resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "my_job": {Permissions: []resources.JobPermission{ + {Level: "CAN_MANAGE", UserName: "someone@example.com"}, + {Level: "CAN_MANAGE_RUN", ServicePrincipalName: "${resources.apps.my_app.service_principal_client_id}"}, + }}, + }, + }, + wantWarning: false, + }, + { + name: "job without permissions no warning", + appResources: []apps.AppResource{ + {Name: "my-job", Job: &apps.AppResourceJob{Id: "${resources.jobs.my_job.id}", Permission: "CAN_MANAGE_RUN"}}, + }, + resources: config.Resources{ + Jobs: map[string]*resources.Job{"my_job": {}}, + }, + wantWarning: false, + }, + { + name: "sql_warehouse with permissions warns", + appResources: []apps.AppResource{ + {Name: "my-wh", SqlWarehouse: &apps.AppResourceSqlWarehouse{Id: "${resources.sql_warehouses.my_wh.id}", Permission: "CAN_USE"}}, + }, + resources: config.Resources{ + SqlWarehouses: map[string]*resources.SqlWarehouse{ + "my_wh": {Permissions: []resources.SqlWarehousePermission{{Level: "CAN_MANAGE", UserName: "someone@example.com"}}}, + }, + }, + wantWarning: true, + wantSummary: `app "my_app" references sql_warehouses "my_wh"`, + }, + { + name: "serving_endpoint with permissions warns", + appResources: []apps.AppResource{ + {Name: "my-ep", ServingEndpoint: &apps.AppResourceServingEndpoint{Name: "${resources.model_serving_endpoints.my_ep.name}", Permission: "CAN_QUERY"}}, + }, + resources: config.Resources{ + ModelServingEndpoints: map[string]*resources.ModelServingEndpoint{ + "my_ep": {Permissions: []resources.ModelServingEndpointPermission{{Level: "CAN_MANAGE", UserName: "someone@example.com"}}}, + }, + }, + wantWarning: true, + wantSummary: `app "my_app" references model_serving_endpoints "my_ep"`, + }, + { + name: "experiment with permissions warns", + appResources: []apps.AppResource{ + {Name: "my-exp", Experiment: &apps.AppResourceExperiment{ExperimentId: "${resources.experiments.my_exp.experiment_id}", Permission: "CAN_READ"}}, + }, + resources: config.Resources{ + Experiments: map[string]*resources.MlflowExperiment{ + "my_exp": {Permissions: []resources.MlflowExperimentPermission{{Level: "CAN_MANAGE", UserName: "someone@example.com"}}}, + }, + }, + wantWarning: true, + wantSummary: `app "my_app" references experiments "my_exp"`, + }, + { + name: "postgres with permissions warns", + appResources: []apps.AppResource{ + {Name: "my-pg", Postgres: &apps.AppResourcePostgres{Branch: "${resources.postgres_projects.my_pg.name}", Permission: "CAN_CONNECT_AND_CREATE"}}, + }, + resources: config.Resources{ + PostgresProjects: map[string]*resources.PostgresProject{ + "my_pg": {Permissions: []resources.Permission{{Level: "CAN_MANAGE", UserName: "someone@example.com"}}}, + }, + }, + wantWarning: true, + wantSummary: `app "my_app" references postgres_projects "my_pg"`, + }, + { + name: "postgres without permissions no warning", + appResources: []apps.AppResource{ + {Name: "my-pg", Postgres: &apps.AppResourcePostgres{Branch: "${resources.postgres_projects.my_pg.name}", Permission: "CAN_CONNECT_AND_CREATE"}}, + }, + resources: config.Resources{ + PostgresProjects: map[string]*resources.PostgresProject{ + "my_pg": {}, + }, + }, + wantWarning: false, + }, + { + name: "non-reference id no warning", + appResources: []apps.AppResource{ + {Name: "my-job", Job: &apps.AppResourceJob{Id: "12345", Permission: "CAN_MANAGE_RUN"}}, + }, + resources: config.Resources{ + Jobs: map[string]*resources.Job{ + "my_job": {Permissions: []resources.JobPermission{{Level: "CAN_MANAGE", UserName: "someone@example.com"}}}, + }, + }, + wantWarning: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tmpDir := t.TempDir() + testutil.Touch(t, tmpDir, "app1", "app.py") + + tc.resources.Apps = map[string]*resources.App{ + "my_app": { + App: apps.App{ + Name: "my_app", + Resources: tc.appResources, + }, + SourceCodePath: "./app1", + }, + } + + b := &bundle.Bundle{ + BundleRootPath: tmpDir, + SyncRootPath: tmpDir, + SyncRoot: vfs.MustNew(tmpDir), + Config: config.Root{ + Workspace: config.Workspace{FilePath: "/foo/bar/"}, + Resources: tc.resources, + }, + } + + bundletest.SetLocation(b, ".", []dyn.Location{{File: filepath.Join(tmpDir, "databricks.yml")}}) + + diags := bundle.ApplySeq(t.Context(), b, Validate()) + warnings := diags.Filter(diag.Warning) + if tc.wantWarning { + require.Len(t, warnings, 1) + require.Contains(t, warnings[0].Summary, tc.wantSummary) + require.Contains(t, warnings[0].Detail, "service_principal_name: ${resources.apps.my_app.service_principal_client_id}") + require.Equal(t, dyn.MustPathFromString("resources.apps.my_app"), warnings[0].Paths[0]) + } else { + require.Empty(t, warnings) + } + }) + } +} + func TestAppsValidateBothSourceCodePathAndGitSource(t *testing.T) { tmpDir := t.TempDir() testutil.Touch(t, tmpDir, "app1", "app.py")