Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split set_fhrp_gateway to avoid accessing link.interfaces before it is populated #1344

Closed
wants to merge 2 commits into from

Conversation

jbemmel
Copy link
Collaborator

@jbemmel jbemmel commented Oct 7, 2024

Possibly points at wrong calling order for set_fhrp_gateway - called too early in process?

Hit this with following lab snippet:

vlans:
  v1:
    gateway:
      id: 1
      protocol: vrrp

Possibly points at wrong calling order for set_fhrp_gateway
@ipspace
Copy link
Owner

ipspace commented Oct 8, 2024

While your quick fix definitely prevents a crash, I would love to understand how we get to a point where a link has no interfaces (because there might be other problems lurking in the background that your fix would mask), so please supply the minimal lab topology (not just a snippet) that causes the bug.

@jbemmel
Copy link
Collaborator Author

jbemmel commented Oct 8, 2024

provider: clab

module: [vlan,gateway]

nodes: [n1,n2]

vlans:
 v1:
  gateway:
   id: 1  # Without this bug is not triggered
links:
- n1:
  n2:
  vlan.trunk: [v1]

in augment/links.py transform assign_link_prefix is called before create_node_interfaces. link.interfaces is not yet populated when set_fhrp_gateway runs - it should probably be split

@jbemmel jbemmel marked this pull request as draft October 8, 2024 13:48
Postpone processing link.interfaces to a point where they are populated
@jbemmel jbemmel changed the title Avoid setting interfaces to '{}' Split set_fhrp_gateway to avoid accessing link.interfaces before it is populated Oct 8, 2024
@jbemmel
Copy link
Collaborator Author

jbemmel commented Oct 8, 2024

Wondering if copy_fhrp_gw_to_interfaces wouldn't be better placed in modules/gateway.py

Actually, perhaps create_node_interfaces could handle the copying of the gateway attribute? gateway is listed in link_no_propagate - perhaps because it has special considerations? (e.g. link has both ipv4/v6 but a node only has ipv4 enabled on its interface -> gateway.ipv6 should not be propagated?)

For example like this (defaults/attributes.yml):

link_no_propagate: [ prefix, interfaces ]                               # 'gateway' removed
link_module_no_propagate: [ vlan, dhcp, gateway ]                       # moved here
link_module_propagate: [ gateway.ipv4, gateway.ipv6, gateway.protocol ] # 'protocol' needed for validation

@ipspace
Copy link
Owner

ipspace commented Oct 8, 2024

I'll start working on this in a day or two. The root cause is the set_fhrp_gateway being called (indirectly) from the VLAN module when the VLAN module is creating virtual links for the VLANs in the VLAN trunk.

It could be that your fix is OK and that the set_fhrp_gateway gets called twice (so the nodes get the gateway IP address anyway), or it could be something nasty that has to be fixed within the VLAN module.

Anyway, thanks a million for a nice catch!

@ipspace
Copy link
Owner

ipspace commented Oct 10, 2024

Replaced by #1352. I hoped I'd be able to find a better solution, but it seems like yours was close to the best that could be done considering all the other dependencies (and the order in which things have to be done).

Anyway, it turns out that specifying low gateway ID on a link triggers another bug (#1351) that is very hard to fix in an optimal way. I will do my best there and document the caveats.

@ipspace ipspace closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants