Skip to content

Redistribute static + IPv6 routes #2279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fzakfeld wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Redistribute static + IPv6 routes #2279

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions osism/tasks/conductor/sonic/config_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1979,9 +1979,15 @@ def _add_vrf_configuration(config, vrf_info, netbox_interfaces):
# Add ROUTE_REDISTRIBUTE for VRF
Copy link
Copy Markdown
Contributor

@ideaship ideaship May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for ROUTE_REDISTRIBUTE

There are currently no tests for emitted ROUTE_REDISTRIBUTE entries — _add_vrf_configuration() is only seen in the orchestrator test as a patch target, with no assertions on its output. This PR changes the emitted entry count from one to four, which is a behavioral change worth pinning. Please add a unit test (alongside the existing BGP globals tests in _config_generator_helpers.py) that calls _add_vrf_configuration() with a VNI-backed VRF fixture and asserts that all four expected keys are present in the resulting ROUTE_REDISTRIBUTE table.

if "ROUTE_REDISTRIBUTE" not in config:
config["ROUTE_REDISTRIBUTE"] = {}
route_redistribute_key = f"{vrf_name}|connected|bgp|ipv4"
config["ROUTE_REDISTRIBUTE"][route_redistribute_key] = {}
logger.info(f"Added ROUTE_REDISTRIBUTE {route_redistribute_key}")
for proto, af in [
("connected", "ipv4"),
("connected", "ipv6"),
Copy link
Copy Markdown
Contributor

@ideaship ideaship May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: IPv6 redistribution entries have no corresponding IPv6 unicast AF

This loop emits {vrf}|connected|bgp|ipv6 and {vrf}|static|bgp|ipv6 into ROUTE_REDISTRIBUTE, but the BGP_GLOBALS_AF block above (lines 1952–1970) only creates {vrf}|ipv4_unicast and {vrf}|l2vpn_evpn for VNI-backed VRFs — it never creates {vrf}|ipv6_unicast. The IPv6 redistribution entries are therefore structurally orphaned for tenant VRFs: there is no IPv6 unicast AF to redistribute into. Either add a {vrf}|ipv6_unicast entry to BGP_GLOBALS_AF alongside the existing ipv4_unicast entry, or explain why the AF is not needed here.

("static", "ipv4"),
Copy link
Copy Markdown
Contributor

@ideaship ideaship May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please confirm: all static routes from every VNI-backed VRF redistributed into BGP with no filtering

With the addition of ("static", "ipv4") and ("static", "ipv6"), this change redistributes every static route from every VNI-backed VRF into BGP unconditionally — no route-map, no prefix-list, no opt-out per VRF. Static routes often include defaults, management escape routes, or other entries that should not leave the local VRF. Is it intentional that there is no filtering mechanism here? If selective export is expected in the future, this is worth noting now so the interface can be designed for it.

Copy link
Copy Markdown
Contributor Author

@fzakfeld fzakfeld May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best if we wait for #2173 which will introduce a way of installing default routes per VRF with default_route_ipv4 and default_route_ipv6. I only need redistribute static in VRFs where this is set anyways, so I think it makes total sense to change that.

Alternative would be a default-originate on the peering sessions to the servers, but this ignores the actual route's presence and would need to be passed to the other switches as well. I much prefer a redistribute static

("static", "ipv6"),
]:
key = f"{vrf_name}|{proto}|bgp|{af}"
config["ROUTE_REDISTRIBUTE"][key] = {}
logger.info(f"Added ROUTE_REDISTRIBUTE {key}")

elif "table_id" in vrf_data:
# VRF with table_id (no RD set in NetBox)
Expand Down