From 53ca4e9f5f46943c3b150d015f4ade6550cb2bea Mon Sep 17 00:00:00 2001 From: Anna Khmelnitsky Date: Wed, 22 Nov 2023 22:30:49 +0000 Subject: [PATCH] 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 --- nsxt/gateway_common.go | 15 ++- nsxt/policy_common.go | 9 ++ nsxt/resource_nsxt_policy_tier0_gateway.go | 12 ++- ...resource_nsxt_policy_tier0_gateway_test.go | 96 +++++++++++++++++++ nsxt/resource_nsxt_policy_tier1_gateway.go | 12 ++- nsxt/utils.go | 18 ++++ nsxt/utils_test.go | 6 ++ .../docs/r/policy_tier0_gateway.html.markdown | 3 + .../docs/r/policy_tier1_gateway.html.markdown | 3 + 9 files changed, 170 insertions(+), 4 deletions(-) diff --git a/nsxt/gateway_common.go b/nsxt/gateway_common.go index 2dd896d87..efefd3455 100644 --- 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 index 169a2f506..3da84f227 100644 --- 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 index 1c9dda910..9dcc4e879 100644 --- 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 index 7951263e8..824952b54 100644 --- 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 index 1bb58f096..65b9b7ba3 100644 --- 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 index 146a7e551..caedfc5be 100644 --- 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 index 4cdc4fea1..ec2558be3 100644 --- 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 index 782c0235b..dd8ba094f 100644 --- a/website/docs/r/policy_tier0_gateway.html.markdown +++ b/website/docs/r/policy_tier0_gateway.html.markdown @@ -92,10 +92,12 @@ resource "nsxt_policy_tier0_gateway" "tier0_gw" { failover_mode = "PREEMPTIVE" locale_service { + nsx_id = "paris" edge_cluster_path = data.nsxt_policy_edge_cluster.paris.path } locale_service { + nsx_id = "london" edge_cluster_path = data.nsxt_policy_edge_cluster.london.path preferred_edge_paths = [data.nsxt_policy_edge_node.edge1.path] } @@ -122,6 +124,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 index 4042bbdcb..2967ebb4e 100644 --- a/website/docs/r/policy_tier1_gateway.html.markdown +++ b/website/docs/r/policy_tier1_gateway.html.markdown @@ -58,10 +58,12 @@ resource "nsxt_policy_tier1_gateway" "tier1_gw" { display_name = "Tier1-gw1" locale_service { + nsx_id = "paris" edge_cluster_path = data.nsxt_policy_edge_cluster.paris.path } locale_service { + nsx_id = "london" edge_cluster_path = data.nsxt_policy_edge_cluster.london.path preferred_edge_paths = [data.nsxt_policy_egde_node.edge1.path] } @@ -119,6 +121,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.