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

Hi SFC WG,

I have some trivial corrections to suggest about the latest draft:

After  page 18, line 25:
	Replace "invalid service function path" with "invalid service function paths"
	Replace "discarded" with "discarded."

After page 24, line 6:
	Replace "the confidentially (and other security) considerations are met" with "confidentiality (and other security) considerations are met"

after  27, line 8:
	Replace "NSH metadata authenticity and confidentially" with "NSH metadata authenticity and confidentiality"

Best regards,
Igor.

-----Original Message-----
From: sfc [mailto:sfc-bounces@ietf.org] On Behalf Of Paul Quinn (paulq)
Sent: Monday, March 20, 2017 5:53 PM
To: Adrian Farrel <adrian@olddog.co.uk>
Cc: sfc@ietf.org
Subject: Re: [sfc] Detailed review of draft-ietf-sfc-nsh-12.txt

Hi Adrian,

> On Feb 27, 2017, at 4:59 PM, Adrian Farrel <adrian@olddog.co.uk> wrote:
> 
> 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.
> 

Again, thank you for the great review!  

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

PQ>  Will update accordingly.  

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

PQ> That's a fair point, so, for #2, perhaps:

2.  Indication of location within a Service Function Path.  

I think the concept of location is more accurate than progress.

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

PQ>  Let's simplify even more: 

3.  Optional, per packet metadata (fixed length or TLV).

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

PQ>  The definition in NSH provides important NSH-centric details that aren't in 7665.  

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

PQ>  That works for me.

> ---
> 
> 3.1
> s/draft/document/
> 
> ---

PQ>  Thanks.

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

PQ>  Given that we've merged the figure into one "blob" this probably makes sense.

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

PQ>  Done.

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

PQ>  AFAIK, the chairs haven't called consensus here.  Once they convey that, we'll update accordingly.

> - Delete the C-bit

PQ>  I just saw consensus around that, so the next version will reflect the change.

> - Update the descriptions of Reserved bits
> - Update the description of MD Type

PQ>  I believe you mean the updates that reflect TTL if/when added?  

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

PQ>  That is where we started, but there was a lot of discussion on the list about the potential for conflict.  I have to go dig up the threads but I'm hoping that some of the folks who expressed concern here will weigh in.

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

PQ>  I can live with this as well, however, I think we need to chairs to make a final call about how the WG has opted to move forward wrt OAM.

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

PQ>  Indeed.

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

PQ>  We tried to draw a distinction between the mandatory fixed vs. tlv headers.  I can see how it's not clear in the above text.

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

PQ>  That works for me.

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

PQ>  I think the current text is more explicit.  Do you have a specific concern?

> 
>   NSH defines two MD types:
> 
> s/NSH/This document/
> s/types/Type values/

PQ>  Will update accordingly.

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

PQ>  Will update.

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

PQ>  Will update to reflect all of the above.

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

PQ>  New text:

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

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

PQ>  Correct.

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

PQ>  Will be updated.

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

PQ>  I suspect this is a vestige from an earlier option where the path ID was considered sufficient in some cases: a SFF could, in principle, use the path ID to select the next SFF if, for example, there were no SFs, or could opt to send packets to a re-classifier based on path alone.  Having said that, I'm not sure there's much need to those cases.  Do others?

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

PQ>  Once we finalize the TTL details, I agree this'll be cleaned up.

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

PQ>  I think figure 4 provides a very concrete representation of a type-1 header so it probably makes sense to keep it.  I do agree: length should reflect the 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."

PQ>  This text should have been changed, it's out of date.  We've moved to a single 16 byte "blob" -- as per figure 4 -- and this text should reflect that.

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

PQ>  New text will be added.

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

PQ>  The text covers the case where the SF needs metadata (let's say for 
PQ> policy enforcement) but does not (yet)
know how to parse the metadata.   I don't see any implication on config vs implementation.  

> - You are assuming that the semantics for the metadata are always  
> dynamically installed in the SF and not known a priori.
> 

PQ>  There was no intent to imply that, perhaps the easiest way to 
PQ> clarify: "...not yet receive..." --> "...not yet know..." (or 
PQ> something similar)

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

PQ>  I don't see that conveyed.  The text is just unambiguous about an SFC-SF.  It makes not claims about others.  

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

PQ> Fair point.

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

PQ>  This works for me.

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

PQ>  Again, I think being explicit with figures helps the reader.  Updating length field makes sense. 

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

PQ>  I re-read it several times, and although it's a bit clumsy, I'm not sure it's confusing.

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

PQ> I agree, I'll update the figure to show a better representation of the structure.  That will probably help with the 3.5 text.

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

PQ>  I'll take a pass at cleaning it up.

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

PQ>  Yup, will update accordingly once we finish the discussions.

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

PQ>  Barring objection from the list, that's fine with me.

> 
> 4.
> 
>   NSH-aware nodes are the only nodes that MAY alter the content of the
>   NSH headers.
> 
> s/MAY/may/

PQ>  Thanks.

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

PQ>  I agree.  Barring objection, will remove.

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

PQ>  I think that's a good clarification but I fear the last part is confusing since I think of post-NSH the packet being un-encapsulated (unless I'm missing something you are trying to convey):

"...and MUST remove it  before forwarding or delivering the un-encapsulated packet."

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

PQ>  Logical in the sense that they need not be distinct classifiers.  We've been consistent with the concept that classifiers, SFFs, etc. are logical: they can be co-mingled or distinct.  

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

PQ>  Minor tweak:

 Non-initial classifiers may re-classify data and that re-classification MAY result in the selection a different Service Function Path.

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

PQ>  Worth fixing, thanks.

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

PQ>  Thanks.

> ---
> 
> 4.
> Bullet 3
> 
>       Classifier(s) MAY update Context Headers if new/updated context
>       is available.
> 
> s/Classifier(s)/Classifiers/
> 

PQ>  Will update.

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

PQ>  Correct, this means update the existing contexts. I believe we decided that only classifiers can add/remove (but as per a point above, those classifiers are logical and can be "part' of an SF).  I need to go back and check the archives.   Joel might remember since I think we talked about it.

> Decide on "non-NSH-aware", "NSH unaware", "non-SFC-aware".
> 
> Say "decrement the service index by one".

PQ>  Will update.

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

PQ>  I think we can simplify and change service function instance to service function.  

> 
> s/service header/NSH/ (twice)

PQ>  Thanks.

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

PQ>  Perhaps remove it, and beef up the first a bit.  Let me fiddle with 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.

PQ>  That's a fair point.  The intent was to convey the forwarding aspect of the SFF.  Update: "Forward NSH Packets"

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

PQ>  Yes.

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

PQ>  I'm not sure what you mean.  The proxy maps the packet to a local connection.  The proxy doesn't have any service policy insight.

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

PQ>  Must there not be some form of indicator -- explicit or otherwise?

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

PQ>  We can add something down the lines of: SF need to be able to return a packet to the appropriate SFF when service processing is complete.  This can be via the over or under-lay and in some case require additional configuration on the SF.

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

PQ>  See below.

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

PQ>  I don't think that's prohibited in the text above: pick the right transport.

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

PQ>  I'm fine with the last sentence.

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

PQ>  We do, it's a core use case and depicted in figure 15 and described in 8.2, bullet 1.

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

PQ>  Will update all three.

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

PQ>  I'll go re-read the thread.
_______________________________________________
sfc mailing list
sfc@ietf.org
https://www.ietf.org/mailman/listinfo/sfc