From 4801fc4670020406fc609dedabc7482d88e3b656 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 12 Oct 2022 14:53:21 -0400 Subject: [PATCH] bgpd: Allow `network XXX` to work with bgp suppress-fib-pending 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 --- bgpd/bgp_route.h | 20 ++++++++++++++-- .../bgp_suppress_fib/r1/bgp_ipv4_allowas.json | 2 +- .../bgp_suppress_fib/r2/bgp_ipv4_allowas.json | 2 +- tests/topotests/bgp_suppress_fib/r2/bgpd.conf | 2 ++ .../topotests/bgp_suppress_fib/r2/zebra.conf | 3 +++ .../bgp_suppress_fib/r3/v4_route3.json | 23 +++++++++++++++++++ .../bgp_suppress_fib/test_bgp_suppress_fib.py | 14 ++++++++--- 7 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 tests/topotests/bgp_suppress_fib/r3/v4_route3.json diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index ddef4ca1bb..1107e3e33e 100644 --- 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 index bc4d0f4796..1a5ede276d 100644 --- 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 index 16561ce837..4a35abfd6f 100644 --- 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 index ebef2012a8..010e86aad7 100644 --- 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 \ No newline at end of file diff --git a/tests/topotests/bgp_suppress_fib/r2/zebra.conf b/tests/topotests/bgp_suppress_fib/r2/zebra.conf index 443fffc703..6e8bce0450 100644 --- 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 new file mode 100644 index 0000000000..ab8c3aa5e5 --- /dev/null +++ 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 index 2c87d9d7b0..96a294cae3 100644 --- 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"