New resource: `azurerm_user_assigned_identity` + enabled UserAssigned identity for VM and VMSS #1448

logachev · 2018-06-26T20:28:12Z

Implementing feature mentioned in the feature request: #1377

tombuildsstuff

Thanks for this PR - apologies for the delayed review here!

I've taken a look through and left some comments inline, but this is off to a good start; if we can fix up the comments we should be able to run the tests and get this merged :)

Thanks!

tombuildsstuff · 2018-06-27T09:15:52Z

website/docs/r/user_assigned_identity.markdown

+User Assigned Identitites can be imported using the `resource id`, e.g.
+
+```shell
+terraform import azurerm_user_assigned_identity.testIdentity /subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/acceptanceTestResourceGroup1/providers/Microsoft.ManagedIdentity/userAssignedIdentities/testIdentity


given this is a new Resource Provider, can we also ensure this is registered when the AzureRM Provider is initialized? This can be done by adding Microsoft.ManagedIdentity (which is case sensitive) to this list

tombuildsstuff · 2018-06-27T09:17:04Z

website/docs/r/user_assigned_identity.markdown

+
+* `resource_group_name` - The name of the resource group in which user assigned identity created.
+
+* `location` - The location/region where the user assigned identity created.


can we remove name, resource_group_name and location here? these outputs can be implied as they're arguments

Removed. Seems like there are two styles in .markdown files: some list arguments as attributes and some don't.

tombuildsstuff · 2018-06-27T09:17:25Z

website/docs/r/user_assigned_identity.markdown

+  resource_group_name = "${azurerm_resource_group.test.name}"
+  location            = "${azurerm_resource_group.test.location}"
+
+  name = "${var.name}"


rather than using a variable, can we add an example name here? e.g. search-api

tombuildsstuff · 2018-06-27T09:20:59Z

azurerm/resource_arm_virtual_machine_test.go

+	resource_group_name = "${azurerm_resource_group.test.name}"
+	location = "${azurerm_resource_group.test.location}"
+
+	name = "test"


can we make this acctest%d rather than test - this'll help to prevent conflicts between acceptance test runs by appending the unique ID to the end

tombuildsstuff · 2018-06-27T09:22:31Z

azurerm/resource_arm_virtual_machine_test.go

@@ -173,3 +220,203 @@ resource "azurerm_virtual_machine" "test" {
 }
 `, rInt, location, rInt, rInt, rInt, rInt, rInt)
 }
