Unable to set vrf attributes in group node_data

[jbemmel]

Discussed in https://github.com/ipspace/netlab/discussions/479

Configure a vrf property on all leaves: ``` groups: leaves: members: [ leaf1,leaf2 ] device: srlinux module: [ bgp,evpn,vlan,vrf,vxlan ] config: [ leaf-policy.j2 ] node_data: vrfs: customer1: evpn.transit_vni: 1234 ```

This currently does not work, because the data is copied into each node after the vrf node transformation hooks get called

[jbemmel]

Debug trace:

jeroen@jvm:~/srlinux/netsim-examples/BGP/SRL-Route-Leaking$ netlab create 
Copying node_data for global modules: ['vlan', 'bgp', 'vxlan', 'vrf', 'evpn']
Copying node_data for global modules: ['vlan', 'bgp', 'vxlan', 'vrf', 'evpn']
Copying node_data for global modules: ['vlan', 'bgp', 'vxlan', 'vrf', 'evpn']
Copying node_data for global modules: ['vlan', 'bgp', 'vxlan', 'vrf', 'evpn']
vrf.node_pre_transform on leaf1 vrfs=None
vrf.node_pre_transform on leaf2 vrfs=None
vrf.node_pre_transform on spine1 vrfs=None
vrf.node_pre_transform on spine2 vrfs=None
vrf.node_pre_transform on mplsnet vrfs=None
JvB transform_data->adjust_groups
JvB copy_group_node_data vrfs -> node leaf1
JvB copy_group_node_data vrfs -> node leaf2
JvB copy_group_node_data clab -> node spine1
JvB copy_group_node_data bgp -> node spine1
JvB copy_group_node_data clab -> node spine2
JvB copy_group_node_data bgp -> node spine2
JvB transform_data->pre_node_transform
node.vrfs: {'customer1': None, 'customer2': None, 'customer3': None}
node.vrfs: {'customer1': None, 'customer2': None, 'customer3': None, 'global': {'bgp': {'neighbors': [{'ifindex': 8, 'local_if': 'ethernet-1/51.4001', 'ipv4_rfc8950': True, 'local_as': 65001, 'name': 'leaf1', 'as': 65111, 'type': 'ebgp', 'ipv4': True, 'ipv6': True}]}}}

Traceback (most recent call last):
  File "/home/jeroen/srlinux/netlab/netlab", line 8, in <module>
    netsim.cli.lab_commands()
  File "/home/jeroen/srlinux/netlab/netsim/cli/__init__.py", line 154, in lab_commands
    mod.run(sys.argv[arg_start:])   # type: ignore
  File "/home/jeroen/srlinux/netlab/netsim/cli/create.py", line 76, in run
    augment.main.transform(topology)
  File "/home/jeroen/srlinux/netlab/netsim/augment/main.py", line 72, in transform
    transform_data(topology)
  File "/home/jeroen/srlinux/netlab/netsim/augment/main.py", line 60, in transform_data
    modules.post_transform(topology)
  File "/home/jeroen/srlinux/netlab/netsim/modules/__init__.py", line 82, in post_transform
    node_transform("post_transform",topology)
  File "/home/jeroen/srlinux/netlab/netsim/modules/__init__.py", line 544, in node_transform
    mod_load[m].call("node_"+method,n,topology)
  File "/home/jeroen/srlinux/netlab/netsim/callback.py", line 39, in call
    method(*args, **kwargs)
  File "/home/jeroen/srlinux/netlab/netsim/modules/bgp.py", line 518, in node_post_transform
    build_bgp_sessions(node,topology)
  File "/home/jeroen/srlinux/netlab/netsim/modules/bgp.py", line 299, in build_bgp_sessions
    build_ebgp_sessions(node,bgp_sessions,topology)
  File "/home/jeroen/srlinux/netlab/netsim/modules/bgp.py", line 255, in build_ebgp_sessions
    if not node.vrfs[l.vrf].bgp.neighbors:

AttributeError: 'NoneType' object has no attribute 'bgp'

[jbemmel]

The code at https://github.com/ipspace/netlab/blob/dev/netsim/modules/__init__.py#L60 has no effect; it tries to call module vlan node_pre_node_transform on node leaf1 for example, instead of node_pre_transform

