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

[Benjamin Kaduk <[e-mail]>]

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

On Thu, Jun 21, 2018 at 06:42:11AM +0000, [e-mail] wrote:
> Hi Benjamin, 
> 
> Thank you for the review.
> 
> Please see inline. 
> 
> Cheers,
> Med
> 
> > -----Message d'origine-----
> > De : Benjamin Kaduk [mailto:[e-mail]]
> > Envoyé : jeudi 21 juin 2018 03:45
> > À : The IESG
> > Cc : [e-mail]; Behcet Sarikaya; sfc-
> > [e-mail]; [e-mail]; [e-mail]
> > Objet : Benjamin Kaduk's Discuss on draft-ietf-sfc-hierarchical-09: (with
> > DISCUSS and COMMENT)
> > 
> > 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! 
> 
> [Med] Thank you. 
> 
>  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.
> 
> [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."

Yes, that's the spot(s).

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

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

> > 
> > Why does Section 4.1 offer five different choices with no guidance on how
> > 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
> > choices really necessary?  Complexity breeds implementation bugs which
> > breed security issues, so I feel that this breadth of choice needs to be
> > 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
> > 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?
> 
> [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.   

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

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

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

> > This seems especially poignant given the already large size of candidate
> > approaches.
> 
> [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.)

> > 
> > 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.
> 
> [Med] The semantic is local to a domain. 

Agreed, it will not escape a single domain.

>   However, I'm not sure that it's
> > actually true, since we generally want multiple vendors to be able to
> > interoperate,
> 
> [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.

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

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.

> > 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.
> > 
> 
> [Med] The document does not specify any metadata. It does only define an architecture for hSFC and explores deployment options.
>  
> 
> > 
> > ----------------------------------------------------------------------
> > 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.
> 
> [Med] Fixed. Thank you. 
> 
> > 
> > Section 4.1.5
> > 
> > Figure 4's caption should indicate where the base NSH fixed-length context
> > header is originally defined.
> 
> [Med] I added "[RFC8300], Section 2.4"
> 
> > 
> > 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.
> > 
> 
> [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.

> > 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"
> 
> [Med] Fixed. 
> 
> > 
> > Also, I think the two occurrences of "to permit specific metadata types"
> > should be "to *only* permit specific metadata types".
> > 
> 
> [Med] Agree. 
> 
> > 
> > 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.
> 
> [Med] Works for me. 
> 
> > 
> > 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.
> 
> [Med] The text in Section 4 is clear that the hSFC requires IBN to behave as a classifier: 
> 
>    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.
> 
> We can add text to A.1 but IMHO this is redundant. 

I'm happy to defer to you here.

-Benjamin