From 11d6560104dd24dcb26451a3baff763341bc1f49 Mon Sep 17 00:00:00 2001 From: Francois Dumontet Date: Wed, 13 Mar 2024 19:22:00 +0100 Subject: [PATCH 1/2] bgpd: make as-path-loop-detection conform to the framework currently: when as-path-loop-detection is set on a peer-group. members of the peer-group are not using that functionnality. analysis: the as-path-loop-detection, is not using the peer's flags related framework. fix: use the peer's flag framework for as-path-loop-detection. Signed-off-by: Francois Dumontet --- bgpd/bgp_route.c | 2 +- bgpd/bgp_updgrp.c | 11 +++++++---- bgpd/bgp_vty.c | 22 +++------------------- bgpd/bgpd.c | 1 + bgpd/bgpd.h | 4 +--- 5 files changed, 13 insertions(+), 27 deletions(-) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 1b9bd7670d17..d726edcc9f9a 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -2242,7 +2242,7 @@ bool subgroup_announce_check(struct bgp_dest *dest, struct bgp_path_info *pi, } /* AS path loop check. */ - if (peer->as_path_loop_detection && + if (CHECK_FLAG(peer->flags, PEER_FLAG_AS_LOOP_DETECTION) && aspath_loop_check(piattr->aspath, peer->as)) { if (bgp_debug_update(NULL, p, subgrp->update_group, 0)) zlog_debug( diff --git a/bgpd/bgp_updgrp.c b/bgpd/bgp_updgrp.c index c522865ebabb..1dab6441f740 100644 --- a/bgpd/bgp_updgrp.c +++ b/bgpd/bgp_updgrp.c @@ -151,7 +151,6 @@ static void conf_copy(struct peer *dst, struct peer *src, afi_t afi, dst->change_local_as = src->change_local_as; dst->shared_network = src->shared_network; dst->local_role = src->local_role; - dst->as_path_loop_detection = src->as_path_loop_detection; if (src->soo[afi][safi]) { ecommunity_free(&dst->soo[afi][safi]); @@ -360,9 +359,12 @@ static unsigned int updgrp_hash_key_make(const void *p) key = jhash_1word(peer->max_packet_size, key); key = jhash_1word(peer->pmax_out[afi][safi], key); - if (peer->as_path_loop_detection) - key = jhash_2words(peer->as, peer->as_path_loop_detection, key); + if (CHECK_FLAG(peer->flags, PEER_FLAG_AS_LOOP_DETECTION)) + key = jhash_2words(peer->as, + CHECK_FLAG(peer->flags, + PEER_FLAG_AS_LOOP_DETECTION), + key); if (peer->group) key = jhash_1word(jhash(peer->group->name, strlen(peer->group->name), SEED1), @@ -464,7 +466,8 @@ static unsigned int updgrp_hash_key_make(const void *p) CHECK_FLAG(peer->af_cap[afi][safi], PEER_UPDGRP_AF_CAP_FLAGS), peer->v_routeadv, peer->change_local_as, - peer->as_path_loop_detection); + !!CHECK_FLAG(peer->flags, + PEER_FLAG_AS_LOOP_DETECTION)); zlog_debug("%pBP Update Group Hash: addpath paths-limit: (send %u, receive %u)", peer, peer->addpath_paths_limit[afi][safi].send, peer->addpath_paths_limit[afi][safi].receive); diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 292040578071..b1f6e2d82874 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -9198,15 +9198,7 @@ DEFPY( NEIGHBOR_ADDR_STR2 "Detect AS loops before sending to neighbor\n") { - struct peer *peer; - - peer = peer_and_group_lookup_vty(vty, neighbor); - if (!peer) - return CMD_WARNING_CONFIG_FAILED; - - peer->as_path_loop_detection = true; - - return CMD_SUCCESS; + return peer_flag_set_vty(vty, neighbor, PEER_FLAG_AS_LOOP_DETECTION); } DEFPY (neighbor_addpath_paths_limit, @@ -9275,15 +9267,7 @@ DEFPY( NEIGHBOR_ADDR_STR2 "Detect AS loops before sending to neighbor\n") { - struct peer *peer; - - peer = peer_and_group_lookup_vty(vty, neighbor); - if (!peer) - return CMD_WARNING_CONFIG_FAILED; - - peer->as_path_loop_detection = false; - - return CMD_SUCCESS; + return peer_flag_unset_vty(vty, neighbor, PEER_FLAG_AS_LOOP_DETECTION); } DEFPY(neighbor_path_attribute_discard, @@ -18460,7 +18444,7 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp, vty_out(vty, " neighbor %s strict-capability-match\n", addr); /* Sender side AS path loop detection. */ - if (peer->as_path_loop_detection) + if (peergroup_flag_check(peer, PEER_FLAG_AS_LOOP_DETECTION)) vty_out(vty, " neighbor %s sender-as-path-loop-detection\n", addr); diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index be816c336139..fcff957d6517 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -4587,6 +4587,7 @@ static const struct peer_flag_action peer_flag_action_list[] = { {PEER_FLAG_GRACEFUL_SHUTDOWN, 0, peer_change_none}, {PEER_FLAG_CAPABILITY_SOFT_VERSION, 0, peer_change_none}, {PEER_FLAG_CAPABILITY_FQDN, 0, peer_change_none}, + {PEER_FLAG_AS_LOOP_DETECTION, 0, peer_change_none}, {0, 0, 0}}; static const struct peer_flag_action peer_af_flag_action_list[] = { diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 94bb10725354..1c5f90c59bf8 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -1470,6 +1470,7 @@ struct peer { #define PEER_FLAG_GRACEFUL_SHUTDOWN (1ULL << 35) #define PEER_FLAG_CAPABILITY_SOFT_VERSION (1ULL << 36) #define PEER_FLAG_CAPABILITY_FQDN (1ULL << 37) /* fqdn capability */ +#define PEER_FLAG_AS_LOOP_DETECTION (1ULL << 38) /* as path loop detection */ /* *GR-Disabled mode means unset PEER_FLAG_GRACEFUL_RESTART @@ -1823,9 +1824,6 @@ struct peer { char *hostname; char *domainname; - /* Sender side AS path loop detection. */ - bool as_path_loop_detection; - /* Extended Message Support */ uint16_t max_packet_size; From 260bdafd8af78f38dacd885c09ee4a52da5c2b68 Mon Sep 17 00:00:00 2001 From: Francois Dumontet Date: Wed, 13 Mar 2024 19:43:36 +0100 Subject: [PATCH 2/2] tests: bgp_sender_as_path_loop_detection add peer-group test add a tests of setting as-path-loop-detection through peer-group Signed-off-by: Francois Dumontet --- .../test_bgp_sender-as-path-loop-detection.py | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/tests/topotests/bgp_sender_as_path_loop_detection/test_bgp_sender-as-path-loop-detection.py b/tests/topotests/bgp_sender_as_path_loop_detection/test_bgp_sender-as-path-loop-detection.py index 3886bc17723a..db6dbc61d220 100644 --- a/tests/topotests/bgp_sender_as_path_loop_detection/test_bgp_sender-as-path-loop-detection.py +++ b/tests/topotests/bgp_sender_as_path_loop_detection/test_bgp_sender-as-path-loop-detection.py @@ -129,6 +129,83 @@ def _bgp_suppress_route_to_r3(): assert result is None, "Routes should not be sent to r1 from r2" +def test_remove_loop_detection_on_one_peer(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + r2 = tgen.gears["r2"] + + def _bgp_reset_route_to_r1(): + output = json.loads( + r2.vtysh_cmd("show ip bgp neighbor 192.168.255.2 advertised-routes json") + ) + expected = {"totalPrefixCounter": 3} + return topotest.json_cmp(output, expected) + + r2.vtysh_cmd( + """ + configure terminal + router bgp 65002 + no neighbor 192.168.255.2 sender-as-path-loop-detection + """ + ) + + r2.vtysh_cmd( + """ + clear bgp * + """ + ) + test_func = functools.partial(_bgp_reset_route_to_r1) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + assert result is None, "Failed bgp to reset route" + + +def test_loop_detection_on_peer_group(): + tgen = get_topogen() + + r2 = tgen.gears["r2"] + + def _bgp_suppress_route_to_r1(): + output = json.loads( + r2.vtysh_cmd("show ip bgp neighbor 192.168.255.2 advertised-routes json") + ) + expected = {"totalPrefixCounter": 0} + return topotest.json_cmp(output, expected) + + def _bgp_suppress_route_to_r3(): + output = json.loads( + r2.vtysh_cmd("show ip bgp neighbor 192.168.254.2 advertised-routes json") + ) + expected = {"totalPrefixCounter": 2} + return topotest.json_cmp(output, expected) + + r2.vtysh_cmd( + """ + configure terminal + router bgp 65002 + neighbor loop_group peer-group + neighbor 192.168.255.2 peer-group loop_group + neighbor loop_group sender-as-path-loop-detection + """ + ) + + r2.vtysh_cmd( + """ + clear bgp * + """ + ) + + test_func = functools.partial(_bgp_suppress_route_to_r3) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + assert result is None, "Route 172.16.255.253/32 should not be sent to r3 from r2" + + test_func = functools.partial(_bgp_suppress_route_to_r1) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + assert result is None, "Routes should not be sent to r1 from r2" + + if __name__ == "__main__": args = ["-s"] + sys.argv[1:] sys.exit(pytest.main(args))