Re: [lisp] AD Review of draft-ietf-lisp-6834bis-09

[Luigi Iannone <[e-mail]>]

Hi Alvaro,

Thanks for the great review.
I’ll try to propose un update by end of next week, so that we can have it ready by end of the month.

Ciao

L.


> On 13 Apr 2022, at 20:54, Alvaro Retana <[e-mail]> wrote:
> 
> Dear authors/WG:
> 
> While I appreciate the small number of changes from rfc6834 and the
> original intent (from 2018) of processing this document quickly, but
> it requires a non-trivial amount of work.  It is now going for
> Proposed Standard, so we need to be more rigorous.
> 
> I put detailed comments inline (below).  There are two main themes
> that I want to highlight here:
> 
> (1) Duplication and overlap
> 
>    Several parts of this document describe the same or related behaviors.
>    This issue has resulted in duplicate descriptions and specifications, not
>    all of them saying the same thing. :-(  The problem extends beyond this
>    document into both rfc6830bis and rfc6833bis.  I pointed and cross-
>    referenced the sections as best as I could below.
> 
>    In general, please specify the behavior only once.  Doing so in multiple
>    places can be confusing and, as in this case, result in the specifications
>    not being in sync.
> 
>    Note that to address this point we may need to update rfc6830bis and
>    rfc6833bis.  I would prefer to see as much of the specification related to
>    Map-Versioning as possible (all?) in this document.  Removing pieces and
>    referencing this document shouldn't require removing them from the RFC
>    Editor's queue -- and they both Normatively depend on this document anyway.
> 
>    One specific action is to remove the text from §13.2/rfc6830bis.  Some of
>    it is already covered in this document -- the remaining pieces, which I
>    believe are only the last couple of paragraphs, can find a better home.
> 
> 
> (2) Document Structure
> 
>    As I started reading this document, I found myself going back to rfc6830bis
>    to look for packet formats -- and I wasn't even past the Introduction!  I
>    then realized that some of the information was in this document but an
>    unexpected order.  Some actionable comments:
> 
>    (1) Simplify the Introduction; there's too much detail.  For example, it
>    mentions the location of the Map-Version in excruciating detail ("the 24
>    low-order bits of the first longword of the LISP header"), but the
>    description is wrong (see more inline).  There are explanations about the
>    topics later on but no forward references.  Instead, paint a high-level
>    picture and tell the reader where to find the details.
> 
>    (2) Beyond the Introduction, the document is ordered in a cumbersome way:
>    The location and format of the Map-Version are presented in §6, but both
>    §4/§5 describe how the versions are handled.  It would be much better for
>    the reader if §6 were moved forward.  This is my suggested ordering:
> 
>    6.  LISP Header and Map-Version Numbers
>    7.  Map Record and Map-Version (this could be a subsection)
>    4.  EID-to-RLOC Map-Version Number
>    5.  Dealing with Map-Version Numbers
> 
>    Also, I suggest moving §8 to an Appendix.
> 
> 
> As we have a constrained timeline for this document, I would like to
> start the IETF Last-Call by the end of this month.  I have a lot of
> comments, but I don't think they will be hard to address.
> 
> Please reply inline to this message to expedite my review of any
> updates.  Also, if you think talking would clear things up faster,
> feel free to find time on my calendar:
> https://doodle.com/mm/alvaroretana/book-a-time
> 
> 
> Padma:  As Shepherd, I'll need you to please update the writeup when
> the final version is ready.
> 
> 
> Thanks!!
> 
> Alvaro.
> 
> 
> [Line numbers from idnits.]
> 
> ...
> 13	Abstract
> 
> 15	   This document describes the LISP (Locator/ID Separation Protocol)
> 16	   Map-Versioning mechanism, which provides in-packet information about
> 17	   Endpoint ID to Routing Locator (EID-to-RLOC) mappings used to
> 18	   encapsulate LISP data packets.  The proposed approach is based on
> 19	   associating a version number to EID-to-RLOC mappings and the
> 20	   transport of such a version number in the LISP-specific header of
> 21	   LISP-encapsulated packets.  LISP Map-Versioning is particularly
> 22	   useful to inform communicating Ingress Tunnel Routers (ITRs) and
> 23	   Egress Tunnel Routers (ETRs) about modifications of the mappings used
> 24	   to encapsulate packets.  The mechanism is optional and transparent to
> 25	   implementations not supporting this feature, since in the LISP-
> 26	   specific header and in the Map Records, bits used for Map-Versioning
> 27	   can be safely ignored by ITRs and ETRs that do not support or do not
> 28	   want to use the mechanism.
> 
> [minor] s/proposed//g
> This mechanism is now on the standards track, so it is not "proposed"
> any more. :-)
> 
> 
> ...
> 93	1.  Introduction
> 
> [] As I mentioned above, please simplify this section.  I left
> comments below, but I think that most of the text can be changed for a
> high-level description.
> 
> 
> ...
> 113	   When Map-Versioning is used, LISP-encapsulated data packets contain
> 114	   the version number of the two mappings used to select the RLOCs in
> 115	   the outer header (i.e., both source and destination RLOCs).  These
> 116	   version numbers are encoded in the 24 low-order bits of the first
> 117	   longword of the LISP header and indicated by a specific bit in the
> 118	   flags (first 8 high-order bits of the first longword of the LISP
> 119	   header).  Note that not all packets need to carry version numbers.
> 
> [major] "version numbers are encoded in the 24 low-order bits of the
> first longword of the LISP header"
> 
> Wow!  That's not an easy reference to follow!
> 
> I went to find the "LISP Header" and found this definition in rfc6830bis:
> 
>    LISP Header:   LISP header is a term used in this document to refer
>       to the outer IPv4 or IPv6 header, a UDP header, and a LISP-
>       specific 8-octet header that follow the UDP header and that an ITR
>       prepends or an ETR strips.
> 
> If the LISP Header includes the "outer IPv4 or IPv6 header, a UDP
> header, and a LISP-specific 8-octet header" then the "24 low-order
> bits of the first longword" maps to something different. :-(
> 
> Instead, it would be easier for everyone to simple show the header
> with the V-bit set (from rfc6830bis):
> 
>      0 x 0 1 x x x x
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |N|L|E|V|I|R|K|K|  Source Map-Version   |   Dest Map-Version    |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |                 Instance ID/Locator-Status-Bits               |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> 
> [major] "indicated by a specific bit in the flags (first 8 high-order
> bits of the first longword of the LISP header)."
> 
> Please replace that with a specific mention to the V-bit.
> 
> 
> 121	   When an ITR (Ingress Tunnel Router) encapsulates a data packet, with
> 122	   a LISP header containing the Map-Version numbers, it puts in the
> 123	   LISP-specific header two version numbers:
> 
> 125	   1.  The version number assigned to the mapping used to select the
> 126	       source RLOC (contained in the EID-to-RLOC Database).
> 
> 128	   2.  The version number assigned to the mapping used to select the
> 129	       destination RLOC (contained in the EID-to-RLOC Map-Cache).
> 
> [major] Please use the names in the packet formats: indicate which is
> the "Source Map-Version" and which the "Dest Map-Version".
> 
> 
> ...
> 154	3.  Definitions of Terms
> 
> 156	   This document uses terms already defined in the main LISP
> 157	   specification ([I-D.ietf-lisp-rfc6830bis],
> 158	   [I-D.ietf-lisp-rfc6833bis]).  Here, we define the terms that are
> 159	   specific to the Map-Versioning mechanism.  Throughout the whole
> 160	   document, Big Endian bit ordering is used.
> 
> [major] Please avoid redefining terms that are already defined
> elsewhere.  Changes in either (even if editorial) may cause the
> documents to fall out of sync.  If you want to call attention to
> specific terms, please point at the original definition or a specific
> section.
> 
> For example:
> 
>    Map-Version Number:  as defined in §7.
> 
> 
> 162	   Map-Version number:  An unsigned 12-bit integer is assigned to an
> 163	     EID-to-RLOC mapping, excluding the value 0 (0x000).
> 
> [major] Beyond being a 12-bit integer, this definition doesn't say
> what the Map-Version number is.  §7 contains a much better
> description.
> 
> 
> 165	   Null Map-Version:  The 12-bit null value of 0 (0x000) is not used as
> 166	     a Map-Version number.  It is used to signal that no Map-Version
> 167	     number is assigned to the EID-to-RLOC mapping.
> 
> [] Suggestion>
>    Null Map-Version:  A Map-Version number with a value of 0x000 (zero),
>       used to signal that no Map-Version number is assigned to the
>       EID-to-RLOC mapping (Section 4.1).
> 
> 
> 169	   Source Map-Version number:  This Map-Version number of the EID-to-
> 170	     RLOC mapping is used to select the source address (RLOC) of the
> 171	     outer IP header of LISP-encapsulated packets.
> 
> [major] This definition describes what the Source Map-Version number
> is used for, but not what it is.  Also, the definition in §6 is
> different from the one here -- it is close, but not the same.  Please
> define terminology in only one place.  It seems to me that leaving
> this definition up to §6 would be best.
> 
> 
> 173	   Destination Map-Version number:  This Map-Version number of the EID-
> 174	     to-RLOC mapping is used to select the destination address (RLOC) of
> 175	     the outer IP header of LISP-encapsulated packets.
> 
> [major] The packet format in rfc6830bis uses "Dest Map-Version"
> instead of "Destination Map-Version".  Please use the same terminology
> in the packet format and throughout the document.
> 
> 
> [major] Same comment as above: it describes what it's used for, not
> what it is.  There's a different definition in §6.
> 
> 
> 177	4.  EID-to-RLOC Map-Version Number
> 
> 179	   The EID-to-RLOC Map-Version number consists of an unsigned 12-bit
> 180	   integer.  The version number is assigned on a per-mapping basis,
> 181	   meaning that different mappings have a different version number,
> 182	   which is also updated independently.  An update in the version number
> 183	   (i.e., a newer version) consists of incrementing by one the older
> 184	   version number.
> 
> [major] "incrementing by one"
> 
> There's an exception in the case where V1+1 would result in 0, right?
> Please mention it.
> 
> I later found that the last paragraph in §4.1 explains this exception.
> Please point to if from here, or move the paragraph to this section.
> Note that the text in §4.1 normatively requires (MUST) the increment,
> but the description here doesn't use normative language.  Please be
> consistent.
> 
> 
> ...
> 212	   Map-version numbers are assigned to mappings by configuration.  The
> 213	   initial Map-Version number of a new EID-to-RLOC mapping SHOULD be
> 214	   assigned randomly, but it MUST NOT be set to the Null Map-Version
> 215	   value (0x000), because the Null Map-Version number has a special
> 216	   meaning (see Section 4.1).
> 
> [major] "Map-version numbers are assigned to mappings by
> configuration.  The initial Map-Version number of a new EID-to-RLOC
> mapping SHOULD be assigned randomly..."
> 
> This sounds like a contradiction: configure, but make it random.  I
> assume that you mean that manual configuration may be the exception.
> 
> Suggestion>
>    The initial Map-Version number of a new EID-to-RLOC mapping SHOULD be
>    assigned randomly; it MUST NOT be set to the Null Map-Version value
>    (0x000) (see Section 4.1).  Optionally, the initial Map-Version number
>    may be configured.
> 
> 
> ...
> 224	4.1.  The Null Map-Version
> 
> [] The comments below indicate that there are multiple places that
> overlap with this section.  The text from this section may be
> relocated (and clarifying actions taken) to make the document clearer.
> This section could then be eliminated.
> 
>    Paragraph 1 could be moved to §6.
> 
>    Paragraph 2 could be moved to §5/§5.1/§5.2.
> 
>    Paragraph 3 could be moved to §7.
> 
>    Paragraph 4 could be moved to §4.
> 
> 
> 226	   The value 0x000 (zero) is not a valid Map-Version number indicating
> 227	   the version of the EID-to-RLOC mapping.  Such a value is used for
> 228	   special purposes and is named the Null Map-Version number.
> 
> 230	   The Null Map-Version MAY appear in the LISP-specific header as either
> 231	   a Source Map-Version number (cf.  Section 5.2) or a Destination Map-
> 232	   Version number (cf.  Section 5.1).  When the Source Map-Version
> 233	   number is set to the Null Map-Version value, it means that no map
> 234	   version information is conveyed for the source site.  This means that
> 235	   if a mapping exists for the source EID in the EID-to-RLOC Map-Cache,
> 236	   then the ETR MUST NOT compare the received Null Map-Version with the
> 237	   content of the EID-to-RLOC Map-Cache.  When the Destination Map-
> 238	   Version number is set to the Null Map-Version value, it means that no
> 239	   map version information is conveyed for the destination site.  This
> 240	   means that the ETR MUST NOT compare the value with the Map-Version
> 241	   number of the mapping for the destination EID present in the EID-to-
> 242	   RLOC Database.
> 
> [major] "Null Map-Version MAY appear"
> 
> s/MAY/may
> 
> This is a statement of fact, not a Normative statement.
> 
> 
> [nit] s/cf.  //g
> 
> 
> [minor] Please put a forward reference to §5.1/§5.2 where "MUST NOT
> compare" is used above.  §5.1/§5.2 don't use "compare" as part of
> their specification, but pointing to them should keep us from changing
> the language here.
> 
> 
> 244	   The other use of the Null Map-Version number is in the Map Records,
> 245	   which are part of the Map-Request, Map-Reply, and Map-Register
> 246	   messages (defined in [I-D.ietf-lisp-rfc6833bis]).  Map Records that
> 247	   have a Null Map-Version number indicate that there is no Map-Version
> 248	   number associated with the mapping.  This means that LISP-
> 249	   encapsulated packets destined to the EID-Prefix referred to by the
> 250	   Map Record MUST either not contain any Map-Version numbers (V bit set
> 251	   to 0) or, if they contain Map-Version numbers (V bit set to 1), then
> 252	   the destination Map-Version number MUST be set to the Null Map-
> 253	   Version number.  Any value different from zero means that Map-
> 254	   Versioning is supported and MAY be used.
> 
> [] See comments in §7 about the requirements in this paragraph.
> 
> 
> [nit] s/(defined in [I-D.ietf-lisp-rfc6833bis])/[I-D.ietf-lisp-rfc6833bis]/g
> 
> 
> [major] "Any value different from zero means that Map-Versioning is
> supported and MAY be used."
> 
> 
> 256	   The fact that the 0 value has a special meaning for the Map-Version
> 257	   number implies that, when updating a Map-Version number because of a
> 258	   change in the mapping, if the next value is 0, then the Map-Version
> 259	   number MUST be incremented by 2 (i.e., set to 1 (0x001), which is the
> 260	   next valid value).
> 
> [] See my comment in §4 about this exception.
> 
> 
> 262	5.  Dealing with Map-Version Numbers
> ...
> 272	   To each mapping, a version number is associated and changes each time
> 273	   the mapping is changed.  Note that Map-Versioning does not introduce
> 274	   new problems concerning the coordination of different ETRs of a
> 275	   domain.  Indeed, ETRs belonging to the same LISP site must return for
> 276	   a specific EID-prefix the same mapping, including the same Map-
> 277	   Version number.  This is orthogonal to whether or not Map-Versioning
> 278	   is used.  The synchronization problem and its implications on the
> 279	   traffic are out of the scope of this document.
> 
> [] See my comment in §7 about leaving this out of scope and the
> requirement from rfc6833bis.
> 
> 
> ...
> 304	   If one or both of the above conditions do not hold, the ETR can send
> 305	   a Map-Request either to make the ITR aware that a new mapping is
> 306	   available (see Section 5.1) or to update the mapping in the local
> 307	   EID-to-RLOC Map-Cache (see Section 5.2).
> 
> [major] Should this behavior be Normative (MUST/SHOULD)?  The text
> currently says that "the ETR can send"; if the text doesn't require
> (or at least recommend) an action then it looks like this document
> just talks about carrying extra numbers around that don't serve a
> specific purpose.  I believe that §10 makes a good case for an action
> to be specified.
> 
> 
> 309	5.1.  Handling Destination Map-Version Number
> 
> 311	   When an ETR receives a packet, the Destination Map-Version number
> 312	   relates to the mapping for the destination EID for which the ETR is
> 313	   an RLOC.  This mapping is part of the ETR EID-to-RLOC Database.
> 314	   Since the ETR is authoritative for the mapping, it has the correct
> 315	   and up-to-date Destination Map-Version number.  A check on this
> 316	   version number can be done, where the following cases can arise:
> 
> [major] "A check on this version number can be done..."  Same comment
> as above: Should this behavior be Normative (MUST/SHOULD)?
> 
> 
> ...
> 323	   2.  The packet arrives with a Destination Map-Version number greater
> 324	       (i.e., newer) than the one stored in the EID-to-RLOC Database.
> 325	       Since the ETR is authoritative on the mapping, meaning that the
> 326	       Map-Version number of its mapping is the correct one, this
> 327	       implies that someone is not behaving correctly with respect to
> 328	       the specifications.  In this case, the packet carries a version
> 329	       number that is not valid; otherwise, the ETR would have the same
> 330	       number, and the packet SHOULD be silently dropped