Skip to content

Commit

Permalink
[Fix] Refactor databricks_permissions and allow the current user to…
Browse files Browse the repository at this point in the history 
… set their own permissions (#3956)

## 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
called`resourcePermissions`. 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
- [x] relevant change in `docs/` folder
- [x] covered with integration tests in `internal/acceptance`
- [x] relevant acceptance tests are passing
- [ ] using Go SDK
  • Loading branch information
@mgyucht
mgyucht authored Oct 1, 2024
1 parent 1cfc531 commit 9c2bf50
Show file tree
Hide file tree
Showing 11 changed files with 2,833 additions and 1,750 deletions.
6 changes: 3 additions & 3 deletions docs/resources/permissions.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ subcategory: "Security"

# databricks_permissions Resource

This resource allows you to generically manage [access control](https://docs.databricks.com/security/access-control/index.html) in Databricks workspace. It would guarantee that only _admins_, _authenticated principal_ and those declared within `access_control` blocks would have specified access. It is not possible to remove management rights from _admins_ group.
This resource allows you to generically manage [access control](https://docs.databricks.com/security/access-control/index.html) in Databricks workspaces. It ensures that only _admins_, _authenticated principal_ and those declared within `access_control` blocks would have specified access. It is not possible to remove management rights from _admins_ group.

-> **Note** Configuring this resource for an object will **OVERWRITE** any existing permissions of the same type unless imported, and changes made outside of Terraform will be reset unless the changes are also reflected in the configuration.
-> **Note** This resource is _authoritative_ for permissions on objects. Configuring this resource for an object will **OVERWRITE** any existing permissions of the same type unless imported, and changes made outside of Terraform will be reset.

-> **Note** It is not possible to lower permissions for `admins` or your own user anywhere from `CAN_MANAGE` level, so Databricks Terraform Provider [removes](https://github.com/databricks/terraform-provider-databricks/blob/main/permissions/resource_permissions.go#L324-L332) those `access_control` blocks automatically.
-> **Note** It is not possible to lower permissions for `admins`, so Databricks Terraform Provider removes those `access_control` blocks automatically.

-> **Note** If multiple permission levels are specified for an identity (e.g. `CAN_RESTART` and `CAN_MANAGE` for a cluster), only the highest level permission is returned and will cause permanent drift.

Expand Down
48 changes: 24 additions & 24 deletions exporter/exporter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,7 @@ func TestImportingClusters(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/clusters/test1",
Resource: "/api/2.0/permissions/clusters/test1?",
Response: getJSONObject("test-data/get-cluster-permissions-test1-response.json"),
},
{
Expand Down Expand Up @@ -913,7 +913,7 @@ func TestImportingClusters(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/clusters/test2",
Resource: "/api/2.0/permissions/clusters/test2?",
Response: getJSONObject("test-data/get-cluster-permissions-test2-response.json"),
},
{
Expand All @@ -923,7 +923,7 @@ func TestImportingClusters(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/cluster-policies/123",
Resource: "/api/2.0/permissions/cluster-policies/123?",
Response: getJSONObject("test-data/get-cluster-policy-permissions.json"),
},
{
Expand All @@ -949,7 +949,7 @@ func TestImportingClusters(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/clusters/awscluster",
Resource: "/api/2.0/permissions/clusters/awscluster?",
Response: getJSONObject("test-data/get-cluster-permissions-awscluster-response.json"),
},
{
Expand All @@ -971,7 +971,7 @@ func TestImportingClusters(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/instance-pools/pool1",
Resource: "/api/2.0/permissions/instance-pools/pool1?",
ReuseRequest: true,
Response: getJSONObject("test-data/get-job-permissions-14.json"),
},
Expand Down Expand Up @@ -1089,7 +1089,7 @@ func TestImportingJobs_JobList(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/jobs/14",
Resource: "/api/2.0/permissions/jobs/14?",
Response: getJSONObject("test-data/get-job-permissions-14.json"),
},
{
Expand All @@ -1112,7 +1112,7 @@ func TestImportingJobs_JobList(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/instance-pools/pool1",
Resource: "/api/2.0/permissions/instance-pools/pool1?",
ReuseRequest: true,
Response: getJSONObject("test-data/get-job-permissions-14.json"),
},
Expand Down Expand Up @@ -1202,7 +1202,7 @@ func TestImportingJobs_JobList(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/cluster-policies/123",
Resource: "/api/2.0/permissions/cluster-policies/123?",
Response: getJSONObject("test-data/get-cluster-policy-permissions.json"),
},
{
Expand All @@ -1218,7 +1218,7 @@ func TestImportingJobs_JobList(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/instance-pools/pool1",
Resource: "/api/2.0/permissions/instance-pools/pool1?",
ReuseRequest: true,
Response: getJSONObject("test-data/get-job-permissions-14.json"),
},
Expand Down Expand Up @@ -1307,7 +1307,7 @@ func TestImportingJobs_JobListMultiTask(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/jobs/14",
Resource: "/api/2.0/permissions/jobs/14?",
Response: getJSONObject("test-data/get-job-permissions-14.json"),
ReuseRequest: true,
},
Expand All @@ -1331,7 +1331,7 @@ func TestImportingJobs_JobListMultiTask(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/instance-pools/pool1",
Resource: "/api/2.0/permissions/instance-pools/pool1?",
ReuseRequest: true,
Response: getJSONObject("test-data/get-job-permissions-14.json"),
},
Expand Down Expand Up @@ -1470,7 +1470,7 @@ func TestImportingJobs_JobListMultiTask(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/cluster-policies/123",
Resource: "/api/2.0/permissions/cluster-policies/123?",
Response: getJSONObject("test-data/get-cluster-policy-permissions.json"),
},
{
Expand All @@ -1486,7 +1486,7 @@ func TestImportingJobs_JobListMultiTask(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/instance-pools/pool1",
Resource: "/api/2.0/permissions/instance-pools/pool1?",
ReuseRequest: true,
Response: getJSONObject("test-data/get-job-permissions-14.json"),
},
Expand Down Expand Up @@ -1777,7 +1777,7 @@ func TestImportingRepos(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/repos/121232342",
Resource: "/api/2.0/permissions/repos/121232342?",
Response: getJSONObject("test-data/get-repo-permissions.json"),
},
},
Expand Down Expand Up @@ -1902,7 +1902,7 @@ func TestImportingSqlObjects(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/directories/4451965692354143",
Resource: "/api/2.0/permissions/directories/4451965692354143?",
Response: getJSONObject("test-data/get-directory-permissions.json"),
},
{
Expand Down Expand Up @@ -1933,7 +1933,7 @@ func TestImportingSqlObjects(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/sql/warehouses/f562046bc1272886",
Resource: "/api/2.0/permissions/sql/warehouses/f562046bc1272886?",
Response: getJSONObject("test-data/get-sql-endpoint-permissions.json"),
},
{
Expand Down Expand Up @@ -1962,12 +1962,12 @@ func TestImportingSqlObjects(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/preview/sql/permissions/queries/16c4f969-eea0-4aad-8f82-03d79b078dcc",
Resource: "/api/2.0/permissions/sql/queries/16c4f969-eea0-4aad-8f82-03d79b078dcc?",
Response: getJSONObject("test-data/get-sql-query-permissions.json"),
},
{
Method: "GET",
Resource: "/api/2.0/preview/sql/permissions/dashboards/9cb0c8f5-6262-4a1f-a741-2181de76028f",
Resource: "/api/2.0/permissions/dbsql-dashboards/9cb0c8f5-6262-4a1f-a741-2181de76028f?",
Response: getJSONObject("test-data/get-sql-dashboard-permissions.json"),
},
{
Expand All @@ -1983,7 +1983,7 @@ func TestImportingSqlObjects(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/preview/sql/permissions/alerts/3cf91a42-6217-4f3c-a6f0-345d489051b9",
Resource: "/api/2.0/permissions/sql/alerts/3cf91a42-6217-4f3c-a6f0-345d489051b9?",
Response: getJSONObject("test-data/get-sql-alert-permissions.json"),
},
},
Expand Down Expand Up @@ -2039,7 +2039,7 @@ func TestImportingDLTPipelines(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/repos/123",
Resource: "/api/2.0/permissions/repos/123?",
Response: getJSONObject("test-data/get-repo-permissions.json"),
},
{
Expand Down Expand Up @@ -2085,12 +2085,12 @@ func TestImportingDLTPipelines(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/pipelines/123",
Resource: "/api/2.0/permissions/pipelines/123?",
Response: getJSONObject("test-data/get-pipeline-permissions.json"),
},
{
Method: "GET",
Resource: "/api/2.0/permissions/notebooks/123",
Resource: "/api/2.0/permissions/notebooks/123?",
Response: getJSONObject("test-data/get-notebook-permissions.json"),
},
{
Expand Down Expand Up @@ -2169,7 +2169,7 @@ func TestImportingDLTPipelines(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/files/789",
Resource: "/api/2.0/permissions/files/789?",
Response: getJSONObject("test-data/get-workspace-file-permissions.json"),
},
},
Expand Down Expand Up @@ -2257,7 +2257,7 @@ func TestImportingDLTPipelinesMatchingOnly(t *testing.T) {
},
{
Method: "GET",
Resource: "/api/2.0/permissions/pipelines/123",
Resource: "/api/2.0/permissions/pipelines/123?",
Response: getJSONObject("test-data/get-pipeline-permissions.json"),
},
{
Expand Down
4 changes: 2 additions & 2 deletions exporter/importables.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (
"github.com/databricks/terraform-provider-databricks/common"
"github.com/databricks/terraform-provider-databricks/jobs"
"github.com/databricks/terraform-provider-databricks/mws"
"github.com/databricks/terraform-provider-databricks/permissions"
"github.com/databricks/terraform-provider-databricks/permissions/entity"
tfpipelines "github.com/databricks/terraform-provider-databricks/pipelines"
"github.com/databricks/terraform-provider-databricks/repos"
tfsettings "github.com/databricks/terraform-provider-databricks/settings"
Expand Down Expand Up @@ -1184,7 +1184,7 @@ var resourcesMap map[string]importable = map[string]importable{
return (r.Data.Get("access_control.#").(int) == 0)
},
Import: func(ic *importContext, r *resource) error {
var permissions permissions.PermissionsEntity
var permissions entity.PermissionsEntity
s := ic.Resources["databricks_permissions"].Schema
common.DataToStructPointer(r.Data, s, &permissions)
for _, ac := range permissions.AccessControlList {
Expand Down
5 changes: 3 additions & 2 deletions exporter/importables_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/databricks/terraform-provider-databricks/common"
"github.com/databricks/terraform-provider-databricks/jobs"
"github.com/databricks/terraform-provider-databricks/permissions"
"github.com/databricks/terraform-provider-databricks/permissions/entity"

"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
dlt_pipelines "github.com/databricks/terraform-provider-databricks/pipelines"
Expand Down Expand Up @@ -220,8 +221,8 @@ func TestPermissions(t *testing.T) {
assert.Equal(t, "abc", name)

d.MarkNewResource()
err := common.StructToData(permissions.PermissionsEntity{
AccessControlList: []permissions.AccessControlChange{
err := common.StructToData(entity.PermissionsEntity{
AccessControlList: []iam.AccessControlRequest{
{
UserName: "a",
},
Expand Down
Loading

0 comments on commit 9c2bf50

Please sign in to comment.