Add Managed Service Identity (MSI) support to VM Scale Sets #1018

day-jeff · 2018-03-22T20:34:08Z

This builds on earlier work enabling MSI for VMs. Adds the identity attribute and principal_id property to the VM scale set resource.

…tually identical to recent MSI support for stand-alone VMs

…ale sets.

tombuildsstuff · 2018-03-22T22:37:32Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -803,6 +835,22 @@ func resourceArmVirtualMachineScaleSetDelete(d *schema.ResourceData, meta interf
 	return nil
 }

+func flattenAzureRmVirtualMachineScaleSetIdentity(identity *compute.VirtualMachineScaleSetIdentity) []interface{} {
+	log.Printf("[DEBUG] entered flattenAzureRmVirtualMachineScaleSetIdentity")


can we remove this debug line?

Yep, will do.

tombuildsstuff

hey @dawg89

Thanks for this PR - 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 minor issues and add an acceptance test then this should be good to merge :)

Thanks!

tombuildsstuff · 2018-03-22T22:37:58Z

azurerm/resource_arm_virtual_machine_scale_set.go

+		log.Printf("[DEBUG] attempting to enable MSI")
+		scaleSetParams.Identity = expandAzureRmVirtualMachineScaleSetIdentity(d)
+	} else {
+		log.Printf("[DEBUG] MSI failed")


can we remove the unnecessary else statement here?

tombuildsstuff · 2018-03-22T22:38:37Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -639,6 +662,13 @@ func resourceArmVirtualMachineScaleSetCreate(d *schema.ResourceData, meta interf
 		Zones: zones,
 	}

+	if _, ok := d.GetOk("identity"); ok {
+		log.Printf("[DEBUG] attempting to enable MSI")


could we remove this log message?

tombuildsstuff · 2018-03-22T22:51:48Z

website/docs/r/virtual_machine_scale_set.html.markdown

+    type_handler_version       = "1.0"
+    auto_upgrade_minor_version = true
+    settings                   = "{\"port\": 50342}"
+    protected_settings         = "{}"


I may be wrong, but I think we can omit this if it's empty?

Tested and removed both auto_upgrade_minor_version & protected_settings.

tombuildsstuff · 2018-03-22T22:52:49Z

website/docs/r/virtual_machine_scale_set.html.markdown

@@ -270,6 +270,41 @@ The following arguments are supported:
 * `tier` - (Optional) Specifies the tier of virtual machines in a scale set. Possible values, `standard` or `basic`.
 * `capacity` - (Required) Specifies the number of virtual machines in the scale set.

+`identity` supports the following:
+
+* `type` - (Required) Specifies the identity type of the virtual machine. The only allowable value is `SystemAssigned`. To enable Managed Service Identity (MSI) on all machines in the scale set, an extension with the type "ManagedIdentityExtensionForWindows" or "ManagedIdentityExtensionForLinux" must also be added. The scale set's Service Principal ID (SPN) can be retrieved after the scale set has been created.


of the virtual machine -> of the virtual machine scale set

tombuildsstuff · 2018-03-22T23:03:15Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -37,6 +37,29 @@ func resourceArmVirtualMachineScaleSet() *schema.Resource {

 			"zones": zonesSchema(),

+			"identity": {


can we add an acceptance test covering this use-case?

- Removed debug logging statements and some related code - Removed superfluous lines from the example HCL - Copy edit to readme text - Added acceptance test

day-jeff

Great review, Tom. Thanks!

day-jeff · 2018-03-23T06:01:38Z

website/docs/r/virtual_machine_scale_set.html.markdown

@@ -270,6 +270,41 @@ The following arguments are supported:
 * `tier` - (Optional) Specifies the tier of virtual machines in a scale set. Possible values, `standard` or `basic`.
 * `capacity` - (Required) Specifies the number of virtual machines in the scale set.

+`identity` supports the following:
+
+* `type` - (Required) Specifies the identity type of the virtual machine. The only allowable value is `SystemAssigned`. To enable Managed Service Identity (MSI) on all machines in the scale set, an extension with the type "ManagedIdentityExtensionForWindows" or "ManagedIdentityExtensionForLinux" must also be added. The scale set's Service Principal ID (SPN) can be retrieved after the scale set has been created.


day-jeff · 2018-03-23T06:02:09Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -639,6 +662,13 @@ func resourceArmVirtualMachineScaleSetCreate(d *schema.ResourceData, meta interf
 		Zones: zones,
 	}

+	if _, ok := d.GetOk("identity"); ok {
+		log.Printf("[DEBUG] attempting to enable MSI")


day-jeff · 2018-03-23T06:02:17Z

azurerm/resource_arm_virtual_machine_scale_set.go

+		log.Printf("[DEBUG] attempting to enable MSI")
+		scaleSetParams.Identity = expandAzureRmVirtualMachineScaleSetIdentity(d)
+	} else {
+		log.Printf("[DEBUG] MSI failed")


day-jeff · 2018-03-23T06:03:10Z

website/docs/r/virtual_machine_scale_set.html.markdown

+    type_handler_version       = "1.0"
+    auto_upgrade_minor_version = true
+    settings                   = "{\"port\": 50342}"
+    protected_settings         = "{}"


Tested and removed both auto_upgrade_minor_version & protected_settings.

day-jeff · 2018-03-23T21:25:39Z

All review comments addressed. Ready for round 2. :)

tombuildsstuff

hey @dawg89

Thanks for pushing those updates (and apologies for the delayed re-review here!). I've noted two minor comments but this otherwise LGTM 👍. If we can get those sorted then this should be good to merge :)

Thanks!

tombuildsstuff · 2018-03-30T21:03:09Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -701,6 +728,8 @@ func resourceArmVirtualMachineScaleSetRead(d *schema.ResourceData, meta interfac
 		return fmt.Errorf("[DEBUG] Error setting Virtual Machine Scale Set Sku error: %#v", err)
 	}

+	d.Set("identity", flattenAzureRmVirtualMachineScaleSetIdentity(resp.Identity))


can we change this to:

if err := d.Set("identity", flattenAzureRmVirtualMachineScaleSetIdentity(resp.Identity)); err != nil { return fmt.Error("Error flattening `identity`: %+v", err) }

this allows any bugs (such as the schema not matching) to be caught

tombuildsstuff · 2018-03-30T21:04:37Z

azurerm/resource_arm_virtual_machine_scale_set_test.go

+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMVirtualMachineScaleSetExists(resourceName),
+					testCheckAzureRMVirtualMachineScaleSetMSI(resourceName),


can we remove this function (which checks the API) in favour of checking the value stored in the state (which users will consume)? we can do this via:

resources.TestCheckResourceAttrSet(resourceName, "identity.0.principal_id"),

day-jeff · 2018-03-31T05:54:38Z

More good changes, Tom. Thx for the reviews.

--Jeff

tombuildsstuff

hey @dawg89

Thanks for pushing the latest changes - this now LGTM 👍 I'll kick off the test suite now :)

Thanks!

tombuildsstuff · 2018-04-04T09:53:27Z

@dawg89 just to let you know that the tests picked up a bug in the tests, so I've pushed a commit to fix this: 03a9c31 - I hope you don't mind :)

tombuildsstuff · 2018-04-04T11:36:43Z

All VM tests before the previous commit:

The final commit fixes the broken test:

This is now good to merge 👍 - thanks for this PR @dawg89 :)

herrkunstler · 2018-05-21T16:48:30Z

According to https://docs.microsoft.com/en-us/azure/active-directory/managed-service-identity/overview, "MSI VM extension endpoint: http://localhost:50342/oauth2/token (to be deprecated)." Since the extension endpoint is being deprecated, will this provider be updated to support enabling MSI without the extension (see also https://docs.microsoft.com/en-us/azure/active-directory/managed-service-identity/tutorial-linux-vm-access-arm)?

ghost · 2020-03-31T14:58:20Z

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!

day-jeff added 3 commits March 22, 2018 12:48

Added MSI support to scale sets by adding the Identity attribute. Vir…

a26b836

…tually identical to recent MSI support for stand-alone VMs

Added documentation for enabling Managed Service Identity (MSI) on sc…

af340d4

…ale sets.

Removed extraneous character

2eba517

tombuildsstuff added enhancement service/vmss Virtual Machine Scale Sets labels Mar 22, 2018

tombuildsstuff reviewed Mar 22, 2018

View reviewed changes

removed debug line

8a22743

tombuildsstuff suggested changes Mar 22, 2018

View reviewed changes

Updates to address PR review comments:

44ac146

- Removed debug logging statements and some related code - Removed superfluous lines from the example HCL - Copy edit to readme text - Added acceptance test

day-jeff commented Mar 23, 2018

View reviewed changes

day-jeff added 2 commits March 22, 2018 23:05

Fixed formatting.

642bddf

Fixed camel case error.

0060fa3

tombuildsstuff suggested changes Mar 30, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Mar 30, 2018

day-jeff added 2 commits March 30, 2018 22:44

Addressed PR review requests.

4598daf

Clean up formatting.

c70715f

tombuildsstuff removed the waiting-response label Apr 3, 2018

tombuildsstuff approved these changes Apr 3, 2018

View reviewed changes

Fixing the resource name to match the test

03a9c31

tombuildsstuff merged commit 615d89a into hashicorp:master Apr 4, 2018

tombuildsstuff added a commit that referenced this pull request Apr 4, 2018

Updating to include #1018

8683631

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Managed Service Identity (MSI) support to VM Scale Sets #1018

Add Managed Service Identity (MSI) support to VM Scale Sets #1018

day-jeff commented Mar 22, 2018

tombuildsstuff Mar 22, 2018

day-jeff Mar 22, 2018

tombuildsstuff left a comment

tombuildsstuff Mar 22, 2018

day-jeff Mar 23, 2018

tombuildsstuff Mar 22, 2018

day-jeff Mar 23, 2018

tombuildsstuff Mar 22, 2018

day-jeff Mar 23, 2018

tombuildsstuff Mar 22, 2018

day-jeff Mar 23, 2018

tombuildsstuff Mar 22, 2018

day-jeff Mar 23, 2018

day-jeff left a comment

day-jeff Mar 23, 2018

day-jeff Mar 23, 2018

day-jeff Mar 23, 2018

day-jeff Mar 23, 2018

day-jeff commented Mar 23, 2018

tombuildsstuff left a comment

tombuildsstuff Mar 30, 2018

day-jeff Mar 31, 2018

tombuildsstuff Mar 30, 2018

day-jeff Mar 31, 2018

day-jeff commented Mar 31, 2018

tombuildsstuff left a comment

tombuildsstuff commented Apr 4, 2018

tombuildsstuff commented Apr 4, 2018 •

edited

Loading

herrkunstler commented May 21, 2018

ghost commented Mar 31, 2020

		@@ -37,6 +37,29 @@ func resourceArmVirtualMachineScaleSet() *schema.Resource {

		"zones": zonesSchema(),

		"identity": {

Add Managed Service Identity (MSI) support to VM Scale Sets #1018

Add Managed Service Identity (MSI) support to VM Scale Sets #1018

Conversation

day-jeff commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

day-jeff 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

day-jeff commented Mar 23, 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

day-jeff commented Mar 31, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Apr 4, 2018

tombuildsstuff commented Apr 4, 2018 • edited Loading

herrkunstler commented May 21, 2018

ghost commented Mar 31, 2020

tombuildsstuff commented Apr 4, 2018 •

edited

Loading