From acd586adf2d0b1c830c49a1261092ae3c57b70f6 Mon Sep 17 00:00:00 2001 From: Eugene Chuvyrov Date: Thu, 1 Jun 2017 09:40:30 -0700 Subject: [PATCH] [MS] provider/azurerm: fixes for bug #8227 and bug #11226 (#13877) * Resolved merge conflicts * Changes conforming to HashiCorp guidelines and additional bug fixes * Rebase merge * Rebase merge * Merging changes * Changes to tests and code constructs --- .../azurerm/import_arm_subnet_test.go | 27 ++- .../azurerm/resource_arm_subnet_test.go | 205 +++++++++++++++++- .../azurerm/resource_arm_virtual_network.go | 83 ++++++- .../resource_arm_virtual_network_test.go | 6 +- 4 files changed, 301 insertions(+), 20 deletions(-) diff --git a/builtin/providers/azurerm/import_arm_subnet_test.go b/builtin/providers/azurerm/import_arm_subnet_test.go index 8384d8b3f..3b16097db 100644 --- a/builtin/providers/azurerm/import_arm_subnet_test.go +++ b/builtin/providers/azurerm/import_arm_subnet_test.go @@ -1,7 +1,6 @@ package azurerm import ( - "fmt" "testing" "github.com/hashicorp/terraform/helper/acctest" @@ -12,7 +11,31 @@ func TestAccAzureRMSubnet_importBasic(t *testing.T) { resourceName := "azurerm_subnet.test" ri := acctest.RandInt() - config := fmt.Sprintf(testAccAzureRMSubnet_basic, ri, ri, ri, ri, ri) + config := testAccAzureRMSubnet_basic(ri) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureRMSubnetDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: config, + }, + + resource.TestStep{ + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccAzureRMSubnet_importWithRouteTable(t *testing.T) { + resourceName := "azurerm_subnet.test" + + ri := acctest.RandInt() + config := testAccAzureRMSubnet_routeTable(ri) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, diff --git a/builtin/providers/azurerm/resource_arm_subnet_test.go b/builtin/providers/azurerm/resource_arm_subnet_test.go index 2c8016ba6..06d8ba473 100644 --- a/builtin/providers/azurerm/resource_arm_subnet_test.go +++ b/builtin/providers/azurerm/resource_arm_subnet_test.go @@ -2,7 +2,9 @@ package azurerm import ( "fmt" + "log" "net/http" + "strings" "testing" "github.com/hashicorp/terraform/helper/acctest" @@ -13,7 +15,7 @@ import ( func TestAccAzureRMSubnet_basic(t *testing.T) { ri := acctest.RandInt() - config := fmt.Sprintf(testAccAzureRMSubnet_basic, ri, ri, ri, ri, ri) + config := testAccAzureRMSubnet_basic(ri) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -30,10 +32,38 @@ func TestAccAzureRMSubnet_basic(t *testing.T) { }) } +func TestAccAzureRMSubnet_routeTableUpdate(t *testing.T) { + + ri := acctest.RandInt() + initConfig := testAccAzureRMSubnet_routeTable(ri) + updatedConfig := testAccAzureRMSubnet_updatedRouteTable(ri) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testCheckAzureRMSubnetDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: initConfig, + Check: resource.ComposeTestCheckFunc( + testCheckAzureRMSubnetExists("azurerm_subnet.test"), + ), + }, + + resource.TestStep{ + Config: updatedConfig, + Check: resource.ComposeTestCheckFunc( + testCheckAzureRMSubnetRouteTableExists("azurerm_subnet.test", fmt.Sprintf("acctest-%d", ri)), + ), + }, + }, + }) +} + func TestAccAzureRMSubnet_disappears(t *testing.T) { ri := acctest.RandInt() - config := fmt.Sprintf(testAccAzureRMSubnet_basic, ri, ri, ri, ri, ri) + config := testAccAzureRMSubnet_basic(ri) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -60,6 +90,8 @@ func testCheckAzureRMSubnetExists(name string) resource.TestCheckFunc { return fmt.Errorf("Not found: %s", name) } + log.Printf("[INFO] Checking Subnet addition.") + name := rs.Primary.Attributes["name"] vnetName := rs.Primary.Attributes["virtual_network_name"] resourceGroup, hasResourceGroup := rs.Primary.Attributes["resource_group_name"] @@ -78,6 +110,60 @@ func testCheckAzureRMSubnetExists(name string) resource.TestCheckFunc { return fmt.Errorf("Bad: Subnet %q (resource group: %q) does not exist", name, resourceGroup) } + if resp.RouteTable == nil { + return fmt.Errorf("Bad: Subnet %q (resource group: %q) does not contain route tables after add", name, resourceGroup) + } + + return nil + } +} + +func testCheckAzureRMSubnetRouteTableExists(subnetName string, routeTableId string) resource.TestCheckFunc { + return func(s *terraform.State) error { + // Ensure we have enough information in state to look up in API + rs, ok := s.RootModule().Resources[subnetName] + if !ok { + return fmt.Errorf("Not found: %s", subnetName) + } + + log.Printf("[INFO] Checking Subnet update.") + + name := rs.Primary.Attributes["name"] + vnetName := rs.Primary.Attributes["virtual_network_name"] + resourceGroup, hasResourceGroup := rs.Primary.Attributes["resource_group_name"] + if !hasResourceGroup { + return fmt.Errorf("Bad: no resource group found in state for subnet: %s", name) + } + + vnetConn := testAccProvider.Meta().(*ArmClient).vnetClient + vnetResp, vnetErr := vnetConn.Get(resourceGroup, vnetName, "") + if vnetErr != nil { + return fmt.Errorf("Bad: Get on vnetClient: %s", vnetErr) + } + + if vnetResp.Subnets == nil { + return fmt.Errorf("Bad: Vnet %q (resource group: %q) does not have subnets after update", vnetName, resourceGroup) + } + + conn := testAccProvider.Meta().(*ArmClient).subnetClient + + resp, err := conn.Get(resourceGroup, vnetName, name, "") + if err != nil { + return fmt.Errorf("Bad: Get on subnetClient: %s", err) + } + + if resp.StatusCode == http.StatusNotFound { + return fmt.Errorf("Bad: Subnet %q (resource group: %q) does not exist", subnetName, resourceGroup) + } + + if resp.RouteTable == nil { + return fmt.Errorf("Bad: Subnet %q (resource group: %q) does not contain route tables after update", subnetName, resourceGroup) + } + + if !strings.Contains(*resp.RouteTable.ID, routeTableId) { + return fmt.Errorf("Bad: Subnet %q (resource group: %q) does not have route table %q", subnetName, resourceGroup, routeTableId) + } + return nil } } @@ -135,7 +221,8 @@ func testCheckAzureRMSubnetDestroy(s *terraform.State) error { return nil } -var testAccAzureRMSubnet_basic = ` +func testAccAzureRMSubnet_basic(rInt int) string { + return fmt.Sprintf(` resource "azurerm_resource_group" "test" { name = "acctestRG-%d" location = "West US" @@ -171,4 +258,114 @@ resource "azurerm_route" "test" { next_hop_type = "VirtualAppliance" next_hop_in_ip_address = "10.10.1.1" } -` +`, rInt, rInt, rInt, rInt, rInt) +} + +func testAccAzureRMSubnet_routeTable(rInt int) string { + return fmt.Sprintf(` +resource "azurerm_resource_group" "test" { + name = "acctestRG-%d" + location = "West US" +} + +resource "azurerm_virtual_network" "test" { + name = "acctestvirtnet%d" + address_space = ["10.0.0.0/16"] + location = "West US" + resource_group_name = "${azurerm_resource_group.test.name}" +} + +resource "azurerm_subnet" "test" { + name = "acctestsubnet%d" + resource_group_name = "${azurerm_resource_group.test.name}" + virtual_network_name = "${azurerm_virtual_network.test.name}" + address_prefix = "10.0.2.0/24" + route_table_id = "${azurerm_route_table.test.id}" +} + +resource "azurerm_route_table" "test" { + name = "acctest-%d" + location = "West US" + resource_group_name = "${azurerm_resource_group.test.name}" +} + +resource "azurerm_route" "route_a" { + name = "acctest-%d" + resource_group_name = "${azurerm_resource_group.test.name}" + route_table_name = "${azurerm_route_table.test.name}" + + address_prefix = "10.100.0.0/14" + next_hop_type = "VirtualAppliance" + next_hop_in_ip_address = "10.10.1.1" +}`, rInt, rInt, rInt, rInt, rInt) +} + +func testAccAzureRMSubnet_updatedRouteTable(rInt int) string { + return fmt.Sprintf(` +resource "azurerm_resource_group" "test" { + name = "acctestRG-%d" + location = "West US" + tags { + environment = "Testing" + } +} + +resource "azurerm_network_security_group" "test_secgroup" { + name = "acctest-%d" + location = "West US" + resource_group_name = "${azurerm_resource_group.test.name}" + + security_rule { + name = "acctest-%d" + priority = 100 + direction = "Inbound" + access = "Allow" + protocol = "Tcp" + source_port_range = "*" + destination_port_range = "*" + source_address_prefix = "*" + destination_address_prefix = "*" + } + + tags { + environment = "Testing" + } +} + +resource "azurerm_virtual_network" "test" { + name = "acctestvirtnet%d" + address_space = ["10.0.0.0/16"] + location = "West US" + resource_group_name = "${azurerm_resource_group.test.name}" + tags { + environment = "Testing" + } +} + +resource "azurerm_subnet" "test" { + name = "acctestsubnet%d" + resource_group_name = "${azurerm_resource_group.test.name}" + virtual_network_name = "${azurerm_virtual_network.test.name}" + address_prefix = "10.0.2.0/24" + route_table_id = "${azurerm_route_table.test.id}" +} + +resource "azurerm_route_table" "test" { + name = "acctest-%d" + location = "West US" + resource_group_name = "${azurerm_resource_group.test.name}" + tags { + environment = "Testing" + } +} + +resource "azurerm_route" "route_a" { + name = "acctest-%d" + resource_group_name = "${azurerm_resource_group.test.name}" + route_table_name = "${azurerm_route_table.test.name}" + + address_prefix = "10.100.0.0/14" + next_hop_type = "VirtualAppliance" + next_hop_in_ip_address = "10.10.1.1" +}`, rInt, rInt, rInt, rInt, rInt, rInt, rInt) +} diff --git a/builtin/providers/azurerm/resource_arm_virtual_network.go b/builtin/providers/azurerm/resource_arm_virtual_network.go index 2d396f8b5..22118b80c 100644 --- a/builtin/providers/azurerm/resource_arm_virtual_network.go +++ b/builtin/providers/azurerm/resource_arm_virtual_network.go @@ -1,6 +1,7 @@ package azurerm import ( + "bytes" "fmt" "log" "net/http" @@ -89,11 +90,15 @@ func resourceArmVirtualNetworkCreate(d *schema.ResourceData, meta interface{}) e location := d.Get("location").(string) resGroup := d.Get("resource_group_name").(string) tags := d.Get("tags").(map[string]interface{}) + vnetProperties, vnetPropsErr := getVirtualNetworkProperties(d, meta) + if vnetPropsErr != nil { + return vnetPropsErr + } vnet := network.VirtualNetwork{ Name: &name, Location: &location, - VirtualNetworkPropertiesFormat: getVirtualNetworkProperties(d), + VirtualNetworkPropertiesFormat: vnetProperties, Tags: expandTags(tags), } @@ -212,7 +217,7 @@ func resourceArmVirtualNetworkDelete(d *schema.ResourceData, meta interface{}) e return err } -func getVirtualNetworkProperties(d *schema.ResourceData) *network.VirtualNetworkPropertiesFormat { +func getVirtualNetworkProperties(d *schema.ResourceData, meta interface{}) (*network.VirtualNetworkPropertiesFormat, error) { // first; get address space prefixes: prefixes := []string{} for _, prefix := range d.Get("address_space").([]interface{}) { @@ -232,26 +237,41 @@ func getVirtualNetworkProperties(d *schema.ResourceData) *network.VirtualNetwork subnet := subnet.(map[string]interface{}) name := subnet["name"].(string) + log.Printf("[INFO] setting subnets inside vNet, processing %q", name) + //since subnets can also be created outside of vNet definition (as root objects) + // do a GET on subnet properties from the server before setting them + resGroup := d.Get("resource_group_name").(string) + vnetName := d.Get("name").(string) + subnetObj, err := getExistingSubnet(resGroup, vnetName, name, meta) + if err != nil { + return nil, err + } + log.Printf("[INFO] Completed GET of Subnet props ") + prefix := subnet["address_prefix"].(string) secGroup := subnet["security_group"].(string) - var subnetObj network.Subnet + //set the props from config and leave the rest intact subnetObj.Name = &name - subnetObj.SubnetPropertiesFormat = &network.SubnetPropertiesFormat{} + if subnetObj.SubnetPropertiesFormat == nil { + subnetObj.SubnetPropertiesFormat = &network.SubnetPropertiesFormat{} + } + subnetObj.SubnetPropertiesFormat.AddressPrefix = &prefix if secGroup != "" { subnetObj.SubnetPropertiesFormat.NetworkSecurityGroup = &network.SecurityGroup{ ID: &secGroup, } + } else { + subnetObj.SubnetPropertiesFormat.NetworkSecurityGroup = nil } - subnets = append(subnets, subnetObj) + subnets = append(subnets, *subnetObj) } } - // finally; return the struct: - return &network.VirtualNetworkPropertiesFormat{ + properties := &network.VirtualNetworkPropertiesFormat{ AddressSpace: &network.AddressSpace{ AddressPrefixes: &prefixes, }, @@ -260,15 +280,56 @@ func getVirtualNetworkProperties(d *schema.ResourceData) *network.VirtualNetwork }, Subnets: &subnets, } + // finally; return the struct: + return properties, nil } func resourceAzureSubnetHash(v interface{}) int { + var buf bytes.Buffer m := v.(map[string]interface{}) - subnet := m["name"].(string) + m["address_prefix"].(string) - if securityGroup, present := m["security_group"]; present { - subnet = subnet + securityGroup.(string) + buf.WriteString(fmt.Sprintf("%s", m["name"].(string))) + buf.WriteString(fmt.Sprintf("%s", m["address_prefix"].(string))) + if v, ok := m["security_group"]; ok { + buf.WriteString(v.(string)) } - return hashcode.String(subnet) + return hashcode.String(buf.String()) +} + +func getExistingSubnet(resGroup string, vnetName string, subnetName string, meta interface{}) (*network.Subnet, error) { + //attempt to retrieve existing subnet from the server + existingSubnet := network.Subnet{} + subnetClient := meta.(*ArmClient).subnetClient + resp, err := subnetClient.Get(resGroup, vnetName, subnetName, "") + + if err != nil { + if resp.StatusCode == http.StatusNotFound { + return &existingSubnet, nil + } + //raise an error if there was an issue other than 404 in getting subnet properties + return nil, err + } + + existingSubnet.SubnetPropertiesFormat = &network.SubnetPropertiesFormat{} + existingSubnet.SubnetPropertiesFormat.AddressPrefix = resp.SubnetPropertiesFormat.AddressPrefix + + if resp.SubnetPropertiesFormat.NetworkSecurityGroup != nil { + existingSubnet.SubnetPropertiesFormat.NetworkSecurityGroup = resp.SubnetPropertiesFormat.NetworkSecurityGroup + } + + if resp.SubnetPropertiesFormat.RouteTable != nil { + existingSubnet.SubnetPropertiesFormat.RouteTable = resp.SubnetPropertiesFormat.RouteTable + } + + if resp.SubnetPropertiesFormat.IPConfigurations != nil { + ips := make([]string, 0, len(*resp.SubnetPropertiesFormat.IPConfigurations)) + for _, ip := range *resp.SubnetPropertiesFormat.IPConfigurations { + ips = append(ips, *ip.ID) + } + + existingSubnet.SubnetPropertiesFormat.IPConfigurations = resp.SubnetPropertiesFormat.IPConfigurations + } + + return &existingSubnet, nil } func expandAzureRmVirtualNetworkVirtualNetworkSecurityGroupNames(d *schema.ResourceData) ([]string, error) { diff --git a/builtin/providers/azurerm/resource_arm_virtual_network_test.go b/builtin/providers/azurerm/resource_arm_virtual_network_test.go index 5cd347868..856d4fe2e 100644 --- a/builtin/providers/azurerm/resource_arm_virtual_network_test.go +++ b/builtin/providers/azurerm/resource_arm_virtual_network_test.go @@ -209,8 +209,8 @@ resource "azurerm_virtual_network" "test" { } tags { - environment = "Production" - cost_center = "MSFT" + environment = "Production" + cost_center = "MSFT" } } ` @@ -233,7 +233,7 @@ resource "azurerm_virtual_network" "test" { } tags { - environment = "staging" + environment = "staging" } } `