fixes: correct BGP-LS NLRI marshalling and IPv6 Router-ID TLV length - #3606
aronjohanns wants to merge 2 commits into
Conversation
| LsTLV: LsTLV{ | ||
| Type: LS_TLV_IPV6_LOCAL_ROUTER_ID, | ||
| Length: 0, | ||
| Length: 16, |
There was a problem hiding this comment.
NewLsTLVRemoteIPv6RouterID and NewLsTLVOpaquePrefixAttr, and NewLsTLVPrefixSID have the same bug (LsTLV's wrong length). Please fix them together.
There was a problem hiding this comment.
Fixed all three in the same commit and retitled it since it now covers all four constructors.
| v6 := netip.MustParseAddr("2001:db8::1") | ||
| assert.EqualValues(t, 4, NewLsTLVLocalIPv4RouterID(&v4).Len()-tlvHdrLen) | ||
| assert.EqualValues(t, 16, NewLsTLVLocalIPv6RouterID(&v6).Len()-tlvHdrLen) | ||
| } |
There was a problem hiding this comment.
Better to test serializing LsTLVLocalIPv6RouterID?
b, err := NewLsTLVLocalIPv6RouterID(&v6).Serialize()
assert.NoError(t, err)
assert.Len(t, b, 20)
There was a problem hiding this comment.
Done, replaced Len() checks with Serialize() for all four RouterID TLVs plus PrefixSID and OpaquePrefixAttr.
|
The commit message says "Serialize writes the real value length, and the two So with Length 0 and a 16-octet address, Serialize fails. On master a BGP-LS The fix is right, but the bug is worse than the message says. |
NewLsTLVLocalIPv6RouterID, NewLsTLVRemoteIPv6RouterID, NewLsTLVPrefixSID and NewLsTLVOpaquePrefixAttr set Length to 0 (4 for the remote IPv6 Router-ID) instead of the length of the value they carry. LsTLV.Serialize rejects a value whose length does not match the Length field, so a BGP-LS attribute carrying any of these TLVs failed to serialize entirely, and Len() understated the descriptor length that NewLsTLVNodeDescriptor sums over its sub-TLVs. Set the IPv6 Router-IDs to the 16 octets RFC 6119 section 4.1 gives them, the Prefix-SID to the 8 octets of its fixed part with a 4-octet SID, and the opaque prefix attribute to the length of its payload. Co-Authored-By: Claude Fable 5 <[email protected]>
MarshalNLRI switches on the concrete Link-State NLRI and had no default branch. An LsAddrPrefix carrying a type the switch does not know, or no NLRI at all, was returned as an LsAddrPrefix whose Nlri field is nil. The gRPC client then sees a route with a type and length but nothing else, and cannot tell that from a decoding bug on its side. Report it as an error instead, as the other address families in this function do. Co-Authored-By: Claude Fable 5.1 <[email protected]>
ea75102 to
b32aeaa
Compare
Ok, I rewrote the commit message to state that |
|
Merged, thanks. |
Two small BGP-LS fixes split out of the work in #3595.