bgpd: Set nexthop length to 16 bytes for IPv4 family prefixes

[ton31337]

This is the issue between other vendors with unnumbered sessions.
I tested with vQFX-re and got this notification from JunOS:

NOTIFICATION sent to fe80::a00:27ff:fec4:4b3e (External AS 65002): code 3 (Update Message Error) subcode 9 (error with optional attribute)

With this fix the session is UP and all prefixes received on JunOS side:

root@vqfx-re> show route 172.16.255.253/32 detail

inet.0: 13 destinations, 15 routes (13 active, 0 holddown, 0 hidden)
172.16.255.253/32 (1 entry, 1 announced)
        *BGP    Preference: 170/-101
                Next hop type: Router, Next hop index: 342
                Address: 0xc65eb68
                Next-hop reference count: 16
                Source: fe80::a00:27ff:fec4:4b3e
                Next hop: fe80::a00:27ff:fec4:4b3e via em1.0, selected
                Session Id: 0x0
                State: <Active Ext>
                Local AS: 65004 Peer AS: 65002
                Age: 4:59       Metric: 0
                Validation State: unverified
                Task: BGP_65002.fe80::a00:27ff:fec4:4b3e
                Announcement bits (2): 0-KRT 1-Resolve tree 1
                AS path: 65002 ?
                Accepted
                Localpref: 100
                Router ID: 2.2.2.2

{master:0}

Related: #6433

Signed-off-by: Donatas Abraitis donatas.abraitis@gmail.com

[LabN-CI]

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/6579 330c60b
Date	06/13/2020
Start	14:59:25
Finish	15:25:17
Run-Time	25:52
Total	1815
Pass	1815
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2020-06-13-14:59:25.txt
Log	autoscript-2020-06-13-15:00:24.log.bz2
Memory	461 483 425

For details, please contact louberger

[NetDEF-CI]

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12666/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Debian 10 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 10 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-12666/artifact/DEB10BUILD/ErrorLog/log_lintian.txt)

W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.4.1 (current is 4.3.0)
W: frr source: pkg-js-tools-test-is-missing
W: frr source: newer-standards-version 4.4.1 (current is 4.3.0)
W: frr-rpki-rtrlib: changelog-file-missing-explicit-entry 6.0-2 -> 7.5-dev-20200613-00-g330c60ba7-0 (missing) -> 7.5-dev-20200613-00-g330c60ba7-0~deb10u1
W: frr-doc: changelog-file-missing-explicit-entry 6.0-2 -> 7.5-dev-20200613-00-g330c60ba7-0 (missing) -> 7.5-dev-20200613-00-g330c60ba7-0~deb10u1
W: frr-snmp: changelog-file-missing-explicit-entry 6.0-2 -> 7.5-dev-20200613-00-g330c60ba7-0 (missing) -> 7.5-dev-20200613-00-g330c60ba7-0~deb10u1
W: frr-pythontools: changelog-file-missing-explicit-entry 6.0-2 -> 7.5-dev-20200613-00-g330c60ba7-0 (missing) -> 7.5-dev-20200613-00-g330c60ba7-0~deb10u1
W: frr: changelog-file-missing-explicit-entry 6.0-2 -> 7.5-dev-20200613-00-g330c60ba7-0 (missing) -> 7.5-dev-20200613-00-g330c60ba7-0~deb10u1

[donaldsharp]

I am not sure I agree with making this change. We are sending data correctly as per the RFC and Juniper must accept it.

[ton31337]

I agree with you, that is an incorrect behavior on JunOS, this is kinda for a discussion how should we treat that.

[donaldsharp]

I have heard through the official routing grapevine that Juniper is going to fix the issue on their end.

[adam-kulagowski]

As an author of #6433 I believe this a kind gray area:

• we have no global IPv6 on link (just the link-local). In result we are sending the same NH twice.
• there is no MUST keyword in RFC

Yes according to the https://tools.ietf.org/html/rfc2545#section-3 The FRR behavior is correct (no question asked) but RFC states:

   The link-local address shall be included in the Next Hop field if and
   only if the BGP speaker shares a common subnet with the entity
   identified by the global IPv6 address carried in the Network Address
   of Next Hop field and the peer the route is being advertised to.

Although there is no MUST/SHOULD sending just one IPv6 NH might OK even in this case. Not handling double NH (and in result breaking the BGP session - thats another story :) )

Maybe we can make this somehow adjustable via BGP export policy? I would be more than happy if was able to set ipv6 next-hop local in outgoing direction (or anything else)

[ton31337]

I don't have how to test with Arista, but seems they had the same problem. I hope it's already fixed.

[adam-kulagowski]

@ton31337

I don't have how to test with Arista, but seems they had the same problem. I hope it's already fixed.

I can confirm that latest Arista in VM does not suffer from this issue.

@donaldsharp That's great news. Was any date/roadmap/PR given?

[ton31337]

Seems brilliant, I'm closing this PR then.

[ton31337]

According https://tools.ietf.org/html/draft-white-linklocalnh-00:

If an implementation intends to send a single link local forwarding
address in the Next Hop field of the MP_REACH_NLRI, it MUST set the
length of the Next Hop field to 16 and include only the IPv6 link
local address in the Next Hop field.

This PR seems what it does with, except without intends, but forcefully for IPv4 prefixes. Maybe we should adopt with set ipv6 next-hop local?