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

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

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.

---

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

---

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."

---

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.

---

Section 3.1 needs to discuss how uniqueness of "S-PE addresses" is
guaranteed/arranged.

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.

---

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.

---

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?

---

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.

---

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.

---

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.

---

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.


    |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.

    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]."

---

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.  

---

4.1

   Selection of this next
   hop MAY involve a selection from a set of possible alternatives.

s/MAY/may/

---

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?

---

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."

---

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?

---

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. 

I presume it remains legal to have a Label Mapping message without an
ER TLV per point 2 of this section.

Perhaps you could clarify this.

---

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.