Add allow list for resources when bundle `run_as` is set by shreyas-goenka · Pull Request #1233 · databricks/cli

shreyas-goenka · 2024-02-22T17:42:49Z

Changes

Note: This is a breaking change. Users will start seeing their deployments start failing if they satisfy the following conditions:

The user deploying the DAB is a workspace / metastore admin.
The DABs has a run_as identity set
The DAB contains a DLT pipeline or a model serving endpoint. This is not a breaking change if the customer DAB does not contain one of those.

This PR introduces an allow list for resource types that are allowed when the run_as for the bundle is not the same as the current deployment user.

This PR also adds a test to ensure that any new resources added to DABs will have to add the resource to either the allow list or add an error to fail when run_as identity is not the same as deployment user.

Tests

Unit tests

codecov-commenter · 2024-03-07T13:51:55Z

Codecov Report

Attention: Patch coverage is 58.49057% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 52.39%. Comparing base (c781856) to head (dcdb87f).
Report is 2 commits behind head on main.

Files	Patch %	Lines
bundle/config/mutator/run_as.go	65.95%	13 Missing and 3 partials ⚠️
bundle/config/root.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #1233 +/- ##
==========================================
+ Coverage 52.24% 52.39% +0.15% 
==========================================
Files 317 317 Lines 18001 18039 +38 ==========================================
+ Hits 9405 9452 +47 + Misses 7903 7889 -14 - Partials 693 698 +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewnester

See comments on usage of dynamic mutations

pietern · 2024-03-27T12:21:03Z

+ currentUser: b.Config.Workspace.CurrentUser.UserName,
+ runAsUser: identity,
+ }
+	}


Can we use an allow list instead?

Not unless we use reflect / dynamic configuration. The allow list semantics are being captured in the unit test though.

