Re: [RTG-DIR] RtgDir Early review: draft-ietf-idr-bgp-ls-segment-routing-ext-06

["Ketan Talaulikar (ketant)" <[e-mail]>]

Hi Victoria,

Thanks a lot for your comments and inputs to improve the document.

Please check inline below for response. I will update the document next week and please let know your feedback in the meanwhile.

Thanks,
Ketan

-----Original Message-----
From: Victoria Pritchard <[e-mail]> 
Sent: 18 May 2018 03:25
To: [e-mail]; [e-mail]
Cc: [e-mail]; [e-mail]
Subject: RtgDir Early review: draft-ietf-idr-bgp-ls-segment-routing-ext-06

Hello

I have been selected to do a routing directorate “early” review of this draft.
https://tools.ietf.org/html/draft-ietf-idr-bgp-ls-segment-routing-ext-06

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.

For more information about the Routing Directorate, please see ​ http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Document: draft-ietf-idr-bgp-ls-segment-routing-ext-06.txt
Reviewer: Victoria Pritchard
Review Date: 17/05/2018
Intended Status: Standards Track

Summary:
I have some minor concerns about this document that I think should be considered, just a few things that weren't clear to me, but these may be obvious to the intended audience.

Comments:

(1)
I found the document difficult to read with so many references to definitions in other drafts. I noticed that the other drafts sometimes duplicate text from each other, so wonder if that should be done here too to aid readability, along with a comment that these parts have the same definition in the IS-IS and OSPF extensions too. e.g. 2.2.1 but applies throughout.
[KT] By its very nature, most of BGP-LS extensions derive and build upon the technology/architecture (in this case SR) and base IGP extensions (in this case ISIS/OSPF SR extensions). Without the context from both the feature/architecture and IGP specifications, I agree that it is difficult to read BGP-LS documents. However, I do not think it makes sense to duplicate text from across multiple documents. Many a times those other documents also evolve based on reviews and inputs so it is likely that such copy/past text gets out of date.

(2)
I found Table 5 6 and 7 really useful, since it became clear that the TLVs defined here for BGP map to OSPF and IS-IS extensions. I'd like to see these tables earlier in the draft.
[KT] Sec 2 text at the very beginning does say what you are asking for and puts forward reference to the exact same tables you point to. IMO putting a table with things that are not defined up front may not be as friendly.

(3)
Sections 2.1, 2.2, 2.3.1, 2.3.4 all mention "SR TLV" or "corresponding TLV"
but it wasn't clear what these were.
[KT] They are actually described right below in following sub-sections. I will change the text to " the corresponding underlying IGP TLV/sub-TLV described below" so this is clearer.

(4)
Section 2.1.1
If length is 3 and the rightmost 20 bits are a label, what are the other bits for?
[KT] There are no "other bits". When the TLV is encoding a label then its size of the value field is 3 and the overall TLV size would be 7.

(5)
2.1.2. The capabilities TLV "advertise the node's SR capabilities and its Segment Routing Global Base (SRGB) range(s)", but the format diagram only contains Segment ID information. What are the capabilities? Or does the presence of this TLV indicate the capability?
[KT] The Capabilities are the SRGB and the flags carried in the TLV.

(6)
2.1.2, 2.1.4
The range size is followed by a single SID? Is this the same for the Range TLV in 2.3.4?
[KT] Sec 2.1.2 and 2.1.4 the range is followed by a SID/label TLV and the text puts the reference to 2.1.1 for it. Under Sec 2.3.4 the Range TLV's sub-TLV encoding mechanisms are different for OSPF and ISIS and so described in sub-sections 2.3.4.1 and 2.3.4.2. 

(7)
2.1.5 The SRMS Preference TLV only contains the Preference value, so it was unclear how this is linked to a mapping server? Is the mapping server some other field in the message?
[KT] The SRMS Preference TLV indicates the preference value for the node NLRI that it is a part of when that node is a mapping server. One would need to go through the referenced IGPs specs and likely the relevant SR specs to understand the mapping server.

(8)
2.3.1
If the flags field is to be used according to the flags from the corresponding source protocol, does the receiver know the source protocol and therefore how to interpret this field? 2.3.2 says to look in NLRI for Protocol-ID so may be worth stating the same here.
[KT] There is similar text also in 2.3.1 but perhaps not as clear. I will update 2.3.1 to have similar text as 2.3.2.

(9)
2.3.4.1 repeats that the flags of the "Range TLV" are set according to the definition in the OSPF drafts. Is that the Range TLV defined here (in which case this is already stated in 2.3.4), or the OSPF Extended Prefix Range TLV (in which case, why would you need to clarify how those flags are used)?
Does this paragraph mean you copy the TLV used in OSPF and also add a Prefix-SID? So for each mapping there are 2 TLVs? 2.3.4.2 is much clearer in the way it presents this.
[KT] I will move the diagram in 2.3.4.2 into the parent section 2.3.4.1 since it applies to both protocols. The sub-sections 2.3.4.1 and 2.3.4.2 are to explain the mappings of TLVs/sub-TLVs from the respective IGPs to the BGP-LS TLVs. I will update this part to make it clearer.
Again a note about Protocol-ID may help here since the sub-TLVs included are different depending on the protocol.
[KT] Ack

(10)
Table 5,6,7. Is the fact the length is variable is interesting in this table? 
[KT] Fixed - not all are variable there was a fixed size TLV that was missing from these tables.

I would like to see this table earlier in the draft to visualise the OSPF and IS-IS information you are aiming to share using this BGP extension
[KT] As mentioned previously, there is a forward reference to this table.