`azurerm_network_interface` - support for multiple IP Configurations / setting the primary #245

tombuildsstuff · 2017-08-15T17:21:15Z

Swapping ip_configuration from a Set -> List
Switching over to using constants within the SDK
Adding support for setting the Primary ip_configuration. Migrated across provider/azurerm: suppport for primary network interface terraform#12277
Detecting changes in the network_security_group_id. Fixes Terraform does not change security group of network interface on Azure #60
Tests covering multiple ip_configurations. Fixes Enhancement: Allow more than one IP Configuration on azurerm_network_interface #26 & a deadlock issue
Refactoring the tests to use Dynamic ID's
Update Documentation
Confirm no state migration needed between Master -> this branch

Fixes a deadlock issue for multiple Subnets in the same Virtual Network

dominik-lekse · 2017-08-16T08:53:21Z

Great work, I have been looking forward for the primary ip_configuration feature.

grubernaut

LGTM, minor question

grubernaut · 2017-08-16T13:26:03Z

azurerm/resource_arm_network_interface.go

@@ -356,7 +362,7 @@ func resourceArmNetworkInterfaceDelete(d *schema.ResourceData, meta interface{})
 		defer azureRMUnlockByName(networkSecurityGroupName, networkSecurityGroupResourceName)
 	}

-	configs := d.Get("ip_configuration").(*schema.Set).List()
+	configs := d.Get("ip_configuration").([]interface{})


could this cast panic?

radeksimko

I left you some comments/questions there - the most important one is regarding state migration.

radeksimko · 2017-08-16T13:24:09Z

azurerm/resource_arm_network_interface.go

-		if *iface.VirtualMachine.ID != "" {
-			d.Set("virtual_machine_id", *iface.VirtualMachine.ID)
-		}
+		d.Set("virtual_machine_id", *iface.VirtualMachine.ID)


I think you can omit the * here to prevent potential crashes?

radeksimko · 2017-08-16T13:26:21Z

azurerm/resource_arm_network_interface.go

 			"virtual_machine_id": {
 				Type:     schema.TypeString,
 				Optional: true,
 				Computed: true,
 			},

 			"ip_configuration": {
-				Type:     schema.TypeSet,
+				Type:     schema.TypeList,


Are you sure this doesn't require state migration? It's the most common example for migration, IMO 🤔

I tested with a resource/state created on master -> this branch and apparently not ¯_(ツ)_/¯

radeksimko · 2017-08-16T13:28:17Z

azurerm/resource_arm_network_interface.go

@@ -356,7 +362,7 @@ func resourceArmNetworkInterfaceDelete(d *schema.ResourceData, meta interface{})
 		defer azureRMUnlockByName(networkSecurityGroupName, networkSecurityGroupResourceName)
 	}

-	configs := d.Get("ip_configuration").(*schema.Set).List()
+	configs := d.Get("ip_configuration").([]interface{})


Correct me if I'm wrong, but I think this will crash with the old state (with Set) without migration.

I may have tested something wrong, but this appeared to be fine when testing the migration from a resource/state created with master?

radeksimko · 2017-08-16T13:31:24Z

azurerm/resource_arm_network_interface_test.go

+	rInt := acctest.RandInt()
+	location := testLocation()
+	config := testAccAzureRMNetworkInterface_basicWithNetworkSecurityGroup(rInt, location)
+	updatedConfig := testAccAzureRMNetworkInterface_basic(rInt, location)


Nitpick, but I think config & updatedConfig don't need to be variables for that single purpose a few lines below. 🙂

radeksimko · 2017-08-16T13:34:50Z

website/docs/r/network_interface.html.markdown

@@ -91,11 +92,13 @@ The `ip_configuration` block supports:

 * `load_balancer_inbound_nat_rules_ids` - (Optional) List of Load Balancer Inbound Nat Rules IDs involving this NIC

+* `primary` - (Optional) Is this the Primary Network Interface? If set to `true` this should be the first `ip_configuration` in the array.


If set to true this should be the first ip_configuration in the array

This feels a little bit unintuitive for the user - is it because the API requires it? Can't we just sort it for the user prior to sending the request to the API?

so, the user can submit the second (or later) ip_configuration as primary and that'll update but lead to a perpetual diff, as it's returned first in the response; we could probably do something there, but this mirrors the UI

The alternative I considered here was just assuming the first would be the primary, but that may not necessarily be optimal? WDYT?

The alternative I considered here was just assuming the first would be the primary

