Skip to content

Commit

Permalink
provider/azurerm: Fix VHD deletion when VM and Storage account are in…
Browse files Browse the repository at this point in the history 
… separate resource groups (hashicorp#9631)

* Improve messaging when storage account isn't found.

* Add client for finding resources when you don't know it's resource group.

* Add function to find Storage Account resource group name.

* Use the storage account resource group, not the virtual machine's resource group when deleting VHDs.

* Add description of storage account ID for clarity.

* Improve VHD deletion test when storage account is in different resource group.

* Use common function for ID parsing of storage account.
  • Loading branch information
@carinadigital @antonbabenko
carinadigital authored and antonbabenko committed Oct 27, 2016
1 parent 8d55d8c commit d624944
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 10 deletions.
7 changes: 7 additions & 0 deletions builtin/providers/azurerm/config.go
View file
Expand Up @@ -64,6 +64,7 @@ type ArmClient struct {
providers resources.ProvidersClient
resourceGroupClient resources.GroupsClient
tagsClient resources.TagsClient
resourceFindClient resources.Client

jobsClient scheduler.JobsClient
jobsCollectionsClient scheduler.JobCollectionsClient
Expand Down Expand Up @@ -314,6 +315,12 @@ func (c *Config) getArmClient() (*ArmClient, error) {
tc.Sender = autorest.CreateSender(withRequestLogging())
client.tagsClient = tc

rf := resources.NewClient(c.SubscriptionID)
setUserAgent(&rf.Client)
rf.Authorizer = spt
rf.Sender = autorest.CreateSender(withRequestLogging())
client.resourceFindClient = rf

jc := scheduler.NewJobsClient(c.SubscriptionID)
setUserAgent(&jc.Client)
jc.Authorizer = spt
Expand Down
39 changes: 34 additions & 5 deletions builtin/providers/azurerm/resource_arm_virtual_machine.go
View file
Expand Up @@ -649,7 +649,7 @@ func resourceArmVirtualMachineDelete(d *schema.ResourceData, meta interface{}) e
return fmt.Errorf("Error expanding OS Disk: %s", err)
}

if err = resourceArmVirtualMachineDeleteVhd(*osDisk.Vhd.URI, resGroup, meta); err != nil {
if err = resourceArmVirtualMachineDeleteVhd(*osDisk.Vhd.URI, meta); err != nil {
return fmt.Errorf("Error deleting OS Disk VHD: %s", err)
}
}
Expand All @@ -664,7 +664,7 @@ func resourceArmVirtualMachineDelete(d *schema.ResourceData, meta interface{}) e
}

for _, disk := range disks {
if err = resourceArmVirtualMachineDeleteVhd(*disk.Vhd.URI, resGroup, meta); err != nil {
if err = resourceArmVirtualMachineDeleteVhd(*disk.Vhd.URI, meta); err != nil {
return fmt.Errorf("Error deleting Data Disk VHD: %s", err)
}
}
Expand All @@ -673,7 +673,7 @@ func resourceArmVirtualMachineDelete(d *schema.ResourceData, meta interface{}) e
return nil
}

func resourceArmVirtualMachineDeleteVhd(uri, resGroup string, meta interface{}) error {
func resourceArmVirtualMachineDeleteVhd(uri string, meta interface{}) error {
vhdURL, err := url.Parse(uri)
if err != nil {
return fmt.Errorf("Cannot parse Disk VHD URI: %s", err)
Expand All @@ -685,13 +685,18 @@ func resourceArmVirtualMachineDeleteVhd(uri, resGroup string, meta interface{})
containerName := path[0]
blobName := path[1]

blobClient, saExists, err := meta.(*ArmClient).getBlobStorageClientForStorageAccount(resGroup, storageAccountName)
storageAccountResourceGroupName, err := findStorageAccountResourceGroup(meta, storageAccountName)
if err != nil {
return fmt.Errorf("Error finding resource group for storage account %s: %s", storageAccountName, err)
}

blobClient, saExists, err := meta.(*ArmClient).getBlobStorageClientForStorageAccount(storageAccountResourceGroupName, storageAccountName)
if err != nil {
return fmt.Errorf("Error creating blob store client for VHD deletion: %s", err)
}

if !saExists {
log.Printf("[INFO] Storage Account %q doesn't exist so the VHD blob won't exist", storageAccountName)
log.Printf("[INFO] Storage Account %q in resource group %q doesn't exist so the VHD blob won't exist", storageAccountName, storageAccountResourceGroupName)
return nil
}

Expand Down Expand Up @@ -1276,3 +1281,27 @@ func expandAzureRmVirtualMachineOsDisk(d *schema.ResourceData) (*compute.OSDisk,

return osDisk, nil
}

func findStorageAccountResourceGroup(meta interface{}, storageAccountName string) (string, error) {
client := meta.(*ArmClient).resourceFindClient
filter := fmt.Sprintf("name eq '%s' and resourceType eq 'Microsoft.Storage/storageAccounts'", storageAccountName)
expand := ""
var pager *int32

rf, err := client.List(filter, expand, pager)
if err != nil {
return "", fmt.Errorf("Error making resource request for query %s: %s", filter, err)
}

results := *rf.Value
if len(results) != 1 {
return "", fmt.Errorf("Wrong number of results making resource request for query %s: %s", filter, len(results))
}

id, err := parseAzureResourceID(*results[0].ID)
if err != nil {
return "", err
}

return id.ResourceGroup, nil
}
71 changes: 66 additions & 5 deletions builtin/providers/azurerm/resource_arm_virtual_machine_test.go
View file
Expand Up @@ -248,8 +248,8 @@ func TestAccAzureRMVirtualMachine_deleteVHDOptOut(t *testing.T) {
func TestAccAzureRMVirtualMachine_deleteVHDOptIn(t *testing.T) {
var vm compute.VirtualMachine
ri := acctest.RandInt()
preConfig := fmt.Sprintf(testAccAzureRMVirtualMachine_basicLinuxMachineDestroyDisks, ri, ri, ri, ri, ri, ri, ri)
postConfig := fmt.Sprintf(testAccAzureRMVirtualMachine_basicLinuxMachineDeleteVM, ri, ri, ri, ri, ri)
preConfig := fmt.Sprintf(testAccAzureRMVirtualMachine_basicLinuxMachineDestroyDisksBefore, ri, ri, ri, ri, ri, ri, ri, ri)
postConfig := fmt.Sprintf(testAccAzureRMVirtualMachine_basicLinuxMachineDestroyDisksAfter, ri, ri, ri, ri, ri, ri)
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Expand Down Expand Up @@ -633,12 +633,17 @@ resource "azurerm_virtual_machine" "test" {
}
`

var testAccAzureRMVirtualMachine_basicLinuxMachineDestroyDisks = `
var testAccAzureRMVirtualMachine_basicLinuxMachineDestroyDisksBefore = `
resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "West US"
}
resource "azurerm_resource_group" "test-sa" {
name = "acctestRG-sa-%d"
location = "West US"
}
resource "azurerm_virtual_network" "test" {
name = "acctvn-%d"
address_space = ["10.0.0.0/16"]
Expand Down Expand Up @@ -667,7 +672,7 @@ resource "azurerm_network_interface" "test" {
resource "azurerm_storage_account" "test" {
name = "accsa%d"
resource_group_name = "${azurerm_resource_group.test.name}"
resource_group_name = "${azurerm_resource_group.test-sa.name}"
location = "westus"
account_type = "Standard_LRS"
Expand All @@ -678,7 +683,7 @@ resource "azurerm_storage_account" "test" {
resource "azurerm_storage_container" "test" {
name = "vhds"
resource_group_name = "${azurerm_resource_group.test.name}"
resource_group_name = "${azurerm_resource_group.test-sa.name}"
storage_account_name = "${azurerm_storage_account.test.name}"
container_access_type = "private"
}
Expand Down Expand Up @@ -733,6 +738,62 @@ resource "azurerm_virtual_machine" "test" {
}
`

var testAccAzureRMVirtualMachine_basicLinuxMachineDestroyDisksAfter = `
resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "West US"
}
resource "azurerm_resource_group" "test-sa" {
name = "acctestRG-sa-%d"
location = "West US"
}
resource "azurerm_virtual_network" "test" {
name = "acctvn-%d"
address_space = ["10.0.0.0/16"]
location = "West US"
resource_group_name = "${azurerm_resource_group.test.name}"
}
resource "azurerm_subnet" "test" {
name = "acctsub-%d"
resource_group_name = "${azurerm_resource_group.test.name}"
virtual_network_name = "${azurerm_virtual_network.test.name}"
address_prefix = "10.0.2.0/24"
}
resource "azurerm_network_interface" "test" {
name = "acctni-%d"
location = "West US"
resource_group_name = "${azurerm_resource_group.test.name}"
ip_configuration {
name = "testconfiguration1"
subnet_id = "${azurerm_subnet.test.id}"
private_ip_address_allocation = "dynamic"
}
}
resource "azurerm_storage_account" "test" {
name = "accsa%d"
resource_group_name = "${azurerm_resource_group.test-sa.name}"
location = "westus"
account_type = "Standard_LRS"
tags {
environment = "staging"
}
}
resource "azurerm_storage_container" "test" {
name = "vhds"
resource_group_name = "${azurerm_resource_group.test-sa.name}"
storage_account_name = "${azurerm_storage_account.test.name}"
container_access_type = "private"
}
`

var testAccAzureRMVirtualMachine_basicLinuxMachineDeleteVM = `
resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
Expand Down

0 comments on commit d624944

Please sign in to comment.