[MS] provider/azurerm: fixes for bug #8227 and bug #11226 #13877

echuvyrov · 2017-04-23T10:09:51Z

This PR should partially fix #8227 and #11226 which I believe is a duplicate of #8227. The fix should preserve all of the subnet properties, including routeTable association, on vNet update.

tombuildsstuff

Hi @echuvyrov

Thanks for this PR - apologies for the delay in reviewing this. I've reviewed and left some comments in-line :)

There's also a few remaining tasks which I couldn't find a place for in-line:

Add some extra tests:
- An Import Test covering a Subnet with a Route Table
- An Import Test covering a Virtual Network with a Route Table
- An Acceptance Test covering updating a Virtual Network
- An Acceptance Test covering adding/removing a Route Table from a Virtual network
Update the Documentation to reflect the new field on Virtual Network

However it otherwise looks good - I've run the acceptance tests and they pass:

$ envchain azurerm make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMSubnet'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/02 09:32:54 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMSubnet -timeout 120m
=== RUN   TestAccAzureRMSubnet_importBasic
--- PASS: TestAccAzureRMSubnet_importBasic (132.34s)
=== RUN   TestAccAzureRMSubnet_basic
--- PASS: TestAccAzureRMSubnet_basic (135.57s)
=== RUN   TestAccAzureRMSubnet_routeTable
--- PASS: TestAccAzureRMSubnet_routeTable (176.13s)
=== RUN   TestAccAzureRMSubnet_routeTable_subnetInline
--- PASS: TestAccAzureRMSubnet_routeTable_subnetInline (124.82s)
=== RUN   TestAccAzureRMSubnet_disappears
--- PASS: TestAccAzureRMSubnet_disappears (135.10s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	703.985s

$ envchain azurerm make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMVirtualNetwork'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/02 10:05:39 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMVirtualNetwork -timeout 120m
=== RUN   TestAccAzureRMVirtualNetworkPeering_importBasic
--- PASS: TestAccAzureRMVirtualNetworkPeering_importBasic (135.31s)
=== RUN   TestAccAzureRMVirtualNetwork_importBasic
--- PASS: TestAccAzureRMVirtualNetwork_importBasic (93.71s)
=== RUN   TestAccAzureRMVirtualNetworkPeering_basic
--- PASS: TestAccAzureRMVirtualNetworkPeering_basic (158.53s)
=== RUN   TestAccAzureRMVirtualNetworkPeering_disappears
--- PASS: TestAccAzureRMVirtualNetworkPeering_disappears (119.50s)
=== RUN   TestAccAzureRMVirtualNetworkPeering_update
--- PASS: TestAccAzureRMVirtualNetworkPeering_update (163.05s)
=== RUN   TestAccAzureRMVirtualNetwork_basic
--- PASS: TestAccAzureRMVirtualNetwork_basic (76.80s)
=== RUN   TestAccAzureRMVirtualNetwork_disappears
--- PASS: TestAccAzureRMVirtualNetwork_disappears (104.64s)
=== RUN   TestAccAzureRMVirtualNetwork_withTags
--- PASS: TestAccAzureRMVirtualNetwork_withTags (116.81s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	968.374s

Thanks! :)

tombuildsstuff · 2017-04-24T11:23:08Z

builtin/providers/azurerm/resource_arm_subnet_test.go

+
+	ri := acctest.RandInt()
+	//initConfig := fmt.Sprintf(testAccAzureRMSubnet_routeTable_subnetInline, ri, ri, ri)
+	updatedConfig := fmt.Sprintf(testAccAzureRMSubnet_updatedRouteTable, ri, ri, ri)


Is this meant to be using the testAccAzureRMSubnet_routeTable_subnetInline config?

tombuildsstuff · 2017-05-02T08:02:56Z

builtin/providers/azurerm/resource_arm_virtual_network.go

+		if resp.StatusCode == http.StatusNotFound {
+			return existingSubnet
+		}
+	}


Can we raise this error if it's not a 404? (That way we should be able to return nil for the existingSubnet, and populate the SubnetPropertiesFormat/AddressPrefix below in the same call)

