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

[<[e-mail]>]

Hi Paul, 

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Paul Quinn (paulq) [mailto:[e-mail]]
> Envoyé : samedi 10 septembre 2016 19:30
> À : BOUCADAIR Mohamed IMT/OLN
> Cc : Jim Guichard (jguichar); [e-mail]
> Objet : Re: [sfc] WG review for draft-ietf-sfc-nsh-07.txt
> 
> 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.).

[Med] Unless that statement is backed with citations or arguments, I still consider it as subjective.

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

[Med] Works for me. 

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

[Med] Please update the text to mention the proxy case. Thank you.

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

[Med] OK. Thanks. 

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

[Med] the SHOULD for MD#2 does not help interoperability. I reiterate my comment to update the language to use MUST for MD#2.

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

[Med] This should be reflected in the specification.

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

[Med] The default value in the text is a SHOULD not a MUST.   

> 
> 
> > 7.1. I suggest to modify NSH draft accordingly.
> 
> PQ>  I'm not clear what you are suggesting?

[Med] The proposal is to use the same wording proposed by Jim for the CP draft. 

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

[Med] No. My concern is how an implementation will consider that a given mandatory header is conveying what is supposed to include. Some validation checks are required.

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

[Med] I'm not aware of any public use case that requires 4 mandatory context headers. Can you please provide a public link to a document where this is discussed?

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

[Med] "reasonable" compared to what? The example I provided above for the 3GPP case shows clearly that 7-bits are not sufficient for TS.29.230. Further, 2^^16 is over dimensioned for owners.

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

[Med] Yes.

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

[Med] Thank you.