Definition of P2MP PW TLV for LSP-Ping Mechanisms
draft-ietf-pals-p2mp-pw-lsp-ping-05

Note: This ballot was opened for revision 03 and is now closed.

Deborah Brungard Yes

Alia Atlas No Objection

Ben Campbell No Objection

Comment (2017-06-21 for -03)
Please expand LSP and FEC on first mention. Please consider expanding PSN and LDP (I see they are marked as "well-known" in the abbreviation list, but I think expansion would be helpful in context.)

Benoit Claise No Objection

Comment (2017-06-21 for -03)
   The LSP Ping Echo request IPv4/UDP packets is encapsulated with the
   MPLS label stack as described in previous sections, followed by one
   of the two encapsulation options:

IPv4 only?

And here Tianran Zhou's OPS DIR review:
No issue found. This document is well written, and is ready for publication.
Only a couple of nits, for the authors consideration:

1. in section 1, "Multipoint LDP (mDLP)" 

Is the acronym "mLDP"?

2. in section 1, 
"Multi-segment Pseudowires support is out of scope of this document at
   present and may be included in future."

At this stage, the I-D is stable. I think you can just say "Multi-segment Pseudowires support is out of scope of this document".

3. in section 6,
"MLDP" should align with the previous acronym in section 1, i.e. "mLDP".

And there is auto check result from the system:

  == Unused Reference: 'RFC5085' is defined on line 325, but no explicit
     reference was found in the text

  ** Obsolete normative reference: RFC 4379


     Summary: 1 error (**), 0 flaws (~~), 1 warning (==), 1 comment (--).

     Run idnits with the --verbose option for more detailed information about
     the items above.

Alissa Cooper No Objection

Spencer Dawkins No Objection

Comment (2017-06-19 for -03)
I share Mirja's curiosity about the potential for amplification attacks, but I'll watch that conversation, so no need to reply to me.

Suresh Krishnan (was Discuss) No Objection

Comment (2017-08-10 for -04)
* I think the P2MP Pseudowire Sub-TLV in Section 4.1 is a bit under-specified. It is unclear how the address family of the originating router's IP address is communicated. Is this purely derived from the IP Addr Len (i.e. Len is 4 => IPv4, Len is 16 => IPv6)? If so, I think it would be useful to state this explicitly and add some validity checking and error handling for values other than 4 and 16. [Authors clarified that this is intentionally conveyed this way as they do not expect to support any other address families]

* Are there no alignment requirements for the IP address in "Originating Routers IP Addr" inside the sub-TLV? I would think that alignment on a 4 byte boundary might be needed.

Warren Kumari No Objection

Comment (2017-06-21 for -03)
I like the solution, but the document could do with some editing.

Major:
1: Sec 1.  Introduction
O:  Multi-segment Pseudowires support is out of scope of this document at  present and may be included in future.
P:  Multi-segment Pseudowires support is out of scope of this document.
C: Once published as an RFC, the document doesn't change. Could be "... may be addressed in a future document", but I'd suggest leaving it out.

2: General
The document has many unexpanded acronyms, e.g: ACH in "... MPLS label stack and IPv4 or IPv6 ACH."  In the Introduction you have: "such as P2MP ATM over PSN." - while PSN might count as a well known acronym, it feels like, in an Intro it should be less opaque - see https://www.rfc-editor.org/materials/abbrev.expansion.txt for RFC known acronyms.

3: The "Controlling Echo Responses" section feels weak -- it says that  "The procedures ... **can** be applied to P2MP PW LSP Ping." (emphasis added) - it feels like this should be a SHOULD? I think better a description of the DoS implications (other than just pointing at RFC6425) is also important. 


Nits:
1: The document would benefit from some serious grammar checking -- e.g:
"... Echo Request to inform the receiver at P2MP MPLS LSP tail, of the P2MP PW being tested." - extra ','.
"For Inclusive P-Trees, P2MP MPLS LSP label itself can uniquely identify the Throughout the document..." - missing 'the' - things like this, and confusion over plurals (especially near acronyms) makes the document hard to read / review.

2: "P2MP ATM over PSN.   Requirements for ... " - extra space (nit!)

3: Sec 8.  Security Considerations
"The proposal introduced in this document does not introduce any new security considerations beyond that already apply to [RFC6425]." -- this sentence is poorly formed. Perhaps "beyond those that..."? Or "beyond those in"?

Mirja K├╝hlewind No Objection

Comment (2017-06-18 for -03)
I understand that this document does not introduce any new mechanisms compared to rfc6425, however, I think both documents enable an amplification attack. Is this not a concern or should that be discussed somewhere?

Terry Manderson No Objection

Alexey Melnikov No Objection

Kathleen Moriarty No Objection

Eric Rescorla (was Discuss) No Objection

Comment (2017-06-20 for -03)
Document: draft-ietf-pals-p2mp-pw-lsp-ping-03.txt

Assuming I am understanding this document correctly, it's just a refinement
of MPLS Echo to point out a specific receiver to respond. Is that correct?
If so, perhaps you could make that clear in the intro.

This document is pretty acronym heavy. Please ensure that every acronym
is expanded on first use. Examples include LSP, VPN, VPLS, etc.
Similarly, LSP Tail/Bud, etc. need citations.

Figure 2 would benefit from some explanations of the packet flow. Where
does the echo packet start, where does it stop? Who answers, etc.

Alvaro Retana No Objection

Comment (2017-06-19 for -03)
I agree with Suresh's DISCUSS.

Adam Roach No Objection

Comment (2017-06-21 for -03)
I share Suhas' concerns.

Please expand the following acronyms upon first use and in the title;
see https://www.rfc-editor.org/materials/abbrev.expansion.txt for guidance.

 - P2MP - Point-to-Multipoint
 - PW - pseudowire
 - LSP - Label Switched Path
 - VPLS - Virtual Private LAN Service
 - TE - Traffic Engineering (?)
 - FEC - Forwarding Equivalence Class
 - LSR - Label Switching Router
 - GAL - Generic Associated Channel Label
 - ACH - Associated Channel Header 
 - CE - ???
 - PE - Provider Edge