Renaming vrf.node_pre_transform -> vrf.node_pre_node_transform makes it get called at the right moment (though it still does not handle merging of empty dicts correctly)

[ipspace]

We can't copy node_data after the _pre_transform hooks have been called because the BGP module builds dynamic groups based on BGP AS numbers https://netsim-tools.readthedocs.io/en/latest/groups.html#automatic-bgp-groups

Looks like we'll just have to document that you can't define VRFs in group data (and why exactly should you?) whereas it should be possible to set VRF attributes in node_data (assuming vrfs/vlans is an allowed keyword -- that should be fixed if it's not).

[jbemmel]

The case above is not trying to define VRFs, it's referencing existing global VRFs to pull them into each leaf

Use case: https://github.com/jbemmel/netsim-examples/blob/master/BGP/SRL-Route-Leaking/topology.yml#L140 (using vlans instead of vrfs, but it's the same issue). In that case I need access to the ipv4 prefix for each vlan, including remote ones

[ipspace]

The case above is not trying to define VRFs, it's referencing existing global VRFs to pull them into each leaf

If there's a currently valid reason to pull VRF data into node data, it will happen. If it doesn't happen, it's a bug that needs to be fixed.

Use case: https://github.com/jbemmel/netsim-examples/blob/master/BGP/SRL-Route-Leaking/topology.yml#L140 (using vlans instead of vrfs, but it's the same issue). In that case I need access to the ipv4 prefix for each vlan, including remote ones

You're yet again trying to implement something outside of the scope of netlab core modules by tweaking the core modules to get results you want. I don't know how many more times I have to tell you to write a plugin -- you could easily write a plugin that would collect global VLAN prefixes, or global VRF prefixes, and store them in some node attribute to be used later by a custom template. Instead of that, we're wasting time arguing about the number of angels that can dance on a pinhead. I won't do that anymore.

[jbemmel]

I'm trying to use the tool for my purposes, and in doing so I encounter various issues. I share those issues and discuss them, because if I'm hitting them other people might too. I try to filter and develop a sense of the type of issues you might appreciate, but I'm still learning and I may over-communicate at times.

For this specific issue, since you agreed "it should be possible to set VRF attributes in node_data" I added https://github.com/ipspace/netlab/pull/481/files#diff-f8c7fe10877d0f03c53624d8addbd3b2ac7a2a4933aac039365240c70201f821R20 which fails on dev. Do you agree that is a valid use case, or does it need to be changed?

[jbemmel]

Alternatively, for vlans:

provider: clab
defaults:
  device: srlinux

module: [vlan]

vlans:
  vlan1:
    mode: route

nodes: [ spine, leaf1, leaf2 ]

groups:
  spines: [ spine ]
  leaves:
    members: [ leaf1,leaf2 ]
    node_data:
      vlans:
        vlan1:
          mode: irb

[ipspace]

I'm trying to use the tool for my purposes, and in doing so I encounter various issues.

No problem there.

I share those issues and discuss them, because if I'm hitting them other people might too.

... they might, but they keep curiously silent ;)

I try to filter and develop a sense of the type of issues you might appreciate, but I'm still learning and I may over-communicate at times.

If anything, I would say you under-communicate, or you a slightly wrong approach. This is how I see things working out lately:

• "the tool should do X, and here's the code that does it" in a PR with some code that may or may not be optimal, consider the implications or not, but definitely works with whatever you're trying to accomplish.
• We get into a lengthy academic discussion where you use various arguments to prove you're right.
• After wasting hours if not days, you finally come along saying "I want to do Y, and I want to tweak the tool to do X, so I can get Y done" whereas it would be much better to write a plugin and move on.

We wasted countless hours discussing EVPN IBGP-over-EBGP until you decided to write a plugin, and once other people expressed interest in solving the same challenge and we agreed it's best to have local-as processing in the BGP configuration module, it happened.

For this specific issue, since you agreed "it should be possible to set VRF attributes in node_data" I added

And here we go again. Instead of documenting that VRF attributes don't work well with node_data (there's a good reason for that, and fixing it would require a major redesign), which is what I will do once of these days, you keep arguing and submitting PRs trying to "solve" the problem so your ACL use case would work... while at the same time mangling the module hooks because you didn't understand the original intent.