Last Call Review of draft-ietf-mpls-rfc4379bis-07
review-ietf-mpls-rfc4379bis-07-genart-lc-davies-2016-10-18-00

  
  
    I am the assigned Gen-ART reviewer for this draft. The General Area
    


    Review Team (Gen-ART) reviews all IETF documents being processed
    


    by the IESG for the IETF Chair.  Please treat these comments just
    


    like any other last call comments.
    





    For more information, please see the FAQ at
    







<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>

.
    





    Document:
    draft-ietf-mpls-rfc4379bis-07.txt


    Reviewer:
    Elwyn Davies


    Review Date:
    2016/10/21


    IETF LC End Date:
    2016/10/18


    IESG Telechat date: (if known)
    -





    Summary:
    Not ready.  There is one major issue (already notified to authors
    and agreed as an issue) and a considerable number of minor and
    editorial issues. I have worked through the various RFCs and errata
    that are subsumed into the new version and almost everything has
    been correctly translated AFAICS.  A couple of minor points are
    covered in the comments. 





    Major issues:
    


    ============


    s3.4: A number of items that are used in the normative Downstream
    Detailed Mapping TLV were originally defined in s3.3 (Downstream
    Mapping TLV format) but have been shifted to Appendix A.2.  This
    appendix is marked as non-normative.  Thus there are now no
    normative definitions for the various TLVs used in s3.4 that are
    defined in A.2.  I fear that these need to be returned to the
    normative part of the specification.





    [I think it would be simplest and least error prone to swap the text
    that was in s3.3 of RFC 4379 back out of A.2 and put backward
    references to the new s3.4 into A.2 as necessary.]





    Minor issues:
    


    ============


    Sender/receiver terminology: The document can be somewhat confusing
    to a naive reader.  Sender and receiver are used in multiple
    contexts.  Where the context appears to relate to LSP ping, both
    senders and receivers are involved in sending LSP ping packets.  In
    general, sender and receiver appear to imply sending and receiving
    of Echo Request messages with their roles reversed with respect to
    Echo Responses, with the receiver stimulated to send an Echo
    Response by receiving an Echo Request.  It would help if this
    terminology and usage was explicitly set out early in the document. 
    Additionally, some instances would be made more comprehensible by
    making the function explicit in the text e.g., in the operation of
    return codes.


     


    s1.4/s3/s6.2.3: The R (Global) flag is defined in RFC 6426. 
    Unfortunately it isn't in the IANA considerations there as was
    spotted in RFC Erratum 4012.  Might be worth mentioning the erratum
    (probably in s1.4?)  Alternatively this document can be made to
    provide the IANA specification for the R flag and the erratum
    closed. 





    s2.1/s6: An update to


