Support IS_OWNER as a top-level permission by lennartkats-db · Pull Request #1387 · databricks/cli

lennartkats-db · 2024-04-22T07:43:17Z

Changes

This adds a top-level IS_OWNER permission to help with collaborative deployment scenarios:

IS_OWNER is now accepted at the top level
Templates now include a top-level owner
Validation for mode: production now accepts a top-level owner as an alternative to a run_as identity.

Tests

Unit tests, manual experimentation.

codecov-commenter · 2024-04-22T07:46:43Z

Codecov Report

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

Project coverage is 53.63%. Comparing base (e22dd8a) to head (e4df0b9).
Report is 125 commits behind head on main.

Files	Patch %	Lines
bundle/permissions/apply_resource_permissions.go	96.77%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #1387 +/- ##
==========================================
+ Coverage 52.25% 53.63% +1.37% 
==========================================
Files 317 351 +34 Lines 18004 20290 +2286 ==========================================
+ Hits 9408 10882 +1474 - Misses 7903 8610 +707 - Partials 693 798 +105

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

lennartkats-db · 2024-06-02T12:58:55Z

- return diag.Errorf("'run_as' must be set for all jobs when using 'mode: production'")
+	// We need to verify that there is only a single deployment of the current target.
+	// A good way to enforce this is to explicitly set root_path or run_as.
+	if !isExplicitRootSet(b) && !isRunAsSet(r) {


We used to require
run_as: alice@company.com
but when considering collaborative deployment, we prefer
root_path: /Users/Alice
This change makes it so either restriction is allowed for mode: production.

Can we also warn if only run_as is set? It doesn't prevent multiple deployments.

lennartkats-db · 2024-06-02T13:00:03Z

+ // Only show a warning in case a principal was used for backward compatibility
+ // with projects from before the DABs GA.
+ if isPrincipalUsed {
+ return diag.Warningf("target with 'mode: production' should specify explicit 'workspace.root_path' to make sure only one copy is deployed")


Service principals still have an exception, like they used to.

lennartkats-db · 2024-06-03T06:51:08Z

+var allowedLevels = []string{CAN_MANAGE, CAN_VIEW, CAN_RUN, IS_OWNER}
 var levelsMap = map[string](map[string]string){
 	"jobs": {
+ IS_OWNER: "IS_OWNER",


Only a few resources, like jobs, actually have an "owner": https://docs.databricks.com/en/security/auth-authz/access-control/index.html. For almost all the others we don't distinguish between the owner and the other can-manage users.
Note that clusters is a special case here. They don't have an owner permission but treat the creator as a kind of owner that has special privileges. There's even a special API for changing that notion of "owner": https://docs.databricks.com/api/workspace/clusters/changeowner. Once we do clusters we should discuss if we want this notion of an owner to affect how clusters are created, or if we perhaps want to show a warning when someone who doesn't have IS_OWNER would be the first creator of a cluster.

lennartkats-db · 2024-07-11T13:37:56Z

@pietern could you take another look?

pietern · 2024-07-15T07:46:38Z

- return diag.Errorf("'run_as' must be set for all jobs when using 'mode: production'")
+	// We need to verify that there is only a single deployment of the current target.
+	// A good way to enforce this is to explicitly set root_path or run_as.
+	if !isExplicitRootSet(b) && !isRunAsSet(r) {


Can we also warn if only run_as is set? It doesn't prevent multiple deployments.

pietern · 2024-07-15T07:53:06Z

-	return &workspaceRootPermissions{}
+func ApplyFolderPermissions() bundle.Mutator {
+	return &applyFolderPermissions{}
 }


Why the rename? The mutator still applies only to the workspace root path.

The mutator still applies only to the workspace root path.
That seems like a bug :(
In any case I renamed this so apply_folder_permisions to make it a bit shorter and to reflect that it applies permissions to folders, while its sister mutator apply_resource_permissions applies permissions to resources.

Btw I don't think we should change the semantics of this module at this time, but we should include this in the upcoming work on permission warnings.

pietern · 2024-07-15T08:01:03Z

@andrewnester Could you take a look as well?

lennartkats-db · 2024-08-06T07:31:52Z

Let's have another brief live discussion about this before merging it @pietern

lennartkats-db · 2024-08-22T19:45:21Z

As discussed, we're going to take this work in a different direction. Closing this for now.

## Changes This updates the templates to include a `permissions` section. Having a permissions section is a best practice, is helpful to understand the notion of permissions, and helps diagnose permission errors (#1386). This is a cherry-pick from #1387. This change was verified to work both in dev and prod. Existing unit tests validate the validity of the templates in these modes.

…ent (#1712) ## Changes This updates `mode: production` to allow `root_path` to indicate uniqueness. Historically, we required `run_as` for this, which isn't actually very effective for that purpose. `run_as` also had the problem that it doesn't work for pipelines. This is a cherry-pick from #1387 --------- Co-authored-by: Pieter Noordhuis <pcnoordhuis@gmail.com>

## Changes This updates the templates to include a `permissions` section. Having a permissions section is a best practice, is helpful to understand the notion of permissions, and helps diagnose permission errors (#1386). This is a cherry-pick from #1387. This change was verified to work both in dev and prod. Existing unit tests validate the validity of the templates in these modes.

…ent (#1712) ## Changes This updates `mode: production` to allow `root_path` to indicate uniqueness. Historically, we required `run_as` for this, which isn't actually very effective for that purpose. `run_as` also had the problem that it doesn't work for pipelines. This is a cherry-pick from #1387 --------- Co-authored-by: Pieter Noordhuis <pcnoordhuis@gmail.com>

Support IS_OWNER as a top-level permission
2cce7f0

lennartkats-db commented Apr 22, 2024
View reviewed changes

Comment threadbundle/config/mutator/run_as_test.go Outdated

lennartkats-db added 2 commits April 22, 2024 15:42

WIP
86b7a85

Merge remote-tracking branch 'databricks/main' into cp-support-owners
b9a03ce

lennartkats-db commented May 16, 2024
View reviewed changes

Comment threadbundle/permissions/apply_folder_permissions.go Outdated

lennartkats-db added 4 commits May 27, 2024 21:04

Fix permissions levels
6f0df28

Restore original semantics for now
e0fd22a

Merge remote-tracking branch 'databricks/main' into cp-support-owners
739df2f

Cleanup
ab04468

lennartkats-db commented Jun 3, 2024
View reviewed changes

Fix test failure caused by changes in main
e4df0b9

lennartkats-db changed the title ~~[WIP] Support IS_OWNER as a top-level permission~~Support IS_OWNER as a top-level permissionJun 3, 2024

lennartkats-db requested review from andrewnester and pietern June 3, 2024 07:03

Merge remote-tracking branch 'databricks/main' into cp-support-owners
6c6aaa8

pietern reviewed Jul 15, 2024
View reviewed changes

Process PR feedback
ef6a43e

lennartkats-db force-pushed the cp-support-owners branch from bff5c98 to ef6a43eCompare July 29, 2024 19:35

lennartkats-db added 2 commits July 29, 2024 20:37

Clarify code
4df6fda

Merge remote-tracking branch 'databricks/main' into cp-support-owners
1870767

lennartkats-db mentioned this pull request Jul 31, 2024
Show actionable errors for collaborative deployment scenarios #1386
Merged

lennartkats-db added the do-not-merge label Aug 6, 2024

lennartkats-db closed this Aug 22, 2024

This was referenced Aug 22, 2024
Encourage the use of root_path in production to ensure single deployment #1712
Merged
Include a permissions section in all templates #1713
Merged

Uh oh!

Conversation

lennartkats-db commented Apr 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 Apr 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lennartkats-db commented Jul 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pietern commented Jul 15, 2024

Uh oh!

lennartkats-db commented Aug 6, 2024

Uh oh!

lennartkats-db commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lennartkats-db commented Apr 22, 2024•
edited
Loading

codecov-commenter commented Apr 22, 2024•
edited
Loading