CLI: * Added `auth describe` command ([#1244](#1244)). * Fixed message for successful auth describe run ([#1336](#1336)). Bundles: * Make bundle validation print text output by default ([#1335](#1335)). * Use UserName field to identify if service principal is used ([#1310](#1310)). * Allow unknown properties in the config file for template initialization ([#1315](#1315)). * Remove support for DATABRICKS_BUNDLE_INCLUDES ([#1317](#1317)). * Make `bundle.deployment` optional in the bundle schema ([#1321](#1321)). * Add allow list for resources when bundle `run_as` is set ([#1233](#1233)). * Fix the generated DABs JSON schema ([#1322](#1322)). * Make bundle loaders return diagnostics ([#1319](#1319)). * Add `bundle debug terraform` command ([#1294](#1294)). * Allow specifying CLI version constraints required to run the bundle ([#1320](#1320)). Internal: * Retain location information of variable reference ([#1333](#1333)). * Define `dyn.Mapping` to represent maps ([#1301](#1301)). * Return `diag.Diagnostics` from mutators ([#1305](#1305)). * Fix flaky test in `libs/process` ([#1314](#1314)). * Move path field to bundle type ([#1316](#1316)). * Load bundle configuration from mutator ([#1318](#1318)). * Return diagnostics from `config.Load` ([#1324](#1324)). * Return warning for nil primitive types during normalization ([#1329](#1329)). * Include `dyn.Path` in normalization warnings and errors ([#1332](#1332)). * Make normalization return warnings instead of errors ([#1334](#1334)). API Changes: * Added `databricks lakeview migrate` command. * Added `databricks lakeview unpublish` command. * Changed `databricks ip-access-lists get` command . New request type is . OpenAPI commit e316cc3d78d087522a74650e26586088da9ac8cb (2024-04-03) Dependency updates: * Bump github.com/databricks/databricks-sdk-go from 0.36.0 to 0.37.0 ([#1326](#1326)).

Breaking Change: * Add allow list for resources when bundle `run_as` is set ([#1233](#1233)). * Make bundle validation print text output by default ([#1335](#1335)). CLI: * Added `auth describe` command ([#1244](#1244)). * Fixed message for successful auth describe run ([#1336](#1336)). Bundles: * Use UserName field to identify if service principal is used ([#1310](#1310)). * Allow unknown properties in the config file for template initialization ([#1315](#1315)). * Remove support for DATABRICKS_BUNDLE_INCLUDES ([#1317](#1317)). * Make `bundle.deployment` optional in the bundle schema ([#1321](#1321)). * Fix the generated DABs JSON schema ([#1322](#1322)). * Make bundle loaders return diagnostics ([#1319](#1319)). * Add `bundle debug terraform` command ([#1294](#1294)). * Allow specifying CLI version constraints required to run the bundle ([#1320](#1320)). Internal: * Retain location information of variable reference ([#1333](#1333)). * Define `dyn.Mapping` to represent maps ([#1301](#1301)). * Return `diag.Diagnostics` from mutators ([#1305](#1305)). * Fix flaky test in `libs/process` ([#1314](#1314)). * Move path field to bundle type ([#1316](#1316)). * Load bundle configuration from mutator ([#1318](#1318)). * Return diagnostics from `config.Load` ([#1324](#1324)). * Return warning for nil primitive types during normalization ([#1329](#1329)). * Include `dyn.Path` in normalization warnings and errors ([#1332](#1332)). * Make normalization return warnings instead of errors ([#1334](#1334)). API Changes: * Added `databricks lakeview migrate` command. * Added `databricks lakeview unpublish` command. * Changed `databricks ip-access-lists get` command . New request type is . OpenAPI commit e316cc3d78d087522a74650e26586088da9ac8cb (2024-04-03) Dependency updates: * Bump github.com/databricks/databricks-sdk-go from 0.36.0 to 0.37.0 ([#1326](#1326)).

pernilak · 2024-04-18T15:07:53Z

Does this mean that all dlt pipelines are always runned by whomever created that pipeline?

andrewnester · 2024-04-18T16:14:35Z

@pernilak yes, that's correct, a pipeline is run by whoever is set as an owner for this pipeline.

## Changes This PR partially reverts the changes in #1233 and puts the old code under an "experimental.use_legacy_run_as" configuration. This gives customers who ran into the breaking change made in the PR a way out. ## Tests Both manually and via unit tests. Manually verified that run_as works for pipelines now. And if a user wants to use the feature they need to be both a Metastore and a workspace admin. --------- Error when the deploying user is a workspace admin but not a metastore admin: ``` Error: terraform apply: exit status 1 Error: cannot update permissions: User is not a metastore admin for Metastore 'deco-uc-prod-aws-us-east-1'. with databricks_permissions.pipeline_foo, on bundle.tf.json line 23, in resource.databricks_permissions.pipeline_foo: 23: } ``` -------- Output of bundle validate: ``` ➜ bundle-playground git:(master) ✗ cli bundle validate Warning: You are using the legacy mode of run_as. The support for this mode is experimental and might be removed in a future release of the CLI. In order to run the DLT pipelines in your DAB as the run_as user this mode changes the owners of the pipelines to the run_as identity, which requires the user deploying the bundle to be a workspace admin, and also a Metastore admin if the pipeline target is in UC. at experimental.use_legacy_run_as in databricks.yml:13:22 Name: bundle-playground Target: default Workspace: Host: https://dbc-a39a1eb1-ef95.cloud.databricks.com User: shreyas.goenka@databricks.com Path: /Users/shreyas.goenka@databricks.com/.bundle/bundle-playground/default Found 1 warning ```

## Changes This PR introduces an allow list for resource types that are allowed when the run_as for the bundle is not the same as the current deployment user. This PR also adds a test to ensure that any new resources added to DABs will have to add the resource to either the allow list or add an error to fail when run_as identity is not the same as deployment user. ## Tests Unit tests

Breaking Change: * Add allow list for resources when bundle `run_as` is set ([#1233](#1233)). * Make bundle validation print text output by default ([#1335](#1335)). CLI: * Added `auth describe` command ([#1244](#1244)). * Fixed message for successful auth describe run ([#1336](#1336)). Bundles: * Use UserName field to identify if service principal is used ([#1310](#1310)). * Allow unknown properties in the config file for template initialization ([#1315](#1315)). * Remove support for DATABRICKS_BUNDLE_INCLUDES ([#1317](#1317)). * Make `bundle.deployment` optional in the bundle schema ([#1321](#1321)). * Fix the generated DABs JSON schema ([#1322](#1322)). * Make bundle loaders return diagnostics ([#1319](#1319)). * Add `bundle debug terraform` command ([#1294](#1294)). * Allow specifying CLI version constraints required to run the bundle ([#1320](#1320)). Internal: * Retain location information of variable reference ([#1333](#1333)). * Define `dyn.Mapping` to represent maps ([#1301](#1301)). * Return `diag.Diagnostics` from mutators ([#1305](#1305)). * Fix flaky test in `libs/process` ([#1314](#1314)). * Move path field to bundle type ([#1316](#1316)). * Load bundle configuration from mutator ([#1318](#1318)). * Return diagnostics from `config.Load` ([#1324](#1324)). * Return warning for nil primitive types during normalization ([#1329](#1329)). * Include `dyn.Path` in normalization warnings and errors ([#1332](#1332)). * Make normalization return warnings instead of errors ([#1334](#1334)). API Changes: * Added `databricks lakeview migrate` command. * Added `databricks lakeview unpublish` command. * Changed `databricks ip-access-lists get` command . New request type is . OpenAPI commit e316cc3d78d087522a74650e26586088da9ac8cb (2024-04-03) Dependency updates: * Bump github.com/databricks/databricks-sdk-go from 0.36.0 to 0.37.0 ([#1326](#1326)).

## Changes This PR partially reverts the changes in #1233 and puts the old code under an "experimental.use_legacy_run_as" configuration. This gives customers who ran into the breaking change made in the PR a way out. ## Tests Both manually and via unit tests. Manually verified that run_as works for pipelines now. And if a user wants to use the feature they need to be both a Metastore and a workspace admin. --------- Error when the deploying user is a workspace admin but not a metastore admin: ``` Error: terraform apply: exit status 1 Error: cannot update permissions: User is not a metastore admin for Metastore 'deco-uc-prod-aws-us-east-1'. with databricks_permissions.pipeline_foo, on bundle.tf.json line 23, in resource.databricks_permissions.pipeline_foo: 23: } ``` -------- Output of bundle validate: ``` ➜ bundle-playground git:(master) ✗ cli bundle validate Warning: You are using the legacy mode of run_as. The support for this mode is experimental and might be removed in a future release of the CLI. In order to run the DLT pipelines in your DAB as the run_as user this mode changes the owners of the pipelines to the run_as identity, which requires the user deploying the bundle to be a workspace admin, and also a Metastore admin if the pipeline target is in UC. at experimental.use_legacy_run_as in databricks.yml:13:22 Name: bundle-playground Target: default Workspace: Host: https://dbc-a39a1eb1-ef95.cloud.databricks.com User: shreyas.goenka@databricks.com Path: /Users/shreyas.goenka@databricks.com/.bundle/bundle-playground/default Found 1 warning ```

shreyas-goenka added 8 commits February 5, 2024 15:42

wip
9adc6e6

Merge remote-tracking branch 'origin' into fix-run-as
39c1f08

Merge remote-tracking branch 'origin' into fix-run-as
76fc145

Merge remote-tracking branch 'origin' into fix-run-as
b2450c1

wip
4da90f2

further along the way
05c9d38

Merge remote-tracking branch 'origin' into fix-run-as
66f288d

add test for when both sp and user are defined
335d0c9

shreyas-goenka changed the title ~~[WIP] Add allow list for resources when bundle run_as is set~~Add allow list for resources when bundle run_as is setMar 7, 2024

shreyas-goenka added 3 commits March 7, 2024 14:40

fix test
776af33

final cleanup
20a0274

fix test
5886a65

shreyas-goenka requested review from andrewnester and pietern March 7, 2024 13:52

fix windows tests
f2d24fc

andrewnester requested changes Mar 11, 2024
View reviewed changes

shreyas-goenka added 2 commits March 11, 2024 21:22

Merge remote-tracking branch 'origin' into fix-run-as
61f08cc

rename vars
a4cc92d

pietern reviewed Mar 11, 2024
View reviewed changes

pietern requested review from andrewnester and pietern March 11, 2024 21:55

andrewnester reviewed Mar 12, 2024
View reviewed changes

Comment threadbundle/config/mutator/run_as.go Outdated
Comment threadbundle/config/mutator/run_as.go Outdated
Comment threadbundle/config/mutator/run_as.go Outdated
Comment threadbundle/config/mutator/run_as.go Outdated
Comment threadbundle/config/mutator/run_as.go Outdated

shreyas-goenka added 5 commits March 13, 2024 15:32

use typed configuration instead
e7416ef

fix tests
aafd767

cleanup
9d6949a

add error for when neither are specified
fab62f3

lint
89c86c8

shreyas-goenka requested review from andrewnester and lennartkats-db March 13, 2024 16:08

shreyas-goenka added 5 commits March 13, 2024 17:08

-
dcdb87f

comment refine
9c2bfc6

Merge remote-tracking branch 'origin' into fix-run-as
a83453b

merge diagnostics changes
e1e5a36

more diagnostics
98f8be2

andrewnester reviewed Mar 27, 2024
View reviewed changes

Comment threadbundle/config/root.go Outdated

andrewnester self-requested a review March 27, 2024 10:57

shreyas-goenka added 2 commits March 27, 2024 13:18

Merge remote-tracking branch 'origin' into fix-run-as
d28ce06

TryLocation -> GetLocation
9e9e18a

pietern reviewed Mar 27, 2024
View reviewed changes

shreyas-goenka added 2 commits March 27, 2024 16:11

address comments about comments
dcaf247

remove branching for windows assertions
a50117e

andrewnester approved these changes Mar 27, 2024
View reviewed changes

shreyas-goenka added this pull request to the merge queue Mar 27, 2024

Merged via the queue into main with commit 5df4c7eMar 27, 2024

shreyas-goenka deleted the fix-run-as branch March 27, 2024 16:23

andrewnester mentioned this pull request Apr 3, 2024
Release v0.217.0 #1337
Merged

shreyas-goenka mentioned this pull request Apr 22, 2024
Add legacy option for run_as#1384
Merged

shreyas-goenka mentioned this pull request May 22, 2024
bundle validate fails when deployment user != run_as user #1448
Closed

Uh oh!

Conversation

shreyas-goenka commented Feb 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

codecov-commenter commented Mar 7, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andrewnester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pieternMar 27, 2024

Choose a reason for hiding this comment

Uh oh!

shreyas-goenkaMar 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pernilak commented Apr 18, 2024

Uh oh!

andrewnester commented Apr 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shreyas-goenka commented Feb 22, 2024•
edited
Loading

codecov-commenter commented Mar 7, 2024•
edited
Loading