Re: [PWE3] AD review of draft-ietf-pwe3-mspw-er

["Adrian Farrel" <[e-mail]>]

All looking good to me, Matthew.
Just waiting for you to post.
A

> -----Original Message-----
> From: Bocci, Matthew (Matthew) [mailto:[e-mail]]
> Sent: 11 July 2014 13:52
> To: [e-mail]; [e-mail]
> Cc: [e-mail]
> Subject: Re: AD review of draft-ietf-pwe3-mspw-er
> 
> Hi Adrian,
> 
> Thanks very much for your detailed review.
> 
> I¹ve replied to your comments in-line below, and will revise the draft to
> incorporate the required changes, shortly.
> 
> Best regards
> 
> Matthew
> 
> 
> 
> On 11/06/2014 22:52, "Adrian Farrel" <[e-mail]> wrote:
> 
> Authors,
> 
> Thanks for your work on this document. I have conducted my normal AD
> review having received the publication request. The purpose of the
> review is to find issues and concerns that improve the quality of the
> document for readability and implementation, and to remove issues that
> might otherwise get found during IETF last call, directorate reviews,
> and IESG evaluation.
> 
> I think that in general this document is fine, but I think it needs some
> work on clarity. Hopefully none of my points is contentious, and they
> can all be handled through some fairly simple edits. of course, every
> point is open for discussion and you can refute each and every one of
> them.
> 
> I'll put the document into "Revised I-D Needed" state and wait to see an
> update and/or email from you.
> 
> Thanks,
> Adrian
> 
> ===
> 
> On a copyright thing...
> Are you sure that some of this material hadn't been lifted pretty much
> unchanged from RFC 3212? You might at least acknowledge that in the
> Acknowledgements section.
> 
> MB> Agreed. I propose adding the following text to the acknowledgements
> section as well as adding a reference to RFC3212 in section 4.2.
> 
> "The authors of this document gratefully acknowledge the contribution of
> the
> RFC3212 authors though the specification of TLVs, which are reused by this
> document."
> 
> ---
> 
> Section 1
> 
>    For 1:1 protection of MS-PWs with
>    primary and backup paths, MS-PWs SHOULD be established through a
>    diverse set of S-PEs (Switching Provider-Edge) nodes to avoid any
>    single points of failure at PW level.
> 
> I think s/SHOULD be/need to be/
> The points are:
> - This is really not a case for RFC 2119 language
> - "should" is not really good enough for proper 1:1 protection
> 
> MB> I agree
> 
> ---
> 
> Do you not think that the Introduction should discuss a little about how
> the path might be known? Specifically how the head-end T-PE knows what
> explicit route to use. It may be enough to say "In many deployments the
> head-end T-PE will not have a view of the topology of S-PEs and so the
> explicit route will need to be supplied from a management application.
> How that management application determines the explicit route is outside
> the scope of this document."
> 
> MB> Agreed. I'll add that statement to the introduction.
> 
> ---
> 
> Section 1
> 
>    This draft proposes an additional mechanism
> 
> Don't say "draft" say "document" because you want this text in an RFC.
> Don't "propose", "define" or "specify" because you want this in a
> protocol specification.
> 
> MB> Agreed
> 
> ---
> 
> Section 3.1 needs to discuss how uniqueness of "S-PE addresses" is
> guaranteed/arranged.
> 
> MB> Actually, this section is simply a duplication of RFC7267, Section
> 3.2. This is a
> result of this draft being forked from draft-ietf-pwe3-dynamic-ms-pw.
> While I agree
> with your comments below on this section, we could just
> rephrase the section in this draft as follows and raise an errata to
> clarify any
> points in RFC7267, as follows:
> 
> "An S-PE Address is an address used to identify a given S-PE among the set
> of S-PEs
> belonging to the PSNs that may be used by an MS-PW. Each S-PE MUST be
> assigned an address
> as specified in [RFC6267] Section 3.2. An S-PE that is capable of dynamic
> MS-PW signaling
> but that has not been assigned an S-PE address and receives a Label
> Mapping message
> for a dynamic MS-PW MUST follow the procedures of [RFC7276] Section 3.2."
> 
> 
> 
> I suggest you formally define the term "S-PE address"
> 
> 
> 
> What does "format similar to" actually mean?
> 
> 
> 
> ---
> 
> Section 3.1
> 
>    If an S-PE is capable of Dynamic MS-PW signaling, but is not assigned
>    with an S-PE address, then on receiving a Dynamic MS-PW label mapping
>    message the S-PE MUST return a label release with the "Resources
>    Unavailable" ( 0x00000038)" status code.
> 
> It is not clear to me whether this is new normative behavior or a
> restatement of draft-ietf-pwe3-dynamic-ms-pw. This isn't helped by the
> fact that "Dynamic MS-PW label mapping message" seems to be a new term.
> 
> You should probably define the term explicitly somehow with reference to
> what it contains that makes it such a message (not just an ordinary
> label mapping message) and a reference to draft-ietf-pwe3-dynamic-ms-pw.
> 
> Then you need to help clarify if this is new behavior. If it is not,
> then you can include "As described in [ref]..."  If it *is* new behavior
> for an existing message you have to explain how this is backward
> compatible with current processing of this message. If it is new
> behavior for a *new* message (i.e. defined in this document) then the
> text is very out of order and at the least you need some explanation of
> where this message is defined and what it does.
> 
> MB> Agreed. It should be aligned with RFC7267. Please see the proposal
> above.
> 
> ---
> 
> Section 3 might usefully say some overview stuff like:
> - An explicitly routed PW is set up using a Label Mapping message that
>   carries an ordered list of the S-PEs through which the PW is expected
>   to run.
> - The ordered list is encoded as a series of ER Hop TLVs encoded in a
>   ER TLV that is carried in the Label Mapping message.
> 
> Doing this would save the reader ploughing through section 3 trying to
> work out what it is all about.
> 
> MB> Agreed. I will add an introductory paragraph based on your text.
> 
> 
> 
> ---
> 
> If the ER Hop TLV can contain an IP address identifying an abstract
> node, why is it necessary for each S-PE to have a unique S-PE address
> formed as an AII?
> 
> MB>
> ---
> 
> Section 3.2
> 
>     Length
>           Specifies the length of the value field in bytes.
> 
> There is, of course, no "value" field, so you could be a little clearer.
> Unfortunately, there is currently a theme in the IESG on multi-byte
> integer fields that means you need to say how the integer is encoded on
> the wire, so you need to add "encoded in line format".
> 
> BUT...
> 
> Wait a minute. You are re-using 0x0800 for the ER TLV. Good. We don't
> need to define multiple TLVs for the same function. But why are you
> writing this document as though you are defining the TLV? Why is there
> no mention of RFC 3212?
> 
> The same questions apply to the ER Hop TLV and the two of its types
> that you are re-using.
> 
> This wouldn't take a lot of text and would allow you to delete a fair
> bit. Something along the lines of...
> 
>    The process defined in this document makes use of the ER TLV defined
>    in [RFC3212].  This TLV can be placed in a Label Mapping message used
>    for dynamic PW setup as described in Section 4.
> 
>    The ER TLV contains one or more ER Hop TLVs, each describing a hop
>    along the desired path or the PW.  The procedures described in this
>    document allow for the use of the "IPv4 Prefix ER-Hop TLV" and the
>    "IPv6 Prefix ER-Hop TLV" (both defined in [RFC3212]) and also the
>    "L2 PW address of PW Switching Point" defined in this document.
> 
> 
> MB> RFC3212 references were originally included in the text from
> draft-ietf-dynamic-ms-pw
> (see Version 13), but these were removed due to some comments around last
> call time that
> we were tying the fate of this draft to the future fate of RFC3212.
> Personally, I am
> OK with reinstating the references as you propose.
> 
> ---
> 
> Curiously, in 3.4.1 you have
>     IP Address
>           A four-byte field indicating the IP Address.
> while in 3.4.2 you have
>     IPv6 address
>           A 128-bit unicast host addresses.
> It makes one wonder.
> 
> This issue would disappear with the fix previously mentioned.
> 
> MB> OK
> 
> ---
> 
> For both the IPv4 and IPv6 hops, I think you need to think about (and
> document) whether you support less than fully specified IP addresses.
> For both of the prefix lengths you appear to allow 1-n (n = 32 or 128)
> yet for IPv6 you seem to say it is a unicast host address which would
> preclude anything other than 128.
> 
> MB> It does seem that way, although that's also how RFC3212 defines it.
> I will discuss with the other authors and make a proposal.
> 
> ---
> 
> 3.4.3 needs some work...
> 
> The section title says "ER-Hop 3: L2 PW Address" but while this may be
> the third ER-Hop type you support in this usage, the hop type is
> actually 5.
> 
> 
>  I suggest you remove the numbers from all three subsection
> titles.
> 
>      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
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |U|F|          0x0802           |      Length = 18              |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> The value you have used in 0x0805. But (of course) you should use
> TBD1 and reflect that in the IANA section.
> 
> MB> Agreed
> 
> 
>     |L|             Reserved                        |    PreLen     |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |  AII Type=02  |    Length     |        Global ID              |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> Having to fields called "Length" makes the documentation ambiguous.
> Suggest you make the first one "TLV-Length"
> 
> 
>     |       Global ID (contd.)      |        Prefix                 |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |       Prefix (contd.)         |        AC ID                  |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>     |      AC ID                    |
>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
>     U/F
>           These bits MUST be set to zero and the procedures of [RFC5036]
>           followed when the TLV is not known to the receiving node.
> 
> 
>      Type
>           A fourteen-bit field carrying the value of the ER-Hop 3, L2 PW
>           Address, Type = 0x0805
> 
>     Length
>           Specifies the length of the value field in bytes = 18.
> 
>     L Bit
>           Set to indicate Loose hop.
>           Cleared to indicate a strict hop.
> 
>     PreLen
>           Prefix Length 1-96
> 
> It is slightly curious that RFC 5003 didn't find a need for indicating
> the prefix length. Since it is (presumably!) your intention that the
> count to 96 spans the Global ID, Prefix, and AC ID fields, I think you
> need to make that clear here and describe its usage later. Otherwise
> one would rationally assume that the prefix length applies to the prefix
> field.
> 
> MB> Agreed. I will follow up with the co-authors to clarify and propose
> some revised text.
> 
>     L2 PW Address
>           An AII Address as defined in [RFC5003].
> 
> Rather than say this (which is confusing because there is no such L2 PW
> Address field) you should say "All other fields (AII Type, Length, Global
> ID, Prefix, and AC ID) define the L2 PW Address and are to be set and
> interpreted as defined in Section 3.2 of [RFC5003]."
> 
> MB> agreed
> 
> ---
> 
> 4.1
> 
>    A PW Label Mapping Message containing an explicit route TLV MUST
>    specify the next hop for a given MS-PW path.
> 
> What do you mean "MUST"? Surely...
> 
>    A PW Label Mapping Message containing an Explicit Route TLV specifies
>    the next hop for a given MS-PW path.
> 
> MB> agreed
> 
> ---
> 
> 4.1
> 
>    Selection of this next
>    hop MAY involve a selection from a set of possible alternatives.
> 
> s/MAY/may/
> 
> MB> OK
> 
> ---
> 
> 4.1
> 
>    The mechanism
>    used to select a particular path is also outside of the scope of this
>    document, but each node MUST attempt to determine a loop-free path.
>    Note that such mechanisms MAY be overridden by local policy.
> 
> This is ambiguous...
> 
> "MUST attempt" but if it can't find one it is OK to go ahead with a
> loop?
> 
> 
> Local policy may override attempting to find a loop-free path?
> 
> Local policy may override the mechanism to select a path (which is a
> local policy on account of not being part of this document)?
> 
> What information does an S-PE have available to select path and
> determine whether there is a loop?
> 
> What should an S-PE do if a path can't be selected without a loop?
> 
> 
> MB> I agree there are some procedures missing here. For a T-PE, if a loop
> free path
> cannot be found, then it must not attempt to signal the MS-PW. For an
> S-PE, if it cannot
> determine a loop free path, then the received Label Mapping MUST be
> released with a
> status code of "PW Loop Detected" as per Section 4.2.3 of RFC7267. I'll
> discuss with the
> authors and propose some text below.
> 
> ---
> 
> 4.1
> 
>    1.  The node receiving the Label Mapping Message must evaluate the
>        first ER-Hop.
> 
> This one should be "MUST", but you also need to qualify with "...that
> contains an ER TLV."
> 
> MB> agreed
> 
> ---
> 
> 4.1
> 
>        If the L bit is set and the local node
>        is not part of the abstract node described by the first ER-Hop,
>        the node selects a next hop that is along the path to the
>        abstract node described by the first ER-Hop.
> 
> And how will it make that determination?
> 
> MB> This is the same as RFC3212, where a specific node determines if it is
> a member of
> an abstract node if its address lies within the prefix (as determined but
> the prefix
> length) specified in the ER-Hop TLV.
> 
> 
> ---
> 
> 4.1
> 
>        If there is no first
>        ER-Hop, the message is also in error and the node should return a
>        "Bad Explicit Routing TLV Error" (0x04000001) status code in a
>        Label Release Message sent to upstream node.
> 
> I presume this means if there is an ER TLV that does not contain an ER
> Hop TLV.
> 
> MB> YEs
> 
> I presume it remains legal to have a Label Mapping message without an
> ER TLV per point 2 of this section.
> 
> MB> Yes
> 
> Perhaps you could clarify this.
> 
> MB> I'll clarify in the next version of the draft.
> 
> ---
> 
> Section 5
> 
>    This draft proposes one new TLV type:
> 
>    TLV Type                               Suggested Value   Reference
>    ------------------------------------   ---------------   ---------
>    L2 PW Address of Switching Point        0x0805           This Document
> 
> I think this needs to read...
> 
>    IANA is requested to assign a further code point from the IETF
>    Consensus portion of this registry as follows:
> 
>    TLV Type                               Value   Reference
>    ------------------------------------   ------- ---------
>    L2 PW Address of Switching Point       TBD1    This Document
> 
>    A value of 0x0805 is requested.
> 
> MB> Agreed.
>