Subject: [sfc] Benjamin Kaduk's Discuss on draft-ietf-sfc-hierarchical-09: (with DISCUSS and COMMENT)

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 20 Jun 2018 18:45:03 -0700

Benjamin Kaduk has entered the following ballot position for
draft-ietf-sfc-hierarchical-09: 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-hierarchical/

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

This does seem like an interesting and potentially useful idea -- thanks
for doing the work!  However, I am not sure that the document as-is is
suitable for publication.

In Section 4.1.5 we hear that preserving metadata and applying metadata to
injected packets is not special and is "usual" functionality, but
section 4.1.2 calls out that the 4.1.2 approach requires the SFs in the
path to be capable of forwarding metadata and attaching metadata to
injected packets as if it is a nontrivial requirement.  This apparent
internal inconsistency needs to be resolved before publication.

Why does Section 4.1 offer five different choices with no guidance on how
to make a cost/benefit analysis amongst them?  Is the full spread of five
choices really necessary?  Complexity breeds implementation bugs which
breed security issues, so I feel that this breadth of choice needs to be
justified.  This also ties into some confusion I have as to the goal of
this document (which currently targets Informational status), akin to what
is stated in Alvaro's ballot position: is it just providing information on
how to assemble existing pieces in a novel way, or presenting a new
protocol specification that is not yet fit for Proposed Standard status, or
is it listing out potential solutions to a problem so that they can be
implemented and experience gained as to which are useful in practice and
which are not?  Accordingly, I would be interested to hear about what
deployment experience exists already and whether this abstraction proves to
be as useful as the use cases seem to promise; if there is little
implementation experience, perhaps Experimental status is more appropriate.

I further am uncertain as to whether the approach described in Section
4.1.3 even merits consideration at all; the technique it describes seems to
have a very leaky abstraction barrier (e.g., w.r.t TTL modifications).
This seems especially poignant given the already large size of candidate
approaches.

The approach described in Section 4.1.5 also seems to be incompletely
specified, in that the hSFC Flow ID semantics are not covered at all.  On
my initial reading I assumed that this field's encoding and semantics were
intended to be left as entirely local matters to the lower domain, avoiding
a need to specify them in this document.  However, I'm not sure that it's
actually true, since we generally want multiple vendors to be able to
interoperate, and this scheme does not appear to require that the node
applying the hSFC Flow ID (and saving state) is the same node that removes
it (and restores state).  That is, these two nodes could potentially be
implemented by different vendors.

Even once the above issues are resolved, I will only be able to move to an
Abstain position, since this document proposes additional usage of
(meta)data in the NSH headers that is not subject to mandatory integrity
protection, as was discussed at length for RFC 8300 and is mandated to be
available by RFC 7665.

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

Section 4

   To achieve the benefits of hierarchy, the IBN should be applying more
   granular traffic classification rules at the lower level than the
   traffic passed to it.  This means that the number of SFPs within the
   lower level is greater than the number of SFPs arriving to the IBN.

"more granular" and "less granular" are unfortunately ambiguous in
practical usage; I suggest "fine-grained" as an alternative for this usage.

Section 4.1.5

Figure 4's caption should indicate where the base NSH fixed-length context
header is originally defined.

Section 4.4 presents another operational choice that contributes to
exponential complexity growth, and further highlights unique properties of
the Section 4.1.3 approach that may render it unsuitable for inclusion.

Section 7.2

   Other fundamental functions required as IBN (e.g., maintaining
   metadata of upper level or decrementing Service Index) are same as
   normal usage.

nit: "the same as in normal usage"

Also, I think the two occurrences of "to permit specific metadata types"
should be "to *only* permit specific metadata types".

Section 10.1

   Security considerations related to the control plane should be
   discussed in the corresponding control specification documents (e.g.,
   [I-D.ietf-bess-nsh-bgp-control-plane],
   [I-D.wu-pce-traffic-steering-sfc], or [I-D.maglione-sfc-nsh-radius]).

I'm going to call this a nit, but "should be discussed" sounds as if this
document is trying to direct the behavior of the other listed documents;
maybe "are discussed" is better.

Section A.1 could perhaps note the potential drawback that the
classification functionality is now distributed across the domains instead
of being totally centralized at the initial entry, which requires greater
coordination between the different classifers.  (This is, of course,
not necessarily a drawback for all deployments, but could be for some.)

---

From: <[email-masked]>
Date: Thu, 21 Jun 2018 06:42:11 +0000

Hi Benjamin, 

