Replace VM resource's wait_for_ip functionality to validate it whether the ip(s) match a given prefix (#305)

* Remove wait_for_ip top level argument in favor of network block scoped expected_ip_cidr

Signed-off-by: Dom Del Nano <[email protected]>

* Implement logic for matching against multiple network interface cidr ranges

Signed-off-by: Dom Del Nano <[email protected]>

* Implement cidr matching for multiple network interfaces and start test
for verifying the lack of a match

* Implement wait for ip test for the case where a network does not converge to the cidr match

Signed-off-by: Dom Del Nano <[email protected]>

* Changes to fix V0 state migration test

Signed-off-by: Dom Del Nano <[email protected]>

* Implement test that verifies migrating from wait_for_ip to expected_cidr_range works as expected

Signed-off-by: Dom Del Nano <[email protected]>

* Fix issue with failing test and update docs

Signed-off-by: Dom Del Nano <[email protected]>

---------

Signed-off-by: Dom Del Nano <[email protected]>

Author: ddelnano

client/vm.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"log"
+	"net"
 	"net/http"
 	"os"
 	"strconv"
@@ -135,15 +136,17 @@ type Vm struct {
 	// These fields are used for passing in disk inputs when
 	// creating Vms, however, this is not a real field as far
 	// as the XO api or XAPI is concerned
-	Disks                          []Disk              `json:"-"`
-	CloudNetworkConfig             string              `json:"-"`
-	VIFsMap                        []map[string]string `json:"-"`
-	WaitForIps                     bool                `json:"-"`
-	Installation                   Installation        `json:"-"`
-	ManagementAgentDetected        bool                `json:"managementAgentDetected"`
-	PVDriversDetected              bool                `json:"pvDriversDetected"`
-	DestroyCloudConfigVdiAfterBoot bool                `json:"-"`
-	CloneType                      string              `json:"-"`
+	Disks              []Disk              `json:"-"`
+	CloudNetworkConfig string              `json:"-"`
+	VIFsMap            []map[string]string `json:"-"`
+	// Map where the key is the network interface index and the
+	// value is a cidr range parsable by net.ParseCIDR
+	WaitForIps                     map[string]string `json:"-"`
+	Installation                   Installation      `json:"-"`
+	ManagementAgentDetected        bool              `json:"managementAgentDetected"`
+	PVDriversDetected              bool              `json:"pvDriversDetected"`
+	DestroyCloudConfigVdiAfterBoot bool              `json:"-"`
+	CloneType                      string            `json:"-"`
 }
 
 type Installation struct {
@@ -603,58 +606,117 @@ func (c *Client) waitForVmState(id string, stateConf StateChangeConf) error {
 	return err
 }
 
-func (c *Client) waitForModifyVm(id string, desiredPowerState string, waitForIp bool, timeout time.Duration) error {
-	if !waitForIp {
-		var pending []string
-		target := desiredPowerState
-		switch desiredPowerState {
-		case RunningPowerState:
-			pending = []string{HaltedPowerState}
-		case HaltedPowerState:
-			pending = []string{RunningPowerState}
-		default:
-			return errors.New(fmt.Sprintf("Invalid VM power state requested: %s\n", desiredPowerState))
+func waitForPowerStateReached(c *Client, vmId, desiredPowerState string, timeout time.Duration) error {
+	var pending []string
+	target := desiredPowerState
+	switch desiredPowerState {
+	case RunningPowerState:
+		pending = []string{HaltedPowerState}
+	case HaltedPowerState:
+		pending = []string{RunningPowerState}
+	default:
+		return errors.New(fmt.Sprintf("Invalid VM power state requested: %s\n", desiredPowerState))
+	}
+	refreshFn := func() (result interface{}, state string, err error) {
+		vm, err := c.GetVm(Vm{Id: vmId})
+
+		if err != nil {
+			return vm, "", err
 		}
-		refreshFn := func() (result interface{}, state string, err error) {
-			vm, err := c.GetVm(Vm{Id: id})
 
-			if err != nil {
-				return vm, "", err
-			}
+		return vm, vm.PowerState, nil
+	}
+	stateConf := &StateChangeConf{
+		Pending: pending,
+		Refresh: refreshFn,
+		Target:  []string{target},
+		Timeout: timeout,
+	}
+	_, err := stateConf.WaitForState()
+	return err
+}
+
+type ifaceMatchCheck struct {
+	cidrRange  string
+	ifaceIdx   string
+	ifaceAddrs []string
+}
 
-			return vm, vm.PowerState, nil
+func waitForIPAssignment(c *Client, vmId string, waitForIps map[string]string, timeout time.Duration) error {
+	var lastResult ifaceMatchCheck
+	refreshFn := func() (result interface{}, state string, err error) {
+		vm, err := c.GetVm(Vm{Id: vmId})
+
+		if err != nil {
+			return vm, "", err
 		}
-		stateConf := &StateChangeConf{
-			Pending: pending,
-			Refresh: refreshFn,
-			Target:  []string{target},
-			Timeout: timeout,
+
+		addrs := vm.Addresses
+		if len(addrs) == 0 || vm.PowerState != RunningPowerState {
+			return addrs, "Waiting", nil
 		}
-		_, err := stateConf.WaitForState()
-		return err
-	} else {
-		refreshFn := func() (result interface{}, state string, err error) {
-			vm, err := c.GetVm(Vm{Id: id})
 
-			if err != nil {
-				return vm, "", err
+		netIfaces := map[string][]string{}
+		for key, addr := range vm.Addresses {
+
+			// key has the following format "{iface_id}/(ipv4|ipv6)/{iface_ip_id}"
+			ifaceIdx, _, _ := strings.Cut(key, "/")
+			if _, ok := netIfaces[ifaceIdx]; !ok {
+				netIfaces[ifaceIdx] = []string{}
 			}
+			netIfaces[ifaceIdx] = append(netIfaces[ifaceIdx], addr)
+		}
 
-			l := len(vm.Addresses)
-			if l == 0 || vm.PowerState != RunningPowerState {
-				return vm, "Waiting", nil
+		for ifaceIdx, cidrRange := range waitForIps {
+			// VM's Addresses member does not contain this network interface yet
+			if _, ok := netIfaces[ifaceIdx]; !ok {
+				return addrs, "Waiting", nil
 			}
 
-			return vm, "Ready", nil
-		}
-		stateConf := &StateChangeConf{
-			Pending: []string{"Waiting"},
-			Refresh: refreshFn,
-			Target:  []string{"Ready"},
-			Timeout: timeout,
+			found := false
+			for _, ipAddr := range netIfaces[ifaceIdx] {
+				_, ipNet, err := net.ParseCIDR(cidrRange)
+
+				if err != nil {
+					return addrs, "Waiting", err
+				}
+
+				if ipNet.Contains(net.ParseIP(ipAddr)) {
+					found = true
+				}
+			}
+
+			if !found {
+				lastResult = ifaceMatchCheck{
+					cidrRange:  cidrRange,
+					ifaceIdx:   ifaceIdx,
+					ifaceAddrs: netIfaces[ifaceIdx],
+				}
+
+				return addrs, "Waiting", nil
+			}
 		}
-		_, err := stateConf.WaitForState()
-		return err
+
+		return addrs, "Ready", nil
+	}
+	stateConf := &StateChangeConf{
+		Pending: []string{"Waiting"},
+		Refresh: refreshFn,
+		Target:  []string{"Ready"},
+		Timeout: timeout,
+	}
+	_, err := stateConf.WaitForState()
+	if _, ok := err.(*TimeoutError); ok {
+		return errors.New(fmt.Sprintf("network[%s] never converged to the following cidr: %s, addresses: %s failed to match", lastResult.ifaceIdx, lastResult.cidrRange, lastResult.ifaceAddrs))
+	}
+	return err
+}
+
+func (c *Client) waitForModifyVm(id string, desiredPowerState string, waitForIps map[string]string, timeout time.Duration) error {
+	if len(waitForIps) == 0 {
+		return waitForPowerStateReached(c, id, desiredPowerState, timeout)
+	} else {
+		return waitForIPAssignment(c, id, waitForIps, timeout)
 	}
 }
 

docs/data-sources/vms.md

@@ -84,7 +84,6 @@ Read-Only:
 - `template` (String)
 - `vga` (String)
 - `videoram` (Number)
-- `wait_for_ip` (Boolean)
 - `xenstore` (Map of String)
 
 <a id="nestedobjatt--vms--disk"></a>
@@ -109,6 +108,7 @@ Read-Only:
 
 - `attached` (Boolean)
 - `device` (String)
+- `expected_ip_cidr` (String)
 - `ipv4_addresses` (List of String)
 - `ipv6_addresses` (List of String)
 - `mac_address` (String)

docs/resources/vm.md

@@ -98,13 +98,14 @@ resource "xenorchestra_vm" "bar" {
     }
 }
 
-# vm resource that uses wait_for_ip
+# vm resource that waits until its first network interface
+# is assigned an IP via DHCP
 resource "xenorchestra_vm" "vm" {
   ...
-  wait_for_ip = true
   # Specify VM with two network interfaces
   network {
     ...
+    expected_ip_cidr = "10.0.0.0/16"
   }
   network {
     ...
@@ -176,14 +177,13 @@ $ xo-cli xo.getAllObjects filter='json:{"id": "cf7b5d7d-3cd5-6b7c-5025-5c935c8cd
 - `timeouts` (Block, Optional) (see [below for nested schema](#nestedblock--timeouts))
 - `vga` (String) The video adapter the VM should use. Possible values include std and cirrus.
 - `videoram` (Number) The videoram option the VM should use. Possible values include 1, 2, 4, 8, 16
-- `wait_for_ip` (Boolean) Whether terraform should wait until IP addresses are present on the VM's network interfaces before considering it created. This only works if guest-tools are installed in the VM. Defaults to false.
 - `xenstore` (Map of String) The key value pairs to be populated in xenstore.
 
 ### Read-Only
 
 - `id` (String) The ID of this resource.
-- `ipv4_addresses` (List of String) This is only accessible if guest-tools is installed in the VM and if `wait_for_ip` is set to true. This will contain a list of the ipv4 addresses across all network interfaces in order. See the example terraform code for more details.
-- `ipv6_addresses` (List of String) This is only accessible if guest-tools is installed in the VM and if `wait_for_ip` is set to true. This will contain a list of the ipv6 addresses across all network interfaces in order.
+- `ipv4_addresses` (List of String) This is only accessible if guest-tools is installed in the VM and if `expected_ip_cidr` is set on any network interfaces. This will contain a list of the ipv4 addresses across all network interfaces in order. See the example terraform code for more details.
+- `ipv6_addresses` (List of String) This is only accessible if guest-tools is installed in the VM and if `expected_ip_cidr` is set on any network interfaces. This will contain a list of the ipv6 addresses across all network interfaces in order.
 
 <a id="nestedblock--disk"></a>
 ### Nested Schema for `disk`
@@ -216,6 +216,7 @@ Required:
 Optional:
 
 - `attached` (Boolean) Whether the device should be attached to the VM.
+- `expected_ip_cidr` (String) Determines the IP cidr range terraform should watch for on this network interface. Resource creation is not complete until the IP address converges to the specified range. This only works if guest-tools are installed in the VM. Defaults to "", which skips IP address matching.
 - `mac_address` (String) The mac address of the network interface. This must be parsable by go's [net.ParseMAC function](https://golang.org/pkg/net/#ParseMAC). All mac addresses are stored in Terraform's state with [HardwareAddr's string representation](https://golang.org/pkg/net/#HardwareAddr.String) i.e. 00:00:5e:00:53:01
 
 Read-Only:

examples/resources/xenorchestra_vm/resource.tf

@@ -67,13 +67,14 @@ resource "xenorchestra_vm" "bar" {
     }
 }
 
-# vm resource that uses wait_for_ip
+# vm resource that waits until its first network interface
+# is assigned an IP via DHCP
 resource "xenorchestra_vm" "vm" {
   ...
-  wait_for_ip = true
   # Specify VM with two network interfaces
   network {
     ...
+    expected_ip_cidr = "10.0.0.0/16"
   }
   network {
     ...

xoa/data_source_vms.go

@@ -84,7 +84,6 @@ func vmToMapList(vms []client.Vm) []map[string]interface{} {
 			"memory_max":           vm.Memory.Static[1],
 			"affinity_host":        vm.AffinityHost,
 			"template":             vm.Template,
-			"wait_for_ip":          vm.WaitForIps,
 			"high_availability":    vm.HA,
 			"ipv4_addresses":       ipv4,
 			"ipv6_addresses":       ipv6,

xoa/internal/state/migrate.go

@@ -6,9 +6,9 @@ import (
 	"log"
 	"net"
 
-	"github.com/vatesfr/terraform-provider-xenorchestra/client"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
+	"github.com/vatesfr/terraform-provider-xenorchestra/client"
 )
 
 var validVga = []string{
@@ -362,3 +362,81 @@ func suppressAttachedDiffWhenHalted(k, old, new string, d *schema.ResourceData)
 	log.Printf("[DEBUG] VM '%s' attribute has transitioned from '%s' to '%s' when PowerState '%s'. Suppress diff: %t", k, old, new, powerState, suppress)
 	return
 }
+
+// Used for tests that require the schema of the provider <= v0.28.1
+func V1TestAccVmConfigWithWaitForIp(testPrefix, vmName, templateName, netName, poolId, srId string) string {
+	return fmt.Sprintf(`
+resource "xenorchestra_cloud_config" "bar" {
+    name = "%s-vm-template-%s"
+    template = "template" # should this be templated?
+}
+
+data "xenorchestra_template" "template" {
+    name_label = "%s"
+    pool_id = "%s"
+}
+
+data "xenorchestra_network" "network" {
+    name_label = "%s"
+    pool_id = "%s"
+}
+resource "xenorchestra_vm" "bar" {
+    memory_max = 4295000000
+    cpus  = 1
+    cloud_config = xenorchestra_cloud_config.bar.template
+    name_label = "%s"
+    name_description = "description"
+    template = data.xenorchestra_template.template.id
+    network {
+	network_id = data.xenorchestra_network.network.id
+    }
+    disk {
+      sr_id = "%s"
+      name_label = "disk 1"
+      size = 10001317888
+    }
+    wait_for_ip = true
+}
+`, testPrefix, vmName, templateName, poolId, netName, poolId, vmName, srId)
+}
+
+// terraform configuration that can be used to block changes that should not destroy a VM.
+// While this doesn't integrate nicely with the sdk's test helpers (failure is vague), there
+// are some cases were options are limited (testing pinned provider versions).
+func TestAccV1VmConfigWithDeletionBlocked(testPrefix, vmName, templateName, netName, poolId, srId, waitForIp string) string {
+	return fmt.Sprintf(`
+resource "xenorchestra_cloud_config" "bar" {
+    name = "%s-vm-template-%s"
+    template = "template" # should this be templated?
+}
+
+data "xenorchestra_template" "template" {
+    name_label = "%s"
+    pool_id = "%s"
+}
+data "xenorchestra_network" "network" {
+    name_label = "%s"
+    pool_id = "%s"
+}
+
+resource "xenorchestra_vm" "bar" {
+    memory_max = 4295000000
+    cpus  = 1
+    cloud_config = xenorchestra_cloud_config.bar.template
+    name_label = "%s"
+    name_description = "description"
+    template = data.xenorchestra_template.template.id
+    network {
+	network_id = data.xenorchestra_network.network.id
+    }
+
+    disk {
+      sr_id = "%s"
+      name_label = "disk 1"
+      size = 10001317888
+    }
+    wait_for_ip  = %s
+    blocked_operations = ["destroy"]
+}
+	`, testPrefix, vmName, templateName, poolId, netName, poolId, vmName, srId, waitForIp)
+}