+
+func testAccAzureRMVirtualMachineSystemAssignedIdentityTemplate(rInt int, location string) string {


minor we can remove the word Template from the end of this method name

I removed it from new functions, but there are a lot of functions in this file following this pattern..

tombuildsstuff · 2018-06-29T16:14:45Z

azurerm/resource_arm_user_assigned_identity.go

+
+	if resp.IdentityProperties == nil || resp.IdentityProperties.PrincipalID == nil {
+		return fmt.Errorf("Error PrincipalID can't be null for User Assigned Identity %q (Resource Group %q): %+v", resourceName, resGroup, err)
+	}


can we remove this?

(see above reply)

tombuildsstuff · 2018-06-29T16:15:32Z

azurerm/resource_arm_user_assigned_identity_test.go

+			{
+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestMatchResourceAttr(resourceName, "id", regexp.MustCompile(resourceIdRegex)),


can we remove this check? if the ID doesn't match we'll get an error in the CRUD methods

tombuildsstuff · 2018-06-29T16:17:58Z

azurerm/resource_arm_virtual_machine.go

+		for _, id := range *identity.IdentityIds {
+			identity_ids = append(identity_ids, id)
+		}
+		result["identity_ids"] = identity_ids


we need to ensure result["identity_ids"] is set all the time? this ensures we can still detect empty changes; the simplest way to do this is to make this:

identity_ids := make([]string, 0) if identity.IdentityIds != nil { for _, id := range *identity.IdentityIds { identity_ids = append(identity_ids, id) } } result["identity_ids"] = identity_ids

tombuildsstuff · 2018-06-29T16:20:52Z

azurerm/resource_arm_virtual_machine.go

+		identityType = compute.ResourceIdentityTypeUserAssigned
+	case strings.ToLower(string(compute.ResourceIdentityTypeSystemAssigned)):
+		identityType = compute.ResourceIdentityTypeSystemAssigned
+	}


we can make this:

identityType = compute.ResourceIdentityType(identity["type"].(string))

I had what you advice in the first version. The problem with this was the fact that ResourceIdentityType is just a string. So, compute.ResourceIdentityType(identity["type"].(string) persists letters case.

As a result, identityType might be equal to userIdentity and it doesn't match compute.ResourceIdentityTypeUserAssigned string UserIdentity. So, you have to remember about this when you compare it later in code.

I think it's better to converge them during initialization, but I'd like to hear your recommendation.

That's fine - in this case we can ensure the case matches the Constants/Enum's by updating the validation function / removing the Diff Suppress function, which will allow us to parse this directly:

ValidateFunc: validation.StringInSlice([]string{ # .. }, false)

tombuildsstuff · 2018-06-29T16:30:59Z

azurerm/resource_arm_virtual_machine_scale_set.go

+		identityType = compute.ResourceIdentityTypeUserAssigned
+	case strings.ToLower(string(compute.ResourceIdentityTypeSystemAssigned)):
+		identityType = compute.ResourceIdentityTypeSystemAssigned
+	}


See above reply..

logachev · 2018-06-29T19:14:27Z

@tombuildsstuff thank you for the review!
I left a few replies to your comments to figure out the best way to address them and addressed remaining feedback.

tombuildsstuff

hey @logachev

Thanks for pushing those changes - I've left a couple of comments (which I'm going to push a commit for, I hope you don't mind!) - but this otherwise LGTM 👍

Thanks!

tombuildsstuff · 2018-07-02T05:15:20Z

azurerm/resource_arm_user_assigned_identity.go

+
+	log.Printf("[INFO] preparing arguments for Azure ARM user identity creation.")
+
+	resourceName := d.Get("name").(string)


🤦‍♂️ sorry I was thinking one thing and wrote another - can we name this variable name, to match the other resources? The name of the resource itself is fine :)

tombuildsstuff · 2018-07-02T05:16:53Z

azurerm/resource_arm_user_assigned_identity.go

+	d.Set("resource_group_name", resGroup)
+	d.Set("location", resp.Location)
+	d.Set("name", resp.Name)
+	d.Set("principal_id", resp.IdentityProperties.PrincipalID.String())


that makes sense in some scenarios, but would cause an error when running terraform plan (when this is outside of the users control, for instance if Azure returns a different schema than is defined, it shouldn't break Terraform) - as such we need to not return an error here, can update this using the code sample you posted?

tombuildsstuff · 2018-07-02T05:39:13Z

azurerm/resource_arm_user_assigned_identity.go

+		Delete: resourceArmUserAssignedIdentityDelete,
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,
+		},


I'd missed there wasn't an Import test for this (these are tracked in a separate file) - I've included this in the commit (but here's an example); these verify the state set via d.Set matches the configuration used to create it (by creating the resource, then removing it from the state, then running terraform import and ensuring the config matches the state)

``` acctests azurerm TestAccAzureRMUserAssignedIdentity_ === RUN TestAccAzureRMUserAssignedIdentity_importBasic --- PASS: TestAccAzureRMUserAssignedIdentity_importBasic (80.67s) === RUN TestAccAzureRMUserAssignedIdentity_basic --- PASS: TestAccAzureRMUserAssignedIdentity_basic (78.45s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 159.156s ```

tombuildsstuff · 2018-07-02T05:42:38Z

azurerm/resource_arm_user_assigned_identity_test.go

+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testCheckAzureRMVirtualMachineScaleSetDestroy,


rather than using the VMSS CheckDestroy function - I'm going to push a commit to use one specific for User Assigned Identities (this ensures that when the resource is destroyed by Terraform, it's not lingering around)

tombuildsstuff · 2018-07-02T07:36:18Z

User Assigned Identities tests pass:

