Subject: [sfc] Detailed review of draft-ietf-sfc-nsh-12.txt

From: "Adrian Farrel" <[email-masked]>
Date: Mon, 27 Feb 2017 21:59:57 -0000

Hi all,

Many thanks to Paul for the updated revision. This makes life a lot
easier.

Herewith a review. I suspect some points I've raised are tracked in
email threads or tickets, or in the minutes from the interim. That's
OK: I just read and scribbled in a continuous stream of consciousness.

A bit of a wood/trees scenario here and I can't promise I have caught
every concern on this pass.

Thanks to all for the work that has gone before and which makes this
detailed review possible.

Cheers,
Adrian

---

Requirements Language should not show as a numbered section.
If you're using XML use <note> within <front> rather than <section>.

---

NSH needs to be expanded on first use in the body of the document (in
the Introduction).

---

Throughout the document you need to look at the use of "NSH". It is
often missing an article (sometimes definite, sometimes indefinite).

---

In section 2

   NSH defines a new service plane protocol specifically for the
   creation of dynamic service chains and is composed of the following
   elements:

   1.  Service Function Path identification

   2.  Transport independent service function chain

   3.  Per-packet network and service metadata or optional variable
       type-length-value (TLV) metadata.

Not sure about the middle of these 3. Isn't that a feature (which is
fine to talk about somewhere) not one of the components? I think the
component missing from the list is the SI. So you might write:

   2.  Indication of the progress along the Service Function Path.

And I have some trouble parsing the third item. I think you mean 
network and service metadata that is per-packet (although that is
perhaps a given as the NSH is also per packet). And I think you mean
that the metadata is optional, fixed length, or TLV-based. How about:

   3.  Network and service metadata that may be optionally included
       per packet and that may be fixed length or constructed from
       type-length-value objects (TLVs).

---

Why do you point to 7665 for the definition of "Classifier" but then
define "Service Classifier" to mean (AFAICS) exactly the same thing?

---

In 2.3 I think point 4 is a use case of metadata and so should be part
of point 3.

---

3.1
s/draft/document/

---

Wondering whether "Context Headers" should be "Context Header"
throughout.

---

3.1

   An NSH is composed of a 4-byte (all references to bytes in this draft
   refer to 8-bit bytes, or octets) Base Header, a 4-byte Service Path
   Header and Context Headers, as shown in Figure 1 below.

s/and Context/and optional Context/

---

3.1
   Service Path Header: provide path identification and location within

s/provide/provides/

---

3.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
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |Ver|O|C|R|R|R|R|R|R|   Length  |    MD Type    | Next Protocol |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Obviously, we have discussed the reformatting of the Base Header and
ended up with 

      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
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |Ver|O|     TTL     |   Length  |R|R|R|R|MD Type| Next Protocol |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Need to:
- Add a description of the TTL field (lots of mailing list discussion)
- Delete the C-bit
- Update the descriptions of Reserved bits
- Update the description of MD Type

---

3.2

   Version: The version field is used to ensure backward compatibility
   going forward with future NSH updates.  It MUST be set to 0x0 by the
   sender, in this first revision of NSH.  Given the widespread
   implementation of existing hardware that uses the first nibble after
   an MPLS label stack for ECMP decision processing, this document
   reserves version 01 and this value MUST NOT be used in future
   versions of the protocol.  Please see [RFC7325] for further
   discussion of MPLS-related forwarding requirements.

I believe this text is presuming the NSH-in-MPLS encapsulation. I don't
think it should do that, but if it does we have to look at the first
nibble not just the first two bits.

Now, as 4385 points out, if the first nibble is 4 or 6, IPv4 or IPv6
will be assumed. So 0b0100 or 0b0110 accounts for you reserving version
1 (just in case the C bit is 0 and for any value of the O bit). So far,
so good!

