-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add allow list for resources when bundle run_as
is set
#1233
Conversation
run_as
is setrun_as
is set
Codecov ReportAttention: Patch coverage is
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments on usage of dynamic mutations
currentUser: b.Config.Workspace.CurrentUser.UserName, | ||
runAsUser: identity, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use an allow list instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not unless we use reflect / dynamic configuration. The allow list semantics are being captured in the unit test though.
bundle/tests/run_as/not_allowed/neither_sp_nor_user_override/override.yml
Show resolved
Hide resolved
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)).
Does this mean that all dlt pipelines are always runned by whomever created that pipeline? |
@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
Note: This is a breaking change. Users will start seeing their deployments start failing if they satisfy the following conditions:
run_as
identity setThis 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