Redistribute static + IPv6 routes

[fzakfeld]

No description provided.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The _evpn_system_mac and _sag_gwmac values are only checked for truthiness/type; consider validating them against a proper MAC address format so bad config_context data fails fast and predictably.
• In _add_vlan_configuration, SAG_GLOBAL['IP'] enables both IPv4 and IPv6 unconditionally; if a device only has anycast addresses for a single address family, you might want to derive which families to enable from the collected ipv4_anycast / ipv6_anycast lists instead of always enabling both.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `_evpn_system_mac` and `_sag_gwmac` values are only checked for truthiness/type; consider validating them against a proper MAC address format so bad config_context data fails fast and predictably.
- In `_add_vlan_configuration`, `SAG_GLOBAL['IP']` enables both IPv4 and IPv6 unconditionally; if a device only has anycast addresses for a single address family, you might want to derive which families to enable from the collected `ipv4_anycast` / `ipv6_anycast` lists instead of always enabling both.

## Individual Comments

### Comment 1
<location path="osism/tasks/conductor/sonic/config_generator.py" line_range="2154" />
<code_context>
                 "min_links": pc_data["min_links"],
                 "mtu": pc_data["mtu"],
             }
+            if pc_data.get("evpn_lag") and evpn_system_mac:
+                pc_config["system_mac"] = evpn_system_mac
+            config["PORTCHANNEL"][pc_name] = pc_config
</code_context>
<issue_to_address>
**suggestion:** EVPN LAG tag without evpn_system_mac is silently ignored, which may be hard to debug.

Here we only set `system_mac` when `evpn_lag` is true *and* `evpn_system_mac` is valid. If a port channel is tagged as EVPN LAG but `evpn_system_mac` is missing or invalid, the tag has no effect and the `system_mac` is silently omitted. Consider logging or warning when `pc_data.get("evpn_lag")` is true but `evpn_system_mac` is falsy to surface misconfigurations.

Suggested implementation:

```python
            if pc_data.get("evpn_lag") and evpn_system_mac:
                pc_config["system_mac"] = evpn_system_mac
            elif pc_data.get("evpn_lag") and not evpn_system_mac:
                logger.warning(
                    "Port-channel '%s' is tagged as EVPN LAG but no valid evpn_system_mac is set; "
                    "EVPN LAG configuration will be ignored for this port-channel.",
                    pc_name,
                )
            config["PORTCHANNEL"][pc_name] = pc_config

```

1. Ensure that a `logger` object is available in this module. If it is not already defined, add at the top of `config_generator.py`:
   - `import logging`
   - `logger = logging.getLogger(__name__)`
2. If the project convention uses a different logger name (e.g. `LOG` or `log`), or a shared logger utility, adjust the `logger.warning(...)` call accordingly to match existing patterns.
</issue_to_address>

---

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.}

[ideaship]

Approving this change would be under the following assumption: entries whose key and value are fully derived from NetBox plus hardcoded SONiC policy are fully owned by the generator and should always be overwritten on regen.

Under that rule, the unconditional assignment at line 1989 (config["ROUTE_REDISTRIBUTE"][key] = {}) is correct. If the intent is that operators can layer route-maps onto generated VRFs, that is a feature request that requires changing the generator's ownership model — it is not a bug in the current code.

The reason for flagging this explicitly: another reviewer may dispute the assumption. generate_sonic_config() deep-copies /etc/sonic/config_db.json, and the PR expands the set of overwritten keys from one (connected|bgp|ipv4) to four, including the static variants where operator policy is most plausible. If the rule above is intentional and documented somewhere, a pointer here would close the question.

[ideaship]

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.

[ideaship]

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.

[ideaship]

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.

[fzakfeld]

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