From aa35ee2b6416e7e3cae29dfe5cedf5ffce077496 Mon Sep 17 00:00:00 2001 From: Alex Kudentsov <43482574+alexk37@users.noreply.github.com> Date: Sun, 18 Oct 2026 16:29:40 +0700 Subject: bgp: T9450: do not re-send a peer-group member's inherited activate FRR only prints a peer-group member's own "neighbor X activate" when the member's AFI/SAFI activation state differs from its peer-group's (bgp_config_write_peer_af(), bgpd/bgp_vty.c). A member bound to a group that already carries the same address-family is activated for it automatically at bind time (peer_group_bind(), bgpd/bgpd.c), so the member's own line was never needed there and frr-reload was re-applying it on every reload. Render the member's own "activate" only when its peer-group does not already carry the same address-family. Applies to both "neighbor ... peer-group" and "neighbor ... interface peer-group" members. A member whose group drops the address-family later still gets its own "activate" back, matching FRR deactivating it (peer_deactivate()). When a peer-group gains an address family that some members already carry, frr-reload deletes those members' own "activate" before it adds the group's, so each such member is deactivated and reactivated once (one session reset, or with dynamic capability one withdraw and re-advertise of that address family) at that commit. Before this change that commit did not reset them. The end state is correct and per-AF overrides survive the reset; this is a one-time cost at the commit that changes the group's address-family, not a lasting functional loss. --- data/templates/frr/bgpd.frr.j2 | 29 ++++++- smoketest/scripts/cli/test_protocols_bgp.py | 121 ++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 2 deletions(-) diff --git a/data/templates/frr/bgpd.frr.j2 b/data/templates/frr/bgpd.frr.j2 index f84f10126..50a051729 100644 --- a/data/templates/frr/bgpd.frr.j2 +++ b/data/templates/frr/bgpd.frr.j2 @@ -7,7 +7,7 @@ {% endif %} {### MACRO definition for recurring peer patter, this can be either fed by a ###} {### peer-group or an individual BGP neighbor ###} -{% macro bgp_neighbor(neighbor, config, peer_group=false) %} +{% macro bgp_neighbor(neighbor, config, peer_group=false, groups=none) %} {# BGP order of peer-group and remote-as placement must be honored #} {% if peer_group == true %} neighbor {{ neighbor }} peer-group @@ -263,7 +263,32 @@ {% if afi_config.disable_send_community.standard is vyos_defined %} no neighbor {{ neighbor }} send-community standard {% endif %} +{% set member_pg = none %} +{% if peer_group == false %} +{% if config.peer_group is vyos_defined %} +{% set member_pg = config.peer_group %} +{% elif config.interface.peer_group is vyos_defined %} +{% set member_pg = config.interface.peer_group %} +{% elif config.interface.v6only.peer_group is vyos_defined %} +{% set member_pg = config.interface.v6only.peer_group %} +{% endif %} +{% endif %} +{# T9450: FRR (bgp_config_write_peer_af(), bgpd/bgp_vty.c) prints a peer-group #} +{# member's own "activate" only when the member's activation state for this #} +{# AFI/SAFI differs from its peer-group's: peer_group_active(peer) true and #} +{# g_peer->afc[afi][safi] == peer->afc[afi][safi] (both on, the case here - a #} +{# member with an address-family stanza is always "activate"d) prints nothing, #} +{# because the member inherits activation from the group. Skip the member's own #} +{# line exactly then - when its peer-group also has this AFI/SAFI configured, #} +{# which makes the group render its own "activate" in the peer-group's block. #} +{% if member_pg is vyos_defined and groups[member_pg].address_family[afi] is vyos_defined %} +{% set skip_activate = true %} +{% else %} +{% set skip_activate = false %} +{% endif %} +{% if not skip_activate %} neighbor {{ neighbor }} activate +{% endif %} exit-address-family ! {# j2lint: disable=jinja-statements-delimeter #} @@ -561,7 +586,7 @@ router bgp {{ system_as }} {{ 'vrf ' ~ vrf if vrf is vyos_defined }} {{ 'as-nota ! {% if neighbor is vyos_defined %} {% for peer, config in neighbor.items() %} -{{ bgp_neighbor(peer, config) }} +{{ bgp_neighbor(peer, config, false, peer_group) }} {# j2lint: disable=jinja-statements-delimeter #} {%- endfor %} {% endif %} diff --git a/smoketest/scripts/cli/test_protocols_bgp.py b/smoketest/scripts/cli/test_protocols_bgp.py index 1346bc3bc..b17db1360 100755 --- a/smoketest/scripts/cli/test_protocols_bgp.py +++ b/smoketest/scripts/cli/test_protocols_bgp.py @@ -2260,5 +2260,126 @@ class TestProtocolsBGP(VyOSUnitTestSHIM.TestCase): self.assertIn(vrf_header, frrconfig) self.assertNotIn(no_reject, frrconfig) + def test_bgp_109_peer_group_member_activate_inherited(self): + # T9450: FRR (bgp_config_write_peer_af(), bgpd/bgp_vty.c) prints a + # peer-group member's own "neighbor X activate" only when the + # member's activation state for an AFI/SAFI differs from the + # group's. A member bound to a group that already has the same + # address-family is activated for it at bind time + # (peer_group_bind(), bgpd/bgpd.c), so re-sending the member's own + # "activate" every commit is both redundant and, at scale, one of + # the lines frr-reload re-applies on every reload. + peer_group = 'foo' + member = '192.0.2.1' + iface_member = 'eth0' + + self.cli_set(base_path + ['peer-group', peer_group, 'remote-as', ASN]) + self.cli_set(base_path + ['peer-group', peer_group, 'address-family', 'ipv4-unicast']) + + self.cli_set(base_path + ['neighbor', member, 'peer-group', peer_group]) + # same AFI as the group - the member-specific route-map proves the + # address-family block itself is still rendered + self.cli_set(base_path + ['neighbor', member, 'address-family', 'ipv4-unicast', 'route-map', 'import', route_map_in]) + # an AFI the group does NOT have - the member must keep its own activate + self.cli_set(base_path + ['neighbor', member, 'address-family', 'ipv6-unicast']) + + # an interface-based ("unnumbered") peer-group member behaves the same way + self.cli_set(base_path + ['neighbor', iface_member, 'interface', 'peer-group', peer_group]) + # address-family is a direct child of "neighbor", not of "interface", + # even for an interface-based (unnumbered) member + self.cli_set(base_path + ['neighbor', iface_member, 'address-family', 'ipv4-unicast']) + + self.cli_commit() + + # This reads the generated FRR config file instead of getFRRconfig(): + # FRR's own bgp_config_write_peer_af() always suppresses a member's + # "activate" from its OWN running-config print when the member's + # state matches its peer-group's, regardless of whether the rendered + # file keeps re-sending the redundant line every commit - so vtysh + # cannot tell if we render it. + frr_conf = '/run/frr/config/vyos.frr.conf' + rendered = read_file(frr_conf) + # scope the ipv4-unicast checks to that address-family's own block - + # 192.0.2.1 legitimately gets its own "activate" under ipv6-unicast + # (checked below), and a whole-text assertNotIn would wrongly catch that + ipv4_block, _, ipv6_and_rest = rendered.partition(' address-family ipv6 unicast') + # the group itself always activates + self.assertIn(f' neighbor {peer_group} activate', ipv4_block) + # the member's ipv4-unicast activation is inherited from the group: + # the rendered file must not re-send it, but the member's own per-AF + # option is still there + self.assertNotIn(f' neighbor {member} activate', ipv4_block) + self.assertIn(f' neighbor {member} route-map {route_map_in} in', ipv4_block) + # ipv6-unicast: the group has nothing there, the member keeps its own activate + self.assertIn(f' neighbor {member} activate', ipv6_and_rest) + # the interface-based member inherits the same way (it has no ipv6-unicast + # config, so the whole-text check is unambiguous) + self.assertNotIn(f' neighbor {iface_member} activate', rendered) + + # FRR's own running config must also be exactly this - neither an + # explicit "activate" nor a "no activate" for the inherited member, + # since the member's own per-AF option proves the block is active + frrconfig = self.getFRRconfig(f'router bgp {ASN}', stop_section='^exit') + self.assertNotIn(f'no neighbor {member} activate', frrconfig) + self.assertNotIn(f'no neighbor {iface_member} activate', frrconfig) + + # functional proof beyond the rendered text: FRR's own per-peer state + # (afc[afi][safi], independent of session reachability) must show the + # member active in both families regardless of which one it inherited + neigh = self.getFRRopmode(f'show bgp neighbors {member}', json=True) + self.assertIn('ipv4Unicast', neigh[member]['addressFamilyInfo']) + self.assertIn('ipv6Unicast', neigh[member]['addressFamilyInfo']) + iface_neigh = self.getFRRopmode(f'show bgp neighbors {iface_member}', json=True) + self.assertIn('ipv4Unicast', iface_neigh[iface_member]['addressFamilyInfo']) + + # the group loses ipv4-unicast later: FRR deactivates the member + # (peer_deactivate(), bgpd.c) and the member must render its own + # activate again to catch up + self.cli_delete(base_path + ['peer-group', peer_group, 'address-family', 'ipv4-unicast']) + self.cli_commit() + + frrconfig = self.getFRRconfig(f'router bgp {ASN}', stop_section='^exit') + self.assertNotIn(f' neighbor {peer_group} activate', frrconfig) + # scope to the ipv4-unicast sub-section: 192.0.2.1 is also active in + # ipv6-unicast, where the group has nothing, so FRR always prints its + # "activate" there regardless of the ipv4 state being checked here + frr_v4 = self.getFRRconfig(f'router bgp {ASN}', stop_section='^exit', + start_subsection=' address-family ipv4 unicast', + stop_subsection='^ exit-address-family') + self.assertIn(f' neighbor {member} activate', frr_v4) + self.assertIn(f' neighbor {member} route-map {route_map_in} in', frrconfig) + self.assertIn(f' neighbor {iface_member} activate', frrconfig) + + neigh = self.getFRRopmode(f'show bgp neighbors {member}', json=True) + self.assertIn('ipv4Unicast', neigh[member]['addressFamilyInfo']) + + # the group gains the address family back: the members' own activate + # is dropped again (frr-reload deletes each member's "activate" before + # it adds the group's - a one-time session reset per affected member + # at this commit, not a lasting functional loss), and FRR must still + # have each member active (inherited) + self.cli_set(base_path + ['peer-group', peer_group, 'address-family', 'ipv4-unicast']) + self.cli_commit() + + rendered = read_file(frr_conf) + ipv4_block = rendered.partition(' address-family ipv6 unicast')[0] + self.assertNotIn(f' neighbor {member} activate', ipv4_block) + # whole-text check, not scoped to ipv4_block: eth0 has no ipv6-unicast + # stanza of its own, so the partition above would put its entire block + # (including this line, if rendered) past the first ipv6-unicast + # boundary introduced by 192.0.2.1 - scoping this assertion to + # ipv4_block would never see it either way + self.assertNotIn(f' neighbor {iface_member} activate', rendered) + + neigh = self.getFRRopmode(f'show bgp neighbors {member}', json=True) + self.assertIn('ipv4Unicast', neigh[member]['addressFamilyInfo']) + iface_neigh = self.getFRRopmode(f'show bgp neighbors {iface_member}', json=True) + self.assertIn('ipv4Unicast', iface_neigh[iface_member]['addressFamilyInfo']) + + frrconfig = self.getFRRconfig(f'router bgp {ASN}', stop_section='^exit') + self.assertIn(f' neighbor {member} route-map {route_map_in} in', frrconfig) + self.assertNotIn(f'no neighbor {member} activate', frrconfig) + self.assertNotIn(f'no neighbor {iface_member} activate', frrconfig) + if __name__ == '__main__': unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on()) -- cgit v1.2.3