[RTG-DIR] Rtgdir last call review of draft-ietf-lisp-gpe-04

[Adrian Farrel <[e-mail]>]

Reviewer: Adrian Farrel
Review result: Has Issues

Hello,

I have been selected as the Routing Directorate reviewer for this draft. The
Routing Directorate seeks to review all routing or routing-related drafts as
they pass through IETF last call and IESG review, and sometimes on special
request. The purpose of the review is to provide assistance to the Routing ADs.
For more information about the Routing Directorate, please see
?http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Although these comments are primarily for the use of the Routing ADs, it would
be helpful if you could consider them as normal review comments. I believe that
this review comes between WG publication and the start of IETF last call - you
may wish to discuss with your AD whether to treat these comments separately or
as part of IETF last call.

Document: draft-ietf-lisp-gpe-04.txt
 Reviewer: Adrian Farrel
 Review Date: 9-August-2018
 IETF LC End Date: No known
 Intended Status: Standards Track

Summary
I have significant concerns about this document and recommend that the Routing
ADs discuss these issues further with the authors. The issues are not
substantially technical in nature, but do indicate the need for significant
reworking of the text. I have tried to make suggestions for new text.

Comments:

This document specifies an alternate LISP header format that can be used to
allow LISP to carry payloads other than IP. A new capabilities flag is defined
so that routers know whether this new format is supported, and a new flag in
the header itself indicates when the new format is in use.

The document is clear and readable, but has some issues of presentation that
could close a few potential misunderstandings and thus improve implmentation
prospects.

No attempt is made in the document to explain how/why the reduction in size of
some standard LISP header fields is acceptable. For example, if implementations
of this spec can safely operate with a 16 bit Nonce or 8 bit Map-Versions, why
does 6830/6830bis feel the need for 24 and 12 bit fields rspectively?

===Major Issues===

Section 3 has a mix of minor and leess minor issues...

OLD
   This document defines the following changes to the LISP header in
   order to support multi-protocol encapsulation:

   P Bit:  Flag bit 5 is defined as the Next Protocol bit.  The P bit
      MUST be set to 1 to indicate the presence of the 8 bit next
      protocol field.

      P = 0 indicates that the payload MUST conform to LISP as defined
      in [I-D.ietf-lisp-rfc6830bis].  Flag bit 5 was chosen as the P bit
      because this flag bit is currently unallocated.

   Next Protocol:  The lower 8 bits of the first 32-bit word are used to
      carry a Next Protocol.  This Next Protocol field contains the
      protocol of the encapsulated payload packet.

      LISP uses the lower 24 bits of the first word for either a nonce,
      an echo-nonce, or to support map-versioning
      [I-D.ietf-lisp-6834bis].  These are all optional capabilities that
      are indicated in the LISP header by setting the N, E, and the V
      bit respectively.

      When the P-bit and the N-bit are set to 1, the Nonce field is the
      middle 16 bits.

      When the P-bit and the V-bit are set to 1, the Version field is
      the middle 16 bits.

      When the P-bit is set to 1 and the N-bit and the V-bit are both 0,
      the middle 16-bits are set to 0.

      This document defines the following Next Protocol values:

      0x1 :  IPv4

      0x2 :  IPv6

      0x3 :  Ethernet

      0x4 :  Network Service Header [RFC8300]

        0                   1                   2                   3
        0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |N|L|E|V|I|P|K|K|        Nonce/Map-Version      | Next Protocol |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |                 Instance ID/Locator-Status-Bits               |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

                              LISP-GPE Header

NOTES
   - It would be helpful to put the figure higher up
   - The use of "MUST" for the P-bit is attenuated wrongly
   - Need to be consistent on "P Bit" or "P-bit" or "P bit"
   - There looks to be a problem in the case of map version. The base
     spec has 12 bits each for source and dest map-version, so this doc
     needs to describe how the reeduced 16 bits is split (presumably not
     12 and 4).
   - You need a pointer to the IANA registry for next protocol
