From 9adc6e604d584e9145c158bb51a4f920c0ab4998 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 5 Feb 2024 15:42:36 +0100 Subject: [PATCH 01/19] wip --- bundle/config/mutator/run_as.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 7d1a491753..1d57845443 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -23,6 +23,23 @@ func (m *setRunAs) Name() string { return "SetRunAs" } +// Resources that specify one of the following conditions: +// 1. Allow to set run_as for the resources to a different user from the current +// deployment user. For example, jobs. +// 2. Does not make sense for these resources to run_as a different user. We do not +// have plans to add platform side support for `run_as` for these resources. +// For example, experiments or model serving endpoints. +var allowSetAsOther = []string{"jobs"} + +// Resources that do not allow setting a run_as identity to a different user but +// have plans to add platform side support for `run_as` for these resources at +// some point in the future. For example, pipelines or lakeview dashboards. +var denySetAsOther = []string{"pipelines"} + +func isAllowToRunAsOther(b *bundle.Bundle) { + +} + func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { runAs := b.Config.RunAs if runAs == nil { From 4da90f2ae24e9a8522eb7e06fe49468bd27c0457 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 4 Mar 2024 11:39:21 +0100 Subject: [PATCH 02/19] wip --- bundle/config/mutator/run_as.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 1d57845443..4cc603bbd0 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -6,6 +6,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/jobs" ) @@ -33,11 +34,20 @@ var allowSetAsOther = []string{"jobs"} // Resources that do not allow setting a run_as identity to a different user but // have plans to add platform side support for `run_as` for these resources at -// some point in the future. For example, pipelines or lakeview dashboards. +// some point in the future. For example, pipelines, model serving endpoints or lakeview dashboards. var denySetAsOther = []string{"pipelines"} -func isAllowToRunAsOther(b *bundle.Bundle) { - +func setRunAsForJobs(b *bundle.Bundle, runAs *jobs.JobRunAs) { + for i := range b.Config.Resources.Jobs { + job := b.Config.Resources.Jobs[i] + if job.RunAs != nil { + continue + } + job.RunAs = &jobs.JobRunAs{ + ServicePrincipalName: runAs.ServicePrincipalName, + UserName: runAs.UserName, + } + } } func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { From 05c9d3810b1ebdc213899d02e42502dc834780f5 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 7 Mar 2024 01:27:25 +0100 Subject: [PATCH 03/19] further along the way --- bundle/config/mutator/run_as.go | 141 +++++++++++++----- bundle/config/mutator/run_as_test.go | 45 ++++++ .../tests/run_as/{ => allowed}/databricks.yml | 26 ++-- .../not_allowed/model_serving/databricks.yml | 14 ++ .../not_allowed/pipelines/databricks.yml | 25 ++++ bundle/tests/run_as_test.go | 108 +++++++++++--- 6 files changed, 290 insertions(+), 69 deletions(-) create mode 100644 bundle/config/mutator/run_as_test.go rename bundle/tests/run_as/{ => allowed}/databricks.yml (70%) create mode 100644 bundle/tests/run_as/not_allowed/model_serving/databricks.yml create mode 100644 bundle/tests/run_as/not_allowed/pipelines/databricks.yml diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 4cc603bbd0..b5b50f1ef7 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -2,10 +2,11 @@ package mutator import ( "context" + "fmt" "slices" + "strings" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/dyn" "github.com/databricks/databricks-sdk-go/service/jobs" ) @@ -13,9 +14,10 @@ import ( type setRunAs struct { } -// SetRunAs mutator is used to go over defined resources such as Jobs and DLT Pipelines -// And set correct execution identity ("run_as" for a job or "is_owner" permission for DLT) -// if top-level "run-as" section is defined in the configuration. +// This mutator does two things: +// 1. It sets the run_as field for jobs to the value of the run_as field in the bundle. +// 2. Validates only supported resource types are defined in the bundle when the run_as +// identity is different from the current deployment user. func SetRunAs() bundle.Mutator { return &setRunAs{} } @@ -24,38 +26,122 @@ func (m *setRunAs) Name() string { return "SetRunAs" } -// Resources that specify one of the following conditions: +// TODO: Make sure the error message includes line numbers and file path for +// where the faulty resource is located. + +// TODO: Add a test that does not allow adding a new resource type without being deliberate +// about whether it belongs to allow list or deny list. + +// Resources that satisfy one of the following conditions: // 1. Allow to set run_as for the resources to a different user from the current // deployment user. For example, jobs. // 2. Does not make sense for these resources to run_as a different user. We do not // have plans to add platform side support for `run_as` for these resources. // For example, experiments or model serving endpoints. -var allowSetAsOther = []string{"jobs"} +var allowListForRunAsOther = []string{"jobs", "models", "registered_models", "experiments"} // Resources that do not allow setting a run_as identity to a different user but // have plans to add platform side support for `run_as` for these resources at // some point in the future. For example, pipelines, model serving endpoints or lakeview dashboards. -var denySetAsOther = []string{"pipelines"} +// +// We expect the allow list and the deny list to form a closure of all resource types +// supported by DABs. +var denyListForRunAsOther = []string{"pipelines", "model_serving_endpoints"} -func setRunAsForJobs(b *bundle.Bundle, runAs *jobs.JobRunAs) { - for i := range b.Config.Resources.Jobs { - job := b.Config.Resources.Jobs[i] - if job.RunAs != nil { - continue - } - job.RunAs = &jobs.JobRunAs{ - ServicePrincipalName: runAs.ServicePrincipalName, - UserName: runAs.UserName, - } +type errorUnsupportedResourceTypeForRunAs struct { + resourceType string + resourceValue dyn.Value + currentUser string + runAsUser string +} + +// TODO(6 March 2024): This error message is big. We should split this once +// diag.Diagnostics is ready. +// TODO: Does this required any updates to the default template? Probably no. +func (e errorUnsupportedResourceTypeForRunAs) Error() string { + return fmt.Sprintf("%s are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [%s]. Location of unsupported resource: %s. Current identity: %s. Run as identity: %s", e.resourceType, strings.Join(allowListForRunAsOther, ", "), e.resourceValue.Location(), e.currentUser, e.runAsUser) +} + +func getRunAsIdentity(runAs dyn.Value) (string, error) { + // Get service principal name and user name from run_as section + runAsSp, err := dyn.Get(runAs, "service_principal_name") + if err != nil && !dyn.IsNoSuchKeyError(err) { + return "", err + } + runAsUser, err := dyn.Get(runAs, "user_name") + if err != nil && !dyn.IsNoSuchKeyError(err) { + return "", err + } + + switch { + case runAsSp != dyn.InvalidValue && runAsUser != dyn.InvalidValue: + // TODO: test this case. + return "", fmt.Errorf("run_as section must specify exactly one identity. Both service_principal_name (%s) and user_name are defined (%s)", runAsSp.Location(), runAsUser.Location()) + case runAsSp != dyn.InvalidValue: + return runAsSp.MustString(), nil + case runAsUser != dyn.InvalidValue: + return runAsUser.MustString(), nil + default: + return "", nil } } func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { + // Return early if run_as is not defined in the bundle runAs := b.Config.RunAs if runAs == nil { return nil } + currentUser := b.Config.Workspace.CurrentUser.UserName + + // Assert the run_as configuration is valid + err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + // Get run_as from the bundle + runAs, err := dyn.Get(v, "run_as") + + // If run_as is not defined in the bundle, return early + if dyn.IsNoSuchKeyError(err) { + return v, nil + } + if err != nil { + return dyn.InvalidValue, err + } + + runAsIdentity, err := getRunAsIdentity(runAs) + if err != nil { + return dyn.InvalidValue, err + } + + // If run_as is the same as the current user, return early. All resource + // types are allowed in this case. + if runAsIdentity == currentUser { + return v, nil + } + + rv, err := dyn.Get(v, "resources") + if err != nil { + return dyn.NilValue, err + } + + r := rv.MustMap() + for k, v := range r { + if !slices.Contains(allowListForRunAsOther, k) { + return dyn.InvalidValue, errorUnsupportedResourceTypeForRunAs{ + resourceType: k, + resourceValue: v, + currentUser: currentUser, + runAsUser: runAsIdentity, + } + } + } + return v, nil + }) + if err != nil { + return err + } + + // Set run_as for jobs for i := range b.Config.Resources.Jobs { job := b.Config.Resources.Jobs[i] if job.RunAs != nil { @@ -67,26 +153,5 @@ func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { } } - me := b.Config.Workspace.CurrentUser.UserName - // If user deploying the bundle and the one defined in run_as are the same - // Do not add IS_OWNER permission. Current user is implied to be an owner in this case. - // Otherwise, it will fail due to this bug https://github.com/databricks/terraform-provider-databricks/issues/2407 - if runAs.UserName == me || runAs.ServicePrincipalName == me { - return nil - } - - for i := range b.Config.Resources.Pipelines { - pipeline := b.Config.Resources.Pipelines[i] - pipeline.Permissions = slices.DeleteFunc(pipeline.Permissions, func(p resources.Permission) bool { - return (runAs.ServicePrincipalName != "" && p.ServicePrincipalName == runAs.ServicePrincipalName) || - (runAs.UserName != "" && p.UserName == runAs.UserName) - }) - pipeline.Permissions = append(pipeline.Permissions, resources.Permission{ - Level: "IS_OWNER", - ServicePrincipalName: runAs.ServicePrincipalName, - UserName: runAs.UserName, - }) - } - return nil } diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go new file mode 100644 index 0000000000..ccd6442f71 --- /dev/null +++ b/bundle/config/mutator/run_as_test.go @@ -0,0 +1,45 @@ +package mutator + +import ( + "slices" + "testing" + + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/exp/maps" +) + +// Every new resource type added to DABs should either be classified in the allow +// list or the deny list for run_as other support. +func TestAllResourceTypesAreClassifiedForRunAs(t *testing.T) { + // Compute supported resource types based on the `Resources{}` struct. + r := config.Resources{} + rv, err := convert.FromTyped(r, dyn.NilValue) + require.NoError(t, err) + normalized, _ := convert.Normalize(r, rv, convert.IncludeMissingFields) + resourceTypes := maps.Keys(normalized.MustMap()) + + // Assert that all resource types are classified in either the allow list or the deny list. + for _, resourceType := range resourceTypes { + if slices.Contains(allowListForRunAsOther, resourceType) && !slices.Contains(denyListForRunAsOther, resourceType) { + continue + } + if !slices.Contains(allowListForRunAsOther, resourceType) && slices.Contains(denyListForRunAsOther, resourceType) { + continue + } + if slices.Contains(allowListForRunAsOther, resourceType) && slices.Contains(denyListForRunAsOther, resourceType) { + t.Errorf("Resource type %s is classified in both allow list and deny list for run_as other support", resourceType) + } + if !slices.Contains(allowListForRunAsOther, resourceType) && !slices.Contains(denyListForRunAsOther, resourceType) { + t.Errorf("Resource type %s is not classified in either allow list or deny list for run_as other support", resourceType) + } + } + + // Assert the total list of resource supported, as a sanity check that using + // the dyn library gives us the correct list of all resources supported. Please + // also update this check when adding a new resource + assert.Equal(t, resourceTypes, []string{"jobs", "pipelines", "models", "experiments", "model_serving_endpoints", "registered_models"}) +} diff --git a/bundle/tests/run_as/databricks.yml b/bundle/tests/run_as/allowed/databricks.yml similarity index 70% rename from bundle/tests/run_as/databricks.yml rename to bundle/tests/run_as/allowed/databricks.yml index 1cdc9e44b2..6cb9cd5a49 100644 --- a/bundle/tests/run_as/databricks.yml +++ b/bundle/tests/run_as/allowed/databricks.yml @@ -11,20 +11,6 @@ targets: user_name: "my_user_name" resources: - pipelines: - nyc_taxi_pipeline: - name: "nyc taxi loader" - - permissions: - - level: CAN_VIEW - service_principal_name: my_service_principal - - level: CAN_VIEW - user_name: my_user_name - - libraries: - - notebook: - path: ./dlt/nyc_taxi_loader - jobs: job_one: name: Job One @@ -52,3 +38,15 @@ resources: - task_key: "task_three" notebook_task: notebook_path: "./test.py" + + models: + model_one: + name: "skynet" + + registered_models: + model_two: + name: "skynet (in UC)" + + experiments: + experiment_one: + name: "experiment_one" diff --git a/bundle/tests/run_as/not_allowed/model_serving/databricks.yml b/bundle/tests/run_as/not_allowed/model_serving/databricks.yml new file mode 100644 index 0000000000..08d4ed1a8f --- /dev/null +++ b/bundle/tests/run_as/not_allowed/model_serving/databricks.yml @@ -0,0 +1,14 @@ +bundle: + name: "run_as" + +run_as: + service_principal_name: "my_service_principal" + +targets: + development: + run_as: + user_name: "my_user_name" + +resources: + model_serving_endpoints: + name: "skynet" diff --git a/bundle/tests/run_as/not_allowed/pipelines/databricks.yml b/bundle/tests/run_as/not_allowed/pipelines/databricks.yml new file mode 100644 index 0000000000..d59c34ab63 --- /dev/null +++ b/bundle/tests/run_as/not_allowed/pipelines/databricks.yml @@ -0,0 +1,25 @@ +bundle: + name: "run_as" + +run_as: + service_principal_name: "my_service_principal" + +targets: + development: + run_as: + user_name: "my_user_name" + +resources: + pipelines: + nyc_taxi_pipeline: + name: "nyc taxi loader" + + permissions: + - level: CAN_VIEW + service_principal_name: my_service_principal + - level: CAN_VIEW + user_name: my_user_name + + libraries: + - notebook: + path: ./dlt/nyc_taxi_loader diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index 98aaf63580..e38b5a2bf5 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -7,12 +7,14 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/databricks-sdk-go/service/iam" + "github.com/databricks/databricks-sdk-go/service/ml" "github.com/stretchr/testify/assert" ) -func TestRunAsDefault(t *testing.T) { - b := load(t, "./run_as") +func TestRunAsForAllowed(t *testing.T) { + b := load(t, "./run_as/allowed") ctx := context.Background() bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { @@ -30,6 +32,7 @@ func TestRunAsDefault(t *testing.T) { assert.Len(t, b.Config.Resources.Jobs, 3) jobs := b.Config.Resources.Jobs + // job_one and job_two should have the same run_as identity as the bundle. assert.NotNil(t, jobs["job_one"].RunAs) assert.Equal(t, "my_service_principal", jobs["job_one"].RunAs.ServicePrincipalName) assert.Equal(t, "", jobs["job_one"].RunAs.UserName) @@ -38,21 +41,19 @@ func TestRunAsDefault(t *testing.T) { assert.Equal(t, "my_service_principal", jobs["job_two"].RunAs.ServicePrincipalName) assert.Equal(t, "", jobs["job_two"].RunAs.UserName) + // job_three should retain the job level run_as identity. assert.NotNil(t, jobs["job_three"].RunAs) assert.Equal(t, "my_service_principal_for_job", jobs["job_three"].RunAs.ServicePrincipalName) assert.Equal(t, "", jobs["job_three"].RunAs.UserName) - pipelines := b.Config.Resources.Pipelines - assert.Len(t, pipelines["nyc_taxi_pipeline"].Permissions, 2) - assert.Equal(t, "CAN_VIEW", pipelines["nyc_taxi_pipeline"].Permissions[0].Level) - assert.Equal(t, "my_user_name", pipelines["nyc_taxi_pipeline"].Permissions[0].UserName) - - assert.Equal(t, "IS_OWNER", pipelines["nyc_taxi_pipeline"].Permissions[1].Level) - assert.Equal(t, "my_service_principal", pipelines["nyc_taxi_pipeline"].Permissions[1].ServicePrincipalName) + // Assert other resources are not affected. + assert.Equal(t, ml.Model{Name: "skynet"}, *b.Config.Resources.Models["model_one"].Model) + assert.Equal(t, catalog.CreateRegisteredModelRequest{Name: "skynet (in UC)"}, *b.Config.Resources.RegisteredModels["model_two"].CreateRegisteredModelRequest) + assert.Equal(t, ml.Experiment{Name: "experiment_one"}, *b.Config.Resources.Experiments["experiment_one"].Experiment) } -func TestRunAsDevelopment(t *testing.T) { - b := loadTarget(t, "./run_as", "development") +func TestRunAsForAllowedWithTargetOverride(t *testing.T) { + b := loadTarget(t, "./run_as/allowed", "development") ctx := context.Background() bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { @@ -70,6 +71,8 @@ func TestRunAsDevelopment(t *testing.T) { assert.Len(t, b.Config.Resources.Jobs, 3) jobs := b.Config.Resources.Jobs + // job_one and job_two should have the same run_as identity as the bundle's + // development target. assert.NotNil(t, jobs["job_one"].RunAs) assert.Equal(t, "", jobs["job_one"].RunAs.ServicePrincipalName) assert.Equal(t, "my_user_name", jobs["job_one"].RunAs.UserName) @@ -78,15 +81,86 @@ func TestRunAsDevelopment(t *testing.T) { assert.Equal(t, "", jobs["job_two"].RunAs.ServicePrincipalName) assert.Equal(t, "my_user_name", jobs["job_two"].RunAs.UserName) + // job_three should retain the job level run_as identity. assert.NotNil(t, jobs["job_three"].RunAs) assert.Equal(t, "my_service_principal_for_job", jobs["job_three"].RunAs.ServicePrincipalName) assert.Equal(t, "", jobs["job_three"].RunAs.UserName) - pipelines := b.Config.Resources.Pipelines - assert.Len(t, pipelines["nyc_taxi_pipeline"].Permissions, 2) - assert.Equal(t, "CAN_VIEW", pipelines["nyc_taxi_pipeline"].Permissions[0].Level) - assert.Equal(t, "my_service_principal", pipelines["nyc_taxi_pipeline"].Permissions[0].ServicePrincipalName) + // Assert other resources are not affected. + assert.Equal(t, ml.Model{Name: "skynet"}, *b.Config.Resources.Models["model_one"].Model) + assert.Equal(t, catalog.CreateRegisteredModelRequest{Name: "skynet (in UC)"}, *b.Config.Resources.RegisteredModels["model_two"].CreateRegisteredModelRequest) + assert.Equal(t, ml.Experiment{Name: "experiment_one"}, *b.Config.Resources.Experiments["experiment_one"].Experiment) + +} + +func TestRunAsErrorForPipelines(t *testing.T) { + b := load(t, "./run_as/not_allowed/pipelines") - assert.Equal(t, "IS_OWNER", pipelines["nyc_taxi_pipeline"].Permissions[1].Level) - assert.Equal(t, "my_user_name", pipelines["nyc_taxi_pipeline"].Permissions[1].UserName) + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "jane@doe.com", + }, + } + return nil + }) + + err := bundle.Apply(ctx, b, mutator.SetRunAs()) + assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of unsupported resource: run_as/not_allowed/pipelines/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") +} + +func TestRunAsNoErrorForPipelines(t *testing.T) { + b := load(t, "./run_as/not_allowed/pipelines") + + // We should not error because the pipeline is being deployed with the same + // identity as the bundle run_as identity. + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "my_service_principal", + }, + } + return nil + }) + + err := bundle.Apply(ctx, b, mutator.SetRunAs()) + assert.NoError(t, err) +} + +func TestRunAsErrorForModelServing(t *testing.T) { + b := load(t, "./run_as/not_allowed/model_serving") + + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "jane@doe.com", + }, + } + return nil + }) + + err := bundle.Apply(ctx, b, mutator.SetRunAs()) + assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of unsupported resource: run_as/not_allowed/model_serving/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") +} + +func TestRunAsNoErrorForModelServingEndpoints(t *testing.T) { + b := load(t, "./run_as/not_allowed/model_serving") + + // We should not error because the model serving endpoint is being deployed + // with the same identity as the bundle run_as identity. + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "my_service_principal", + }, + } + return nil + }) + + err := bundle.Apply(ctx, b, mutator.SetRunAs()) + assert.NoError(t, err) } From 335d0c91990405d5f452ecd157a0b98c225690ac Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 7 Mar 2024 14:37:53 +0100 Subject: [PATCH 04/19] add test for when both sp and user are defined --- bundle/config/mutator/run_as.go | 39 +++++++++---------- .../both_sp_and_user/databricks.yml | 17 ++++++++ bundle/tests/run_as_test.go | 21 +++++++++- 3 files changed, 55 insertions(+), 22 deletions(-) create mode 100644 bundle/tests/run_as/not_allowed/both_sp_and_user/databricks.yml diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index b5b50f1ef7..ce3028ec76 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -16,8 +16,9 @@ type setRunAs struct { // This mutator does two things: // 1. It sets the run_as field for jobs to the value of the run_as field in the bundle. -// 2. Validates only supported resource types are defined in the bundle when the run_as -// identity is different from the current deployment user. +// 2. Validates the types of resources used in the bundle. Not all Databricks resource +// types are supported when the current deployment identity is different from +// the bundle's run_as identity. func SetRunAs() bundle.Mutator { return &setRunAs{} } @@ -26,12 +27,6 @@ func (m *setRunAs) Name() string { return "SetRunAs" } -// TODO: Make sure the error message includes line numbers and file path for -// where the faulty resource is located. - -// TODO: Add a test that does not allow adding a new resource type without being deliberate -// about whether it belongs to allow list or deny list. - // Resources that satisfy one of the following conditions: // 1. Allow to set run_as for the resources to a different user from the current // deployment user. For example, jobs. @@ -44,8 +39,8 @@ var allowListForRunAsOther = []string{"jobs", "models", "registered_models", "ex // have plans to add platform side support for `run_as` for these resources at // some point in the future. For example, pipelines, model serving endpoints or lakeview dashboards. // -// We expect the allow list and the deny list to form a closure of all resource types -// supported by DABs. +// We expect the allow list and the deny list to form mutually exclusive and exhaustive +// sets of all resource types that are supported by DABs. var denyListForRunAsOther = []string{"pipelines", "model_serving_endpoints"} type errorUnsupportedResourceTypeForRunAs struct { @@ -57,9 +52,10 @@ type errorUnsupportedResourceTypeForRunAs struct { // TODO(6 March 2024): This error message is big. We should split this once // diag.Diagnostics is ready. -// TODO: Does this required any updates to the default template? Probably no. +// TODO(6 March 2024): Link the docs page describing run_as semantics here once +// the page is ready. func (e errorUnsupportedResourceTypeForRunAs) Error() string { - return fmt.Sprintf("%s are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [%s]. Location of unsupported resource: %s. Current identity: %s. Run as identity: %s", e.resourceType, strings.Join(allowListForRunAsOther, ", "), e.resourceValue.Location(), e.currentUser, e.runAsUser) + return fmt.Sprintf("%s are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [%s]. Location of the unsupported resource: %s. Current identity: %s. Run as identity: %s", e.resourceType, strings.Join(allowListForRunAsOther, ", "), e.resourceValue.Location(), e.currentUser, e.runAsUser) } func getRunAsIdentity(runAs dyn.Value) (string, error) { @@ -73,14 +69,16 @@ func getRunAsIdentity(runAs dyn.Value) (string, error) { return "", err } + sp, spIsDefined := runAsSp.AsString() + user, userIsDefined := runAsUser.AsString() + switch { - case runAsSp != dyn.InvalidValue && runAsUser != dyn.InvalidValue: - // TODO: test this case. - return "", fmt.Errorf("run_as section must specify exactly one identity. Both service_principal_name (%s) and user_name are defined (%s)", runAsSp.Location(), runAsUser.Location()) - case runAsSp != dyn.InvalidValue: - return runAsSp.MustString(), nil - case runAsUser != dyn.InvalidValue: - return runAsUser.MustString(), nil + case spIsDefined && userIsDefined: + return "", fmt.Errorf("run_as section must specify exactly one identity. A service_principal_name %q is specified at %s. A user_name %s is defined at %s", sp, runAsSp.Location(), user, runAsUser.Location()) + case spIsDefined: + return sp, nil + case userIsDefined: + return user, nil default: return "", nil } @@ -95,7 +93,7 @@ func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { currentUser := b.Config.Workspace.CurrentUser.UserName - // Assert the run_as configuration is valid + // Assert the run_as configuration is valid in the context of the bundle err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { // Get run_as from the bundle runAs, err := dyn.Get(v, "run_as") @@ -126,6 +124,7 @@ func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { r := rv.MustMap() for k, v := range r { + // If the resource type is not in the allow list, return an error if !slices.Contains(allowListForRunAsOther, k) { return dyn.InvalidValue, errorUnsupportedResourceTypeForRunAs{ resourceType: k, diff --git a/bundle/tests/run_as/not_allowed/both_sp_and_user/databricks.yml b/bundle/tests/run_as/not_allowed/both_sp_and_user/databricks.yml new file mode 100644 index 0000000000..dfab50e94b --- /dev/null +++ b/bundle/tests/run_as/not_allowed/both_sp_and_user/databricks.yml @@ -0,0 +1,17 @@ +bundle: + name: "run_as" + +# This is not allowed because both service_principal_name and user_name are set +run_as: + service_principal_name: "my_service_principal" + user_name: "my_user_name" + +resources: + jobs: + job_one: + name: Job One + + tasks: + - task_key: "task_one" + notebook_task: + notebook_path: "./test.py" diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index e38b5a2bf5..69afa7b493 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -107,7 +107,7 @@ func TestRunAsErrorForPipelines(t *testing.T) { }) err := bundle.Apply(ctx, b, mutator.SetRunAs()) - assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of unsupported resource: run_as/not_allowed/pipelines/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as/not_allowed/pipelines/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") } func TestRunAsNoErrorForPipelines(t *testing.T) { @@ -143,7 +143,7 @@ func TestRunAsErrorForModelServing(t *testing.T) { }) err := bundle.Apply(ctx, b, mutator.SetRunAs()) - assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of unsupported resource: run_as/not_allowed/model_serving/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as/not_allowed/model_serving/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") } func TestRunAsNoErrorForModelServingEndpoints(t *testing.T) { @@ -164,3 +164,20 @@ func TestRunAsNoErrorForModelServingEndpoints(t *testing.T) { err := bundle.Apply(ctx, b, mutator.SetRunAs()) assert.NoError(t, err) } + +func TestRunAsErrorWhenBothUserAndSpSpecified(t *testing.T) { + b := load(t, "./run_as/not_allowed/both_sp_and_user") + + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "my_service_principal", + }, + } + return nil + }) + + err := bundle.Apply(ctx, b, mutator.SetRunAs()) + assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as/not_allowed/both_sp_and_user/databricks.yml:6:27. A user_name my_user_name is defined at run_as/not_allowed/both_sp_and_user/databricks.yml:7:14") +} From 776af33674c3885e45dedaa6249d25a38d39c0cd Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 7 Mar 2024 14:40:01 +0100 Subject: [PATCH 05/19] fix test --- bundle/config/mutator/run_as_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index ccd6442f71..71ed33e5f7 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -21,6 +21,7 @@ func TestAllResourceTypesAreClassifiedForRunAs(t *testing.T) { require.NoError(t, err) normalized, _ := convert.Normalize(r, rv, convert.IncludeMissingFields) resourceTypes := maps.Keys(normalized.MustMap()) + slices.Sort(resourceTypes) // Assert that all resource types are classified in either the allow list or the deny list. for _, resourceType := range resourceTypes { From 20a0274fea0abb15fdfa4a31b0e5553557efa5be Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 7 Mar 2024 14:44:36 +0100 Subject: [PATCH 06/19] final cleanup --- bundle/config/mutator/run_as.go | 8 ++++---- bundle/tests/run_as_test.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index ce3028ec76..ccd84ada33 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -32,7 +32,7 @@ func (m *setRunAs) Name() string { // deployment user. For example, jobs. // 2. Does not make sense for these resources to run_as a different user. We do not // have plans to add platform side support for `run_as` for these resources. -// For example, experiments or model serving endpoints. +// For example, experiments or registered models. var allowListForRunAsOther = []string{"jobs", "models", "registered_models", "experiments"} // Resources that do not allow setting a run_as identity to a different user but @@ -52,8 +52,8 @@ type errorUnsupportedResourceTypeForRunAs struct { // TODO(6 March 2024): This error message is big. We should split this once // diag.Diagnostics is ready. -// TODO(6 March 2024): Link the docs page describing run_as semantics here once -// the page is ready. +// TODO(6 March 2024): Link the docs page describing run_as semantics in the error below +// once the page is ready. func (e errorUnsupportedResourceTypeForRunAs) Error() string { return fmt.Sprintf("%s are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [%s]. Location of the unsupported resource: %s. Current identity: %s. Run as identity: %s", e.resourceType, strings.Join(allowListForRunAsOther, ", "), e.resourceValue.Location(), e.currentUser, e.runAsUser) } @@ -74,7 +74,7 @@ func getRunAsIdentity(runAs dyn.Value) (string, error) { switch { case spIsDefined && userIsDefined: - return "", fmt.Errorf("run_as section must specify exactly one identity. A service_principal_name %q is specified at %s. A user_name %s is defined at %s", sp, runAsSp.Location(), user, runAsUser.Location()) + return "", fmt.Errorf("run_as section must specify exactly one identity. A service_principal_name %q is specified at %s. A user_name %q is defined at %s", sp, runAsSp.Location(), user, runAsUser.Location()) case spIsDefined: return sp, nil case userIsDefined: diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index 69afa7b493..258607c905 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -179,5 +179,5 @@ func TestRunAsErrorWhenBothUserAndSpSpecified(t *testing.T) { }) err := bundle.Apply(ctx, b, mutator.SetRunAs()) - assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as/not_allowed/both_sp_and_user/databricks.yml:6:27. A user_name my_user_name is defined at run_as/not_allowed/both_sp_and_user/databricks.yml:7:14") + assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as/not_allowed/both_sp_and_user/databricks.yml:6:27. A user_name \"my_user_name\" is defined at run_as/not_allowed/both_sp_and_user/databricks.yml:7:14") } From 5886a6587c8c4d3c1c4e5ca5d8b5cbbac696934e Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 7 Mar 2024 14:48:33 +0100 Subject: [PATCH 07/19] fix test --- bundle/config/mutator/run_as_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index 71ed33e5f7..fbc6ecd61f 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -42,5 +42,5 @@ func TestAllResourceTypesAreClassifiedForRunAs(t *testing.T) { // Assert the total list of resource supported, as a sanity check that using // the dyn library gives us the correct list of all resources supported. Please // also update this check when adding a new resource - assert.Equal(t, resourceTypes, []string{"jobs", "pipelines", "models", "experiments", "model_serving_endpoints", "registered_models"}) + assert.Equal(t, []string{"experiments", "jobs", "model_serving_endpoints", "models", "pipelines", "registered_models"}, resourceTypes) } From f2d24fcd699da8c3900c275c9707fbd24b0f7bbf Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 7 Mar 2024 15:02:29 +0100 Subject: [PATCH 08/19] fix windows tests --- bundle/tests/run_as_test.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index 258607c905..89ad2917e4 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -2,6 +2,7 @@ package config_tests import ( "context" + "runtime" "testing" "github.com/databricks/cli/bundle" @@ -107,7 +108,12 @@ func TestRunAsErrorForPipelines(t *testing.T) { }) err := bundle.Apply(ctx, b, mutator.SetRunAs()) - assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as/not_allowed/pipelines/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + + if runtime.GOOS == "windows" { + assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as\\not_allowed\\pipelines\\databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + } else { + assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as/not_allowed/pipelines/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + } } func TestRunAsNoErrorForPipelines(t *testing.T) { @@ -143,7 +149,12 @@ func TestRunAsErrorForModelServing(t *testing.T) { }) err := bundle.Apply(ctx, b, mutator.SetRunAs()) - assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as/not_allowed/model_serving/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + + if runtime.GOOS == "windows" { + assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as\\not_allowed\\model_serving\\databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + } else { + assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as/not_allowed/model_serving/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + } } func TestRunAsNoErrorForModelServingEndpoints(t *testing.T) { @@ -179,5 +190,10 @@ func TestRunAsErrorWhenBothUserAndSpSpecified(t *testing.T) { }) err := bundle.Apply(ctx, b, mutator.SetRunAs()) - assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as/not_allowed/both_sp_and_user/databricks.yml:6:27. A user_name \"my_user_name\" is defined at run_as/not_allowed/both_sp_and_user/databricks.yml:7:14") + + if runtime.GOOS == "windows" { + assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as\\not_allowed\\both_sp_and_user\\databricks.yml:6:27. A user_name \"my_user_name\" is defined at run_as\\not_allowed\\both_sp_and_user\\databricks.yml:7:14") + } else { + assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as/not_allowed/both_sp_and_user/databricks.yml:6:27. A user_name \"my_user_name\" is defined at run_as/not_allowed/both_sp_and_user/databricks.yml:7:14") + } } From a4cc92d95cc0a2654ec4c53406829b6427f22df3 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 11 Mar 2024 21:24:28 +0100 Subject: [PATCH 09/19] rename vars --- bundle/config/mutator/run_as.go | 8 ++++---- bundle/config/mutator/run_as_test.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index ccd84ada33..6e7831220e 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -33,7 +33,7 @@ func (m *setRunAs) Name() string { // 2. Does not make sense for these resources to run_as a different user. We do not // have plans to add platform side support for `run_as` for these resources. // For example, experiments or registered models. -var allowListForRunAsOther = []string{"jobs", "models", "registered_models", "experiments"} +var allowListForRunAs = []string{"jobs", "models", "registered_models", "experiments"} // Resources that do not allow setting a run_as identity to a different user but // have plans to add platform side support for `run_as` for these resources at @@ -41,7 +41,7 @@ var allowListForRunAsOther = []string{"jobs", "models", "registered_models", "ex // // We expect the allow list and the deny list to form mutually exclusive and exhaustive // sets of all resource types that are supported by DABs. -var denyListForRunAsOther = []string{"pipelines", "model_serving_endpoints"} +var denyListForRunAs = []string{"pipelines", "model_serving_endpoints"} type errorUnsupportedResourceTypeForRunAs struct { resourceType string @@ -55,7 +55,7 @@ type errorUnsupportedResourceTypeForRunAs struct { // TODO(6 March 2024): Link the docs page describing run_as semantics in the error below // once the page is ready. func (e errorUnsupportedResourceTypeForRunAs) Error() string { - return fmt.Sprintf("%s are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [%s]. Location of the unsupported resource: %s. Current identity: %s. Run as identity: %s", e.resourceType, strings.Join(allowListForRunAsOther, ", "), e.resourceValue.Location(), e.currentUser, e.runAsUser) + return fmt.Sprintf("%s are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [%s]. Location of the unsupported resource: %s. Current identity: %s. Run as identity: %s", e.resourceType, strings.Join(allowListForRunAs, ", "), e.resourceValue.Location(), e.currentUser, e.runAsUser) } func getRunAsIdentity(runAs dyn.Value) (string, error) { @@ -125,7 +125,7 @@ func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { r := rv.MustMap() for k, v := range r { // If the resource type is not in the allow list, return an error - if !slices.Contains(allowListForRunAsOther, k) { + if !slices.Contains(allowListForRunAs, k) { return dyn.InvalidValue, errorUnsupportedResourceTypeForRunAs{ resourceType: k, resourceValue: v, diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index fbc6ecd61f..6ff73f463c 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -25,16 +25,16 @@ func TestAllResourceTypesAreClassifiedForRunAs(t *testing.T) { // Assert that all resource types are classified in either the allow list or the deny list. for _, resourceType := range resourceTypes { - if slices.Contains(allowListForRunAsOther, resourceType) && !slices.Contains(denyListForRunAsOther, resourceType) { + if slices.Contains(allowListForRunAs, resourceType) && !slices.Contains(denyListForRunAs, resourceType) { continue } - if !slices.Contains(allowListForRunAsOther, resourceType) && slices.Contains(denyListForRunAsOther, resourceType) { + if !slices.Contains(allowListForRunAs, resourceType) && slices.Contains(denyListForRunAs, resourceType) { continue } - if slices.Contains(allowListForRunAsOther, resourceType) && slices.Contains(denyListForRunAsOther, resourceType) { + if slices.Contains(allowListForRunAs, resourceType) && slices.Contains(denyListForRunAs, resourceType) { t.Errorf("Resource type %s is classified in both allow list and deny list for run_as other support", resourceType) } - if !slices.Contains(allowListForRunAsOther, resourceType) && !slices.Contains(denyListForRunAsOther, resourceType) { + if !slices.Contains(allowListForRunAs, resourceType) && !slices.Contains(denyListForRunAs, resourceType) { t.Errorf("Resource type %s is not classified in either allow list or deny list for run_as other support", resourceType) } } From aafd7672a4c3843d509ac92e965f73f777973e55 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 13 Mar 2024 15:41:51 +0100 Subject: [PATCH 10/19] fix tests --- bundle/config/mutator/run_as.go | 6 ++++++ .../tests/run_as/not_allowed/model_serving/databricks.yml | 3 ++- bundle/tests/run_as_test.go | 8 ++++---- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 3549473f56..348668878e 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -13,6 +13,7 @@ type setRunAs struct { } // This mutator does two things: +// // 1. Sets the run_as field for jobs to the value of the run_as field in the bundle. // // 2. Validates the bundle run_as configuration is valid in the context of the bundle. @@ -70,6 +71,11 @@ func validateRunAs(b *bundle.Bundle) error { identity = runAs.UserName } + // All resources are supported if the run_as identity is the same as the current deployment identity. + if identity == b.Config.Workspace.CurrentUser.UserName { + return nil + } + // DLT pipelines do not support run_as in the API. if len(b.Config.Resources.Pipelines) > 0 { return errUnsupportedResourceTypeForRunAs{ diff --git a/bundle/tests/run_as/not_allowed/model_serving/databricks.yml b/bundle/tests/run_as/not_allowed/model_serving/databricks.yml index 08d4ed1a8f..cdd7e09135 100644 --- a/bundle/tests/run_as/not_allowed/model_serving/databricks.yml +++ b/bundle/tests/run_as/not_allowed/model_serving/databricks.yml @@ -11,4 +11,5 @@ targets: resources: model_serving_endpoints: - name: "skynet" + foo: + name: "skynet" diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index 89ad2917e4..f2ab1f2f67 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -110,9 +110,9 @@ func TestRunAsErrorForPipelines(t *testing.T) { err := bundle.Apply(ctx, b, mutator.SetRunAs()) if runtime.GOOS == "windows" { - assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as\\not_allowed\\pipelines\\databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: run_as\\not_allowed\\pipelines\\databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") } else { - assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as/not_allowed/pipelines/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: run_as/not_allowed/pipelines/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") } } @@ -151,9 +151,9 @@ func TestRunAsErrorForModelServing(t *testing.T) { err := bundle.Apply(ctx, b, mutator.SetRunAs()) if runtime.GOOS == "windows" { - assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as\\not_allowed\\model_serving\\databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: run_as\\not_allowed\\model_serving\\databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") } else { - assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. List of supported resources: [jobs, models, registered_models, experiments]. Location of the unsupported resource: run_as/not_allowed/model_serving/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") + assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: run_as/not_allowed/model_serving/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") } } From 9d6949aa027afaaafac0071219e20e87e07389b5 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 13 Mar 2024 16:03:28 +0100 Subject: [PATCH 11/19] cleanup --- bundle/config/mutator/run_as.go | 2 -- bundle/config/mutator/run_as_test.go | 17 +++++++++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 348668878e..eedf811119 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -34,8 +34,6 @@ type errUnsupportedResourceTypeForRunAs struct { runAsUser string } -// TODO(6 March 2024): This error message is big. We should split this once -// diag.Diagnostics is ready. // TODO(6 March 2024): Link the docs page describing run_as semantics in the error below // once the page is ready. func (e errUnsupportedResourceTypeForRunAs) Error() string { diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index 6b5c744f2a..53baf5178f 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -29,7 +29,16 @@ func allResourceTypes(t *testing.T) []string { // Assert the total list of resource supported, as a sanity check that using // the dyn library gives us the correct list of all resources supported. Please // also update this check when adding a new resource - require.Equal(t, []string{"experiments", "jobs", "model_serving_endpoints", "models", "pipelines", "registered_models"}, resourceTypes) + require.Equal(t, []string{ + "experiments", + "jobs", + "model_serving_endpoints", + "models", + "pipelines", + "registered_models", + }, + resourceTypes, + ) return resourceTypes } @@ -114,7 +123,7 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { // the relevant team whether the resource should be on the allow list or (implicitly) on // the deny list. Any resources that could have run_as semantics in the future // should be on the deny list. - // For example: Teams for pipelines, model serving endpoints or lakeview dashboards + // For example: Teams for pipelines, model serving endpoints or Lakeview dashboards // are planning to add platform side support for `run_as` for these resources at // some point in the future. These resources are (implicitly) on the deny list. allowList := []string{ @@ -146,6 +155,8 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { continue } + // Add an instance of the resource type that is not on the allow list to + // the bundle configuration. nv, err := dyn.SetByPath(v, dyn.NewPath(dyn.Key("resources"), dyn.Key(rt)), dyn.V(map[string]dyn.Value{ "foo": dyn.V(map[string]dyn.Value{ "path": dyn.V("bar"), @@ -153,10 +164,12 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { })) require.NoError(t, err) + // Get back typed configuration from the new invalid bundle configuration. r := &config.Root{} err = convert.ToTyped(r, nv) require.NoError(t, err) + // Assert this invalid bundle configuration fails validation. b := &bundle.Bundle{ Config: *r, } From fab62f39ac694479d6bece8499329fc9ab6bb904 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 13 Mar 2024 17:02:24 +0100 Subject: [PATCH 12/19] add error for when neither are specified --- bundle/config/mutator/run_as.go | 5 +-- .../neither_sp_nor_user/databricks.yml | 4 ++ .../databricks.yml | 8 ++++ .../neither_sp_nor_user_override/override.yml | 4 ++ bundle/tests/run_as_test.go | 43 +++++++++++++++++++ 5 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 bundle/tests/run_as/not_allowed/neither_sp_nor_user/databricks.yml create mode 100644 bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/databricks.yml create mode 100644 bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/override.yml diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index eedf811119..b0725c0b97 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -98,15 +98,14 @@ func validateRunAs(b *bundle.Bundle) error { } func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { - // Return early if run_as is not defined in the bundle + // Mutator is a no-op if run_as is not specified in the bundle runAs := b.Config.RunAs if runAs == nil { return nil } - // Mutator is a no-op if neither are specified in the bundle if runAs.ServicePrincipalName == "" && runAs.UserName == "" { - return nil + return fmt.Errorf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s", b.Config.TryLocation("run_as")) } // Assert the run_as configuration is valid in the context of the bundle diff --git a/bundle/tests/run_as/not_allowed/neither_sp_nor_user/databricks.yml b/bundle/tests/run_as/not_allowed/neither_sp_nor_user/databricks.yml new file mode 100644 index 0000000000..a328fbd8c2 --- /dev/null +++ b/bundle/tests/run_as/not_allowed/neither_sp_nor_user/databricks.yml @@ -0,0 +1,4 @@ +bundle: + name: "abc" + +run_as: diff --git a/bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/databricks.yml b/bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/databricks.yml new file mode 100644 index 0000000000..f7c1d728d8 --- /dev/null +++ b/bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/databricks.yml @@ -0,0 +1,8 @@ +bundle: + name: "abc" + +run_as: + user_name: "my_user_name" + +include: + - ./override.yml diff --git a/bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/override.yml b/bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/override.yml new file mode 100644 index 0000000000..d093e4c95b --- /dev/null +++ b/bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/override.yml @@ -0,0 +1,4 @@ +targets: + development: + default: true + run_as: diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index f2ab1f2f67..f82ed62df0 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -197,3 +197,46 @@ func TestRunAsErrorWhenBothUserAndSpSpecified(t *testing.T) { assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as/not_allowed/both_sp_and_user/databricks.yml:6:27. A user_name \"my_user_name\" is defined at run_as/not_allowed/both_sp_and_user/databricks.yml:7:14") } } + +func TestRunAsErrorNeitherUserOrSpSpecified(t *testing.T) { + b := load(t, "./run_as/not_allowed/neither_sp_nor_user") + + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "my_service_principal", + }, + } + return nil + }) + + err := bundle.Apply(ctx, b, mutator.SetRunAs()) + if runtime.GOOS == "windows" { + assert.EqualError(t, err, "run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at run_as\\not_allowed\\neither_sp_nor_user\\databricks.yml:4:8") + } else { + assert.EqualError(t, err, "run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at run_as/not_allowed/neither_sp_nor_user/databricks.yml:4:8") + } +} + + +func TestRunAsErrorNeitherUserOrSpSpecifiedAtTargetOverride(t *testing.T) { + b := loadTarget(t, "./run_as/not_allowed/neither_sp_nor_user_override", "development") + + ctx := context.Background() + bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) error { + b.Config.Workspace.CurrentUser = &config.User{ + User: &iam.User{ + UserName: "my_service_principal", + }, + } + return nil + }) + + err := bundle.Apply(ctx, b, mutator.SetRunAs()) + if runtime.GOOS == "windows" { + assert.EqualError(t, err, "run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at run_as\\not_allowed\\neither_sp_nor_user_override\\override.yml:4:12") + } else { + assert.EqualError(t, err, "run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at run_as/not_allowed/neither_sp_nor_user_override/override.yml:4:12") + } +} From 89c86c89526d790bfcbb349fcf942768ceec7271 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 13 Mar 2024 17:04:23 +0100 Subject: [PATCH 13/19] lint --- bundle/tests/run_as_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index f82ed62df0..c6c75a15e9 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -219,7 +219,6 @@ func TestRunAsErrorNeitherUserOrSpSpecified(t *testing.T) { } } - func TestRunAsErrorNeitherUserOrSpSpecifiedAtTargetOverride(t *testing.T) { b := loadTarget(t, "./run_as/not_allowed/neither_sp_nor_user_override", "development") From dcdb87f12beffd50f92b88d7eb1ad1a387a0b5bf Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 13 Mar 2024 17:08:59 +0100 Subject: [PATCH 14/19] - --- bundle/config/mutator/run_as.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index b0725c0b97..58af1a7612 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -54,6 +54,11 @@ func (e errBothSpAndUserSpecified) Error() string { func validateRunAs(b *bundle.Bundle) error { runAs := b.Config.RunAs + // Error if neither service_principal_name nor user_name are specified + if runAs.ServicePrincipalName == "" && runAs.UserName == "" { + return fmt.Errorf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s", b.Config.TryLocation("run_as")) + } + // Error if both service_principal_name and user_name are specified if runAs.UserName != "" && runAs.ServicePrincipalName != "" { return errBothSpAndUserSpecified{ @@ -104,10 +109,6 @@ func (m *setRunAs) Apply(_ context.Context, b *bundle.Bundle) error { return nil } - if runAs.ServicePrincipalName == "" && runAs.UserName == "" { - return fmt.Errorf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s", b.Config.TryLocation("run_as")) - } - // Assert the run_as configuration is valid in the context of the bundle if err := validateRunAs(b); err != nil { return err From 9c2bfc6345c1afa1dc4c9b12cd111a13c5a68011 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 25 Mar 2024 14:22:23 +0100 Subject: [PATCH 15/19] comment refine --- bundle/config/mutator/run_as_test.go | 14 ++++++++------ bundle/config/root.go | 2 ++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index 53baf5178f..0540fb9b6d 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -98,7 +98,7 @@ func TestRunAsWorksForAllowedResources(t *testing.T) { } func TestRunAsErrorForUnsupportedResources(t *testing.T) { - // Bundle "run_as" has two mode of operations, each with a different set of + // Bundle "run_as" has two modes of operation, each with a different set of // resources that are supported. // Cases: // 1. When the bundle "run_as" identity is same as the current deployment @@ -110,22 +110,24 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { // To be a part of the allow list, the resource must satisfy one of the following // two conditions: // 1. The resource supports setting a run_as identity to a different user - // from the owner/creator of the job. For example, jobs. + // from the owner/creator of the resource. For example, jobs. // 2. Run as semantics do not apply to the resource. We do not plan to add // platform side support for `run_as` for these resources. For example, // experiments or registered models. // // Any resource that is not on the allow list cannot be used when the bundle // run_as is different from the current deployment user. "bundle validate" must - // return an error if such a resource has been defined in the bundle configuration. + // return an error if such a resource has been defined, and the run_as identity + // is different from the current deployment identity. // // Action Item: If you are adding a new resource to DABs, please check in with - // the relevant team whether the resource should be on the allow list or (implicitly) on + // the relevant owning team whether the resource should be on the allow list or (implicitly) on // the deny list. Any resources that could have run_as semantics in the future // should be on the deny list. // For example: Teams for pipelines, model serving endpoints or Lakeview dashboards // are planning to add platform side support for `run_as` for these resources at - // some point in the future. These resources are (implicitly) on the deny list. + // some point in the future. These resources are (implicitly) on the deny list, since + // they are not on the allow list below. allowList := []string{ "jobs", "models", @@ -164,7 +166,7 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { })) require.NoError(t, err) - // Get back typed configuration from the new invalid bundle configuration. + // Get back typed configuration from the newly created invalid bundle configuration. r := &config.Root{} err = convert.ToTyped(r, nv) require.NoError(t, err) diff --git a/bundle/config/root.go b/bundle/config/root.go index c2e4c8816a..e5d8c83085 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -459,6 +459,8 @@ func validateVariableOverrides(root, target dyn.Value) (err error) { } // Best effort to get the location of configuration value at the specified path. +// This function is useful to annotate error messages with the location, because +// we don't want to fail with a different error message if we cannot retrieve the location. func (r *Root) TryLocation(path string) dyn.Location { v, err := dyn.Get(r.value, path) if err != nil { From 98f8be2f49e105e801335f6f505b7d507ce39dd0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 25 Mar 2024 17:01:46 +0100 Subject: [PATCH 16/19] more diagnostics --- bundle/config/mutator/run_as_test.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/bundle/config/mutator/run_as_test.go b/bundle/config/mutator/run_as_test.go index 0540fb9b6d..d6fb2939f6 100644 --- a/bundle/config/mutator/run_as_test.go +++ b/bundle/config/mutator/run_as_test.go @@ -14,7 +14,6 @@ import ( "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "golang.org/x/exp/maps" ) func allResourceTypes(t *testing.T) []string { @@ -23,7 +22,10 @@ func allResourceTypes(t *testing.T) []string { rv, err := convert.FromTyped(r, dyn.NilValue) require.NoError(t, err) normalized, _ := convert.Normalize(r, rv, convert.IncludeMissingFields) - resourceTypes := maps.Keys(normalized.MustMap()) + resourceTypes := []string{} + for _, k := range normalized.MustMap().Keys() { + resourceTypes = append(resourceTypes, k.MustString()) + } slices.Sort(resourceTypes) // Assert the total list of resource supported, as a sanity check that using @@ -89,8 +91,8 @@ func TestRunAsWorksForAllowedResources(t *testing.T) { Config: config, } - err := bundle.Apply(context.Background(), b, SetRunAs()) - assert.NoError(t, err) + diags := bundle.Apply(context.Background(), b, SetRunAs()) + assert.NoError(t, diags.Error()) for _, job := range b.Config.Resources.Jobs { assert.Equal(t, "bob", job.RunAs.UserName) @@ -175,7 +177,12 @@ func TestRunAsErrorForUnsupportedResources(t *testing.T) { b := &bundle.Bundle{ Config: *r, } - err = bundle.Apply(context.Background(), b, SetRunAs()) - assert.ErrorAs(t, err, &errUnsupportedResourceTypeForRunAs{}, "expected run_as not supported error for resource type: %s", rt) + 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) } } From 9e9e18a9ffa3f0f5dbce8eaffe3eb6eb2f3e7eb7 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 27 Mar 2024 13:19:20 +0100 Subject: [PATCH 17/19] TryLocation -> GetLocation --- bundle/config/mutator/run_as.go | 10 +++++----- bundle/config/root.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 242ba5e035..262de0bc6f 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -57,7 +57,7 @@ func validateRunAs(b *bundle.Bundle) error { // Error if neither service_principal_name nor user_name are specified if runAs.ServicePrincipalName == "" && runAs.UserName == "" { - return fmt.Errorf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s", b.Config.TryLocation("run_as")) + return fmt.Errorf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s", b.Config.GetLocation("run_as")) } // Error if both service_principal_name and user_name are specified @@ -65,8 +65,8 @@ func validateRunAs(b *bundle.Bundle) error { return errBothSpAndUserSpecified{ spName: runAs.ServicePrincipalName, userName: runAs.UserName, - spLoc: b.Config.TryLocation("run_as.service_principal_name"), - userLoc: b.Config.TryLocation("run_as.user_name"), + spLoc: b.Config.GetLocation("run_as.service_principal_name"), + userLoc: b.Config.GetLocation("run_as.user_name"), } } @@ -84,7 +84,7 @@ func validateRunAs(b *bundle.Bundle) error { if len(b.Config.Resources.Pipelines) > 0 { return errUnsupportedResourceTypeForRunAs{ resourceType: "pipelines", - resourceLocation: b.Config.TryLocation("resources.pipelines"), + resourceLocation: b.Config.GetLocation("resources.pipelines"), currentUser: b.Config.Workspace.CurrentUser.UserName, runAsUser: identity, } @@ -94,7 +94,7 @@ func validateRunAs(b *bundle.Bundle) error { if len(b.Config.Resources.ModelServingEndpoints) > 0 { return errUnsupportedResourceTypeForRunAs{ resourceType: "model_serving_endpoints", - resourceLocation: b.Config.TryLocation("resources.model_serving_endpoints"), + resourceLocation: b.Config.GetLocation("resources.model_serving_endpoints"), currentUser: b.Config.Workspace.CurrentUser.UserName, runAsUser: identity, } diff --git a/bundle/config/root.go b/bundle/config/root.go index b30b0211fb..0e54c04ce4 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -452,7 +452,7 @@ func validateVariableOverrides(root, target dyn.Value) (err error) { // Best effort to get the location of configuration value at the specified path. // This function is useful to annotate error messages with the location, because // we don't want to fail with a different error message if we cannot retrieve the location. -func (r *Root) TryLocation(path string) dyn.Location { +func (r *Root) GetLocation(path string) dyn.Location { v, err := dyn.Get(r.value, path) if err != nil { return dyn.Location{} From dcaf247eeb33fb7c07409cd003105896b70169c2 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 27 Mar 2024 16:11:43 +0100 Subject: [PATCH 18/19] address comments about comments --- bundle/config/mutator/run_as.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/run_as.go b/bundle/config/mutator/run_as.go index 262de0bc6f..578591eb14 100644 --- a/bundle/config/mutator/run_as.go +++ b/bundle/config/mutator/run_as.go @@ -17,7 +17,7 @@ type setRunAs struct { // // 1. Sets the run_as field for jobs to the value of the run_as field in the bundle. // -// 2. Validates the bundle run_as configuration is valid in the context of the bundle. +// 2. Validates that the bundle run_as configuration is valid in the context of the bundle. // If the run_as user is different from the current deployment user, DABs only // supports a subset of resources. func SetRunAs() bundle.Mutator { @@ -90,7 +90,7 @@ func validateRunAs(b *bundle.Bundle) error { } } - // DLT model serving endpoints do not support run_as in the API. + // Model serving endpoints do not support run_as in the API. if len(b.Config.Resources.ModelServingEndpoints) > 0 { return errUnsupportedResourceTypeForRunAs{ resourceType: "model_serving_endpoints", From a50117e82cb9094b1c70f8ddfc2e031cc7a0f8fa Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 27 Mar 2024 16:22:42 +0100 Subject: [PATCH 19/19] remove branching for windows assertions --- bundle/tests/run_as_test.go | 38 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/bundle/tests/run_as_test.go b/bundle/tests/run_as_test.go index 37146c29c7..3b9deafe0d 100644 --- a/bundle/tests/run_as_test.go +++ b/bundle/tests/run_as_test.go @@ -2,7 +2,8 @@ package config_tests import ( "context" - "runtime" + "fmt" + "path/filepath" "testing" "github.com/databricks/cli/bundle" @@ -111,11 +112,8 @@ func TestRunAsErrorForPipelines(t *testing.T) { diags := bundle.Apply(ctx, b, mutator.SetRunAs()) err := diags.Error() - if runtime.GOOS == "windows" { - assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: run_as\\not_allowed\\pipelines\\databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") - } else { - assert.EqualError(t, err, "pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: run_as/not_allowed/pipelines/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") - } + configPath := filepath.FromSlash("run_as/not_allowed/pipelines/databricks.yml") + assert.EqualError(t, err, fmt.Sprintf("pipelines are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: %s:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal", configPath)) } func TestRunAsNoErrorForPipelines(t *testing.T) { @@ -153,11 +151,8 @@ func TestRunAsErrorForModelServing(t *testing.T) { diags := bundle.Apply(ctx, b, mutator.SetRunAs()) err := diags.Error() - if runtime.GOOS == "windows" { - assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: run_as\\not_allowed\\model_serving\\databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") - } else { - assert.EqualError(t, err, "model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: run_as/not_allowed/model_serving/databricks.yml:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal") - } + configPath := filepath.FromSlash("run_as/not_allowed/model_serving/databricks.yml") + assert.EqualError(t, err, fmt.Sprintf("model_serving_endpoints are not supported when the current deployment user is different from the bundle's run_as identity. Please deploy as the run_as identity. Location of the unsupported resource: %s:14:5. Current identity: jane@doe.com. Run as identity: my_service_principal", configPath)) } func TestRunAsNoErrorForModelServingEndpoints(t *testing.T) { @@ -195,11 +190,8 @@ func TestRunAsErrorWhenBothUserAndSpSpecified(t *testing.T) { diags := bundle.Apply(ctx, b, mutator.SetRunAs()) err := diags.Error() - if runtime.GOOS == "windows" { - assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as\\not_allowed\\both_sp_and_user\\databricks.yml:6:27. A user_name \"my_user_name\" is defined at run_as\\not_allowed\\both_sp_and_user\\databricks.yml:7:14") - } else { - assert.EqualError(t, err, "run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at run_as/not_allowed/both_sp_and_user/databricks.yml:6:27. A user_name \"my_user_name\" is defined at run_as/not_allowed/both_sp_and_user/databricks.yml:7:14") - } + configPath := filepath.FromSlash("run_as/not_allowed/both_sp_and_user/databricks.yml") + assert.EqualError(t, err, fmt.Sprintf("run_as section must specify exactly one identity. A service_principal_name \"my_service_principal\" is specified at %s:6:27. A user_name \"my_user_name\" is defined at %s:7:14", configPath, configPath)) } func TestRunAsErrorNeitherUserOrSpSpecified(t *testing.T) { @@ -218,11 +210,8 @@ func TestRunAsErrorNeitherUserOrSpSpecified(t *testing.T) { diags := bundle.Apply(ctx, b, mutator.SetRunAs()) err := diags.Error() - if runtime.GOOS == "windows" { - assert.EqualError(t, err, "run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at run_as\\not_allowed\\neither_sp_nor_user\\databricks.yml:4:8") - } else { - assert.EqualError(t, err, "run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at run_as/not_allowed/neither_sp_nor_user/databricks.yml:4:8") - } + configPath := filepath.FromSlash("run_as/not_allowed/neither_sp_nor_user/databricks.yml") + assert.EqualError(t, err, fmt.Sprintf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s:4:8", configPath)) } func TestRunAsErrorNeitherUserOrSpSpecifiedAtTargetOverride(t *testing.T) { @@ -241,9 +230,6 @@ func TestRunAsErrorNeitherUserOrSpSpecifiedAtTargetOverride(t *testing.T) { diags := bundle.Apply(ctx, b, mutator.SetRunAs()) err := diags.Error() - if runtime.GOOS == "windows" { - assert.EqualError(t, err, "run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at run_as\\not_allowed\\neither_sp_nor_user_override\\override.yml:4:12") - } else { - assert.EqualError(t, err, "run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at run_as/not_allowed/neither_sp_nor_user_override/override.yml:4:12") - } + configPath := filepath.FromSlash("run_as/not_allowed/neither_sp_nor_user_override/override.yml") + assert.EqualError(t, err, fmt.Sprintf("run_as section must specify exactly one identity. Neither service_principal_name nor user_name is specified at %s:4:12", configPath)) }