-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 prefix same as nexthop in label per nexthop #16990
base: master
Are you sure you want to change the base?
Conversation
When a prefix is imported using the "network" command under a vrf, which is a connected prefix, and in the context of label allocation per nexthop: .. >router bgp 1 vrf vrf1 > address-family ipv4 unicast > redistribute static > network 172.16.0.1/32 <--- connected network > network 192.168.106.0/29 > label vpn export auto > label vpn export allocation-mode per-nexthop .. We encounter an MPLS entry where the nexthop is the prefix itself: > 18 BGP 172.16.0.1 - Actually, when using the "network" command, a bnc context is used, but it is filled by using the prefix itself instead of the nexthop for other BGP updates. Consequently, when picking up the original nexthop for label allocation, the function behaves incorrectly. Instead ensure that the nexthop type of bnc->nexthop is not a nexthop_ifindex; otherwise fallback to the per vrf label. Update topotests. PR: 91877 Signed-off-by: Loïc Sang <loic.sang@6wind.com>
WARNING: topo: Waiting time is too small (count=10, wait=0.5), using default values (count=20, wait=3) supress warning by inscreasing wait time. Signed-off-by: Loïc Sang <loic.sang@6wind.com>
@@ -1598,6 +1598,16 @@ vpn_leak_from_vrf_get_per_nexthop_label(afi_t afi, struct bgp_path_info *pi, | |||
return bgp_mplsvpn_get_vpn_label(&from_bgp->vpn_policy[afi]); | |||
} | |||
|
|||
if (is_bgp_static_route && | |||
pi->nexthop->nexthop->type == NEXTHOP_TYPE_IFINDEX) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not check against pi->sub_type being BGP_TYPE_REDISTRIBUTED? Also, aggregated routes should be taken into account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to target only the manually added prefixes using the "network" command in BGP (bgp_static_set
). I am not considering routes that were redistributed from other protocols or created through aggregation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant BGP_TYPE_STATIC, not redistributed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_bgp_static_route = bpi_ultimate->sub_type == BGP_ROUTE_STATIC &&
› › bpi_ultimate->type == ZEBRA_ROUTE_BGP;
The check is made in is_bgp_static_route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then what is the reason of this check pi->nexthop->nexthop->type == NEXTHOP_TYPE_IFINDEX
?
When a prefix is imported using the "network" command under a vrf, which
is a connected prefix, and in the context of label allocation per
nexthop:
..
We encounter an MPLS entry where the nexthop is the prefix itself:
Actually, when using the "network" command, a bnc context is used, but
it is filled by using the prefix itself instead of the nexthop for other
BGP updates. Consequently, when picking up the original nexthop for
label allocation, the function behaves incorrectly. Instead ensure that
the nexthop type of bnc->nexthop is not a nexthop_ifindex; otherwise
fallback to the per vrf label.
Update topotests.