Re: [spring] Routing directorate review of draft-ietf-spring-segment-routing-central-epe-04

[Jonathan Hardwick <[e-mail]>]

Thanks Stefano.  I'm happy with these resolutions.
Cheers
Jon

-----Original Message-----
From: Stefano Previdi (sprevidi) [mailto:[e-mail]] 
Sent: 08 March 2017 06:37
To: Jonathan Hardwick <[e-mail]>
Cc: [e-mail]; [e-mail]; [e-mail]; [e-mail]
Subject: Re: Routing directorate review of draft-ietf-spring-segment-routing-central-epe-04

Hi Jon,

many thanks for your review. Some comments inline.

where you don’t see any answer to your comments is because I applied them to the draft.


> On Mar 7, 2017, at 7:35 PM, Jonathan Hardwick <[e-mail]> wrote:
> 
> Hello
> 
> I have been selected to do a routing directorate “early” review of this draft.
> https://datatracker.ietf.org/doc/draft-ietf-spring-segment-routing-cen
> tral-epe/
> 
> The routing directorate will, on request from the working group chair, perform an “early” review of a draft before it is submitted for publication to the IESG.  The early review can be performed at any time during the draft’s lifetime as a working group document.  The purpose of the early review depends on the stage that the document has reached.  As this document is in working group last call, my focus for the review was to determine whether the document is ready to be published.  Please consider my comments along with the other working group last call comments.
> 
> For more information about the Routing Directorate, please see 
> ​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
> 
> Best regards
> Jon
> 
> 
> Document: draft-ietf-spring-segment-routing-central-epe-04.txt
> Reviewer: Jonathan Hardwick
> Review Date: 7 March 2017
> Intended Status: Informational
> 
> Summary
> Congratulations on a very clear and well-written document.  I have a few minor comments below but otherwise the document looks ready to advance.
> 
> Abstract
> s/It requires minor/It requires a minor/ Expand acronym SDN on 1st use
> 
> Section 1
> s/SID's/SIDs/
> 3rd bullet - why is the reference here?


I believe you refer to this paragraph:

   [I-D.ietf-spring-segment-routing] defines three types of BGP peering
   segments/SID's: PeerNode SID, PeerAdj SID and PeerSet SID.

the peerNode/Adj/Set segment are indeed defined in draft-ietf-spring-segment-routing. In this draft we illustrate the use case and the SR solution to it.


> "The solution is described for IPv4..." - I am obliged to discourage the use of exclusively IPv4 examples in this document.  See https://www.iab.org/2016/11/07/iab-statement-on-ipv6/.


I’ll work out that... there’s a substantial amount of addresses so I’ll be sure not to mess up everything ;-)


> Section 1.1 can be removed - section 13 lists the references.
> Section 1.2 bullet 6: s/ingress EPE/ingress PE/ Section 1.2 bullet 6: 
> s/at an source/at a source/
> 
> Section 3
> I found it a bit strange that you did not list the PeerNode segments contiguously in this section (they are 1012, 1022 and 1052).  But it's not a big deal - I can live with it.
> Section 3.6 s/An BGP-EPE enabled/A BGP-EPE enabled/
> 
> It's not clear if the FRR behaviour you are specifying in 3.6 is mandatory or just an example. 


in fact it’s just an illustrative example. I changed the “SHOULD” into a “MAY” and added more text.


>  However, the PeerNode SID and PeerAdj SID have the following backup rule.
> "2. Else backup via another PeerNode SID to the same AS."
> 
> That's reasonable under some circumstances but it might not agree with the policy of the adjacent AS.  For whatever reason that AS might want to steer traffic to certain IP destinations away from certain links, by not advertising BGP routes over those links, or advertising them with different MEDs.  Is there scope for the EPE controller taking these preferences into account?


yes, that’s a good point and I added text on that.


> Section 4
> s/an BGP-EPE controller/a BGP-EPE controller/ Section 4.1: When you 
> say "engineered peers" do you mean "BGP-EPE enabled border routers"?
> Section 4.1: "add-path all" sounds like a vendor specific CLI command.  Could you rephrase as "with the router configured to advertise all paths using BGP add-path [RFC7911]"?
> 
> Section 4.3: s/described in the section 2 (BGP-LS 
> advertisements)/described in section 2/ Section 4.4 s/an BGP-EPE/a 
> BGP-EPE/
> 
> Section 4.6 This section leaves me with a few questions.  What are 
> "business policies"?  How should they be collected, and why?  Do you 
> mean "collected" or "configured”?s


it could be both but of course these mechanisms are out of scope of this draft.


> Section 4.7: What is SID 64?  I infer it's the SID for PE C.  It should probably be given in section 3.


it is defined in 1.2

      “C’s loopback is 203.0.113.3/32 with SID 64”

I added a reference.


> Section 5
> Section 5.2 "The tunnel and the steering policy could be configured via..." - Do we need a list?  It could also be configured by CLI - does it matter?
> Section 5.3 s/them BGP upstream peers/their BGP upstream peers/ 
> Section 5.4 This example confused me as it appears to contradict section 1.2 bullet 1 when applied to Internet traffic.  Or is this example just talking about an inter-AS L3VPN service?


It’s doesn’t need to be “inter-AS”=. It’s a way to build a vpn route in the controller with a vpn label representing an EPE resource (peer, adj, set).


> Section 5.5 Unlike the other examples in section 5, the details of the FlowSpec route do not contain the actual IP addresses and SID/Labels in use.


I’d propose to remove the FlowSpec section since it has more to do with a SR policy definition and we have other drafts for that.


> 
> Section 7
> I don't think this section is required - I recommend taking it out.


I’m ok removing it, however I found it useful for the reader to grab the benefits (or characteristics/properties if you prefer) of the solution.


> Bullet 2 says that this works with "next hop self" but the example in section 4.1 does not use next hop self and I don't immediately see how it could work if next hop self was enabled on the BGP-EPE border router.


why not ? in fact, the re-writing of the next-hop is orthogonal to the solution. It is there just to facilitate the resolution (i.e.: avoid redistributing external interface routes into the igp.

Thanks.
s.


> s/avail the/assuming the/