bgpd, tests: don't send local nexthop from rr client

[louis-6wind]

Depends on #17071 merge

AS 65000  | AS 65001
          |
      RR  |
       |  |
R1 --- | --- R2
          |

When r1 peer is an iBGP route reflector client of rr and r2 peer is a
eBGP neighbor of rr, and all three routers shares the same network, r2
receives announcements coming from r1 with a IPv6 link-local nexthop
from rr. This is incorrect as r2 should send traffic to r1 without
involving rr.

Do not send an IPv6 link-local nexthop if the originating peer is a
route-reflector client.

Link: #16219 (comment)
Link: #17037 (comment)

[leonshaw]

Should LL nexthop be included if peer R2 has next-hop-self configured on RR?

[louis-6wind]

Should LL nexthop be included if peer R2 has next-hop-self configured on RR?

Link-Local nexthop is not mandatory. As soon as the next-hop-self option sets the global nexthop correctly, the route-reflector will be the nexthop as expected.

The next-hop-self case is not taken into account even in the other situations (route-server...). If it matters for you, feel free to create a pull request with the appropriate topotests

[leonshaw]

Thanks for the clarification.
In above case the routers happens to share the same network, so the next-hop is unchanged. But if RR-R1 and RR-R2 are different networks, since R2 is an eBGP peer of RR, RR will implicitly change the next-hop to itself when advertising to R2. Is it better to append a link-local next-hop for this case?
IMO the key point is not whether the route is received from RR client, but whether the next-hop is self. (except for nexthop-local unchanged)

[louis-6wind]

Thanks for the clarification. In above case the routers happens to share the same network, so the next-hop is unchanged.

That is correct.

But if RR-R1 and RR-R2 are different networks, since R2 is an eBGP peer of RR, RR will implicitly change the next-hop to itself when advertising to R2. Is it better to append a link-local next-hop for this case?

It would better to append a link-local nexthop for sure. But technically, I cannot know in subgroup_announce_check() whether the originating (from peer) and the destination peer are in same network. So I have no other options than not sending any IPv6 link-local nexthop in the route-reflector case.

Indeed, in subgroup_announce_check(), peer references one of the peer of the BGP subgroup. And a BGP subgroup can include peers of different subnets. For the option nexthop-local unchanged, peers from different subnets are no more included in the same BGP subgroup since #17071. But we don't want to split subgroup by network subnets in the other cases to not impact the performance.

Conclusion: since the IPv6 Link-local nexthop is optional, we never include for prefix originating from a route-reflector client unless
nexthop-local unchanged is set on the destination peer.

IMO the key point is not whether the route is received from RR client, but whether the next-hop is self. (except for nexthop-local unchanged)

Again, this issue is out of scope of this pull-request.

[leonshaw]

And a BGP subgroup can include peers of different subnets.

I think this reveals a more fundamental problem: in current implementation, a subgroup can't guarantee the number of next-hops is the same for all peers. (As an example, we've recently hit this when enabling IPv6-unicast AF between IPv4 peers. Due to some timing issue, some link-local address has not been processed when peer is establishing. Resulting in some peers have valid nexthop.v6_local while some don't - in the same subgroup.)

Again, this issue is out of scope of this pull-request.

I agree this PR is not intended to solve them all, but I don't think it's a good approach to fix for specific cases here and there. I also don't see a strong connection between RR client and link-local next-hop (it's almost the same if R1 is an eBGP peer of RR, provided the three share the same network).

Maybe could postpone the decision of LL next-hop till concrete peer processing? Or just disable LL next-hop whenever we are not confident enough.

[louis-6wind]

And a BGP subgroup can include peers of different subnets.

I think this reveals a more fundamental problem: in current implementation, a subgroup can't guarantee the number of next-hops is the same for all peers. (As an example, we've recently hit this when enabling IPv6-unicast AF between IPv4 peers. Due to some timing issue, some link-local address has not been processed when peer is establishing. Resulting in some peers have valid nexthop.v6_local while some don't - in the same subgroup.)

Please open an issue with more details.

I have seen an issue with global but not with link-local so far. I have a fix for that ed7710f

May be a similar issue with link-local ?

Again, this issue is out of scope of this pull-request.

I agree this PR is not intended to solve them all, but I don't think it's a good approach to fix for specific cases here and there. I also don't see a strong connection between RR client and link-local next-hop (it's almost the same if R1 is an eBGP peer of RR, provided the three share the same network).

Maybe could postpone the decision of LL next-hop till concrete peer processing? Or just disable LL next-hop whenever we are not confident enough.

We will see with the issue

[leonshaw]

May be a similar issue with link-local ?

Yes, it's similar. Link-local next-hop is not updated in bgp_interface_address_add().

We will see with the issue

I will open an issue later.

[riw777]

looks good

[leonshaw]

Hi, I opened a discussion #17128 (for general link-local next-hop handling). Please have a look.