Skip to content
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

Added databricks_mws_permission_assignment resource #1491

Merged
merged 7 commits into from
Aug 4, 2022

Conversation

nfx
Copy link
Contributor

@nfx nfx commented Jul 21, 2022

databricks_mws_permission_assignment Resource

These resources are invoked in the account context. Provider must have account_id attribute configured.

Example Usage

In account context, adding account-level group to a workspace:

provider "databricks" {
  // <other properties>
  account_id = "<databricks account id>"
}

resource "databricks_group" "data_eng" {
  display_name = "Data Engineering"
}

resource "databricks_mws_permission_assignment" "add_admin_group" {
  workspace_id = databricks_mws_workspaces.this.workspace_id
  principal_id = databricks_group.data_eng.id
  permissions  = ["ADMIN"]
}

In account context, adding account-level user to a workspace:

provider "databricks" {
  // <other properties>
  account_id = "<databricks account id>"
}

resource "databricks_user" "me" {
  user_name = "me@example.com"
}

resource "databricks_mws_permission_assignment" "add_user" {
  workspace_id = databricks_mws_workspaces.this.workspace_id
  principal_id = databricks_user.me.id
  permissions  = ["USER"]
}

In account context, adding account-level service principal to a workspace:

provider "databricks" {
  // <other properties>
  account_id = "<databricks account id>"
}

resource "databricks_service_principal" "sp" {
  display_name = "Automation-only SP"
}

resource "databricks_mws_permission_assignment" "add_admin_spn" {
  workspace_id = databricks_mws_workspaces.this.workspace_id
  principal_id = databricks_service_principal.sp.id
  permissions  = ["ADMIN"]
}

Fix #1458

@nfx nfx requested a review from rohitgupta10 July 21, 2022 22:25
@nfx nfx changed the title Added databricks_mws_permissionassignments resource Added databricks_mws_permissionassignment resource Jul 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2022

Codecov Report

Merging #1491 (7f689f0) into master (c72250b) will decrease coverage by 0.34%.
The diff coverage is 60.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1491      +/-   ##
==========================================
- Coverage   90.13%   89.79%   -0.35%     
==========================================
  Files         130      132       +2     
  Lines       10434    10551     +117     
==========================================
+ Hits         9405     9474      +69     
- Misses        658      704      +46     
- Partials      371      373       +2     
Impacted Files Coverage Δ
access/resource_permissionassignment.go 0.00% <0.00%> (ø)
internal/acceptance/acceptance.go 0.00% <0.00%> (ø)
common/pair.go 93.10% <63.63%> (-3.33%) ⬇️
qa/testing.go 81.04% <75.00%> (-0.17%) ⬇️
mws/resource_mws_permission_assignment.go 96.92% <96.92%> (ø)
catalog/resource_metastore_assignment.go 95.23% <100.00%> (+0.23%) ⬆️
provider/provider.go 95.17% <100.00%> (+0.06%) ⬆️

@alexott
Copy link
Contributor

alexott commented Jul 22, 2022

Is it only for MWS, or it covers all clouds? can we also change name to databricks_mws_permission_assignment - it's easier to read

@nfx nfx force-pushed the feature/identity-federation branch 2 times, most recently from f6844bf to 58dbcc0 Compare July 28, 2022 19:34
@nfx nfx changed the title Added databricks_mws_permissionassignment resource Added databricks_mws_permission_assignment resource Jul 28, 2022
@nfx nfx requested a review from rohitgupta10 July 28, 2022 19:41
Comment on lines +10 to +11
In account context, adding account-level group to a workspace:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we only have docs for updating role assignment in the account context. I think thats fine and sufficient but it will not work for workspace admin personas who are not account admins.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we'll add that "workspace level account scim user listing" in one of the following releases.

Copy link

@baishen-db baishen-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. I'm very new to terraform so will defer to Rohit to do another round of review

---
# databricks_mws_permission_assignment Resource

These resources are invoked in the account context. Provider must have `account_id` attribute configured.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 will there be a documentation for access/permission_assignment? For that one API doesn't need workspace id or account id, they can be derived from the workspace url.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have a way to retrieve account-level IDs from workspace in the provider yet. please "publish" the api ;)

docs/resources/mws_permission_assignment.md Show resolved Hide resolved
"github.com/databricks/terraform-provider-databricks/qa"
)

func TestAccAssignGroupToWorkspace(t *testing.T) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, what does this test do? Does it test that the template can be accepted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does create, read, update, and delete test assignments of test principals

Read: true,
Removed: true,
AccountID: "abc",
ID: "123|456",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this ID mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mws/resource_mws_permission_assignment_test.go Outdated Show resolved Hide resolved
mws/resource_mws_permission_assignment_test.go Outdated Show resolved Hide resolved
qa.CornerCaseAccountID("abc"))
}

func TestPermssionAssignmentFuzz_ApiErrors(t *testing.T) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this test work? Do we need to verify that API errors are properly handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. it does some fuzzing. see CONTRIBUTING.md for details

@nfx nfx force-pushed the feature/identity-federation branch from 7f689f0 to 06cae22 Compare August 4, 2022 14:55
Copy link
Contributor Author

@nfx nfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nfx nfx merged commit b72863a into master Aug 4, 2022
@nfx nfx deleted the feature/identity-federation branch August 4, 2022 15:01
@nfx nfx mentioned this pull request Aug 4, 2022
michael-berk pushed a commit to michael-berk/terraform-provider-databricks that referenced this pull request Feb 15, 2023
These resources are invoked in the account context. Provider must have `account_id` attribute configured. Example usage in account context, adding account-level group to a workspace:

```hcl
provider "databricks" {
  // <other properties>
  account_id = "<databricks account id>"
}

resource "databricks_group" "data_eng" {
  display_name = "Data Engineering"
}

resource "databricks_mws_permission_assignment" "add_admin_group" {
  workspace_id = databricks_mws_workspaces.this.workspace_id
  principal_id = databricks_group.data_eng.id
  permissions  = ["ADMIN"]
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ISSUE] Support assignment of users/groups/SPs to workspaces at account level
5 participants