http://www.iana.org/assignments/iana-ipv4-special-registry/iana-ipv4-special-registry.xhtml


    is needed to replace RFC 4379 with RFC-to-be for special exceptions
    to usage rules.





    s3.5, Clandestine Channel via Pad TLV:  As specified the value part
    of a Pad TLV can serve as a clandestine channel since the  value
    field contents are ignored.





    s3.5, Usefulness of Pad TLV:  Could you explain circumstances in
    which a Pad TLV would be needed please. I can't see any at present.





    Nits/editorial comments: 


    ======================


    General: s/i.e. /i.e., / (two instances  s3.2, last para; s4.5.1,
    para 3)





    s1, para 1: s/methods of reliable reply/methods of providing
    reliable reply/





    s1.4, bullet 4: Need to expand acronym PW on first use.





    s1.4, bullet 4: need to move expansion of FEC acronym to here from
    s2.





    s1.4, bullet 8: Acronyms DSMAP/DDMAP:  When defining Return Code 14
    in s3.1, the text is 'See DDM TLV...'.  DDM is not expanded anywhere
    although it is clearly the same as DDMAP.  But has by now made it
    into the IANA repository and is probably better to use it for
    'Downstream Detailed Mapping', so I suggest:


    OLD:


       o  Incorporate the updates from RFC 6424, by deprecating the


          Downstream Mapping TLV (DSMAP) and adding the Downstream
    Detailed


          Mapping TLV (DDMAP), updating two new return codes, updating
    the


          procedures, IANA section, Security Considerations, and
    obsoleting


          RFC 6424.


    NEW:


       o  Incorporate the updates from RFC 6424, by deprecating the


          Downstream Mapping TLV (DSM) and adding the Downstream
    Detailed


          Mapping TLV (DDM), adding two new return codes, updating the


          procedures, IANA section, Security Considerations, and
    obsoleting


          RFC 6424.


    END


    Then s/DSMAP/DSM/g, s/DDMAP/DDM/g in the rest of the document.





    s1.4:  Ought to mention the addition of the motivation (LSP
    stitching) for the additions in RFC 6424.





    s2.1, paras 7 and 8: This contains "the newly designated IPv4 link
    local addresses".  Given that RFC 3927 is now over 11 years old, the
    qualifier is no longer appropriate, but it might be useful to
    provide a ref. Thus:


    OLD:


    the newly designated IPv4 link local addresses


    NEW:


    the IPv4 link local addresses [RFC3927]


    END


    The text in para 8 is also no longer appropriate. Suggest


    OLD:


       Furthermore, the IPv4 link local address range has only recently
    been


       allocated.  Many deployed routers would forward a packet with an


       address from that range toward the default route.


    NEW:


       Older deployed routers may not correctly implement link local
    addresses


       and would forward a packet with an address from that range toward
    the 


       default route.


    END





    s2.1, para 9: s/embedded in as/embedded in an/





    s2.1, para 9: Useful to add a reference to RFC 4291.





    s2.2, para 1:  To be clearer about the distinction between IPv4 and
    IPv6, suggest:


    OLD:


       This document requires the use of the Router Alert Option (RAO)
    set


       in IP header in order to have the transit node process the MPLS
    OAM


       payload.


    NEW:


       This document requires that the Router Alert Option (RAO) is
    carried 


       in the IP header in order to have the transit node process the
    MPLS OAM


       payload.  For IPv4 packets the RAO [RFC2111] MUST be added to the
    IPv4 


       header; for IPv6 packets a hop-by-hop RAO [RFC2711] must be
    chained to 


       the IPv6 header. 


    END





    s3, para 1:







An MPLS echo request is a (possibly labeled) IPv4 or IPv6 UDP packet;





     This format applies to both requests and responses but the response
    case is not made explicit. Suggest:


    OLD:


    An MPLS echo request is a (possibly labeled) IPv4 or IPv6 UDP
    packet;


    NEW:


    An MPLS LSP ping message, is a (possibly labeled) IPv4 or IPv6 UDP
    packet;


    END





    s3, main packet format and associated text: The Sender's Handle is
    not the packet sender's handle but the Echo Request Sender's Handle
    - it is copied in to the corresponding Echo Reply.  Suggest renaming
    the Sender's Handle and Sequence Number to Echo Request Sender's
    Handle and Echo Request Sequence Number.  This would affect para 5
    of s4.3, para 2 of s4.5 and para 1 of s4.6 also. 





    s3, Timestamp format: RFC 5905 allows for 3 different time formats -
    the 32 bit basic format is intended:


    OLD:


       The TimeStamp Sent is the time-of-day (according to the sender's


       clock) in NTP format [RFC5905]


    NEW:


       The TimeStamp Sent is the time-of-day (according to the sender's


       clock) in 32 bit NTP format [RFC5905]


    END





    s3, Global flags: Technically, this doc only defines the V flag: 
    Also forcing the other bits to be zero restricts addition of new
    flags>


    OLD:


       This document defines three flags, the R, T, and V bits; the rest


       MUST be set to zero when sending and ignored on receipt.


    NEW:


       At the time of writing three flags are defined, the R, T, and V
    bits; the rest


       SHOULD be set to zero when sending and ignored on receipt.


    END





    s3, 

