From f075b0214da36bd1cf728f1a295a767169341c56 Mon Sep 17 00:00:00 2001 From: Evan Brown Date: Mon, 16 May 2016 11:57:04 -0700 Subject: [PATCH 1/5] providers/google: Don't fail deleting disks that don't exist. Addresses #5942 --- .../providers/google/resource_compute_disk.go | 6 ++++ .../google/resource_compute_instance_test.go | 33 ++++++++++++++++--- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/builtin/providers/google/resource_compute_disk.go b/builtin/providers/google/resource_compute_disk.go index b307505f8..c6811deb2 100644 --- a/builtin/providers/google/resource_compute_disk.go +++ b/builtin/providers/google/resource_compute_disk.go @@ -184,6 +184,12 @@ func resourceComputeDiskDelete(d *schema.ResourceData, meta interface{}) error { op, err := config.clientCompute.Disks.Delete( project, d.Get("zone").(string), d.Id()).Do() if err != nil { + if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 { + log.Printf("[WARN] Removing Disk %q because it's gone", d.Get("name").(string)) + // The resource doesn't exist anymore + d.SetId("") + return nil + } return fmt.Errorf("Error deleting disk: %s", err) } diff --git a/builtin/providers/google/resource_compute_instance_test.go b/builtin/providers/google/resource_compute_instance_test.go index 8c9610a21..c133b97e7 100644 --- a/builtin/providers/google/resource_compute_instance_test.go +++ b/builtin/providers/google/resource_compute_instance_test.go @@ -126,7 +126,7 @@ func TestAccComputeInstance_IP(t *testing.T) { }) } -func TestAccComputeInstance_disks(t *testing.T) { +func TestAccComputeInstance_disksWithoutAutodelete(t *testing.T) { var instance compute.Instance var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10)) var diskName = fmt.Sprintf("instance-testd-%s", acctest.RandString(10)) @@ -137,7 +137,7 @@ func TestAccComputeInstance_disks(t *testing.T) { CheckDestroy: testAccCheckComputeInstanceDestroy, Steps: []resource.TestStep{ resource.TestStep{ - Config: testAccComputeInstance_disks(diskName, instanceName), + Config: testAccComputeInstance_disks(diskName, instanceName, false), Check: resource.ComposeTestCheckFunc( testAccCheckComputeInstanceExists( "google_compute_instance.foobar", &instance), @@ -149,6 +149,29 @@ func TestAccComputeInstance_disks(t *testing.T) { }) } +func TestAccComputeInstance_disksWithAutodelete(t *testing.T) { + var instance compute.Instance + var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10)) + var diskName = fmt.Sprintf("instance-testd-%s", acctest.RandString(10)) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeInstanceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccComputeInstance_disks(diskName, instanceName, true), + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeInstanceExists( + "google_compute_instance.foobar", &instance), + testAccCheckComputeInstanceDisk(&instance, instanceName, true, true), + testAccCheckComputeInstanceDisk(&instance, diskName, true, false), + ), + }, + }, + }) +} + func TestAccComputeInstance_local_ssd(t *testing.T) { var instance compute.Instance var instanceName = fmt.Sprintf("instance-test-%s", acctest.RandString(10)) @@ -702,7 +725,7 @@ func testAccComputeInstance_ip(ip, instance string) string { }`, ip, instance) } -func testAccComputeInstance_disks(disk, instance string) string { +func testAccComputeInstance_disks(disk, instance string, autodelete bool) string { return fmt.Sprintf(` resource "google_compute_disk" "foobar" { name = "%s" @@ -722,7 +745,7 @@ func testAccComputeInstance_disks(disk, instance string) string { disk { disk = "${google_compute_disk.foobar.name}" - auto_delete = false + auto_delete = %v } network_interface { @@ -732,7 +755,7 @@ func testAccComputeInstance_disks(disk, instance string) string { metadata { foo = "bar" } - }`, disk, instance) + }`, disk, instance, autodelete) } func testAccComputeInstance_local_ssd(instance string) string { From a869d2f8a995f71f16cbb391081f79b2048ce2d9 Mon Sep 17 00:00:00 2001 From: stack72 Date: Fri, 20 May 2016 09:34:40 +0100 Subject: [PATCH 2/5] provider/azurerm: `azurerm_network_interface` diffs didn't match during apply The IP COnfiguration block of `azurerm_network_interface` didn't have a hash created in a way that changes to the optional params were being picked up: ``` ~ azurerm_network_interface.test ip_configuration.273485505.name: "testconfiguration1" => "" ip_configuration.273485505.private_ip_address_allocation: "dynamic" => "" ip_configuration.273485505.subnet_id: "/subscriptions/34ca515c-4629-458e-bf7c-738d77e0d0ea/resourceGroups/acctestrg/providers/Microsoft.Network/virtualNetworks/acctvn/subnets/acctsub" => "" ip_configuration.~273485505.load_balancer_backend_address_pools_ids.#: "" => "" ip_configuration.~273485505.load_balancer_inbound_nat_rules_ids.#: "" => "" ip_configuration.~273485505.name: "" => "testconfiguration1" ip_configuration.~273485505.private_ip_address: "" => "" ip_configuration.~273485505.private_ip_address_allocation: "" => "dynamic" ip_configuration.~273485505.public_ip_address_id: "" => "${azurerm_public_ip.test.id}" ip_configuration.~273485505.subnet_id: "" => "/subscriptions/34ca515c-4629-458e-bf7c-738d77e0d0ea/resourceGroups/acctestrg/providers/Microsoft.Network/virtualNetworks/acctvn/subnets/acctsub" ``` This caused the following error: ``` Error applying plan: 1 error(s) occurred: * azurerm_network_interface.test: diffs didn't match during apply. This is a bug with Terraform and should be reported as a GitHub Issue. Please include the following information in your report: ``` Notice that the hash didn't change. This change adds the remaining optional params to the hash so that the hash id will change. ``` ~ azurerm_network_interface.test ip_configuration.4255411321.load_balancer_backend_address_pools_ids.#: "" => "" ip_configuration.4255411321.load_balancer_inbound_nat_rules_ids.#: "" => "" ip_configuration.4255411321.name: "" => "testconfiguration1" ip_configuration.4255411321.private_ip_address: "" => "" ip_configuration.4255411321.private_ip_address_allocation: "" => "dynamic" ip_configuration.4255411321.public_ip_address_id: "" => "/subscriptions/34ca515c-4629-458e-bf7c-738d77e0d0ea/resourceGroups/acctestrg/providers/Microsoft.Network/publicIPAddresses/public-ip" ip_configuration.4255411321.subnet_id: "" => "/subscriptions/34ca515c-4629-458e-bf7c-738d77e0d0ea/resourceGroups/acctestrg/providers/Microsoft.Network/virtualNetworks/acctvn/subnets/acctsub" ip_configuration.966273186.name: "testconfiguration1" => "" ip_configuration.966273186.private_ip_address_allocation: "dynamic" => "" ip_configuration.966273186.subnet_id: "/subscriptions/34ca515c-4629-458e-bf7c-738d77e0d0ea/resourceGroups/acctestrg/providers/Microsoft.Network/virtualNetworks/acctvn/subnets/acctsub" => "" ``` This allows the Update to work as expected :) ``` azurerm_network_interface.test: Modifications complete Apply complete! Resources: 0 added, 1 changed, 0 destroyed. ``` --- .../azurerm/resource_arm_network_interface_card.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/builtin/providers/azurerm/resource_arm_network_interface_card.go b/builtin/providers/azurerm/resource_arm_network_interface_card.go index 2bef8ca04..e279e411a 100644 --- a/builtin/providers/azurerm/resource_arm_network_interface_card.go +++ b/builtin/providers/azurerm/resource_arm_network_interface_card.go @@ -328,7 +328,13 @@ func resourceArmNetworkInterfaceIpConfigurationHash(v interface{}) int { m := v.(map[string]interface{}) buf.WriteString(fmt.Sprintf("%s-", m["name"].(string))) buf.WriteString(fmt.Sprintf("%s-", m["subnet_id"].(string))) + if m["private_ip_address"] != nil { + buf.WriteString(fmt.Sprintf("%s-", m["private_ip_address"].(string))) + } buf.WriteString(fmt.Sprintf("%s-", m["private_ip_address_allocation"].(string))) + if m["public_ip_address_id"] != nil { + buf.WriteString(fmt.Sprintf("%s-", m["public_ip_address_id"].(string))) + } return hashcode.String(buf.String()) } From cd0c4522eed540baf40e13e7c7fdb1b2195ecb2a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 17 May 2016 07:58:11 -0700 Subject: [PATCH 3/5] core: honor "destroy" diffs for data resources Previously the "planDestroy" pass would correctly produce a destroy diff, but the "apply" pass would just ignore it and make a fresh diff, turning it back into a "create" because data resources are always eager to refresh. Now we consider the previous diff when re-diffing during apply and so we can preserve the plan to destroy and then ultimately actually "destroy" the data resource (remove from the state) when we get to ReadDataApply. This ensures that the state is left empty after "terraform destroy"; previously we would leave behind data resource states. --- terraform/eval_read_data.go | 47 +++++++++++++++++++++------------ terraform/transform_resource.go | 1 + 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 9625bcffa..61fa3e125 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -12,12 +12,14 @@ type EvalReadDataDiff struct { OutputState **InstanceState Config **ResourceConfig Info *InstanceInfo + + // Set Previous when re-evaluating diff during apply, to ensure that + // the "Destroy" flag is preserved. + Previous **InstanceDiff } func (n *EvalReadDataDiff) Eval(ctx EvalContext) (interface{}, error) { // TODO: test - provider := *n.Provider - config := *n.Config err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PreDiff(n.Info, nil) @@ -26,21 +28,32 @@ func (n *EvalReadDataDiff) Eval(ctx EvalContext) (interface{}, error) { return nil, err } - diff, err := provider.ReadDataDiff(n.Info, config) - if err != nil { - return nil, err - } - if diff == nil { - diff = new(InstanceDiff) - } + var diff *InstanceDiff - // id is always computed, because we're always "creating a new resource" - diff.init() - diff.Attributes["id"] = &ResourceAttrDiff{ - Old: "", - NewComputed: true, - RequiresNew: true, - Type: DiffAttrOutput, + if n.Previous != nil && *n.Previous != nil && (*n.Previous).Destroy { + // If we're re-diffing for a diff that was already planning to + // destroy, then we'll just continue with that plan. + diff = &InstanceDiff{Destroy: true} + } else { + provider := *n.Provider + config := *n.Config + + diff, err := provider.ReadDataDiff(n.Info, config) + if err != nil { + return nil, err + } + if diff == nil { + diff = new(InstanceDiff) + } + + // id is always computed, because we're always "creating a new resource" + diff.init() + diff.Attributes["id"] = &ResourceAttrDiff{ + Old: "", + NewComputed: true, + RequiresNew: true, + Type: DiffAttrOutput, + } } err = ctx.Hook(func(h Hook) (HookAction, error) { @@ -83,7 +96,7 @@ func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { // If the diff is for *destroying* this resource then we'll // just drop its state and move on, since data resources don't // support an actual "destroy" action. - if diff.Destroy { + if diff != nil && diff.Destroy { if n.Output != nil { *n.Output = nil } diff --git a/terraform/transform_resource.go b/terraform/transform_resource.go index 14cd54f54..dccc3cd94 100644 --- a/terraform/transform_resource.go +++ b/terraform/transform_resource.go @@ -810,6 +810,7 @@ func (n *graphNodeExpandedResource) dataResourceEvalNodes(resource *Resource, in &EvalReadDataDiff{ Info: info, Config: &config, + Previous: &diff, Provider: &provider, Output: &diff, }, From ab1551e863a49d3fecca4a8e8182cee705cf5c74 Mon Sep 17 00:00:00 2001 From: James Nugent Date: Fri, 20 May 2016 16:57:31 -0500 Subject: [PATCH 4/5] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77ccd336f..6f6876010 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ BUG FIXES: * provider/aws: fix Elastic Beanstalk `cname_prefix` continual plans [GH-6653] * provider/aws: Make 'stage_name' required in api_gateway_deployment [Gh-6797] * provider/azurerm: Fixes terraform crash when using SSH keys with `azurerm_virtual_machine` [GH-6766] + * provider/azurerm: Fix a bug causing 'diffs do not match' on `azurerm_network_interface` resources [GH-6790] * provider/azurerm: Normalizes `availability_set_id` casing to avoid spurious diffs in `azurerm_virtual_machine` [GH-6768] * provider/openstack: Reassociate Floating IP on network changes [GH-6579] * provider/vsphere: `gateway` and `ipv6_gateway` are now read from `vsphere_virtual_machine` resources [GH-6522] From 7945a6d7cf322663b2ae8e91e08cd1fc8e839ae3 Mon Sep 17 00:00:00 2001 From: James Nugent Date: Fri, 20 May 2016 17:00:17 -0500 Subject: [PATCH 5/5] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f6876010..1b7562322 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ BUG FIXES: * provider/azurerm: Fixes terraform crash when using SSH keys with `azurerm_virtual_machine` [GH-6766] * provider/azurerm: Fix a bug causing 'diffs do not match' on `azurerm_network_interface` resources [GH-6790] * provider/azurerm: Normalizes `availability_set_id` casing to avoid spurious diffs in `azurerm_virtual_machine` [GH-6768] + * provider/google: Fix a bug causing an error attempting to delete an already-deleted `google_compute_disk` [GH-6689] * provider/openstack: Reassociate Floating IP on network changes [GH-6579] * provider/vsphere: `gateway` and `ipv6_gateway` are now read from `vsphere_virtual_machine` resources [GH-6522] * provider/vsphere: `ipv*_gateway` parameters won't force a new `vsphere_virtual_machine` [GH-6635]