Hm, I think making assumptions is probably even worse UX. This is at the very least explicit. So if you think it's ok then go ahead - I just thought as a user I'd be little bit annoyed or confused 🙂

radeksimko

As discussed via Slack - this doesn't require migration. (TIL)

tombuildsstuff · 2017-08-17T11:59:57Z

Tests pass:

ghost · 2020-04-01T15:27:16Z

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!

tombuildsstuff added 7 commits August 15, 2017 12:02

Leaning on the SDK constants

ef71301

Continued refactoring

f6519f0

Refactoring the ip_configuration from a Set -> List

5a10479

Consistency for the NIC ip_configuration block

dafc5c2

Support for setting the Primary NIC ID

f5d67da

Fixes a deadlock issue for multiple Subnets in the same Virtual Network

Adding tests for multiple subnets

392095c

Adding a test to check the network_security_group_id is removed

4a97740

tombuildsstuff added bug enhancement labels Aug 15, 2017

tombuildsstuff added 2 commits August 15, 2017 18:29

Making the tests use dynamic data

53c27ad

Linting

85eec29

tombuildsstuff changed the title ~~[WIP] azurerm_network_interface - support for multiple IP Configurations / setting the primary~~ azurerm_network_interface - support for multiple IP Configurations / setting the primary Aug 15, 2017

tombuildsstuff requested a review from grubernaut August 15, 2017 17:40

documentation

2240a5a

tombuildsstuff mentioned this pull request Aug 15, 2017

provider/azurerm: suppport for primary network interface hashicorp/terraform#12277

Closed

tombuildsstuff requested review from radeksimko and removed request for grubernaut August 16, 2017 10:04

Check for ensuring one of the ip_configurations is set to primary

22fd360

grubernaut approved these changes Aug 16, 2017

View reviewed changes

radeksimko suggested changes Aug 16, 2017

View reviewed changes

radeksimko approved these changes Aug 16, 2017

View reviewed changes

tombuildsstuff mentioned this pull request Aug 16, 2017

azurerm_public_ip support for IPv6 #250

Closed

tombuildsstuff added 2 commits August 17, 2017 12:55

Network Security Group ID needs to not be computed

a6bed06

Fixing the multiple LB tests

22c2379

tombuildsstuff force-pushed the primary-nic branch from 3161160 to 22c2379 Compare August 17, 2017 11:59

tombuildsstuff merged commit fc06f59 into master Aug 17, 2017

tombuildsstuff deleted the primary-nic branch August 17, 2017 12:00

tombuildsstuff added a commit that referenced this pull request Aug 17, 2017

Updating to include #245

e7aafdb

tombuildsstuff added a commit that referenced this pull request Aug 17, 2017

Fixing the description of #245

00a58f0

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_network_interface` - support for multiple IP Configurations / setting the primary #245

`azurerm_network_interface` - support for multiple IP Configurations / setting the primary #245

tombuildsstuff commented Aug 15, 2017 •

edited

Loading

dominik-lekse commented Aug 16, 2017

grubernaut left a comment

grubernaut Aug 16, 2017

radeksimko left a comment

radeksimko Aug 16, 2017

radeksimko Aug 16, 2017

tombuildsstuff Aug 16, 2017

radeksimko Aug 16, 2017

tombuildsstuff Aug 16, 2017

radeksimko Aug 16, 2017

radeksimko Aug 16, 2017

tombuildsstuff Aug 16, 2017

radeksimko Aug 16, 2017

radeksimko left a comment

tombuildsstuff commented Aug 17, 2017

ghost commented Apr 1, 2020

		@@ -91,11 +92,13 @@ The `ip_configuration` block supports:

		* `load_balancer_inbound_nat_rules_ids` - (Optional) List of Load Balancer Inbound Nat Rules IDs involving this NIC

		* `primary` - (Optional) Is this the Primary Network Interface? If set to `true` this should be the first `ip_configuration` in the array.

azurerm_network_interface - support for multiple IP Configurations / setting the primary #245

azurerm_network_interface - support for multiple IP Configurations / setting the primary #245

Conversation

tombuildsstuff commented Aug 15, 2017 • edited Loading

dominik-lekse commented Aug 16, 2017

grubernaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko 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

radeksimko left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Aug 17, 2017

ghost commented Apr 1, 2020

`azurerm_network_interface` - support for multiple IP Configurations / setting the primary #245

`azurerm_network_interface` - support for multiple IP Configurations / setting the primary #245

tombuildsstuff commented Aug 15, 2017 •

edited

Loading