[Fix] Refactor `databricks_permissions` and allow the current user to set their own permissions by mgyucht · Pull Request #3956 · databricks/terraform-provider-databricks

mgyucht · 2024-08-27T15:18:27Z

Changes

In c441517, a check was added to prevent users from assigning any permissions for themselves in databricks_permissions. This unfortunately makes it impossible for users to legitimately assign themselves as the owner of certain resources, such as jobs, if they are currently owned by a different principal. This PR removes this unnecessary restriction. If the user requests to set permissions for an object in a way that is incompatible with the object, such as removing themselves as owner, the failure will be propagated from the backend to the user instead. This does not make any changes to the way the ownership ACLs are set up (e.g. for resources that require an owner, like jobs, if the Terraform user did not specify an owner, the provider will still set the current user as the owner).

This PR also refactors the permissions resource substantially. The logic for implementing each resource type's permissions, including the field name, object type and resource-specific modifications, are all colocated with the resource's own definition. The type encapsulating this is calledresourcePermissions. As a result, the control flow is easier to follow:

Read reads from the API, customizes the response in a resource-specific way, maps the response to the TF state representation, and stores, or marks as deleted if there are no permissions.
Create and Update read the desired permissions from ResourceData, perform some validation, perform resource-specific, then puts the update with an owner if not specified.
Delete resets the ACLs to only admins + resource-specific customizations.

Customizations are defined in the permissions/read and permissions/update packages. For update, a mini expression language is defined to support conditional application of customizations.

Lastly, this PR also migrates the resource to the Databricks SDK.

Fixes #2407.

Tests

This PR adds integration test coverage for the databricks_permissions resource for nearly all supported resource types. I wasn't able to run the integration test for authorization = "passwords" because password-based auth is deprecated, nor for serving endpoints because of a likely race condition.

Integration tests cover all permission levels, and all principal types. Included is special edge case testing for root directory and all registered models.

make test run locally
relevant change in docs/ folder
covered with integration tests in internal/acceptance
relevant acceptance tests are passing
using Go SDK

mgyucht · 2024-09-20T14:38:19Z

permissions/resource_permissions_test.go

-	}.ExpectError(t, "permission_level WHATEVER is not supported with cluster_id objects")
-}
-
-func TestResourcePermissionsCustomizeDiff_ErrorOnPermissionsDecreate(t *testing.T) {


This behavior is changed in this PR: users are allowed to change their permissions as long as they maintain management-level permissions.

mgyucht · 2024-09-20T14:38:55Z

permissions/resource_permissions_test.go

 						{
 							UserName:        TestingAdminUser,
-							PermissionLevel: "IS_OWNER",
+							PermissionLevel: "CAN_MANAGE",


Order of permissions won't affect the behavior in the API.

tanmay-db

LGTM, had some comments.

tanmay-db · 2024-09-25T13:03:57Z

permissions/resource_permissions.go

-}
-
-// safePutWithOwner is a workaround for the limitation where warehouse without owners cannot have IS_OWNER set
-func (a PermissionsAPI) safePutWithOwner(objectID string, objectACL AccessControlChangeList, originalAcl []AccessControlChange) error {


Is this something we should also have in Go SDK and hence might be better to move it there?

TBH I don't think it really belongs in the Go SDK. I would actually consider this more along the lines of an API issue for the permissions API for warehouses. This may not even be needed anymore now that warehouses and other SQL objects are using the global permissions API endpoints, but I need to test it out.

tanmay-db · 2024-09-25T13:05:04Z

permissions/resource_permissions.go

+// Delete gracefully removes permissions of non-admin users. After this operation, the object is managed
+// by the current user and admin group. If the resource has IS_OWNER permissions, they are reset to the
+// object creator, if it can be determined.
+func (a PermissionsAPI) Delete(objectID string, mapping resourcePermissions) error {


Same for this, if true -- we could move the whole PermissionsAPI there. Currently, are SDK users expected to take care of this on their own?

Delete permissions are fairly Terraform-specific since they define the meaning of "removing a databricks_permissions resource." Generally speaking, I would like to see more evidence for reusability of such a method before adding it to the SDK, especially when there are other methods that provide equivalent power.

tanmay-db · 2024-09-25T13:09:12Z

permissions/acl_customizers.go

+	}
+}
+
+// Whether the object requires explicit manage permissions for the calling user if not set.


Thanks for the comments, easy to get context about this.

tanmay-db · 2024-09-25T13:14:06Z

permissions/acl_customizers.go

+			filteredAcl = append(filteredAcl, a)
+		}
+	}
+	acl.AccessControlList = filteredAcl