acctests azurerm TestAccAzureRMUserAssignedIdentity_
=== RUN   TestAccAzureRMUserAssignedIdentity_importBasic
--- PASS: TestAccAzureRMUserAssignedIdentity_importBasic (80.67s)
=== RUN   TestAccAzureRMUserAssignedIdentity_basic
--- PASS: TestAccAzureRMUserAssignedIdentity_basic (78.45s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	159.156s

tombuildsstuff · 2018-07-02T12:22:43Z

VM tests pass:

VM Scale Set tests pass:

logachev · 2018-07-02T16:32:49Z

@tombuildsstuff Your changes look good.
Sorry, I missed import tests and didn't notice destroy function is not some generic function.
Thank you for your assistance!

ghost · 2020-03-30T15:26:02Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

Kirill Logachev added 11 commits June 21, 2018 17:28

Add userAssigned identity support for VM and VMSS

270081f

Update VM and VMSS Identity tests

9d5c108

Add msi user assigned identity package

a5200d8

Add User Assigned Identity resource

b513663

Add User Assigned Identity tests

537e9ff

User assigned identity test for VMSS

add9e05

Add principal_id for user assigned identity

35dcf6e

Update docs

9bcaf99

Fix docs typos & move docs to the right place

fb8fb21

Add link to ms docs

edb0bde

Merge latest from master branch

181a320

tombuildsstuff added enhancement service/vmss Virtual Machine Scale Sets labels Jun 27, 2018

tombuildsstuff suggested changes Jun 29, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jun 29, 2018

tombuildsstuff added this to the Soon milestone Jun 29, 2018

Kirill Logachev added 2 commits June 29, 2018 12:07

Address review feedback

81fa080

Merge with latest master

5a3ddf1

Kirill Logachev added 2 commits June 29, 2018 13:14

Update tests to limit UAI name to 24 symbols

d3c8a5f

More review feedback

7540fd8

tombuildsstuff removed the waiting-response label Jul 2, 2018

tombuildsstuff modified the milestones: Soon, 1.9.0 Jul 2, 2018

tombuildsstuff approved these changes Jul 2, 2018

View reviewed changes

tombuildsstuff reviewed Jul 2, 2018

View reviewed changes

tombuildsstuff merged commit 241525e into hashicorp:master Jul 2, 2018

tombuildsstuff added a commit that referenced this pull request Jul 2, 2018

Updating to include #1448

7e85903

tombuildsstuff mentioned this pull request Jul 2, 2018

Add UserAssigned identity type #1377

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource: `azurerm_user_assigned_identity` + enabled UserAssigned identity for VM and VMSS #1448

New resource: `azurerm_user_assigned_identity` + enabled UserAssigned identity for VM and VMSS #1448

logachev commented Jun 26, 2018

tombuildsstuff left a comment

tombuildsstuff Jun 27, 2018

tombuildsstuff Jun 27, 2018

logachev Jun 29, 2018

tombuildsstuff Jun 27, 2018

tombuildsstuff Jun 27, 2018

tombuildsstuff Jun 27, 2018

logachev Jun 29, 2018

tombuildsstuff Jun 29, 2018

logachev Jun 29, 2018

tombuildsstuff Jun 29, 2018

tombuildsstuff Jun 29, 2018

tombuildsstuff Jun 29, 2018

logachev Jun 29, 2018

tombuildsstuff Jun 29, 2018

tombuildsstuff Jun 29, 2018

logachev Jun 29, 2018

logachev commented Jun 29, 2018

tombuildsstuff left a comment

tombuildsstuff Jul 2, 2018

tombuildsstuff Jul 2, 2018

tombuildsstuff Jul 2, 2018

tombuildsstuff Jul 2, 2018

tombuildsstuff commented Jul 2, 2018

tombuildsstuff commented Jul 2, 2018

logachev commented Jul 2, 2018

ghost commented Mar 30, 2020


		* `resource_group_name` - The name of the resource group in which user assigned identity created.

		* `location` - The location/region where the user assigned identity created.


		log.Printf("[INFO] preparing arguments for Azure ARM user identity creation.")

		resourceName := d.Get("name").(string)

New resource: azurerm_user_assigned_identity + enabled UserAssigned identity for VM and VMSS #1448

New resource: azurerm_user_assigned_identity + enabled UserAssigned identity for VM and VMSS #1448

Conversation

logachev commented Jun 26, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logachev commented Jun 29, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Jul 2, 2018

tombuildsstuff commented Jul 2, 2018

logachev commented Jul 2, 2018

ghost commented Mar 30, 2020

New resource: `azurerm_user_assigned_identity` + enabled UserAssigned identity for VM and VMSS #1448

New resource: `azurerm_user_assigned_identity` + enabled UserAssigned identity for VM and VMSS #1448