Last Call Review of draft-ietf-bess-nsh-bgp-control-plane-13
review-ietf-bess-nsh-bgp-control-plane-13-rtgdir-lc-singh-2020-01-29-00

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