Update and Refresh for create_vnic_details (#301)

* Update and Refresh for create_vnic_details

Schema TypeMap as is currently used for create_vnic_details does not support properties such
as Computed and ForceNew, which prevented using this with update and refresh. TypeList
does appear to handle these correctly, and switching does not require any changes from users.

Tests run:
make test run=ResourceCoreVnicAttachmentTestSuite debug=1
make test run=ResourceCoreInstanceTestSuite debug=1

* respond to comments

Author: briangustafson

docs/examples/compute/multi_vnic/multi_vnic.tf

@@ -7,7 +7,7 @@ variable "region" {}
 variable "ssh_public_key" {}
 
 variable "SecondaryVnicCount" {
-    default = 2
+    default = 1
 }
 
 # Choose an Availability Domain

helpers_core.go

@@ -3,13 +3,50 @@
 package main
 
 import (
-	"strconv"
-
+	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/oracle/bmcs-go-sdk"
 )
 
-func SetCreateVnicOptions(rawCreateVnicDetails interface{}) (vnicOpts *baremetal.CreateVnicOptions, err error) {
-	vnic := rawCreateVnicDetails.(map[string]interface{})
+var createVnicDetailsSchema = &schema.Resource{
+	Schema: map[string]*schema.Schema{
+		"assign_public_ip": {
+			Type:     schema.TypeBool,
+			Optional: true,
+			Default:  true,
+			ForceNew: true,
+		},
+		"display_name": {
+			Type:     schema.TypeString,
+			Optional: true,
+			Computed: true,
+		},
+		"hostname_label": {
+			Type:     schema.TypeString,
+			Optional: true,
+			Computed: true,
+		},
+		"private_ip": {
+			Type:     schema.TypeString,
+			Optional: true,
+			Computed: true,
+			ForceNew: true,
+		},
+		"skip_source_dest_check": {
+			Type:     schema.TypeBool,
+			Optional: true,
+			Default:  false,
+		},
+		"subnet_id": {
+			Type:     schema.TypeString,
+			Required: true,
+			ForceNew: true,
+		},
+	},
+}
+
+// vnicDetailsList is assumed to be non-nil and non-empty.
+func SetCreateVnicOptions(vnicDetailsList []interface{}) (vnicOpts *baremetal.CreateVnicOptions) {
+	vnic := vnicDetailsList[0].(map[string]interface{})
 
 	vnicOpts = &baremetal.CreateVnicOptions{}
 	vnicOpts.SubnetID = vnic["subnet_id"].(string)
@@ -29,21 +66,52 @@ func SetCreateVnicOptions(rawCreateVnicDetails interface{}) (vnicOpts *baremetal
 		vnicOpts.PrivateIp = privateIp.(string)
 	}
 
-	// Work around for tf bug https://github.com/hashicorp/terraform/issues/13512.
-	// For bool values that are nested in maps, if the value is set to true/false then
-	// it will appear here as "1"/"0". However, if the value is set to "true"/"false"
-	// then it will appear here as "true"/"false". ParseBool() handles both of these cases.
 	assignPublicIp := vnic["assign_public_ip"]
 	if assignPublicIp != nil {
 		vnicOpts.AssignPublicIp = new(bool)
-		*vnicOpts.AssignPublicIp, err = strconv.ParseBool(assignPublicIp.(string))
+		*vnicOpts.AssignPublicIp = assignPublicIp.(bool)
+	}
+
+	skipSourceDestCheck := vnic["skip_source_dest_check"]
+	if skipSourceDestCheck != nil {
+		vnicOpts.SkipSourceDestCheck = new(bool)
+		*vnicOpts.SkipSourceDestCheck = skipSourceDestCheck.(bool)
+	}
+
+	return
+}
+
+// vnicDetailsList is assumed to be non-nil and non-empty.
+func SetUpdateVnicOptions(vnicDetailsList []interface{}) (vnicOpts *baremetal.UpdateVnicOptions) {
+	vnic := vnicDetailsList[0].(map[string]interface{})
+	vnicOpts = &baremetal.UpdateVnicOptions{}
+
+	displayName := vnic["display_name"]
+	if displayName != nil {
+		vnicOpts.DisplayName = displayName.(string)
+	}
+
+	hostnameLabel := vnic["hostname_label"]
+	if hostnameLabel != nil {
+		vnicOpts.HostnameLabel = hostnameLabel.(string)
 	}
 
 	skipSourceDestCheck := vnic["skip_source_dest_check"]
 	if skipSourceDestCheck != nil {
 		vnicOpts.SkipSourceDestCheck = new(bool)
-		*vnicOpts.SkipSourceDestCheck, err = strconv.ParseBool(skipSourceDestCheck.(string))
+		*vnicOpts.SkipSourceDestCheck = skipSourceDestCheck.(bool)
 	}
 
 	return
 }
+
+func RefreshCreateVnicDetails(resourceData *schema.ResourceData, vnic *baremetal.Vnic) {
+	vnicDetails := make(map[string]interface{})
+	vnicDetails["subnet_id"] = vnic.SubnetID
+	vnicDetails["assign_public_ip"] = len(vnic.PublicIPAddress) > 0
+	vnicDetails["display_name"] = vnic.DisplayName
+	vnicDetails["hostname_label"] = vnic.HostnameLabel
+	vnicDetails["private_ip"] = vnic.PrivateIPAddress
+	vnicDetails["skip_source_dest_check"] = vnic.SkipSourceDestCheck
+	resourceData.Set("create_vnic_details", []interface{}{vnicDetails})
+}

resource_obmcs_core_instance.go

@@ -30,43 +30,14 @@ func InstanceResource() *schema.Resource {
 		Delete: deleteInstance,
 		Schema: map[string]*schema.Schema{
 			"create_vnic_details": {
-				Type:     schema.TypeMap,
+				Type:     schema.TypeList,
 				Optional: true,
-				ForceNew: true,
-				Elem: &schema.Resource{
-					Schema: map[string]*schema.Schema{
-						"assign_public_ip": {
-							Type:     schema.TypeBool,
-							Optional: true,
-							ForceNew: true,
-						},
-						"display_name": {
-							Type:     schema.TypeString,
-							Optional: true,
-							ForceNew: true,
-						},
-						"hostname_label": {
-							Type:     schema.TypeString,
-							Optional: true,
-							ForceNew: true,
-						},
-						"private_ip": {
-							Type:     schema.TypeString,
-							Optional: true,
-							ForceNew: true,
-						},
-						"skip_source_dest_check": {
-							Type:     schema.TypeBool,
-							Optional: true,
-							ForceNew: true,
-						},
-						"subnet_id": {
-							Type:     schema.TypeString,
-							Required: true,
-							ForceNew: true,
-						},
-					},
-				},
+				// This must be set to computed, since it's optional and required subnet_id param is being refreshed.
+				// If this isn't computed, then that would always force a change on users who do not set create_vnic_details.
+				Computed: true,
+				MaxItems: 1,
+				MinItems: 1,
+				Elem:     createVnicDetailsSchema,
 			},
 			"availability_domain": {
 				Type:     schema.TypeString,
@@ -260,7 +231,7 @@ func (s *InstanceResourceCrud) Create() (e error) {
 	}
 
 	if rawVnic, ok := s.D.GetOk("create_vnic_details"); ok {
-		opts.CreateVnicOptions, e = SetCreateVnicOptions(rawVnic)
+		opts.CreateVnicOptions = SetCreateVnicOptions(rawVnic.([]interface{}))
 	}
 
 	if e == nil {
@@ -277,12 +248,11 @@ func (s *InstanceResourceCrud) Create() (e error) {
 }
 
 /*
- * Return the public, private IP pair associated with the instance's primary Vnic.
+ * Return the primary VNIC for this instance.
  *
- * NOTE while the instance is still being created, calls to this function
- * can return  an error priort to the Vnic being attached.
+ * Note that this may return an error during instance creation or deletion.
  */
-func (s *InstanceResourceCrud) getInstanceIPs() (public_ip string, private_ip string, e error) {
+func (s *InstanceResourceCrud) getPrimaryVnic() (vnic *baremetal.Vnic, e error) {
 	compartmentID := s.Resource.CompartmentID
 
 	opts := &baremetal.ListVnicAttachmentsOptions{}
@@ -303,7 +273,7 @@ func (s *InstanceResourceCrud) getInstanceIPs() (public_ip string, private_ip st
 	}
 
 	if len(attachments) < 1 {
-		return "", "", errors.New("No VNIC attachments found.")
+		return nil, errors.New("No VNIC attachments found.")
 	}
 
 	for _, attachment := range attachments {
@@ -312,12 +282,12 @@ func (s *InstanceResourceCrud) getInstanceIPs() (public_ip string, private_ip st
 
 			// Ignore errors on GetVnic, since we might not have permissions to view some secondary VNICs.
 			if vnic != nil && vnic.IsPrimary {
-				return vnic.PublicIPAddress, vnic.PrivateIPAddress, nil
+				return vnic, nil
 			}
 		}
 	}
 
-	return "", "", errors.New("Primary VNIC not found.")
+	return nil, errors.New("Primary VNIC not found.")
 }
 
 func (s *InstanceResourceCrud) Get() (e error) {
@@ -326,25 +296,6 @@ func (s *InstanceResourceCrud) Get() (e error) {
 		s.Resource = res
 	}
 
-	if e != nil {
-		return e
-	}
-
-	// Compute instance IPs through attached Vnic
-	if s.Resource.State != baremetal.ResourceRunning {
-		return
-	}
-	public_ip, private_ip, e2 := s.getInstanceIPs()
-	if e2 != nil {
-		log.Printf("[DEBUG] Primary VNIC could not be found: %q (InstanceID: %q, State: %q)", e2, s.Resource.ID, s.Resource.State)
-	}
-
-	if public_ip != "" {
-		s.public_ip = public_ip
-	}
-	if private_ip != "" {
-		s.private_ip = private_ip
-	}
 	return
 }
 
@@ -355,6 +306,27 @@ func (s *InstanceResourceCrud) Update() (e error) {
 	}
 
 	s.Resource, e = s.Client.UpdateInstance(s.D.Id(), opts)
+	if e != nil {
+		return
+	}
+
+	// HasChange returns true for any changes within create_vnic_details.
+	if !s.D.HasChange("create_vnic_details") {
+		log.Printf("[DEBUG] No changes to primary VNIC. Instance ID: %q", s.Resource.ID)
+		return
+	}
+
+	log.Printf("[DEBUG] Updating instance's primary VNIC. Instance ID: %q", s.Resource.ID)
+	vnic, e := s.getPrimaryVnic()
+	if e != nil {
+		log.Printf("[ERROR] Primary VNIC could not be found during instance update: %q (Instance ID: %q, State: %q)", e, s.Resource.ID, s.Resource.State)
+		return
+	}
+
+	if rawVnic, ok := s.D.GetOk("create_vnic_details"); ok {
+		_, e = s.Client.UpdateVnic(vnic.ID, SetUpdateVnicOptions(rawVnic.([]interface{})))
+	}
+
 	return
 }
 
@@ -369,8 +341,21 @@ func (s *InstanceResourceCrud) SetData() {
 	s.D.Set("shape", s.Resource.Shape)
 	s.D.Set("state", s.Resource.State)
 	s.D.Set("time_created", s.Resource.TimeCreated.String())
-	s.D.Set("public_ip", s.public_ip)
-	s.D.Set("private_ip", s.private_ip)
+
+	if s.Resource.State != baremetal.ResourceRunning {
+		return
+	}
+
+	vnic, vnicError := s.getPrimaryVnic()
+	if vnicError != nil {
+		log.Printf("[WARN] Primary VNIC could not be found during instance refresh: %q (Instance ID: %q, State: %q)", vnicError, s.Resource.ID, s.Resource.State)
+		return
+	}
+
+	s.D.Set("public_ip", vnic.PublicIPAddress)
+	s.D.Set("private_ip", vnic.PrivateIPAddress)
+
+	RefreshCreateVnicDetails(s.D, vnic)
 }
 
 func (s *InstanceResourceCrud) Delete() (e error) {