NEW
   This document defines two changes to the LISP header in order to
   support multi-protocol encapsulation: the introduction of the P-bit
   and the definition of a Next Protocol field.  This is shown in
   Figure 1 and described below.

        0                   1                   2                   3
        0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |N|L|E|V|I|P|K|K|        Nonce/Map-Version      | Next Protocol |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |                 Instance ID/Locator-Status-Bits               |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

                         Figure 1 : The LISP-GPE Header

   P-Bit:  Flag bit 5 is defined as the Next Protocol bit.

      If the P-bit is clear (0) the LISP header conforms to the
      definition in [I-D.ietf-lisp-rfc6830bis].

      The P-bit is set to 1 to indicate the presence of the 8 bit Next
      Protocol field.

   Next Protocol:  The lower 8 bits of the first 32-bit word are used to
      carry a Next Protocol.  This Next Protocol field contains the
      protocol of the encapsulated payload packet.

      In [I-D.ietf-lisp-6834bis], LISP uses the lower 24 bits of the
      first word for a nonce, an echo-nonce, or to support map-
      versioning.  These are all optional capabilities that are
      indicated in the LISP header by setting the N, E, and V bits
      respectively.

      When the P-bit and the N-bit are set to 1, the Nonce field is the
      middle 16 bits (i.e., encoded in 16 bits, not 24 bits).  Note that
      the E-bit only has meaning when the N-bit is set.

      When the P-bit and the V-bit are set to 1, the Version fields use
      the middle 16 bits: the Source Map-Version uses the high-order 8
      bits, and the Dest Map-Version uses the low-order 8 bits.

      When the P-bit is set to 1 and the N-bit and the V-bit are both 0,
      the middle 16-bits MUST be set to 0 on transmission and ignored on
      receipt.

      This document defines the following Next Protocol values:

      0x1 :  IPv4

      0x2 :  IPv6

      0x3 :  Ethernet

      0x4 :  Network Service Header [RFC8300]

      The values are tracked in an IANA registry as described in Section
      5.

---

Section 4 must describe the error case when a LISP-GPE capable router
sets the P-bit on a packet to a non LISP-GPE capable router. So...

OLD
   When encapsulating IP packets to a non LISP-GPE capable router the P
   bit MUST be set to 0.
NEW
   When encapsulating IP packets to a non LISP-GPE capable router the P-
   bit MUST be set to 0.  That is, the encapsulation format defined in
   this document MUST NOT be sent to a router that has not indicated
   that it supports this specification because such a router would
   ignore the P-bit (as described in [I-D.ietf-lisp-rfc6830bis]) and so
   would misinterpret the other LISP header fields possibly causing
   significant errors.
END

---

4.1

Not your fault that RFC 8060 doesn't have a registry for bits in the
LCAF, but now you really need one or else future orthogonal specs risk
colliding with the g-bit.  A bit odd to add this in this document, but
not worth a bis on 8060.

===Minor Issues ===

Section 2

OLD
   The LISP header [I-D.ietf-lisp-rfc6830bis] contains a series of flags
   (some defined, some reserved), a Nonce/Map-version field and an
   instance ID/Locator-status-bit field.  The flags provide flexibility
   to define how the various fields are encoded.  Notably, Flag bit 5 is
   the last reserved bit in the LISP header.

        0                   1                   2                   3
        0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |N|L|E|V|I|R|K|K|            Nonce/Map-Version                  |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
       |                 Instance ID/Locator-Status-Bits               |
       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

                                LISP Header
NOTES
   We need to be careful not to risk any confusion. At least, "some
   reserved" is an over-statement. But also we should not show a repeat
   of the Lisp header as that causes a duplicate definition.
NEW
   The LISP header is defined in [I-D.ietf-lisp-rfc6830bis] and contains
   a series of flags of which one (bit 5) is shown in that document as
   "reserved for future use".  The setting of the flag fields defined
   how the subsequent header fields are interpretted.
END

---

4.1
I don't think you should reproduce the Multiple Data-Planes LCAF Type
figue from 8060 here as it creates a duplicate definition.  The text
explanation of which bit is the g-bit shold be enough.

===Nits===

Abstract
OLD
   This document describes extending the Locator/ID Separation Protocol
   (LISP) Data-Plane, via changes to the LISP header, to support multi-
   protocol encapsulation.
NEW
   This document describes extentions to the Locator/ID Separation
   Protocol (LISP) Data-Plane, via changes to the LISP header, to
   support multi-protocol encapsulation.
END

---

1.
OLD
   LISP Data-Plane, as defined in in [I-D.ietf-lisp-rfc6830bis], defines
   an encapsulation format that carries IPv4 or IPv6 (henceforth
   referred to as IP) packets in a LISP header and outer UDP/IP
   transport.
NEW
   The LISP Data-Plane is defined in [I-D.ietf-lisp-rfc6830bis].  It
   specifies an encapsulation format that carries IPv4 or IPv6 packets
   (henceforth jointly referred to as IP) in a LISP header and outer
   UDP/IP transport.

---

1.1
Please use the new boilerplate...
   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
   "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
   "OPTIONAL" in this document are to be interpreted as described in BCP
   14 [RFC2119] [RFC8174] when, and only when, they appear in all
   capitals, as shown here.

---

1.2
Nothwithstanding the text in this section, abbreviations need to be
expanded either on first use or in this section.
I see:
- LCAF
- ETR
- ITR
- RLOC
- xTR

---

2.
s/As described in the introduction/As described in Section 1/
s/LISP is limited to carry IP payloads/LISP is limited to carrying IP payloads/

---

4.1
s/field as g bit/field as the g-bit/

---

8.1
Please add RFC 8174