But 4385 defines a first nibble of 0000 to mean "here is a control word"
and that would mean you needed to reserve version 0 as well :-(

So my advice here would be that encapsulation in MPLS is not in scope 
for this document, and you should not play with reserving version
numbers.

---

3.2

There's been a long thread on the text about the O-bit.

   O bit: Setting this bit indicates an Operations, Administration, and
   Maintenance (OAM) packet.  The actual packet format and processing of
   SFC OAM messages is outside the scope of this specification (see [I-
   D.ietf-sfc-oam-framework]).

First para OK.

   SF/SFF/SFC Proxy/Classifer implementations, which do not support SFC
   OAM procedures, SHALL discard packets with O-bit set.

   SF/SFF/SFC Proxy/Classifer implementations MAY support a configurable
   parameter to enable forwarding received SFC OAM packets unmodified to
   the next element in the chain.  Such behavior may be acceptable for a
   subset of OAM functions, but can result in unexpected outcomes for
   others, thus it is recommended to analyze the impact of forwarding an
   OAM packet for all OAM functions prior to enabling this behavior.
   The configurable parameter MUST be disabled by default.

These paras are controversial as discarding OAM packets (rather than
passing them through) can also result in unexpected outcomes. Consider,
if you will, simple end-to-end ping functions.

(Also s/Classifer/Classifier/ twice)

There is also a mismatch of "SHALL" and "MAY".

I would like this text to read...

   ===
   SF/SFF/SFC Proxy/Classifer implementations that do not support SFC
   OAM procedures, SHALL ignore the setting of the O-bit and SHALL
   attempt to process the packet as normal.
   ===

...but I could live with...

   ===    
   SF/SFF/SFC Proxy/Classifer implementations that do not support SFC
   OAM procedures SHOULD discard packets with O-bit set, but MAY
   support a configurable parameter to enable forwarding received SFC
   OAM packets unmodified to the next element in the chain.
   ===

   For non OAM packets, the O-bit MUST be cleared and MUST NOT be
   modified along the SFP.

This paragraph needs simple clarification as follows

   === 
   The O-bit MUST be set for OAM packets and MUST NOT be set for non-OAM
   packets. The O-bit MUST NOT be modified along the SFP.
   ===

---

3.2

C-bit is being retired as discussed.

---

3.2

   Length: total length, in 4-byte words, of NSH including the Base
   Header, the Service Path Header and the context headers or optional
   variable length metadata.  The Length MUST be of value 0x6 for MD
   Type equal to 0x1 and MUST be of value 0x2 or greater for MD Type
   equal to 0x2.  The NSH header length MUST be an integer number of 4
   bytes.  The length field indicates the "end" of NSH and where the
   original packet/frame begins.

There is no "or optional variable length metadata." That is exactly what
the context headers are, and the context headers are optional. And the
last sentence is a little too much. So...

   Length: total length, in 4-byte words, of the NSH including the Base
   Header, the Service Path Header, and any Context Headers.  It the 
   MD Type is 0x1, the Length MUST be 0x6.  If the MD Type is 0x2, the
   Length MUST be 0x2 or greater.  The actual NSH header length MUST be
   an integer multiple of 4 bytes and is padded if necessary. 

---

3.2

   MD Type: indicates the format of NSH beyond the mandatory Base Header
   and the Service Path Header.  MD Type defines the format of the
   metadata being carried.  Please see IANA Considerations section
   below.

Maybe "...indicates the format of the metadata carried in the Context 
Headers..."

   NSH defines two MD types:

s/NSH/This document/
s/types/Type values/

   0x1 - which indicates that the format of the header includes fixed
   length context headers (see Figure 4 below).

   0x2 - which does not mandate any headers beyond the Base Header and
   Service Path Header, but may contain optional variable length context
   information.

Add "...not defined in this specification."

---

3.2

   Next Protocol: indicates the protocol type of the encapsulated data.
   NSH does not alter the inner payload, and the semantics on the inner
   protocol remain unchanged due to NSH service function chaining.
   Please see IANA Considerations section below.

s/due to/by/

   This draft defines the following Next Protocol values:

s/draft/document/

   0x1 : IPv4
   0x2 : IPv6
   0x3 : Ethernet
   0x4: NSH
   0x5: MPLS

Fix formatting

   0x6-0xFD: Unassigned
   0xFE-0xFF: Experimental

Suggest to delete the last two lines as they are not protocols, but info
for the IANA Considerations section.

---

3.3

   Service Path Identifier (SPI): identifies a service path.
   Participating nodes MUST use this identifier for Service Function
   Path selection.  The initial classifier MUST set the appropriate SPI
   for a given classification result.

Both uses of "MUST" are unnecessary and the words could be deleted 
without loss of meaning or tightness of specification.

---

3.3

   Service Index (SI): provides location within the SFP.  The initial
   classifier for a given SFP SHOULD set the SI to 255, 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).
   Service Index MUST be decremented by Service Functions or by SFC
   Proxy nodes after performing required services and the new
   decremented SI value MUST be used in the egress NSH packet.  The
   initial Classifier MUST send the packet to the first SFF in the
   identified SFP for forwarding along an SFP.  If re-classification
   occurs, and that re-classification results in a new SPI, the
   (re)classifier is, in effect, the initial classifier for the
   resultant SPI.

