Re: [Pals] RtgDir review of draft-ietf-pals-status-reduction-01.txt

[Luca Martini <[e-mail]>]

Adrian,
Sorry for the late response , this fell though the cracks.
I'm having some trouble with the ID submission tool, but the revision
will be out shortly.
Comments below:
Thanks.
Luca

>     > -----Original Message-----
>     > From: Adrian Farrel [mailto:[e-mail]
>     <mailto:[e-mail]>]
>     > Sent: Wednesday, September 28, 2016 9:05 AM
>     > To: [e-mail] <mailto:[e-mail]>
>     > Cc: [e-mail] <mailto:[e-mail]>;
>     [e-mail]
>     <mailto:[e-mail]>;
>     > [e-mail] <mailto:[e-mail]>
>     > Subject: RtgDir review of draft-ietf-pals-status-reduction-01.txt
>     >
>     > 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
>     <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?)
>     >
>
I agree with your comment. In fact the mechanism is a bit complex, and I
went out of my way to ask people a few years ago to check it for problems.
At the time we fine tuned some things, and did not find any major
issues. I welcome anybody taking a close look at this !

>     > ==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.
>
OK, maybe George has something to add here , as I cannot remember adding
this section.
>
>     >
>     > 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.
>     >
>
I changed it. I meant "Label" in a more liberal term , as you guessed.

>     > Section 4
>     > You have not described all of the fields in the figure.
>
What did I miss ?
>
>     >
>     > 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"
>     >
>
Sure , but it has not been allocated yet. This is for the RFC editor to
replace the text once the allocation is dune.
 
>
>     > 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?
>     >
>
no - this is just a suggestion. Since this is local , it does not matter
, but I wanted a staring point that is known to work in case some junior
coder needed it.
Yes I agree that there should be a re-use and damping mechanism, but
that is over specifying it a bit.

>     > 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.
>     >
>
the remote PE will return a notification - 6  "PW configuration not
supported." . I'll add text to clarify this.
>
>     > 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
>     >
>
good catch - thanks.
>
>     > Section 4
>     > Do you want IANA to track the flags field?
>     >
>
no.
I think this needs to be allocated by a new RFC. Is that not the default ?
Making flags easy to allocate when there are only 6 bits is not wise and
will lead to misuse.

>     > 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.
>     >
>
yes- they are not OPTIONAL at random, but rather in sequence with the
length field used to parse where things stop....
I'll add text to clarify this.
NEW:
It should be noted that the Checksum, Message Sequence Number, Last
Received Message Sequence Number, Message Type, Flags, and control
message body are OPTIONAL. The length field is used to parse how many
optional fields are included. Hence all optional fields that precede a
specific field that needs to be included in a specific implementation
MUST be included if that optional field is also included. 
>
>     > 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.
>
no - the control message is the full message, while the control message
construct is only the control information without the Checksum,
 Message Sequence Number, Last Received Message Sequence Number, 
Message Type, Flags.
what is missing is this :

A PW refresh reduction message is also called a PW status refresh
reduction Control Message if it contains a  control message construct.

>     >   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.
>     >
>
the notification message is a different type , and cannot carry the
control construct.
the text I added should clarify that once a control structure is added ,
ad pw refresh message is also known as a PW status refresh reduction
Control Message.
However i also fixed the text to say "message sequence number" instead
of "control message sequence number" for clarity.

>     > 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.
>
These are really obvious. I do not see the point of adding obvious text.
Go look at all the BGP specs , most of them are extremely unreadable,
this is much more clear then any of those.
like if i receive an ack message sequence number that is after the last
message i sent , clearly is have a "Unacknowledged control message" error...

>
>     > Section 5.0.1
>     > What is the setting of the C flag on the Notification message?
>     >
>
The notification message has a different message type , and no C flag
defined in it.
i will add this :
 The 7 bits of flags are reserved for future use, they MUST be set to 0
transmission, and ignored on reception.

>     > 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."
>
yes. i thought this was obvious since there is no specification on how
to fragment the message itself... , but i will add it.
>
>     > - 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."
>
yes. i thought this was obvious as well since we have a C bit. However i
will add it.

>     >
>     > 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.
>
ok, but a TLV is a TLV .... i will add a picture of a TLV.
>
>     > However, you also need to describe how "unknown" TLVs are handled.
>     >
>
The unknown TLVs are handled as per section 4 . I will add a bit of text
there to clarify that the U bit applies unknown TLVs inside a known
message as well as a completely unknown message. Thanks for catching this.

>     > 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?
>     >
>
There are multiple combinations of configuration TLVs that can result in
invalid configurations for multiple reasons.
All result in returning a notification of "PW  configuration not supported".
That is explained in section 6
>
>     > 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?
>
yes good catch - 7 is the correct number.
There are multiple combinations of configuration TLVs that can result in
invalid configurations for multiple reasons.
All result in returning a notification of "PW  configuration not supported".
>
>     >
>     > 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."
>     >
>
Yes, this is implied in "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 will add it for clarity.


>     > 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
>     >
>
Yes , i fixed it as you suggested.

>     > 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)?
>
no. This protocol is for a direct attached point to point link. I'm not
clear how this would help in a attack given that anybody knows how to
calculate the checksum.


>     > Should you also give guidance (here or in section 4) about when
>     it is
>     > acceptable to use a zero checksum.
>     >
>
since i do not think that the checksum adds any security, i'm not clear
how it helps. Any transport link will have it's own error checking
mechanism anyway... This was only here in case some nasty people come
along an put this into a pseudoiwire ;-). And they should not !

>     > 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?
>     >
>
because when one range is exhausted, the next range will be harder to
exhaust , and so forth.
>From past experience, this allocation policy is  better at protecting
the limited resources , then FCFS when i cannot foresee all possible
outcomes...

>     > Section 8.1
>     > s/IETF consensus/IETF review/
>     >
>
fixed.
>
>     > 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.
>     >
>
we did not do that in the past.  I assume that the Expert is an Expert
in this field , and has good judgment.
maybe i should add "at sole discretion of the expert" ?

>     > 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.
>
??? i want the vendor proprietary extensions to be FCFS, does this not
say that ?
the point is that if something is not a proprietary extension it should
not use this.
for example it prevents people like me from using FEC type 128 for a
standard protocol ...;-)


>     >
>     > 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.
>     >
>
ok will add it.

>     > ==Nits:==
>     >
>     > I believe Luca may want to change his reported affiliation.
>     >
>
ok.
>
>     > 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
>
fixed.
>
>     >
>     > 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
>
fixed.
>
>     >
>     > 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.)
>     >
>
yes-  i fixed what I found , and the great RFC editor will do the rest.
>
>     > 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
>     >
>
no C in notification messages.... Besides this I'm unclear what is
different in your suggestion.

>     > Section 4 Unknown flag bit.
>     > s/MUST be acknowledge/MUST be acknowledged/
>     >
>
ok
>
>     > The sub-section numbering in Section 5 is broken. You can have,
>     > for example, "5.0.1".
>     >
>
fixed
>
>     > In a number of places you have "sub-TLVs" and in other places
>     "TLVs".
>     > I think they are all "TLVs".
>
yes, i mean sub-tlv because it's within another tlv.


>     > 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).
>     >
>
no, this is what this is . If you want an MPLS tunnel ID , it would be
different.

>     > 2.1.1, 2.1.3, and 5.0.2.3
>     > "unprovisioned"
>     > I think the word is "deprovisioned"
>     >
>
unprovisioned means it is not provisioned. deprovisioned , i take to
mean that it was once provisioned, now it is not ....
I like the more generic "unprovisioned"

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