Hi Tim,
Thanks for taking the time to review the draft, much appreciated. Some
comments inline:
On 13/10/22 01:41, Tim Evens (tievens) wrote:
Hi Paolo, Yunan,
From section 1, intro:
“This means that both Route Monitoring and Peer Down messages have a
non-extensible format.”
The above has been updated by section 5.3 RFC9069 where reason code 6
includes TLVs.
Ack, i will refine text.
“The proposal of this document is to bump the BMP version, for backward
compatibility, and allow all message types to make provision for
trailing TLV data.”
Do all messages have to be version 4 for a session or can a BMP session
use both versions based on the need for additional TLVs or not?
Good point, whichever direction it's good to add some text in this
sense. I would myself lean towards having a one unique protocol version.
So encode all messages as v4 if the implementation is compliant with
this draft.
Some comments:
* The main use-cases call out route-monitor and peer down messages
that didn’t have optional TLVs. RFC9069 updates Peer Down with
reason code 6, that indicates TLVs to follow. Might make sense to
use that instead of changing it in version 4.
I find this a bit restrictive since reason code 6 calls only for TLV
data whereas other reason codes, ie. 1 and 3 do call also for the BGP
Notification PDU. I'd see this as complement to TLVs introduced with
code 6 and it would be indeed good for me to add some text / reference
about it.
* Instead of sorting TLVs by code point/type/… , wouldn’t it be okay
to process them in order as they are encoded? In other words, let
the sender define the order by how the sender encodes them. Having
to sort would require buffering to process all TLVs so they can be
sorted before processing/forwarding on.
It is a SHOULD so effectively you can do that without violating the
document. Would you have a preference to further relax it? The point
originally came from Jeff and what i like about it is that it implies
that if there are repetitions, they are batched together.
* To me, encoding per NLRI characteristics in TLVs with indexing is
duplicate of attributes. It also could get large when a handful of
NLRIs (sharing the same BGP attributes, packed into the same
message) have different or shared BMP TLV characteristics. For
example, 10 NLRIs packed, 8 of them share characteristic A and the
other 2 share characteristic B. The TLV cannot be indexed as 0
because all of them do not share the TLVs in common, resulting in 10
TLVs being needed.
I see your point. I think we may have two main strategies here: take
care of this at packing time or propose a way to group NLRIs, in the
lack of better inspiration in this moment, a "Group TLV" with index 0
defining a new index to group a number of NLRIs. Would you have
(different) preferences, (different) proposal?
* The current TLV types suggest the primary use case is per BMP
message conveyance of BGP capabilities that effect how to parse the
message itself. Such as add-paths, multiple labels, … ASN encoding
is already indicated by the “A” flag. Both RFC8277 and 3107 are the
same in terms of decoding multiples of label 3 octets in length
minus the prefix length. This can be handled stateless still.
RFC8277 does clarify what to expect in terms of number of labels,
but from a stateless standpoint can’t we still process it as defined
by 3107? This draft focuses on stateless processing, where the Peer
Up with the OPEN message was not seen and/or not considered. I
believe the only capability that is a problem is add-paths.
Add-paths could be handled with a new flag. I believe all the other
BGP TLVs are defined well enough to process the message without
having to see the OPEN message exchange. Are there others that
cannot be processed stateless?
Just a small note to comment that the main purpose of this document is
really to build an equal surface to all existing message types (except
Route Mirroring) for the sake of future extensibility as well as mandate
that any new message type will have to be extensible as well, hence
bringing BMP on par with other telemetries.
Stateless parsing, mentioned also in the document, wanted to be an
use-case. I agree that ASN encoding is already specified by flags hence
some additional text would be needed there about ensuring
synchronization of the two; but i also see the opposite point that you
make about multiple labels and especially that, being on the verge of
flexing our fingers for a 7854-bis (which is a change in scenario), an
Add-Path flag could find room there - and we can call it a day. The most
important point though, IMO, is your concluding question: do we envision
other elements relevant to stateless parsing? Now i don't but i don't
know what future brings: whatever sensible flags space we define, it
will be always both finite and a scarce resource; whereas TLVs are only
finite. Then yet again, finding myself an opposite point to my own point
(something on which i have been vocal in the past as well), is it
elegant to have to skip the PDU in order to infer its own
characteristics? Probably not and probably TLVs should remain in the
domain of annotating content of the PDU, extract router state, etc.
* A VRF name is conveyed via Peer Up and Peer Down and not included in
each route-monitor message. Strictly stateless, the receiver would
not know which VRF name the per-peer header correlates to without
having some level of state correlation of per-peer header values to
Peer Up/Down. Maybe for VRF name it doesn’t matter, but at some
point receivers are expected to keep some level of state for peering
sessions. This is needed with RIB state tracking and
route-refreshes, which may come in via route-mirror message or
another/repeated Peer Up message, but not in the form of a
route-monitor message.
Good point & I agree. I'd only need one clarification: since you say
"Maybe for VRF name it doesn’t matter", do you propose to add some text
/ recommendation? Or would you like the VRF name among the TLVs defined
by this document?
Paolo
_______________________________________________
GROW mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/grow