We are modifying the input and returning that, I think if we want to modify the input then returning it is redundant (since it's already passed by reference) and if not (which is more safer since we don't modify the input acl which preserves the information about admins group (if needed)) we could return a new acl.

Also since we are always returning nil as error in current implementation -- should we remove returning it / add error pathway?

Good suggestion. I can change this method to return a new ObjectPermissions instance and not modify the original one.

We could theoretically get rid of the error pathway, as it isn't used now, but I've kept it for symmetry. I suppose we can add it later if we need to add customizations in the future which could fail.

permissions/acl_customizers.go

permissions/permission_definitions.go

permissions/resource_permissions.go

nkvuong

just a few minor comments, I will run a few test locally as well to confirm

### New Features and Improvements * Add `databricks_budget` resource ([#3955](#3955)). * Add `databricks_mlflow_models` data source ([#3874](#3874)). * Add computed attribute `table_serving_url` to `databricks_online_table` ([#4048](#4048)). * Add support for Identity Column in `databricks_sql_table` ([#4035](#4035)). ### Bug Fixes * Fix Permissions Dashboard Test ([#4071](#4071)). * Ignore presence or absence of `/Workspace` prefix for dashboard resource ([#4061](#4061)). * Refactor `databricks_permissions` and allow the current user to set their own permissions ([#3956](#3956)). * Set ID for online table resource if creation succeeds but it isn't available yet ([#4072](#4072)). ### Documentation * Add guide for OIDC authentication ([#4016](#4016)). * Correctly use native markdown callouts supported by TF Registry ([#4073](#4073)). * Fixing links to `databricks_service_principal` in TF guides ([#4020](#4020)). ### Internal Changes * Bump Go SDK to latest and generate TF structs ([#4062](#4062)). * Skip Budget tests on GCP ([#4070](#4070)). * Update to latest OpenAPI spec and bump Go SDK ([#4069](#4069)).

### New Features and Improvements * Add `databricks_budget` resource ([#3955](#3955)). * Add `databricks_mlflow_models` data source ([#3874](#3874)). * Add computed attribute `table_serving_url` to `databricks_online_table` ([#4048](#4048)). * Add support for Identity Column in `databricks_sql_table` ([#4035](#4035)). ### Bug Fixes * Add Sufficient Network Privileges to the Databricks Default Cross Account Policy ([#4027](#4027)) * Ignore presence or absence of `/Workspace` prefix for dashboard resource ([#4061](#4061)). * Refactor `databricks_permissions` and allow the current user to set their own permissions ([#3956](#3956)). * Set ID for online table resource if creation succeeds but it isn't available yet ([#4072](#4072)). ### Documentation * Update CONTRIBUTING guide for plugin framework resources ([#4078](#4078)) * Add guide for OIDC authentication ([#4016](#4016)). * Correctly use native markdown callouts supported by TF Registry ([#4073](#4073)). * Fixing links to `databricks_service_principal` in TF guides ([#4020](#4020)). ### Internal Changes * Fix Permissions Dashboard Test ([#4071](#4071)). * Bump Go SDK to latest and generate TF structs ([#4062](#4062)). * Skip Budget tests on GCP ([#4070](#4070)). * Update to latest OpenAPI spec and bump Go SDK ([#4069](#4069)).

NicholasFiorentini · 2024-10-24T14:08:28Z

I suspect this change has introduced a regression. See #4143.

