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

[Stewart Bryant <[e-mail]>]

Ben

Many thanks for a thorough review.

The actions I have taken on your comments are shown inline.

> On 25 Feb 2021, at 09:14, Benjamin Kaduk via Datatracker <[e-mail]> wrote:
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-mpls-rfc6374-sfl-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-mpls-rfc6374-sfl/
> 
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> There may be a couple of internal (or external?) inconsistencies
> lingering, and while my confidence level of that is not as high as it
> often is, it's probably still worth checking.
> Specifics in the COMMENT, but at a high level, do we use ACH value
> 0x000A (what's shown in Section 9) or the TBD IANA-allocated values; we
> seem to refer to RFC 6374 as providing some things that I can't find in
> it; and the IANA considerations registration request includes a "TLV
> follows" column that does not appear to be present in the registry (and
> does not appear to match the described behavior in the rest of the
> document).  Perhaps this (the current registry state) is the result of
> RFC 7026's actions, but I'm not entirely sure.
> 

SB> This is fixed as agreed.

> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> A bit more introduction about how the term "delay" is used as
> effectively synonymous with "inter-packet gap" (for purposes of the
> mechanisms presented in this document) would be appreciated.

SB> I am not sure how to address this. We really are measuring delay.
SB> The availability of good time references makes this easier than it used to be.
SB> There is some average reporting to compress the statistics by delay really is delay.

> 
> Abstract, Introduction
> 
> (editorial) the phrases "general MPLS LSPs" and "general MPLS case",
> while intended to contrast against RFC 6374's restriction to just
> MPLS-TP, looks rather similar to "generalized MPLS" and could be a bit
> confusing.  I'd suggest a phrasing like "the generic case of MPLS LSPs,
> not limited to MPLS-TP".

SB> I changed this to:
SB> This document describes a method of extending RFC 6374 performance
SB> measurements from flows carried over MPLS-TP to flows carried over generic MPLS LSPs.
> 
> Section 5
> 
> I'd suggest a legend that A/B are colors for loss measurement and C/D
> are delay groups, and what is depicted is a time series of (the marking
> of) packets transmitted.

SB> I included:
     A & B are packets where loss is being measured
     C & D are pacekts where loss and delay is being measured


> 
>   measurement types would be identical.  Thus, it is proposed that the
>   two measurements are made relatively independent from each other.
> 
> How does this "proposed" relate to the simplifying rules in Section 6
> that put some fairly strong dependencies on how delay measurements
> relate to loss measurements?

SB> changed to 
Thus, we use a technique that
permits the two measurements are made concurrently and yet relatively
independent from each other.

SB> I will look at this working again during Auth48.

SB> So long as you are making loss measurements (which you can ignore) you
Can make delay measurements whenever you wish

> 
> Section 7.1
> 
>   The packet format of an RFC6374 Time Bucket Jitter Measurement
>   Message is shown below:
> 
> It seems a little misleading to have "RFC6374" in the name here without
> some classifier such as "framework" or "style", since the actual
> structure does not appear in RFC 6374.
> (Similarly for the other message formats.)

SB> I have removed the RFC6374 from the names and called the messages by their full
Names which should address the concern.

> 
> Section 7.2.1
> 
>   The Version, Flags, Control Code, Message Length, QTF, RTF, RPTF,
>   Session Identifier, and DS Fields are as defined in section 3.7 of
>   RFC6374.  The remaining fields are as follows:
> 
> I don't see a Section 3.7 in RFC 6374.
> This does, however, look like a lot like Section 3.3 thereof.

SB> Thanks for catching that, 3.2 is what was intended.

> 
> Section 7.4
> 
>   In this approach the packet is time-stamped at arrival and the
>   Responder returns the sum of the time-stamps and the number of times-
>   tamps.  From this the analytics engine can determine the mean delay.
>   An alternative model is that the Responder returns the time stamp of
>   the first and last packet and the number of packets.  This method has
>   the advantage of allowing the average delay to be determined at a
>   number of points along the packet path and allowing the components of
>   the delay to be characterized.
> 
> This prose suggests that an implementation or deployment might choose
> only one or the other of the "alternative model"s, but the packet layout
> and accompanying field descriptions suggest that all fields are to be
> populated, always.  Some clarity on whether it is permissible to select
> only one of the alternatives would be welcome.

SB> I added

Unless specifically configured otherwise, the
responder may return either or both types of response and
the analytics engine should process the response appropriately.

The responder will be an NPU, but I assume that the analytics engine
Is a heavyweight computer so can cope with any of the response types.

> 
>   The Version, Flags, Control Code, Message Length, QTF, RTF, RPTF,
>   Session Identifier, and DS Fields are as defined in section 3.7 of
>   RFC6374.  The remaining fields are as follows:
> 
> (There is still no Section 3.7 in RFC 6374.)

SB> Fixed as well
=======

> Section 8
> 
> (editorial) The purpose of this section is somewhat unclear.  It seems
> that the current text can (very roughly) be summarized as: "up until
> now, we assume we measure every packet in a colored interval.  RFC 8321
> did some other stuff; [details elided].  The RFC 8321 approach is not
> impacted by ECMP effects."  Nowhere does it seem to say that the RFC
> 8321 approach is directly applicable to the RFC 6374-style packets
> defined in this document, or some other reason why these facts are
> noteworthy in the context of this document.

SB> I have added

This sampled approach may be used if supported by the Responder and
configured by the opertor.

> 
> Section 9

=============
> 
> Why do we only depict the use of ACH 0x000A (MPLS Direct Loss
> Measurement) when we also perform delay measurement (which might use ACH
> 0x000C) and also seem to be allocating three additional codepoints that
> would go in this range?

