Re: [Int-dir] Intdir telechat review of draft-ietf-sfc-multi-layer-oam-26

[CARLOS JESUS BERNARDOS CANO <[e-mail]>]

Hi Greg,

Apologies for my belated reply. Thanks for the updates. They work for me.

Thanks,

Carlos

On Thu, Jun 29, 2023 at 1:56 AM Greg Mirsky <[e-mail]> wrote:

> Hi Carlos,
> thank you for your thoughtful comments and helpful suggestions. Please
> find my notes below tagged by GIM>>,
> I wincluded updates in the new working version. Please take a look into
> attached diff that highlights all the updates discussed below.
>
> Regards,
> Greg
>
> On Wed, Jun 28, 2023 at 11:04 AM Carlos Bernardos via Datatracker <
> [e-mail]> wrote:
>
>> Reviewer: Carlos Bernardos
>> Review result: Ready with Issues
>>
>> I am an assigned INT directorate reviewer for
>> draft-ietf-sfc-multi-layer-oam.
>> These comments were written primarily for the benefit of the Internet Area
>> Directors. Document editors and shepherd(s) should treat these comments
>> just
>> like they would treat comments from any other IETF contributors and
>> resolve
>> them along with any other Last Call comments that have been received. For
>> more
>> details on the INT Directorate, see
>> https://datatracker.ietf.org/group/intdir/about/
>> <https://datatracker.ietf.org/group/intdir/about/>.
>>
>> Based on my review, if I was on the IESG I would ballot this document as
>> NO
>> OBJECTION.
>>
>> The following are other issues I found with this document that SHOULD be
>> corrected before publication, including also minor issues (typos,
>> misspelling,
>> minor text improvements):
>>
>> Section 2.2:
>> I’d add references to the documents where the acronyms are
>> defined/introduced.
>>
> GIM>> I agree that that could be helpful to a reader but, as I understand
> it, we expect that the reader is already familiar with the terminology
> through reading documents that define architecture, and applicable
> solutions for the particular problem area.
>
>>
>> Section 3:
>> “Segment SFC OAM” is not introduced/defined before in the document and I
>> think
>> it’d be good to do it.
>>
> GIM>> Would the following update make the interpretation of Segment SFC
> OAM clearer:
> OLD TEXT:
>    Segment SFC OAM is between two elements that are part of the
>    same SFP.
> NEW TEXT:
>    The scope of Segment SFC OAM is between two elements that
>    are part of the same SFP.
>
>>
>> “For example, in the E2E FM SFC OAM case, the egress, SFF3, in the
>> example in
>> Figure 1, could be the entity that detects the SFP's failure by
>> monitoring a
>> flow of periodic test packets” —> this can be rewritten, to avoid
>> repetitive
>> use of “example”
>>
> GIM>>  Thank you for pointing this out. Would the following update improve
> it:
>    For example,
>    in the E2E FM SFC OAM case, the egress, SFF3 (Figure 1) could be the
>    entity that detects the SFP's failure by monitoring a flow of
>    periodic test packets.
>
>>
>> “*REQ#8: Can be addressed by an extension of the SFC Echo Request/ Reply
>> described in this document adding proxy capability“ —> I find this not
>> easy to
>> parse (it refers to a way to satisfy a previously enumerated requirement,
>> but
>> it is written differently to the other ones). Besides, it mentions “proxy
>> capability” and this is not mentioned using these words anywhere else in
>> the
>> document.
>>
> GIM>> Indeed, that is an awkward sentence. Would the following re-wording
> help:
>       REQ#8: Can be addressed by adding the proxying capability to the
>       SFC Echo Request/Reply described in this document.  [RFC7555]
>       describes an example of a proxy function for an Echo Request.
>       Specification of proxy function for SFC Echo Request is outside
>       the scope of this document.
>
>>
>> Section 4:
>> It mentions message and packet. I guess both terms are used to refer to
>> the
>> same thing, but I’d just use one term.
>>
> GIM>> It seems like both terms are equally used in the document - message
> (60) vs. packet (56). I hope that it can be left for the RFC Editor
> consideration. WDYT?
>
>>
>> Section 5:
>> I’d probably explicitely add over which protocols an “Active SFC OAM
>> Header”
>> can be transported.
>>
> GIM>> To ensure that an SFC active OAM packet traverses the same path,
> i.e., SFFs and network that interconnect them, the SFC OAM packet,
> including the Active SFC OAM Header, MUST be transported over the same
> protocols as an SFC data packet. One example is SFC NSH.
>
>>
>> Any requirement on the length/alignment of the SFC Active OAM Control
>> Packet?
>>
> GIM>> I cannot think of any such requirement.
>
>>
>> Section 6.3.1:
>> Should the title of this be “Source ID TLV”? Same applies to the caption
>> of
>> Figure 5. I find a bit misleading using “Source TLV” and “Source ID TLV”
>> and
>> “SFC Source TLV”.
>>
> GIM>> Thank you for catching this inconsistency in terminology.  I changed
> everything to Source ID TLV.
>
>>
>> I find the first paragraph of this section a bit too convoluted, because
>> it
>> describes which destination IP and UDP to use, while these fields are in
>> the
>> TLV message described after this paragraph.
>>
> GIM>> I reworked it. Would the new version be clearer?
>    Because the NSH does not identify the ingress
>    node that generated the Echo Request, information that sufficiently
>    identifies the source MUST be included in the message so that the IP
>    destination address and destination UDP port number for IP/UDP
>    encapsulation of the SFC Echo Reply could be derived.
>
>>
>> When describing the “IP address” field it says that the value of the
>> field MUST
>> be used as the destination IP address. But this is not said when
>> describing the
>> “Port Number” field. I’d do the same for consistency.
>>
> GIM>> Agree. Done.
>
>>
>> Section 6.4:
>> When the document says “the control plane is triggered”, does this mean
>> when an
>> SFC Echo Reply is triggered? It is not 100% clear what is referred here.
>>
> GIM>> You are correct. We describe conditions that cause punting SFC Echo
> Request out from the data plane for processing in the node's control plane.
> I re-worded it as follows:
>    Punting a received SFC Echo Request to the control plane for
>    validation and processing is triggered by one of the following packet
>    processing exceptions: NSH TTL expiration, NSH Service Index (SI)
>    expiration, or the receiver is the terminal SFF for an SFP.
>
>>
>> Again “Source TLV” should be checked for consistency and use whatever
>> term is
>> finally used.
>>
> GIM>> Changed to "Source ID TLV"
>
>>
>> Section 6.5.3:
>> Minor: any recommendation on how many past SFC Echo Requests to keep to
>> match
>> agains Echo Replies received?
>>
> GIM>> An interesting question, thank you. AFAIK, the echo request has a
> timer that would expire if no matching echo reply is received. Thus, only
> one outstanding echo request is allowed.
>
>>
>> Section 6.6.1:
>> “Must to include” —> “MUST include”
>>
> GIM>> Thank you for catching this.
>
>>
>> “SF Information Sub-TLV: The sub-TLV is as defined in Figure 10.”
>> —> I’d use “Section 6.6.2” instead of “Figure 10”.
>>
>
> GIM>> Agreed.
>
>>
>> Section 6.6.2:
>> Any alignment considerations for the “SF Identifier” field?
>>
> GIM>> Good question, thank you. It appears that implicitly we expect that
> an IP address will be used as an SF ID. Could there be other types of
> identification? What do you think?
>
>>
>> Sections 6.6.3.1 and 6.6.3.2:
>> Though I understand that there is a semantic difference, should we use
>> the same
>> nomenclature in Figure 11 and Figure 12 regarding “CVRep1(SF1,SF2)” and
>> “CVRep1({SF1a,SF1b})”.
>>
> GIM>> Changed to only round brackets.
>
>>
>> Section 7:
>> “To protect against unauthorized sources trying to obtain information
>> about the
>> overlay and/or underlay, an implementation MAY check that the source of
>> the
>> Echo Request is indeed part of the SFP.” —> I personally find this MAY
>> weird.
>> Either it is left to the implementation how to check if the source of the
>> Echo
>> Request is authorized (independently of whether is part of the SFP or
>> not) or
>> I’d change this to be a SHOULD or a MUST if the sender has to be part of
>> the
>> SFP.
>>
> GIM>> Thank you for pointing this out. I agree that 'MAY' is too weak.
> Changed to MUST.
>