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

[Greg Mirsky <[e-mail]>]

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.