Subject: [RTG-DIR] RtgDir review of draft-ietf-pals-status-reduction-01.txt

From: "Adrian Farrel" <[email-masked]>
Date: Wed, 28 Sep 2016 14:04:42 +0100

Hello,

I have been selected as the Routing Directorate reviewer for this draft. The
Routing Directorate seeks to review all routing or routing-related drafts as
they pass through IETF last call and IESG review, and sometimes on special
request. The purpose of the review is to provide assistance to the Routing ADs.
For more information about the Routing Directorate, please see
http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Although these comments are primarily for the use of the Routing ADs, it would
be helpful if you could consider them along with any other IETF Last Call
comments that you receive, and strive to resolve them through discussion or by
updating the draft.

Document: draft-ietf-pals-status-reduction-01.txt
 Reviewer: Adrian Farrel
 Review Date: 27 September 2016
 IETF LC End Date: Not yet started
 Intended Status: Standards Track

==Summary:==
I have some minor concerns about this document that I think should be resolved
before publication.
The volume of minor concerns add up to a significant concern although no one
issue is large. I recommend that the Routing ADs discuss these issues further
with the authors and consider whether an updated I-D would need to be
re-reviewed by the WG.

==Comments:==
This document defines a mechanism to "bundle" status reports on the PWs carried
on an MPLS tunnel. The effect is to significantly reduce the number of messages
need in the (normal) stable state.
I don't see any problems with the mechanism defined and I am sure it would work.
The writing style is mainly clear.
However, there is a host of minor issues and holes in the documentation that
puts the specification below the level necessary to guarantee interoperable
implementations.
My review is not as an implementer, and it worries me that if I find so many
questions and issues, an implementer would find far more.
I checked to see whether anyone had commented in WGLC that they had read the
document and I didn't see anything (perhaps my list subscription is broken?)

==Minor Issues:==

Section 1.3
I think you need to give a reference for the base BNF you are using.
You could choose from RFC 5234 or RFC 5511.
But it seems peculiar that you have set out to define your own notation for
things that can be expressed in existing BNF.

Section 4
The figure makes it look like MPLS labels are 32 bits long.
I guess you could fix this by s/label/label stack entry/
Or you could show all of the LSE fields.

Section 4
You have not described all of the fields in the figure.

Section 4
You have...
Channel type 0xZZ pending IANA allocation.
...but no mention in the IANA section
The figure shows "0xZZ PW OAM Message", but surely this is 
"PW Status Reduction Message"

Section 4
In the description of the Session ID CRC16 seed really "recommended"?
I mean, is this "RECOMMENDED", and if so, why?
You *do* need a locally selected, locally unique value.
It does not need to be unique over time, but you may need suggest a
damping mechanism on re-use of session ID.
However, recommending this mechanism for generating session IDs seems
to me to be over-weaning. What is the reason?

