sonic: Add support for BGP IPv6 neighbors on VLAN by berendt · Pull Request #2328 · osism/python-osism

berendt · 2026-06-03T07:38:10Z

Summary

The VLAN BGP neighbor generation in _add_bgp_configurations only looked up the IPv4 address of the connected peer interface, so IPv6 peers never received a BGP_NEIGHBOR entry — 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)
- plus _address_matches_ip_version for 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 matching BGP_NEIGHBOR_AF (ipv4_unicast / ipv6_unicast). Per-address dedup is preserved; v6only=false is set for both families in the default VRF (numbered global addresses, consistent with the prior IPv4 behaviour).

Tests

Extended the patch_bgp fixture with peer_ipv6.
New cases: IPv6-only peer, dual-stack (both neighbors created), non-default VRF (no v6only), and IPv6 dedup.
Direct unit tests in test_connections.py for the version detection and the IPv6 lookup (direct + FHRP VIP).

Closes #2312

🤖 Generated with Claude Code

sourcery-ai

Hey - I've left some high level feedback:

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.

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.

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

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

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. Add get_connected_interface_ipv6_address (factored out of the existing IPv4 lookup into a shared, IP-version-parameterized helper that covers both direct addresses and FHRP VIPs) and use it in the VLAN section so a dual-stack peer yields one numbered neighbor per address family, each with the matching BGP_NEIGHBOR_AF (ipv4_unicast / ipv6_unicast). Closes #2312 AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>

ideaship · 2026-06-04T06:17:43Z

+                        device, sonic_iface_name, netbox
+                    )
+                    if peer_ipv6:
+                        peer_ips.append((peer_ipv6, BGP_AF_IPV6_UNICAST))


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.

ideaship · 2026-06-04T06:17:43Z

+    or ``:``; an unparseable address simply does not match any version.
+    """
+    try:
+        return ipaddress.ip_address(address).version == ip_version


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.

ideaship · 2026-06-04T06:17:43Z

+                    if peer_ipv4:
+                        peer_ips.append((peer_ipv4, BGP_AF_IPV4_UNICAST))
+
+                    peer_ipv6 = get_connected_interface_ipv6_address(


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.

ideaship · 2026-06-04T06:17:43Z



-def get_connected_interface_ipv4_address(device, sonic_port_name, netbox):
+def _address_matches_ip_version(address, ip_version):


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.

ideaship · 2026-06-04T06:17:43Z

+                # 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 = []


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.

ideaship · 2026-06-04T06:17:43Z

            assigned_object_id=connected_interface.id,
        )

        for ip_address in ip_addresses:


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.

osism-agent added this to Human Board Jun 3, 2026

github-project-automation Bot moved this to Ready in Human Board Jun 3, 2026

sourcery-ai Bot reviewed Jun 3, 2026

View reviewed changes

berendt force-pushed the sonic-bgp-ipv6-vlan-neighbors branch from db7f77e to 158d4cf Compare June 3, 2026 07:54

berendt force-pushed the sonic-bgp-ipv6-vlan-neighbors branch from 158d4cf to 7672315 Compare June 3, 2026 08:07

berendt requested a review from ideaship June 3, 2026 08:12

ideaship requested changes Jun 4, 2026

View reviewed changes

github-project-automation Bot moved this from Ready to In review in Human Board Jun 4, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sonic: Add support for BGP IPv6 neighbors on VLAN#2328

sonic: Add support for BGP IPv6 neighbors on VLAN#2328
berendt wants to merge 1 commit into
mainfrom
sonic-bgp-ipv6-vlan-neighbors

berendt commented Jun 3, 2026

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

ideaship Jun 4, 2026

Uh oh!

ideaship Jun 4, 2026

Uh oh!

ideaship Jun 4, 2026

Uh oh!

ideaship Jun 4, 2026

Uh oh!

ideaship Jun 4, 2026

Uh oh!

ideaship Jun 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants



		def get_connected_interface_ipv4_address(device, sonic_port_name, netbox):
		def _address_matches_ip_version(address, ip_version):

Conversation

berendt commented Jun 3, 2026

Summary

Changes

Tests

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ideaship Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

ideaship Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

ideaship Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

ideaship Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

ideaship Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

ideaship Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants