Last Call Review of draft-ietf-teas-rsvp-egress-protection-09
review-ietf-teas-rsvp-egress-protection-09-genart-lc-bryant-2018-02-19-00

Request Review of draft-ietf-teas-rsvp-egress-protection
Requested rev. no specific revision (document currently at 16)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2018-02-27
Requested 2018-02-13
Authors Huaimo Chen, Autumn Liu, Tarek Saad, Fengman Xu, Lu Huang
Draft last updated 2018-02-19
Completed reviews Rtgdir Last Call review of -09 by Russ White (diff)
Genart Last Call review of -09 by Stewart Bryant (diff)
Secdir Last Call review of -09 by Rifaat Shekh-Yusef (diff)
Secdir Telechat review of -13 by Rifaat Shekh-Yusef (diff)
Genart Telechat review of -14 by Stewart Bryant (diff)
Assignment Reviewer Stewart Bryant 
State Completed
Review review-ietf-teas-rsvp-egress-protection-09-genart-lc-bryant-2018-02-19
Reviewed rev. 09 (document currently at 16)
Review result On the Right Track
Review completed: 2018-02-19

Review
review-ietf-teas-rsvp-egress-protection-09-genart-lc-bryant-2018-02-19

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-teas-rsvp-egress-protection-09

Reviewer: Stewart Bryant
Review Date: 2018-02-19
IETF LC End Date: 2018-02-27
IESG Telechat date: 2018-03-08

Summary: 

I think that there are a number of issues that need to be looked at in detail before this is ready. It could be that a number of these are assumed knowledge of the subject area, but that is not the case for all of them.

Section 6, the example, explains how this works and the motivation. It is very abbreviated and I am unclear about how the solution fits together.
This needs clarification and then the rest of the text needs checking against that.

There are a lot of cases where terms and abbreviations are used before definition.


Major issues:

Section 6 needs a re-write for clarity. As written it looks like a number of slides were just transcribed int the text rather than taking care to write for clarity.
The section makes a number of assumptions that really should be spelled out.

I think that it needs to be clear that you are protecting the egress nodes, but that as far as I can see you are vulnerable to egress link failure. Certainly the example in section 6 does a BFD test between R3 and L1 which only tests L1 reachability. Related to which are you constraining BFD to run over the LSP?  If it is running as regular IP I don't think that validates the LSP.

In this text: 

   This can be achieved by configuring
   a same local address on L1 and La, using the address as a destination
   of the LSP and BGP next hop for VPN traffic.

SB> Don't you have to do cost management to get the right behaviour?
SB> what if you really want to send to them individually, does this
SB> confuse things?

Minor issues:

1.1.  Egress Local Protection

   Figure 1 shows an example of using backup LSPs to locally protect
   egresses of a primary P2MP LSP from ingress R1 to two egresses, 

SB> What is an egress in this context?

=============

   x01 (Egress local protection bit):  It is set (1) to indicate an
      egress local protection.

   x02 (S2L sub LSP backup desired bit):  It is set (1) to indicate S2L
      Sub LSP (ref to RFC 4875) is desired for protecting an egress of a
      P2MP LSP.

SB> Maybe this is standard RSVP notation, but I find it confusing.
SB> I assume that x01 means bit one of the E-Flags field rather than
SB> set the flags field to be 0x01, but it is a strange notation.
SB> Same for x02. 
SB> is bit 1 bit 28 or bit 31 of the longword?
SB> S2L is not expanded in this document.

 The Reserved parts MUST set to zero.

SB> It is more conventional to call the parts fields as you do below.

==============


Nits/editorial comments: 

   The final (third) subobject in the SERO contains the egress node of
   the backup LSP, i.e., the address of the backup egress node.

SB> It is odd that the second item in the list is called the third
SB> and vice versa

============

   The Type of an IPv4 primary egress subobject is 1, and the body of
   the subobject is given below:

SB> This really ought to be tied in with the IANA registry
SB> Same with the similar IPv6 object, and the other
SB> two objects you register.

============

     o Tunnel ID:
         A 16-bit identifier being constant over the life of the tunnel
     o Extended Tunnel ID:
         A 4-byte identifier being constant over the life of the tunnel

SB> Tunnel ID really needs a reference.
SB> Also there ought to be MBZ consistency in the descriptions used in the
SB> document.

===========

 If S2L Sub LSP backup is desired to protect a primary egress of a
SB> I am not sure S2L is defined before use.
SB> Indeed the whole document could use a thorough scrub for used before defined terms.
SB> UA, PLR and Resv are another examples, but these are only the ones I noticed. The 
SB> draft needs a systematic review of this to save the RFC Editor doing it and asking about them.