From be785e356abf6b4c8ced8a9083ff772a935e2851 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 16 Apr 2021 11:34:30 -0400 Subject: [PATCH] bgpd, tests: Add code to handle failed installations Currently the Wait for Install code ( bgp_suppress_fib ) does not properly handle two states from zebra: ROUTE_INSTALL_FAILED and BETTER_ADMIN_DISTANCE_WON. Pre this change the WFI code would just never notify our peers about a route install failure but more is needed. In the ROUTE_INSTALL_FAILED and the BETTER_ADMIN_DISTANCE_WON we need to notify our peers with a withdrawal about the route, else we will continue to draw traffic to us when we cannot legally do so. Why is this needed? In either case imagine that we've already received a bgp route, installed it and sent to our peers. In the Better admin distance won case, say a static route is installed at this point in time we must stop advertising the route through us since we are not installed. As such a withdrawal must be sent. In the ROUTE_INSTALL_FAILED case, the code was not properly handling the situation where we have Route A, it was successfully installed and then we received a update to Route A that was attempted to be installed but failed. In this case we also need to send a withdrawal Finally update the bgp_suppress_fib topotest to test both of these situations. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 22 +++- bgpd/bgp_route.h | 27 +++++ bgpd/bgp_updgrp_adv.c | 20 +++- bgpd/bgp_zebra.c | 65 ++++++----- .../bgp_suppress_fib/r1/bgp_ipv4_allowas.json | 40 +++++++ .../bgp_suppress_fib/r2/bgp_ipv4_allowas.json | 68 +++++++++++ .../bgp_suppress_fib/r2/bgpd.allowas_in.conf | 18 +++ tests/topotests/bgp_suppress_fib/r2/bgpd.conf | 3 + .../r2/no_bgp_ipv4_allowas.json | 1 + .../bgp_suppress_fib/r2/v4_override.json | 20 ++++ .../bgp_suppress_fib/r3/v4_override.json | 4 + .../bgp_suppress_fib/test_bgp_suppress_fib.py | 110 ++++++++++++++++++ 12 files changed, 363 insertions(+), 35 deletions(-) create mode 100644 tests/topotests/bgp_suppress_fib/r1/bgp_ipv4_allowas.json create mode 100644 tests/topotests/bgp_suppress_fib/r2/bgp_ipv4_allowas.json create mode 100644 tests/topotests/bgp_suppress_fib/r2/bgpd.allowas_in.conf create mode 100644 tests/topotests/bgp_suppress_fib/r2/no_bgp_ipv4_allowas.json create mode 100644 tests/topotests/bgp_suppress_fib/r2/v4_override.json create mode 100644 tests/topotests/bgp_suppress_fib/r3/v4_override.json diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index cd9893fd91..7ecd3c29e2 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -2627,9 +2627,14 @@ void subgroup_process_announce_selected(struct update_subgroup *subgrp, /* Route is selected, if the route is already installed * in FIB, then it is advertised */ - if (advertise) - bgp_adj_out_set_subgroup(dest, subgrp, &attr, - selected); + if (advertise) { + if (!bgp_check_withdrawal(bgp, dest)) + bgp_adj_out_set_subgroup( + dest, subgrp, &attr, selected); + else + bgp_adj_out_unset_subgroup( + dest, subgrp, 1, addpath_tx_id); + } } else bgp_adj_out_unset_subgroup(dest, subgrp, 1, addpath_tx_id); @@ -2908,6 +2913,11 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, if (bgp_fibupd_safi(safi) && !bgp_option_check(BGP_OPT_NO_FIB)) { + if (BGP_SUPPRESS_FIB_ENABLED(bgp) + && new_select->sub_type == BGP_ROUTE_NORMAL) + SET_FLAG(dest->flags, + BGP_NODE_FIB_INSTALL_PENDING); + if (new_select->type == ZEBRA_ROUTE_BGP && (new_select->sub_type == BGP_ROUTE_NORMAL || new_select->sub_type @@ -3007,11 +3017,16 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, /* FIB update. */ if (bgp_fibupd_safi(safi) && (bgp->inst_type != BGP_INSTANCE_TYPE_VIEW) && !bgp_option_check(BGP_OPT_NO_FIB)) { + if (new_select && new_select->type == ZEBRA_ROUTE_BGP && (new_select->sub_type == BGP_ROUTE_NORMAL || new_select->sub_type == BGP_ROUTE_AGGREGATE || new_select->sub_type == BGP_ROUTE_IMPORTED)) { + if (BGP_SUPPRESS_FIB_ENABLED(bgp)) + SET_FLAG(dest->flags, + BGP_NODE_FIB_INSTALL_PENDING); + /* if this is an evpn imported type-5 prefix, * we need to withdraw the route first to clear * the nh neigh and the RMAC entry. @@ -8336,6 +8351,7 @@ void bgp_redistribute_add(struct bgp *bgp, struct prefix *p, bgp_aggregate_increment(bgp, p, new, afi, SAFI_UNICAST); bgp_path_info_add(bn, new); bgp_dest_unlock_node(bn); + SET_FLAG(bn->flags, BGP_NODE_FIB_INSTALLED); bgp_process(bgp, bn, afi, SAFI_UNICAST); if ((bgp->inst_type == BGP_INSTANCE_TYPE_VRF) diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h index 4cc56c8649..3e957630d8 100644 --- a/bgpd/bgp_route.h +++ b/bgpd/bgp_route.h @@ -587,6 +587,33 @@ static inline bool bgp_check_advertise(struct bgp *bgp, struct bgp_dest *dest) (!bgp_option_check(BGP_OPT_NO_FIB)))); } +/* + * If we have a fib result and it failed to install( or was withdrawn due + * to better admin distance we need to send down the wire a withdrawal. + * This function assumes that bgp_check_advertise was already returned + * as good to go. + */ +static inline bool bgp_check_withdrawal(struct bgp *bgp, struct bgp_dest *dest) +{ + struct bgp_path_info *pi; + + 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)) + continue; + + if (pi->sub_type != BGP_ROUTE_NORMAL) + return true; + } + + if (CHECK_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED)) + return false; + + return true; +} + /* called before bgp_process() */ DECLARE_HOOK(bgp_process, (struct bgp * bgp, afi_t afi, safi_t safi, struct bgp_dest *bn, diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index e43ae5c13f..c6ddb1c1a7 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -686,11 +686,21 @@ void subgroup_announce_table(struct update_subgroup *subgrp, dest_p, &attr, false)) { /* Check if route can be advertised */ - if (advertise) - bgp_adj_out_set_subgroup(dest, - subgrp, - &attr, - ri); + if (advertise) { + if (!bgp_check_withdrawal(bgp, + dest)) + bgp_adj_out_set_subgroup( + dest, subgrp, + &attr, ri); + else + bgp_adj_out_unset_subgroup( + dest, subgrp, 1, + bgp_addpath_id_for_peer( + peer, + afi, + safi, + &ri->tx_addpath)); + } } else { /* If default originate is enabled for * the peer, do not send explicit diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c index bbb7d5469d..de25aa30d9 100644 --- a/bgpd/bgp_zebra.c +++ b/bgpd/bgp_zebra.c @@ -2528,34 +2528,26 @@ static int bgp_zebra_route_notify_owner(int command, struct zclient *zclient, case ZAPI_ROUTE_INSTALLED: new_select = NULL; /* Clear the flags so that route can be processed */ - if (CHECK_FLAG(dest->flags, - BGP_NODE_FIB_INSTALL_PENDING)) { - UNSET_FLAG(dest->flags, - BGP_NODE_FIB_INSTALL_PENDING); - SET_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED); - if (BGP_DEBUG(zebra, ZEBRA)) - zlog_debug("route %pRN : INSTALLED", dest); - /* Find the best route */ - for (pi = dest->info; pi; pi = pi->next) { - /* Process aggregate route */ - bgp_aggregate_increment(bgp, &p, pi, - afi, safi); - if (CHECK_FLAG(pi->flags, - BGP_PATH_SELECTED)) - new_select = pi; - } - /* Advertise the route */ - if (new_select) - group_announce_route(bgp, afi, safi, - dest, new_select); - else { - flog_err(EC_BGP_INVALID_ROUTE, - "selected route %pRN not found", - dest); + UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING); + SET_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED); + if (BGP_DEBUG(zebra, ZEBRA)) + zlog_debug("route %pRN : INSTALLED", dest); + /* Find the best route */ + for (pi = dest->info; pi; pi = pi->next) { + /* Process aggregate route */ + bgp_aggregate_increment(bgp, &p, pi, afi, safi); + if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) + new_select = pi; + } + /* Advertise the route */ + if (new_select) + group_announce_route(bgp, afi, safi, dest, new_select); + else { + flog_err(EC_BGP_INVALID_ROUTE, + "selected route %pRN not found", dest); - bgp_dest_unlock_node(dest); - return -1; - } + bgp_dest_unlock_node(dest); + return -1; } break; case ZAPI_ROUTE_REMOVED: @@ -2568,15 +2560,34 @@ static int bgp_zebra_route_notify_owner(int command, struct zclient *zclient, zlog_debug("route %pRN: Removed from Fib", dest); break; case ZAPI_ROUTE_FAIL_INSTALL: + new_select = NULL; if (BGP_DEBUG(zebra, ZEBRA)) zlog_debug("route: %pRN Failed to Install into Fib", dest); + UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING); + UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED); + for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) { + if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) + new_select = pi; + } + if (new_select) + group_announce_route(bgp, afi, safi, dest, new_select); /* Error will be logged by zebra module */ break; case ZAPI_ROUTE_BETTER_ADMIN_WON: if (BGP_DEBUG(zebra, ZEBRA)) zlog_debug("route: %pRN removed due to better admin won", dest); + new_select = NULL; + UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALL_PENDING); + UNSET_FLAG(dest->flags, BGP_NODE_FIB_INSTALLED); + for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next) { + bgp_aggregate_decrement(bgp, &p, pi, afi, safi); + if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED)) + new_select = pi; + } + if (new_select) + group_announce_route(bgp, afi, safi, dest, new_select); /* No action required */ break; case ZAPI_ROUTE_REMOVE_FAIL: diff --git a/tests/topotests/bgp_suppress_fib/r1/bgp_ipv4_allowas.json b/tests/topotests/bgp_suppress_fib/r1/bgp_ipv4_allowas.json new file mode 100644 index 0000000000..bc4d0f4796 --- /dev/null +++ b/tests/topotests/bgp_suppress_fib/r1/bgp_ipv4_allowas.json @@ -0,0 +1,40 @@ +{ + "prefix":"192.168.1.1/32", + "paths":[ + { + "aspath":{ + "string":"2", + "segments":[ + { + "type":"as-sequence", + "list":[ + 2 + ] + } + ], + "length":1 + }, + "origin":"incomplete", + "metric":0, + "valid":true, + "bestpath":{ + "overall":true, + "selectionReason":"First path received" + }, + "nexthops":[ + { + "ip":"10.0.0.2", + "afi":"ipv4", + "metric":0, + "accessible":true, + "used":true + } + ], + "peer":{ + "peerId":"10.0.0.2", + "routerId":"10.0.0.9", + "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 new file mode 100644 index 0000000000..16561ce837 --- /dev/null +++ b/tests/topotests/bgp_suppress_fib/r2/bgp_ipv4_allowas.json @@ -0,0 +1,68 @@ +{ + "prefix":"192.168.1.1/32", + "paths":[ + { + "aspath":{ + "string":"1 2", + "segments":[ + { + "type":"as-sequence", + "list":[ + 1, + 2 + ] + } + ], + "length":2 + }, + "origin":"incomplete", + "valid":true, + "fibInstalled":true, + "nexthops":[ + { + "ip":"10.0.0.1", + "afi":"ipv4", + "metric":0, + "accessible":true, + "used":true + } + ], + "peer":{ + "peerId":"10.0.0.1", + "routerId":"10.0.0.1", + "type":"external" + } + }, + { + "aspath":{ + "string":"Local", + "segments":[ + ], + "length":0 + }, + "origin":"incomplete", + "metric":0, + "weight":32768, + "valid":true, + "sourced":true, + "bestpath":{ + "overall":true, + "selectionReason":"Weight" + }, + "fibInstalled":true, + "nexthops":[ + { + "ip":"10.0.0.10", + "afi":"ipv4", + "metric":0, + "accessible":true, + "used":true + } + ], + "peer":{ + "peerId":"0.0.0.0", + "routerId":"10.0.0.9" + } + } + ] +} diff --git a/tests/topotests/bgp_suppress_fib/r2/bgpd.allowas_in.conf b/tests/topotests/bgp_suppress_fib/r2/bgpd.allowas_in.conf new file mode 100644 index 0000000000..caebb0e922 --- /dev/null +++ b/tests/topotests/bgp_suppress_fib/r2/bgpd.allowas_in.conf @@ -0,0 +1,18 @@ +access-list access seq 10 permit 192.168.1.1/32 +! +ip route 192.168.1.1/32 10.0.0.10 +! +debug bgp bestpath +debug bgp nht +debug bgp updates +debug bgp update-groups +debug bgp zebra +debug zebra rib detail +! +router bgp 2 + address-family ipv4 uni + redistribute static + neighbor 10.0.0.10 allowas-in 1 + neighbor 10.0.0.1 allowas-in 1 + ! +! diff --git a/tests/topotests/bgp_suppress_fib/r2/bgpd.conf b/tests/topotests/bgp_suppress_fib/r2/bgpd.conf index 8321c915e3..ebef2012a8 100644 --- a/tests/topotests/bgp_suppress_fib/r2/bgpd.conf +++ b/tests/topotests/bgp_suppress_fib/r2/bgpd.conf @@ -1,3 +1,6 @@ +debug bgp updates +debug bgp bestpath 40.0.0.0/8 +debug bgp zebra ! router bgp 2 no bgp ebgp-requires-policy diff --git a/tests/topotests/bgp_suppress_fib/r2/no_bgp_ipv4_allowas.json b/tests/topotests/bgp_suppress_fib/r2/no_bgp_ipv4_allowas.json new file mode 100644 index 0000000000..0967ef424b --- /dev/null +++ b/tests/topotests/bgp_suppress_fib/r2/no_bgp_ipv4_allowas.json @@ -0,0 +1 @@ +{} diff --git a/tests/topotests/bgp_suppress_fib/r2/v4_override.json b/tests/topotests/bgp_suppress_fib/r2/v4_override.json new file mode 100644 index 0000000000..f17907f011 --- /dev/null +++ b/tests/topotests/bgp_suppress_fib/r2/v4_override.json @@ -0,0 +1,20 @@ +{ + "40.0.0.0\/8":[ + { + "prefix":"40.0.0.0\/8", + "protocol":"static", + "vrfName":"default", + "selected":true, + "destSelected":true, + "installed":true, + "nexthops":[ + { + "fib":true, + "ip":"10.0.0.10", + "afi":"ipv4", + "active":true + } + ] + } + ] +} diff --git a/tests/topotests/bgp_suppress_fib/r3/v4_override.json b/tests/topotests/bgp_suppress_fib/r3/v4_override.json new file mode 100644 index 0000000000..a35d49e9e8 --- /dev/null +++ b/tests/topotests/bgp_suppress_fib/r3/v4_override.json @@ -0,0 +1,4 @@ +{ + "0.0.0.0\/0":[ + ] +} 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 5a22fbbc54..2c87d9d7b0 100644 --- a/tests/topotests/bgp_suppress_fib/test_bgp_suppress_fib.py +++ b/tests/topotests/bgp_suppress_fib/test_bgp_suppress_fib.py @@ -29,6 +29,7 @@ import json import pytest from functools import partial from time import sleep +from lib.topolog import logger CWD = os.path.dirname(os.path.realpath(__file__)) sys.path.append(os.path.join(CWD, "../")) @@ -112,6 +113,115 @@ def test_bgp_route(): assert result is None, assertmsg +def test_bgp_better_admin_won(): + "A better Admin distance protocol may come along and knock us out" + + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r2 = tgen.gears["r2"] + r2.vtysh_cmd("conf\nip route 40.0.0.0/8 10.0.0.10") + + json_file = "{}/r2/v4_override.json".format(CWD) + expected = json.loads(open(json_file).read()) + + logger.info(expected) + test_func = partial( + topotest.router_json_cmp, r2, "show ip route 40.0.0.0 json", expected + ) + + _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assertmsg = '"r2" static route did not take over' + assert result is None, assertmsg + + r3 = tgen.gears["r3"] + + json_file = "{}/r3/v4_override.json".format(CWD) + expected = json.loads(open(json_file).read()) + + test_func = partial( + topotest.router_json_cmp, r3, "show ip route 40.0.0.0 json", expected + ) + + _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assertmsg = '"r3" route to 40.0.0.0 should have been lost' + assert result is None, assertmsg + + r2.vtysh_cmd("conf\nno ip route 40.0.0.0/8 10.0.0.10") + + json_file = "{}/r3/v4_route.json".format(CWD) + expected = json.loads(open(json_file).read()) + + test_func = partial( + topotest.router_json_cmp, + r3, + "show ip route 40.0.0.0 json", + expected, + ) + _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assertmsg = '"r3" route to 40.0.0.0 did not come back' + assert result is None, assertmsg + + +def test_bgp_allow_as_in(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r2 = tgen.gears["r2"] + + config_file = "{}/r2/bgpd.allowas_in.conf".format(CWD) + r2.run("vtysh -f {}".format(config_file)) + + json_file = "{}/r2/bgp_ipv4_allowas.json".format(CWD) + expected = json.loads(open(json_file).read()) + + test_func = partial( + topotest.router_json_cmp, + r2, + "show bgp ipv4 uni 192.168.1.1/32 json", + expected, + ) + _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assertmsg = '"r2" static redistribution failed into bgp' + assert result is None, assertmsg + + r1 = tgen.gears["r1"] + + json_file = "{}/r1/bgp_ipv4_allowas.json".format(CWD) + expected = json.loads(open(json_file).read()) + + test_func = partial( + topotest.router_json_cmp, + r1, + "show bgp ipv4 uni 192.168.1.1/32 json", + expected, + ) + + _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assertmsg = '"r1" 192.168.1.1/32 route should have arrived' + assert result is None, assertmsg + + r2.vtysh_cmd("conf\nno ip route 192.168.1.1/32 10.0.0.10") + + json_file = "{}/r2/no_bgp_ipv4_allowas.json".format(CWD) + expected = json.loads(open(json_file).read()) + + test_func = partial( + topotest.router_json_cmp, + r2, + "show bgp ipv4 uni 192.168.1.1/32 json", + expected, + ) + + _, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assertmsg = '"r2" 192.168.1.1/32 route should be gone' + assert result is None, assertmsg + + if __name__ == "__main__": args = ["-s"] + sys.argv[1:] sys.exit(pytest.main(args))