From 7ee70320d37010026d0b3c1ce7871fb5c72b26d9 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Fri, 17 Mar 2023 14:46:13 +0100 Subject: [PATCH 1/2] bgpd: add cli command to control explicit-null label usage In BGP labeled unicast address-family, it is not possible to send explicit-null label values with redistributed or network declared prefixes. A new CLI command is introduced: > [no] bgp labeled-unicast explicit-null When used, the explicit-null value for IPv4 ('0' value) or IPv6 ('2' value) will be used. It is necessary to reconfigure the networks or the redistribution in order to inherit this new behaviour. Add the documentation. Signed-off-by: Philippe Guibert --- bgpd/bgp_route.c | 37 ++++++++++++++++++++++++------------- bgpd/bgp_vty.c | 21 +++++++++++++++++++++ bgpd/bgpd.h | 2 ++ doc/user/bgp.rst | 11 +++++++++++ 4 files changed, 58 insertions(+), 13 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index a23bffd4a..b49206adc 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -3070,7 +3070,9 @@ static void bgp_process_evpn_route_injection(struct bgp *bgp, afi_t afi, * the IMPLICIT_NULL label. This is pretty specialized: it's only called * in a path where we basically _know_ this is a BGP-LU route. */ -static bool bgp_lu_need_imp_null(const struct bgp_path_info *new_select) +static bool bgp_lu_need_null_label(struct bgp *bgp, + const struct bgp_path_info *new_select, + afi_t afi, mpls_label_t *label) { /* Certain types get imp null; so do paths where the nexthop is * not labeled. @@ -3079,12 +3081,20 @@ static bool bgp_lu_need_imp_null(const struct bgp_path_info *new_select) || new_select->sub_type == BGP_ROUTE_AGGREGATE || new_select->sub_type == BGP_ROUTE_REDISTRIBUTE) return true; - else if (new_select->extra == NULL || - !bgp_is_valid_label(&new_select->extra->label[0])) - /* TODO -- should be configurable? */ - return true; - else + else if (new_select->extra && + bgp_is_valid_label(&new_select->extra->label[0])) return false; + if (label == NULL) + return true; + if (!!CHECK_FLAG(bgp->flags, BGP_FLAG_LU_EXPLICIT_NULL)) + /* Disable PHP : explicit-null */ + *label = afi == AFI_IP ? MPLS_LABEL_IPV4_EXPLICIT_NULL + : MPLS_LABEL_IPV6_EXPLICIT_NULL; + else + /* Enforced PHP popping: implicit-null */ + *label = MPLS_LABEL_IMPLICIT_NULL; + + return true; } /* @@ -3113,6 +3123,7 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, struct bgp_path_info *old_select; struct bgp_path_info_pair old_and_new; int debug = 0; + mpls_label_t mpls_label_null; if (CHECK_FLAG(bgp->flags, BGP_FLAG_DELETE_IN_PROGRESS)) { if (dest) @@ -3167,7 +3178,7 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, * Right now, since we only deal with per-prefix labels, it is not * necessary to do this upon changes to best path. Exceptions: * - label index has changed -> recalculate resulting label - * - path_info sub_type changed -> switch to/from implicit-null + * - path_info sub_type changed -> switch to/from null label value * - no valid label (due to removed static label binding) -> get new one */ if (bgp->allocate_mpls_labels[afi][safi]) { @@ -3176,11 +3187,12 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, || bgp_label_index_differs(new_select, old_select) || new_select->sub_type != old_select->sub_type || !bgp_is_valid_label(&dest->local_label)) { - /* Enforced penultimate hop popping: - * implicit-null for local routes, aggregate - * and redistributed routes + /* control label imposition for local routes, + * aggregate and redistributed routes */ - if (bgp_lu_need_imp_null(new_select)) { + mpls_label_null = MPLS_LABEL_IMPLICIT_NULL; + if (bgp_lu_need_null_label(bgp, new_select, afi, + &mpls_label_null)) { if (CHECK_FLAG( dest->flags, BGP_NODE_REGISTERED_FOR_LABEL) @@ -3189,8 +3201,7 @@ static void bgp_process_main_one(struct bgp *bgp, struct bgp_dest *dest, BGP_NODE_LABEL_REQUESTED)) bgp_unregister_for_label(dest); dest->local_label = mpls_lse_encode( - MPLS_LABEL_IMPLICIT_NULL, 0, 0, - 1); + mpls_label_null, 0, 0, 1); bgp_set_valid_label(&dest->local_label); } else bgp_register_for_label(dest, diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index f7b0caf49..de781d6b1 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -2820,6 +2820,21 @@ DEFUN(no_bgp_ebgp_requires_policy, no_bgp_ebgp_requires_policy_cmd, return CMD_SUCCESS; } +DEFPY(bgp_lu_uses_explicit_null, bgp_lu_uses_explicit_null_cmd, + "[no] bgp labeled-unicast explicit-null", + NO_STR BGP_STR + "BGP Labeled-unicast options\n" + "Use explicit-null label values for local prefixes\n") +{ + VTY_DECLVAR_CONTEXT(bgp, bgp); + + if (no) + UNSET_FLAG(bgp->flags, BGP_FLAG_LU_EXPLICIT_NULL); + else + SET_FLAG(bgp->flags, BGP_FLAG_LU_EXPLICIT_NULL); + return CMD_SUCCESS; +} + DEFUN(bgp_suppress_duplicates, bgp_suppress_duplicates_cmd, "bgp suppress-duplicates", BGP_STR @@ -18235,6 +18250,9 @@ int bgp_config_write(struct vty *vty) ? "" : "no "); + if (!!CHECK_FLAG(bgp->flags, BGP_FLAG_LU_EXPLICIT_NULL)) + vty_out(vty, " bgp labeled-unicast explicit-null\n"); + /* draft-ietf-idr-deprecate-as-set-confed-set */ if (bgp->reject_as_sets) vty_out(vty, " bgp reject-as-sets\n"); @@ -19154,6 +19172,9 @@ void bgp_vty_init(void) install_element(BGP_NODE, &bgp_ebgp_requires_policy_cmd); install_element(BGP_NODE, &no_bgp_ebgp_requires_policy_cmd); + /* bgp labeled-unicast explicit-null */ + install_element(BGP_NODE, &bgp_lu_uses_explicit_null_cmd); + /* bgp suppress-duplicates */ install_element(BGP_NODE, &bgp_suppress_duplicates_cmd); install_element(BGP_NODE, &no_bgp_suppress_duplicates_cmd); diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index a08a2870e..b6491bf79 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -500,6 +500,8 @@ struct bgp { #define BGP_FLAG_HARD_ADMIN_RESET (1ULL << 31) /* Evaluate the AIGP attribute during the best path selection process */ #define BGP_FLAG_COMPARE_AIGP (1ULL << 32) +/* For BGP-LU, force local prefixes to use explicit-null label */ +#define BGP_FLAG_LU_EXPLICIT_NULL (1ULL << 33) /* BGP default address-families. * New peers inherit enabled afi/safis from bgp instance. diff --git a/doc/user/bgp.rst b/doc/user/bgp.rst index 946f0699f..97d7ce6b7 100644 --- a/doc/user/bgp.rst +++ b/doc/user/bgp.rst @@ -2767,6 +2767,17 @@ happened automatically if local-role is set. value of his role (by setting local-role on his side). Otherwise, a Role Mismatch Notification will be sent. +Labeled unicast +--------------- + +*bgpd* supports labeled information, as per :rfc:`3107`. + +.. clicmd:: bgp labeled-unicast explicit-null + +By default, locally advertised prefixes use the `implicit-null` label to +encode in the outgoing NLRI. The following command uses the `explicit-null` +label value for all the BGP instances. + .. _bgp-l3vpn-vrfs: L3VPN VRFs From eee086e6d24b018a61ed0444ae077f4ef2890954 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Fri, 24 Mar 2023 07:08:48 +0100 Subject: [PATCH 2/2] topotests: add topotest to check bgp lu explicit-null service The test ensures that the incoming prefixes are received with the appropriate label value, and that connectivity is ensured between prefixes. Signed-off-by: Philippe Guibert --- .../bgp_lu_explicitnull/r1/bgpd.conf | 15 ++ .../bgp_lu_explicitnull/r1/zebra.conf | 6 + .../bgp_lu_explicitnull/r2/bgpd.conf | 15 ++ .../bgp_lu_explicitnull/r2/zebra.conf | 6 + .../test_bgp_lu_explicitnull.py | 194 ++++++++++++++++++ 5 files changed, 236 insertions(+) create mode 100644 tests/topotests/bgp_lu_explicitnull/r1/bgpd.conf create mode 100644 tests/topotests/bgp_lu_explicitnull/r1/zebra.conf create mode 100644 tests/topotests/bgp_lu_explicitnull/r2/bgpd.conf create mode 100644 tests/topotests/bgp_lu_explicitnull/r2/zebra.conf create mode 100644 tests/topotests/bgp_lu_explicitnull/test_bgp_lu_explicitnull.py diff --git a/tests/topotests/bgp_lu_explicitnull/r1/bgpd.conf b/tests/topotests/bgp_lu_explicitnull/r1/bgpd.conf new file mode 100644 index 000000000..a31439c98 --- /dev/null +++ b/tests/topotests/bgp_lu_explicitnull/r1/bgpd.conf @@ -0,0 +1,15 @@ +router bgp 65500 + bgp router-id 192.0.2.1 + no bgp ebgp-requires-policy + bgp labeled-unicast explicit-null + neighbor 192.0.2.2 remote-as 65501 +! + address-family ipv4 unicast + no neighbor 192.0.2.2 activate + network 192.168.2.1/32 + exit-address-family + ! + address-family ipv4 labeled-unicast + neighbor 192.0.2.2 activate + exit-address-family +! diff --git a/tests/topotests/bgp_lu_explicitnull/r1/zebra.conf b/tests/topotests/bgp_lu_explicitnull/r1/zebra.conf new file mode 100644 index 000000000..b84574891 --- /dev/null +++ b/tests/topotests/bgp_lu_explicitnull/r1/zebra.conf @@ -0,0 +1,6 @@ +interface r1-eth0 + ip address 192.0.2.1/24 +! +interface r1-eth1 + ip address 192.168.2.1/32 +! \ No newline at end of file diff --git a/tests/topotests/bgp_lu_explicitnull/r2/bgpd.conf b/tests/topotests/bgp_lu_explicitnull/r2/bgpd.conf new file mode 100644 index 000000000..41c2b9b6f --- /dev/null +++ b/tests/topotests/bgp_lu_explicitnull/r2/bgpd.conf @@ -0,0 +1,15 @@ +router bgp 65501 + bgp router-id 192.0.2.2 + no bgp ebgp-requires-policy + bgp labeled-unicast explicit-null + neighbor 192.0.2.1 remote-as 65500 +! + address-family ipv4 unicast + no neighbor 192.0.2.1 activate + network 192.168.2.2/32 + exit-address-family + ! + address-family ipv4 labeled-unicast + neighbor 192.0.2.1 activate + exit-address-family +! diff --git a/tests/topotests/bgp_lu_explicitnull/r2/zebra.conf b/tests/topotests/bgp_lu_explicitnull/r2/zebra.conf new file mode 100644 index 000000000..9a639610c --- /dev/null +++ b/tests/topotests/bgp_lu_explicitnull/r2/zebra.conf @@ -0,0 +1,6 @@ +interface r2-eth0 + ip address 192.0.2.2/24 +! +interface r2-eth1 + ip address 192.168.2.2/32 +! diff --git a/tests/topotests/bgp_lu_explicitnull/test_bgp_lu_explicitnull.py b/tests/topotests/bgp_lu_explicitnull/test_bgp_lu_explicitnull.py new file mode 100644 index 000000000..d53ac68e8 --- /dev/null +++ b/tests/topotests/bgp_lu_explicitnull/test_bgp_lu_explicitnull.py @@ -0,0 +1,194 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: ISC +# +# test_bgp_explicitnull.py +# +# Part of NetDEF Topology Tests +# +# Copyright 2023 by 6WIND S.A. +# + +""" +test_bgp_lu_explicitnull.py: Test BGP LU label allocation +""" + +import os +import sys +import json +import functools +import pytest + +# Save the Current Working Directory to find configuration files. +CWD = os.path.dirname(os.path.realpath(__file__)) +sys.path.append(os.path.join(CWD, "../")) + +# pylint: disable=C0413 +# Import topogen and topotest helpers +from lib import topotest +from lib.topogen import Topogen, TopoRouter, get_topogen +from lib.topolog import logger + + +pytestmark = [pytest.mark.bgpd] + + +# Basic scenario for BGP-LU. Nodes are directly connected. +# The 192.168.2.2/32 prefix is advertised from r2 to r1 +# The explicit-null label should be used +# The 192.168.2.1/32 prefix is advertised from r1 to r2 +# The explicit-null label should be used +# Traffic from 192.168.2.1 to 192.168.2.2 should use explicit-null label +# +# AS65500 BGP-LU AS65501 +# +-----+ +-----+ +# | |.1 .2| | +# | 1 +----------------+ 2 + 192.168.0.2/32 +# | | 192.0.2.0/24 | | +# +-----+ +-----+ + + +def build_topo(tgen): + "Build function" + + # Create routers + tgen.add_router("r1") + tgen.add_router("r2") + + # r1-r2 + switch = tgen.add_switch("s1") + switch.add_link(tgen.gears["r1"]) + switch.add_link(tgen.gears["r2"]) + + # r1 + switch = tgen.add_switch("s2") + switch.add_link(tgen.gears["r1"]) + + # r2 + switch = tgen.add_switch("s3") + switch.add_link(tgen.gears["r2"]) + + +def setup_module(mod): + "Sets up the pytest environment" + # This function initiates the topology build with Topogen... + tgen = Topogen(build_topo, mod.__name__) + + # Skip if no mpls support + if not tgen.hasmpls: + logger.info("MPLS is not available, skipping test") + pytest.skip("MPLS is not available, skipping") + return + + # ... and here it calls Mininet initialization functions. + tgen.start_topology() + + # This is a sample of configuration loading. + router_list = tgen.routers() + + # Enable mpls input for routers, so we can ping + sval = "net.mpls.conf.{}.input" + topotest.sysctl_assure(router_list["r2"], sval.format("r2-eth0"), 1) + topotest.sysctl_assure(router_list["r1"], sval.format("r1-eth0"), 1) + + # For all registred routers, load the zebra configuration file + for rname, router in router_list.items(): + router.load_config( + TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname)) + ) + router.load_config( + TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname)) + ) + + # After loading the configurations, this function loads configured daemons. + tgen.start_router() + + +def teardown_module(mod): + "Teardown the pytest environment" + tgen = get_topogen() + + # This function tears down the whole topology. + tgen.stop_topology() + + +def check_show_ip_label_prefix_found(router, ipversion, prefix, label): + output = json.loads( + router.vtysh_cmd("show {} route {} json".format(ipversion, prefix)) + ) + expected = {prefix: [{"prefix": prefix, "nexthops": [{"fib": True, "labels": [label]}]}]} + ret = topotest.json_cmp(output, expected) + if ret is None: + return "not good" + return None + +def test_converge_bgplu(): + "Wait for protocol convergence" + + tgen = get_topogen() + # Don't run this test if we have any failure. + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + # tgen.mininet_cli(); + r1 = tgen.gears["r1"] + r2 = tgen.gears["r2"] + # Check r1 gets prefix 192.168.2.2/32 + test_func = functools.partial( + check_show_ip_label_prefix_found, + tgen.gears["r1"], + "ip", + "192.168.2.2/32", + "0", + ) + success, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assert ( + success + ), "r1, prefix 192.168.2.2/32 from r2 not present" + + # Check r2 gets prefix 192.168.2.1/32 + test_func = functools.partial( + check_show_ip_label_prefix_found, + tgen.gears["r2"], + "ip", + "192.168.2.1/32", + "0", + ) + success, result = topotest.run_and_expect(test_func, None, count=10, wait=0.5) + assert ( + success + ), "r2, prefix 192.168.2.1/32 from r1 not present" + +def test_traffic_connectivity(): + "Wait for protocol convergence" + + tgen = get_topogen() + # Don't run this test if we have any failure. + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + def _check_ping(name, dest_addr, src_addr): + tgen = get_topogen() + output = tgen.gears[name].run("ping {} -c 1 -w 1 -I {}".format(dest_addr, src_addr)) + logger.info(output) + if " 0% packet loss" not in output: + return True + + logger.info("r1, check ping 192.168.2.2 from 192.168.2.1 is OK") + tgen = get_topogen() + func = functools.partial(_check_ping, "r1", "192.168.2.2", "192.168.2.1") + # tgen.mininet_cli() + success, result = topotest.run_and_expect(func, None, count=10, wait=0.5) + assert result is None, "r1, ping to 192.168.2.2 from 192.168.2.1 fails" + +def test_memory_leak(): + "Run the memory leak test and report results." + tgen = get_topogen() + if not tgen.is_memleak_enabled(): + pytest.skip("Memory leak test/report is disabled") + + tgen.report_memory_leaks() + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args))