Add availability zone support #811

nordbergm · 2018-02-08T16:39:35Z

Hey all,

This PR adds availability zone support to resources that are included in the AZ public preview. I'm unclear on what the policy is on preview features in Terraform, but it would be great if we could get this in.

It should also address feature request #370.

Updated Resources

azurerm_virtual_machine
azurerm_virtual_machine_scale_set
azurerm_public_ip
azurerm_managed_disk
azurerm_lb

Updated Data Sources

azurerm_managed_disk

tombuildsstuff

hey @nordbergm

Thanks for opening this PR :)

I've taken a look through and left some comments in-line - but this mostly looks good to me. Since this functionality is in Public Preview (where users can self register) - we should be able to add this functionality providing we add an info box stating how users can join in the Preview (I've added an example info box in a comment).

Thanks!

tombuildsstuff · 2018-02-10T14:16:27Z

azurerm/resource_arm_managed_disk_test.go

+    storage_account_type = "Standard_LRS"
+    create_option = "Empty"
+    disk_size_gb = "1"
+	zones = ["1"]


minor can we fix the spacing here?

tombuildsstuff · 2018-02-10T14:18:23Z

azurerm/resource_arm_public_ip_test.go

+    location = "${azurerm_resource_group.test.location}"
+    resource_group_name = "${azurerm_resource_group.test.name}"
+    public_ip_address_allocation = "static"
+	zones = ["1"]


minor could we make the spacing consistent here?

tombuildsstuff · 2018-02-10T14:19:08Z

azurerm/resource_arm_virtual_machine_managed_disks_test.go

+
+    os_profile_linux_config {
+		disable_password_authentication = false
+    }


minor could we make the spacing consistent here?

tombuildsstuff · 2018-02-10T14:19:48Z

azurerm/resource_arm_virtual_machine_scale_set_test.go

+  }
+
+  storage_profile_os_disk {
+    name 		  = ""


I believe this is defaulted if not set, so I don't think we need to specify this?

tombuildsstuff · 2018-02-10T14:20:37Z

azurerm/zones.go

+	}
+}
+
+func zoneValuesToStrings(v []interface{}) *[]string {


we'd tend to name this expandZones (since it's parsing the values out) - can we update this to match?

tombuildsstuff · 2018-02-10T17:50:02Z

website/docs/r/virtual_machine.html.markdown

@@ -246,6 +246,7 @@ The following arguments are supported:
 * `network_interface_ids` - (Required) Specifies the list of resource IDs for the network interfaces associated with the virtual machine.
 * `primary_network_interface_id` - (Optional) Specifies the resource ID for the primary network interface associated with the virtual machine.
 * `tags` - (Optional) A mapping of tags to assign to the resource.
+* `zones` - (Optional) A collection containing the availability zone to allocate the Virtual Machine in.


can we add a note (blue box) that this functionality is in Preview:

-> **Please Note**: Availability Zones are [in Preview and only supported in several regions at this time](https://docs.microsoft.com/en-us/azure/availability-zones/az-overview) - as such you must be opted into the Preview to use this functionality. You can [opt into the Availability Zones Preview in the Azure Portal](http://aka.ms/azenroll).

tombuildsstuff · 2018-02-10T17:50:05Z

website/docs/r/public_ip.html.markdown

@@ -60,6 +60,8 @@ The following arguments are supported:

 * `tags` - (Optional) A mapping of tags to assign to the resource.

+* `zones` - (Optional) A collection containing the availability zone to allocate the Public IP in.


can we add a note (blue box) that this functionality is in Preview:

-> **Please Note**: Availability Zones are [in Preview and only supported in several regions at this time](https://docs.microsoft.com/en-us/azure/availability-zones/az-overview) - as such you must be opted into the Preview to use this functionality. You can [opt into the Availability Zones Preview in the Azure Portal](http://aka.ms/azenroll).

tombuildsstuff · 2018-02-10T17:50:08Z

website/docs/r/managed_disk.html.markdown

@@ -106,6 +106,8 @@ The following arguments are supported:

 * `tags` - (Optional) A mapping of tags to assign to the resource.

+* `zones` - (Optional) A collection containing the availability zone to allocate the Managed Disk in.


can we add a note (blue box) that this functionality is in Preview:

-> **Please Note**: Availability Zones are [in Preview and only supported in several regions at this time](https://docs.microsoft.com/en-us/azure/availability-zones/az-overview) - as such you must be opted into the Preview to use this functionality. You can [opt into the Availability Zones Preview in the Azure Portal](http://aka.ms/azenroll).

tombuildsstuff · 2018-02-10T17:50:19Z

website/docs/r/loadbalancer.html.markdown

@@ -58,6 +58,7 @@ The following arguments are supported:
 * `private_ip_address` - (Optional) Private IP Address to assign to the Load Balancer. The last one and first four IPs in any range are reserved and cannot be manually assigned.
 * `private_ip_address_allocation` - (Optional) Defines how a private IP address is assigned. Options are Static or Dynamic.
 * `public_ip_address_id` - (Optional) Reference to Public IP address to be associated with the Load Balancer.
+* `zones` - (Optional) A collection containing the availability zone to allocate the IP in.


can we add a note (blue box) that this functionality is in Preview:

-> **Please Note**: Availability Zones are [in Preview and only supported in several regions at this time](https://docs.microsoft.com/en-us/azure/availability-zones/az-overview) - as such you must be opted into the Preview to use this functionality. You can [opt into the Availability Zones Preview in the Azure Portal](http://aka.ms/azenroll).

tombuildsstuff · 2018-02-10T17:50:21Z

website/docs/d/managed_disk.html.markdown

@@ -111,3 +111,4 @@ resource "azurerm_virtual_machine" "test" {
 * `os_type` - The operating system for managed disk. Valid values are `Linux` or `Windows`
 * `disk_size_gb` - The size of the managed disk in gigabytes.
 * `tags` - A mapping of tags assigned to the resource.
+* `zones` - (Optional) A collection containing the availability zone the managed disk is allocated in.


can we add a note (blue box) that this functionality is in Preview:

-> **Please Note**: Availability Zones are [in Preview and only supported in several regions at this time](https://docs.microsoft.com/en-us/azure/availability-zones/az-overview) - as such you must be opted into the Preview to use this functionality. You can [opt into the Availability Zones Preview in the Azure Portal](http://aka.ms/azenroll).

tombuildsstuff · 2018-02-10T18:57:34Z

Fixes #370

nordbergm · 2018-02-12T17:16:40Z

hey @tombuildsstuff,

Thanks for the review! Your pointers should be sorted now, but let me know if there's anything else.

tombuildsstuff · 2018-02-13T00:48:20Z

azurerm/resource_arm_managed_disk.go

@@ -135,7 +138,8 @@ func resourceArmManagedDiskCreate(d *schema.ResourceData, meta interface{}) erro
 		Sku: &compute.DiskSku{
 			Name: (skuName),
 		},
-		Tags: expandedTags,
+		Tags:  expandedTags,
+		Zones: zones,


Given we set this value always - if a user isn't opted into the Preview most deployments will fail:

* azurerm_managed_disk.test: compute.DisksClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="ResourceTypeNotSupportAvailabilityZones" Message="The resource type 'disks' does not support availability zones at location 'westeurope' and api-version '2017-03-30'."

Can we instead change this to assign Zones only if the field is set? e.g.

if v, ok := d.Get("zones"); ok { zones := expandZones(v.([]interface{})) createDisk.Zones = zones }

which should remove this error

tombuildsstuff · 2018-02-13T00:49:02Z

azurerm/resource_arm_loadbalancer.go

 		frontEndConfig := network.FrontendIPConfiguration{
 			Name: &name,
 			FrontendIPConfigurationPropertiesFormat: &properties,
+			Zones: zones,


from what I can see, this will be the same issue as below (where a user isn't opted into the Preview, this fails) - can we update it to use the pattern identified below?

tombuildsstuff · 2018-02-13T00:49:45Z

azurerm/resource_arm_public_ip.go

@@ -157,7 +160,8 @@ func resourceArmPublicIpCreate(d *schema.ResourceData, meta interface{}) error {
 		Location: &location,
 		Sku:      &sku,
 		PublicIPAddressPropertiesFormat: &properties,
-		Tags: expandTags(tags),
+		Tags:  expandTags(tags),
+		Zones: zones,


from what I can see, this will be the same issue as above (where a user isn't opted into the Preview, this fails) - can we update it to use the pattern identified below?

tombuildsstuff · 2018-02-13T00:49:54Z

azurerm/resource_arm_virtual_machine.go

@@ -577,7 +580,8 @@ func resourceArmVirtualMachineCreate(d *schema.ResourceData, meta interface{}) e
 		Name:                     &name,
 		Location:                 &location,
 		VirtualMachineProperties: &properties,
-		Tags: expandedTags,
+		Tags:  expandedTags,
+		Zones: zones,


from what I can see, this will be the same issue as above (where a user isn't opted into the Preview, this fails) - can we update it to use the pattern identified below?

tombuildsstuff · 2018-02-13T00:50:01Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -564,6 +566,7 @@ func resourceArmVirtualMachineScaleSetCreate(d *schema.ResourceData, meta interf
 	location := d.Get("location").(string)
 	resGroup := d.Get("resource_group_name").(string)
 	tags := d.Get("tags").(map[string]interface{})
+	zones := expandZones(d.Get("zones").([]interface{}))


from what I can see, this will be the same issue as above (where a user isn't opted into the Preview, this fails) - can we update it to use the pattern identified below?

tombuildsstuff · 2018-02-13T00:51:38Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -632,6 +635,7 @@ func resourceArmVirtualMachineScaleSetCreate(d *schema.ResourceData, meta interf
 		Tags:     expandTags(tags),
 		Sku:      sku,
 		VirtualMachineScaleSetProperties: &scaleSetProps,
+		Zones: zones,


from what I can see, this will be the same issue as above (where a user isn't opted into the Preview, this fails) - can we update it to use the pattern identified below?

tombuildsstuff · 2018-02-13T01:00:20Z

hey @nordbergm

Thanks for pushing those updates - I kicked off the test suite here without the Preview enabled (to confirm the existing tests work) - however the existing tests fail as we're setting the Zones field regardless:

* azurerm_managed_disk.test: compute.DisksClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="ResourceTypeNotSupportAvailabilityZones" Message="The resource type 'disks' does not support availability zones at location 'westeurope' and api-version '2017-03-30'."

Can we update the assignment of the Zones field to check if there's any items before setting it, as shown in this comment which would fix the issue. Once that's done - both the existing and the new tests should pass as expected :)

Thanks!

nordbergm · 2018-02-13T14:31:45Z

hey @tombuildsstuff,

Sorry about those test failures. I initially looked at the Azure Go client and how Zones marshalling is defined as omitempty.

Zones           *[]string `json:"zones,omitempty"`

My expectation was that it would omit an empty array from the request. After some testing it seems this is generally the case, but since the field is defined as a pointer, the pointer has to be nil to be omitted.

A perhaps simpler and more central change, would be to have expandZones return nil in case the array is empty. How do you feel about that approach? I'm rather new with Go, so maybe this is some big no no.

nordbergm · 2018-02-16T11:12:06Z

@tombuildsstuff I pushed an example of my suggested approach. Let me know what you think, and if you still experience any issues with the tests.

agup006 · 2018-03-04T19:26:09Z

@tombuildsstuff is the approach @nordbergm pushed ok? Any updates are appreciated

tombuildsstuff · 2018-03-05T22:18:22Z

@agup006 @nordbergm sorry for the delay in re-reviewing this - we're trying to work through the review backlog - I'm taking a look into this now.

tombuildsstuff · 2018-03-05T23:23:38Z

azurerm/resource_arm_loadbalancer.go

@@ -306,6 +310,7 @@ func flattenLoadBalancerFrontendIpConfiguration(ipConfigs *[]network.FrontendIPC
 	for _, config := range *ipConfigs {
 		ipConfig := make(map[string]interface{})
 		ipConfig["name"] = *config.Name
+		ipConfig["zones"] = *config.Zones


so there's a crash here if config.Zones is nil - can we make this:

zones := make([]string, 0) if zs := config.Zones; zs != nil { for _, zone := range *zs { zones = append(zones, zone) } } ipConfig["zones"] = zones

I hope you don't mind, but I've pushed a commit to fix this

tombuildsstuff · 2018-03-05T23:28:29Z

hey @nordbergm @agup006

Thanks for pushing the latest updates - apologies about the delayed review here! The latest updates looks good, however running the tests there appears to be a crash in the Load Balancer resource (which I've commented in-line about)

If we can sort that out, we'll take another look through/kick of the tests - but this should be good :)

Thanks!

Tests:

Data Source tests pass:

Managed Disk resource tests pass:

Public Ip tests pass:

Reminder of the test prefixes for me, for later:

refs/pull/811/merge

TestAccDataSourceAzureRMManagedDisk_
TestAccAzureRMLoadBalancer_
TestAccAzureRMManagedDisk_
TestAccAzureRMPublicIp
TestAccAzureRMVirtualMachineScaleSet_
TestAccAzureRMVirtualMachine_

Tests pass: ``` $ acctests azurerm TestAccAzureRMLoadBalancer_basic === RUN TestAccAzureRMLoadBalancer_basic --- PASS: TestAccAzureRMLoadBalancer_basic (59.25s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 59.281s ```

tombuildsstuff · 2018-03-06T16:44:04Z

Load Balancer tests pass:

tombuildsstuff · 2018-03-06T17:51:31Z

VM/VMSS tests pass, ignoring a transient Azure failure:

tombuildsstuff

Hey @nordbergm

I've pushed a commit to fix the crash identified in the tests and ran the tests - and this now LGTM 👍

Thanks!

nordbergm · 2018-03-07T00:03:48Z

Thanks @tombuildsstuff!

ghost · 2019-03-06T14:33:12Z

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!

nordbergm and others added 9 commits February 6, 2018 15:48

Add zones to eligible resources

7fa4231

Correctly extract zones from LB frontend_ip_configuration response

a0057e0

Virtual machine availability zone tests

5ba9fa3

Virtual machine scale set availability zone tests

3a6e00f

Managed disk availability zone tests

7296944

Public IP availability zone tests

429caa8

Load balancer availability zone tests

3c18df8

Add availability zones to docs

a1a6939

Add availability zones to managed disk data source

66d6d1e

metacpp requested review from mbfrahry and tombuildsstuff February 8, 2018 18:08

metacpp added this to the 1.1.2 milestone Feb 8, 2018

metacpp added the enhancement label Feb 8, 2018

metacpp self-assigned this Feb 9, 2018

metacpp self-requested a review February 9, 2018 18:18

metacpp removed their assignment Feb 9, 2018

tombuildsstuff suggested changes Feb 10, 2018

View reviewed changes

tombuildsstuff added service/availability waiting-response labels Feb 10, 2018

nordbergm added 2 commits February 12, 2018 17:04

Update docs with availability zone preview notes

c2e5fdc

Fix up code review pointers

94cc4ee

nordbergm force-pushed the availability-zones branch from c3b7916 to 94cc4ee Compare February 12, 2018 17:06

tombuildsstuff removed the waiting-response label Feb 12, 2018

tombuildsstuff self-assigned this Feb 12, 2018

tombuildsstuff reviewed Feb 13, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Feb 13, 2018

tombuildsstuff modified the milestones: 1.1.2, 1.1.3 Feb 15, 2018

Ensure empty zones are nil

4f02abd

tombuildsstuff removed the waiting-response label Feb 20, 2018

tombuildsstuff modified the milestones: 1.1.3, 1.1.4 Feb 28, 2018

tombuildsstuff reviewed Mar 5, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Mar 5, 2018

LB: fixing a crash where Zones is nil

5bc57f1

Tests pass: ``` $ acctests azurerm TestAccAzureRMLoadBalancer_basic === RUN TestAccAzureRMLoadBalancer_basic --- PASS: TestAccAzureRMLoadBalancer_basic (59.25s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 59.281s ```

tombuildsstuff force-pushed the availability-zones branch from 77ad002 to 5bc57f1 Compare March 6, 2018 16:35

tombuildsstuff removed the waiting-response label Mar 6, 2018

tombuildsstuff approved these changes Mar 6, 2018

View reviewed changes

tombuildsstuff merged commit 218dc6e into hashicorp:master Mar 6, 2018

tombuildsstuff added a commit that referenced this pull request Mar 6, 2018

Updating to include #811

42b105f

tombuildsstuff mentioned this pull request Mar 14, 2018

Feature Request - Availability Zones #370

Closed

ghost locked and limited conversation to collaborators Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add availability zone support #811

Add availability zone support #811

nordbergm commented Feb 8, 2018

tombuildsstuff left a comment

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff Feb 10, 2018

tombuildsstuff commented Feb 10, 2018

nordbergm commented Feb 12, 2018

tombuildsstuff Feb 13, 2018

tombuildsstuff Feb 13, 2018

tombuildsstuff Feb 13, 2018

tombuildsstuff Feb 13, 2018

tombuildsstuff Feb 13, 2018

tombuildsstuff Feb 13, 2018

tombuildsstuff commented Feb 13, 2018

nordbergm commented Feb 13, 2018

nordbergm commented Feb 16, 2018

agup006 commented Mar 4, 2018 •

edited

Loading

tombuildsstuff commented Mar 5, 2018

tombuildsstuff Mar 5, 2018

tombuildsstuff Mar 6, 2018

tombuildsstuff commented Mar 5, 2018

tombuildsstuff commented Mar 6, 2018

tombuildsstuff commented Mar 6, 2018

tombuildsstuff left a comment

nordbergm commented Mar 7, 2018

ghost commented Mar 6, 2019

		@@ -60,6 +60,8 @@ The following arguments are supported:

		* `tags` - (Optional) A mapping of tags to assign to the resource.

		* `zones` - (Optional) A collection containing the availability zone to allocate the Public IP in.

		@@ -106,6 +106,8 @@ The following arguments are supported:

		* `tags` - (Optional) A mapping of tags to assign to the resource.

		* `zones` - (Optional) A collection containing the availability zone to allocate the Managed Disk in.

Add availability zone support #811

Add availability zone support #811

Conversation

nordbergm commented Feb 8, 2018

Updated Resources

Updated Data Sources

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

tombuildsstuff commented Feb 10, 2018

nordbergm commented Feb 12, 2018

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

tombuildsstuff commented Feb 13, 2018

nordbergm commented Feb 13, 2018

nordbergm commented Feb 16, 2018

agup006 commented Mar 4, 2018 • edited Loading

tombuildsstuff commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Mar 5, 2018

tombuildsstuff commented Mar 6, 2018

tombuildsstuff commented Mar 6, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

nordbergm commented Mar 7, 2018

ghost commented Mar 6, 2019

agup006 commented Mar 4, 2018 •

edited

Loading