sonic: Add support for BGP IPv6 neighbors on VLAN - #2328
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_address_matches_ip_versionheuristic based on'.'vs':'is brittle (e.g. unusual notations or future refactors); consider usingipaddress.ip_address()to detect the IP version more robustly instead of string inspection. - In
config_generator._add_bgp_configurations, theaddress_familystrings ("ipv4_unicast","ipv6_unicast") are passed around as raw literals; introducing small helpers or constants for AF names would reduce the chance of typos and make future AF extensions easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_address_matches_ip_version` heuristic based on `'.'` vs `':'` is brittle (e.g. unusual notations or future refactors); consider using `ipaddress.ip_address()` to detect the IP version more robustly instead of string inspection.
- In `config_generator._add_bgp_configurations`, the `address_family` strings (`"ipv4_unicast"`, `"ipv6_unicast"`) are passed around as raw literals; introducing small helpers or constants for AF names would reduce the chance of typos and make future AF extensions easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
158d4cf to
7672315
Compare
| device, sonic_iface_name, netbox | ||
| ) | ||
| if peer_ipv6: | ||
| peer_ips.append((peer_ipv6, BGP_AF_IPV6_UNICAST)) |
There was a problem hiding this comment.
Finding 1 — VLAN BGP neighbor address-families are not gated by the local SVI's address families (correctness, data-dependent)
The SVI's own address families aren't consulted before emitting peer neighbors — both peer_ipv4 and peer_ipv6 are looked up regardless of what addresses (line 1351) contains. An IPv4-only SVI here will still emit an IPv6 numbered neighbor (and vice versa), and a numbered session can't come up without a connected route in that AF on the SVI. This gap is pre-existing — the IPv4-only path never gated on the SVI having an IPv4 address either — so it's a widening to IPv6 rather than a regression. But the new test_untagged_member_with_peer_ipv6 now codifies the bad case, which makes this PR the natural place to close it. Suggest parsing vlan_interface_data["addresses"] into the set of local AFs and only appending a peer of a family the SVI can source. Note test_untagged_member_with_peer_ipv6 (test file line 370) currently asserts the IPv6 neighbor on the IPv4-only default _vlan_info — that test should set an IPv6 SVI address once the gate is in, otherwise it's locking in the unsourceable case.
| or ``:``; an unparseable address simply does not match any version. | ||
| """ | ||
| try: | ||
| return ipaddress.ip_address(address).version == ip_version |
There was a problem hiding this comment.
Finding 2 — Link-local IPv6 can be emitted as a numbered BGP neighbor (correctness, data-dependent)
_address_matches_ip_version accepts link-local addresses (fe80::/10, and on the v4 side 169.254.0.0/16). When the connected interface carries a link-local IPv6, this returns it and the VLAN path emits it as a numbered BGP_NEIGHBOR[vrf|fe80::…], which is invalid — link-local neighbors need the unnumbered/v6only form. The IPv4 side has had the same latent gap (169.254.0.0/16), so this isn't a regression — but link-local is far more likely to actually appear on an IPv6 interface in practice, which is why it's worth filtering now. Suggest excluding .is_link_local (and arguably .is_loopback/.is_multicast) here, or filtering at the selection site, so only usable global addresses are returned.
| if peer_ipv4: | ||
| peer_ips.append((peer_ipv4, BGP_AF_IPV4_UNICAST)) | ||
|
|
||
| peer_ipv6 = get_connected_interface_ipv6_address( |
There was a problem hiding this comment.
Finding 3 — Peer IP lookup doubles NetBox I/O per untagged member (cleanup / efficiency)
These two helpers each independently re-fetch the connected interface (connections.py:458), re-filter its IP addresses (line 485), and re-query FHRP assignments (line 506) — so every untagged member pays two full round-trips where one would do. Consider a combined _get_connected_interface_ip_addresses() that fetches interface/IPs/FHRP once and returns both (ipv4, ipv6); the two thin wrappers can remain for existing callers.
|
|
||
|
|
||
| def get_connected_interface_ipv4_address(device, sonic_port_name, netbox): | ||
| def _address_matches_ip_version(address, ip_version): |
There was a problem hiding this comment.
Finding 4 — _address_matches_ip_version duplicates an existing version-detection idiom (cleanup)
Note the project also detects IP version inline via ipaddress.ip_interface(addr).version (config_generator.py:925-926, 1839-1842). Not worth a refactor on its own, but if you add link-local filtering here (see other comment), consider promoting this to a single shared helper so the two idioms can't drift on that rule.
| # Get peer IP addresses (IPv4 and IPv6) using the existing | ||
| # FHRP VIP detection logic. A dual-stack peer yields one | ||
| # neighbor per address family. | ||
| peer_ips = [] |
There was a problem hiding this comment.
Finding 5 — Confusable variable names in the VLAN BGP block (cleanup)
peer_ips (list of (addr, family) tuples) and peer_ips_found (set of addr strings, line 1378) live in the same scope and read as siblings, but hold different shapes. Consider renaming this one to peer_ip_afs / peer_neighbors so a future x in peer_ips written by analogy with the dedup set can't silently compare against tuples.
| assigned_object_id=connected_interface.id, | ||
| ) | ||
|
|
||
| for ip_address in ip_addresses: |
There was a problem hiding this comment.
Finding 6 — IPv6 FHRP "direct address and VIP" precedence is untested (missing/weak test)
The direct-address-before-VIP precedence here is inherited from the IPv4 helper, so it's intentional — but no test covers the "connected interface has both a direct IPv6 and an FHRP VIP" case (the VIP test only reaches the VIP path because there's no direct IPv6). Worth a one-line test asserting the direct address wins when both exist, plus a comment noting that's deliberate — so a future reader doesn't "fix" it toward VIP peering.
The VLAN BGP neighbor generation only looked up the IPv4 address of the connected peer interface, so IPv6 peers never got a BGP_NEIGHBOR entry. Factor the IPv4-only lookup into get_connected_interface_ip_addresses(), which returns the peer's (ipv4, ipv6) addresses in a single NetBox round-trip and covers both directly-assigned addresses and FHRP VIPs (a direct address still wins over a VIP of the same family). The v4/v6 wrappers remain for existing callers. The VLAN section uses it so a dual-stack peer yields one numbered neighbor per address family, each with the matching BGP_NEIGHBOR_AF (ipv4_unicast / ipv6_unicast). Only addresses usable as numbered neighbors are emitted: - VLAN BGP neighbors are gated on the local SVI's address families, so an IPv4-only SVI does not emit an unsourceable IPv6 numbered neighbor (and vice versa). - A shared is_numbered_neighbor_address() helper rejects link-local, loopback and multicast addresses, since a numbered session needs a routable address on both ends and cannot come up over e.g. a link-local fe80::/10 address. Adds tests covering the SVI address-family gate, the direct-address-over -VIP precedence and link-local filtering. Closes #2312 AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
b7e59e3 to
37ba719
Compare
3 participants
Summary
The VLAN BGP neighbor generation in
_add_bgp_configurationsonly looked up the IPv4 address of the connected peer interface, so IPv6 peers never received aBGP_NEIGHBORentry — the case described in #2312.This change adds IPv6 support for BGP neighbors on VLAN interfaces (SVIs).
Changes
connections.py: Factored the existing IPv4 lookup into a shared, IP-version-parameterized helper_get_connected_interface_ip_address(..., ip_version)(covers both direct addresses and FHRP VIPs). Added two thin wrappers:get_connected_interface_ipv4_address(behaviour unchanged)get_connected_interface_ipv6_address(new)_address_matches_ip_versionfor the family check (.vs:).config_generator.py: The VLAN section now resolves both the IPv4 and IPv6 peer address. A dual-stack peer yields one numbered neighbor per address family, each with the matchingBGP_NEIGHBOR_AF(ipv4_unicast/ipv6_unicast). Per-address dedup is preserved;v6only=falseis set for both families in the default VRF (numbered global addresses, consistent with the prior IPv4 behaviour).Tests
patch_bgpfixture withpeer_ipv6.v6only), and IPv6 dedup.test_connections.pyfor the version detection and the IPv6 lookup (direct + FHRP VIP).Closes #2312
🤖 Generated with Claude Code