Subject: [RTG-DIR] Review of draft-ietf-idr-bgpls-segment-routing-epe

From: Ravi Singh <[email-masked]>
Date: Fri, 14 Apr 2017 06:02:45 +0000

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.

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.

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.

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.

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?

Regards
Ravi

---

From: "Stefano Previdi (sprevidi)" <[email-masked]>
Date: Thu, 20 Apr 2017 11:48:12 +0000

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 <[email-masked]> wrote:
> 
> Hi
> …

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.

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

> 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 SubT…

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.

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

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