Subject: [sfc] Mirja Kühlewind's Discuss on draft-ietf-sfc-nsh-24: (with DISCUSS and COMMENT)

From: Mirja Kühlewind <[email-masked]>
Date: Wed, 27 Sep 2017 10:02:45 -0700

Mirja Kühlewind has entered the following ballot position for
draft-ietf-sfc-nsh-24: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-sfc-nsh/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I have a couple of comments on the design. I know, as always in IESG review
state, it's probably too late to make any changes to the actual header format,
therefore most of my comments are actually in the comment section below. I
still decided to note them so at least people can consider these points.
However, there are a few things that I need clarification for before
publication, which I note in this section:

1) Sec 2.2
"SF/SFF/SFC Proxy/Classifier 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.  Forwarding OAM
   packets unmodified by SFC elements that do not support SFC OAM
   procedures 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."
This part is really unclear to me and I believe needs to be further specified.
Where should this configurable parameter be? In the Context header? Why don't
you just use one of the unassigned bit to indicate if an unknown (OAM) packet
should be forwarded or not? Moreover, I also disagree with this text. If there
is a bit/a way to indicate if a not supported OAM packet should be forwarded or
not, it should just be defined like this, while any considerations if that bit
should be set or not depend on the OAM function itself and do not need to be
discussed here. Finally, it is not well explained what an OAM packet is at all.
Is that a 'fake' packet that is generated by the operator to actively test the
(potentially newly configured) SFP? If so, why does a SF need to know if a
packet is an OAM packet or not? Usually it's a bad idea to use different kind
of traffic for testing compared to what will be used in operations. Please
provide more explanation here!

2) section 2.4
"An SFC-aware SF MUST receive the data semantics first in order to
   process the data placed in the mandatory context field.  The data
   semantics include both the allocation schema and the meaning of the
   included data.  How an SFC-aware SF gets the data semantics is
   outside the scope of this specification."
This is really confusing to me. I think this is what you need an actually data
semantics aka type field for in the base header. Or is there an actual reason
to not put this information directly in the base header where it is need but
instead assuming some magical way this information may take to reach the node?
If the assumption is that the SF is configured to know based of the SFI what
the content of the context header has to be, you a) need to say that in the
draft, and b) that's really error-prone because it's really hard to tell if the
conext header actually holds the information that you need or just random crap
(of course depending of the expected data type of this information). In short,
I think you really need a type field somewhere here. In any case, you really
need to explain this more! Also, the text further says: "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..."
How does the SFC proxy know that it knows the format or not if there is no type
field or identifier that indicates what the format should be?

Also, a related question from me: why is the context header present in all
types of NSH if there is no use for it defined in this document yet? Why is
there no fixed length NSH without a context header then?

3) Section 2.5.1:
"If multiple instances of the same metadata are included in an NSH
   packet, but the definition of that context header does not allow for
   it, the SFC-aware SF MUST process the first instance and ignore
   subsequent instances."
This seems error prone to me. If the same metadata appears multiple where it
should not, that seems clearly like an error case for me. Just using the first
one and proceed normally might not be the right thing to do. In any case I
think such an occasion should at least be logged. If the multiple instances are
just a copy of each other and carry the same information, it's probably okay to
use that information and proceed. If the different instances carry different
information, it maybe a bit dangerous to just use the first one and ignore
others silently. In this case I would rather recommend to drop the packet...

4) In line with the second comment from the tsv-art review (thanks Wes!), I
don't really understand why this documents says (sec 6) that there can be
multiple next hops for the same SFP or SFs can be traversed in a different
order. May understanding (from a quick look at RFC7665) would be that, if those
things are needed e.g. for load balancing, then one should define different
SFPs and the Classifier must have the knowledge that two SFP are equivalent and
select them respectively. The reason why I'm really concerned about this is
that usually a number of packets below to a flow and all packets belonging to
the same flow just ideally take the same route. But usually only the Classifier
has a notion of what a flow is and respectively will assign the SFI to the
packets belonging to the same flow. If now any SF on the path can more or less
randomly decided to forward packet belong to the same flow to one or another
next nodes, I would assume that this is not only a problem for the flow, e.g.
reordering, but also for SF itself in many cases.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Further considerations:

1) I don't really see why a TTL in the base header is needed. I mostly
understand why there is the Service Index in service header, also I think there
should be better mean to validate that you SFP is correct and I ideally you
should really not need this. However, loop prevention can be provided by both
mechanism and moreover there is probably also often a TTL in the encapsulation
protocol and loop prevention should really be a function of that forwarding
protocol and not the NSH.

2) I don't see why you need the type field in the base header. This is fully
redundant because because all you need is the length field. If the length is
0x2 it is what you have defined as type 1 if the length field is larger it is
type 2. I also don't see the need for any other types in future because you
also have the version field; if you need anything else you should go or
probably have to go for a new version. Note that the general probably with
unnecessary redundancy is that is add complexity. If you keep this redundancy
you have to separately handle and implement the case(s) where the type is 1 but
the length is larger than expected. If at all you could probably just use one
bit to indicate that the length field is present and if not the length is 0x2.
However, saving bits does not really seem to be a concern for you, so that
might not actually be an advantage.

3) sec 2.5.1
"Unassigned bit: One unassigned bit is available for future use.  This
   bit MUST NOT be set, and MUST be ignored on receipt."
Is there an actual reason to have an unassigned bit here? Because I would
assume that the type already provided enough flexibility for way to extend the
metadata format in any way needed.

