EVPN Layer-2 features

[fzakfeld]

• Support for static default routes: by adding default_route_ipv4 and default_route_ipv6 to custom field sonic_parameters of a VLAN interface, an IPv4 and IPv6 default route will be installed within the assigned VRF
• Support for L2 VNIs using VxLAN / EVPN: by adding the custom tag evpn-l2vni to a VLAN, a VxLAN tunnel entry will be created on all switches using that VLAN. The VNI will be the same as the VLAN ID.
• Support for Static Anycast Gateway (SAG): when assigning an IP address with the role "Anycast" to a VLAN interface, it will be added as an Anycast address. Additionally, SAG will be globally enabled when needed
• Support for EVPN Multihoming: by tagging a portchannel with the custom tag evpn-lag it will be shared across other switches

Closes osism/issues#1348
Closes osism/issues#1346

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• In the management interface setup, config["STATIC_ROUTE"]["mgmt|0.0.0.0/0"] = ... is now executed without ensuring config["STATIC_ROUTE"] exists (the earlier initialization line was removed), which will raise a KeyError; add a guard or reintroduce the dict initialization before assigning the mgmt default route.
• l2vni_vlans is only defined inside the try block in get_device_vlans, but is always referenced in the final return; if an exception occurs before its assignment, this will cause a NameError, so initialize l2vni_vlans = {} before the try block.
• The debug log logger.debug(f"Adding static default routes for VRF {interface_data}") in _add_vlan_configuration logs the full interface_data dict instead of the VRF identifier, which is noisy and less helpful; consider logging vrf_name / vid instead.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In the management interface setup, `config["STATIC_ROUTE"]["mgmt|0.0.0.0/0"] = ...` is now executed without ensuring `config["STATIC_ROUTE"]` exists (the earlier initialization line was removed), which will raise a `KeyError`; add a guard or reintroduce the dict initialization before assigning the mgmt default route.
- `l2vni_vlans` is only defined inside the `try` block in `get_device_vlans`, but is always referenced in the final return; if an exception occurs before its assignment, this will cause a `NameError`, so initialize `l2vni_vlans = {}` before the `try` block.
- The debug log `logger.debug(f"Adding static default routes for VRF {interface_data}")` in `_add_vlan_configuration` logs the full `interface_data` dict instead of the VRF identifier, which is noisy and less helpful; consider logging `vrf_name` / `vid` instead.

## Individual Comments

### Comment 1
<location path="osism/tasks/conductor/sonic/config_generator.py" line_range="264" />
<code_context>
         config["MGMT_INTERFACE"][f"eth0|{oob_ip}/{prefix_len}"] = {}
         metalbox_ip = _get_metalbox_ip_for_device(device)
-        config["STATIC_ROUTE"] = {}
         config["STATIC_ROUTE"]["mgmt|0.0.0.0/0"] = {"nexthop": metalbox_ip}
     else:
         oob_ip = None
</code_context>
<issue_to_address>
**issue (bug_risk):** STATIC_ROUTE may not exist here anymore, leading to a potential KeyError.

Previously this branch initialized `config["STATIC_ROUTE"] = {}` before use. Without that line, if OOB management is enabled and `STATIC_ROUTE` hasn’t been set elsewhere, this will raise a `KeyError`. This branch should be self-contained; add a guard like:

```python
if "STATIC_ROUTE" not in config:
    config["STATIC_ROUTE"] = {}
```

before assigning the mgmt default route.
</issue_to_address>

### Comment 2
<location path="osism/tasks/conductor/netbox.py" line_range="305-314" />
<code_context>
                     # Skip if interface name doesn't follow Vlan<number> pattern
                     pass

+        # Determine which VLANs are tagged evpn-l2vni by fetching full VLAN objects
+        l2vni_vlans = {}
+        if vlan_obj_ids:
</code_context>
<issue_to_address>
**issue (bug_risk):** l2vni_vlans is defined inside the try block and may be undefined in the exception path.

If an exception occurs before `l2vni_vlans = {}` in the `try` block, the `except` will run and the subsequent `return l2vni_vlans` will raise a `NameError`. Initialize `l2vni_vlans = {}` before the `try` and only modify it inside the `try` block to guarantee it always exists.
</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.}

[osfrickler]

please clean up the commit stack a bit, e.g. 84b1c84 should get squashed into the parent, merge commits are not needed ...

that will allow us to apply the commits as they are, without having to squash everything into a single big blob

also some questions regarding the choice of mac addresses

[osfrickler]

where is this mac coming from? is this some random choice or some well known default? might be good to explain in a comment

[fzakfeld]

Random choice, X2-XX is a locally administered / non vendor range. The port channel MACs are calculated using this + the port channel index. For us it is fine to set this as a default, as this won't be used for far-stretched layer 2 where this could come in conflict with other setups. I'll add a comment regarding this

[osfrickler]

is this intentionally different from the evpn system mac?

[fzakfeld]

Yes, the system mac is for the port channels / LACP and this one for the (virtual) L3 gateways (what responds to ARP/ND). I'll also add some comment here