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

[jklare]

Hi @osfrickler @fzakfeld , I just stumbled over this and was wondering if there is still something blocking to get this merged and resolve the linked issues. Currently, it looks like this needs a rebase to resolve the merge conflict, but apart from that, I do not see any requested changes any more.

[osfrickler]

I think this needs to wait for #2297 and be adapted to it afterwards. It likely would also be helpful to wait for the unit tests to be implemented for this code branch and then be able to add unit tests for the new features (though I don't necessarily expect the submitter to perform that task).

We then also need to run some manual tests that assure that existing usecases do not get broken by these changes.

[osfrickler]

I cannot find this in the Sonic User Guide that I have access to, so I wonder whether this might be a new feature that was only recently introduced? This might imply we should better safeguard this by a version check so we don't mistakenly generate config that an older sonic version cannot process?

I also wonder whether we should track ipv4_anycast and ipv6_anycast seperately and only enable the needed AF here?

[fzakfeld]

4.5.0 defenitely supports it, probably earlier versions as well, but I can add a safeguard. Makes sense to track IPv4 and IPv6 seperately, I'll change that as well