Skip to content

Implement BGP paths limit capability#1387

Closed
jamestiotio wants to merge 1 commit intoExa-Networks:mainfrom
jamestiotio:main
Closed

Implement BGP paths limit capability#1387
jamestiotio wants to merge 1 commit intoExa-Networks:mainfrom
jamestiotio:main

Conversation

@jamestiotio
Copy link
Copy Markdown
Contributor

Implement the capability as specified by the draft-abraitis-idr-addpath-paths-limit-04 document.

Corresponding unit tests are also added to validate the functionality of this feature.

@jamestiotio
Copy link
Copy Markdown
Contributor Author

This closes #1218. First time contributing to ExaBGP, so apologies for any beginner mistakes.

@thomas-mangin
Copy link
Copy Markdown
Member

Thank you @jamestiotio
I will look into it in the next few days.

Implement the capability as specified by the `draft-abraitis-idr-addpath-paths-limit-04` document.

Corresponding unit tests are also added to validate the functionality of this feature.

Signed-off-by: James Raphael Tiovalen <jamestiotio@meta.com>
@thomas-mangin
Copy link
Copy Markdown
Member

The code was merged but then changed to match a different configuration format (and some issues were found by Claude which resolved them).

@thomas-mangin
Copy link
Copy Markdown
Member

Only for information, the findings were:

🔴 prefix_index() only implemented for INET

NLRI.prefix_index() falls back to self.index(), which includes the path-ID. For VPN, EVPN, FlowSpec, and other AddPath-capable families, paths-limit enforcement and audit will silently malfunction (each path-ID treated as a separate prefix, so the limit is never hit). Either implement prefix_index() on other NLRI subclasses or document this as an IPv4/IPv6 unicast-only feature.

🟡 Unbounded memory in _path_counts/_path_warned

IncomingRIB._path_counts grows with every unique prefix seen over the session lifetime. Withdrawn prefixes are cleaned up, but a peer that announces-then-withdraws millions of distinct prefixes will accumulate warned-set entries until clear(). Not critical since audit is optional, but worth noting.

🟡 unpack_capability loop repeats data = data[5:] three times

pathslimit.py:250,256,259 - could restructure to slice once at loop end. Minor readability nit.

🟡 Configuration output in neighbor.py:351-354

The paths-limit block emission mixes + concatenation with f-string implicit concatenation. Works, but fragile if someone later edits the surrounding f-string.

@thomas-mangin
Copy link
Copy Markdown
Member

Thank you for your help.

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