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

azuread_group - support for the owners property #62

Merged
merged 7 commits into from
Jul 24, 2019

Conversation

tiwood
Copy link
Contributor

@tiwood tiwood commented Mar 13, 2019

(1/2 of #36)

This adds a new resource to manage Azure AD Group Owners with Terraform.
Additionally this adds support for owners in the resource azuread_group.

  • New resource 'azuread_group_owner'
  • New attribute 'owners' for resource 'azuread_group'
  • Add tests
  • Add documentation
  • azuread_group: If owners is not set, it should leave existing owners as is.

Example Usage

resource "azuread_group" "my_group" {
  name = "MyGroup"
}

data "azuread_user" "my_user" {
  user_principal_name = "[email protected]"
}

resource "azuread_group_owner" "default_owner" {
  group_object_id = "${azuread_group.my_group.id}"
  owner_object_id = "${data.azuread_user.my_user.id}"
}

Example for azuread_group

resource "azuread_group" "my_group" {
  name     = "my group"
  owners  = ["9c9b3992-78ac-11e9-a3ef-a31d2f3e9d2c"] 
}

Argument Reference

The following arguments are supported:

  • group_object_id - (Required) The object id of the Azure AD Group where the Owner should be added.
  • owner_object_id - (Required) The object id of the Azure AD User you want to add as Owner.

Caveats

Azure requires at least one owner per Azure AD group. That means the destruction of 'azuread_group_owner' resources will fail if no additional owner is present on the group.

@tiwood
Copy link
Contributor Author

tiwood commented Mar 13, 2019

Related: #36

@tiwood tiwood changed the title New resource 'azuread_group_owner' [WIP] New resource 'azuread_group_owner' Mar 13, 2019
@ghost ghost added size/XL and removed size/L labels Mar 13, 2019
@tiwood
Copy link
Contributor Author

tiwood commented Mar 14, 2019

@katbyte, @tombuildsstuff, do you guys have any ideas how we could tackle the issue of the 'not deletable' (see known issues above) group owners?

BTW you have the same behaviour in the Azure Portal.

  1. Create a group
  2. Add a new owner
  3. Try to remove the owner (this fails if its the last owner of the group)

Should we leave this up to the User?

@twendt
Copy link
Contributor

twendt commented Apr 9, 2019

Would it be an option to not implement it as a separate resource but instead integrate it into the group resource? There you could always pass the full list of owners in and not allow the list to be empty.

@tiwood
Copy link
Contributor Author

tiwood commented Apr 9, 2019

@twendt, I think this is the right way to do it. I'm going to look into it.
Thanks.

@katbyte
Copy link
Collaborator

katbyte commented Apr 12, 2019

@tiwood & @twendt,

There are reasons to have it as a separate resource (you can separate out creating & managing the owners) & within the resource (central location, ability to explicitly define who the owners should be)

And as such we should support both.

as to the issue of not being able to remove the last user, I say we fail destruction/update and leave that up to the user as theres not much we can do about it.

@tiwood
Copy link
Contributor Author

tiwood commented May 6, 2019

I will look into this in the next days.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the new resource @tiwood,

Aside from a few minor comments i've left inline this is looking pretty good! As with the group_user resource i would like to release with support to set this in the adgroup resource aswell. let me know if that is to big of an ask and i'll look into adding that myself 🙂

azuread/resource_group_owner.go Outdated Show resolved Hide resolved
azuread/resource_group_owner.go Outdated Show resolved Hide resolved
website/docs/r/group_owner.markdown Outdated Show resolved Hide resolved
website/docs/r/group_owner.markdown Outdated Show resolved Hide resolved
@ghost ghost removed the waiting-response label May 17, 2019
@tiwood tiwood changed the title [WIP] New resource 'azuread_group_owner' New resource 'azuread_group_owner' May 17, 2019
@tiwood tiwood changed the title New resource 'azuread_group_owner' WIP: ew resource 'azuread_group_owner' May 17, 2019
@tiwood tiwood changed the title WIP: ew resource 'azuread_group_owner' WIP: New resource 'azuread_group_owner' May 17, 2019
@ghost ghost added size/XXL and removed size/XL labels Jun 6, 2019
@katbyte katbyte added this to the v0.5.0 milestone Jun 17, 2019
@ghost ghost added size/XL and removed size/XXL labels Jul 19, 2019
@ghost ghost added size/XXL and removed size/XL labels Jul 21, 2019
@katbyte katbyte requested review from a team, tombuildsstuff and mbfrahry July 22, 2019 03:52
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

@tiwood,

Hope yo don't mind but i've pushed the required changes to this branch to get it good to merge 🙂 LGTM now 👍

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM with some minor spelling

@@ -83,11 +93,22 @@ func resourceGroupCreate(d *schema.ResourceData, meta interface{}) error {
if v, ok := d.GetOk("members"); ok {
members := tf.ExpandStringSlicePtr(v.(*schema.Set).List())

// we could lock here against the group ember resource, but the should not be used together (todo conflicts with at a resource level?)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// we could lock here against the group ember resource, but the should not be used together (todo conflicts with at a resource level?)
// we could lock here against the group member resource, but they should not be used together (todo conflicts with at a resource level?)

if v, ok := d.GetOk("owners"); ok {
members := tf.ExpandStringSlicePtr(v.(*schema.Set).List())

// we could lock here against the group owner resource, but the should not be used together (todo conflicts with at a resource level?)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// we could lock here against the group owner resource, but the should not be used together (todo conflicts with at a resource level?)
// we could lock here against the group owner resource, but they should not be used together (todo conflicts with at a resource level?)

@ghost ghost added size/XL and removed size/XXL labels Jul 24, 2019
@katbyte katbyte changed the title WIP: New resource 'azuread_group_owner' azuread_group - support for the owners property Jul 24, 2019
@katbyte katbyte merged commit 6980676 into hashicorp:master Jul 24, 2019
katbyte added a commit that referenced this pull request Jul 24, 2019
@ghost
Copy link

ghost commented Aug 23, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Aug 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants