Allow to configure nsx_id for locale service

Rather than recreating locale services with every update, allow to change existing locale service by specifying it nsx_id When nsx_id is specified for locale service, update will happen in place, thus keeping all objects dependant on the locale service intact (for example, service interfaces). Note that nsx_id would only be set in state if specified by user, otherwise non-empty plan would always be produced due to SDK design issue that offers limited support for Computed fields in TypeSet nested schema. This workaround should be removed after upgrading to plugin framework. When nsx_id is not speicified, there is no change of behavior. Signed-off-by: Anna Khmelnitsky <[email protected]>
vmware · Dec 5, 2023 · 1692eab · 1692eab
1 parent e9cf9cf
commit 1692eab
Show file tree

Hide file tree

Showing 9 changed files with 166 additions and 4 deletions.
diff --git a/nsxt/gateway_common.go b/nsxt/gateway_common.go
@@ -90,6 +90,7 @@ func getPolicyLocaleServiceSchema(isTier1 bool) *schema.Schema {
 	}
 
 	elemSchema := map[string]*schema.Schema{
+		"nsx_id": getNsxIDSchema(),
 		"edge_cluster_path": {
 			Type:         schema.TypeString,
 			Description:  "The path of the edge cluster connected to this gateway",
@@ -297,15 +298,25 @@ func initGatewayLocaleServices(context utl.SessionContext, d *schema.ResourceDat
 		}
 	}
 	lsType := "LocaleServices"
+	idMap := make(map[string]bool)
 	for _, service := range services {
 		cfg := service.(map[string]interface{})
 		edgeClusterPath := cfg["edge_cluster_path"].(string)
 		edgeNodes := interfaceListToStringList(cfg["preferred_edge_paths"].([]interface{}))
 		path := cfg["path"].(string)
+		nsxID := cfg["nsx_id"].(string)
+		// validate unique ids are provided for services
+		if len(nsxID) > 0 {
+			if _, ok := idMap[nsxID]; ok {
+				return nil, fmt.Errorf("Duplicate nsx_id for locale_service %s - please specify unique id", nsxID)
+			}
+			idMap[nsxID] = true
+		}
 
 		var serviceID string
-		if path != "" {
-			serviceID = getPolicyIDFromPath(path)
+		if nsxID != "" {
+			log.Printf("[DEBUG] Updating locale service %s", path)
+			serviceID = nsxID
 		} else {
 			serviceID = newUUID()
 			log.Printf("[DEBUG] Preparing to create locale service %s for gateway %s", serviceID, d.Id())

diff --git a/nsxt/policy_common.go b/nsxt/policy_common.go
@@ -36,6 +36,15 @@ func getNsxIDSchema() *schema.Schema {
 	}
 }
 
+func getNestedNsxIDSchema() *schema.Schema {
+	return &schema.Schema{
+		Type:        schema.TypeString,
+		Description: "NSX ID for this resource",
+		Optional:    true,
+		ForceNew:    true,
+	}
+}
+
 func getFlexNsxIDSchema(readOnly bool) *schema.Schema {
 	return &schema.Schema{
 		Type:        schema.TypeString,

diff --git a/nsxt/resource_nsxt_policy_tier0_gateway.go b/nsxt/resource_nsxt_policy_tier0_gateway.go
@@ -947,12 +947,14 @@ func resourceNsxtPolicyTier0GatewayRead(d *schema.ResourceData, m interface{}) e
 		return handleReadError(d, "Locale Service for T0", id, err)
 	}
 	var services []map[string]interface{}
-	_, shouldSetLS := d.GetOk("locale_service")
+	intentServices, shouldSetLS := d.GetOk("locale_service")
 	// decide if we should set locale_service or edge_cluser_path
 	// for GM, it is always locale_service; for LM, config dependent
 	if isGlobalManager {
 		shouldSetLS = true
 	}
+	// map of nsx IDs that was provided in locale_services in intent
+	nsxIDMap := getAttrKeyMapFromSchemaSet(intentServices, "nsx_id")
 	if len(localeServices) > 0 {
 		for i, service := range localeServices {
 			if shouldSetLS {
@@ -962,6 +964,14 @@ func resourceNsxtPolicyTier0GatewayRead(d *schema.ResourceData, m interface{}) e
 				cfgMap["preferred_edge_paths"] = service.PreferredEdgePaths
 				cfgMap["revision"] = service.Revision
 				cfgMap["display_name"] = service.DisplayName
+				// to avoid diff and recreation of locale service, we set nsx_id only
+				// if user specified it in the intent.
+				// this workaround is necessary due to lack of proper support for computed
+				// values in TypeSet
+				// TODO: refactor this post upgrade to plugin framework
+				if _, ok := nsxIDMap[*service.Id]; ok {
+					cfgMap["nsx_id"] = service.Id
+				}
 				redistributionConfigs := getLocaleServiceRedistributionConfig(&localeServices[i])
 				if d.Get("redistribution_set").(bool) {
 					// redistribution_config is deprecated and should be

diff --git a/nsxt/resource_nsxt_policy_tier0_gateway_test.go b/nsxt/resource_nsxt_policy_tier0_gateway_test.go
@@ -5,6 +5,7 @@ package nsxt
 
 import (
 	"fmt"
+	"regexp"
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
@@ -65,6 +66,57 @@ func TestAccResourceNsxtPolicyTier0Gateway_basic(t *testing.T) {
 	})
 }
 
+// This test requires at least 2 edge nodes per cluster
+func TestAccResourceNsxtPolicyTier0Gateway_localeService(t *testing.T) {
+	name := getAccTestResourceName()
+	testResourceName := "nsxt_policy_tier0_gateway.test"
+	regexpPath, err := regexp.Compile("/.*/" + name)
+	if err != nil {
+		t.Errorf("Error while compiling regexp: %v", err)
+		return
+	}
+
+	resource.Test(t, resource.TestCase{
+		PreCheck: func() {
+			testAccPreCheck(t)
+			testAccNSXVersion(t, "3.0.0")
+			testAccEnvDefined(t, "NSXT_TEST_ADVANCED_TOPOLOGY")
+		},
+		Providers: testAccProviders,
+		CheckDestroy: func(state *terraform.State) error {
+			return testAccNsxtPolicyTier0CheckDestroy(state, name)
+		},
+		Steps: []resource.TestStep{
+			{
+				Config: testAccNsxtPolicyTier0CreateWithLocaleTemplate(name),
+				Check: resource.ComposeTestCheckFunc(
+					testAccNsxtPolicyTier0Exists(testResourceName),
+					resource.TestCheckResourceAttr(testResourceName, "display_name", name),
+					resource.TestCheckResourceAttr(testResourceName, "ha_mode", "ACTIVE_STANDBY"),
+					resource.TestCheckResourceAttr(testResourceName, "locale_service.#", "1"),
+					resource.TestCheckResourceAttr(testResourceName, "locale_service.0.nsx_id", name),
+					resource.TestMatchResourceAttr(testResourceName, "locale_service.0.path", regexpPath),
+					resource.TestCheckResourceAttrSet(testResourceName, "path"),
+					resource.TestCheckResourceAttrSet(testResourceName, "revision"),
+				),
+			},
+			{
+				Config: testAccNsxtPolicyTier0UpdateWithLocaleTemplate(name),
+				Check: resource.ComposeTestCheckFunc(
+					testAccNsxtPolicyTier0Exists(testResourceName),
+					resource.TestCheckResourceAttr(testResourceName, "display_name", name),
+					resource.TestCheckResourceAttr(testResourceName, "ha_mode", "ACTIVE_STANDBY"),
+					resource.TestCheckResourceAttr(testResourceName, "locale_service.#", "1"),
+					resource.TestCheckResourceAttr(testResourceName, "locale_service.0.nsx_id", name),
+					resource.TestMatchResourceAttr(testResourceName, "locale_service.0.path", regexpPath),
+					resource.TestCheckResourceAttrSet(testResourceName, "path"),
+					resource.TestCheckResourceAttrSet(testResourceName, "revision"),
+				),
+			},
+		},
+	})
+}
+
 func TestAccResourceNsxtPolicyTier0Gateway_withId(t *testing.T) {
 	name := getAccTestResourceName()
 	id := "test-id"
@@ -693,6 +745,50 @@ resource "nsxt_policy_tier0_gateway" "test" {
 }`, id, name, testAccNsxtPolicyTier0EdgeClusterTemplate())
 }
 
+func testAccNsxtPolicyTier0CreateWithLocaleTemplate(name string) string {
+	config := testAccNsxtPolicyGatewayFabricDeps(true) + fmt.Sprintf(`
+data "nsxt_policy_edge_node" "node1" {
+    edge_cluster_path = data.nsxt_policy_edge_cluster.EC.path
+    member_index      = 0
+}
+
+resource "nsxt_policy_tier0_gateway" "test" {
+  display_name              = "%s"
+  failover_mode             = "PREEMPTIVE"
+  ha_mode                   = "ACTIVE_STANDBY"
+
+  locale_service {
+    nsx_id = "%s"
+    edge_cluster_path    = data.nsxt_policy_edge_cluster.EC.path
+    preferred_edge_paths = [data.nsxt_policy_edge_node.node1.path]
+  }
+}`, name, name)
+
+	return testAccAdjustPolicyInfraConfig(config)
+}
+
+func testAccNsxtPolicyTier0UpdateWithLocaleTemplate(name string) string {
+	config := testAccNsxtPolicyGatewayFabricDeps(true) + fmt.Sprintf(`
+data "nsxt_policy_edge_node" "node1" {
+    edge_cluster_path = data.nsxt_policy_edge_cluster.EC.path
+    member_index      = 1
+}
+
+resource "nsxt_policy_tier0_gateway" "test" {
+  display_name              = "%s"
+  failover_mode             = "PREEMPTIVE"
+  ha_mode                   = "ACTIVE_STANDBY"
+
+  locale_service {
+    nsx_id = "%s"
+    edge_cluster_path    = data.nsxt_policy_edge_cluster.EC.path
+    preferred_edge_paths = [data.nsxt_policy_edge_node.node1.path]
+  }
+}`, name, name)
+
+	return testAccAdjustPolicyInfraConfig(config)
+}
+
 func testAccNsxtPolicyTier0SubnetsTemplate(name string) string {
 	return fmt.Sprintf(`
 resource "nsxt_policy_tier0_gateway" "test" {

diff --git a/nsxt/resource_nsxt_policy_tier1_gateway.go b/nsxt/resource_nsxt_policy_tier1_gateway.go
@@ -582,10 +582,12 @@ func resourceNsxtPolicyTier1GatewayRead(d *schema.ResourceData, m interface{}) e
 		return handleReadError(d, "Locale Service for T1", id, err)
 	}
 	var services []map[string]interface{}
-	_, shouldSetLS := d.GetOk("locale_service")
+	intentServices, shouldSetLS := d.GetOk("locale_service")
 	if context.ClientType == utl.Global {
 		shouldSetLS = true
 	}
+	// map of nsx IDs that was provided in locale_services in intent
+	nsxIDMap := getAttrKeyMapFromSchemaSet(intentServices, "nsx_id")
 
 	if len(localeServices) > 0 {
 		for _, service := range localeServices {
@@ -596,6 +598,14 @@ func resourceNsxtPolicyTier1GatewayRead(d *schema.ResourceData, m interface{}) e
 				cfgMap["preferred_edge_paths"] = service.PreferredEdgePaths
 				cfgMap["revision"] = service.Revision
 				cfgMap["display_name"] = service.DisplayName
+				// to avoid diff and recreation of locale service, we set nsx_id only
+				// if user specified it in the intent.
+				// this workaround is necessary due to lack of proper support for computed
+				// values in TypeSet
+				// TODO: refactor this post upgrade to plugin framework
+				if _, ok := nsxIDMap[*service.Id]; ok {
+					cfgMap["nsx_id"] = service.Id
+				}
 				services = append(services, cfgMap)
 
 			} else {

diff --git a/nsxt/utils.go b/nsxt/utils.go
@@ -70,6 +70,24 @@ func getStringListFromSchemaList(d *schema.ResourceData, schemaAttrName string)
 	return interface2StringList(d.Get(schemaAttrName).([]interface{}))
 }
 
+// helper to construct a map based on curtain attribute in schema set
+// this helper is only relevant for Sets of nested objects (not scalars), and attrName
+// is the object attribute value of which would appear as key in the returned map object.
+// this is useful when Read function needs to make a decision based on intent provided
+// by user in a nested schema
+func getAttrKeyMapFromSchemaSet(schemaSet interface{}, attrName string) map[string]bool {
+
+	keyMap := make(map[string]bool)
+	for _, item := range schemaSet.(*schema.Set).List() {
+		mapItem := item.(map[string]interface{})
+		if value, ok := mapItem[attrName]; ok {
+			keyMap[value.(string)] = true
+		}
+	}
+
+	return keyMap
+}
+
 func intList2int64List(configured []interface{}) []int64 {
 	vs := make([]int64, 0, len(configured))
 	for _, v := range configured {

diff --git a/nsxt/utils_test.go b/nsxt/utils_test.go
@@ -191,6 +191,12 @@ func getTestAnotherSiteName() string {
 	return os.Getenv("NSXT_TEST_ANOTHER_SITE_NAME")
 }
 
+func getTestAdvancedTopology() string {
+	// Non-basic testing topology available
+	// For now this is used by tests that have minimum 2 edge nodes per cluster
+	return os.Getenv("NSXT_TEST_ADVANCED_TOPOLOGY")
+}
+
 func getTestCertificateName(isClient bool) string {
 	if isClient {
 		return os.Getenv("NSXT_TEST_CLIENT_CERTIFICATE_NAME")

diff --git a/website/docs/r/policy_tier0_gateway.html.markdown b/website/docs/r/policy_tier0_gateway.html.markdown
@@ -122,6 +122,7 @@ The following arguments are supported:
 * `nsx_id` - (Optional) The NSX ID of this resource. If set, this ID will be used to create the policy resource.
 * `edge_cluster_path` - (Optional) The path of the edge cluster where the Tier-0 is placed.For advanced configuration and on Global Manager, use `locale_service` clause instead. Note that for some configurations (such as BGP) setting edge cluster is required.
 * `locale_service` - (Optional) This is required on NSX Global Manager. Multiple locale services can be specified for multiple locations.
+  * `nsx_id` - (Optional) NSX id for the locale service. It is recommended to specify this attribute in order to avoid unnecessary recreation of this object. Should be unique within the gateway.
   * `edge_cluster_path` - (Required) The path of the edge cluster where the Tier-0 is placed.
   * `preferred_edge_paths` - (Optional) Policy paths to edge nodes. Specified edge is used as preferred edge cluster member when failover mode is set to `PREEMPTIVE`.
   * `display_name` - (Optional) Display name for the locale service.

diff --git a/website/docs/r/policy_tier1_gateway.html.markdown b/website/docs/r/policy_tier1_gateway.html.markdown
@@ -119,6 +119,7 @@ The following arguments are supported:
   * `project_id` - (Required) The ID of the project which the object belongs to
 * `edge_cluster_path` - (Optional) The path of the edge cluster where the Tier-1 is placed.For advanced configuration, use `locale_service` clause instead. 
 * `locale_service` - (Optional) This argument is required on NSX Global Manager. Multiple locale services can be specified for multiple locations.
+  * `nsx_id` - (Optional) NSX id for the locale service. It is recommended to specify this attribute in order to avoid unnecessary recreation of this object. Should be unique within the gateway.
   * `edge_cluster_path` - (Required) The path of the edge cluster where the Tier-0 is placed.
   * `preferred_edge_paths` - (Optional) Policy paths to edge nodes. Specified edge is used as preferred edge cluster member when failover mode is set to `PREEMPTIVE`.
   * `display_name` - (Optional) Display name for the locale service.