Hi Luuk,
On 13/3/23 16:54, Luuk Hendriks wrote:
Hi Paolo, all,
On Sun 12 Mar 2023, 03:09, Paolo Lucente wrote:
On 10/3/23 15:28, Luuk Hendriks wrote:
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.
Not sure I follow: what do you mean with 'This' here?
I was referring to the fact that BGP PDU TLV being preceded by other TLVs
was proposed in RFC7854 for the Route Mirroring message, 4.7:
"Type = 0: BGP Message. A BGP PDU. This PDU may or may not be an
Update message. If the BGP Message TLV occurs in the Route
Mirroring message, it MUST occur last in the list of TLVs."
Another thing is: true now it is all TLV'd but some comments to the previous
way of structuring the Route Monitoring message with TLVs, ie. BGP Update
first then TLVs, was criticized because TLVs may contain info that may be
good to have upfront when parsing the BGP PDU.
Thanks, that clarifies things. My thinking here was that parsing the BGP
PDU should always be possible without having to rely on the other TLVs
(to prevent workarounds/hacks for things that should actually have go in
other places, such as flags in the per-peer header). But, perhaps
disallowing preceding TLVs is not the right way to prevent such
workarounds anyway.
Exactly & agree.
- can multiple TLVs point to a Group TLV index? (I assume yes)
Yes. Do you believe text would be needed for this?
It'd be nice to leave as little room for interpretation as possible.
Currently in sec 3 the text states:
Multiple TLVs of the same type and with the same index can be repeated
as part of the same message.
Maybe generalize this to:
Multiple TLVs of the same type and/or with the same (Group) index
can be repeated as part of the same message.
(Though at that point the Group TLV is not yet introduced, so maybe this
is more confusing than it is actually helpful.)
I still struggle with this a bit - can i ask you an example? Because a Group
TLV with the same Group Index would be like re-defining the Group, wouldn't
it? And actually i was now thinking it would be good to add some text to say
we don't want that.
I now realise my proposed text was quite unclear: what I meant was that
a newly defined Group Index can occur more than once in Normal TLVs,
i.e. the new index is indeed defined only once but _used_ one or more
times.
Perhaps trying to catch all this in one sentence and one single place is
not the way to go. Maybe we can leave the part in sec 3 as is, and
incorporate it into the last paragraph in 4.2.1. The current version
seems to use the term 'Group Index' for multiple different things
though, in the last sentence:
Current text:
A NLRI index can be listed as part of multiple Group TLVs within the
same message. NLRI indexes within a Group TLV SHOULD be sorted by the
sender. A Group Index can not reference an NLRI index 0. Finally, a
_Group Index_ MUST not recursively include another _Group Index_.
Proposal:
A NLRI index can be listed as part of multiple Group TLVs within the
same message. NLRI indexes within a Group TLV SHOULD be sorted by the
sender. A Group Index can not reference an NLRI index 0. Finally, a
Group TLV MUST NOT include its own or another Group Index.
Multiple TLVs can point to the same Group Index, i.e. a group can be
reused within the same Route Monitoring message.
Thank you, that looks nice. As i inaugurated a draft for -12, i have
committed your text here:
https://github.com/paololucente/draft-ietf-grow-bmp-tlv/commit/b237535bafe0215e91d31523e59dd49fceee1744
Additionally, the second bullet point in 4.2.1 currently mentions
'indexes', perhaps that should be 'NLRI indexes' for clarity.
Ack, agree!
Paolo
_______________________________________________
GROW mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/grow