TLV types: The values 4, 6 and 8 for TLV type and the value
      5 for Tthe sub-type of TLV type 1 are specified as 'Not
      assigned':  To be clear for the future, should these really be
      marked as 'Reserved' or could they be assigned in future (and
      hence s/b marked as 'Available for assignment')?





    s3: For clarity it would be useful to add a sentence to the end of
    the section stating:


         In Sections 3.2 - 3.4 and their various sub-sections, only the
    value section of the TLV is specified.





    s3, TLV length calculation:  This is shown by example only.  I think
    it ought to be explained explicitly in text.  I suggest:


        The length of a sub-TLV or a TLV whose value is not a list of
    sub-TLVs


        is the number of significant octets in the value part of the
    (sub-)TLV 


        excluding any final padding.  If the value of a TLV is a list of
    sub-TLVs, 


        the length of the TLV is the sum of the overall lengths of the
    sub-TLVs 


        including the sub-TLV header and the length of the padding, i.e.


        4 + ((sub-TLV.length + 4) mod 4)





    s3.1, para 1: I think this should be interpreted as saying that the
    Return Code MUST always be zero in an Echo Request and the Return
    Code is set to an appropriate one of the possible values in an Echo
    Reply.  To be clear: I take it that it would not be normal for an
    Echo Reply to carry a zero Return Code.  Assuming this is right...


    OLD:


       The Return Code is set to zero by the sender.  The receiver can
    set


       it to one of the values listed below.


    NEW:


       The Return Code MUST be set to zero in an Echo Request message. 
    


       The responder sets the Return Code in the Echo Reply message to 


       an appropriate value other then zero from the list below. 


    END





    s3.1, Return code 14:  Some of the extra text from Section 3.2.1 of
    RFC 6424 ought to be essential as it contains 'MUSTS'.  Suggest
    adding this as an extra note against Return Code 14:


       Note 2: 


          Return Code 14 is used to indicate that an Echo Reply contains
    one or more


          DDM TLVs (see Section 3.4).  In this case there will be one
    Return Code and 


          corresponding <RSC> for each path described and these
    are passed in the 


          DDM TLV(s).  This Return Code MUST only be used in the Echo
    Reply message 


          header and MUST NOT be used in the Echo Request message even
    if the message 


          contains a DDM TLV.





    s3.1:  The term IS_EGRESS is used later in the document to indicate
    an Echo Reply message with a Return  Code of 3.  It should defined
    here.  The meaning is fairly obvious at its first use in s3.4(e) but
    there is not a formal definition.  (AFAICS textual acronyms are not
    used for any of the other codes.)





    s3.2, last but one para: s/previx/prefix/





    s3.2.8/s3.2.9/s3.2.11: It would be useful to use the name of the FEC
    type from RFC 4447 (PWid FEC) rather than just its number. (Also in
    A.1.1).





    s3.2.9: s/sender's PE IPv4 address/Sender's PE IPv4 Address/;
    s/remote PE IPv4 address/Remote PE IPv4 Address/





    s3.2.9, para 3: Need to expand PE acronym on first use.





    s3.2.10, para 1: The text uses source PE IPv4 address whereas the
    diagram uses Sender's PE IPv4 Address.  Consistency is needed.  See
    also the previous comment regarding consistency and capitalization.


      


    s3.2.10/s3.2.12: : It would be useful to use the name of the FEC
    type from RFC 4447 (Generalized PWid FEC) rather than just its
    number.





    s3.2.12: The text uses source whereas the diagram and field name use
    Sender's... consistency again?





    s3.4, DS Flags:







      I  Interface and Label Stack Object Request

         When this flag is set, it indicates that the replying
         router SHOULD include an Interface and Label Stack
         Object in the echo reply message.







    What circumstances would cause the replaying router not to do this? 
    What should it do otherwise?





    s3.4, Return Code:







      The Return Code is set to zero by the sender.  The receiver can
      set it to one of the values specified in the "Multi-Protocol Label
      Switching (MPLS) Label Switched Paths (LSPs) Ping Parameters"
      registry, "Return Codes" sub-registry.





    a) I suspect that in the basic LSP ping described in this document,
    the return codes that ought to be available are only those specified
    in s3.1 of this document except for 14 (which is specifically only
    allowed in the header).  The registry now contains a number of other
    return code values but a basic implementation wouldn't understand
    them in general.


    b)  See the previous comments on meaning of sender and receiver.
    Suggest:


    OLD:


          The Return Subcode is set to zero by the sender.  The receiver
    can


          set it to one of the values specified in the "Multi-Protocol
    Label


          Switching (MPLS) Label Switched Paths (LSPs) Ping Parameters"


          registry, "Return Codes" sub-registry.


    NEW:


          The Return Code in the (one) DMM TLV in an Echo Request
    message


          MUST be set to zero. The responder sets the Return Code in any
    


          DMM TLV in the Echo Reply message to an appropriate value
    other 


          then zero or 14  ("See DDM TLV for Return Code and Return
    Subcode") 


          taken from the list in Section 3.1.  


    END





    s3.4, Sub-tlv Length:  I think that the components of the DSM are
    all multiples of 4 octets long so there is no padding to consider
    (apart from possibly in FECs ).


    OLD:


          Total length in bytes of the sub-TLVs associated with this
    TLV.


    NEW:


          Total length in octets   of the sub-TLVs associated with this
    TLV including the TLV headers and any padding.


    END





    s3.4.1.3, FEC TLV length: Does this include any trailing padding and
    the TLV header?





    s3.4.1.3, Operation Rules: Shouldn't these be in s4?





    s3.6: Should contain a reference to the IANA registry URL.





    s4.1, last para: s/some information how each/some information as to
    how each/





    s4.2: s/to differentiate whether/to ascertain whether/





    s4.3, para 1: s/MUST be set in IP header/MUST be set in appropriate
    IP options/





    s4.4, item 1: It would be helpful to remind implementers how TLVs
    are marked to be ignored:


    OLD:


    If there are any TLVs not marked as "Ignore"


    NEW:


    If there are any TLVs not marked as "Ignore" (i.e., if the TLV type
    is less than 32768, see Section 3)


    END





    s4.4: s/subsection/Section/g





    s4.4, item 3: s/If there is no entry for L {/If there is no entry
    for Label-L {/





    s4.4, item 4:


    OLD:


                   Set Best-return-code to Return Code 9, "Label
    switched


                   but no MPLS forwarding at stack-depth" and set
    Best-rtn-


                   subcode to Label-stack-depth and goto
    Send_Reply_Packet.


    NEW:


                   Set Best-return-code to Return Code 9, "Label
    switched


                   but no MPLS forwarding at stack-depth" and set
    Best-rtn-


                   subcode to Label-stack-depth and goto step 7 (Send
    Reply Packet).


    END





    s4.4.1, item 5: s/advertise FEC/advertise the FEC/





    s4.5:







   If the replying router is the destination of the FEC, then Downstream
   Detailed Mapping TLVs SHOULD NOT be included in the echo reply.






    Under what circumstances  might one be included?  I think this is a
    MUST NOT.





    s4.5.2:  This section is derived from s4.1.2 of RFC 6424.  Whilst
    the new version appears to contain sufficient to define the proper
    normative behaviour, RFC 6424 contains additional examples of
    usage.  These look useful to me. I wonder if it might be useful
    either to copy the illustrative material to an appendix or maybe
    point back to RFC 6424. I am not sure how the powers-that-be would
    consider back pointers to obsoleted documents!  Maybe something
    like:


       [RFC6242] which originally specified the techniques needed to
    support tunnel transition contains some 


       examples, in Section 4.1.2, of situations where the technique
    would be applied.





    s4.6:


       If the echo reply contains Downstream Detailed Mappings, and X
    wishes


       to traceroute further, it SHOULD copy the Downstream Detailed


       Mapping(s) into its next echo request(s) (with TTL incremented by


       one).


    Presumably this means one DMM per Echo Request... might be worth
    being more explicit. 





    s5: Security risks of Router Alert.  Mention RFC 6398 and maybe copy
    2nd para of s6 of RFC 7506.





    s5, Security risks of DoS using Errored TLV?





    s6: Given the responses from IANA, a note is needed to say that
    entries originated other than from RFC 4379 should remain unaltered
    in the registry.  The only exception might be the R flag in Global
    Flags where it might be sensible to use this document to fix erratum
    4012.





    s6.2.5, last line: Remove ']]' which appears to be spurious.





    s8: Several new references are mentioned in these comments and would
    need to be added if the suggestions are actioned.