bgpd, topotests: fix validate imported routes only in 'import vrf' cases

The bgp_vpnv4_gre test validates imported routes as ok when route-map is
detached. Whereas, The nexthop of MPLS paths must either be a labelled
nexthop or point to a GRE interface (by using route-map).

The case where this check does not matter is when local importation is
performed by using the 'import vrf' command. Fix this by applying the
automatic validation only when this command is used to import VPN paths.

Enforce the current topotest by really checking the invalidity of the path.
Apply black style to modified topotest.

Fixes: bgpd: ("Validate imported routes next-hop that is in a default VRF")

Signed-off-by: Philippe Guibert <[email protected]>

Author: pguibert6WIND

bgpd/bgp_nht.c

@@ -480,13 +480,19 @@ int bgp_find_or_add_nexthop(struct bgp *bgp_route, struct bgp *bgp_nexthop, afi_
 		path_nh_map(pi, bnc, true);
 
 		bpi_ultimate = bgp_get_imported_bpi_ultimate(pi);
+		if (bpi_ultimate != pi && safi == SAFI_UNICAST &&
+		    (CHECK_FLAG(bgp_route->af_flags[afi][SAFI_UNICAST],
+				BGP_CONFIG_VRF_TO_VRF_IMPORT) ||
+		     CHECK_FLAG(bgp_route->af_flags[afi][SAFI_UNICAST],
+				BGP_CONFIG_VRF_TO_VRF_EXPORT)))
+			/* import does not require any valid MPLS label */
+			SET_FLAG(bnc->flags, BGP_NEXTHOP_ULTIMATE);
+
 		if (CHECK_FLAG(bnc->flags, BGP_NEXTHOP_VALID) && bnc->metric)
 			(bgp_path_info_extra_get(bpi_ultimate))->igpmetric =
 				bnc->metric;
 		else if (bpi_ultimate->extra)
 			bpi_ultimate->extra->igpmetric = 0;
-
-		SET_FLAG(bnc->flags, BGP_NEXTHOP_ULTIMATE);
 	} else if (peer) {
 		/*
 		 * Let's not accidentally save the peer data for a peer

tests/topotests/bgp_vpnv4_gre/test_bgp_vpnv4_gre.py

@@ -9,8 +9,8 @@
 #
 
 """
- test_bgp_vpnv4_gre.py: Test the FRR BGP daemon with BGP IPv6 interface
- with route advertisements on a separate netns.
+test_bgp_vpnv4_gre.py: Test the FRR BGP daemon with BGP IPv6 interface
+with route advertisements on a separate netns.
 """
 
 import os
@@ -27,6 +27,7 @@
 # Import topogen and topotest helpers
 from lib import topotest
 from lib.topogen import Topogen, TopoRouter, get_topogen
+from lib.common_config import retry
 from lib.topolog import logger
 
 # Required to instantiate the topology builder class.
@@ -106,6 +107,19 @@ def teardown_module(_mod):
     tgen.stop_topology()
 
 
+@retry(retry_timeout=10)
+def _check_show_bgp_mpls_not_selected(router, vrf, ipv4prefix):
+    valid = True
+    output = json.loads(router.vtysh_cmd(f"show bgp vrf {vrf} ipv4 {ipv4prefix} json"))
+    paths = output["paths"]
+    for path in paths:
+        if "remoteLabel" in path.keys():
+            valid = path.get("valid", False)
+    if not valid:
+        return True
+    return f"MPLS path to {prefix} in vrf {vrf} not found or considered as valid"
+
+
 def test_protocols_convergence():
     """
     Assert that all protocols have converged
@@ -148,7 +162,7 @@ def test_protocols_convergence():
     assertmsg = '"{}" JSON output mismatches'.format(router.name)
     assert result is None, assertmsg
 
-    # Check BGP IPv4 routing tables on r2 not installed
+    # Check BGP IPv4 convergence on r2
     logger.info("Checking BGP IPv4 routes for convergence on r2")
     router = tgen.gears["r2"]
     json_file = "{}/{}/bgp_ipv4_routes.json".format(CWD, router.name)
@@ -166,6 +180,13 @@ def test_protocols_convergence():
     assertmsg = '"{}" JSON output mismatches'.format(router.name)
     assert result is None, assertmsg
 
+    # Check BGP IPv4 route 10.201.0.0/24 on r2 not installed
+    logger.info("Checking BGP IPv4 route 10.201.0.0/24 for invalidity on r2")
+    success = _check_show_bgp_mpls_not_selected(
+        tgen.gears["r2"], "vrf1", "10.201.0.0/24"
+    )
+    assert success is True, "network 10.201.0.0/24 invalid for MPLS: not found on r2"
+
 
 def test_memory_leak():
     "Run the memory leak test and report results."