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

New resource & data source 'azuread_group' #14

Merged
merged 9 commits into from
Jan 22, 2019

Conversation

tiwood
Copy link
Contributor

@tiwood tiwood commented Jan 13, 2019

This PR adds a new resource and data source for Azure Active Directory groups.

The is based on the PR hashicorp/terraform-provider-azurerm#1839

  • New data source 'azuread_group'
  • New resource 'azuread_group'
  • Add markdown documentation
  • Add tests

Example Usage

// This creates a new Azure AD group with the display name "my_group"
resource "azuread_group" "my_group" {
 name = "my_group"
}

// This queries an Azure AD group by ID
data "azuread_group" "my_group_by_id" {
  object_id = "78722cfc-8946-11e8-95f1-2200ec79ad01"
}

// This queries an Azure AD group by Name
data "azuread_group" "my_group_by_name" {
  name = "my_group"
}

The following attributes are exported:

  • id - The Object ID for the Azure AD Group.
  • name - The Display Name for the Azure AD Group.

@tiwood tiwood changed the title New resource & data source 'azurerm_azuread_group' New resource & data source 'azuread_group' Jan 13, 2019
@tiwood
Copy link
Contributor Author

tiwood commented Jan 13, 2019

Tests pass:

TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureADGroup* -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azuread/version.ProviderVersion=acc"
?   	github.com/terraform-providers/terraform-provider-azuread	[no test files]
=== RUN   TestAccAzureADGroup_basic
--- PASS: TestAccAzureADGroup_basic (2.87s)
=== RUN   TestAccAzureADGroup_complete
--- PASS: TestAccAzureADGroup_complete (2.26s)
PASS
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccDataSourceAzureADGroup* -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azuread/version.ProviderVersion=acc"
?   	github.com/terraform-providers/terraform-provider-azuread	[no test files]
=== RUN   TestAccDataSourceAzureADGroup_byObjectId
--- PASS: TestAccDataSourceAzureADGroup_byObjectId (3.42s)
=== RUN   TestAccDataSourceAzureADGroup_byName
--- PASS: TestAccDataSourceAzureADGroup_byName (3.19s)
PASS

@tombuildsstuff
Copy link
Contributor

thanks for porting this over @tiwood - will take a look shortly :)

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @tiwood

Thanks for porting this PR over from the other repo - on the whole this looks good to me; I've left a few (mostly minor) comments in-line but if we can fix those up this otherwise LGTM 👍

Thanks!

azuread/data_source_group.go Outdated Show resolved Hide resolved
azuread/data_source_group.go Outdated Show resolved Hide resolved
azuread/resource_group.go Outdated Show resolved Hide resolved
azuread/data_source_group.go Outdated Show resolved Hide resolved
azuread/data_source_group.go Outdated Show resolved Hide resolved
azuread/data_source_group.go Outdated Show resolved Hide resolved
website/docs/d/group.html.markdown Outdated Show resolved Hide resolved
website/docs/r/group.markdown Outdated Show resolved Hide resolved
website/docs/d/group.html.markdown Outdated Show resolved Hide resolved
@tiwood
Copy link
Contributor Author

tiwood commented Jan 14, 2019

@tombuildsstuff, this is ready for another review.
(I think I actually have to add a new comment to remove the waiting-response tag)

@ghost ghost removed the waiting-response label Jan 14, 2019
Copy link
Contributor

@tombuildsstuff tombuildsstuff 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 pushing those changes @tiwood - this now LGTM (and I’ll kick off the tests shortly) 👍

@tombuildsstuff
Copy link
Contributor

@tiwood sorry for the delay - been trying to get the permissions sorted for the service principal - will take a look at that again today

@tiwood
Copy link
Contributor Author

tiwood commented Jan 22, 2019

@tiwood sorry for the delay - been trying to get the permissions sorted for the service principal - will take a look at that again today

No worries, if I can help let me know.

For reference, this is how I do it in cloud shell:

$aadUserAdminRole = Get-AzureADDirectoryRole | ? DisplayName -eq "User Account Administrator"
$terraformServicePrincipal = Get-AzureADServicePrincipal | ? ObjectId -eq "_YourTerraformServicePrincipalObjectId_"
Add-AzureADDirectoryRoleMember -ObjectId $aadUserAdminRole.ObjectId -RefObjectId $terraformServicePrincipal.ObjectId

-> **NOTE:** Additionally, due to a limitation within the API, you have to assign **one** of the following Azure Active Directory Roles to the Service Principal to be able to delete Groups:

* User Account Administrator
* Company Administrator
Copy link
Contributor

Choose a reason for hiding this comment

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

after spending some time looking into this, it appears that Company Administrator should be the only role necessary - the User Account Administrator doesn't appear to have sufficient rights on it's own (I'm assuming it may for the azuread_user resource however?) - as such could we update this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User Account Administrator should definitely work, see https://docs.microsoft.com/en-us/azure/active-directory/users-groups-roles/directory-assign-admin-roles#user-account-administrator.

I use these permissions exclusively on my Service Principal and it seems to work.

I've found, that if you apply a role to the Service principal, it may take a few min to propagate.

@tombuildsstuff
Copy link
Contributor

@tiwood thanks for the help - got the permissions working in the end - and the tests now run for me:

$ acctests azuread TestAccAzureADGroup_
=== RUN   TestAccAzureADGroup_basic
--- PASS: TestAccAzureADGroup_basic (4.89s)
=== RUN   TestAccAzureADGroup_complete
--- PASS: TestAccAzureADGroup_complete (4.57s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azuread/azuread	9.605s

@tombuildsstuff tombuildsstuff added this to the 0.2.0 milestone Jan 22, 2019
@tombuildsstuff
Copy link
Contributor

@tiwood thanks for this PR - this now LGTM 👍

@tombuildsstuff tombuildsstuff merged commit c196fb5 into hashicorp:master Jan 22, 2019
tombuildsstuff added a commit that referenced this pull request Jan 22, 2019
@AdamCoulterOz
Copy link

AdamCoulterOz commented Feb 7, 2019

@tiwood the docs here don't specify the option to use object_id or group name (only name), is that still part of this feature?

@tiwood
Copy link
Contributor Author

tiwood commented Feb 7, 2019

@tiwood the docs here don't specify the option to use object_id or group name (only name), is that still part of this feature?

Hi @ksix, can you explain further what you need?
Originally it was possible to use the data source with the object_id parameter, but after some discussions we decided to remove it for now. Do you have a use case for this?

@tiwood tiwood deleted the ds_r_azuread_group branch February 7, 2019 20:58
@erick-thompson
Copy link

Do you have any idea when 0.2.0 might be released? I'd love to start using the group resource instead of littering object_id all over the place.

@AdamCoulterOz
Copy link

@tiwood I want to create groups in one script and assign them in another (asynchronously), coupling to the name is unsafe because it can be changed by others or may not even return the right group.

@ghost
Copy link

ghost commented Mar 5, 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 Mar 5, 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.

7 participants