4) Also section 2.5.1:
"   Length: Indicates the length of the variable metadata, in bytes.  In
   case the metadata length is not an integer number of 4-byte words,
   the sender MUST add pad bytes immediately following the last metadata
   byte to extend the metadata to an integer number of 4-byte words.
   The receiver MUST round up the length field to the nearest 4-byte
   word boundary, to locate and process the next field in the packet."
Your definition of the length field might be more error-prone than needed. It
would probably be easier to simply define the length as 4-byte words, and the 
type of course defines the content of the metadata field and as such can simply
define which part of the total metadata field holds certain data of a certain
type and with part is padding.

5) And finally I have to say it is unclear to me why the SFI and SI field are
described as a separate header. Given they have to be present in all SFH, I
would consider them as two fields of the base header. But it is after all
really just an editorial issue. However, all this together with my previous
comments makes the protocol spec actually much more complicated than it needs
to be...

6) I also have to agree to the last comment of the tsv-art review: I think it
would have been nicer to not only described the NSH but also define mappings to
a set of possible encapsulations because I would assume that for each
encapsulation there are a couple specific considerations that need to be made
to make things work successfully. I don't think that all encapsulations can be
captured by general consideration and I cannot make up my mind to go through
all cases in my head to figure out if there are things that needs to be noted.

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Wed, 27 Sep 2017 19:13:28 +0000

Thank you, Mirja, for these questions and details.

Please find follow-ups inline.

> On Sep 27, 2017, at 1:02 PM, Mirja Kühlewind <[email-masked]> wrote:
> 
> should be forwarded or not?

The configurable parameter refers to a configuration state and device operational state variable, and not a data plane packet field.

In other words, the default is drop. But if the setting `configure sff forward oam` then it is forwarded.

I will say that this was a strong contentious point in the WG process to keep this configuration option (although my personal preference was not to have it, the WG was loud on it.)

> Moreover, I also disagree with this text. If there
> discussed here.

I think the disagreement comes from believing the config option is a data plane bit.

> Finally, it is not well explained what an OAM packet is at all.

If it helps, although it seems redundant, we can add a pointer to RFC 6291 or RFC 7276. 

> packet is an OAM packet or not?

It is an OAM packet, which can include connectivity verification, continuity check, performance management, etc. It could be a synthetic packet, or, well, an OAM packet :-)

> Usually it's a bad idea to use different kind

I am not sure how to address this, since the definition of OAM for SFP is outside the scope. 

Data plane protocols use an 'interception marker’ to differentiate them from actual traffic, in the context of Active or Hybrid OAM (as opposed to Passive OAM).

> instead assuming some magical way this information may take to reach the node?

Believe it or not, no magic involved. It’s called “control plane” which is out of scope.

> the content of the context header has to be,

No, the assumption is not that an SF “is configured”. And the assumption is not that the SF knows what the content of the context header is.

The assumption is that the SF needs to receive the data semantics before processing the data plane packet.

> you a) need to say that in the
> draft,

The document says what it means and means what it says in this regards. If you find the text confusing, we can try to clarify it somehow.

> and b) that's really error-prone because it's really hard to tell if the

Is “crap” a technical term here?

> I think you really need a type field somewhere here.

Mirja, there’s self-describing data and non-self-descriving data. Both have their place and can be used based on what a protocol is optimizing. This text described a use of the latter.

> In any case, you really
>  such an NSH…"

I keep reading these couple of sentences and I understand what they say. So I am not positive at this point how to help out.

> field or identifier that indicates what the format should be?

Same as the first sentence you included in this DISCUSS item #2: 

"An SFC-aware SF MUST receive the data semantics first in order to
 process the data placed in the mandatory context field.”

Since an SFC proxy acts as an SFC-aware SF. That is clear in the document.

That said, I will add the following to make it unambiguously clear:

"An SFC-aware SF or SF Proxy MUST receive the data semantics first in order to
 process the data placed in the mandatory context field.”

And 

OLD:
 included data.  How an SFC-aware SF gets the data semantics is
 outside the scope of this specification.”

NEW:
 included data.  How an SFC-aware SF or SFC Proxy gets the data semantics is
 outside the scope of this specification."

> there no fixed length NSH without a context header then?

This document defines two Context Header Types: fixed size, and TLV-based. There is not need, as far as the WG had concluded, for a fixed size no metadata MD Type. 

What you describe can be achieved with MD Type 2 Length = 2.

> think such an occasion should at least be logged.

OK, will add optional logging and error counting of the potentially error case event.

> If the multiple instances are
> others silently. In this case I would rather recommend to drop the packet…
> 
> select them respectively.

Not really. Like you said, for LB.

> The reason why I'm really concerned about this is

https://tools.ietf.org/html/rfc7665#section-5.5

The WG had a strong voice on the need of this field.

(I do not disagree with you)

But there are cases where SI is not decremented, nor is the encapsulation-level TTL.

> type 2.

Thanks for the attempt at overloading, but no. Length = 6?

> I also don't see the need for any other types in future because you
> also have the version field;

NO.

We do *not* want to bump up the Version field for the whole protocol when potentially adding a new MD Type.

> if you need anything else you should go or

I did not fully follow the train of branching pros and cons in your statement, but like you said earlier, there’s running code on this and would not be productive to define a different similar protocol.

> Is there an actual reason to have an unassigned bit here?

I am not sure I understand the question.

The header is 32-bit aligned, and there’s less fields defined that available bits in a 32-bit word… 

> Because I would

?

> type and with part is padding.

Thank you. See above.

> described as a separate header. Given they have to be present in all SFH,

What is “SFH”?

> I
> to be…

Noted.

> all cases in my head to figure out if there are things that needs to be noted.

OK.

Thanks again,

Carlos.