Comments inline. On 10/13/22, 7:54 AM, "Paolo Lucente" <[email protected]> wrote:
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. [tevens] I agree for all messages in a TCP session be of the same version, but it does require that every message be set, such as init, peer up, peer down, … Little is being updated to support TLVs here, specifically in RM messages. A version change will cause incompatibility with receivers for the entire stream of messages, including stats. If we do a version change, it might make sense to add more than TLVs. Let’s make it count. > 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. [tevens] That would be good. IMO, it would be good to reuse the existing IANA table TLVs that effect Peer Up/Down (https://www.iana.org/assignments/bmp-parameters/bmp-parameters.xhtml#initiation-peer-up-tlvs). A new registry for other BMP message types makes sense… such as route-monitor TLVs. > * 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. [tevens] Might be more clear to call out if it is the sender or receiver that is sorting. IMO, I prefer the sender to encode in the order the sender needs them to be processed by the receiver, regardless of type value ascending. I don’t believe any of the TLVs require order processing by the receiver right now, but in the future that could change. The problem with sorting by type ascending value is that a new value added to the registry later may require it to be processed before a previous type… for example to support an override or influence how to process another TLV. > * 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? [tevens] A Group TLV would make sense, but it seems like that might be adding more complexity. Hopefully folks wouldn’t use TLVs on route-monitor messages as a way to avoid or override path attributes. I’m running into this now with ATTR_SET(128) https://www.juniper.net/documentation/us/en/software/junos/static-routing/topics/ref/statement/independent-domain-edit-routing-options.html. I’m updating OpenBMP to support multiple sets of path attributes. Adding these TLVs could easily grow into yet another set of attributes for NLRIs. Maybe this draft could add some guidance around what is allowed in these TLVs. > * 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. [tevens] While TLVs can be used to help define how to decode a message, the flags already defined much of this. It’s missing add-paths and in the future maybe something else. 7854-bis could address this, but that might be overloading -bis. This is where a version change makes sense if we need to increase the flags from 8 to 16 bits. The more I think about it, the more I’m inclined to suggest that this draft shouldn’t change the version and instead we should have a new BMP version draft. > * 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? [tevens] I think we should try to reuse the same IANA registries for TLVs if possible. Duplicating and creating new registries is a bit conflicting in terms of when to encode type value from registry X. IMO, the registries don’t have to be BMP version specific. Currently the TLVs are under https://www.iana.org/assignments/bmp-parameters/bmp-parameters.xhtml#initiation-peer-up-tlvs. This could be updated. Paolo
_______________________________________________ GROW mailing list [email protected] https://www.ietf.org/mailman/listinfo/grow
