bgpd: Allow network XXX to work with bgp suppress-fib-pending

donaldsharp · donaldsharp · commit 4801fc467002 · 2022-10-12T14:56:27.000-04:00
When bgp is using `bgp suppress-fib-pending` and the end operator is using network statements, bgp was not sending the network'ed prefix'es to it's peers. Fix this. Also update the test cases for bgp_suppress_fib to test this new corner case( I am sure that there are going to be others that will need to be added ). Fixes: #12112 Signed-off-by: Donald Sharp <sharpd@nvidia.com>
diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
@@ -608,19 +608,35 @@ static inline bool bgp_check_advertise(struct bgp *bgp, struct bgp_dest *dest)
  */
 static inline bool bgp_check_withdrawal(struct bgp *bgp, struct bgp_dest *dest)
 {
-	struct bgp_path_info *pi;
+	struct bgp_path_info *pi, *selected = NULL;
 
 	if (!BGP_SUPPRESS_FIB_ENABLED(bgp))
 		return false;
 
 	for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) {
-		if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED))
+		if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) {
+			selected = pi;
 			continue;
+		}
 
 		if (pi->sub_type != BGP_ROUTE_NORMAL)
 			return true;
 	}
 
+	/*
+	 * pi is selected and bgp is dealing with a static route
+	 * ( ie a network statement of some sort ).  FIB installed
+	 * is irrelevant
+	 *
+	 * I am not sure what the above for loop is wanted in this
+	 * manner at this point.  But I do know that if I have
+	 * a static route that is selected and it's the one
+	 * being checked for should I withdrawal we do not
+	 * want to withdraw the route on installation :)
+	 */
+	if (selected && selected->sub_type == BGP_ROUTE_STATIC)
+		return false;
+
 	if (CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED))
 		return false;
 
diff --git a/tests/topotests/bgp_suppress_fib/r1/bgp_ipv4_allowas.json b/tests/topotests/bgp_suppress_fib/r1/bgp_ipv4_allowas.json
@@ -32,7 +32,7 @@
       ],
       "peer":{
         "peerId":"10.0.0.2",
-        "routerId":"10.0.0.9",
+        "routerId":"60.0.0.1",
         "type":"external"
       }
     }
diff --git a/tests/topotests/bgp_suppress_fib/r2/bgp_ipv4_allowas.json b/tests/topotests/bgp_suppress_fib/r2/bgp_ipv4_allowas.json
@@ -61,7 +61,7 @@
       ],
       "peer":{
         "peerId":"0.0.0.0",
-        "routerId":"10.0.0.9"
+        "routerId":"60.0.0.1"
       }
     }
   ]
diff --git a/tests/topotests/bgp_suppress_fib/r2/bgpd.conf b/tests/topotests/bgp_suppress_fib/r2/bgpd.conf
@@ -7,3 +7,5 @@ router bgp 2
   bgp suppress-fib-pending
   neighbor 10.0.0.1 remote-as 1
   neighbor 10.0.0.10 remote-as 3
+  address-family ipv4 uni
+     network 60.0.0.0/24
diff --git a/tests/topotests/bgp_suppress_fib/r2/zebra.conf b/tests/topotests/bgp_suppress_fib/r2/zebra.conf
@@ -1,4 +1,7 @@
 !
+interface lo
+ ip address 60.0.0.1/24
+!
 interface r2-eth0
  ip address 10.0.0.2/30
 !
diff --git a/tests/topotests/bgp_suppress_fib/r3/v4_route3.json b/tests/topotests/bgp_suppress_fib/r3/v4_route3.json
@@ -0,0 +1,23 @@
+{
+  "60.0.0.0/24":[
+    {
+      "prefix":"60.0.0.0/24",
+      "protocol":"bgp",
+      "selected":true,
+      "destSelected":true,
+      "distance":20,
+      "metric":0,
+      "installed":true,
+      "table":254,
+      "nexthops":[
+        {
+          "fib":true,
+          "ip":"10.0.0.9",
+          "afi":"ipv4",
+          "interfaceName":"r3-eth0",
+          "active":true
+        }
+      ]
+    }
+  ]
+}
diff --git a/tests/topotests/bgp_suppress_fib/test_bgp_suppress_fib.py b/tests/topotests/bgp_suppress_fib/test_bgp_suppress_fib.py
@@ -84,8 +84,6 @@ def test_bgp_route():
 
     r3 = tgen.gears["r3"]
 
-    sleep(5)
-
     json_file = "{}/r3/v4_route.json".format(CWD)
     expected = json.loads(open(json_file).read())
 
@@ -95,7 +93,7 @@ def test_bgp_route():
         "show ip route 40.0.0.0 json",
         expected,
     )
-    _, result = topotest.run_and_expect(test_func, None, count=2, wait=0.5)
+    _, result = topotest.run_and_expect(test_func, None, count=20, wait=0.5)
     assertmsg = '"r3" JSON output mismatches'
     assert result is None, assertmsg
 
@@ -112,6 +110,16 @@ def test_bgp_route():
     assertmsg = '"r3" JSON output mismatches'
     assert result is None, assertmsg
 
+    json_file = "{}/r3/v4_route3.json".format(CWD)
+    expected = json.loads(open(json_file).read())
+
+    test_func = partial(
+        topotest.router_json_cmp,
+        r3,
+        "show ip route 10.0.0.3 json",
+        expected,
+        )
+    _, result = topotest.run_and_expect(test_func, None, count=3, wait=0.5)
 
 def test_bgp_better_admin_won():
     "A better Admin distance protocol may come along and knock us out"

Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@`
`32`	`32`	`],`
`33`	`33`	`"peer":{`
`34`	`34`	`"peerId":"10.0.0.2",`
`35`		`- "routerId":"10.0.0.9",`
	`35`	`+ "routerId":"60.0.0.1",`
`36`	`36`	`"type":"external"`
`37`	`37`	`}`
`38`	`38`	`}`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@`
`61`	`61`	`],`
`62`	`62`	`"peer":{`
`63`	`63`	`"peerId":"0.0.0.0",`
`64`		`- "routerId":"10.0.0.9"`
	`64`	`+ "routerId":"60.0.0.1"`
`65`	`65`	`}`
`66`	`66`	`}`
`67`	`67`	`]`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,7 @@`
`1`	`1`	`!`
	`2`	`+interface lo`
	`3`	`+ ip address 60.0.0.1/24`
	`4`	`+!`
`2`	`5`	`interface r2-eth0`
`3`	`6`	`ip address 10.0.0.2/30`
`4`	`7`	`!`