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

[Adrian Farrel <[e-mail]>]

Hi Ravi,

Thanks for a thorough and useful review.

Responses in line…

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

Doing so in -14

> Minor issues:
>
> 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.

We've changed this to...
Within the context of a specific SFP, an SI references a set of one or more SFs.  Each
of those SFs may be supported by one or more Service Function Instances (SFIs).
...although we are slightly worried that we may have lost some precision.

> 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.
>
> [JD]  Fine w/ me 
 
We have put in the forward pointer.

> 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,”

Ack

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

4 should be treated the same as 1, 2, 6, and 7 
Fixed

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

8 should be “Unknown SFIR-RD found in an SFT 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.

3.2.1.2 now reads
"At least one sub-TLV MUST be present. This document defines the SFT Sub-TLV (see Section 3.2.1.3) and the MPLS Swapping/Stacking Sub-TLV (see Section 3.2.1.4): other sub-TLVs may be defined in future."

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

Contemplated changing this to...
 “and the SI set to the value for the next hop in the SFP”.

However, in discussion way back, the SFC WG was pretty adamant about using the term "decremented" and we would like to remain consistent with their usage.

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

s/this function/reducing the SI/

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

We re-read the associated text and thought it was clear. 

The blocking is not "just because they share the same type" but is indicative of them sharing the same type.

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

Within a given VPN, the set of SPIs is unique as is the set of SFC Context Labels.  When an SFF receives a packet it needs to know whether the topmost label is an SPI value or an SFC context label.  Hence the two sets of labels need to be disjoint. 

We have include a little extra clarity as the result of an IESG comment to arrive at...

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 (i.e., a label value cannot be used both to indicate an SFC context and an SPI, and it can be determined from knowledge of the label spaces whether a label indicates an SFC context or an SPI).

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

It may be too subtle, but, there is a difference between SFPR and SFP in the quoted text. Consider two controllers that both want to announce the same SFP.

However, even discarding this subtlety, the worst is that there is an over-statement of the rule.

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

Oh well, maybe. Or maybe not 😊
How about, 
"The Extended Length bit is set if the length of the SFP attribute is encoded in one octet (set to 0) or two octets (set to 1) as described in [RFC4271]."

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

Ack

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

It is the set of SFIRs local to the SFF the we care about. We can make that clearer.

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

You weren't the first person to query our (correct) use of "obtains".
We have s/obtains the originator of the SFPR /applies on the controller that originated the SFPR,/

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

Ack

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

Yes, that was a legacy thing that got missed. Thanks. Fixed.

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

Added  “If such an SFIR is received it SHOULD be ignored.”

> 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

The term ‘disposition’ was introduced in the EVPN RFCs.  It’s used as
a single term to describe a multiplicity of possible actions.

If the RPC barfs we'll conjure a new word.

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

Ack

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

s/2/s/

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

Yes

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

I looked through 8.9.2 carefully. Within any one SFP, I don't see the same SFI identified more than once.
In different SFPs, the change in SI for the same SFI reflects a different ordering.
You need that different ordering for the reverse direction SFPs.

Best,
Adrian