What you are saying, I think is that the initial classifier MUST set
the SI equal to the first SI in the SFP.  This SHOULD be 255, but an
SFP MAY be configured to start at a lower SI value.

s/decremented/decremented by one/ (reference the endless email threads)

---

3.3

   SI SHOULD be used in conjunction with Service Path Identifier for
   Service Function Path Selection and for determining the next SFF/SF
   in the path.
   
What is the variation to this "SHOULD"?

   In addition to indicating
   the location within a Service Function Path, SI can be used for
   service plane loop detection.

I think that, although this might still be true, the TTL will be more
helpful for loop detection and we should strike this text.

---

3.4
I suggest deleting Figure 4. It does not differ from the information in 
Figures 1, 2, and 3, and risks not being kept up-to-date with those
figures.

OTOH, you should say here that Length has a specific value.

---

3.4

   When the Base Header specifies MD Type = 0x1, four Context Headers,
   4-byte each, MUST be added immediately following the Service Path
   Header, as per Figure 4.  Context Headers that carry no metadata MUST
   be set to zero.

I think this is adding new and strange meaning to "Context Header".
See previous comments, but why is a Context Header suddenly a 4-byte
thing? Why is this not 3 3-byte Context Headers followed by 1 7-byte
Context Header?  Or more realistically, why not just talk about "Up to
16 bytes of metadata carried in the 16 byte Context Header that follows
the Base Header. Any context Header bytes not used to carry metadata
MUST be set to zero."

---

3.4

OLD
   This specification does not make any assumption about the content
   placed in the mandatory context field of the NSH header, and does not
   describe the structure or meaning of the included metadata.
NEW
   This specification does not make any assumptions about the content of
   the 16 byte Context Header that must be present when the MD Type 
   field is set to 1, and does not describe the structure or meaning of
   the included metadata.
END

---

3.4

   Upon receiving an NSH MD-type 1 packet, if the SFC-aware SF is
   configured for mandatory use of metadata but does not yet receive the
   data semantics for the mandatory context field, it MUST NOT process
   the packet and MUST log at least once per the SPI for which a
   mandatory metadata is missing.

There seem to be some assumptions here:
- An SF having mandatory use of metadata is a config item and can't be
  an implementation thing (i.e., you are not allowing an SF that must
  always use metadata).
- You are assuming that the semantics for the metadata are always 
  dynamically installed in the SF and not known a priori.

I don't think you should make those restrictions (although, obviously,
we should allow them as options).

You also seem to be assuming that a non-SFC-aware SF can receive a 
packet with an NSH and metadata and will somehow manage to parse the NSH
and find the metadata that it doesn't understand. Frankly, I think this
document talks only about SFC-aware SFs and SFC proxies. Other SFs are
by definition not in scope as they will never receive an NSH (except for
a bug that will reasonably cause the SF to drop the packet - or crash).

Finally "must not process" is ambiguous. I think you mean "must discard"

So, maybe...

   An SF or SFC Proxy that does not know the format or semantics of the 
   Context Header for an NSH with MD Type 1 MUST discard any packet with
   such an NSH (i.e., MUST NOT ignore the metadata that it cannot 
   process), and MUST log the event at least once per the SPI for which
   the event occurs (subject to thresholding).

---

3.5

Again, Figure 5 is de trop. 
You also need to discuss the value of the Length field.

---

3.5

This section has the usual confusion of whether there are multiple
context headers (each of variable length), multiple variable length
metadata, a single variable length context header (also called a 
variable context header) containing metadata, or whatever.

Figure 6, for example, shows one piece of variable length metadata
in an encoding element labelled "Variable Context Headers".

This is made more confusing in the context of 3.4 where you have 
the concept of 4 byte context headers.

Later, in 3.5.1, you call it a TLV.

I wish we could sort out this language.

I think what you are trying to say is that the Context Header (singular)
can contain zero, one, or multiple metadata elements each encoded as a
TLV as shown in Figure 6.

---

3.5.1

Discussions on the list about the sizing of the Metadata Class and 
Type fields, the deprecation of the C-bit, the naming of the Type field
(which looks like Metadata Type which is easily confused with MD Type),
and the deprecation of the split ranges of the Type field.

---

3.5.1

   If multiple instances of the same TLV are included in an NSH packet,
   but the definition of that TLV does not allow for it, the SFC-aware
   SF MUST NOT process the packet and MUST log at least once per the SPI
   for which multiple instances of that TLV is supplied.

This is OK, but I prefer the more usual (and future-proof)...

   If multiple instances of the same TLV are included in an NSH packet,
   but the definition of that TLV does not allow for it, the SFC-aware
   SF MUST process first instance and ignore subsequent instances.

---

4.

   NSH-aware nodes are the only nodes that MAY alter the content of the
   NSH headers.

s/MAY/may/

Is there a difference between an "SFC-aware node" and an "NSH-aware
node"?

Isn't this sentence silly? "Only nodes that can understand the content
of the NSH can change the content of the NSH"

---

4.
Bullet 1

OLD
       At the end of a service function path, a SFF, MUST be
       the last node operating on the service header and MUST remove it.
NEW
       At the end of a service function path, a SFF, MUST be
       the last node operating on the service header and MUST remove it
       before forwarding or delivering the encapsulated packet.
END

---

4.
Bullet 1

       Multiple logical classifiers may exist within a given service
       path.

How so "logical"? Are they or are they not "classifiers"?
(Multiple occurrences in this bullet.)

---

4.
Bullet 1

OLD
       Non-initial classifiers may re-classify data and that re-
       classification MAY result in a new Service Function Path.
NEW
       Non-initial classifiers may re-classify data and that re-
       classification MAY result in a the packet being assigned to a
       different Service Function Path.
END

---

4.

   2.  Select service path: The Service Path Header provides service
       chain information and is used by SFFs to determine correct
       service path selection.  SFFs MUST use the Service Path Header
       for selecting the next SF or SFF in the service path.

It is probably a minor point, but the Service Path Header provides
service *path* information and not service chain information.

---

4.

   3.  Update NSH: NSH-aware service functions (SF) MUST decrement the
       service index.  If an SFF receives a packet with an SPI and SI
       that do not correspond to a valid next hop in a valid Service
       Function Path, that packet MUST be dropped by the SFF.

Non-NSH-aware SFs will, of course, either not see NSHs or barf. So it is
enough to say "SFs" (you have already defined the abbreviation).

Say "decrement the service index by one".

---

4.
Bullet 3

       Classifier(s) MAY update Context Headers if new/updated context
       is available.

s/Classifier(s)/Classifiers/

---

4.
Bullet 3

       If an SFC proxy is in use (acting on behalf of a non-NSH-aware
       service function for NSH actions), then the proxy MUST update
       Service Index and MAY update contexts.  When an SFC proxy
       receives an NSH-encapsulated packet, it MUST remove the NSH
       headers before forwarding it to an NSH unaware SF.  When the SFC
       Proxy receives a packet back from an NSH unaware SF, it MUST re-
       encapsulates it with the correct NSH, and MUST decrement the
       Service Index.

I think "update contexts" means "update the metadata carried in the
Context Header. This does not say whether a proxy can add a Context
Header if one is not already present, or whether it can add a metadata
TLV, or remove one.

Decide on "non-NSH-aware", "NSH unaware", "non-SFC-aware".

Say "decrement the service index by one".

---

4.

   4.  Service policy selection: Service Function instances derive
       policy (i.e. service actions such as permit or deny) selection
       and enforcement from the service header.  Metadata shared in the
       service header can provide a range of service-relevant
       information such as traffic classification.  Service functions
       SHOULD use NSH to select local service policy.

"Service Function instance" is not properly defined in 7665 and not
previously mentioned in this document. OTOH, the term could usefully be
used in 7.1 para 2.

s/service header/NSH/ (twice)

Does the last sentence add anything? It appears to say what the previous
sentences say, but the "SHOULD" is ambiguous. Suggest deleting it.

---

In Figure 8

- "Select Service Function Path" is ambiguous. Isn't the SFF bound by 
  the SPI and so cannot select the path, only use the one it is
  instructed to use? OTOH, the Classifier is responsible for determining
  which path to use.

- Surely the SFC proxy can update the Context Header as in bullet 3,
  above.

- Might an SFC proxy also select service policy for an NSH-unaware SF?

---

5.

   The presence of NSH is
   indicated via protocol type or other indicator in the outer
   encapsulation.

You are prejudging the "encapsulation of NSH" work that is to be done.
You may be largely correct, but not always. We should omit this text.

---

7.1

   This indirection -- path ID to overlay -- creates a true service
   plane.  That is the SFF/SF topology is constructed without impacting
   the network topology but more importantly service plane only
   participants (i.e. most SFs) need not be part of the network overlay
   topology and its associated infrastructure (e.g. control plane,
   routing tables, etc.).  As mentioned above, an existing overlay
   topology may be used provided it offers the requisite connectivity.

There is a lot hidden or implied in this paragraph. But consider an SF
that is somehow remote from the SFF. The SFF knows how to send to the
SF over the underlay network. But does the SF know how to send back to
the SFF? In some underlay technologies you can just reverse the 
encapsulation addresses. In others, you can't.

Perhaps the relationship between SF and SFF is configured (i.e., known
a priori)?

Now consider an SF that is used on multiple SFPs. And consider further
that the SF might be reachable from different SFFs on the different 
SFPs. Now selecting the return path becomes a challenge.

Maybe "most SFs" is the clue. Maybe "some SFs" participate in the 
control plane and routing tables. 

Perhaps this could be clearer.

---

8.1

   NSH
   itself does not provide privacy functions, rather it relies on the
   transport/overlay layer.  An operator can select the appropriate
   transport to ensure the confidentially (and other security)
   considerations are met.

I think you are making a big leap to decide what the metadata documents
will define.

You are also failing to consider metadata that needs to be kept private
between the SF at SI 250 and the SF at SI 240 when there are other SFs
in between.

I suggest replacing this paragraph with something like...

   This specification does not provide any privacy or security functions 
   for the NSH. Hop-by-hop security and privacy can be achieved by using
   features of the underlay (transport) connections. Metadata privacy 
   and security considerations are a matter for the documents that 
   define metadata formats, encodings, and TLVs.

---

8.2

