Resource: 'azuread_group_member'

[evenh]

This PR continues where #63 left off. In addition to fixing the comments pointed out in the review, it introduces support for adding members to a group.

Go is not my main language and I'm not that familiar with Terraform so don't be afraid to do a proper review :-)

[evenh]

ping @katbyte :)

[katbyte]

Thanks for opening this PR to get this in @evenh,

I've left some comments inline with my main concerns being:

• moving some of the shared code into to graph helper package
• supporting all object types in the group.members property
• changing the ID to group_id/members/object_id sp it doesn't collide with group owners

In addition to the inline comments could we add a test that has 1 or more of each type of member as well as an update test going from none -> one -> many -> differnt_one -> none with import checks in between each step?

[katbyte]

Will this fail if the member already exists?

[katbyte]

Could we also reuse the graph.GroupAddMember function discussed above?

[evenh]

I've refactored to use the new helper function. However, if a member already exists it fails. What is the desired outcome here? Given the following HCL:

// User one already exists
resource "azuread_group" "some_group" {
  name = "SomeGroup"
}

resource "azuread_group_member" "one" {
  group_object_id   = azuread_group.some_group.id
  member_object_id  = azuread_user.one.id
}

resource "azuread_group_member" "another" {
  group_object_id   = azuread_group.some_group.id
  member_object_id  = azuread_user.one.id
}

In that case Terraform sees two "new" resource in azuread_group_member.one and azuread_group_member.another. After applying the first one, another fails with

graphrbac.GroupsClient#AddMember: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Request_BadRequest","date":"2019-06-18T11:09:20","message":{"lang":"en","value":"One or more added object references already exist for the following modified properties: 'members'."},"requestId":"c7ad6982-559e-46eb-bfed-56b6455141c8"}}]

[katbyte]

I have also opened a PR for a data source to get object IDs for multiple users (#109) that would be nice to be used here in the example/tests here somewhere. but its not a blocker as i can add that in afterwards

[evenh]

Some of the comments is now fixed!

I've used a day trying to make acceptance tests work from my local setup but to no avail (makes writing new acceptance tests a little hard). Any pointers on how the service principal used for running acceptance tests is configured?

[katbyte]

Hi @evenh,

You should be able to follow the instructions here: https://www.terraform.io/docs/providers/azuread/auth/service_principal_client_secret.html

Make sure you grant it the appropriate access here: https://www.terraform.io/docs/providers/azuread/auth/service_principal_configuration.html

I believe there is also an example on master in the examples folder demonstrating this too if you'd prefer a terraform'y way

[katbyte]

@evenh, is this ready for re-review?

[evenh]

Ready for a re-review now @katbyte. Also note the two open inline comments :-)

[katbyte]

Hi @evenh,

Thank you for all the changes, the is looking pretty good. I have left some comments inline with my main concern being the tests. Would we change them to be more consistent with how the others are written?

Look forward to get this this merged once the comments are all addressed 🙂

[katbyte]

Clicked the wrong button 😓

[evenh]

I've fixed most of your comments @katbyte and added some of my own :)

[evenh]

I've converted all of resource_group_test.go to HCL @katbyte :)

[katbyte]

Thanks for the revisions @evenh,

Only a couple more minor comments and this should be good to merge 🙂

[evenh]

There you go @katbyte :-)

[evenh]

Could you please have another look @katbyte?

[aambert]

Up

[katbyte]

Thanks for the revisions @evenh! Apologies for the delay in reviewing it but azurerm required some attention.

This LGTM however there is a test failure due to replication:

------- Stdout: -------
=== RUN   TestAccAzureADGroup_progression
--- FAIL: TestAccAzureADGroup_progression (65.21s)
    testing.go:568: Step 8 error: errors during apply:
        
        Error: Error Deleting group member "994abfa7-3681-40a5-962d-50cca6383c46" from Azure AD Group with ID "cd0932a7-2fda-4572-9350-f86f09dbd0c3": graphrbac.GroupsClient#RemoveMember: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Request_BadRequest","date":"2019-07-11T22:06:08","message":{"lang":"en","value":"One or more removed object references do not exist for the following modified properties: 'members'."},"requestId":"33425947-652e-45df-b224-37d0aa7ec671"}}]
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test261138976/main.tf line 2:
          (source code not available)
        
        
FAIL

However I feel like that is larger than the scope of this PR (will also apply to owners) so I am going to merge anyways.

[ghost]

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!