Re: [sfc] WG review for draft-ietf-sfc-nsh-07.txt

["Paul Quinn (paulq)" <[e-mail]>]

Med,

Med, as always, thank you for your review.


> On Aug 26, 2016, at 5:31 AM, [e-mail] wrote:
> 
> Hi Jim, all,
> 
> Likewise, I would like to see this document progress as well. 
> 
> In addition to the comment raised in a separate message about the C-bit of TLVs, I have the following comments:
> 
> (1) Section 2: I suggest to delete the following sentence: 
> 
>   NSH is designed to be easy to implement across a range of devices,
>   both physical and virtual, including hardware platforms.
> 
> Reason: the use of "easy to implement" should be justified if that text is maintained. Otherwise, this is subjective. 
> 
> or to reword it as follows: 
> 
>   "NSH can be implemented in both physical and virtual platforms."

PQ>  The "easy to implement" is a reference to the nature of type-1 and how, as noted by hardware implementors, this vastly simplifies implementation.  Further, the language is widely used in RFCs (e.g. 7321, 3160, etc.).

> 
> (2) Section 2.3: I suggest to change this OLD text:
> 
> The semantics of
>       the shared metadata is communicated via a control plane, which is
>       outside the scope of this document, to participating nodes.
> 
> NEW:
> 
> The semantics of 
> the shared metadata is communicated via a control plane (Section 3.3 of [I-D.ietf-sfc-control-plane]).
> 

PQ>  Proposed text: "The semantics of the shared metadata is communicated via a control plane, which is outside the scope of this document, to participating nodes.  [ietf-sfc-control-plane] provides an example of such in section 3.3.


> (3) Section 2.3- Not sure I would maintain this text: 
> 
>   5.  NSH offers a common and standards-based header for service
>       chaining to all network and service nodes.
> 
> "to all" is not accurate as "legacy" nodes are still there.  
> 

PQ>  Given that "legacy" nodes are supported via proxy, this is indeed true.


> (4) Section 2.3:
> 
> OLD:
> Transport Agnostic: NSH is transport independent and is often
>       carried via a network transport protocol,
> 
> NEW:
> Transport Agnostic: NSH is transport independent. Any network transport protocol can be used to transport NSH-encapsulated traffic.


PQ>  Proposed text: NSH is transport independent. An appropriate (for a given deployment) network transport protocol can be used to transport NSH-encapsulated traffic.


> 
> (5) Section 3.2:
> 
>   NSH implementations MUST support MD-Type = 0x1, and SHOULD support
>   MD- Type = 0x2.
> 
> Because:
> * of potential interoperability issues.
> * MD#2 is more compact when no metadata is to be supplied
> * MD#2 allows to convey more information compared to MD#1
> 
> I suggest we revisit that sentence to have MD#2 be mandatory to be supported (my favorite option), or at least require that both MDs defined in this spec MUST be supported. 
> 
PQ>  As per the previous discussion, this MUST/SHOULD reflect the practical reality of implementation and helps ensure deployability.  



> (6) Section 3.2:
> 
>   C bit: Indicates that a critical metadata TLV is present.  This bit
>   acts as an indication for hardware implementers to decide how to
>   handle the presence of a critical TLV without necessarily needing to
>   parse all TLVs present.  For an MD Type of 0x1 (i.e. no variable
>   length metadata is present), the C bit MUST be set to 0x0.
> 
> 6.1. What is the behavior of the implementation if C-bit is set but not critical metadata is found in the payload?
> 6.2. What is the behavior of the implementation if C-bit is unset but a critical metadata is found in the payload?
> 6.3. What is the behavior of the implementation with regards to this bit if a critical metadata is added in-path?
> 6.4. What is the behavior of the implementation with regards to this bit if a critical metadata is stripped in-path?   
> 6.5. Should the specification recommend an order of metadata so that critical metadata are always positioned first?

PQ>  

6.1: essentially continue "as is" with an optional warning message
6.2: generate an warning error, possibly set C
6.3: the adder sets C
6.4: the remover clears C
6.5: no, this creates too many limitations/complexity.  An implementation might opt to optimize.  


