From 9e2912897d57056d4b6ec8899514e8826e1ee790 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Sun, 24 Jan 2021 10:48:41 +0200 Subject: [PATCH 1/4] bgpd: Log prefix when community filter fails This is needed when NO_ADVERTISE or NO_EXPORT is handled for outgoing updates. Signed-off-by: Donatas Abraitis --- bgpd/bgp_route.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 6735c1a952..2a3fdc65c9 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -1836,7 +1836,8 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi, /* If community is not disabled check the no-export and local. */ if (!transparent && bgp_community_filter(peer, piattr)) { if (bgp_debug_update(NULL, p, subgrp->update_group, 0)) - zlog_debug("%s: community filter check fail", __func__); + zlog_debug("%s: community filter check fail for %pFX", + __func__, p); return false; } From aade37d7274d4fbbe073811a91a35a132c40de60 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Sun, 24 Jan 2021 10:54:50 +0200 Subject: [PATCH 2/4] bgpd: Set no-export community for blackhole tagged prefixes RFC says to prevent propagation of the prefix outside the local AS. So, let's use NO_EXPORT. Signed-off-by: Donatas Abraitis --- bgpd/bgp_route.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 2a3fdc65c9..f7945ce140 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -3506,18 +3506,18 @@ bool bgp_update_martian_nexthop(struct bgp *bgp, afi_t afi, safi_t safi, return ret; } -static void bgp_attr_add_no_advertise_community(struct attr *attr) +static void bgp_attr_add_no_export_community(struct attr *attr) { struct community *old; struct community *new; struct community *merge; - struct community *noadv; + struct community *no_export; old = attr->community; - noadv = community_str2com("no-advertise"); + no_export = community_str2com("no-export"); if (old) { - merge = community_merge(community_dup(old), noadv); + merge = community_merge(community_dup(old), no_export); if (!old->refcnt) community_free(&old); @@ -3525,10 +3525,10 @@ static void bgp_attr_add_no_advertise_community(struct attr *attr) new = community_uniq_sort(merge); community_free(&merge); } else { - new = community_dup(noadv); + new = community_dup(no_export); } - community_free(&noadv); + community_free(&no_export); attr->community = new; attr->flag |= ATTR_FLAG_BIT(BGP_ATTR_COMMUNITIES); @@ -3738,7 +3738,7 @@ int bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id, if (new_attr.community && community_include(new_attr.community, COMMUNITY_BLACKHOLE)) - bgp_attr_add_no_advertise_community(&new_attr); + bgp_attr_add_no_export_community(&new_attr); /* If we receive the graceful-shutdown community from an eBGP * peer we must lower local-preference */ From a5de4a566d6f2732e2156bc5a51685d2824e875f Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Sun, 24 Jan 2021 10:59:27 +0200 Subject: [PATCH 3/4] tests: Check if BLACKHOLE community prefixes are visible inside local AS Signed-off-by: Donatas Abraitis --- .../bgp_blackhole_community/r2/bgpd.conf | 1 + .../bgp_blackhole_community/r2/zebra.conf | 3 ++ .../bgp_blackhole_community/r4/bgpd.conf | 5 +++ .../bgp_blackhole_community/r4/zebra.conf | 6 +++ .../test_bgp_blackhole_community.py | 45 ++++++++++++++++--- 5 files changed, 54 insertions(+), 6 deletions(-) create mode 100644 tests/topotests/bgp_blackhole_community/r4/bgpd.conf create mode 100644 tests/topotests/bgp_blackhole_community/r4/zebra.conf diff --git a/tests/topotests/bgp_blackhole_community/r2/bgpd.conf b/tests/topotests/bgp_blackhole_community/r2/bgpd.conf index a4fb45e1ff..5a69b99810 100644 --- a/tests/topotests/bgp_blackhole_community/r2/bgpd.conf +++ b/tests/topotests/bgp_blackhole_community/r2/bgpd.conf @@ -3,4 +3,5 @@ router bgp 65002 no bgp ebgp-requires-policy neighbor r2-eth0 interface remote-as external neighbor r2-eth1 interface remote-as external + neighbor r2-eth2 interface remote-as internal ! diff --git a/tests/topotests/bgp_blackhole_community/r2/zebra.conf b/tests/topotests/bgp_blackhole_community/r2/zebra.conf index 307e5187ca..cf6fb6d984 100644 --- a/tests/topotests/bgp_blackhole_community/r2/zebra.conf +++ b/tests/topotests/bgp_blackhole_community/r2/zebra.conf @@ -5,5 +5,8 @@ interface r2-eth0 interface r2-eth1 ip address 192.168.1.1/24 ! +interface r2-eth2 + ip address 192.168.2.1/24 +! ip forwarding ! diff --git a/tests/topotests/bgp_blackhole_community/r4/bgpd.conf b/tests/topotests/bgp_blackhole_community/r4/bgpd.conf new file mode 100644 index 0000000000..40dd72f3ec --- /dev/null +++ b/tests/topotests/bgp_blackhole_community/r4/bgpd.conf @@ -0,0 +1,5 @@ +! +router bgp 65002 + no bgp ebgp-requires-policy + neighbor r4-eth0 interface remote-as internal +! diff --git a/tests/topotests/bgp_blackhole_community/r4/zebra.conf b/tests/topotests/bgp_blackhole_community/r4/zebra.conf new file mode 100644 index 0000000000..e2ccaed52a --- /dev/null +++ b/tests/topotests/bgp_blackhole_community/r4/zebra.conf @@ -0,0 +1,6 @@ +! +interface r4-eth0 + ip address 192.168.2.2/24 +! +ip forwarding +! diff --git a/tests/topotests/bgp_blackhole_community/test_bgp_blackhole_community.py b/tests/topotests/bgp_blackhole_community/test_bgp_blackhole_community.py index b61ad354e2..a856c9278f 100644 --- a/tests/topotests/bgp_blackhole_community/test_bgp_blackhole_community.py +++ b/tests/topotests/bgp_blackhole_community/test_bgp_blackhole_community.py @@ -21,7 +21,7 @@ """ Test if 172.16.255.254/32 tagged with BLACKHOLE community is not -re-advertised downstream. +re-advertised downstream outside local AS. """ import os @@ -38,13 +38,14 @@ from lib import topotest from lib.topogen import Topogen, TopoRouter, get_topogen from lib.topolog import logger from mininet.topo import Topo +from lib.common_config import step class TemplateTopo(Topo): def build(self, *_args, **_opts): tgen = get_topogen(self) - for routern in range(1, 4): + for routern in range(1, 5): tgen.add_router("r{}".format(routern)) switch = tgen.add_switch("s1") @@ -55,6 +56,10 @@ class TemplateTopo(Topo): switch.add_link(tgen.gears["r2"]) switch.add_link(tgen.gears["r3"]) + switch = tgen.add_switch("s3") + switch.add_link(tgen.gears["r2"]) + switch.add_link(tgen.gears["r4"]) + def setup_module(mod): tgen = Topogen(TemplateTopo, mod.__name__) @@ -88,10 +93,10 @@ def test_bgp_blackhole_community(): output = json.loads( tgen.gears["r2"].vtysh_cmd("show ip bgp 172.16.255.254/32 json") ) - expected = {"paths": [{"community": {"list": ["blackhole", "noAdvertise"]}}]} + expected = {"paths": [{"community": {"list": ["blackhole", "noExport"]}}]} return topotest.json_cmp(output, expected) - def _bgp_no_advertise(): + def _bgp_no_advertise_ebgp(): output = json.loads( tgen.gears["r2"].vtysh_cmd( "show ip bgp neighbor r2-eth1 advertised-routes json" @@ -105,15 +110,43 @@ def test_bgp_blackhole_community(): return topotest.json_cmp(output, expected) + def _bgp_no_advertise_ibgp(): + output = json.loads( + tgen.gears["r2"].vtysh_cmd( + "show ip bgp neighbor r2-eth2 advertised-routes json" + ) + ) + expected = { + "advertisedRoutes": {"172.16.255.254/32": {}}, + "totalPrefixCounter": 2, + } + + return topotest.json_cmp(output, expected) + test_func = functools.partial(_bgp_converge) success, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) assert result is None, 'Failed bgp convergence in "{}"'.format(tgen.gears["r2"]) - test_func = functools.partial(_bgp_no_advertise) + step("Check if 172.16.255.254/32 is not advertised to eBGP peers") + + test_func = functools.partial(_bgp_no_advertise_ebgp) success, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) - assert result is None, 'Advertised blackhole tagged prefix in "{}"'.format( + assert ( + result is None + ), 'Advertised blackhole tagged prefix to eBGP peers in "{}"'.format( + tgen.gears["r2"] + ) + + step("Check if 172.16.255.254/32 is advertised to iBGP peers") + + test_func = functools.partial(_bgp_no_advertise_ibgp) + success, result = topotest.run_and_expect(test_func, None, count=60, wait=0.5) + + assert ( + result is None + ), 'Withdrawn blackhole tagged prefix to iBGP peers in "{}"'.format( tgen.gears["r2"] ) From b4efa101a8a8354fe763d828bdd116cff89c60aa Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Mon, 25 Jan 2021 09:51:22 +0200 Subject: [PATCH 4/4] bgpd: Assert that community_str2com("no-export") always returns non-NULL community_str2com("no-export"); returns ALWAYS non-NULL. If NULL returned here, we really have a bigger problems in the call path. Signed-off-by: Donatas Abraitis --- bgpd/bgp_route.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index f7945ce140..e6276d060e 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -3516,6 +3516,8 @@ static void bgp_attr_add_no_export_community(struct attr *attr) old = attr->community; no_export = community_str2com("no-export"); + assert(no_export); + if (old) { merge = community_merge(community_dup(old), no_export);