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

[Benjamin Kaduk <[e-mail]>]

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