Add bgp.role support to bgp.policy plugin#3429
Conversation
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.
| """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).
There was a problem hiding this comment.
I've refactored the PR to propose the following structure:
bgp.role: customer
or
bgp.role:
name: customer
strict: true
The must_be_bgp_role etc validation functions could be replaced by more generic validation code (some sub cases of user defined type validation aren't implemented, see #3430
|
@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. |
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>
Replace bgp.role_strict with an optional strict flag inside bgp.role, normalize neighbors to a role dictionary, add global defaults.bgp.role.strict inheritance, and merge the three integration tests into 70-bgp-roles.yml. Co-authored-by: Cursor <cursoragent@cursor.com>
jbemmel
force-pushed
the
feature/bgp-role-policy
branch
from
d4b9333 to
0ce91d1
Compare
|
|
||
| _requires = [ 'bgp' ] | ||
| _requires = [ 'bgp' ] | ||
| _execute_after = [ 'bgp.session' ] |
There was a problem hiding this comment.
AI added this following documented comments - let me know if you prefer to leave it out
Define bgp_role_name once in plugin defaults and extend validate.py so type: and _alt_types references resolve user-defined schemas; drop the custom must_be_bgp_role validator and stop propagating defaults.bgp.role. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove unused modules import and avoid a # type: comment that mypy misread when resolving user-defined attribute types. Co-authored-by: Cursor <cursoragent@cursor.com>
Stock _alt_types only invoke registered must_be_* functions (see ipspace#3430). Register bgp_role and bgp_role_strict in bgp.policy; keep valid_values in defaults.yml via a YAML anchor. Co-authored-by: Cursor <cursoragent@cursor.com>
Merge inherited role onto the interface before apply_policy_attributes; drop the separate role.attr list and special-case neighbor handling. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Consolidate BGP role handling into bgp.policy as bgp.role (with optional bgp.role.strict) instead of a separate plugin, with FRR/BIRD support, tests, and documentation.
Replaces #3428
New proposed attribute structure would benefit from core validation changes - see #3430.