[probably-a-BUG] Link custom attributes not preserved after vlan transformation

ssasso commented 10 months ago

Describe the bug

It seems that, after vlan transformation, custom link attributes are not preserved.

To Reproduce

Topology:

provider: clab

defaults.device: arubacx

# Define custom data for lag
defaults.attributes.link.lag_id: int

module: [ vlan, vrf ]

vrfs:
  tenant:

vlans:
  red:
    vrf: tenant

nodes:
  sw1:
  sw2:

links:
- sw1:
  sw2:
  lag_id: 1
  vlan.access: red

- sw1:
  sw2:
  lag_id: 1

Expected behavior

Given the above topology, and taking as example the file host_vars/sw1/topology.yml (copying only the relevant part), I would expect to find the lag_id attribute on both the two interfaces. Instead, it is present only on the interface without the vlan information.

interfaces:
- bridge: test2_1
  clab:
    name: eth1
  ifindex: 1
  ifname: 1/1/1
  linkindex: 1
  mtu: 1500
  type: lan
  vlan:
    access: red
    access_id: 1000
- clab:
    name: eth2
  ifindex: 2
  ifname: 1/1/2
  ipv4: 10.1.0.1/30
  lag_id: 1
  linkindex: 2
  mtu: 1500
  name: sw1 -> sw2
  neighbors:
  - ifname: 1/1/2
    ipv4: 10.1.0.2/30
    node: sw2
  type: p2p
- bridge_group: 1
  ifindex: 3
  ifname: vlan1000
  ipv4: 172.16.0.1/24
  name: VLAN red (1000) -> [sw2]
  neighbors:
  - ifname: vlan1000
    ipv4: 172.16.0.2/24
    node: sw2
    vrf: tenant
  type: svi
  virtual_interface: true
  vlan:
    mode: irb
  vrf: tenant

Additional context

I know it may seems a "nonsense" to have a vlan declared on a physical interface that will be part of a lag, but I expect to use a custom config template and "inherit" the vlan attributes for the lag from the first physical interface assigned to it.

jbemmel commented 10 months ago

Consider what would happen here:

links:
- sw1:
  sw2:
  lag_id: 1
  vlan.access: red
- sw2:
  sw3:
  lag_id: 2
  vlan.access: red

The problem is that VLANs (on the 'industry standard boxes at least') are global objects. So if multiple links have the same vlan, which attribute do you pick?

It used to be the first interface, but that's kind of arbitrary, so I believe we ended up with "don't do that". Ivan can add more color

ssasso commented 10 months ago

thanks for the notes, but please consider that I won't make this public available, and this is only for my topology and use case (parsing this from custom config file). So I won't lose my mind thinking about edge cases, since I know my topology and config in advance.

Anyway, even if I call the custom attribute label_xxx the raised problem is still the same.

ssasso commented 10 months ago

(even with changing the custom attribute at interface level, which may have more sense, still it is not propagated after vlan modifies the interface)

links:
- sw1:
    lag_id: 1
    vlan.access: red
  sw2:
    lag_id: 2

jbemmel commented 10 months ago

Thanks for helping me feel less alone... :)

I totally get where you're coming from, have been there myself. Let's wait for Ivan to chime in

ipspace commented 10 months ago

OK, here's how things work (https://netlab.tools/module/vlan/#vlan-interface-parameters hints at that, but obviously not in enough details):

A VLAN interface is created once there's a physical interface (or some other reason) with that access VLAN or VLAN in a trunk.
In the old days, the physical interface parameters were copied to the VLAN interface... but then what do you do when you have different parameters on different physical interfaces? The only sane solution is not to have parameters that could be applied on VLAN interfaces defined on physical interfaces but in the vlans dictionary.
Also, we cannot leave the parameters on the physical interfaces, or we risk failing on most devices (imagine configuring IPv4 address on physical interface and the VLAN interface), so they are removed from the physical interfaces.

However (as always), there are exceptions. Right now they "copy from phy to vlan" exceptions are hardcoded in https://github.com/ipspace/netlab/blob/dev/netsim/modules/vlan.py#L769. "clean attr from phy" is done in https://github.com/ipspace/netlab/blob/dev/netsim/modules/vlan.py#L796 from list computed in https://github.com/ipspace/netlab/blob/dev/netsim/modules/vlan.py#L718. We should transform the lists into dicts (https://github.com/ipspace/netlab/blob/dev/netsim/modules/vlan.yml#L19, https://github.com/ipspace/netlab/blob/dev/netsim/modules/vlan.yml#L22 and https://github.com/ipspace/netlab/blob/dev/netsim/modules/vlan.yml#L25) to make then extensible and document the whole thing a bit better.

More details in https://github.com/ipspace/netlab/issues/868

ssasso commented 10 months ago

Ok, I know it could not be the best option, but I have this working workaround in place. A "local" plugin with:

def init(topology: Box) -> None:
    topology.defaults.vlan.attributes.phy_ifattr.append('xxx_to_preserve')
    return

ipspace commented 10 months ago

Ok, I know it could not be the best option, but I have this working workaround in place. A "local" plugin with:
def init(topology: Box) -> None:
    topology.defaults.vlan.attributes.phy_ifattr.append('xxx_to_preserve')
    return

Yes, that's exactly what I had in mind. That list needs to be changed into a dictionary, and then you could add values (keys) with the lab topology.

ipspace commented 10 months ago

@ssasso 86ff6b6 transformed all those lists into dictionaries. You should be able to add a custom physical interface attribute with

defaults.vlan.attributes.phy_ifattr.xxx_to_preserve:

Would love to hear whether this solves your challenge (and yes, I know it still has to be documented).

ssasso commented 10 months ago

This helps thanks @ipspace !

I decided to ruin my xmas doing EVPN interop testing between two vendors, adding to the picture also mclag and anycast vtep... I think I just entered the rabbit (black)hole with this challenge ;-)

And since it was "too easy" to use a custom config file for every device, I thought to create generic templates was much... challenging :D

ipspace commented 10 months ago

I decided to ruin my xmas doing EVPN interop testing between two vendors, adding to the picture also mclag and anycast vtep... I think I just entered the rabbit (black)hole with this challenge ;-)

There are worse ways to ruin xmas break ;) Have fun!

ipspace commented 10 months ago

The "bug" (more like "annoyance" ;) part of this seems to be fixed, I still have to write the documentation.

ipspace / netlab