Fix: Mark phantom links used to create multihop EBGP sessions (#2502)

The ebgp.multihop plugin creates bogus ('phantom' sounds so much better)
links to persuade the BGP module to establish BGP sessions between
endpoints of multihop EBGP sessions.

Unfortunately, other modules (in particular 'routing.static') find
those links and want to use them.

With this change, ebgp.multihop plugin marks the interfaces attached to
those links with the '_phantom_link' flag, and the nexthop resolution
for static routes avoids those interfaces. Likewise, the function copying
interfaces into VRF routing protocol data structures skips the phantom
links.

At the moment, these seem to be the only undesired side effect of 
phantom links, but of course something else is bound to pop up
eventually :(

Fixes #2414

Author: ipspace

netsim/extra/ebgp.multihop/plugin.py

@@ -12,16 +12,6 @@
 _execute_after = [ 'ebgp.utils', 'bgp.session' ]
 _requires    = [ 'bgp' ]
 
-def pre_transform(topology: Box) -> None:
-  global _config_name
-  session_idx  = data.find_in_list(['ebgp.utils','bgp.session'],topology.plugin)
-  multihop_idx = data.find_in_list([ _config_name ],topology.plugin)
-
-  if session_idx is not None and multihop_idx is not None and session_idx > multihop_idx:
-    log.error(
-      'ebgp.multihop plugin must be included after bgp.session/ebgp.utils plugin',
-      log.IncorrectValue,'ebgp.multihop')
-
 def pre_link_transform(topology: Box) -> None:
   if not 'multihop' in topology.get('bgp',{}):
     return
@@ -31,6 +21,7 @@ def pre_link_transform(topology: Box) -> None:
 
   sessions = links.adjust_link_list(topology.bgp.multihop.sessions,topology.nodes,'bgp.multihop[{link_cnt}]')
   topology.bgp.multihop.sessions = sessions
+  next_linkindex = links.get_next_linkindex(topology)
   for s in sessions:
     for attr in list(s.keys()):                       # Change session attributes into BGP link attributes
       # Skip internal attributes and BGP/VRF attributes already within BGP namespace
@@ -40,8 +31,9 @@ def pre_link_transform(topology: Box) -> None:
       s.pop(attr,None)
 
     s.type = 'tunnel'
-    s.linkindex = links.get_next_linkindex(topology)
-    s._bgp_session = True
+    s.linkindex = next_linkindex
+    next_linkindex += 1
+    s._phantom_link = True
 
     validate_attributes(
       data=s,                                         # Validate pseudo-link data
@@ -82,7 +74,8 @@ def pre_link_transform(topology: Box) -> None:
       intf.bgp.multihop = 255
       if 'vrf' in intf:
         intf.bgp._vrf = intf.vrf
-      intf._bgp_session = True
+      intf._mh_bgp_session = True
+      intf._phantom_link = True
       intf.ifname = f'_ebgp_multihop_{s.linkindex}'
 
   topology.links.extend(sessions)
@@ -136,28 +129,17 @@ def augment_af_activation(ndata: Box, topology: Box) -> None:
           chg.pop(bgp_af,None)                                    # Otherwise remove it
 
 '''
-remove_fake_interfaces: remove all fake interfaces created to build BGP neighbor adjacencies
+Check whether a node has EBGP multihop sessions
 '''
-def remove_fake_interfaces(ndata: Box, topology: Box) -> bool:
-  intf_count = len(ndata.interfaces)
-  ndata.interfaces = [ intf for intf in ndata.interfaces if not intf.get('_bgp_session',None) ]
+def ebgp_multihop_sessions(ndata: Box, topology: Box) -> bool:
+  mh_intf = [ intf for intf in ndata.interfaces if intf.get('_mh_bgp_session',False) ]
+  if not mh_intf:                                 # Do we have any fake interfaces for MH EBGP sessions?
+    return False
 
-  return len(ndata.interfaces) != intf_count
-
-'''
-cleanup_interfaces: remove fake interfaces from global interface list and VRF OSPF interface list
-'''
-def cleanup_interfaces(ndata: Box, topology: Box) -> None:
-  changed = remove_fake_interfaces(ndata,topology)  # Cleanup global interface list
-  for vname,vdata in ndata.get('vrfs',{}).items():  # Iterate over VRFs
-    if 'ospf' in vdata:                             # .. and cleanup OSPF interface list if needed
-      v_chg   = remove_fake_interfaces(vdata.ospf,topology)
-      changed = changed or v_chg
-
-  if changed:                                       # Did the interface count change?
-    check_multihop_support(ndata,topology)
-    api.node_config(ndata,_config_name)             # We must have some multihop sessions, add extra config
-    augment_af_activation(ndata,topology)
+  check_multihop_support(ndata,topology)          # Check that the node supports it
+  api.node_config(ndata,_config_name)             # We must have some multihop sessions, add extra config
+  augment_af_activation(ndata,topology)           # ... and activate relevant address families on these sessions
+  return True
 
 '''
 fix_vrf_loopbacks:
@@ -179,7 +161,7 @@ def fix_vrf_loopbacks(ndata: Box, topology: Box) -> None:
         if af in lb:                                            # ... removing whatever might have come from the
           ngb[af] = str(ipaddress.ip_interface(lb[af]).ip)      # ... global loopback
 
-    if '_src_vrf' in ngb:                                       # Is out endpoint in a VRF?
+    if '_src_vrf' in ngb:                                       # Is our endpoint in a VRF?
       if not isinstance(features.bgp.multihop,Box) or features.bgp.multihop.vrf is not True:
         log.error(
           f'Device {ndata.device} does not support VRF EBGP multihop sessions (node {ndata.name}, VRF {ngb._src_vrf})',
@@ -211,7 +193,14 @@ def fix_vrf_loopbacks(ndata: Box, topology: Box) -> None:
 def post_transform(topology: Box) -> None:
   global _config_name
   for ndata in topology.nodes.values():
-    fix_vrf_loopbacks(ndata,topology)
-    cleanup_interfaces(ndata,topology)
+    if ebgp_multihop_sessions(ndata,topology):
+      fix_vrf_loopbacks(ndata,topology)
+
+'''
+Cleanup: remove interfaces and links with _phantom_link flag
+'''
+def cleanup(topology: Box) -> None:
+  for ndata in topology.nodes.values():
+    ndata.interfaces = [ intf for intf in ndata.interfaces if not intf.get('_phantom_link',None) ]
 
-  topology.links = [ link for link in topology.links if not link.get('_bgp_session',None) ]
+  topology.links = [ link for link in topology.links if not link.get('_phantom_link',None) ]

netsim/modules/_routing.py

@@ -297,6 +297,8 @@ def build_vrf_interface_list(
       continue
     if 'vrf' not in l:                                                # Interface not in a VRF?
       continue
+    if l.get('_phantom_link',False):                                  # Is this an interface on a phantom link?
+      continue                                                        # Skip it, the interface will be removed anyway
     if node.vrfs[l.vrf][proto] is True:                               # Handle 'force' the protocol by setting it to True
       node.vrfs[l.vrf][proto] = { 'active': True }
     elif node.vrfs[l.vrf][proto] is False:                            # Skip protocols disabled on VRF level

netsim/modules/routing.py

@@ -829,6 +829,8 @@ def resolve_node_nexthop(sr_data: Box, node: Box, topology: Box) -> Box:
 
   node_found = False
   for intf in node.interfaces:
+    if intf.get('_phantom_link',False):
+      continue
     for ngb in intf.neighbors:
       if ngb.node != sr_data.nexthop.node:
         continue

tests/topology/expected/ebgp.utils.yml

@@ -12,15 +12,17 @@ bgp:
       bgp:
         password: Funny
       interfaces:
-      - _bgp_session: true
+      - _mh_bgp_session: true
+        _phantom_link: true
         bgp:
           local_as: 123
           multihop: 255
         ifindex: 20000
         ifname: _ebgp_multihop_5
         ipv4: 10.0.0.2/32
         node: r2
-      - _bgp_session: true
+      - _mh_bgp_session: true
+        _phantom_link: true
         bgp:
           local_as: 456
           multihop: 255