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 <noreply@xxxxxxxx> 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.
<<< text/html; charset="US-ASCII"; name="draft-ietf-sfc-multi-layer-oam-27.diff.html": Unrecognized >>>
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
