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

["Andrew G. Malis" <[e-mail]>]

Luca et al,

I just confirmed the submission for -02 and it’s up.

Cheers,
Andy


On Sun, Feb 12, 2017 at 6:56 PM, Luca Martini <[e-mail]> wrote:

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