Why do you not consider imposing metadata at a subsequent point along
the path? This would happen, for example, if one SF needed to convey
information to another SF for a packet that did not already have 
metadata.

---

12

Obviously the IANA stuff needs updating to reflect changes elsewhere:

12.2.1 The C bit has gone and the reserved bits have moved.

12.2.2 See my comment on section 3.2

12.2.3 The size of the MD Type registry is reduced.
       Allowing one experimental code point looks like enough to me.

12.2.4 There is a thread about this.
       The registry has to indicate that MD Class 0 has been assigned
       by this document for "IETF use".
       The advice to DEs needs to be supplied.
       The size of the range of classes may be reduced to allow an 
       increase in the number of "types"

Missing registry for "types" within the MD Class 0
       The same thread applies.
       (I recall there was also action from the interim)
       The registry will be empty but should be created in this doc
       The name of this registry is confusable with "MD type"
       The size of the range may be increases as the MD class range is
       reduced.

---

From: <[email-masked]>
Date: Wed, 1 Mar 2017 06:55:50 +0000

Hi Adrian, all,

Please see some comments inline. 

Cheers,
Med

> -----Message d'origine-----
> De : sfc [mailto:[email-masked]] De la part de Adrian Farrel
> Envoyé : lundi 27 février 2017 23:00
> À : [email-masked]
> Objet : [sfc] Detailed review of draft-ietf-sfc-nsh-12.txt
> 
> Hi all,
> 
[SNIP]
>    attempt to process the packet as normal.
[Med] Ignoring the O-bit while processing the packet "as normal" for a trace message, for example, will result in a broken SFC trace service (given that some elements won't be able to insert their information in the record list).    

>    OAM packets unmodified to the next element in the chain.

[Med] OK with this one. 

>    packets. The O-bit MUST NOT be modified along the SFP.
[Med] Agree. 

>    length context headers (see Figure 4 below).

[Med] s/length context headers/length context header

There is only one fixed length context header (MD#1).

> Add "...not defined in this specification."
[SNIP]
> I think this is adding new and strange meaning to "Context Header".

[Med] As already reported to the authors, this text was forgotten when implementing the change agreed in https://trac.ietf.org/trac/sfc/ticket/22. There isn't anymore 4 context headers. 

>    field is set to 1, and does not describe the structure or meaning of

[Med] I'm OK with your proposal. "s/mandatory context field/fixed length context field" would be OK, too.

>   dynamically installed in the SF and not known a priori.

[Med] How an SF is "configured" and how it does "receive the data semantic" are not detailed in the text. This can be manual configuration, hardcoded parameters, dynamic provisioning, etc. 

> document talks only about SFC-aware SFs and SFC proxies.

[Med] There is a need to distinguish SFs that are SFC-aware and those that aren't (typically, those serviced by an SF proxy). 

 Other SFs are
>    the event occurs (subject to thresholding).

[Med] IMHO, this is too restrictive as failures will be experienced each time a path involved an SF, not aware of the data semantic, even if that SF is not supposed to consume/supply a context data for a given chain. I'd like failures to be declared only by SFs who require the mandatory context data for executing their service.

---
[SNIP]

>    SF MUST process first instance and ignore subsequent instances.

[Med] The OLD wording is consistent with the specification of a TLV indicating explicitly that multiple instances MUST NOT be allowed. 
Further, picking the first instance may be problematic. Consider for example, a TLV that carries a subscriber identifier + the definition of this TLV indicates that only one instance is allowed per chain. Consider now that for some reason, two subscriber-identifier TLV instances are present in a packet, because there are no requirements about the ordering of the TLVs (including order preservation when processing and forwarding NSH packets), picking the first instance may lead to applying per-subscriber policies that may not those of the correct one. The consequence may be for example access to privacy data, access to a service while that user is not allowed, and so no. 

I prefer the text as it is in -12.

The logs can trigger alarms to fix problems in configuring an SFC domain.  

[snip]