SB> I think that the text to address the discuss fixes this.

> 
> Should the figure still refer to an "RFC6374 Fixed Header" even though
> we admit variable-length headers for some measurements?  (Also, it's a
> bit surprising to use that terminology when RFC 6374 itself does not.)

SB> We are using a (pure) RFC6474 example so I think the references to 6374 are OK

SB> RFC6374 used the term “fixed-format” portion of the message, so I have used that in the figure and the corresponding text


> 
>   The RFC6374 measurement message consists of the three components, the
>   RFC6374 fixed header as specified in [RFC6374] carried over the ACH
>   channel type specified the type of measurement being made (currently:
>   loss, delay or loss and delay) as specified in RFC6374.
> 
> It doesn't quite seem that "as specified in RFC 6374" applies, since
> (IIUC) the "fixed header" is referring to the message formats from
> Sections 7.1, 7.2.1, and 7.4 of this document.

SB> We are extending RFC6374 with some additional measurement types, but the original measurements types (messages) still work. The section is an illustration, so I think it is OK to use the illustrative format to explain the protocol.

> 
>   Two optional TLVs MAY also be carried if needed.  The first is the
>   SFL TLV specified in Section 9.1.  This is used to provide the
>   implementation with a reminder of the SFL that was used to carry the
>   RFC6374 message.  This is needed because a number of MPLS
>   implementations do not provide the MPLS label stack to the MPLS OAM
>   handler.  This TLV is required if RFC6374 messages are sent over UDP
>   [RFC7876].  This TLV MUST be included unless, by some method outside
>   the scope of this document, it is known that this information is not
>   needed by the RFC6374 Responder.
> 
> Is any entity tasked with ensuring consistency between the actual label
> stack and the value in this TLV?

SB> I am not sure that we can include a receiver check as it is there for some hardware implementations that do not preserve the information, and if the transmitter get it wrong all bets are off. However if it is wrong the measurement will be wrong and this will be noticed by the operator. However I will add a note in the security section.

SB> I have added

An attacker that manages to corrupt the RFC6374 SFL TLV {{sfltlv}} could
disrupt the measurements in a way that the RFC6374 responder is unable to
detect. However, the network opertator is likely to notice the
anomalous network performance measurements, and in any case
normal MPLS network security proceedures make this type of attack extremely unlikley.

> 
>   otherwise MUST be carried.  The return address TLV is defined in
>   RFC6378 and the optional UDP Return Object is defined in [RFC7876].

SB> Thanks. For some reason this was our ascii not a reference so did not get picked up by the normal compilation checks. 


> The Return Address TLV is specified in RFC 6374, not RFC 6378.
> 
SB> Thanks. For some reason this was our ascii not a reference so did not get picked up by the normal compilation checks. 

> Section 9.1
> 
>   The required RFC6374 SFL TLV is shown in Figure 6.  This contains the
> 
> (nit) per the previous section, this TLV is only mostly required.

SB> Thanks - shanked the text to 

The RFC6374 SFL TLV is shown

> 
>   SPL Index The index into the list of SFLs that were assigned
>             against the FEC that corresponds to the SFL.
> 
>             Multiple SFLs can be assigned to a FEC each
>             with different actions. This index is an optional
>             convenience for use in mapping between the TLV
>             and the associated data structures in the LSRs.
> 
> Is there any entity tasked with ensuring consistency between the index,
> the SFL, and the batch?
> 

SB> No, but I think this would be picked up by normal defensive programming.


> Section 12
> 
> While I agree that there's not much new in this document, incorporating
> by reference the considerations from (e.g.) 6374 and 8372, if not more,
> seems in order.  If I understand correctly those would also pull in the
> documentation of the generic privacy considerations with using user data
> for monitoring.
> 
> One thing that might be new in this document is the potential for data
> skew between the label stack and the SFL TLV, so that depending on one's
> implementation the reported results would be different.  (Similarly for
> batch/index/SFL relationships within the SFL TLV.)

SB> I added:

The security considerations documented  in {{RFC6374}} and {{RFC8372}}
(which in turn calls up {{RFC7258}} and {{RFC5920}}) are applicable to this
protocol.

> 
> Section 13.1
> 
>   As per the IANA considerations in [RFC5586], IANA is requested to
>   allocate the following codeponts in the "MPLS Generalized Associated
>   Channel (G-ACh) Type" registry, in the "Generic Associated Channel
>   (G-ACh) Parameters" name space:
> 
> The only thing I see at
> https://www.iana.org/assignments/g-ach-parameters/g-ach-parameters.xhtml
> that seems to match this description is written there as "MPLS
> Generalized Associated Channel (G-ACh) Types (including Pseudowire
> Associated Channel Types)".
> 
> I also don't see any "TLV follows" columns on that page, and don't
> understand why "no" is listed for these, since Section 9 is clear that
> the SFL TLV is mostly mandatory.

SB> We deprecated the TLVs. I have added the RFCs that updated RFC5586 and deleted the TLV column.
> 
> Section 16
> 
> If we are deferring to draft-bryant-mpls-sfl-control for the contents of
> some protocol message fields, I can't see how it's anything other than a
> normative reference.
> 

SB> draft-bryant-mpls-sfl-control is only one of the control protocols that some claim we need (though it is the only one that I propose to write). My concern was that if I made it normative this would stall on that text, and yet the design is valid without any control protocol as it could be controlled by an SDN controller. Indeed I suspect that SDN controllers are now so popular compared to when I started this that it will be the overwhelming deployment model.


- Stewart