fix(sonic): key BGP_NEIGHBOR_AF like its neighbor

[ideaship]

Warning

This changes configuration pushed to switches. Where the key mismatch
exists today, the BGP session is activated for no address family; after this
fix it activates, so sessions that were previously dead can come up and
traffic may move. Correct, but it needs a deliberate rollout decision, not
just a code review.

Problem

BGP_NEIGHBOR_AF.neighbor is a YANG leafref into BGP_NEIGHBOR, restricted to
the same VRF (sonic-bgp-neighbor.yang#L138-L146), so the two
tables have to name the same peer. Nothing derived them from a single decision,
so they disagreed in three ways.

The tables were keyed differently. The physical-interface and port-channel
loops keyed BGP_NEIGHBOR by the resolved peer address but keyed
BGP_NEIGHBOR_AF by the interface or port-channel name, so the neighbor got no
activated address family and the address families referenced a neighbor that did
not exist.

A peer address alone decided the peering mode. The resolved address is the
remote endpoint's — any direct IP or FHRP VIP — while whether an
ipv6_unicast row was emitted was decided by has_transfer_ipv4, a property of
the local interface restricted to transfer-role prefixes. Independent
conditions, so an interface carrying no IPv4 of its own facing an endpoint that
carries one produced a numbered BGP_NEIGHBOR this switch cannot source — no
local_addr is found for it either — and an ipv6_unicast row keyed by that
IPv4 literal. The outer has_transfer_ipv4 or not has_direct_ipv4 gate admits
exactly that shape.

v6only followed the same wrong input, so an interface-keyed neighbor could
be emitted with v6only: "false".

The port-channel loop had none of the machinery at all: it keyed on the peer
address alone, emitted both address families unconditionally, hardcoded
v6only: "true", and left local_addr as a TODO.

Why this is not cosmetic

frrcfgd's AF handler never creates the neighbor. It enters the address family
and runs the AF commands against the key it was given
(frrcfgd.py#L2869-L2877; admin_status maps to
neighbor {} activate at #L1896). Replayed against FRR 10.2.6
under SONiC's own no bgp default ipv4-unicast
(bgpd.main.conf.j2#L108):

vtysh -c 'router bgp 65000' -c 'address-family ipv4 unicast' \
      -c 'neighbor PortChannel1 activate'
% Specify remote-as or peer-group commands first

and the numbered neighbor ends up activated for no address family at all, so
the session comes up and exchanges nothing. Nothing masks it: no peer group is
emitted anywhere, and the shipped base config declares none.

The fix

Every attribute of the session follows one decision, answered by
_numbered_peer_address(): a numbered session needs a routable IPv4 at both
ends — the resolved peer address and a transfer-role IPv4 on this side to
source it from. A peer address on its own is not enough, because with no local
address the session cannot come up. Without both, the peering is unnumbered.

From that one answer follow the key, v6only and the address families, in all
four loops:

• the key is the peer address for a numbered session and the interface or
  port-channel name otherwise, so the two tables agree by construction rather
  than by coincidence;
• the address families: a numbered session runs over an IPv4 transfer link
  and activates ipv4_unicast alone; an unnumbered one is an IPv6 link-local
  session and carries both families over it;
• v6only is "false" for a numbered key and "true" for an interface
  key. This part fixes no behaviour on the community SONiC path, where the field
  is inert — absent from sonic-bgp-neighbor.yang, and not read by frrcfgd,
  which picks the neighbor {} interface form from the key shape alone. It is
  worth emitting correctly regardless: v6only: "false" on an interface-keyed
  neighbor is a contradiction on its face, and vendor stacks outside the
  community tree do document and consume the field.

The port-channel loops now take the same path, from the same inputs: the
transfer-role lookup reaches a LAG whose NetBox name is canonical, so a port
channel is numbered under the same condition as a physical interface and finally
carries local_addr. That lookup is now _local_ipv4_address(), shared by both
neighbor loops because neither can assume the SONiC name and the NetBox name
agree.

Unnumbered peers keep working throughout: with no numbered peer to key on, both
tables fall back to the interface name, which is what a link-local session wants
anyway.

Where the split came from

The AF loops were once keyed the way this restores. The stated reason for
changing them to always use interface names was consistency with IPv6 — but at
that commit both address families already shared one neighbor_id, so the
asymmetry being removed did not exist in the code it changed. What the change
added was interface names even where a numbered address is available. The
plausible misreading is that IPv6 peering here is unnumbered and is
legitimately named by interface — but the fallback already covered that case.

Tests

New tests cover each branch of the rule. The ones asserting that the two tables
agree on the key keep the resolver real instead of patching it out, so they can
observe which branch it takes.

Three existing tests move with the rule, which is worth stating plainly
rather than leaving to the diff:

• TestBgpNeighborPortChannels::test_default_vrf_peer_ip_no_local_addr is
  dropped. It asserted that a port-channel neighbor keyed by a peer address
  carries no local_addr — the numbered-neighbor-with-no-local-address shape
  this change eliminates. That shape is unreachable now, since a numbered port
  channel requires the transfer-role address local_addr is read from. The
  input it used is asserted in full, key and every field, by a new test.
• TestBgpNeighborAfInterfaces::test_transfer_role_ipv4_adds_ipv4_only and
  TestBgpNeighborInterfaces::test_transfer_role_ipv4_v6only_false still assert
  what their names say, but reaching a numbered transfer link now takes a
  resolvable peer, so both gain one. Their previous input — a transfer address
  with no peer — is an unnumbered session, and a new test covers it.

Worth flagging separately: every other BGP test in that module patches the
resolver away, so the suite emits identical output whether the keys agree or
not and is structurally incapable of seeing this class of defect.

753 passed in tests/unit/tasks/conductor/sonic/, 3213 passed in
tests/unit (3 pre-existing xfails), flake8 and black clean.

Effect on generated config

Verified with the SONiC E2E golden harness rather than by inspection. On the
existing fixtures the only change is the AF keys lining up with the neighbors
that already existed:

- "default|Ethernet0|ipv4_unicast"  →  + "default|192.168.30.1|ipv4_unicast"
- "default|Ethernet0|l2vpn_evpn"    →  + "default|192.168.30.1|l2vpn_evpn"
- "default|Ethernet4|ipv4_unicast"  →  + "default|192.168.30.3|ipv4_unicast"
- "default|Ethernet4|l2vpn_evpn"    →  + "default|192.168.30.3|l2vpn_evpn"

BGP_NEIGHBOR on that device is already keyed default|192.168.30.1 and
default|192.168.30.3. No v6only value moves on those fixtures, because every
addressed fabric link resolves a peer and so was already numbered.

The committed goldens live only on the unmerged E2E series, so nothing in main
pins the old shape and this change breaks no committed golden. Regenerating
those goldens, and adding fixtures that reach the peering modes above — a
connected port channel, and a port whose peer carries an address it does not —
belongs to that series.

[sourcery-ai]

Hey - I've reviewed your changes and they look great!

Sourcery assessment

Needs a human reviewer. If the resolver supplies an incorrect peer address, BGP_NEIGHBOR_AF can reference the wrong or nonexistent neighbor, leaving address-family configuration inactive and potentially disrupting routing. Reverting and reapplying the previous generator output can restore configuration, but any routing outage or traffic impact that occurred before the revert is not undone.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[osfrickler]

I think this guard is potentially inconsistent with the above. If neighbor_id = connected_ipv4, it makes no sense at all to construct an ipv6_key containing it. One would need a connected_ipv6 ID instead, catering for the case that both IPv4 and IPv6 transfer nets are configured on the interface. But that's a second step to add a new feature maybe. For now just resolving the discrepancy between using connected_ipv4 as a guard above and using has_transfer_ipv4 here should be resolved, I'm just not sure into which direction.

[ideaship]

Thank you for catching this. I tried to keep the change small, but that was the wrong priority. Should be fixed now.

[osfrickler]

This is even more wrong than above. If there is only an IPv4 transfer net, IPv6 AF should not be enabled at all.