tombuildsstuff · 2017-05-02T08:16:59Z

builtin/providers/azurerm/resource_arm_subnet_test.go

@@ -30,6 +32,55 @@ func TestAccAzureRMSubnet_basic(t *testing.T) {
 	})
 }

+func TestAccAzureRMSubnet_routeTable(t *testing.T) {


Given this is updating, can we rename this TestAccAzureRMSubnet_routeTableUpdate?

tombuildsstuff · 2017-05-02T08:19:33Z

builtin/providers/azurerm/resource_arm_subnet_test.go

@@ -171,3 +278,145 @@ resource "azurerm_route" "test" {
 	next_hop_in_ip_address = "10.10.1.1" 
 }
 `
+
+var testAccAzureRMSubnet_routeTable = `
+resource "azurerm_resource_group" "rtTable2" {


minor: we tend to use test as the resource name within tests, unless there's multiple of the resource (e.g. when testing the creation of 3 subnets) - could we update these to match?

tombuildsstuff · 2017-05-02T08:31:13Z

builtin/providers/azurerm/resource_arm_subnet_test.go

+}`
+
+var testAccAzureRMSubnet_routeTable_subnetInline = `
+resource "azurerm_resource_group" "rtTable2" {


(same here)

tombuildsstuff · 2017-05-02T08:31:25Z

builtin/providers/azurerm/resource_arm_subnet_test.go

+}`
+
+var testAccAzureRMSubnet_updatedRouteTable = `
+resource "azurerm_resource_group" "rtTable2" {


(same here)

tombuildsstuff · 2017-05-02T09:29:24Z

builtin/providers/azurerm/resource_arm_virtual_network.go

@@ -268,6 +293,40 @@ func resourceAzureSubnetHash(v interface{}) int {
 	}
 	return hashcode.String(subnet)
 }
+func getExistingSubnet(resGroup string, vnetName string, subnetName string, meta interface{}) network.Subnet {


In order to raise an error, can we update this to return a (*network.Subnet, error)?

echuvyrov · 2017-05-10T14:25:14Z

@tombuildsstuff I made the changes requested and removed the route_table definition from the vNet - additional tests revealed that there was a collision between this way of adding a route table to subnet and the other, more preferred way as a root-level resource. Please review and let me know if the changes address all the comments you've raised.

tombuildsstuff

Hi @echuvyrov

Thanks for pushing those updates - I've taken another look and left some comments in-line, but this otherwise looks good :)

Thanks!

tombuildsstuff · 2017-05-17T09:42:54Z

builtin/providers/azurerm/resource_arm_subnet_test.go

+}
+
+resource "azurerm_route_table" "test" {
+  name                = "test-RT"


can we rename this acctest-%d, to ensure it's unique across test runs?

tombuildsstuff · 2017-05-17T09:43:05Z

builtin/providers/azurerm/resource_arm_subnet_test.go

+}
+
+resource "azurerm_route" "route_a" {
+  name                = "TestRouteA"


(same here)

tombuildsstuff · 2017-05-17T09:43:16Z

builtin/providers/azurerm/resource_arm_subnet_test.go

+}
+
+resource "azurerm_network_security_group" "test_secgroup" {
+    name = "acceptanceTestSecurityGroup1"


(same here)

tombuildsstuff · 2017-05-17T09:43:30Z

builtin/providers/azurerm/resource_arm_subnet_test.go

+}
+
+resource "azurerm_route" "route_a" {
+  name                = "TestRouteA"


(same here)

tombuildsstuff · 2017-05-17T09:43:32Z

builtin/providers/azurerm/resource_arm_subnet_test.go

+}
+
+resource "azurerm_route_table" "test" {
+  name                = "test-RT"


(same here)

tombuildsstuff · 2017-05-17T09:45:09Z

builtin/providers/azurerm/resource_arm_virtual_network.go

@@ -257,16 +278,55 @@ func getVirtualNetworkProperties(d *schema.ResourceData) *network.VirtualNetwork
 			DNSServers: &dnses,
 		},
 		Subnets: &subnets,
-	}
+	}, nil


