bgpd: Allow next hop recursion in zebra if any multipath is not eBGP

When "bgp bestpath peer-type multipath-relax" is enabled, multipaths
with both eBGP and iBGP learned routes may exist. It is not desirable
for the iBGP next hops to be discarded from the FIB because they are not
directly connected. When publishing a nexthop group to zebra, the
ZEBRA_FLAG_ALLOW_RECURSION flag is normally not set when the best path
is eBGP; when "bgp bestpath aspath multipath-relax" is configured, the
flag will now be set if any paths are from iBGP peers. This leaves
all-eBGP multipaths still requiring nexthops over connected routes.

Signed-off-by: Joanne Mikkelson <jmmikkel@arista.com>

diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c
index afdd5123fb5a3255245e919677594d59586df632..932dba8d0d9c01c82e3a0fe5344bd8ae3a35e681 100644 (file)
--- a/bgpd/bgp_zebra.c
+++ b/bgpd/bgp_zebra.c
@@ -1180,6 +1180,7 @@ void bgp_zebra_announce(struct bgp_dest *dest, const struct prefix *p,
        int nh_family;
        unsigned int valid_nh_count = 0;
        int has_valid_label = 0;
+       bool allow_recursion = false;
        uint8_t distance;
        struct peer *peer;
        struct bgp_path_info *mpinfo;
@@ -1257,7 +1258,7 @@ void bgp_zebra_announce(struct bgp_dest *dest, const struct prefix *p,
            || CHECK_FLAG(peer->flags, PEER_FLAG_DISABLE_CONNECTED_CHECK)
            || CHECK_FLAG(bgp->flags, BGP_FLAG_DISABLE_NH_CONNECTED_CHK))
 
-               SET_FLAG(api.flags, ZEBRA_FLAG_ALLOW_RECURSION);
+               allow_recursion = true;
 
        if (info->attr->rmap_table_id) {
                SET_FLAG(api.message, ZAPI_MESSAGE_TABLEID);
@@ -1383,6 +1384,15 @@ void bgp_zebra_announce(struct bgp_dest *dest, const struct prefix *p,
                if (!nh_updated)
                        continue;
 
+               /* Allow recursion if it is a multipath group with both
+                * eBGP and iBGP paths.
+                */
+               if (!allow_recursion
+                   && CHECK_FLAG(bgp->flags, BGP_FLAG_PEERTYPE_MULTIPATH_RELAX)
+                   && (mpinfo->peer->sort == BGP_PEER_IBGP
+                       || mpinfo->peer->sort == BGP_PEER_CONFED))
+                       allow_recursion = true;
+
                if (mpinfo->extra
                    && bgp_is_valid_label(&mpinfo->extra->label[0])
                    && !CHECK_FLAG(api.flags, ZEBRA_FLAG_EVPN_ROUTE)) {
@@ -1411,6 +1421,9 @@ void bgp_zebra_announce(struct bgp_dest *dest, const struct prefix *p,
                memcpy(api.opaque.data, aspath->str, api.opaque.length);
        }
 
+       if (allow_recursion)
+               SET_FLAG(api.flags, ZEBRA_FLAG_ALLOW_RECURSION);
+
        /*
         * When we create an aggregate route we must also
         * install a Null0 route in the RIB, so overwrite

diff --git a/tests/topotests/bgp_peer-type_multipath-relax/test_bgp_peer-type_multipath-relax.py b/tests/topotests/bgp_peer-type_multipath-relax/test_bgp_peer-type_multipath-relax.py
index f1ffcf23bc2167ad01849665524457b62203f2cf..39a0beeb118c483400166bde3647f05544f47e3d 100755 (executable)
--- a/tests/topotests/bgp_peer-type_multipath-relax/test_bgp_peer-type_multipath-relax.py
+++ b/tests/topotests/bgp_peer-type_multipath-relax/test_bgp_peer-type_multipath-relax.py
@@ -28,6 +28,12 @@ Test the effects of the "bgp bestpath peer-type multipath-relax" command
 - enabling the command allows eBGP, iBGP, and confed routes to be multipath
 - the choice of best path is not affected
 - disabling the command removes iBGP/confed routes from multipath
+- enabling the command does not forgive eBGP routes of the requirement
+  (when enabled) that next hops resolve over connected routes
+- a mixed-type multipath next hop, when published to zebra, does not
+  require resolving next hops over connected routes
+- with the command enabled, an all-eBGP multipath next hop still requires
+  resolving next hops over connected routes when published to zebra
 
 Topology used by the test:
 
@@ -243,6 +249,128 @@ def test_bgp_peer_type_multipath_relax():
     assertMsg = "Reenabling peer-type multipath-relax did not take effect"
     assert res is None, assertMsg
 
+    logger.info("Check recursive resolution of eBGP next hops is not affected")
+    # eBGP next hop resolution rejects recursively resolved next hops by
+    # default, even with peer-type multipath-relax
+    exabgp_cmd(
+        "peer4", "announce route {} next-hop {}\n".format(prefix3, ebgp_resolved_nh)
+    )
+    reffile = os.path.join(CWD, "r1/prefix3-no-recursive.json")
+    expected = json.loads(open(reffile).read())
+    test_func = functools.partial(
+        topotest.router_json_cmp,
+        router,
+        "show ip bgp {} json".format(prefix3),
+        expected,
+    )
+    _, res = topotest.run_and_expect(test_func, None, count=10, wait=1)
+    assertMsg = "Recursive eBGP next hop not as expected for {}".format(prefix3)
+    assert res is None, assertMsg
+
+    exabgp_cmd(
+        "peer4", "announce route {} next-hop {}\n".format(prefix1, ebgp_resolved_nh)
+    )
+    reffile = os.path.join(CWD, "r1/prefix1-no-recursive.json")
+    expected = json.loads(open(reffile).read())
+    test_func = functools.partial(
+        topotest.router_json_cmp,
+        router,
+        "show ip bgp {} json".format(prefix1),
+        expected,
+    )
+    _, res = topotest.run_and_expect(test_func, None, count=10, wait=1)
+    assertMsg = "Recursive eBGP next hop not as expected for {}".format(prefix1)
+    assert res is None, assertMsg
+
+    # When other config allows recursively resolved eBGP next hops,
+    # such next hops in all-eBGP multipaths should be valid
+    router.vtysh_cmd("conf\n router bgp 64510\n neighbor 10.0.4.2 ebgp-multihop\n")
+    reffile = os.path.join(CWD, "r1/prefix3-recursive.json")
+    expected = json.loads(open(reffile).read())
+    test_func = functools.partial(
+        topotest.router_json_cmp,
+        router,
+        "show ip bgp {} json".format(prefix3),
+        expected,
+    )
+    _, res = topotest.run_and_expect(test_func, None, count=10, wait=1)
+    assertMsg = "Recursive eBGP next hop not as expected for {}".format(prefix3)
+    assert res is None, assertMsg
+
+    reffile = os.path.join(CWD, "r1/prefix1-recursive.json")
+    expected = json.loads(open(reffile).read())
+    test_func = functools.partial(
+        topotest.router_json_cmp,
+        router,
+        "show ip bgp {} json".format(prefix1),
+        expected,
+    )
+    _, res = topotest.run_and_expect(test_func, None, count=10, wait=1)
+    assertMsg = "Recursive eBGP next hop not as expected for {}".format(prefix1)
+    assert res is None, assertMsg
+
+    logger.info("Check mixed-type multipath next hop recursive resolution in FIB")
+    # There are now two eBGP-learned routes with a recursively resolved next;
+    # hop; one is all-eBGP multipath, and the other is iBGP/eBGP/
+    # confed-external. The peer-type multipath-relax feature only enables
+    # recursive resolution in FIB if any next hop is iBGP/confed-learned. The
+    # all-eBGP multipath will have only one valid next hop in the FIB.
+    reffile = os.path.join(CWD, "r1/prefix3-ip-route.json")
+    expected = json.loads(open(reffile).read())
+    test_func = functools.partial(
+        topotest.router_json_cmp,
+        router,
+        "show ip route {} json".format(prefix3),
+        expected,
+    )
+    _, res = topotest.run_and_expect(test_func, None, count=10, wait=1)
+    assertMsg = "FIB next hops mismatch for all-eBGP multipath"
+    assert res is None, assertMsg
+
+    # check confed-external enables recursively resolved next hops by itself
+    exabgp_cmd(
+        "peer1",
+        "withdraw route {} next-hop {} as-path [ 64499 ]\n".format(
+            prefix1, resolved_nh1
+        ),
+    )
+    reffile = os.path.join(CWD, "r1/prefix1-eBGP-confed.json")
+    expected = json.loads(open(reffile).read())
+    test_func = functools.partial(
+        topotest.router_json_cmp,
+        router,
+        "show ip route {} json".format(prefix1),
+        expected,
+    )
+    _, res = topotest.run_and_expect(test_func, None, count=10, wait=1)
+    assertMsg = "FIB next hops mismatch for eBGP+confed-external multipath"
+    assert res is None, assertMsg
+
+    # check iBGP by itself
+    exabgp_cmd(
+        "peer1",
+        "announce route {} next-hop {} as-path [ 64499 ]\n".format(
+            prefix1, resolved_nh1
+        ),
+    )
+    exabgp_cmd(
+        "peer2",
+        "withdraw route {} next-hop {} as-path [ 64499 ]\n".format(
+            prefix1, resolved_nh2
+        ),
+    )
+    reffile = os.path.join(CWD, "r1/prefix1-eBGP-iBGP.json")
+    expected = json.loads(open(reffile).read())
+    test_func = functools.partial(
+        topotest.router_json_cmp,
+        router,
+        "show ip route {} json".format(prefix1),
+        expected,
+    )
+    _, res = topotest.run_and_expect(test_func, None, count=10, wait=1)
+    assertMsg = "FIB next hops mismatch for eBGP+iBGP multipath"
+    assert res is None, assertMsg
+
 
 def test_memory_leak():
     "Run the memory leak test and report results."