Merge pull request #7922 from ton31337/fix/bgpd_blackhole_community_ibgp

bgpd: Advertise BLACKHOLE community tagged prefixes to iBGP peers

6 files changed, 65 insertions, 14 deletions

diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c
index 6735c1a952..e6276d060e 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;
 	}
 
@@ -3505,18 +3506,20 @@ 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");
+
+	assert(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);
@@ -3524,10 +3527,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);
@@ -3737,7 +3740,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 */
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 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, 'Advertised blackhole tagged prefix in "{}"'.format(
+    assert (
+        result is None
+    ), 'Withdrawn blackhole tagged prefix to iBGP peers in "{}"'.format(
         tgen.gears["r2"]
     )