Use SetPermissions instead of UpdatePermissions when setting folder permissions based on top-level ones #1822

andrewnester · 2024-10-10T12:15:07Z

Changes

Changed to use SetPermissions() to configure the permissions which remove other permissions on deployment folders.

Tests

Added unit test

bundle/permissions/workspace_root.go

lennartkats-db · 2024-10-11T15:30:25Z

bundle/permissions/workspace_root.go

+		rootPath += "/"
+	}
+
+	if !strings.HasPrefix(b.Config.Workspace.ArtifactPath, rootPath) {


Artifacts in /Volumes/ should get special treatment. But for this PR I'd just make sure that it doesn't fail for Volumes.

For followup considerations: Volumes rely on grants, not permissions. And they lack fine-grained permissions. Plus users often won't have permissions to change grants. But what we may be able to do is check if the grants are correct. Doing so might be the job for a "top-level grants" feature though, rather than the "top-level permissions" feature.

cc @pietern

bundle/permissions/workspace_root.go

bundle/libraries/workspace_path.go

bundle/permissions/workspace_root.go

pietern · 2024-10-24T08:32:59Z

bundle/permissions/workspace_root.go

+}
+
+func setPermissions(ctx context.Context, w workspace.WorkspaceInterface, path string, permissions []workspace.WorkspaceObjectAccessControlRequest) error {
+	obj, err := w.GetStatusByPath(ctx, path)


Is it guaranteed that all paths exist at this point?

If not, then this function should do a create-if-not-exists.

It's guaranteed because it's done after files.Upload

Does this also hold for the other paths?

@andrewnester Can you clarify this one?

E.g. does the state path or the resources path exist after files.Upload?

Yes, all of them will be created prior, the current mutator order is

libraries.Upload(), // creates artifacts path ... files.Upload(outputHandler), // creates files and resources path ... deploy.StatePush(), // creates state path permissions.ApplyWorkspaceRootPermissions(),

…ermissions based on top-level ones

outdated

bundle/paths/paths.go

pietern · 2024-10-29T10:46:19Z

Auto-merge won't kick in until the integration tests have run (enabled this past Friday).

You can trigger them either manually or by adding a commit.

eng-dev-ecosystem-bot · 2024-10-29T10:51:27Z

Test Details: go/deco-tests/11572324073

New features for Databricks Asset Bundles: This release adds support for managing AI/BI dashboards as part of your bundle configuration. The `bundle generate` command is updated to support producing dashboard bundle configuration as well as dashboard payloads. You can find an example configuration and walkthrough at https://github.com/databricks/bundle-examples/tree/main/knowledge_base/dashboard_nyc_taxi Bundles: * Add support for AI/BI dashboards ([#1743](#1743)). * Added validator for folder permissions ([#1824](#1824)). * Add bundle generate variant for dashboards ([#1847](#1847)). * Use SetPermissions instead of UpdatePermissions when setting folder permissions based on top-level ones ([#1822](#1822)). Internal: * Attempt to reduce test flakiness on Windows ([#1845](#1845)). * Reuse resource resolution code for the run command ([#1858](#1858)). * [Internal] Automatically trigger integration tests on PR ([#1857](#1857)). * Add privacy notice to README ([#1841](#1841)). * [Internal] Add test instructions for external contributors ([#1863](#1863)). * Add `libs/dyn/jsonsaver` ([#1862](#1862)). Dependency updates: * Bump github.com/fatih/color from 1.17.0 to 1.18.0 ([#1861](#1861)).

**New features for Databricks Asset Bundles:** This release adds support for managing AI/BI dashboards as part of your bundle configuration. The `bundle generate` command is updated to support producing dashboard bundle configuration as well as a serialized JSON representation of the dashboard. You can find an example configuration and walkthrough at https://github.com/databricks/bundle-examples/tree/main/knowledge_base/dashboard_nyc_taxi CLI: * Add privacy notice to README ([#1841](#1841)). Bundles: * Add support for AI/BI dashboards ([#1743](#1743)). * Added validator for folder permissions ([#1824](#1824)). * Add bundle generate variant for dashboards ([#1847](#1847)). * Use SetPermissions instead of UpdatePermissions when setting folder permissions based on top-level ones ([#1822](#1822)). Internal: * Attempt to reduce test flakiness on Windows ([#1845](#1845)). * Reuse resource resolution code for the run command ([#1858](#1858)). * [Internal] Automatically trigger integration tests on PR ([#1857](#1857)). * [Internal] Add test instructions for external contributors ([#1863](#1863)). * Add `libs/dyn/jsonsaver` ([#1862](#1862)). Dependency updates: * Bump github.com/fatih/color from 1.17.0 to 1.18.0 ([#1861](#1861)). --------- Co-authored-by: Pieter Noordhuis <[email protected]>

moritzmeister · 2024-10-30T16:19:10Z

Hi @andrewnester @pietern ,
it looks like this has broken our existing bundle deployments.
We are now always getting the following errors, but only on "mode: production" type bundle targets:

15:53:42 DEBUG Apply pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=seq mutator=ApplyWorkspaceRootPermissions
15:53:42 DEBUG GET /api/2.0/workspace/get-status?path=/Shared/.bundle/wombat/prod/resources
< HTTP/2.0 404 Not Found
< {
<   "error_code": "RESOURCE_DOES_NOT_EXIST",
<   "message": "Path (/Shared/.bundle/wombat/prod/resources) doesn't exist."
< } pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=seq mutator=ApplyWorkspaceRootPermissions sdk=true
15:53:42 DEBUG non-retriable error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist. pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=seq mutator=ApplyWorkspaceRootPermissions sdk=true
15:53:42 ERROR Error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist. pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=seq mutator=ApplyWorkspaceRootPermissions
15:53:42 ERROR Error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist. pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=seq
15:53:42 DEBUG Apply pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=lock:release
15:53:42  INFO Releasing deployment lock pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=lock:release
15:53:42 DEBUG GET /api/2.0/workspace/get-status?path=/Workspace/Shared/.bundle/wombat/prod/state/deploy.lock&return_export_info=true
< HTTP/2.0 200 OK
< {
<   "created_at": 1730303605917,
<   "modified_at": 1730303605917,
<   "object_id": sfgssdfg,
<   "object_type": "FILE",
<   "path": "/Workspace/Shared/.bundle/wombat/prod/state/deploy.lock",
<   "resource_id": "fetch"
< } pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=lock:release sdk=true
15:53:42 DEBUG GET /api/2.0/workspace/get-status?path=/Workspace/Shared/.bundle/wombat/prod/state/deploy.lock&return_export_info=true
< HTTP/2.0 200 OK
< {
<   "created_at": 1730303605917,
<   "modified_at": 1730303605917,
<   "object_id": sfgsdfg,
<   "object_type": "FILE",
<   "path": "/Workspace/Shared/.bundle/wombat/prod/state/deploy.lock",
<   "resource_id": "gee Tee Seth"
< } pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=lock:release sdk=true
15:53:43 DEBUG GET /api/2.0/workspace/export?direct_download=true&path=/Workspace/Shared/.bundle/wombat/prod/state/deploy.lock
< HTTP/2.0 200 OK
< <Streaming response> pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=lock:release sdk=true
15:53:43 DEBUG POST /api/2.0/workspace/delete
> {
>   "path": "/Workspace/Shared/.bundle/wombat/prod/state/deploy.lock"
> }
< HTTP/2.0 200 OK
< {} pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred mutator=lock:release sdk=true
15:53:43 ERROR Error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist. pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq mutator=deferred
15:53:43 ERROR Error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist. pid=1549 mutator=seq mutator=deploy mutator=seq mutator=seq
15:53:43 ERROR Error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist. pid=1549 mutator=seq mutator=deploy mutator=seq
15:53:43 ERROR Error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist. pid=1549 mutator=seq mutator=deploy
15:53:43 ERROR Error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist. pid=1549 mutator=seq
Warning: the bundle root path /Workspace/Shared/.bundle/wombat/prod is writable by all workspace users

The bundle is configured to use /Workspace/Shared, which will give read/write access to all users. If this is intentional, add CAN_MANAGE for 'group_name: users' permission to your bundle configuration. If the deployment should be restricted, move it to a restricted folder such as /Workspace/Users/<username or principal name>.

Error: Path (/Shared/.bundle/wombat/prod/resources) doesn't exist.

15:53:43 ERROR failed execution pid=1549 exit_code=1 error=AlreadyPrinted

It looks like it's checking for some resources directory, which we don't have, I am not sure if that's because we have the resource yml files in a subdirectory, not on the same level as databricks.yml. Our directory structure looks basically like this:

databricks.yml
project1/resources/job1.yml

pietern · 2024-10-30T16:43:19Z

@moritzmeister Thanks for reporting. We're looking into it.

pietern · 2024-10-30T16:51:22Z

@moritzmeister You can work around this issue by prefixing the workspace.root_path setting with /Workspace.

It would look something like:

workspace:
  root_path: /Workspace/Shared/...

We're working on a patch release as well.

moritzmeister · 2024-10-30T16:52:53Z

Thanks for the quick reply, will test the workaround later tonight!

## Changes `/Workspace` prefix needs to be added to `resource_path` as well. Fixes the issue mentioned here: #1822 (comment) Fixes #1867 ## Tests Added regression test

andrewnester · 2024-10-30T18:20:43Z

@moritzmeister The patch release containing the fix (0.232.1) was just released, please upgrade and see if it fixes your issue

moritzmeister · 2024-10-30T20:21:24Z

Thanks for the quick turnaround, this seems to work. Or at least getting a different error now 🤣 but that one seems to be my fault.

andrewnester requested review from pietern and lennartkats-db October 10, 2024 12:15

pietern reviewed Oct 10, 2024

View reviewed changes

bundle/permissions/workspace_root.go Outdated Show resolved Hide resolved

andrewnester requested a review from pietern October 10, 2024 13:25

pietern reviewed Oct 10, 2024

View reviewed changes

bundle/permissions/workspace_root.go Outdated Show resolved Hide resolved

bundle/permissions/workspace_root.go Outdated Show resolved Hide resolved

bundle/permissions/workspace_root.go Show resolved Hide resolved

lennartkats-db previously requested changes Oct 11, 2024

View reviewed changes

andrewnester requested review from pietern and lennartkats-db October 22, 2024 14:28

pietern reviewed Oct 24, 2024

View reviewed changes

andrewnester added 4 commits October 24, 2024 18:25

Use SetPermissions instead of UpdatePermissions when setting folder p…

ca1ddbf

…ermissions based on top-level ones

fixes

99fbf33

fixes

a51c573

reuse

16f02c0

andrewnester force-pushed the fix/update-to-set-permissions branch from c45c441 to 16f02c0 Compare October 24, 2024 16:39

andrewnester enabled auto-merge October 24, 2024 16:39

andrewnester requested a review from pietern October 24, 2024 16:40

pietern approved these changes Oct 29, 2024

View reviewed changes

bundle/paths/paths.go Outdated Show resolved Hide resolved

changed name

6659e57

andrewnester temporarily deployed to test-trigger-is October 29, 2024 10:50 — with GitHub Actions Inactive

andrewnester added this pull request to the merge queue Oct 29, 2024

Merged via the queue into main with commit f018daf Oct 29, 2024
8 checks passed

andrewnester deleted the fix/update-to-set-permissions branch October 29, 2024 12:14

andrewnester mentioned this pull request Oct 30, 2024

[Release] Release v0.232.0 #1865

Merged

andrewnester mentioned this pull request Oct 30, 2024

Fixed adding /Workspace prefix for resource paths #1866

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SetPermissions instead of UpdatePermissions when setting folder permissions based on top-level ones #1822

Use SetPermissions instead of UpdatePermissions when setting folder permissions based on top-level ones #1822

andrewnester commented Oct 10, 2024

lennartkats-db Oct 11, 2024

pietern Oct 24, 2024

andrewnester Oct 24, 2024

pietern Oct 24, 2024

pietern Oct 29, 2024

andrewnester Oct 29, 2024

pietern commented Oct 29, 2024

eng-dev-ecosystem-bot commented Oct 29, 2024

moritzmeister commented Oct 30, 2024 •

edited

Loading

pietern commented Oct 30, 2024

pietern commented Oct 30, 2024

moritzmeister commented Oct 30, 2024

andrewnester commented Oct 30, 2024 •

edited by pietern

Loading

moritzmeister commented Oct 30, 2024

Use SetPermissions instead of UpdatePermissions when setting folder permissions based on top-level ones #1822

Use SetPermissions instead of UpdatePermissions when setting folder permissions based on top-level ones #1822

Conversation

andrewnester commented Oct 10, 2024

Changes

Tests

lennartkats-db Oct 11, 2024

Choose a reason for hiding this comment

pietern Oct 24, 2024

Choose a reason for hiding this comment

andrewnester Oct 24, 2024

Choose a reason for hiding this comment

pietern Oct 24, 2024

Choose a reason for hiding this comment

pietern Oct 29, 2024

Choose a reason for hiding this comment

andrewnester Oct 29, 2024

Choose a reason for hiding this comment

pietern commented Oct 29, 2024

eng-dev-ecosystem-bot commented Oct 29, 2024

moritzmeister commented Oct 30, 2024 • edited Loading

pietern commented Oct 30, 2024

pietern commented Oct 30, 2024

moritzmeister commented Oct 30, 2024

andrewnester commented Oct 30, 2024 • edited by pietern Loading

moritzmeister commented Oct 30, 2024

moritzmeister commented Oct 30, 2024 •

edited

Loading

andrewnester commented Oct 30, 2024 •

edited by pietern

Loading