From 719b4497ea7d51d9a0b30f59a00425ef14bcb767 Mon Sep 17 00:00:00 2001 From: Anna Khmelnitsky Date: Wed, 18 May 2022 21:10:24 +0000 Subject: [PATCH] Fix bgp config for VRF gateway Since BGP on VRF gateway is mostly inherited from parent gateway, recent NSX versions disallow configuring most of BGP settings on it. Even resource defaults would cause BGP resource on VRF to error out. This change fixes this issue, and in addition fixes potential segmentation fault for BGP resource when locale service is not present. Signed-off-by: Anna Khmelnitsky --- nsxt/resource_nsxt_policy_bgp_config.go | 62 +++++++++++++------ nsxt/resource_nsxt_policy_tier0_gateway.go | 28 ++++++++- ...resource_nsxt_policy_tier0_gateway_test.go | 21 +++++-- 3 files changed, 86 insertions(+), 25 deletions(-) diff --git a/nsxt/resource_nsxt_policy_bgp_config.go b/nsxt/resource_nsxt_policy_bgp_config.go index 4fb5e32d8..44d79245d 100644 --- a/nsxt/resource_nsxt_policy_bgp_config.go +++ b/nsxt/resource_nsxt_policy_bgp_config.go @@ -70,7 +70,7 @@ func resourceNsxtPolicyBgpConfigRead(d *schema.ResourceData, m interface{}) erro return nil } -func resourceNsxtPolicyBgpConfigToStruct(d *schema.ResourceData) (*model.BgpRoutingConfig, error) { +func resourceNsxtPolicyBgpConfigToStruct(d *schema.ResourceData, isVRF bool) (*model.BgpRoutingConfig, error) { ecmp := d.Get("ecmp").(bool) enabled := d.Get("enabled").(bool) interSrIbgp := d.Get("inter_sr_ibgp").(bool) @@ -97,25 +97,40 @@ func resourceNsxtPolicyBgpConfigToStruct(d *schema.ResourceData) (*model.BgpRout } } - restartTimerStruct := model.BgpGracefulRestartTimer{ - RestartTimer: &restartTimer, - StaleRouteTimer: &staleTimer, + routeStruct := model.BgpRoutingConfig{ + Ecmp: &ecmp, + Enabled: &enabled, + RouteAggregations: aggregationStructs, } - restartConfigStruct := model.BgpGracefulRestartConfig{ - Mode: &restartMode, - Timer: &restartTimerStruct, - } + // For BGP on VRF, only limited attributes can be set + if isVRF { + vrfError := "%s can not be specified on VRF Gateway" + if restartTimer != int64(policyBGPGracefulRestartTimerDefault) { + return &routeStruct, fmt.Errorf(vrfError, "graceful_restart_timer") + } + if staleTimer != int64(policyBGPGracefulRestartStaleRouteTimerDefault) { + return &routeStruct, fmt.Errorf(vrfError, "graceful_restart_stale_route_timer") + } + if restartMode != model.BgpGracefulRestartConfig_MODE_HELPER_ONLY { + return &routeStruct, fmt.Errorf(vrfError, "graceful_restart_mode") + } + } else { + restartTimerStruct := model.BgpGracefulRestartTimer{ + RestartTimer: &restartTimer, + StaleRouteTimer: &staleTimer, + } - routeStruct := model.BgpRoutingConfig{ - Ecmp: &ecmp, - Enabled: &enabled, - RouteAggregations: aggregationStructs, - Tags: tags, - InterSrIbgp: &interSrIbgp, - LocalAsNum: &localAsNum, - MultipathRelax: &multipathRelax, - GracefulRestartConfig: &restartConfigStruct, + restartConfigStruct := model.BgpGracefulRestartConfig{ + Mode: &restartMode, + Timer: &restartTimerStruct, + } + + routeStruct.Tags = tags + routeStruct.InterSrIbgp = &interSrIbgp + routeStruct.LocalAsNum = &localAsNum + routeStruct.MultipathRelax = &multipathRelax + routeStruct.GracefulRestartConfig = &restartConfigStruct } return &routeStruct, nil @@ -132,7 +147,11 @@ func resourceNsxtPolicyBgpConfigCreate(d *schema.ResourceData, m interface{}) er } sitePath := d.Get("site_path").(string) - obj, err := resourceNsxtPolicyBgpConfigToStruct(d) + isVrf, err := resourceNsxtPolicyTier0GatewayIsVrf(gwID, connector, isPolicyGlobalManager(m)) + if err != nil { + return handleCreateError("BgpRoutingConfig", gwID, err) + } + obj, err := resourceNsxtPolicyBgpConfigToStruct(d, isVrf) if err != nil { return handleCreateError("BgpRoutingConfig", gwID, err) } @@ -183,7 +202,12 @@ func resourceNsxtPolicyBgpConfigUpdate(d *schema.ResourceData, m interface{}) er _, gwID := parseGatewayPolicyPath(gwPath) serviceID := d.Get("locale_service_id").(string) - obj, err := resourceNsxtPolicyBgpConfigToStruct(d) + isVrf, err := resourceNsxtPolicyTier0GatewayIsVrf(gwID, connector, isPolicyGlobalManager(m)) + if err != nil { + return handleCreateError("BgpRoutingConfig", gwID, err) + } + + obj, err := resourceNsxtPolicyBgpConfigToStruct(d, isVrf) if err != nil { return handleUpdateError("BgpRoutingConfig", gwID, err) } diff --git a/nsxt/resource_nsxt_policy_tier0_gateway.go b/nsxt/resource_nsxt_policy_tier0_gateway.go index 1a76bcf9f..305974375 100644 --- a/nsxt/resource_nsxt_policy_tier0_gateway.go +++ b/nsxt/resource_nsxt_policy_tier0_gateway.go @@ -357,7 +357,7 @@ func getPolicyTier0GatewayLocaleServiceWithEdgeCluster(gwID string, connector *c return &objInList, nil } - return nil, nil + return nil, fmt.Errorf("No locale services found for GW %v", gwID) } func initPolicyTier0BGPConfigMap(bgpConfig *model.BgpRoutingConfig) map[string]interface{} { @@ -525,6 +525,32 @@ func resourceNsxtPolicyTier0GatewayExists(id string, connector *client.RestConne return false, logAPIError("Error retrieving Tier0", err) } +func resourceNsxtPolicyTier0GatewayIsVrf(id string, connector *client.RestConnector, isGlobalManager bool) (bool, error) { + if isGlobalManager { + client := gm_infra.NewTier0sClient(connector) + obj, err := client.Get(id) + + if err == nil { + return obj.VrfConfig != nil, nil + } + + return false, err + } + + client := infra.NewTier0sClient(connector) + obj, err := client.Get(id) + + if err == nil { + return obj.VrfConfig != nil, nil + } + + if isNotFoundError(err) { + return false, nil + } + + return false, logAPIError("Error retrieving Tier0", err) +} + func resourceNsxtPolicyTier0GatewayBGPConfigSchemaToStruct(cfg interface{}, isVrf bool, gwID string) model.BgpRoutingConfig { cfgMap := cfg.(map[string]interface{}) revision := int64(cfgMap["revision"].(int)) diff --git a/nsxt/resource_nsxt_policy_tier0_gateway_test.go b/nsxt/resource_nsxt_policy_tier0_gateway_test.go index 73714d66e..e4f14b68c 100644 --- a/nsxt/resource_nsxt_policy_tier0_gateway_test.go +++ b/nsxt/resource_nsxt_policy_tier0_gateway_test.go @@ -363,7 +363,7 @@ func TestAccResourceNsxtPolicyTier0Gateway_withVRF(t *testing.T) { }, Steps: []resource.TestStep{ { - Config: testAccNsxtPolicyTier0WithVRFTemplate(name, true, true), + Config: testAccNsxtPolicyTier0WithVRFTemplate(name, true, true, true), Check: resource.ComposeTestCheckFunc( testAccNsxtPolicyTier0Exists(testResourceName), resource.TestCheckResourceAttr(testResourceName, "display_name", name), @@ -381,7 +381,7 @@ func TestAccResourceNsxtPolicyTier0Gateway_withVRF(t *testing.T) { ), }, { - Config: testAccNsxtPolicyTier0WithVRFTemplate(updateName, false, false), + Config: testAccNsxtPolicyTier0WithVRFTemplate(updateName, false, false, false), Check: resource.ComposeTestCheckFunc( testAccNsxtPolicyTier0Exists(testResourceName), resource.TestCheckResourceAttr(testResourceName, "display_name", updateName), @@ -689,8 +689,7 @@ data "nsxt_policy_realization_info" "realization_info" { }`, name) } -// TODO: add vrf_config tags when bug 2557096 is resolved -func testAccNsxtPolicyTier0WithVRFTemplate(name string, targets bool, rdAdmin bool) string { +func testAccNsxtPolicyTier0WithVRFTemplate(name string, targets bool, rdAdmin bool, withBGP bool) string { var routeTargets string if targets { @@ -706,6 +705,16 @@ func testAccNsxtPolicyTier0WithVRFTemplate(name string, targets bool, rdAdmin bo if rdAdmin { rdAdminAddress = `rd_admin_address = "192.168.0.2"` } + + var bgpConfig string + if withBGP { + bgpConfig = ` +resource "nsxt_policy_bgp_config" "test" { + gateway_path = nsxt_policy_tier0_gateway.test.path + enabled = true + ecmp = true +}` + } return testAccNsxtPolicyGatewayInterfaceDeps("11, 12") + fmt.Sprintf(` resource "nsxt_policy_tier0_gateway" "parent" { nsx_id = "vrf-parent" @@ -746,9 +755,11 @@ resource "nsxt_policy_tier0_gateway_interface" "test" { access_vlan_id = 12 } +%s + data "nsxt_policy_realization_info" "realization_info" { path = nsxt_policy_tier0_gateway.test.path -}`, name, routeTargets, rdAdminAddress, name) +}`, name, routeTargets, rdAdminAddress, name, bgpConfig) } func testAccNsxtPolicyTier0WithVRFTearDown() string {