Thank you for the review.

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Benjamin Kaduk [mailto:[email-masked]]
> Envoyé : jeudi 21 juin 2018 03:45
> À : The IESG
> Cc : [email-masked]; Behcet Sarikaya; sfc-
> [email-masked]; [email-masked]; [email-masked]
> Objet : Benjamin Kaduk's Discuss on draft-ietf-sfc-hierarchical-09: (with
> DISCUSS and COMMENT)
> 
> for doing the work! 

[Med] Thank you. 

 However, I am not sure that the document as-is is
> internal inconsistency needs to be resolved before publication.

[Med] I guess you are referring to: 

(4.1.2)

"This approach requires the SFs in the path to be capable of
   forwarding the metadata and appropriately attaching metadata to any
   packets injected for a flow."

And

(4.1.5)

"No special functionality is required to be supported by an SFC-
      aware SF, other than the usual ability to preserve metadata and to
      apply metadata to injected packets."

"usual" is used in the sense that manipulating metadata (update context header and service policy selection) is part of the roles of an SFC-aware SF. Please refer to " Figure 8: NSH Action and Role Mapping" in RFC8300. 

I suggest to update 4.1.5 as follows: 

"No new SFC-related functionality is required to be supported by an SFC-
      aware SF, other than the ability to preserve metadata and to
      apply metadata to injected packets."

> to make a cost/benefit analysis amongst them?

[Med] The rationale of the document is to describe options and leave it to the taste of those who (will) deploy to decide which ones among those is more appropriate for their deployment context. Key advantages and issues are called out. 

We cannot speculate about cost analysis. Really. 

  Is the full spread of five
> justified.

[Med] I would agree with you if we are recommending all (or most) of them.  

  This also ties into some confusion I have as to the goal of
> this document (which currently targets Informational status), 

[Med] The main contributions of the document are as follows: 
- Define an architecture for hSFC (hierarchy level, introduce IBN, define IBN role). This proposed hsFC architecture is generic enough.
- Identify and describe options to glue levels

The document is informational. As such, it does not make use of normative language or pick a favorite option. 

akin to what
> which are not?

[Med] As mentioned above, the document does not declare a favorite option to be deployed (I have one though), but documents candidates that can be considered when deploying hSFC. Some of the options are implementation-specific (4.1.1), some of them are deployment-specific (4.1.3), some require more standardization effort.   

  Accordingly, I would be interested to hear about what
> have a very leaky abstraction barrier (e.g., w.r.t TTL modifications).

[Med] Glad to see that the description of that section is clear enough about the complications that are inherent with this option. Our target reader is likely to have the same reaction as you. 

The question is whether it is harmful to have the option described. 

> This seems especially poignant given the already large size of candidate

[Med] 5 is not a "large size" of candidates, IMO. If this is an issue, we can consider moving some of the options into an appendix.

> a need to specify them in this document.

[Med] The semantic is local to a domain. 

  However, I'm not sure that it's
> interoperate,

[Med] Intermediate SFC elements do not need to understand the semantic of flowID. They will handle the flowID as an opaque value.   

 and this scheme does not appear to require that the node
> implemented by different vendors.

[Med] State sync/replication is indicated in the text: 

   Disadvantages include those of other stateful approaches, including
   state timeout and replication mentioned in Section 4.1.1.

> protection, as was discussed at length for RFC 8300 and is mandated to be

[Med] The document does not specify any metadata. It does only define an architecture for hSFC and explores deployment options.

[Med] Fixed. Thank you. 

> header is originally defined.

[Med] I added "[RFC8300], Section 2.4"

> the Section 4.1.3 approach that may render it unsuitable for inclusion.

[Med] What about moving Section 4.1.3 into an appendix?

> nit: "the same as in normal usage"

[Med] Fixed. 

> should be "to *only* permit specific metadata types".

[Med] Agree. 

> maybe "are discussed" is better.

[Med] Works for me. 

> coordination between the different classifers.

[Med] The text in Section 4 is clear that the hSFC requires IBN to behave as a classifier: 

   traffic passed to it.

