Adding rr_mesh option to BGP Configuration Module

[aeangel]

I noticed something strange: when we have more than one route reflector in the topology, they try to establish sessions with each other—even without a direct link between them. I don't think that's the intended behavior.
I propose adding an rr_mesh option to control this process. To avoid breaking existing setups, the default value should be set to true.
I’ve already added this to the code in my lab, and it seems to work without causing any issues.

def build_ibgp_sessions(node: Box, sessions: Box, topology: Box) -> None:
  node_as  = node.bgp['as']                       # Set up variables that will come handy, starting with node AS
  is_rr    = node.bgp.get("rr",None)              # Is this node an RR?
  bgp_nhs  = node.bgp.get("next_hop_self",None)   # Do we have to set next hop on IBGP sessions?
  has_ibgp = False                                # Assume we have no IBGP sessions (yet)
  rr_mesh  = node.bgp.get("rr_mesh", False)            # get rr_mesh option
  rrlist = [] if is_rr else find_bgp_rr(node_as,topology)

  if is_rr or not rrlist:                         # If the current node is RR or we have a full mesh
    ibgp_ngb_list = [                             # ... we need IBGP sessions to all nodes in the AS
      ngb for ngb in topology.nodes.values()      # ... but while building the node list
        if ngb.name != node.name and              # ... skip current node
           ngb.get('bgp.as',None) == node_as and    # ... and everyone not in the current AS (or not running BGP)
           (rr_mesh or not ngb.bgp.get('rr', None)) # Skip other RRs if rr_mesh is False
                    ]
  else:
    ibgp_ngb_list = rrlist

  for n in ibgp_ngb_list:
    n_intf = get_remote_ibgp_endpoint(n)
    neighbor_data = bgp_neighbor(n,n_intf,'ibgp',sessions,get_neighbor_rr(n))
    if not neighbor_data is None:
      if 'loopback' in node:
        neighbor_data._source_intf = node.loopback
      if bgp_nhs:
        neighbor_data.next_hop_self = 'ebgp'
      if is_rr and not 'rr' in neighbor_data:
        neighbor_data.rr_client = True
      node.bgp.neighbors.append(neighbor_data)
      has_ibgp = True

  if not has_ibgp:
    return

and also add to bgp.yml

attributes:
  global:
    rr_mesh: bool  #add mesh between rr

-----
       node:
    rr_mesh:                     #add rr_mesh

[ipspace]

I noticed something strange: when we have more than one route reflector in the topology, they try to establish sessions with each other—even without a direct link between them. I don't think that's the intended behavior.

It is. That's how route reflectors were designed to be used (see https://blog.ipspace.net/2022/02/bgp-rr-myths/ for details); the only scenario where you do not need an IBGP session between them is when you run them on spine nodes in a leaf-and-spine fabric.

I propose adding an rr_mesh option to control this process.

I would prefer an opinionated plugin (for example, bgp.dc) that would remove IBGP sessions between route reflectors and set extended BGP community propagation on all sessions.

Any thoughts @jbemmel @ssasso?

[jbemmel]

wouldn’t break the existing setup but would introduce flexibility for cases like mine.

It adds flexibility but also complexity - because now there is an additional variable ( to be documented, tested, etc. )

It is easy to add nerd knobs - and we already have so many, what harm could one more do?

This is the constant tension we run into

[ipspace]

This is the constant tension we run into

Well said!

@aeangel -- it's not that anyone is saying "we won't do that", it's just that (as @jbemmel explained) adding yet another nerd knob into the core code triggers all sorts of things, including potential edge cases that will bite use later. It's much better to isolate niche functionality (whenever possible) into a plugin.

[aeangel]

Thank you for the explanations. I’m really not very experienced in programming or in the internal architecture of Netlab, so I might underestimate the complexity of adding new options into the core.
My intention was simply to share an idea that seemed useful in my case. I understand that every new “nerd knob” requires documentation, testing, and can add unnecessary complexity.

[jbemmel]

@aeangel unrelated - was reading through your labs and wondered if you had seen https://github.com/ipspace/netlab/tree/dev/netsim/extra/mlag.vtep

It handles the configuration of a shared VTEP ip in mlag scenarios

Any thoughts on what might have helped you find it?

[aeangel]

@jbemmel I can’t say for sure whether I came across plugin mlag.vtep. However, my intention was to dig deeper and demonstrate how the custom config works. My main source for studying the documentation has been the official website.

[jbemmel]

I was looking back through past issues to see if I raised something similar before - it felt familiar

Happy to see I’m not the only opinionated one…

[aeangel]

Thanks — it’s reassuring to know I’m not the only one. Hopefully, with enough thoughtful discussion, we might convince the maintainers to consider it.

[ipspace]

@ssasso @jbemmel -- assuming we don't want to have yet-another plugin for yet-another nerd knob, would you prefer having this one in:

• fabric plugin, because it deals with leaf-and-spine fabrics which are the primary use case for this unorthodox RR design, or
• bgp.domain plugin, because it already prunes IBGP sessions, so it would be a perfect fit for some more pruning.

In both cases, I'd go for bgp.rr_mesh boolean node attribute (giving the users all the rope they need to irreparably entangle themselves into RR knots 🤪).

[jbemmel]

I think fabric would be easier to find for users who care about this knob

[ipspace]

Implemented in #2847