-
Notifications
You must be signed in to change notification settings - Fork 386
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
[Fix] Refactor databricks_permissions
and allow the current user to set their own permissions
#3956
Conversation
}.ExpectError(t, "permission_level WHATEVER is not supported with cluster_id objects") | ||
} | ||
|
||
func TestResourcePermissionsCustomizeDiff_ErrorOnPermissionsDecreate(t *testing.T) { |
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.
This behavior is changed in this PR: users are allowed to change their permissions as long as they maintain management-level permissions.
{ | ||
UserName: TestingUser, | ||
PermissionLevel: "CAN_USE", | ||
}, | ||
{ | ||
UserName: TestingAdminUser, | ||
PermissionLevel: "IS_OWNER", | ||
PermissionLevel: "CAN_MANAGE", |
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.
Order of permissions won't affect the behavior in the API.
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.
LGTM, had some comments.
} | ||
|
||
// 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 { |
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.
Is this something we should also have in Go SDK and hence might be better to move it there?
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.
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.
// 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 { |
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.
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?
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.
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.
permissions/acl_customizers.go
Outdated
} | ||
} | ||
|
||
// Whether the object requires explicit manage permissions for the calling user if not set. |
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.
Thanks for the comments, easy to get context about this.
permissions/acl_customizers.go
Outdated
filteredAcl = append(filteredAcl, a) | ||
} | ||
} | ||
acl.AccessControlList = filteredAcl |
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.
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.
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.
Also since we are always returning nil as error in current implementation -- should we remove returning it / add error pathway?
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.
Good suggestion. I can change this method to return a new ObjectPermissions instance and not modify the original one.
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.
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.
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.
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 * 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)).
I suspect this change has introduced a regression. See #4143. |
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: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 forauthorization = "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 locallydocs/
folderinternal/acceptance