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

["Bocci, Matthew (Matthew)" <[e-mail]>]

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.