Hi Tim,
Inline:
On 19/10/22 02:27, Tim Evens (tievens) wrote:
Comments inline.
[ .. ]
> “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.
True it does create an incompatibility but that was wanted by design in
the very early conversations. If you think about it, we could also not
change the version, yet a collector may infer the presence of trailing
TLVs by the fact that the BMP Route Monitoring message is longer than
the enclosed PDU; back then argument was: what if an existing & running
collector does not expect such situation, because RM messages length in
BMP version 3 has to match with headers + PDU? There is no universal
answer to such question and it was felt cleaner that the version would
be bumped.
> * 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 <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.
Ack & let me refine this a bit. I agree that Peer Down should use the
same TLV registry as Peer Up but Peer Up should be split from Init. This
was proposed by John some time ago & i volunteered to help him bring it
forward
https://datatracker.ietf.org/doc/html/draft-ietf-grow-bmp-peer-up-00 . I
will add a bit of text in this sense too, ie. renaming of "Peer Up TLVs"
into "Peer Up and Peer Down TLVs". John, if you are reading, would you
have any comments on doing this?
> * 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.
Ack & indeed i do agree it's the sender that is sorting. I will add some
verbage in this sense. The current text, including the new verbage, reads as
"TLVs SHOULD be sorted by the sender by their code point. Multiple TLVs
of the same type can be repeated as part of the same message, and it is
left to the specific use-cases whether all, any, the first or the last
TLV should be considered as well as whether ordering matters."
The only part where i do not have strong feelings, since this is a
SHOULD anyway, is the sorting by the code point. Jeff, if you are
reading this, would you have any thoughts at this propo?
> * 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 <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.
More than avoid or override path attributes i can see how one may want
to extract further state from the router than what BGP can tell, a-la
annotate the NLRIs. As an example, see the BMP Path Marking draft. On
one hand i see how we may want to be wise wrt the bits that hit the
wire, on the other we should not kill the "gimme the data" nature of a
monitoring / telemetry protocol.
This said, if we see a Group TLV mechanism potentially working for both
the sender and the receiver (i myself am positive for the receiver
side), why not adding this facility? It may result useful, it does
enrich the support of TLVs and it's all optional. I will propose some
text in this sense.
> * 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.
See above the rationale about the proposed version change. And i do
agree with you that a change in the size of the flags field would
require a new version bump. I also see how proposing a version change in
a -bis draft feels a bit like mixing whiskey and majo. But we could go
incremental: keep the flags size as-is, add the add-path flag, and
include this in the -bis, no?
> * 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 <https://www.iana.org/assignments/bmp-parameters/bmp-parameters.xhtml#initiation-peer-up-tlvs>. This could be updated.
I do have mixed feeling conflating Peer Up / Peer Down with Route
Monitoring (see previous note on the split of Init and Peer Up
registries). I can total see how we may get in wanting to have a TLV
that is related to the BGP Update PDU and/or any of the enclosed NLRIs
that would make total no sense in the context of Peer Up / Peer Down
(see as a tangible example the BMP Path Marking draft); probably also
the vice-versa can become true. I do see though how something like the
VRF name we may want to maintain across all 3 and would be good to add
to the RM registry too.
Paolo
_______________________________________________
GROW mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/grow