given we're returning two objects here, IMO this would read cleaner if we assigned this to a variable and then returned those, e.g.:

properties := &network.... return properties, nil

tombuildsstuff · 2017-05-17T09:48:09Z

builtin/providers/azurerm/resource_arm_virtual_network.go

+	if m["security_group"] != nil {
+		buf.WriteString(fmt.Sprintf("%s-", m["security_group"].(string)))
+	}
+	return hashcode.String(buf.String())


given a - has been added at the end of each value this hash code is going to be different, and thus a change will show up as required on the plan. To avoid this, we can either increment the Schema Version and write a State Migration - or just remove the - from these strings (which I would suggest here :))

tombuildsstuff · 2017-05-17T09:49:35Z

builtin/providers/azurerm/resource_arm_virtual_network.go

-		subnet = subnet + securityGroup.(string)
+	buf.WriteString(fmt.Sprintf("%s-", m["name"].(string)))
+	buf.WriteString(fmt.Sprintf("%s-", m["address_prefix"].(string)))
+	if m["security_group"] != nil {


I believe we can make this cleaner by using:

if v, ok := m["security_group"]; ok { buf.WriteString(v.(string)) }

tombuildsstuff · 2017-05-17T09:55:13Z

builtin/providers/azurerm/resource_arm_subnet_test.go

@@ -171,3 +257,108 @@ resource "azurerm_route" "test" {
 	next_hop_in_ip_address = "10.10.1.1" 
 }
 `
+
+var testAccAzureRMSubnet_routeTable = `


FYI for newer test config's we're tending to make these functions which take an Int and handle formatting the string, rather than spreading the string format throughout the test functions (given we're now using them in multiple places), i.e.

func testAccAzureRMSubnet_routeTable(rInt int) string { return fmt.Sprintf(....., rInt,rInt) }

echuvyrov · 2017-05-19T19:37:23Z

@tombuildsstuff I made the changes requested, I think those will definitely improve code - thanks so much!

tombuildsstuff

Hi @echuvyrov

Thanks for pushing the latest changes - I've taken another look and this LGTM :)

The tests pass:

$ envchain azurerm make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMVirtualNetwork'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/01 16:19:48 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMVirtualNetwork -timeout 120m
=== RUN   TestAccAzureRMVirtualNetworkPeering_importBasic
--- PASS: TestAccAzureRMVirtualNetworkPeering_importBasic (141.83s)
=== RUN   TestAccAzureRMVirtualNetwork_importBasic
--- PASS: TestAccAzureRMVirtualNetwork_importBasic (85.22s)
=== RUN   TestAccAzureRMVirtualNetworkPeering_basic
--- PASS: TestAccAzureRMVirtualNetworkPeering_basic (142.01s)
=== RUN   TestAccAzureRMVirtualNetworkPeering_disappears
--- PASS: TestAccAzureRMVirtualNetworkPeering_disappears (130.45s)
=== RUN   TestAccAzureRMVirtualNetworkPeering_update
--- PASS: TestAccAzureRMVirtualNetworkPeering_update (194.56s)
=== RUN   TestAccAzureRMVirtualNetwork_basic
--- PASS: TestAccAzureRMVirtualNetwork_basic (66.86s)
=== RUN   TestAccAzureRMVirtualNetwork_disappears
--- PASS: TestAccAzureRMVirtualNetwork_disappears (82.76s)
=== RUN   TestAccAzureRMVirtualNetwork_withTags
--- PASS: TestAccAzureRMVirtualNetwork_withTags (92.87s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	936.588s

$ envchain azurerm make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMSubnet'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/01 16:23:03 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMSubnet -timeout 120m
=== RUN   TestAccAzureRMSubnet_importBasic
--- PASS: TestAccAzureRMSubnet_importBasic (142.12s)
=== RUN   TestAccAzureRMSubnet_importWithRouteTable
--- PASS: TestAccAzureRMSubnet_importWithRouteTable (131.49s)
=== RUN   TestAccAzureRMSubnet_basic
--- PASS: TestAccAzureRMSubnet_basic (144.82s)
=== RUN   TestAccAzureRMSubnet_routeTableUpdate
--- PASS: TestAccAzureRMSubnet_routeTableUpdate (159.74s)
=== RUN   TestAccAzureRMSubnet_disappears
--- PASS: TestAccAzureRMSubnet_disappears (176.57s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	754.753s

Thanks!

ghost · 2020-04-11T02:33:32Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

echuvyrov changed the title ~~Fixes for bug #8227 and bug #11226~~ provider/azurerm fixes for bug #8227 and bug #11226 Apr 23, 2017

echuvyrov changed the title ~~provider/azurerm fixes for bug #8227 and bug #11226~~ provider/azurerm: fixes for bug #8227 and bug #11226 Apr 23, 2017

tombuildsstuff added enhancement provider/azurerm labels Apr 24, 2017

tombuildsstuff self-assigned this Apr 24, 2017

tombuildsstuff reviewed May 2, 2017

View reviewed changes

tombuildsstuff added the waiting-response An issue/pull request is waiting for a response from the community label May 2, 2017

echuvyrov added 4 commits May 12, 2017 15:33

Resolved merge conflicts

b886af2

Changes conforming to HashiCorp guidelines and additional bug fixes

39245c2

Rebase merge

9f9ec6a

Rebase merge

db33315

echuvyrov force-pushed the bug8227 branch from a6c85f3 to db33315 Compare May 13, 2017 05:07

Merging changes

d25f614

tombuildsstuff removed the waiting-response An issue/pull request is waiting for a response from the community label May 16, 2017

tombuildsstuff reviewed May 17, 2017

View reviewed changes

tombuildsstuff added the waiting-response An issue/pull request is waiting for a response from the community label May 17, 2017

Changes to tests and code constructs

a1ac0c5

echuvyrov changed the title ~~provider/azurerm: fixes for bug #8227 and bug #11226~~ [MS] provider/azurerm: fixes for bug #8227 and bug #11226 May 19, 2017

tombuildsstuff removed the waiting-response An issue/pull request is waiting for a response from the community label Jun 1, 2017

tombuildsstuff approved these changes Jun 1, 2017

View reviewed changes

tombuildsstuff merged commit acd586a into hashicorp:master Jun 1, 2017

tombuildsstuff added a commit that referenced this pull request Jun 1, 2017

Updating the changelog to include #13877

4e878aa

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MS] provider/azurerm: fixes for bug #8227 and bug #11226 #13877

[MS] provider/azurerm: fixes for bug #8227 and bug #11226 #13877

echuvyrov commented Apr 23, 2017

tombuildsstuff left a comment

tombuildsstuff Apr 24, 2017

tombuildsstuff May 2, 2017

tombuildsstuff May 2, 2017

tombuildsstuff May 2, 2017

tombuildsstuff May 2, 2017

tombuildsstuff May 2, 2017

tombuildsstuff May 2, 2017

echuvyrov commented May 10, 2017 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff May 17, 2017

tombuildsstuff May 17, 2017

tombuildsstuff May 17, 2017

tombuildsstuff May 17, 2017

tombuildsstuff May 17, 2017

tombuildsstuff May 17, 2017

tombuildsstuff May 17, 2017

tombuildsstuff May 17, 2017

tombuildsstuff May 17, 2017

echuvyrov commented May 19, 2017

tombuildsstuff left a comment

ghost commented Apr 11, 2020

[MS] provider/azurerm: fixes for bug #8227 and bug #11226 #13877

[MS] provider/azurerm: fixes for bug #8227 and bug #11226 #13877

Conversation

echuvyrov commented Apr 23, 2017

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

echuvyrov commented May 10, 2017 • edited Loading

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

echuvyrov commented May 19, 2017

tombuildsstuff left a comment

Choose a reason for hiding this comment

ghost commented Apr 11, 2020

echuvyrov commented May 10, 2017 •

edited

Loading