diff --git a/bgpd/bgp_open.c b/bgpd/bgp_open.c index 28cacd6086..7248f034a5 100644 --- a/bgpd/bgp_open.c +++ b/bgpd/bgp_open.c @@ -1152,7 +1152,7 @@ static bool bgp_role_violation(struct peer *peer) return true; } if (remote_role == ROLE_UNDEFINED && - CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE)) { + CHECK_FLAG(peer->flags, PEER_FLAG_ROLE_STRICT_MODE)) { const char *err_msg = "Strict mode. Please set the role on your side."; bgp_notify_send_with_data(peer, BGP_NOTIFY_OPEN_ERR, diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index f8980ba102..1bff1c6b97 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -6449,7 +6449,7 @@ static int peer_role_set_vty(struct vty *vty, const char *ip_str, { struct peer *peer; - peer = peer_lookup_vty(vty, ip_str); + peer = peer_and_group_lookup_vty(vty, ip_str); if (!peer) return CMD_WARNING_CONFIG_FAILED; uint8_t role = get_role_by_name(role_str); @@ -6463,7 +6463,7 @@ static int peer_role_unset_vty(struct vty *vty, const char *ip_str) { struct peer *peer; - peer = peer_lookup_vty(vty, ip_str); + peer = peer_and_group_lookup_vty(vty, ip_str); if (!peer) return CMD_WARNING_CONFIG_FAILED; return bgp_vty_return(vty, peer_role_unset(peer)); @@ -16785,13 +16785,13 @@ static void bgp_config_write_peer_global(struct vty *vty, struct bgp *bgp, } /* role */ - if (peer->local_role != ROLE_UNDEFINED) { + if (peergroup_flag_check(peer, PEER_FLAG_ROLE) && + peer->local_role != ROLE_UNDEFINED) vty_out(vty, " neighbor %s local-role %s%s\n", addr, bgp_get_name_by_role(peer->local_role), - CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE) + CHECK_FLAG(peer->flags, PEER_FLAG_ROLE_STRICT_MODE) ? " strict-mode" : ""); - } /* ttl-security hops */ if (peer->gtsm_hops != BGP_GTSM_HOPS_DISABLED) { diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 8ece201198..b6f2a294a6 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -2731,6 +2731,9 @@ static void peer_group2peer_config_copy(struct peer_group *group, } } + /* role */ + PEER_ATTR_INHERIT(peer, group, local_role); + /* Update GR flags for the peer. */ bgp_peer_gr_flags_update(peer); @@ -4256,7 +4259,8 @@ static const struct peer_flag_action peer_flag_action_list[] = { {PEER_FLAG_UPDATE_SOURCE, 0, peer_change_none}, {PEER_FLAG_DISABLE_LINK_BW_ENCODING_IEEE, 0, peer_change_none}, {PEER_FLAG_EXTENDED_OPT_PARAMS, 0, peer_change_reset}, - {PEER_FLAG_STRICT_MODE, 0, peer_change_reset}, + {PEER_FLAG_ROLE_STRICT_MODE, 0, peer_change_reset}, + {PEER_FLAG_ROLE, 0, peer_change_reset}, {0, 0, 0}}; static const struct peer_flag_action peer_af_flag_action_list[] = { @@ -4929,36 +4933,109 @@ int peer_ebgp_multihop_unset(struct peer *peer) /* Set Open Policy Role and check its correctness */ int peer_role_set(struct peer *peer, uint8_t role, bool strict_mode) { - if (peer->sort != BGP_PEER_EBGP) - return BGP_ERR_INVALID_INTERNAL_ROLE; + struct peer *member; + struct listnode *node, *nnode; - if (peer->local_role == role) { - if (CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE) && - !strict_mode) - /* TODO: Is session restart needed if it was down? */ - UNSET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE); - if (!CHECK_FLAG(peer->flags, PEER_FLAG_STRICT_MODE) && - strict_mode) { - SET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE); - /* Restart session to throw Role Mismatch Notification - */ - if (peer->remote_role == ROLE_UNDEFINED) - bgp_session_reset(peer); + peer_flag_set(peer, PEER_FLAG_ROLE); + + if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) { + if (peer->sort != BGP_PEER_EBGP) + return BGP_ERR_INVALID_INTERNAL_ROLE; + + if (peer->local_role == role) { + if (CHECK_FLAG(peer->flags, + PEER_FLAG_ROLE_STRICT_MODE) && + !strict_mode) + /* TODO: Is session restart needed if it was + * down? + */ + UNSET_FLAG(peer->flags, + PEER_FLAG_ROLE_STRICT_MODE); + if (!CHECK_FLAG(peer->flags, + PEER_FLAG_ROLE_STRICT_MODE) && + strict_mode) { + SET_FLAG(peer->flags, + PEER_FLAG_ROLE_STRICT_MODE); + /* Restart session to throw Role Mismatch + * Notification + */ + if (peer->remote_role == ROLE_UNDEFINED) + bgp_session_reset(peer); + } + } else { + peer->local_role = role; + if (strict_mode) + SET_FLAG(peer->flags, + PEER_FLAG_ROLE_STRICT_MODE); + else + UNSET_FLAG(peer->flags, + PEER_FLAG_ROLE_STRICT_MODE); + bgp_session_reset(peer); } - } else { - peer->local_role = role; - if (strict_mode) - SET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE); - else - UNSET_FLAG(peer->flags, PEER_FLAG_STRICT_MODE); - bgp_session_reset(peer); + + return CMD_SUCCESS; } - return 0; + + peer->local_role = role; + for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) { + if (member->sort != BGP_PEER_EBGP) + return BGP_ERR_INVALID_INTERNAL_ROLE; + + if (member->local_role == role) { + if (CHECK_FLAG(member->flags, + PEER_FLAG_ROLE_STRICT_MODE) && + !strict_mode) + /* TODO: Is session restart needed if it was + * down? + */ + UNSET_FLAG(member->flags, + PEER_FLAG_ROLE_STRICT_MODE); + if (!CHECK_FLAG(member->flags, + PEER_FLAG_ROLE_STRICT_MODE) && + strict_mode) { + SET_FLAG(peer->flags, + PEER_FLAG_ROLE_STRICT_MODE); + SET_FLAG(member->flags, + PEER_FLAG_ROLE_STRICT_MODE); + /* Restart session to throw Role Mismatch + * Notification + */ + if (member->remote_role == ROLE_UNDEFINED) + bgp_session_reset(member); + } + } else { + member->local_role = role; + + if (strict_mode) { + SET_FLAG(peer->flags, + PEER_FLAG_ROLE_STRICT_MODE); + SET_FLAG(member->flags, + PEER_FLAG_ROLE_STRICT_MODE); + } else { + UNSET_FLAG(member->flags, + PEER_FLAG_ROLE_STRICT_MODE); + } + bgp_session_reset(member); + } + } + + return CMD_SUCCESS; } int peer_role_unset(struct peer *peer) { - return peer_role_set(peer, ROLE_UNDEFINED, 0); + struct peer *member; + struct listnode *node, *nnode; + + peer_flag_unset(peer, PEER_FLAG_ROLE); + + if (!CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP)) + return peer_role_set(peer, ROLE_UNDEFINED, 0); + + for (ALL_LIST_ELEMENTS(peer->group->peer, node, nnode, member)) + peer_role_set(member, ROLE_UNDEFINED, 0); + + return CMD_SUCCESS; } /* Neighbor description. */ diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h index 6be05c180a..bc5f6cf6fd 100644 --- a/bgpd/bgpd.h +++ b/bgpd/bgpd.h @@ -1337,9 +1337,12 @@ struct peer { #define PEER_FLAG_EXTENDED_OPT_PARAMS (1ULL << 30) /* BGP Open Policy flags. - * Enforce using roles on both sides + * Enforce using roles on both sides: + * `local-role ROLE strict-mode` configured. */ -#define PEER_FLAG_STRICT_MODE (1ULL << 31) +#define PEER_FLAG_ROLE_STRICT_MODE (1ULL << 31) + /* `local-role` configured */ +#define PEER_FLAG_ROLE (1ULL << 32) /* *GR-Disabled mode means unset PEER_FLAG_GRACEFUL_RESTART diff --git a/tests/topotests/bgp_roles_capability/r1/bgpd.conf b/tests/topotests/bgp_roles_capability/r1/bgpd.conf index 4ec83093dc..273f933d6e 100644 --- a/tests/topotests/bgp_roles_capability/r1/bgpd.conf +++ b/tests/topotests/bgp_roles_capability/r1/bgpd.conf @@ -2,18 +2,24 @@ router bgp 64501 bgp router-id 192.168.2.1 network 192.0.2.0/24 ! Correct role pair - neighbor 192.168.2.2 remote-as 64502 + neighbor 192.168.2.2 remote-as external neighbor 192.168.2.2 local-role provider neighbor 192.168.2.2 timers 3 10 ! Incorrect role pair - neighbor 192.168.3.2 remote-as 64503 + neighbor 192.168.3.2 remote-as external neighbor 192.168.3.2 local-role provider neighbor 192.168.3.2 timers 3 10 ! Missed neighbor role - neighbor 192.168.4.2 remote-as 64504 + neighbor 192.168.4.2 remote-as external neighbor 192.168.4.2 local-role provider neighbor 192.168.4.2 timers 3 10 ! Missed neighbor role with strict-mode - neighbor 192.168.5.2 remote-as 64505 + neighbor 192.168.5.2 remote-as external neighbor 192.168.5.2 local-role provider strict-mode neighbor 192.168.5.2 timers 3 10 +! Testing peer-groups + neighbor PG peer-group + neighbor PG remote-as external + neighbor PG local-role provider + neighbor PG timers 3 10 + neighbor 192.168.6.2 peer-group PG diff --git a/tests/topotests/bgp_roles_capability/r1/zebra.conf b/tests/topotests/bgp_roles_capability/r1/zebra.conf index 3e90a261c1..301a1e8890 100644 --- a/tests/topotests/bgp_roles_capability/r1/zebra.conf +++ b/tests/topotests/bgp_roles_capability/r1/zebra.conf @@ -11,5 +11,8 @@ interface r1-eth2 interface r1-eth3 ip address 192.168.5.1/24 ! +interface r1-eth4 + ip address 192.168.6.1/24 +! ip forwarding ! diff --git a/tests/topotests/bgp_roles_capability/r2/bgpd.conf b/tests/topotests/bgp_roles_capability/r2/bgpd.conf index e741aa16ce..aa8ba72cad 100644 --- a/tests/topotests/bgp_roles_capability/r2/bgpd.conf +++ b/tests/topotests/bgp_roles_capability/r2/bgpd.conf @@ -1,5 +1,5 @@ router bgp 64502 bgp router-id 192.168.2.2 - neighbor 192.168.2.1 remote-as 64501 + neighbor 192.168.2.1 remote-as external neighbor 192.168.2.1 local-role customer neighbor 192.168.2.1 timers 3 10 diff --git a/tests/topotests/bgp_roles_capability/r3/bgpd.conf b/tests/topotests/bgp_roles_capability/r3/bgpd.conf index d1404260e6..5ed93d63e5 100644 --- a/tests/topotests/bgp_roles_capability/r3/bgpd.conf +++ b/tests/topotests/bgp_roles_capability/r3/bgpd.conf @@ -1,4 +1,4 @@ router bgp 64503 - neighbor 192.168.3.1 remote-as 64501 + neighbor 192.168.3.1 remote-as external neighbor 192.168.3.1 local-role peer neighbor 192.168.3.1 timers 3 10 diff --git a/tests/topotests/bgp_roles_capability/r4/bgpd.conf b/tests/topotests/bgp_roles_capability/r4/bgpd.conf index e6a0a9222a..118132d490 100644 --- a/tests/topotests/bgp_roles_capability/r4/bgpd.conf +++ b/tests/topotests/bgp_roles_capability/r4/bgpd.conf @@ -1,3 +1,3 @@ router bgp 64504 - neighbor 192.168.4.1 remote-as 64501 + neighbor 192.168.4.1 remote-as external neighbor 192.168.4.1 timers 3 10 diff --git a/tests/topotests/bgp_roles_capability/r5/bgpd.conf b/tests/topotests/bgp_roles_capability/r5/bgpd.conf index eeeed7ed30..2f62fbf11a 100644 --- a/tests/topotests/bgp_roles_capability/r5/bgpd.conf +++ b/tests/topotests/bgp_roles_capability/r5/bgpd.conf @@ -1,3 +1,3 @@ router bgp 64505 - neighbor 192.168.5.1 remote-as 64501 + neighbor 192.168.5.1 remote-as external neighbor 192.168.5.1 timers 3 10 diff --git a/tests/topotests/bgp_roles_capability/r6/bgpd.conf b/tests/topotests/bgp_roles_capability/r6/bgpd.conf new file mode 100644 index 0000000000..44b12febf2 --- /dev/null +++ b/tests/topotests/bgp_roles_capability/r6/bgpd.conf @@ -0,0 +1,4 @@ +router bgp 64506 + neighbor 192.168.6.1 remote-as external + neighbor 192.168.6.1 local-role customer + neighbor 192.168.6.1 timers 3 10 diff --git a/tests/topotests/bgp_roles_capability/r6/zebra.conf b/tests/topotests/bgp_roles_capability/r6/zebra.conf new file mode 100644 index 0000000000..c8428c4ba2 --- /dev/null +++ b/tests/topotests/bgp_roles_capability/r6/zebra.conf @@ -0,0 +1,6 @@ +! +interface r6-eth0 + ip address 192.168.6.2/24 +! +ip forwarding +! diff --git a/tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py b/tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py index f72b6d2cee..d72fd19b91 100644 --- a/tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py +++ b/tests/topotests/bgp_roles_capability/test_bgp_roles_capability.py @@ -43,7 +43,7 @@ from lib.topolog import logger pytestmark = [pytest.mark.bgpd] -topodef = {f"s{i}": ("r1", f"r{i}") for i in range(2, 6)} +topodef = {f"s{i}": ("r1", f"r{i}") for i in range(2, 7)} @pytest.fixture(scope="module") @@ -162,6 +162,24 @@ def test_role_strict_mode(tgen): assert success, "Session between r1 and r5 was not correctly closed" +def test_correct_pair_peer_group(tgen): + # provider-customer pair (using peer-groups) + router = tgen.gears["r1"] + neighbor_ip = "192.168.6.2" + check_r6_established = functools.partial( + check_session_established, router, neighbor_ip + ) + success, _ = topotest.run_and_expect(check_r6_established, True, count=20, wait=3) + assert success, "Session with r6 is not Established" + + neighbor_status = find_neighbor_status(router, neighbor_ip) + assert neighbor_status["localRole"] == "provider" + assert neighbor_status["remoteRole"] == "customer" + assert ( + neighbor_status["neighborCapabilities"].get("role") == "advertisedAndReceived" + ) + + if __name__ == "__main__": args = ["-s"] + sys.argv[1:] sys.exit(pytest.main(args))