> 
> (7) Section 3.3: The following text 
> 
> " ... however the
>   control plane MAY configure the initial value of SI as appropriate
>   (i.e. taking into account the length of the service function path). "
> 
> is not aligned with the outcome of the discussion we had during the WG LC of draft-ietf-sfc-control-plane (https://www.ietf.org/mail-archive/web/sfc/current/msg04594.html):
> 
>   The control plane must instruct the classifier about the initial
>   values of the Service Index (SI).
> 

PQ>  The control plane can assign but NSH provides a default value recommendation, I'm not sure there's a conflict.


> 7.1. I suggest to modify NSH draft accordingly. 

PQ>  I'm not clear what you are suggesting?

> 7.2. Please consider adding a reference to draft-ietf-sfc-control-plane

PQ>  I'll update.

> 
> (8) Section 3.4: I still do think this section is underspecified. E.g., 
> 
> 8.1. The use of "Mandatory Context Header" conflicts with this text in Section 3:
> 
>   A Network Service Header (NSH) contains service path information and
>   optionally metadata that are added to a packet or frame and used to
>   ^^^^^^^^^^^^^^^^^^^
>   create a service plane.
> 
> 8.2. The specification does not forbid that all context headers are set to zero. Otherwise, MD#2 should be used instead.


PQ>  In either care, 1 or 2, no metadata is valid: all zeroes in type 1 or no data in type 2.  


> 8.3. The specification does not specify how these context headers are to be validated. 

PQ>  By validated, so you mean cryptographically asserted, or simply compared to known value?  In either case, that is outside of the scope of this draft.


> 8.4. I still don't understand why four headers are chosen.  

PQ>  As per previous discussions, based experience, use case and implementation, four strikes a balance. The widespread implementations of type 1 support this.


> 
> (9) Section 3.5.1:
> 
> The length of "Metadata Class" is over-dimensioned. I suggest to reduce the length of this field and increase the length of "Type".
> 
> For example, let's consider the proposal in https://tools.ietf.org/html/draft-napper-sfc-nsh-broadband-allocation-00#section-4.2: 
> 
>    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>   |     TLV Class = 3GPP          |C|    Type     |R|R|R|   Len   |
>   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>   |    Data ...
>   +-+-+-+-+-+-+-+-+
> 
>                         Figure 5: TLV Allocation
> 
>   The intended use of the header is for TLVs associated with 3GPP Radio
>   Access Networks as described in [TS.29.230].  This TLV can be used by
>   3GPP to extend the metadata as per use cases.  Having this TLV helps
>   to carry more information that does not fit within the MD Type 0x01.
> 
> The list of codes in Table 7.1 of TS.29.230 cannot be conveyed with a "Type" field of 7 bits.

PQ>  At this point, the allocation seems reasonable, provides for a large range of "owners", and the TLV schemes proposed work within this scope.

> 
> (10) Section 4: 
> 
> OLD:
> 
> +---------------+------------------+-------+----------------+---------+
> |                |  Insert         |Select |   Update       |Service  |
> |                |  or remove NSH  |Service|    NSH         |policy   |
> |                |                 |Function|               |selection|
> | Component      +--------+--------+Path   +----------------+         |
> |                |        |        |       | Dec.   |Update |         |
> |                | Insert | Remove |       |Service |Context|         |
> |                |        |        |       | Index  |Header |         |
> +----------------+--------+--------+-------+--------+-------+---------+
> |                |   +    |   +    |       |        |   +   |         |
> |Classifier      |        |        |       |        |       |         |
> +--------------- +--------+--------+-------+--------+-------+---------+
> |Service Function|        |   +    |  +    |        |       |         |
> |Forwarder(SFF)  |        |        |       |        |       |         |
> +--------------- +--------+--------+-------+--------+-------+---------+
> |Service         |        |        |       |   +    |   +   |   +     |
> |Function  (SF)  |        |        |       |        |       |         |
> +--------------- +--------+--------+-------+--------+-------+---------+
> |SFC Proxy       |   +    |   +    |       |   +    |       |         |
> +----------------+--------+--------+-------+--------+-------+---------+
> 
> NEW:
> 
> +---------------+------------------+-------+----------------+---------+
> |                |  Insert         |Select |   Update       |Service  |
> |                |  or remove NSH  |Service|    NSH         |policy   |
> |                |                 |Function|               |selection|
> | Component      +--------+--------+Path   +----------------+         |
> |                |        |        |       | Dec.   |Update |         |
> |                | Insert | Remove |       |Service |Context|         |
> |                |        |        |       | Index  |Header |         |
> +----------------+--------+--------+-------+--------+-------+---------+
> |                |   +    |   +    |       |        |   +   |         |
> |Classifier      |        |        |       |        |       |         |
> +--------------- +--------+--------+-------+--------+-------+---------+
> |Service Function|        |   +    |  +    |        |       |         |
> |Forwarder(SFF)  |        |        |       |        |       |         |
> +--------------- +--------+--------+-------+--------+-------+---------+
> |Service         |        |        |       |   +    |   +   |   +     |
> |Function  (SF)  |        |        |       |        |       |         |
> +--------------- +--------+--------+-------+--------+-------+---------+
> |SFC Proxy       |   +    |   +    |       |   +    |   +   |         |
> +----------------+--------+--------+-------+--------+-------+---------+

PQ>  Just to be clear (formatting was a bit off in the email):

In the last line, SFC Proxy, add "Update Context Header"?

> 
> (11) Section 7.2: Please consider adding an IPv6 example.

PQ>  Happily.