Add bgp.role support to bgp.policy plugin#3429
Conversation
Consolidate BGP role handling into bgp.policy as bgp.role and bgp.role_strict instead of a separate plugin, with FRR/BIRD support, tests, and documentation. Co-authored-by: Cursor <cursoragent@cursor.com>
Check device support first, inline role_strict validation, and restrict role application to EBGP neighbors only. Co-authored-by: Cursor <cursoragent@cursor.com>
Only validate device support after confirming bgp.role or bgp.role_strict is configured, avoiding false errors on topologies using bgp.policy without roles. Co-authored-by: Cursor <cursoragent@cursor.com>
ipspace
left a comment
ipspace
left a comment
There was a problem hiding this comment.
The code needs to be cleaned up. Also, we're not running three integration tests for a simple feature. Squeeze whatever you want to have tested into one.
| * Arista EOS and Aruba CX do not support node-level default local preference. Node-level **bgp.locpref** attribute (if specified) is thus applied to all interfaces that do not have an explicit **bgp.locpref** attribute. That might interfere with the **bgp.policy** interface attributes. | ||
| * FRR implements BGP Roles starting with release 8.4. BIRD implements them starting with release 2.0.11. On BIRD, BGP roles are rendered into the BGP module configuration file (`daemons/bird/bgp.j2`). | ||
|
|
||
| (plugin-bgp-policy-role)= |
There was a problem hiding this comment.
I would move this section to the end (just before the "Sample Topologies")
| api.node_config(node,_config_name) # Remember that we have to do extra configuration | ||
| _bgp.clear_bgp_session(node,ngb) | ||
|
|
||
| def _use_role_plugin_template(node: Box) -> bool: |
There was a problem hiding this comment.
This feels so wrong. Why would you ever hard-code a device into a plugin code?
| """Return False for the BIRD daemon (roles are rendered in daemons/bird/bgp.j2).""" | ||
| return not (node.get('_daemon') and node.device == 'bird') | ||
|
|
||
| def apply_role_attributes(node: Box, ngb: Box, intf: Box, topology: Box) -> bool: |
There was a problem hiding this comment.
This feels like duplicating a lot of functionality that should be already available elsewhere. If you want to ensure "role_strict" is only used when "role" is there, we could solve that in the generic validation code (if it's not there yet).
|
|
||
| route_aggregation(ndata,topology) | ||
| _bgp.cleanup_neighbor_attributes(ndata,topology,_attr_list + [ 'policy' ]) | ||
| _bgp.cleanup_neighbor_attributes(ndata,topology,_attr_list + _ROLE_ATTR_LIST + [ 'policy' ]) |
There was a problem hiding this comment.
It would be better to add role keywords into "_direct" or "_compound" attribute list? Is there a reason not to do it?
| communities.append('extended') | ||
| if copy_locpref and not intf.get('bgp.locpref',False): | ||
| intf.bgp.locpref = ndata.bgp.locpref | ||
| apply_role_attributes(ndata,ngb,intf,topology) |
There was a problem hiding this comment.
It looks like the only reason you're dealing with roles as an exception is because of Bird template stuff. The easiest way to solve that is to have an empty template in the plugin directory.
|
@jbemmel -- I understand you find it exciting to use AI to generate code, but I won't review the bloated results in the future (like the special "dealing with role" function). Please act as a sanity check on the code before submitting the PR. |
2 participants
Consolidate BGP role handling into bgp.policy as bgp.role and bgp.role_strict instead of a separate plugin, with FRR/BIRD support, tests, and documentation.
Replaces #3428