Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: fix for the validity and the presence of prefixes in the BGP VPN table. #17370

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

louis-6wind
Copy link
Contributor

Fix for the validity and the presence of prefixes in the BGP VPN table.

See individial commit logs

@frrbot frrbot bot added bgp tests Topotests, make check, etc labels Nov 6, 2024
@louis-6wind louis-6wind marked this pull request as draft November 6, 2024 15:50
@riw777 riw777 self-requested a review November 12, 2024 14:57
@frrbot frrbot bot added the bugfix label Nov 14, 2024
@louis-6wind louis-6wind marked this pull request as ready for review November 15, 2024 16:45
@louis-6wind louis-6wind marked this pull request as draft November 21, 2024 10:23
@louis-6wind louis-6wind force-pushed the fix-vpn-leaking branch 2 times, most recently from b4c2e9b to dc3a481 Compare November 25, 2024 14:02
@louis-6wind
Copy link
Contributor Author

ci:rerun

1 similar comment
@louis-6wind
Copy link
Contributor Author

ci:rerun


if (attrhash_cmp(bpi->attr, new_attr) && labelssame &&
!CHECK_FLAG(bpi->flags, BGP_PATH_REMOVED) &&
!!leak_update_nexthop_valid(to_bgp, bn, new_attr, afi, safi, source_bpi, bpi,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: leak_update_nexthop_valid() is already bool, no need for !!.

@louis-6wind
Copy link
Contributor Author

ci:rerun

@louis-6wind louis-6wind marked this pull request as ready for review November 28, 2024 15:25
Copy link

github-actions bot commented Dec 3, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@riw777
Copy link
Member

riw777 commented Dec 3, 2024

conflicts need to be resolved and @ton31337 's comment addressed before we can push ...

They are the same value.

Signed-off-by: Louis Scalbert <[email protected]>
Mark a nexthop as invalid if the origin VRF is unusable, either because
it does not exist or its interface is down.

Signed-off-by: Louis Scalbert <[email protected]>
When leak_update() rechecks an existing path, it considers nothing to
update if the attributes and labels are not changed. However, it does
not take into account the nexthop validity.

Perform a leak update if the nexthop validity has changed.

Signed-off-by: Louis Scalbert <[email protected]>
Do not leak if origin vrf is not usable

Signed-off-by: Louis Scalbert <[email protected]>
Update route_leak_basic tests. The routes with an unusable nexthop VRF
are no more present in the RIB.

Signed-off-by: Louis Scalbert <[email protected]>
The bgp_bmp test is failing because r2 lacks the vrf1 VRF, which
prevents it from exporting VPN prefixes from the configured vrf1 BGP
instance. Previous versions allowed the export of static BGP prefixes
from a non-existent VRF, so the test passed under those conditions.

Add a vrf1 VRF on r2.

Fixes: d748544 ("topotests: add basic bgp bmp test")
Signed-off-by: Louis Scalbert <[email protected]>
Check bgp vpn table in bgp_vpnv4_route_leak_basic

Signed-off-by: Louis Scalbert <[email protected]>
In bgp_vpnv4_route_leak_basic, remove and add back the static prefix
172.16.3.0/24 on VRF DONNA. Before the previous fixes, the 172.16.3.0/24
prefix re-appeared when it was added back in the VPN table but it was
marked as invalid.

Signed-off-by: Louis Scalbert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants