[mpls] Re: Shepherd review of draft-ietf-mpls-1stnibble

[Greg Mirsky <[e-mail]>]

Hi Adrian,
thank you for the review and your thoughtful suggestions. Please find my
notes in-lined below under the GIM>> tag. Also, attached are the new
working version and the diff highlighting the updates.

Regards,
Greg

On Tue, Jul 16, 2024 at 4:52 AM Adrian Farrel <[e-mail]> wrote:

> Hi,
>
> I'm your document shepherd for this journey.
>
> I have done my shepherd review of revision -08. I have quite a lot of
> nits, but also some more substantial points. I don't want to go against
> WG consensus, but I'd like you to answer my comments and we can see
> where to go from there.
>
> There may be more questions as I work on the shepherd write-up.
>
> Thanks,
> Adrian
>
> ===
>
> Number of authors
>
> While having more than five front-page authors is something we can
> achieve, I am required (as shepherd) to provide a credible reason. Can
> you supply me with this, or move someone to be a Contributor?
>
> Since you have one of your number designated as Editor, one solution
> would be to move everyone else to be Contributors.
>
GIM>> As Loa expressed in the separate thread, and I completely support him
as a co-author and the Editor of this document, all authors provided
essential contributions in the course of this work.

>
> ---
>
> Loa's affiliation.
>
> Are you sure that Loa's affiliation is still Huawei?
>
> ---
>
> id-nits
>
> Line 474 at the top of section 3.1 uses smart quotes. Please squash
> them.
>
> There is a nit in your BCP 14 boilerplate. s/BCP14/BCP 14/
>
GIM>> Fixed both

>
> ---
>
> Abstract
>
> Say what you are doing, not what you hope to do.
>
> s/The goal of this memo is to create/This memo creates/
>
GIM>> Accepted, thank you.

>
> ---
>
> Abstract
>
> The update reference for 4928 in the Abstract is too conditional. You
> already have "if approved" in the header. What you want in the rest of
> the text is to write material that can be included in the RFC without
> further modification. Thus, "This document updates RFC 4928."
>
> However, you should give a clue as to the nature of the update. You
> don't have to write an essay (that comes in the main body), but a simple
> "This document updates RFC 4928 by changing the colour of the curtains."
>
GIM>> Would the following update address this issue:
OLD TEXT:
    This memo, if published, would update RFC 4928.
NEW TEXT:
   This document updates RFC 4928 by deprecating the heuristic method
   for identifying the type of packet encapsulated in MPLS.

>
> ---
>
> Introduction
>
> The first paragraph makes a number of (true) statements that would
> benefit from citations.
>
GIM>> I've spurced the paragraph with references (PW CW and BIER header).
What would you suggest adding to further clarify the text?

>
> ---
>
> Introduction
>
> Pedantically, and with regard to the note that the information about the
> PSH and the embedded data packet is communicated via means such as the
> routing protocols that signal the labels in the stack, isn't it wrong to
> say that
>    the
>    correct interpretation of the Post-stack First Nibble (PFN) in a PSH
>    can be made only in the context of the Label Stack Element (LSE) or
>    group of LSEs in the preceding label stack that characterize the type
>    of the PSH
> This is nearly right, but I think that the LSEs indicate the context:
> they are not the context.
>
GIM>> Thank you for bringing this to the discussion. Indeed, it is the
context that is associated with label that is communicated via control or
management plane. I propose the following update:
OLD TEXT:
   the
   correct interpretation of the Post-stack First Nibble (PFN) in a PSH
   can be made only in the context of the Label Stack Element (LSE) or
   group of LSEs in the preceding label stack that characterize the type
   of the PSH
NEW TEXT:
   the correct interpretation of the Post-stack First Nibble (PFN) in a
   PSH can be made only in the context associated using the control or
   management plane with the Label Stack Element (LSE) or group of LSEs
   in the preceding label stack that characterize the type of the PSH

WDYT?

>
> ---
>
> 1.
>
>    *  To document the assignments already made.
>
> Technically, there being no registry up to now, no assignments have
> been made. Perhaps...
>
>    *  To document the values already in use.
>
GIM>> It makes text more accurate, thank you.

>
> ---
>
> 1.
>
>    *  To provide straightforward documentation of future assignments
>       through the creation of a "Post-stack First Nibble registry".
>
> This document doesn't provide that documentation. It provides a way to
> achieve that documentation. So...
>
>    *  To provide a mechanism to document future assignments through the
>       creation of a "Post-stack First Nibble" IANA registry.
>
GIM>> Thank you for your help in improving the quality of this document.
That is excellent suggestion.

>
> ---
>
> 1.
>
>    *  Provide a method for tracking usage by requiring more consistent
>       documentation.
>
> Consistent with what? Perhaps "more detailed" or "more rigorous"?
>
GIM>> I think that "detailed" is the right one. Thank you.

>
> ---
>
> 1.
>
> Getting ahead of the details of the document, but...
>
>    *  To stress the importance that any MPLS packet not carrying plain
>       IPv4 or IPv6 packets contains a PSH.
>
> So, when IPv13 comes along, this will or will not require the use of a
> PSH?
>
> I found the answer to this in 2.4, but I think you should call out that
> all other versions of IP MUST be encoded using a PSH.
>
GIM>> Would the forward reference to Sect.2.4 be acceptable and sufficient:
NEW TEXT:
   *  To stress the importance that any MPLS packet not carrying plain
      IPv4 or IPv6 packets contains a PSH, including any new version of
      IP (Section 2.4).

>
> ---
>
> 1.
>
> Again, avoid the conditional and provide RFC-ready text
>
> s/, if published, would update [RFC4928]/updates [RFC4928]/
>
GIM>> Right, it's about time ;)

>
> ---
>
> 1.1
>
> Expand BoS on first use.
>
GIM>> It turns out that it is the only place we use BoS. Replaced with the
expanded form.

>
> ---
>
> 1.1
>
>       Examples include a control
>       word or an associated channel.
>
> Needs a reference.
>
GIM>> Are these references what you envisioned:
NEW TEXT:
     Examples include a control
      word [RFC4385] or an associated channel [RFC4385], [RFC5586].

>
> ---
>
> 1.1
>
> Very odd to have the figures in this section especially as figure 2 is
> not mentioned until 2.1.1.1.
>
> I suggest breaking out to a new section "Reference Figures", adding a
> mention of Figure 2 within that section, and giving Figure 2 a caption.
>
GIM>> Was I able to address your recommendation with the updates?

>
> ---
>
> 1.1 Embedded packet
>
> Perhaps,
> OLD
>    Embedded Packet:  all octets beyond the PSH (if any).
> NEW
>    Embedded Packet:  all octets beyond the MPLS label stack excluding
>    the PSH (if present).
> END
>
> s/packet ,/packet,/
>
GIM>> Would the follwoing be acceptable:
NEW TEXT:
   Embedded Packet:  an embedded packet follows immediately after the
      MPLS Label Stack and an optional PSH.
END
WDYT?

>
> ---
>
> 1.1
>
>    Deprecation:  regardless of how the deprecation is understood in
>       other IETF documents, the interpretation in this document is that
>       if a practice has been deprecated, that practice should not be
>       included in the new implementations or deployed in deployments.
>
> s/the new/new/
>
> When you say "should not be ... deployed in deployments" what does that
> say about pre-existing implementations that are already deployed?
>
GIM>> We extensively discussed two actions - deprecate and obsolete. As I
understand them, your question about the latter. To that the document, in
update to Section 3 RFC 4928, states:
   |  At the time RFC XXXX was written, it was planned to obsolete MPLS
   |  encapsulations without PSH of non-IP payload when sufficient
   |  evidence exists that there are no marketed or deployed
   |  implementations using the heuristic practice.

>
> ---
>
> 1.2
>
> LSR is not used in the document
>
GIM>> I found two occurences in the definition of a Post-Stack Header:
      Post-stack Header (PSH):  optional field of interest to the egress
      Label Switching Router (LSR) (and possibly to transit LSRs).

> SPL could use a reference
>
GIM>> RFC 7274?

>
> Add
> BoS
>
GIM>> I've found it is used only once and I've replaced it with the
expanded form.

> G-ACh
>
GIM>> It is used only in the table for IANA new registry. I've replaced it
with the expanded form (and it fits)

> NSH
>
GIM>> Used several times but, as G-ACh, only in the table for IANA new
registry. Added the expanded form. WDYT?

> OUI
>
GIM>> I found it is used once and replaced it with the expanded form
"Organizationally Unique Identifier". Would you agree?

>
> ---
>
> 2.1
>
> Should the section title have a question mark?
>
GIM>> I defer that to people more proficient in English than I am. AFAICS,
"why" could be an introduction to an explanation, e.g., "Why we fight".

>
> ---
>
> 2.1
>
>    An MPLS packet can contain many types of embedded packets.
>
> I think an MPLS packet can only contain one type of embedded packet. So,
>
>    MPLS packets can contain many types of embedded packet.
>
> Or,
>
>    There are many types packet that may be carried by MPLS packets.
>
GIM>> I borrowed from your note:
NEW TEXT:
   An MPLS packet can contain one of many types of embedded packets.
WDYT?

>
> ---
>
> 2.1
>
>    The most common types are
>
> I have no doubt what you say is true, but it is something of an
> unsubstantiated assertion. It is also not clear that it will stand the
> test of time.
>
> Perhaps:
>    Three common types are
>
GIM>> Indeed. Thank you.

>
> ---
>
> 2.1
>
>    In addition, there may be a PSH ahead of the embedded packet, and it
>    needs to be parsed.  The PFN is currently used for both of these
>    purposes.
>
> a) "it needs to be parsed"  By whom? Why?
> b) "The PFN is currently used for both of these purposes." This is not
>    very clear as the first purpose (identifying the payload packet type)
>    has not be introduced as something that needs to be done, and the
>    second purpose (indicting that there is a PSH present) has not been
>    mentioned.
>
GIM>> To address a) and b) I propose the following update:
OLD TEXT:
   In addition, there may be a PSH ahead of the embedded packet, and it
   needs to be parsed.  The PFN is currently used for both of these
   purposes.
NEW TEXT:
   In addition, there may be a PSH ahead of the embedded packet.  The
   value of PFN is considered to ensure that the PSH can be correctly
   parsed.  If no PSH follows the label stack, then the value of PFN
   indicates the version number of the IP packet header.

Is it more clear now?

>
> ---
>
> 2.1.1
>
>    3.  One can do even better by "divining" the type of embedded packet,
>        and using fields from the guessed header.  The ramifications of
>        using this load-balancing technique are discussed in detail in
>        Section 2.2.
>
>    4.  One can do best by using either an Entropy Label [RFC6790] or a
>        Flow-Aware Transport (FAT) Pseudowire Label [RFC6391]; see
>        Section 2.2.)
>
> I think that is s/2.2/2.1.1.1/ in both cases.
>
GIM>> Great catch, thank you.

>
> ---
>
> 2.1.1
>
> "dangerous" may be over doing it. Perhaps "risky"?
>
GIM>> Agreed.

>
> ---
>
> 2.1.1.1
>
>    For example, if payload B is an
>    Ethernet frame, then the PFN is the first nibble of the OUI of the
>    destination MAC address, which can be 0x4 or 0x6, and if so would
>    lead to very bad load-balancing.
>
> "Very bad load-balancing" is an interesting phrase. Maybe expand a
> little, such as...
>
>    would lead to the packet being treated as an IPv4 or IPv6 packet such
>    that data at the offsets of specific relevant fields would be used as
>    input to the load-balancing heuristic resulting in unpredictable load
>    balancing.
>
> BTW "load-balancing" as an adjective, but "load balancing" as a noun.
>
GIM>> Thank you for the suggested text.

>
> ---
>
> 2.1.1.1
>
>    A consequence of that heuristic approach is that while legacy routers
>    may look for a PFN of 0x4 [RFC0791] or 0x6 [RFC8200], no router will
>    look for any other PFN, regardless of what future IP version numbers
>    will be, for load-balancing purposes.
>
> When you say "no router will look for", I there are two things:
> - No *legacy* router will look for any other future IP version numbers
>
GIM>> s/no router/no legacy router/

> - New routers that follow the recommendations in this document will not
>   look for other IP versions except if they are included in the registry
>
GIM>> I think that that will happen only after a new IP version is
published and the registry updated. Before that, a router that follows the
recommendation in the draft will not look for any value other than 0x4 and
0x6 as an indication that the embedded packet is an IP packet.

>
> But, possibly I am confused by "that heuristic" because the previous
> paragraph recommends a "mechanism" and recommends against using a
> "heuristic".
>
GIM>> s/that heuristic approach/the heuristic approach/?

>
> ---
>
> 2.2
>
> All of the text in 2.2 is very forward referential.
> I don't think it is right to update 4928 to say "if conformant to XXXX".
> You should just update the text to what you want the case to be. Thus...
>
>    NEW TEXT:
>
>    |  Network equipment MUST use a PSH (Post-Stack Header) with a PFN
>    |  (Post-stack First Nibble) value that is neither 0x4 nor 0x6 in all
>    |  cases when the MPLS payload is not an IP packet.
>
GIM>> Thank you for suggestions.

>
> Except, do you want "not an IPv4 or IPv6 packet."
>
GIM>> Yes, that is the practical outcome of the requirement. But we wanted
to arrive to it from the point of PFN values.

>
> ...and...
>
>    NEW TEXT:
>
>    |  The practice of deducing the payload type based on the PFN value
>    |  is deprecated to avoid inaccurate load balancing.  This means that
>    |  means that older implementations and deployments can continue to
>    |  use that heuristic, while it must not be part of new
>    |  implementations or deployments.  It also means that concerns about
>    |  load balancing for future IP versions with a version number of 0x0
>    |  or 0x1 are no longer relevant.
>
GIM>> Thenk you for the suggested text that further improves the document.

>
> I was also surprised by...
>
>    |  At the time RFC XXXX was written, it was planned to obsolete MPLS
>    |  encapsulations without PSH of non-IP payload when sufficient
>    |  evidence exists that there are no marketed or deployed
>    |  implementations using the heuristic practice.
>
> ... There was no discussion of this anywhere else in the draft. Anyway,
> it is really odd to see what-will-be RFC XXX saying "at the time
> RFC XXXX was written".
>
> I suggest you move this paragraph to a separate section that discusses
> the issue and proposes to collect evidence and make the further change
> in the future.
>
GIM>> Extracting that paragraph and adding a new section:
2.5.  Next Step to More Deterministic Load -balancing in an MPLS Network

   Network evolution is impossible to control, but it develops over a
   period of time determined by various factors.  This document prevents
   further proliferation of the implementations that could lead to
   undesired effects affecting data flow.  At some time in the future,
   it was planned to obsolete MPLS encapsulations without PSH of non-IP
   payload.  Before that it is paramount to collect sufficient evidence
   that there are no marketed or deployed implementations using the
   heuristic practice to load-balancing MPLS data flows.
What are your thoughts? Suggestions to improve the new section?

>
> ---
>
> I think section 2.3 is referring to the MNA work when it says...
>
>    The MPLS WG is currently engaged in updating the MPLS architecture;
>
> The world has moved on a bit, and you can be a bit more specific.
>
> Perhaps...
>
>    Support for MPLS Network Actions (MNAs) is described in
>    [I-D.ietf-mpls-mna-fwk] and is an enhancement to the MPLS
>    architecture.  The use of post-stack data (PSD) to encode the MNA
>    indicators and ancillary data is described in section 3.6 might place
>    data in the PFN that could conflict with other uses of that nibble.
>    This issue is described in section 3.6.1 of [I-D.ietf-mpls-mna-fwk]
>    and is further illustrated by the PFN value of 0x0 which has two
>    different formats depending on whether the PSH is a pseudowire
>    control word or a DetNet control word: disambiguation requires the
>    context of the service label.
>
>    With a registry, PSHs become easier to parse; not needing means
>    outside the data plane to interpret them correctly; and their
>    semantics and usage are documented.
>
GIM>> Updated as you sugegsted, thank you.

>
> ---
>
> 3.1
>
> I wonder whether the new registry will ever be used for anything other
> than MPLS.  The draft describes it as being to capture the first nibble
> after an MPLS label stack.  Doesn't that make it a part of the MPLS
> registry group?
>

GIM>> We had an early IANA review and, after a number of iterations, it
seems like IANA has agreed with the proposed update.