Re: [Idr] Review of draft-ietf-idr-bgpls-segment-routing-epe

["Stefano Previdi (sprevidi)" <[e-mail]>]

Hi Ravi,

thanks for the review. I have integrated most of your comments and will submit a new version asap. Some more comments inline.


> On Apr 14, 2017, at 8:02 AM, Ravi Singh <[e-mail]> wrote:
> 
> Hi
> I had been designated as the RTG-DIR reviewer for this draft.
>  
> Overall the draft is clear. 
> However, it could be use some editorial revision for improved readability.
>  
> Specific comments listed below:
>  
> 1.       Document title could be less heavy on consecutive adjectives: some suggestions:
> a.       BGP-LS extensions for Segment Routing BGP Egress Peer Engineering
> b.      BGP-LS extensions for "BGP-EPE using SR"
> c.       Segment Routing BGP Egress Peer Engineering extensions for BGP-LS
> …
> 2.       Section 1:
> "   This document defines new types of segments: a Peer Node segment
>    describing the BGP session between two nodes; a Peer Adjacency
>    Segment describing the link (one or more) that is used by the BGP
>    session; the Peer Set Segment describing an arbitrary set of sessions
>    or links between the local BGP node and its peers."
> Above has an unintended meaning. This should instead have stated that this doc defines BGP-LS extensions to communicate the above listed SIDs that are defined in "draft-ietf-spring-segment-routing"
>  
> 3.       Section 3: 
> "
>    This document defines the BGP-EPE Peering Segments:
>  
>    o  Peer Node Segment (Peer-Node-SID)
>  
>    o  Peer Adjacency Segment (Peer-Adj-SID)
>  
>   o  Peer Set Segment (Peer-Set-SID)"
>  
> Same issue as listed above for section 1 above.
> Section 5 has the same issue.


indeed there’s a bit of confusion between “segment” and “SID” terminology. While the segment has been defined in draft-ietf-spring-segment-routing-central-epe, this draft defines the SID and how they are advertised in BGP-LS. I updated the text.


> 4.       Section 4.2: I'd suggest that this be broken into 2 separate sub-sections: each listing the mandatory/optional sub-TLVs for the local and remote node descriptors respectively. That will make for improved readability.
>  
> 5.       Section 4.2: please clarify in text as to Why no "BGP-LS ID" in link NLRI in the remote node descriptor.


Well, this is not related to EPE but to BGP-LS base spec where the BGP-LS identifier is locally assigned.

Therefore, a node running EPE and describing [i|e]BGP topology, doesn’t know the BGP-LS identifier of its peers.


> 6.       Section 4.3: 
> a.       values of the flags are specified only for the Per-Adj-SID. Explicit text should be listed for per-node-SID and per-set-SID.
> b.      "
>    The Peer-Node-SID MUST be present when BGP-LS is used for the use
>    case described in [I-D.ietf-spring-segment-routing-central-epe] and
>    MAY be omitted for other use cases."
> Is there really a need to state what other use-cases might do, considering that this doc is specific to BGP-LS for SR-EPE? Perhaps can be reworded. Similarly for "Peer-Adj-SID and Peer-Set-SID SubTLVs MAY" in next paragraph.


Well, we don’t want to shut the doors to other use cases that could leverage the same encodings so we just state what EPE requires but certainly, other options are available.

 
> 7.       There is some repetition of info between sections 4 & 5. eg. What local and remote node descriptors contain. Please reword these sections to avoid the repetition while still presenting the info.
>  
> 8.       Sections 5.1 & 5.2: have lots of repetitive text. Tabular presentation of the text in these sections would allow describing per-node-sid and per-adj-sid without the repetition.


Well, these sections are the core of the draft:
. section 4 describes the encoding of the TLVs/attributes
. section 5 describe how a SID is constructed.

I know it’s quite a bit of information but since w’re supposed to be exhaustive in the way encoding is done, I believe these details are wroth to be mentioned.


> 9.       Section 9: typo in language in first sentence.
>  
> 10.   Section 10: please explicitly call out any additional (beyond rfc7752) security considerations or include text stating that none exist.
>  
> 11.   Typos
> a.        Section 3:
>                                        i.            "an BGP-EPE" -> "a BGP-EPE"
> b.      Section 4:
>                                        i.            "Link-type" NLRI -> "link-state" NLRI?


RFC7752 section 3.2.  defines the “Link NLRI” type which is one possible type for the Link-State NLRI.

s.



>  
> Regards
> Ravi