[bess] RtgDir review: draft-ietf-bess-nsh-bgp-control-plane-13

[Ravi Singh <[e-mail]>]

Hello,

I have been selected as the Routing Directorate reviewer for this draft. The Routing Directorate seeks to review all routing or routing-related drafts as they pass through IETF last call and IESG review, and sometimes on special request. The purpose of the review is to provide assistance to the Routing ADs. For more information about the Routing Directorate, please see ​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir <http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir>
Although these comments are primarily for the use of the Routing ADs, it would be helpful if you could consider them along with any other IETF Last Call comments that you receive, and strive to resolve them through discussion or by updating the draft.

Document: draft-ietf-bess-nsh-bgp-control-plane-13.txt
Reviewer: Ravi Singh
Review Date: 01-26-2020
Intended Status: Standards Track

Summary:
I have some minor concerns about this document that I think should be resolved before publication.


Comments: 
0.  Having finished reading through the entire doc, the whole picture comes together clearly. 
However, given the size of this document…..as one is reading this document it takes a high level of effort to stay with the picture.
I feel the readability could be somewhat improved.  See #1 under “minor issues” for a suggestion.

Major Issues:
No major issues found.

Minor issues:

1.
Section2: During an initial reading, the terminology comes across as overly repetitive and a bit pedantic….which takes away from the readability a bit when one is reading the sections in the order listed. Content of RFC7665/8300 are also contributors to this.
Eg: "In fact, each SI is mapped to one or more SFs that are implemented by one or more Service Function Instances (SFIs) that support those specified SFs. "  : almost sounds like legalese when someone who has not grasped the complete picture of the draft.
 
Wrote up the above as I was reading that section for the first time.
However, after finishing the review…started to appreciate this.
 
In having read the sections in order, I think the readability would have been greatly enhanced if I had read section 8 first, else once just gets lost in all the details.
It would be worthwhile suggesting in the text that once the reader has read through section 2, it would help to read section 8 before reviewing the intervening sections.
 
2. Section 3.2.1: page 16:
"[RFC7606]
   revises BGP error handling specifically for the for UPDATE message,"
 
->
"[RFC7606]
   revises BGP error handling specifically for the UPDATE message,”

3. 
Page 17:
  a. Was the intention for treating error 4 any different from errors, [1,2,6,7]? If not, why the need to call out 4 separately?
  b. "Unknown SFIR-RD found in a Hop TLV." :
The format of the Hop TLV in section 3.2.1.2 contains no reference to an RD. So, was the intention instead to refer to the SFIR-RD list of one of the SFT TLVs inside the Hop TLV?

4.
Section 3.2.1.2: "At least one sub-TLV MUST be present. "
Where are these sub-TLVs defined?
In this regard,
  a. Section 3.2.1.3 says "The SFT TLV MAY be included in the list of sub-TLVs of the Hop TLV.":
  b. Section 3.2.1.4 says "The MPLS Swapping/Stacking TLV (Type value 4) is a zero length sub-TLV that is OPTIONAL in the Hop TLV "

In the absence of specific mention of details of sub-TLVs in section 3.2.1.2, is a reader to assume that SFT TLVs & the MPLS swap/stack TVLs are the only possible subTLVs under a hop-TLV (as intended in this revision of the ID)?
This aspect becomes clear later, when one gets to reading the description of the subsequently mentioned TLVs. Might be worth alluding to this in 3.2.1.2.

5.
"In the normal case the SPI remains unchanged and the SI will have been decremented to indicate the next SF along the path.": will SI really be decremented instead of just setting it to the appropriate value? As per this draft, set to an appropriate lower value…

6.
What exactly does the following mean? "Also, as described in [RFC8300], an SFF receiving an SI that is unknown in the context of the SPI can reduce the value to the next meaningful SI value in the SFP indicated by the SPI. If no such value exists or if the SFF does not support this function, the SFF drops the packet and should log the event: such logs are also subject to rate limits."

7. 
Figure 1: showing the SFIs hosted on SFF2 and SFF3 in the same conceptual block (just because they share the same type) is a bit confusing when the diagram is showing a logical view of the physical layout of the elements of the solution.

8.
Section 3.1:the following is an ambiguous sentence & I suggest rewording to clarify:
"Note that it is assumed that each SFF has one or more globally unique SFC Context Labels and that the context label space and the SPI address space are disjoint." 
This sounds like that the "context label space" and the "SPI address space" are disjoint w.r.t. each other. What appears to be intended is that the SFC context labels and SPI-address-space should be disjoint across the different SFFs.
However, is it not sufficient to just have the SFC-context label spaces be disjoint across SFFs?

9.
Section 3.2: Why is "If two SFPRs are originated from different Controllers they MUST have different RDs" needed when "All SFPs MUST be associated with different RDs." is already stated?

10.
Section 3.2.1: "The Extended Length bit is set according to the length of the SFP attribute as defined in [RFC4271].": minor quibble: but this sentence needs rewording for correctness of intended meaning.

11.
Pg26: Typo in "This makes the bevahior "

12.
Section 5: pg 29: suggest rewording "Thus, at any point in time when an SFF selects its next hop, it chooses from the intersection of the set of next hop RDs contained in the SFPR and the RDs contained in its local set of SFIRs." to
"Thus, at any point in time when an SFF selects its next hop, it chooses from the intersection of the set of next hop RDs contained in the SFPR and the RDs contained in the SFPR's local set of SFIRs."

13.
Section 5 pg 29: Not clear "Similarly, when this condition obtains " what is intended here?

14.
Section 6: typo in "If the SPI indicates anther path,"

15.
Section 6.1: SI (As defined in SFIR format in section 3.1) is a 1 octet quantity. So, want to make the SI field 1 byte long and the reserved field 4 bytes long?

16.
Section 6.1: "Note that Special Purpose SFTs MUST NOT be advertised in SFIRs.": what is the intended behavior if these were so advertised?

17.
Section 7.4: "that can be used to disposition those packets" and "it MUST NOT be used for dispositioning the packets of the specified" feels a little strange: perhaps the RFC-editor will have more to say about this, if this really is strange

18.
Section 7.6: "with that SFP’ last hop." -> "with that SFP’s last hop."

19.
Section 8.4: "path selecting between all SFF2 that support an SF of type 43 and SFF3 that supports" could use some rewording.

20.
Section 8.7: "[SI = 245, SFT = 42, RD = 192.0.2.3:7]" could be made more in line with the encoding, by showing it as [SFT = 42, RD = 192.0.2.3:7] with the SI=245 being nested under [SI=245, …] on the lines of as is done in section 8.4.

21.

Section 8.9.2: what is gained by having a given SFI be identified using multiple different SI s?
Eg:
[SI = 255, SFT = 41, RD = 192.0.2.1:11],
And
[SI = 253, SFT = 41, RD = 192.0.2.1:11]
 
The above 2 representations are for the same SFI: i.e. SFT & RD are the same. So, why represent them using different SI s?
This ties to the question about: why worry about the numeric ordering of the SI values in a given SFP?

Regards
Ravi