Hi Luuk,
First and foremost, thanks for your comments. Inline:
On 16/1/23 13:35, Luuk Hendriks wrote:
[ .. ]
General:
- For stateless parsing, I think all the necessary information (Four
Octet Capable, AddPath, Multiple Labels) should come before the BGP
PDU starts. I am strongly in favour of using the peer flags in the
Per-Peer Header for that, and not TLVs. And as such, I think we
should introduce a bit for AddPath, and one for Multiple Labels.
Ack, i have also matured similar feelings. Let's see if this will be
part of the -bis document we intend to write with Tim (makes sense to
me) or we should seek a different placement.
- Regarding bumping the version: I think this draft describes changes
that do not require a version bump (i.e. the new flags as described
above), while others perhaps do (i.e. changing the RouteMonitoring
wireformat). A version bump should have maximum visibility and as such
either a -bis or a new document might be the better place. That would
unfortunately mean the ebit and path marking drafts will be stalled as
they depend on the TLVs, if I recall things correctly. But it might
prevent a lot of confusion later on, especially for new implementers.
I think we have two main ways to proceed. Add support for TLVs to
existing Route Monitoring message and bump the version for backward
compatibility or create a new Route Monitoring message with support for
TLVs and not execute the bump - i'd definitely not conflate this with a
-bis that can take quite some time to pass through.
Among the two solutions I am myself OK with both. I prefer what we do
already have in place but i am also here to serve the community and if
there is a strong preference elsewhere, i'd have zero issue in changing
the direction. Chair(s) did express a favor in the current direction in
the past and neither of these two solutions would stall ebit and path
marking drafts.
Looking forward to further feedback on this aspect.
Per section:
Sec 3:
- For indexed TLVs, the text is not explicit about whether the length
field includes the 2 bytes for the index field, or describes the
length of only the value.
I don't see a benefit of one over the other, as long as the text is
clear about it.
Section 3 does refer to Section 4.4 of RFC7854 where the length field is
defined as "The length of the following Information field, in bytes.".
Do you feel this is under defined?
- If an indexed TLV for a specific NLRI (index > 0) exists alongside a
TLV with index 0, does the specific TLV trump the generic one for that
NLRI, or does it compliment the generic one?
I'd assume the latter.
I'd tend to say that this should not be specified by the tlv draft and
rather be left as application specific. For example, a path marking or a
REL draft would specify that.
Sec 4.2
- Do we really want the BGP PDU TLV to be possibly preceded by other
TLVs? Are there any examples of when this would be useful, other than
TLVs containing info needed to correctly parse the BGP PDU (which we
should not want anyway in my opinion)?
For use cases where one just wants the BGP PDU, being able to bail out
as early as possible sounds useful.
Or another way to think about this: the BGP PDU TLV is a mandatory TLV
for the RouteMonitoring message. To me it makes sense to first list the
mandatory TLVs, then the optional ones.
This was the original thinking behind Route Mirroring in RFC7854. Hence
i wanted to get this new proposal to be consistent to something that had
already been (successfully) proposed. What are the arguments against it?
4.2.1
- I had hard time figuring out how the Group TLV was supposed to work,
until I looked back at some older mails on the list. If my
understanding below is correct, I'm happy to propose some text.
The first two bytes in the value of the Group TLV are a new, made up
index, representing a group of NLRIs. The remainder of the value are
the indices of the actual NLRI in the BGP PDU that belong to the
group. Then, other, 'normal' TLVs can point to the newly made up
index, and carry a value that applies to all the NLRI listed in the
Group TLV.
To confirm that we are in sync.
If that is the correct interpretation, some points/questions I think
should be answered in the text:
- the text might need an additional MUST, stating the newly made up
index MUST be unique for that RouteMonitoring message.
Added text.
- can one NLRI appear in more than one Group TLV? (I assume yes)
Yes. Added text.
- do the Group TLVs appear before or after the normal TLVs? If we do
enforce sorting by code points, we could steer this behavior by
picking a certain code point.
I was not thinking to make Group TLVs somehow special compared to any
other TLVs that we may define later such that ordering would matter.
- do the indices in the Group TLV have to be sorted? This might be
convenient for parsing/processing at the BMP station.
Added text, i felt to go with a SHOULD but we can review this.
- can multiple TLVs point to a Group TLV index? (I assume yes)
Yes. Do you believe text would be needed for this?
- can Group TLV indices appear in the pointed-to indices in another
Group TLV, i.e. nested grouping? (I'm not sure this makes things
easier for anyone in the end, so if there is no convincing use-case
for it, perhaps it should be explicitly forbidden?)
Good one, i agree we could start with explicitely forbidding this and
then re-visiting if anybody comes with a use-case around it. Added text.
For my own understanding I created this example RouteMonitoring message
(in sort of pseudo-wireformat if you will) comprising:
- the usual CommonHeader and PerPeerHeader
- the BGP PDU in the first TLV, containing 10 NLRI
- two Group TLVs, pointing to four and three actual TLVs, respectively
- one TLV pointing to a single NLRI (NLRI number 7)
- one TLV pointing to the first Group TLV
- one TLV pointing to the second Group TLV
RouteMonitoring
CommonHeader
PerPeerHeader
TLVs
[type=TBD1, length=x, index=0, value=$BGP_PDU{ NLRI1, .. NLRI10 } ]
[type=TBD2, length=x, index=0, value={0x0001, 0x0002, 0x0003, 0x000a}]
[type=TBD2, length=x, index=0, value={0x0004, 0x0005, 0x0006}]
[type=TBDw, length=x, index=7, value=something_that_applies_to_NLRI7]
[type=TBDx, length=x, index=0, value=something_that_applies_to_all]
[type=TBDy, length=x, index=0x000b, value=applies_to_four_nlri]
[type=TBDz, length=x, index=0x000c, value=applies_to_three_nlri]
If this is correctly interpreted and somehow useful for the draft, I'm
happy to make this into a proper diagram.
Please, that contribution would be very much appreciated!
4.2.2
As mentioned, I think these TLVs should go in favour of flags in the
Per-Peer header.
Ack!
4.3
Wouldn't it make sense to wrap the BGP PDU in a TLV (for reason codes 1
and 3), for the same reasons we want to wrap the PDU in RouteMonitoring
messages?
Good point and, again as i may have already stated, my purpose was to
try to be consistent to what already exists.
Text was added in commit
https://github.com/paololucente/draft-ietf-grow-bmp-tlv/commit/0278888e2c615d63b72ac7a851b6efabfb03c932
. Just let me know any additions / deletions / changes. Otherwise these
are good to go for upcomign -11 revision!
Paolo
_______________________________________________
GROW mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/grow