We can add text to A.1 but IMHO this is redundant. 

  (This is, of course,

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 22 Jun 2018 13:20:43 -0500

Hi Med,

I see that you already uploaded a -10 that moves the intended status to
Experimental, with a nice clear introduction that lays out the scope and
goals of the proposed experiment(s).  That's a big improvement, so I will
clear my DISCUSS.  More inline...

>       apply metadata to injected packets."

Yes, that's the spot(s).

>       apply metadata to injected packets."

"No new [...], other than <Y>" still can be read to say that the
functionality Y is in fact new.  I think it would be more clear to say
something like "only the existing ability to preserve and apply metadata is
needed" or "The SFC-related functionality required by this approach in an
SFC-aware SF is to be able to preserve and apply metadata, which is a
requirement that was already present in RFC 8300".  But your
explanation/pointer to 8300 suffices to clear my discuss point, so please
treat this as a non-blocking comment.

> [Med] As mentioned above, the document does not declare a favorite option to be deployed (I have one though), but documents candidates that can be considered when deploying hSFC. Some of the option…

The desire to get more deployment experience does seem to fit pretty well
with the Experimental status.

> The question is whether it is harmful to have the option described. 

For an experiment, it seems okay to leave it in.

> [Med] 5 is not a "large size" of candidates, IMO. If this is an issue, we can consider moving some of the options into an appendix.

I don't think 5 is a large size for an experiment, only for a final
protocol.  (Sometimes we still end up with that many, but I remember at
least one case where I complained about it in my ballot position.)

> [Med] The semantic is local to a domain. 

Agreed, it will not escape a single domain.

> [Med] Intermediate SFC elements do not need to understand the semantic of flowID. They will handle the flowID as an opaque value.   

Agreed.  I think it might help to call out (as is done in Section 4.1.1)
that if the egress IBN differs from the ingress IBN, there is a need for
state synchronization between those nodes.

>    state timeout and replication mentioned in Section 4.1.1.

I had read the Section 4.1.1 text as only applying to that particular
approach, implying that state synchronization would not necessarily be
needed for the other approaches.

> [Med] What about moving Section 4.1.3 into an appendix?

I'd say leave it where it is, if we are going with Experimental status.

> We can add text to A.1 but IMHO this is redundant. 

I'm happy to defer to you here.

-Benjamin

---

From: <[email-masked]>
Date: Mon, 25 Jun 2018 14:06:26 +0000

Hi Benjamin, 

Please see inline. 

Cheers,
Med

> Envoyé : vendredi 22 juin 2018 20:21
> À : BOUCADAIR Mohamed IMT/OLN
> Cc : The IESG; [email-masked]; Behcet Sarikaya; sfc-
> [email-masked]; [email-masked]
> Objet : Re: Benjamin Kaduk's Discuss on draft-ietf-sfc-hierarchical-09: (with
> DISCUSS and COMMENT)
> 
> Hi Med,
> 
> clear my DISCUSS.

[Med] Thank you. 

  More inline...
> > "usual" is used in the sense that manipulating metadata (update context
> header and service policy selection) is part of the roles of an SFC-aware SF.
> Please refer to " Figure 8: NSH Action and Role Mapping" in RFC8300.
> >

[Med] The second proposal works for me. 

> > [Med] The rationale of the document is to describe options and leave it to
> the taste of those who (will) deploy to decide which ones among those is more
> appropriate for their deployment context. Key advantages and issues are
> called out.
> >
> > - Define an architecture for hSFC (hierarchy level, introduce IBN, define
> IBN role). This proposed hsFC architecture is generic enough.
> > [Med] As mentioned above, the document does not declare a favorite option
> to be deployed (I have one though), but documents candidates that can be
> considered when deploying hSFC. Some of the options are implementation-
> specific (4.1.1), some of them are deployment-specific (4.1.3), some require
> more standardization effort.
> 

[Med] Yes. 

> > [Med] Glad to see that the description of that section is clear enough
> about the complications that are inherent with this option. Our target reader
> is likely to have the same reaction as you.
> >

[Med] OK. 

> > [Med] 5 is not a "large size" of candidates, IMO. If this is an issue, we
> can consider moving some of the options into an appendix.
> 
> protocol. 

[Med] In full agreement. 

 (Sometimes we still end up with that many, but I remember at
> > [Med] Intermediate SFC elements do not need to understand the semantic of
> flowID. They will handle the flowID as an opaque value.
> 

[Med] This is the intent of "state replication mentioned in Section 4.1.1". 

I changed the text to "state synchronization mentioned in Section 4.1.1".

> > [Med] The document does not specify any metadata. It does only define an
> architecture for hSFC and explores deployment options.
> >
> >
> > >

[Med] OK, thanks. I'm planning to leave the text as it is.

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 25 Jun 2018 15:21:57 -0500

> Please see inline. 

This all looks good; just one final note inline

> I changed the text to "state synchronization mentioned in Section 4.1.1".

Ah, looking at the -10 now I finally see what you are referring to.  I
think when I first read "state timeout and replication mentioned in Section
4.1.1" I did not make the connection that it was specifically the *state*
replication as well.  Probably that's just me reading to fast, but I think
your change does increase clarity.

Thanks again,

Benjamin

---

From: <[email-masked]>
Date: Tue, 26 Jun 2018 06:12:11 +0000

Hi Benjamin, 

Deal! 

I uploaded a new version with the changes discussed in this thread. 

Thank you very much for engaging and reviewing. 

Cheers,
Med 

> Envoyé : lundi 25 juin 2018 22:22