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 ipv6 nexthop-local unchanged #17037

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

Conversation

louis-6wind
Copy link
Contributor

@louis-6wind louis-6wind commented Oct 8, 2024

#16219 attempted to solve #16198 issue but removed ipv6 link-local nexthop in situation it was legitimate. For example, when an ipv4-mapped IPv6 address is present in global IPv6 nexthop, the link-local is also useful if the peer does not understand the ipv4-mapped IPv6 address format.

Revert #16219 and apply fixes to the "ipv6 nexthop-local unchanged" behavior. When the parameter is present:

  • don't insert a link-local when the origin peer does not send any link-local
  • reset the link-local of the origin peer to the local router link-local when the origin and destination peer does not share the same network

Add a topotest to check the ipv6 nexthops in various situations including the use case from #16219 (comment) and #17037 (comment)

@@ -0,0 +1,23 @@
router bgp 65000
Copy link
Contributor

Choose a reason for hiding this comment

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

All the peers here are iBGP. We are seeing the issue when the prefix is advertised towards an eBGP peer, still on the same network. Because they still share same network, we expect the RR to leave the NH unchanged, which it does, but it also inserts a link-local.

Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of debugging, if we enable BGP update debugging we can see the incoming update without a link-local and then us (RR) sending it with a link-local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For your use-case, you will need "neighbor XXXX nexthop-local unchanged". If unset, it will keep inserting the link-local from the route-reflector. This is what "neighbor XXXX nexthop-local unchanged" is designed for.

Copy link
Contributor

Choose a reason for hiding this comment

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

That also doesn't (or didn't) work to prevent the (F)RR adding in it's link-local. Although we could live with using it this way, it is different behaviour to IPv4 case where nexthop is unchanged when all iBGP and eBGP neighbours share the same segment without any additional flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see the proposed change. Thank you for the solution!

@louis-6wind louis-6wind marked this pull request as ready for review October 9, 2024 14:52
@louis-6wind louis-6wind changed the title bgpd: check route-reflector on the same network. Revert 16219 bgpd: fix ipv6 nexthop-local unchanged Oct 9, 2024
Do not add an IPv6 link-local nexthop if the originating peer does not
provide one and the nexthop-local unchanged setting is enabled.

Signed-off-by: Louis Scalbert <[email protected]>
If the "nexthop-local unchanged" setting is enabled, it preserves the
IPv6 link-local nexthop from the originating peer. However, if the
originating and destination peers are not on the same network segment,
the originating peer's IPv6 link-local address will be unreachable from
the destination peer.

In such cases, reset the IPv6 link-local nexthop, even if "nexthop-local
unchanged" is set on the destination peer.

Signed-off-by: Louis Scalbert <[email protected]>
Add bgp_nexthop_ipv6 to check the ipv6 link-local nexthop conformity in
several situations.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants