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))