Virtual Circuit public_prefixes to be updatable and importable

Author: Ravi Tandon

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ### Fixed
 - Timeout should be updatable for the `oci_containerengine_cluster` and `oci_containerengine_node_pool` resources
+- Virtual Circuit `public_prefixes` to be updatable and importable. [Issue #700](https://github.com/terraform-providers/terraform-provider-oci/issues/700)
 
 ## 3.14.0 (January 29, 2019)
 

GNUmakefile

@@ -5,7 +5,7 @@ GOFMT_FILES?=$$(find . -name '*.go' |grep -v vendor)
 PKG_NAME=oci
 WEBSITE_REPO=github.com/hashicorp/terraform-website
 
-prefix := $(if $(debug),TF_LOG=DEBUG OCI_GO_SDK_DEBUG=1, )
+prefix := $(if $(debug),TF_LOG=DEBUG OCI_GO_SDK_DEBUG=v, )
 timeout := $(if $(timeout), $(timeout), 120m)
 run_regex := $(if $(run), -run $(run), )
 skip_goimports_check_flag := $(if $(skip_goimports_check), -s, )

docs/examples/fast_connect/variables.tf

@@ -41,15 +41,19 @@ variable "virtual_circuit_display_name" {
 }
 
 variable "virtual_circuit_public_prefixes_cidr_block" {
-  default = "0.0.0.0/5"
+  default = "0.0.0.0/6"
 }
 
 variable "virtual_circuit_public_prefixes_cidr_block2" {
-  default = "206.209.218.0/24"
+  default = "206.209.218.0/25"
+}
+
+variable "virtual_circuit_public_prefixes_cidr_block3" {
+  default = "206.209.219.0/24"
 }
 
 variable "virtual_circuit_region" {
-  default = "r1"
+  default = "us-phoenix-1"
 }
 
 variable "virtual_circuit_state" {

docs/examples/fast_connect/virtual_circuit.tf

@@ -45,6 +45,7 @@ resource "oci_core_virtual_circuit" "virtual_circuit_public" {
   display_name     = "${var.virtual_circuit_display_name}"
 
   #provider_service_id = "${oci_core_provider_service.provider_service.id}"
+
   public_prefixes = [
     {
       #Required
@@ -53,8 +54,10 @@ resource "oci_core_virtual_circuit" "virtual_circuit_public" {
     {
       cidr_block = "${var.virtual_circuit_public_prefixes_cidr_block2}"
     },
+    {
+      cidr_block = "${var.virtual_circuit_public_prefixes_cidr_block3}"
+    },
   ]
-
   region = "${var.virtual_circuit_region}"
 }
 

oci/core_virtual_circuit_resource.go

@@ -6,12 +6,11 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/hashicorp/terraform/helper/hashcode"
 	"github.com/hashicorp/terraform/helper/schema"
 
-	"strings"
-
 	oci_core "github.com/oracle/oci-go-sdk/core"
 )
 
@@ -107,16 +106,13 @@ func CoreVirtualCircuitResource() *schema.Resource {
 			"public_prefixes": {
 				Type:     schema.TypeSet,
 				Optional: true,
-				Computed: true,
-				ForceNew: true,
 				Set:      publicPrefixesHashCodeForSets,
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						// Required
 						"cidr_block": {
 							Type:     schema.TypeString,
 							Required: true,
-							ForceNew: true,
 						},
 
 						// Optional
@@ -356,27 +352,35 @@ func (s *CoreVirtualCircuitResourceCrud) Get() error {
 
 	s.Res = &response.VirtualCircuit
 
-	// VirtualCircuitPublicPrefixes are returned from another API, make a List call on them if the VirtualCircuit is of type 'PUBLIC'
-	if type_, ok := s.D.GetOkExists("type"); ok && strings.EqualFold(type_.(string), string(oci_core.VirtualCircuitTypePublic)) && s.Res.PublicPrefixes == nil {
-		request2 := oci_core.ListVirtualCircuitPublicPrefixesRequest{}
-		request2.VirtualCircuitId = request.VirtualCircuitId
-
-		response2, err2 := s.Client.ListVirtualCircuitPublicPrefixes(context.Background(), request2)
+	ppRequest := oci_core.ListVirtualCircuitPublicPrefixesRequest{}
+	ppRequest.RequestMetadata.RetryPolicy = getRetryPolicy(s.DisableNotFoundRetries, "core")
+	ppRequest.VirtualCircuitId = request.VirtualCircuitId
 
-		publicPrefixes := []string{}
-		for _, item := range response2.Items {
-			publicPrefixes = append(publicPrefixes, *item.CidrBlock)
-		}
+	ppResponse, ppErr := s.Client.ListVirtualCircuitPublicPrefixes(context.Background(), ppRequest)
+	if ppErr != nil {
+		return ppErr
+	}
 
-		s.Res.PublicPrefixes = publicPrefixes
-		if err2 != nil {
-			return err2
-		}
+	publicPrefixes := []string{}
+	for _, item := range ppResponse.Items {
+		publicPrefixes = append(publicPrefixes, *item.CidrBlock)
 	}
+
+	s.Res.PublicPrefixes = publicPrefixes
+
 	return nil
 }
 
 func (s *CoreVirtualCircuitResourceCrud) Update() error {
+	// Update public prefixes, if changed
+	// Cannot update PublicPrefix when the VirtualCircuit is in state PROVISIONING so public prefixes should be updated first
+	if s.D.HasChange("public_prefixes") {
+		err := s.updatePublicPrefixes()
+		if err != nil {
+			return fmt.Errorf("unable to update 'public_prefixes', error: %v", err)
+		}
+	}
+
 	request := oci_core.UpdateVirtualCircuitRequest{}
 
 	if bandwidthShapeName, ok := s.D.GetOkExists("bandwidth_shape_name"); ok {
@@ -435,6 +439,73 @@ func (s *CoreVirtualCircuitResourceCrud) Update() error {
 	}
 
 	s.Res = &response.VirtualCircuit
+	return nil
+}
+
+// Update public prefixes using BulkAdd and BulkDelete APIs by computing the diff
+func (s *CoreVirtualCircuitResourceCrud) updatePublicPrefixes() error {
+	virtualCircuitId := s.D.Id()
+
+	o, n := s.D.GetChange("public_prefixes")
+	if o == nil {
+		o = new(schema.Set)
+	}
+	if n == nil {
+		n = new(schema.Set)
+	}
+
+	os := o.(*schema.Set)
+	ns := n.(*schema.Set)
+
+	newPublicPrefixesToAdd := ns.Difference(os).List()
+	oldPublicPrefixesToDelete := os.Difference(ns).List()
+
+	var publicPrefixesToAdd []oci_core.CreateVirtualCircuitPublicPrefixDetails
+	var publicPrefixesToDelete []oci_core.DeleteVirtualCircuitPublicPrefixDetails
+
+	for _, nppMap := range newPublicPrefixesToAdd {
+		npp := mapToCreateVirtualCircuitPublicPrefixDetails(nppMap.(map[string]interface{}))
+		publicPrefixesToAdd = append(publicPrefixesToAdd, npp)
+	}
+
+	for _, oppMap := range oldPublicPrefixesToDelete {
+		opp := mapToDeleteVirtualCircuitPublicPrefixDetails(oppMap.(map[string]interface{}))
+		publicPrefixesToDelete = append(publicPrefixesToDelete, opp)
+	}
+
+	// Add the public prefixes first, if any
+	// And, wait for the update to complete before proceeding for subsequent updates if state is PROVISIONING
+	if len(publicPrefixesToAdd) > 0 {
+		addRequest := oci_core.BulkAddVirtualCircuitPublicPrefixesRequest{}
+		addRequest.RequestMetadata.RetryPolicy = getRetryPolicy(s.DisableNotFoundRetries, "core")
+		addRequest.PublicPrefixes = publicPrefixesToAdd
+		addRequest.VirtualCircuitId = &virtualCircuitId
+		addErr := s.Client.BulkAddVirtualCircuitPublicPrefixes(context.Background(), addRequest)
+		if addErr != nil {
+			return fmt.Errorf("failed to add public prefixes, error: %v", addErr)
+		}
+
+		if waitErr := waitForUpdatedState(s.D, s); waitErr != nil {
+			return waitErr
+		}
+	}
+
+	// Delete the old public prefixes, if any, after adding new ones
+	// And, wait for the update to complete before proceeding for subsequent updates if state is PROVISIONING
+	if len(publicPrefixesToDelete) > 0 {
+		deleteRequest := oci_core.BulkDeleteVirtualCircuitPublicPrefixesRequest{}
+		deleteRequest.RequestMetadata.RetryPolicy = getRetryPolicy(s.DisableNotFoundRetries, "core")
+		deleteRequest.PublicPrefixes = publicPrefixesToDelete
+		deleteRequest.VirtualCircuitId = &virtualCircuitId
+		deleteErr := s.Client.BulkDeleteVirtualCircuitPublicPrefixes(context.Background(), deleteRequest)
+		if deleteErr != nil {
+			return fmt.Errorf("failed to delete public prefixes, error: %v", deleteErr)
+		}
+
+		if waitErr := waitForUpdatedState(s.D, s); waitErr != nil {
+			return waitErr
+		}
+	}
 
 	return nil
 }
@@ -492,13 +563,11 @@ func (s *CoreVirtualCircuitResourceCrud) SetData() error {
 
 	s.D.Set("provider_state", s.Res.ProviderState)
 
-	if s.Res.PublicPrefixes != nil {
-		publicPrefixes := []interface{}{}
-		for _, item := range s.Res.PublicPrefixes {
-			publicPrefixes = append(publicPrefixes, CreateVirtualCircuitPublicPrefixDetailsToMap(item))
-		}
-		s.D.Set("public_prefixes", schema.NewSet(publicPrefixesHashCodeForSets, publicPrefixes))
+	publicPrefixes := []interface{}{}
+	for _, item := range s.Res.PublicPrefixes {
+		publicPrefixes = append(publicPrefixes, CreateVirtualCircuitPublicPrefixDetailsToMap(item))
 	}
+	s.D.Set("public_prefixes", schema.NewSet(publicPrefixesHashCodeForSets, publicPrefixes))
 
 	if s.Res.ReferenceComment != nil {
 		s.D.Set("reference_comment", *s.Res.ReferenceComment)
@@ -521,6 +590,30 @@ func (s *CoreVirtualCircuitResourceCrud) SetData() error {
 	return nil
 }
 
+// Converting raw set data from state diff to DeleteVirtualCircuitPublicPrefixDetails
+func mapToDeleteVirtualCircuitPublicPrefixDetails(publicPrefix map[string]interface{}) oci_core.DeleteVirtualCircuitPublicPrefixDetails {
+	result := oci_core.DeleteVirtualCircuitPublicPrefixDetails{}
+
+	if cidrBlock, ok := publicPrefix["cidr_block"]; ok {
+		tmp := cidrBlock.(string)
+		result.CidrBlock = &tmp
+	}
+
+	return result
+}
+
+// Converting raw set data from state diff to CreateVirtualCircuitPublicPrefixDetails
+func mapToCreateVirtualCircuitPublicPrefixDetails(publicPrefix map[string]interface{}) oci_core.CreateVirtualCircuitPublicPrefixDetails {
+	result := oci_core.CreateVirtualCircuitPublicPrefixDetails{}
+
+	if cidrBlock, ok := publicPrefix["cidr_block"]; ok {
+		tmp := cidrBlock.(string)
+		result.CidrBlock = &tmp
+	}
+
+	return result
+}
+
 func (s *CoreVirtualCircuitResourceCrud) mapToCreateVirtualCircuitPublicPrefixDetails(fieldKeyFormat string) (oci_core.CreateVirtualCircuitPublicPrefixDetails, error) {
 	result := oci_core.CreateVirtualCircuitPublicPrefixDetails{}
 
@@ -553,20 +646,24 @@ func (s *CoreVirtualCircuitResourceCrud) mapToCrossConnectMapping(fieldKeyFormat
 		result.CrossConnectOrCrossConnectGroupId = &tmp
 	}
 
-	if customerBgpPeeringIp, ok := s.D.GetOkExists(fmt.Sprintf(fieldKeyFormat, "customer_bgp_peering_ip")); ok {
-		tmp := customerBgpPeeringIp.(string)
-		result.CustomerBgpPeeringIp = &tmp
-	}
+	// customer_bgp_peering_ip, oracleBgpPeeringIp are not allowed in requests for PUBLIC virtual circuit
+	if vcType, ok := s.D.GetOkExists("type"); ok && !strings.EqualFold(vcType.(string), string(oci_core.VirtualCircuitTypePublic)) {
+		if customerBgpPeeringIp, ok := s.D.GetOkExists(fmt.Sprintf(fieldKeyFormat, "customer_bgp_peering_ip")); ok {
+			tmp := customerBgpPeeringIp.(string)
+			result.CustomerBgpPeeringIp = &tmp
+		}
 
-	if oracleBgpPeeringIp, ok := s.D.GetOkExists(fmt.Sprintf(fieldKeyFormat, "oracle_bgp_peering_ip")); ok {
-		tmp := oracleBgpPeeringIp.(string)
-		result.OracleBgpPeeringIp = &tmp
+		if oracleBgpPeeringIp, ok := s.D.GetOkExists(fmt.Sprintf(fieldKeyFormat, "oracle_bgp_peering_ip")); ok {
+			tmp := oracleBgpPeeringIp.(string)
+			result.OracleBgpPeeringIp = &tmp
+		}
 	}
 
 	if vlan, ok := s.D.GetOkExists(fmt.Sprintf(fieldKeyFormat, "vlan")); ok {
 		tmp := vlan.(int)
-		//Vlan value must be greater than or equal to 100. It cannot be specified for certain circuit types.
-		if tmp >= 100 {
+		// Vlan value must be greater than or equal to 100.
+		// It cannot be specified for certain circuit types, hence protecting against default '0' values
+		if tmp > 0 {
 			result.Vlan = &tmp
 		}
 	}

oci/core_virtual_circuit_test.go

@@ -91,7 +91,7 @@ var (
 		"oracle_bgp_peering_ip":   Representation{repType: Required, create: `10.0.0.19/31`, update: `10.0.0.21/31`},
 	}
 	virtualCircuitPublicPrefixesRepresentation = map[string]interface{}{
-		"cidr_block": Representation{repType: Required, create: `0.0.0.0/5`},
+		"cidr_block": Representation{repType: Required, create: `0.0.0.0/5`, update: `0.0.0.0/6`},
 	}
 
 	VirtualCircuitWithProviderResourceConfigFilter = `
@@ -175,6 +175,32 @@ func TestCoreVirtualCircuitResource_basic(t *testing.T) {
 					},
 				),
 			},
+			// verify update - PUBLIC Virtual Circuit
+			{
+				Config: config + VirtualCircuitPublicPropertyVariables + compartmentIdVariableStr + VirtualCircuitResourceDependencies +
+					generateResourceFromRepresentationMap("oci_core_virtual_circuit", "test_virtual_circuit", Required, Update, virtualCircuitPublicRequiredOnlyRepresentation),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					resource.TestCheckResourceAttr(resourceName, "compartment_id", compartmentId),
+					resource.TestCheckResourceAttr(resourceName, "cross_connect_mappings.#", "1"),
+					resource.TestCheckResourceAttrSet(resourceName, "cross_connect_mappings.0.cross_connect_or_cross_connect_group_id"),
+					resource.TestCheckResourceAttr(resourceName, "cross_connect_mappings.0.vlan", "300"),
+					resource.TestCheckResourceAttr(resourceName, "customer_bgp_asn", "11"),
+					resource.TestCheckResourceAttr(resourceName, "public_prefixes.#", "1"),
+					CheckResourceSetContainsElementWithProperties(resourceName, "public_prefixes", map[string]string{
+						"cidr_block": "0.0.0.0/6",
+					},
+						[]string{}),
+					resource.TestCheckResourceAttr(resourceName, "type", "PUBLIC"),
+
+					func(s *terraform.State) (err error) {
+						resId2, err = fromInstanceState(s, resourceName, "id")
+						if resId != resId2 {
+							return fmt.Errorf("Resource recreated when it was supposed to be updated.")
+						}
+						return err
+					},
+				),
+			},
 			// delete before next create
 			{
 				Config: config + compartmentIdVariableStr + VirtualCircuitResourceDependencies,
@@ -341,14 +367,11 @@ func TestCoreVirtualCircuitResource_basic(t *testing.T) {
 			},
 			// verify resource import
 			{
-				Config: config + compartmentIdVariableStr + VirtualCircuitResourceDependencies + VirtualCircuitPrivatePropertyVariables +
-					generateResourceFromRepresentationMap("oci_core_virtual_circuit", "test_virtual_circuit", Optional, Update, virtualCircuitRepresentation),
+				Config:            config,
 				ImportState:       true,
 				ImportStateVerify: true,
 				ImportStateVerifyIgnore: []string{
-					"public_prefixes",
 					"region",
-					"virtual_circuit_id",
 				},
 				ResourceName: resourceName,
 			},

oci/crud_helpers.go

@@ -372,6 +372,18 @@ func stateRefreshFunc(sync StatefulResource) resource.StateRefreshFunc {
 	}
 }
 
+// Helper function to wait for update to reach terminal state before doing another update
+// Useful in situations where more than one update is needed and prior update needs to complete
+func waitForUpdatedState(d *schema.ResourceData, sync ResourceUpdater) error {
+	if stateful, ok := sync.(StatefullyUpdatedResource); ok {
+		if e := waitForStateRefresh(stateful, d.Timeout(schema.TimeoutUpdate), "update", stateful.UpdatedPending(), stateful.UpdatedTarget()); e != nil {
+			return e
+		}
+	}
+
+	return nil
+}
+
 // waitForStateRefresh takes a StatefulResource, a timeout duration, a list of states to treat as Pending, and a list of states to treat as Target. It uses those to wrap resource.StateChangeConf.WaitForState(). If the resource returns a missing status, it will not be treated as an error.
 //
 // sync.D.Id must be set.