Section 4
There are a number of rules expressed here about the setting of the
message fields (such as the Refresh Timer that is a "non zero unsigned
16 bit integer value greater or equal to 10").  What happens if a
message is received that violates one of these rules.

Section 4
OLD
       7 bits of flags reserved for future use, they MUST be set to 0 on
       transmission, and ignored on reception.
NEW
       6 bits of flags reserved for future use, they MUST be set to 0 on
       transmission, and ignored on reception.
END

Section 4
Do you want IANA to track the flags field?

Section 4
You have...
   It should be noted that the Checksum, Message Sequence Number, Last
   Received Message Sequence Number, Message Type, Flags, and control
   message body are OPTIONAL.
This *sounds* like each field is individually optional. But that is not
the case, I think (because the resulting message would be impossible to
parse). So you need to clarify.
Probably that the length field indicates where the sequence stops, or 
that the whole set may be omitted or included en mass.

Section 5 opening paragraphs are confusing.
Either you have a "control message" or you have a "control construct
carried on a status reduction message".
- It is possible you mean the former.
  That is "There are two status reduction messages defined in this
  document and they are both control messages: the Notification message
  and the PW Configuration message.
- It is possible you mean have the latter.
  Thus, when you talk about the "message sequence number of the control
  message" you really mean the "message sequence number of the status
  reduction message that carries the control construct".
  You can probably note that the control construct can be carried on
  a Notification message or a PW Configuration message. 
  You should probably also say whether the control construct can be 
  carried on other (future) messages.

Section 5
In 8.3 you list a number of notification codes. A little can be deduced
from their names, but you do not describe anywhere when or why most of 
them are used (5.0.2.3 and 6 are the exceptions). You should, and 
section 5 or 6 is probably the place.

Section 5.0.1
What is the setting of the C flag on the Notification message?

Section 5.0.2
   The message has no                                 
   preset length limit, however its total length will be limited by the
   transport network Maximum Transmit Unit (MTU).
This is fine, however:
- You probably want to add "Status Reduction messages MUST NOT be
  fragmented."
- You probably also want "If a sender has more configuration information
  to send than will fit into one PW Configuration Message it may send
  further messages carrying further TLVs."

Section 5.0.2
I think you need to define a generic TLV format so that new TLVs can
be added and parsed over. Since you use a common format for your TLVs,
this should not be hard.
However, you also need to describe how "unknown" TLVs are handled.

Section 5.0.2.1
Is the MPLS-TP Tunnel ID TLV optional? 5.0.2.2 shows the PW ID
configured List TLV as optional, so it looks like the MPLS-TP Tunnel ID 
might be mandatory TLV.
Furthermore, you seem to have said that the order of TLVs is arbitrary
but wouldn't it be best to have this TLV show up first?
What happens if there are multiple of this TLV present?

Section 5.0.2.2 (and 5.0.2.3)
   The number of PW Path IDs in the TLV will be inferred by the length
   of the TLV up to a maximum of 8. 
Now, "The PW Path ID is a 32 octet pseudowire path identifier"
8*32=256
The Length field has 8 bits and so encodes a maximum of 255 octets.
So you can actually only carry 7 PW Path IDs in this TLV.
Furthermore, Length values that are not a multiple of 32 are an
encoding error, but you haven't stated how to handle encoding errors.
What happens if the same PW Path ID appears twice in a list?

Section 6
   A PE that desires to use the PW configuration message to verify the
   configuration of PWs on a particular LSP, should advertise its PW
   configuration to the remote PE on LSPs that have active keepalive
   sessions.
I think that probably hidden in here is "MUST NOT include a PW
configuration message on a status reduction message unless the local
protocol state is ACTIVE."

Section 6
   the following action SHOULD be taken:
        -i. The local PW MUST be considered in "Not Forwarding" State.
That creates a composite "SHOULD-MUST" which is not very clear.
Suggest you change to...
   the following actions are taken:
        -i. MUST
        -ii. SHOULD
        -iii. SHOULD

Section 7
Why does this section not discuss the Checksum field? Doesn't this add
a measure of security (at least against simple, random attacks)?
Should you also give guidance (here or in section 4) about when it is
acceptable to use a zero checksum.

Section 8.1 - meta-point to be addressed before the following issue
with this section...
You seem to have an overly-complex assignment policy for this registry.
All of the types seem to have equal weight (i.e., there are no "special
purpose" values yet you cover the range with:
- Expert review
- IETF review
- FCFS
Since FCFS guarantees an assignment without any further review, why
would anyone bother with Expert Review.  For that matter, why would
anyone bother with IETF Review?
So, why not make life easier and say all code points are FCFS?

Section 8.1
s/IETF consensus/IETF review/

Section 8.1
You have a range assigned by Expert Review. It is a requirement that
you give guidance to the experts about how they should review
requests.

Section 8.1
The values 128 through 254 are assigned using FCFS. You cannot then
attempt to further constrain how the values are used (e.g., "for vendor
proprietary extensions"), and you should avoid the word "reserved" since
it has special meaning for IANA.

Section 8.2
I have essentially an identical set of points about 8.2 carried from 8.1

Section 8.3
Again the same set of comments.
Additionally, you need to tell IANA what the "Error?" column means. This
is probably...
   For each value assigned IANA should also track whether the value
   constitutes an error as described in Section 5.0.1.  When values are
   assigned by IETF Review, the setting of this column must be
   documented in the RFC that requests the allocation.  For Expert
   Review and FCFS assignments, the setting of this column must be made
   clear by the requested at the time of assignment.

==Nits:==

I believe Luca may want to change his reported affiliation.

Abstract
OLD
   This document describes a method for generating an aggregated
   pseudowire status message on Multi-Protocol Label Switching (MPLS)
   network Label Switched Path (LSP).
NEW
   This document describes a method for generating an aggregated
   pseudowire status message transmitted on a Multi-Protocol Label
   Switching (MPLS) Label Switched Path (LSP) to indicate the status of
   one or more pseudowires carried on the LSP.
END

Introduction
Expand "PW" on first use.
s/they are setup/they are set up/

Section 2
"MPLS Generic Associated Channel" needs a reference to 5586

Throughout the document you need to be consistent (and with prior art)
about capitalisation ("generic associated channel" or "Generic Associated
Channel" etc.) and abbreviations ("GACH" or "G-ACh" etc.)

Section 4
Message Sequence Number
s/A unsigned/An unsigned/

Section 4 (from the figure)
OLD
   |  Last Received Seq Number     | Message Type  |U C Flags      |
NEW
   |  Last Received Seq Number     | Message Type  |U|C|Flags      |
END

Section 4 Unknown flag bit.
s/MUST be acknowledge/MUST be acknowledged/

The sub-section numbering in Section 5 is broken. You can have,
for example, "5.0.1".

In a number of places you have "sub-TLVs" and in other places "TLVs".
I think they are all "TLVs".

In 5.0.2.1 "the address of the MPLS-TP tunnel ID" is confused. It is
"the identifier of the MPLS tunnel". Indeed, where did MPLS-TP suddenly
come from since you want this to work for all LSPs (see also 8.2).

2.1.1, 2.1.3, and 5.0.2.3
"unprovisioned"
I think the word is "deprovisioned"

Section 6
   If
   a PE receives such a notification it should stop sending PW
   configuration control messages for the duration of the PW refresh
   reduction keepalive session.
s/should/SHOULD/

Section 8
   All the registries in this section are to be created or updated as
   apropriate in the PW Name Spaces.
No such name space error.
I think you mean "Pseudowire Name Spaces (PWE3)"
s/apropriate/appropriate/

s/10. Author's Addresses/10. Authors' Addresses/