…with `/warehouses/...` IDs (#4158) ## Changes #4143 reported a regression to the `databricks_permissions` resource caused by #3956. Normally, the ID for this resource when configured for a SQL warehouse is `/sql/warehouses/<ID>`. However, it seems like at some point in the past, some users may have had an ID of `/warehouses/<ID>`. It's possible that importing this resource worked like this: when calling the permissions REST API, whether using object type `sql/warehouses` or `warehouses`, the API returns permissions for the same resources: ``` 15:13:01 DEBUG GET /api/2.0/permissions/sql/warehouses/<ID> < HTTP/2.0 200 OK < { < "access_control_list": [ < { < "all_permissions": [ < { < "inherited": false, < "permission_level": "IS_OWNER" < } < ], < "display_name": "<ME>", < "user_name": "<ME>" < }, < { < "all_permissions": [ < { < "inherited": true, < "inherited_from_object": [ < "/sql/warehouses/" < ], < "permission_level": "CAN_MANAGE" < } < ], < "group_name": "admins" < } < ], < "object_id": "/sql/warehouses/<ID>", < "object_type": "warehouses" < } pid=53287 sdk=true ... 15:12:56 DEBUG GET /api/2.0/permissions/warehouses/<ID> < HTTP/2.0 200 OK < { < "access_control_list": [ < { < "all_permissions": [ < { < "inherited": false, < "permission_level": "IS_OWNER" < } < ], < "display_name": "<ME>", < "user_name": "<ME>" < }, < { < "all_permissions": [ < { < "inherited": true, < "inherited_from_object": [ < "/sql/warehouses/" < ], < "permission_level": "CAN_MANAGE" < } < ], < "group_name": "admins" < } < ], < "object_id": "/sql/warehouses/<ID>", < "object_type": "warehouses" < } pid=53248 sdk=true ``` This PR modifies the SQL warehouse configuration for `databricks_permissions` to be chosen for instances with an ID of the form `/warehouses/...`. ## Tests The additional integration test ensures that a resource can be imported with the `/warehouses/<ID>` format.

mgyucht added 2 commits August 27, 2024 15:57

Allow databricks_permissions to set permissions for the current user

ca45b53

better integration test and docs

d457710

mgyucht marked this pull request as ready for review August 27, 2024 15:18

mgyucht requested review from a team as code owners August 27, 2024 15:18

mgyucht requested review from hectorcast-db and removed request for a team August 27, 2024 15:18

mgyucht added 15 commits September 18, 2024 13:42

Merge branch 'main' into issue-2407

6407156

big refactor

74d5d82

address

7ef4cf6

clearer naming

600a63e

everything is contained in resourcePermissions

1a8428c

move things around a bit

8e14e36

better

92ebb61

fmt

a5f9189

fixes

74e033d

fix anyways

d1acfa1

some fixes

73118c5

some work

332423e

fixes

3d292b7

tweaks

450935f

work

4f4abee

mgyucht changed the title ~~[Fix] Allow databricks_permissions to set permissions for the current user~~ [Fix][WIP] Allow databricks_permissions to set permissions for the current user Sep 19, 2024

mgyucht added the do-not-merge label Sep 19, 2024

mgyucht changed the title ~~[Fix][WIP] Allow databricks_permissions to set permissions for the current user~~ [Fix] [WIP] Allow databricks_permissions to set permissions for the current user Sep 19, 2024

mgyucht added 4 commits September 20, 2024 15:59

add tests

974db83

fixes

3aee4dd

clean

576baba

comments

4382548

mgyucht commented Sep 20, 2024

View reviewed changes

mgyucht removed the do-not-merge label Sep 25, 2024

tanmay-db approved these changes Sep 25, 2024

View reviewed changes

mgyucht added 2 commits September 25, 2024 15:50

address comments

932f2b5

fix some tests

e28e1ad

nkvuong reviewed Sep 26, 2024

View reviewed changes

permissions/acl_customizers.go Outdated Show resolved Hide resolved

permissions/permission_definitions.go Outdated Show resolved Hide resolved

permissions/permission_definitions.go Show resolved Hide resolved

permissions/resource_permissions.go Show resolved Hide resolved

nkvuong reviewed Sep 26, 2024

View reviewed changes

nkvuong requested a review from alexott September 26, 2024 09:50

mgyucht added 9 commits September 26, 2024 14:13

more work

e52d9ec

Do not run serving endpoint permissions tests on GCP

91f203d

fix test

b6153aa

fix test

63397d4

actually just skip serving endpoints tests

e1244f8

fixed

81c9d7e

one last admin tweak

f67c4e7

better doccomment

abbcd75

fix test

b24ee9d

nkvuong approved these changes Sep 28, 2024

View reviewed changes

tweaks and one more integration test

94e8197

mgyucht added this pull request to the merge queue Oct 1, 2024

Merged via the queue into main with commit 9c2bf50 Oct 1, 2024

mgyucht deleted the issue-2407 branch October 1, 2024 11:29

parthban-db mentioned this pull request Oct 6, 2024

[Release] Release v1.53.0 #4076

Merged

mgyucht mentioned this pull request Oct 25, 2024

[Fix] Tolerate databricks_permissions resources for SQL warehouses with /warehouses/... IDs #4158

Merged

andrewnester mentioned this pull request Sep 30, 2025

Do not filter out current user from permissions list databricks/cli#3688

Closed

Conversation

mgyucht commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanmay-db left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkvuong left a comment

Choose a reason for hiding this comment

Uh oh!

NicholasFiorentini commented Oct 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mgyucht commented Aug 27, 2024 •

edited

Loading