-
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
implement resource databricks_service_pricnipal #386
Conversation
One question about the SCIM service principal schema about |
Hey @tcz001, TravisBuddy Request Identifier: d7d35b20-17e3-11eb-bc4d-2726ba7bdf50 |
Codecov Report
@@ Coverage Diff @@
## master #386 +/- ##
==========================================
+ Coverage 76.32% 80.86% +4.54%
==========================================
Files 65 63 -2
Lines 6636 6011 -625
==========================================
- Hits 5065 4861 -204
+ Misses 1172 759 -413
+ Partials 399 391 -8
|
Hey @tcz001, TravisBuddy Request Identifier: 4053db80-1806-11eb-bc4d-2726ba7bdf50 |
Hi @nfx, any feedback? |
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.
Looks good, some remarks:
- remove methods that are not needed
- rename some of the methods
- add 90% code coverage for new code (there's drop of 15% in SCIM in this PR)
} | ||
|
||
resource "databricks_service_principal" "me" { | ||
application_id = "00000000-0000-0000-0000-000000000000" |
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.
i'm currently running this by our engineering folks to agree on name for this field, because service principal identification might look differently on AWS
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.
any suggestions?
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.
still internal discussions :) i'll try to run this PR against AWS deployment this week
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.
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.
@nfx any updates?
identity/scim.go
Outdated
@@ -45,6 +46,13 @@ type UserPatchOperations struct { | |||
Value *GroupsValue `json:"value,omitempty"` | |||
} | |||
|
|||
// ServicePrincipalPatchOperations is a list of path operations for add or removing service principal attributes | |||
type ServicePrincipalPatchOperations struct { |
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.
there's no need to create a dedicated *PatchOperations
struct, you can reuse the user 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.
sounds good, will change it, but should we rename the struct of UserPatchOperation to make it more abstract?
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.
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.
@tcz001 yes, it can be just PathOperations
+change to databricks_permissions resource doc in this particular case |
…concept into docs
Travis tests have failedHey @tcz001, 1st Buildcurl -sSL "https://github.com/gotestyourself/gotestsum/releases/download/v0.4.2/gotestsum_0.4.2_linux_amd64.tar.gz" | sudo tar -xz -C /usr/local/bin gotestsum
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0
time make lint
time make test
2nd Buildcurl -sSL "https://github.com/gotestyourself/gotestsum/releases/download/v0.4.2/gotestsum_0.4.2_linux_amd64.tar.gz" | sudo tar -xz -C /usr/local/bin gotestsum
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0
time make lint
TravisBuddy Request Identifier: 67ea7ff0-1d51-11eb-8505-818d591f0ed7 |
Travis tests have failedHey @tcz001, 1st Buildcurl -sSL "https://github.com/gotestyourself/gotestsum/releases/download/v0.4.2/gotestsum_0.4.2_linux_amd64.tar.gz" | sudo tar -xz -C /usr/local/bin gotestsum
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0
time make lint
2nd Buildcurl -sSL "https://github.com/gotestyourself/gotestsum/releases/download/v0.4.2/gotestsum_0.4.2_linux_amd64.tar.gz" | sudo tar -xz -C /usr/local/bin gotestsum
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.25.0
time make lint
TravisBuddy Request Identifier: 6c03de20-1d64-11eb-8505-818d591f0ed7 |
Hey @tcz001, TravisBuddy Request Identifier: 8eafb0f0-1d75-11eb-8505-818d591f0ed7 |
Hey @tcz001, TravisBuddy Request Identifier: a296b680-200a-11eb-9dab-419bf2960c30 |
Hey @tcz001, TravisBuddy Request Identifier: 34a11d70-2062-11eb-9dab-419bf2960c30 |
Hey @tcz001, TravisBuddy Request Identifier: 20f81f60-22bc-11eb-b1e0-17a3b2b69400 |
Hi @nfx any further feedback? |
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.
Minor changes requested
docs/resources/permissions.md
Outdated
@@ -92,7 +92,7 @@ resource "databricks_permissions" "policy_usage" { | |||
|
|||
## Instance Pool usage | |||
|
|||
[Instance Pools](instance_pool.md) access control [allows to](https://docs.databricks.com/security/access-control/pool-acl.html) assign `CAN_ATTACH_TO` and `CAN_MANAGE` permissions to users and groups. It's also possible to grant creation of Instance Pools to individual [groups](group.md#allow_instance_pool_create) and [users](user.md#allow_instance_pool_create). | |||
[Instance Pools](instance_pool.md) access control [allows to](https://docs.databricks.com/security/access-control/pool-acl.html) assign `CAN_ATTACH_TO` and `CAN_MANAGE` permissions to users, service principals and groups. It's also possible to grant creation of Instance Pools to individual [groups](group.md#allow_instance_pool_create) and [users](user.md#allow_instance_pool_create), [service principals](service_principal.md#allow_instance_pool_create). |
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.
to use consistent punctuation, please add coma before and
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.
Hi @nfx , sorry I'm a bit lost on these comments because I'm not english native speaker, to save the time, can you maybe directly push to my branch, if you don't have access to push please let me know. https://github.com/tcz001/terraform-provider-databricks-1/invitations
docs/resources/permissions.md
Outdated
@@ -52,7 +52,7 @@ resource "databricks_permissions" "cluster_usage" { | |||
|
|||
## Cluster Policy usage | |||
|
|||
Cluster policies allow creation of [clusters](cluster.md), that match [given policy](https://docs.databricks.com/administration-guide/clusters/policies.html). It's possible to assign `CAN_USE` permission to users and groups: | |||
Cluster policies allow creation of [clusters](cluster.md), that match [given policy](https://docs.databricks.com/administration-guide/clusters/policies.html). It's possible to assign `CAN_USE` permission to users, service principals and groups: |
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.
to use consistent punctuation, please add coma before and
@@ -128,12 +128,12 @@ resource "databricks_permissions" "pool_usage" { | |||
|
|||
## Job usage | |||
|
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.
to use consistent punctuation, please add coma before and
docs/resources/permissions.md
Outdated
@@ -128,12 +128,12 @@ resource "databricks_permissions" "pool_usage" { | |||
|
|||
## Job usage | |||
|
|||
There are four assignable [permission levels](https://docs.databricks.com/security/access-control/jobs-acl.html#job-permissions) for [databricks_job](job.md): `CAN_VIEW`, `CAN_MANAGE_RUN`, `IS_OWNER`, and `CAN_MANAGE`. Admins are granted the `CAN_MANAGE` permission by default, and they can assign that permission to non-admin users. | |||
There are four assignable [permission levels](https://docs.databricks.com/security/access-control/jobs-acl.html#job-permissions) for [databricks_job](job.md): `CAN_VIEW`, `CAN_MANAGE_RUN`, `IS_OWNER`, and `CAN_MANAGE`. Admins are granted the `CAN_MANAGE` permission by default, and they can assign that permission to non-admin users and service principals. |
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.
to use consistent punctuation, please add coma before and
docs/resources/permissions.md
Outdated
|
||
* The creator of a job has `IS_OWNER` permission. Destroying `databricks_permissions` resource for a job would revert ownership to creator. | ||
* A job must have exactly one owner. If resource is changed and no owner is specified, currently authenticated principal would become new owner of the job. Nothing would change, per se, if the job was created through Terraform. | ||
* A job cannot have a group as an owner. | ||
* Jobs triggered through *Run Now* assume the permissions of the job owner and not the user who issued Run Now. | ||
* Jobs triggered through *Run Now* assume the permissions of the job owner and not the user and service principal who issued Run Now. |
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.
to use consistent punctuation, please add coma before and
docs/resources/permissions.md
Outdated
* All users have `CAN_MANAGE` permission for items in the Workspace > Shared Icon Shared folder. You can grant `CAN_MANAGE` permission to notebooks and folders by moving them to the Shared Icon Shared folder. | ||
* All users have `CAN_MANAGE` permission for objects the user creates. | ||
* User home directory - The user has `CAN_MANAGE` permission. All other users can list their directories. | ||
* All users(or service principals) have `CAN_MANAGE` permission for items in the Workspace > Shared Icon Shared folder. You can grant `CAN_MANAGE` permission to notebooks and folders by moving them to the Shared Icon Shared folder. |
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.
space before opening bracket
docs/resources/permissions.md
Outdated
@@ -273,7 +273,7 @@ resource "databricks_permissions" "folder_usage" { | |||
|
|||
## Passwords usage | |||
|
|||
By default on AWS deployments, all admin users can sign in to Databricks using either SSO or their username and password, and all API users can authenticate to the Databricks REST APIs using their username and password. As an admin, you [can limit](https://docs.databricks.com/administration-guide/users-groups/single-sign-on/index.html#optional-configure-password-access-control) admin users’ and API users’ ability to authenticate with their username and password by configuring `CAN_USE` permissions using password access control. | |||
By default on AWS deployments, all admin users(or service principals) can sign in to Databricks using either SSO or their username and password, and all API users(or service principals) can authenticate to the Databricks REST APIs using their username and password. As an admin, you [can limit](https://docs.databricks.com/administration-guide/users-groups/single-sign-on/index.html#optional-configure-password-access-control) admin users’ and API users’ ability to authenticate with their username and password by configuring `CAN_USE` permissions using password access control. |
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.
space before opening bracket
@tcz001 good news! we can merge changes with application_id field. please finish up existing comments and let's get ready to merge. |
Hi @nfx, we pushed some changes based on your comments regarding the documentation. We are colleagues of @tcz001 and he is away for a while, so our team will take care of the changes in the meantime. Let us know if there is anything else missing from the comments or if there's anything else we need to take care of. Thanks |
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.
Looks good! I'll aim to include it in v0.3.0 release
@avillalain please resolve conflicts in |
@nfx conflicts are resolved. Thanks |
Directly creates service principal, that could be added to a group within workspace. ```hcl resource "databricks_service_principal" "sp" { application_id = "00000000-0000-0000-0000-000000000000" } ``` Co-authored-by: tcz001 <fan.torchz@gmail.com> Co-authored-by: Angel Villalain Garcia <avillalaingarcia@humana.com> Co-authored-by: Serge Smertin <serge.smertin@databricks.com>
congratulations @tcz001 , @avillalain , it's merged in #432 and is going to be part of 0.3.0 release. |
This should address #28