Re: [sfc] #24 (nsh): -07 review (M. Boucadair)

["sfc issue tracker" <[e-mail]>]

#24: -07 review (M. Boucadair)


Comment (by [e-mail]):

 Please review the email responses sent by Paul Quinn on 9/10/2016 that
 seem to address many of the below comments.



 Replying to [comment:1 mohamed.boucadair@…]:
 > Updating the tracker to record the pending points:
 >
 > ==
 > (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."
 >
 >  (2) Section 2.3: CLOSED
 >
 >  (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.
 >
 >  (4) Section 2.3: CLOSED
 >
 >  (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.
 >
 >  (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?
 >
 >  (7) Section 3.3: CLOSED
 >
 >  (8) Section 3.4:
 >
 >  8.2. The specification does not forbid that all context headers are set
 to
 >  zero. Otherwise, MD#2 should be used instead.
 >  8.3. The specification does not specify how these context headers are
 to
 >  be validated.
 >  8.4. I still don't understand why four headers are chosen.
 >
 >  (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.
 >
 >  (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       |   +    |   +    |       |   +    |   +   |
 |
 >
 +----------------+--------+--------+-------+--------+-------+---------+
 >
 >  (11) Section 7.2: CLOSED.
 >
 >

-- 
-------------------------------------+-------------------------------------
 Reporter:                           |       Owner:  draft-ietf-sfc-
  [e-mail]       |  [e-mail]
     Type:  defect                   |      Status:  new
 Priority:  major                    |   Milestone:
Component:  nsh                      |     Version:
 Severity:  -                        |  Resolution:
 Keywords:                           |
-------------------------------------+-------------------------------------

Ticket URL: <https://trac.tools.ietf.org/wg/sfc/trac/ticket/24#comment:2>
sfc <https://tools.ietf.org/sfc/>