From nobody Fri May 19 18:41:17 2023 Return-Path: X-Original-To: rtg-dir@ietfa.amsl.com Delivered-To: rtg-dir@ietfa.amsl.com Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 404D3C14CE3F; Fri, 19 May 2023 18:41:16 -0700 (PDT) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: -2.094 X-Spam-Level: X-Spam-Status: No, score=-2.094 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=0.001] autolearn=ham autolearn_force=no Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jU7sUHATkKlJ; Fri, 19 May 2023 18:41:12 -0700 (PDT) Received: from mail-ua1-x92c.google.com (mail-ua1-x92c.google.com [IPv6:2607:f8b0:4864:20::92c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 7AFA9C14F738; Fri, 19 May 2023 18:41:12 -0700 (PDT) Received: by mail-ua1-x92c.google.com with SMTP id a1e0cc1a2514c-783f7e82f28so1200097241.1; Fri, 19 May 2023 18:41:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684546871; x=1687138871; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=5U132aJFT8axFvDJXwa+gETeJD7IwVyhek7zEJU6h1U=; b=KYVYRK6pC9K9Zxif1+eoS8tvv0Xjfrsruvyh/UgGhBkUdTuIQka+SRlsO/2VLzSBW+ rq/LN1PgzXGtVKyPA6nxXAgxwqW4cfJe3ynGI3k+i9V41FNGaW6K3wtgKmtCZAWNe9DD jlSezu4uw60jKK/fQpsDJmyGIuOgj8DXtwgLVagKBMCduHU2qM01hmGdEzXW7fkQSO80 XKnDDiQXoJCyjD2KTxVNemCAtN1apgbZ4CM9DtDWx21Lvkll7qG8yT0kB0XZDBHk6F2I 8TQ2+Wj3/Y0gXasHwEVf71OURCIY5A6lOuENpXmqZYRAruUHdm73jchSm1xDO2U21xBA j15Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684546871; x=1687138871; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=5U132aJFT8axFvDJXwa+gETeJD7IwVyhek7zEJU6h1U=; b=QAbgDHOygFg9gygNyYKpv8Ds75yyR+x8zPW0EnuXtw0qGCjoQQ6uh8vnhBy4nuLKMi w3pOMxj+usJYpQ7sPLg2mSzc8YuNxnsI7W3VVsBbsV5ZC567kDLQacBbZetVtnE2VNU0 IdlCns5NQvbnbrdof3UPrWHSmUl2czOKYXKDxNSk0hYTYqyVR6PN+lgmrjzXXvWuUVpZ nXmIZTbOwILigqezI1PF74mnL7AdKXdr3REff1Oq9iev4UVP7iTKl+5OpHXEv28VpI6j bOAvd8EwTG6PMsuGaz/pmi/iL1F5LZRqIg6KKsm/6LMe38jUMRROLcGXcRyx5D8WTBrc XFyQ== X-Gm-Message-State: AC+VfDwhQi6/wQFY3TRAu27BFbAoHb9ucQrtnWbiqqkXkCXuFciTNCZL 3vOVvJaBOvJCoPl5iTZcGM1r5yYrPX3fvM8byjWeFEXQ X-Google-Smtp-Source: ACHHUZ5CLD6hz9B6u3D1KA+VoyZLjYChxjE8JVS+z+X59xwj0/VOr8kFRDIrC97vClZgur/VRgJGXC4Owwd336U/9R8= X-Received: by 2002:a1f:d182:0:b0:44f:cc25:2007 with SMTP id i124-20020a1fd182000000b0044fcc252007mr1529016vkg.11.1684546870786; Fri, 19 May 2023 18:41:10 -0700 (PDT) MIME-Version: 1.0 References: <168267823708.49129.2910500144998874882@ietfa.amsl.com> In-Reply-To: From: Dhruv Dhody Date: Fri, 19 May 2023 20:40:34 -0500 Message-ID: To: Greg Mirsky Cc: Dhruv Dhody , Routing Directorate , BIER WG , draft-ietf-bier-ping.all@ietf.org Content-Type: multipart/alternative; boundary="000000000000d7404105fc162140" Archived-At: Subject: Re: [RTG-DIR] [Bier] Rtgdir early review of draft-ietf-bier-ping-08 X-BeenThere: rtg-dir@ietf.org X-Mailman-Version: 2.1.39 Precedence: list List-Id: Routing Area Directorate List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 May 2023 01:41:16 -0000 --000000000000d7404105fc162140 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks Greg! I did a quick browse, and it seems to be okay! I have not checked each comment resolution but I trust you to do the right thing! On Fri, May 19, 2023 at 2:37=E2=80=AFPM Greg Mirsky = wrote: > Hi Dhruv, > a new version of the draft (10) includes updates that I proposed in our > discussion. Also, it includes updates addressing IntDir comments. I > appreciate it if you could review the updates (diff between the -08 versi= on > and the latest version 10 is attached for your convenience). Please let m= e > know if you find your comments reasonably addressed. > > Best regards, > Greg > > On Mon, May 8, 2023 at 9:19=E2=80=AFAM Greg Mirsky wrote: > >> Hi Dhruv, >> many thanks for your review and comments. Please find my answers below >> under the GIM>> tag. The new version of the draft >> also includes >> updates addressing comments from Sec and Int areas. >> Please let me know if you have any questions or additional suggestions. >> >> Kind regards, >> Greg >> >> On Fri, Apr 28, 2023 at 3:37=E2=80=AFAM Dhruv Dhody via Datatracker < >> noreply@ietf.org> wrote: >> >>> Reviewer: Dhruv Dhody >>> Review result: Has Issues >>> >>> # RTGDIR early review of draft-ietf-bier-ping-08 >>> >>> Hello, >>> >>> I have been selected to do a routing directorate =E2=80=9Cearly=E2=80= =9D review of this >>> draft. >>> https://datatracker.ietf.org/doc/draft-ietf-bier-ping/ >>> >>> The routing directorate will, on request from the working group chair, >>> perform >>> an =E2=80=9Cearly=E2=80=9D review of a draft before it is submitted for= publication to >>> the >>> IESG. The early review can be performed at any time during the draft=E2= =80=99s >>> lifetime >>> as a working group document. The purpose of the early review depends on >>> the >>> stage that the document has reached. As this document is >>> post-working-group-last-call, my focus for the review was to determine >>> whether >>> the document is ready to be published. >>> >>> For more information about the Routing Directorate, please see >>> https://wiki.ietf.org/en/group/rtg/RtgDir >>> >>> Document: draft-ietf-bier-ping-08 >>> Reviewer: Dhruv Dhody >>> Review Date: 28 April 2023 >>> Intended Status: Standards Track >>> >>> ## Summary: >>> >>> I have significant concerns about this document. It needs more work >>> before >>> being submitted to the IESG. >>> >>> ## Comments: >>> >>> The need for OAM for BIER is well established, the mechanism is sound >>> but there >>> is a need to tighten up the text (as needed for a standards track >>> document) and >>> increase the quality of the WG output that is to be considered ready to >>> be sent >>> to the IESG for publication. >>> >>> The good news is that most of my comments are easy to handle. I wonder >>> why >>> these were not caught during the normal WG review itself. Interesting t= o >>> see >>> that the WGLC was initiated way back in 2018. >>> >>> ### General >>> >>> * Request the shepherd to make sure that there is a valid justification >>> for 6 >>> authors. >>> >>> * The document could benefit from an overview of the OAM operation that >>> this >>> document defines before taking a direct jump into the packet formats an= d >>> TLVs. >>> >> GIM>> Thank you for this suggestion. The following text is proposed to >> prepend the last paragraph of the Introduction: >> NEW TEXT: >> Operations, Administration, and Maintenance (OAM) mechanisms are >> expected to support the detection of network failures. After the >> detection, operators localize and characterize the network defect. A >> query-based tool, e.g., ICMP [RFC0792] and LSP Ping [RFC8029], >> [RFC6425], is broadly used to detect and localize a network defect. >> Additionally, this mechanism can be used to check the consistency >> between the data and control planes. >> >>> >>> ### Section 2.1 >>> >>> * Consider adding QTF, RTF, MTU, DA, DIA >>> >> GIM>> Done >> >>> >>> ### Section 3 & 3.1 >>> >>> * The text in Section 3 states - >>> >>> ```` >>> [RFC8296] defines a 4-bit field as "Proto" to identify the payload >>> following the BIER header. >>> ```` >>> >>> whereas RFC 8296 says - >>> >>> ```` >>> Proto: >>> >>> This 6-bit "Next Protocol" field identifies the type of the >>> payload. >>> ```` >>> >>> I also see that the BIER OAM message format defined in this I-D is usin= g >>> 4-bits. Should this not be consistent across BIER documents? - Both >>> number of >>> bits and the name! >>> >> GIM>> Indeed, excellent suggestion. Done. >> >>> >>> * The relationship between the first figure and the second needs to be >>> more >>> explicit. Moreover, there are the following inconsistencies: >>> >>> * Missing OAM Message Length in the 2nd figure >>> >> GIM>> Great catch, thank you. Fixed. >> >>> * Initially non-zero 'Proto' values are allowed and then it says it >>> needs >>> to be zero. >>> >> GIM>> Figure 1 shows the OAM BIER Header. The document states: >> >> This field MUST be set to 0 if there is no data packet following the OA= M >> payload. >> >> Figure 2 shows BIER Echo Request/Reply with the OAM BIER Header. Because >> Echo Request/Reply is an example of active OAM protocol, the document >> requires that: >> >> Proto field MUST be set to 0 for Echo Request/Reply header. >> >> What would you suggest to make it clearer? >> >>> >>> * You need to describe the handling of the first Reserved field i.e. it >>> MUST be >>> set to zero and ignored on receipt. >>> >> GIM>> Thank you for pointing it out to me. Added the following: >> >> Reserved - a fourteen-bit field. The value MUST be zeroed on >> transmission and ignored on receipt. >> >> >>> * I also suggest stating clearly the size of each of the fields in the >>> text. >>> >> GIM>> I went through the draft making field lengths explicitly >> identified. >> >>> >>> * Think about adding some text about version number change as future >>> guidance. >>> (refer to RFC 4379 for inspiration) >>> >> GIM>> I updated the description of the Version field to the following: >> Ver - a four-bit field indicates the current version of the BIER >> OAM header. The current value is 1. The version number is to be >> incremented whenever a change is made that affects the ability of >> an implementation to parse or process the BIER OAM header >> correctly. For example, if syntactic or semantic changes are made >> to any of the fixed fields. >> I hope that is acceptable. >> >>> >>> * Please add a normative reference for IEEE 1588-2008 (1588v2). >>> >> GIM>> Agreed and done. >> >>> >>> * I am unsure about - "Any other value MAY be considered as sanity chec= k >>> failure"; IMHO document should clearly state which timestamp format it >>> supports, it should have guidance on what the prefered one; it should >>> allow for >>> another format to be added in future, thus differentiating between a >>> junk value >>> v/s unsupported. Should this not have an IANA registry? >>> >> GIM>> At this time, it is unlikely that there will be a new format. Woul= d >> you agree that explicitly identifying two valid values in the specificat= ion >> is a reasonable alternative to creating an IANA sub-registry? >> >>> >>> * The timestamp fields in the figure and text are unclear. Is it to be >>> considered as two 32 bits fields for seconds and microseconds or a sing= le >>> 64-bit field? >>> >> GIM>> Thank you for pointing out this ambiguity in Figure 2. The >> timestamp is a 64-bit field that is interpreted according to the format >> identified in QTF/RTF. I've updated the figure and clarified it in the >> text. I hope it is clearer now. >> >>> >>> * Can QTF and RTF be of different formats? >>> >> GIM>> Yes, timestamp formats used by the Sender and Responder could be >> different. >> >>> >>> ### Section 3.2 >>> >>> * What is the reason for missing value 7? If it is intentional, you can >>> mark it >>> as unassigned if that is the case. >>> >> GIM>> Thank you for the question. Couldn't find the reason. AFAIK, the >> existing implementation uses code points as listed in the draft. I hope >> that it would be acceptable to leave the allocation as-is. Section 5.4.3= . >> BIER Echo Return Codes lists it as Reserved. I agree that Unassigned is = the >> right thing. Done. >> >>> >>> * There is no description for DDMAP mismatch. >>> >> GIM>> Thank you for pointing that out to me. I've added the following: >> NEW TEXT: >> If the BitString in Header-H does not match the BitString in Egress >> BitString Sub-TLV of DDMAP TLV, a responding BFR will use "DDMAP >> Mismatch" to report the problem. >> WDYT? >> >>> >>> ### Section 3.3 >>> >>> * Consider adding some text on the handling of padding (if it can exist= ) >>> and >>> its handling in the length field in the TLV. >>> >> GIM>> That is an interesting idea. The main objective of BIER Ping (we >> can use this common term as a shorter version of Echo Request/Reply) is = to >> monitor the continuity of the path between the BFIR and selected BFER(s)= . >> Also, BIER Ping helps to validate the consistency of information between >> the data plane and control plane in several aspects. These goals can be >> achieved without using packets of variable length. You are right, BIER P= ing >> can be used to detect packet loss and measure packet delays between >> BFIR and BFER. But it is not expected that the measured delay will be >> highly accurate. It seems like another OAM protocol, specifically design= ed >> for performance monitoring, could be a better option. That could be an >> active measurement protocol like STAMP or a hybrid protocol like IOAM or >> the Alternate Marking. >> >>> >>> * Consider having a table for OAM TLV types defined in this document >>> upfront. >>> >> GIM>> Each TLV is described in a separate sub-section and, as a result, >> is explicitly listed in the Table of Contents. Would you suggest listing >> these TLVs in an additional format, e.g., a table, elsewhere in the draf= t? >> >>> >>> * Are multiple instances of TLV allowed? I see text for some but not al= l. >>> >> GIM>> I've added text throughout the document. I hope the new text makes >> the handling of such cases clearer. >> >>> >>> * How to handle unknown TLV? >>> >> GIM>> The Responder is expected to transmit Echo Reply with the Return >> Code set to One or more of the TLVs is not supported (2). >> >>> >>> * Without context it is difficult to appreciate the need for various TL= Vs >>> (Original, Target, Incoming, downstream....); Consider adding some >>> background >>> text. >>> >> GIM>> Thank you for the suggestion. Extended descriptions of the TLVs. >> >>> >>> ### Section 3.3.4 >>> >>> * Add description for MTU. Just MTU is not specific enough. What is MTU >>> in BIER >>> context? >>> >> GIM>> Added text explaining that this is the MTU of the ingress >> interface. >> >>> >>> * Add text for Rsvd - MUST be set to zero when sending and ignored on >>> receipt. >>> (please check this at all places) >>> >> GIM>> Thank you for pointing this out. Should be done. Please let me kno= w >> if I missed any occasion. >> >>> >>> * Why is the Sub-TLV type not being encoded? If we call it a TLV we >>> should >>> encode the T part of the TLV! I suggest both TLV and sub-TLV follow the >>> same >>> format as defined in Section 3.3. Or am I not understanding the meaning >>> of >>> Sub-tlv Length (which anyway is not described in the text)? >>> >> GIM>> Good catch, thank you! I've updated the figures that display the >> formats of two sub-TLVs defined in the draft. Hope that it is clearer no= w. >> >>> >>> * Can both sub-TLVs be carried at the same time? >>> >> GIM>> Yes, that is a valid scenario. >> >>> >>> ### Section 3.3.5 >>> >>> * Please mention the length field value. >>> >> GIM>> In some cases, the Length field value is variable. Updated where >> its value is known a priori. >> >>> >>> ### Section 3.4 >>> >>> * The text "This encoding is similar..." is unclear, is it the same or >>> is there >>> any difference because of entropy? If there are differences that need t= o >>> be >>> stated. >>> >> GIM>> Good question. The encoding is identical to Type 9 defined in RFC >> 8029. Propose the following update: >> OLD TEXT: >> This encoding is similar to the >> Multipath Type 9 encoding is defined in Section 3.4.1.1.1 of >> [RFC8029]. >> NEW TEXT: >> This encoding is the same as the >> Multipath Type 9 encoding, defined in Section 3.4.1.1.1 of [RFC8029]. >> >>> >>> ### Section 4.1 >>> >>> * Consider adding references here for Router Alert, and TTL for >>> BIER-MPLS. >>> >> GIM>> The MPLS WG adopted the proposal to deprecate the Router Alert. >> Added the reference accordingly. >> >>> >>> ### Section 4.2 >>> >>> * In this text - >>> >>> ```` >>> BIER >>> OAM MUST support ECMP path discovery between a BFIR and a given BFER >>> and MUST support path validation and failure detection of any >>> particular ECMP path between BFIR and BFER. >>> ```` >>> >>> Is this stating a requirement for BIER OAM solution to fulfill or are y= ou >>> stating that an implementation MUST support it? If it is a requirement >>> it does >>> not make much sense here! >>> >> GIM>> A good point, thank you. No, that is not a requirement, and I >> propose the following update: >> OLD TEXT: >> BIER >> OAM MUST support ECMP path discovery between a BFIR and a given BFER >> and MUST support path validation and failure detection of any >> particular ECMP path between BFIR and BFER. >> NEW TEXT: >> BIER >> OAM is expected to support ECMP path discovery between a BFIR and a >> given BFER and MUST support path validation and failure detection of >> any particular ECMP path between BFIR and BFER. >> I hope the update helps make the message clear and consistent. >> >>> >>> ### Section 4.3 >>> >>> * Can the Sequence Number wrap? Why SHOULD for increment? >>> >>> ### Section 4.5 >>> >>> * Note that the 'MUST not' is not accepted usage as per RFC 2119. Pleas= e >>> change >>> that to MUST NOT in this paragraph: >> >> >>> ```` >>> If Reply-Flag=3D0, BFR MUST release the variables and MUST not sen= d >>> any response to the Initiator. If Reply-Flag=3D1, proceed as belo= w: >> >> ```` >>> >> GIM>> Thank you for catching this. Fixed. >> >>> >>> ### Section 5 >>> >>> * My suggestion would be to identify the registry. For the UDP Port, it >>> might >>> be useful and explicit to provide the other fields like service-name an= d >>> description as per the registry - >>> >>> https://www.iana.org/assignments/service-names-port-numbers/service-nam= es-port-numbers.xhtml >>> >>> * The allocation policy for the registry is missing. It is left for IAN= A >>> to >>> figure out what are the fields to be maintained in each of the >>> registries. >>> >>> * For section 5.3, please state that this is a new sub-registry and >>> clearly >>> state its name. You state that usage is not limited to OAM, then does i= t >>> belong >>> in "BIER OAM Parameters"? I am also wondering about this mixed registry >>> for >>> type and sub-type with a common value field - not sure if this is bette= r >>> as >>> compared to separate sub-registries for TLV and sub-TLV space. >>> >>> ### Section 6 >>> >>> * I would guess that the surface area for attack is higher because of >>> BIER >>> which should be highlighted. Is that not the case? >>> >>> ## Nits: >>> >>> * Expand BIER in the document title, that is the usual practice in >>> published >>> BIER RFCs >>> >>> * Expand OAM in the Abstract as well. You expand it in the Introduction= . >>> >>> * Your Abstract and Introduction are almost the same. IMHO they serve a >>> different purpose and thus consider having another look. If the >>> intention is to >>> keep them the same then consider making them exactly the same (currentl= y >>> you >>> have this extra text in the Introduction - "without any dependency on >>> other >>> layers like the IP layer."). >>> >>> * Consider adding figure numbers and titles >>> >>> * Section 3.1; Better to be explicit here and say UDP header. >>> >>> ``` >>> When the >>> Reply mode is set to 2, the Responder BFR encapsulates the Echo repl= y >>> payload with the IP header. >>> ``` >>> >>> * Section 3.3.1; for BS Len, RFC 8296 uses the term BSL. Consider being >>> consistent. I also see BitStringLen. >>> >>> * Add a "~" at the start of the field "BitString (last 32 bits)" -= - >>> this >>> issue exists at multiple places. >>> >>> * Multipath encoding is used only in 3.3.4.1.1, why not just move >>> section 3.4 >>> there? >>> >>> * Some of the TLV has "TLV Type=3D" in the figure, please just use Type= =3D >>> for >>> consistency. >>> >>> * Section 4.4; In "... and the BIER header to BIER-Header."; I think yo= u >>> mean >>> Header-H to BIER Header? >>> >>> * I don't think you intend to thank "DIA Length - Downstream Interface >>> Address >>> field Length" in the Acknowledgement :) >>> >>> * Please check the HTML version and you will see various formatting >>> issues with >>> figures, consider fixing them now itself. >>> >>> Thanks! >>> Dhruv >>> >>> --- >>> >>> *In case of bad formatting, refer to this message at - >>> https://notes.ietf.org/draft-ietf-bier-ping?view* >>> >>> >>> _______________________________________________ > BIER mailing list > BIER@ietf.org > https://www.ietf.org/mailman/listinfo/bier > --000000000000d7404105fc162140 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks Greg!=C2=A0
I did a quick br= owse, and it seems to be okay! I have not checked each comment resolution b= ut I trust you to do the right thing!=C2=A0

On Fri, May 19, 2023 at 2:= 37=E2=80=AFPM Greg Mirsky <greg= imirsky@gmail.com> wrote:
Hi Dhruv,
a new version of the draft = (10) includes updates that I proposed in our discussion. Also, it includes = updates addressing IntDir comments. I appreciate it if you could review the= updates (diff between the -08 version and the latest version 10 is attache= d for your convenience). Please let me know if you find your comments reaso= nably addressed.

Best regards,
Greg

On Mon, May 8, 2023 at 9:19=E2=80=AFAM Greg Mirsky <gregimirsky@gmail.com> wrote= :
<= div dir=3D"ltr">Hi=C2=A0Dhruv,
many thanks for your review and comments= . Please find my answers=C2=A0below under the GIM>> tag. Th= e new version of the draft also includes updates addressing comments fr= om Sec and Int areas.
Please let me know=C2=A0if you have any que= stions or additional suggestions.

Kind regards,
Greg

On Fri, Apr 28, 2023 at 3:37=E2=80=AFAM Dhruv Dhody via D= atatracker <norepl= y@ietf.org> wrote:
Reviewer: Dhruv Dhody
Review result: Has Issues

# RTGDIR early review of draft-ietf-bier-ping-08

Hello,

I have been selected to do a routing directorate =E2=80=9Cearly=E2=80=9D re= view of this draft.
https://datatracker.ietf.org/doc/draft-ietf-bi= er-ping/

The routing directorate will, on request from the working group chair, perf= orm
an =E2=80=9Cearly=E2=80=9D review of a draft before it is submitted for pub= lication to the
IESG. The early review can be performed at any time during the draft=E2=80= =99s lifetime
as a working group document. The purpose of the early review depends on the=
stage that the document has reached. As this document is
post-working-group-last-call, my focus for the review was to determine whet= her
the document is ready to be published.

For more information about the Routing Directorate, please see
https://wiki.ietf.org/en/group/rtg/RtgDir

Document: draft-ietf-bier-ping-08
Reviewer: Dhruv Dhody
Review Date: 28 April 2023
Intended Status: Standards Track

## Summary:

I have significant concerns about this document. It needs more work before<= br> being submitted to the IESG.

## Comments:

The need for OAM for BIER is well established, the mechanism is sound but t= here
is a need to tighten up the text (as needed for a standards track document)= and
increase the quality of the WG output that is to be considered ready to be = sent
to the IESG for publication.

The good news is that most of my comments are easy to handle. I wonder why<= br> these were not caught during the normal WG review itself. Interesting to se= e
that the WGLC was initiated way back in 2018.

### General

* Request the shepherd to make sure that there is a valid justification for= 6
authors.

* The document could benefit from an overview of the OAM operation that thi= s
document defines before taking a direct jump into the packet formats and TL= Vs.
GIM>> Thank you for this suggestion. The fol= lowing text is proposed to prepend the last paragraph of the Introduction:<= /div>
NEW TEXT:=C2=A0
=C2=A0 =C2=A0Operations, Administr= ation, and Maintenance (OAM) mechanisms are
=C2=A0 =C2=A0expected= to support the detection of network failures.=C2=A0 After the
= =C2=A0 =C2=A0detection, operators localize and characterize the network def= ect.=C2=A0 A
=C2=A0 =C2=A0query-based tool, e.g., ICMP [RFC0792] = and LSP Ping [RFC8029],
=C2=A0 =C2=A0[RFC6425], is broadly used t= o detect and localize a network defect.
=C2=A0 =C2=A0Additionally= , this mechanism can be used to check the consistency
=C2=A0 =C2= =A0between the data and control planes.

### Section 2.1

* Consider adding QTF, RTF, MTU, DA, DIA
GIM>> D= one=C2=A0

### Section 3 & 3.1

* The text in Section 3 states -

````
=C2=A0 =C2=A0[RFC8296] defines a 4-bit field as "Proto" to identi= fy the payload
=C2=A0 =C2=A0following the BIER header.
````

whereas RFC 8296 says -

````
=C2=A0 =C2=A0Proto:

=C2=A0 =C2=A0 =C2=A0 This 6-bit "Next Protocol" field identifies = the type of the
=C2=A0 =C2=A0 =C2=A0 payload.
````

I also see that the BIER OAM message format defined in this I-D is using 4-bits. Should this not be consistent across BIER documents? - Both number = of
bits and the name!
GIM>> Indeed, excellent sugge= stion. Done.=C2=A0=C2=A0

* The relationship between the first figure and the second needs to be more=
explicit. Moreover, there are the following inconsistencies:

=C2=A0 =C2=A0 * Missing OAM Message Length in the 2nd figure
GIM>> Great catch, thank you. Fixed.=C2=A0
=C2=A0 =C2=A0 * Initially non-zero 'Proto' values are allowed and t= hen it says it needs
=C2=A0 =C2=A0 to be zero.
GIM>> Figure 1 shows t= he OAM BIER Header. The document states:
=C2=A0This field MUST be = set to 0 if there is no data packet following the OAM payload.
<= /div>
Figure 2 shows BIER Echo Request/Reply with the OAM= BIER Header. Because Echo Request/Reply is an example of active OAM protoc= ol, the document requires that:
=C2=A0Proto field MUS= T be set to 0 for Echo Request/Reply header.
What w= ould you suggest to make it clearer?

* You need to describe the handling of the first Reserved field i.e. it MUS= T be
set to zero and ignored on receipt.
GIM>> Thank = you for pointing it out to me. Added the following:
=
=C2=A0Reserved - a fourteen-bit = field. The value MUST be zeroed on transmission and ignored on receipt.

* I also suggest stating clearly the size of each of the fields in the text= .
GIM>> I went through the draft making field le= ngths explicitly identified.=C2=A0

* Think about adding some text about version number change as future guidan= ce.
(refer to RFC 4379 for inspiration)
GIM>> I upda= ted the description of the Version field to the following:
=C2=A0= =C2=A0 =C2=A0 Ver - a four-bit field indicates the current version of the = BIER
=C2=A0 =C2=A0 =C2=A0 OAM header.=C2=A0 The current value is = 1.=C2=A0 The version number is to be
=C2=A0 =C2=A0 =C2=A0 increme= nted whenever a change is made that affects the ability of
=C2=A0= =C2=A0 =C2=A0 an implementation to parse or process the BIER OAM header
=C2=A0 =C2=A0 =C2=A0 correctly.=C2=A0 For example, if syntactic or = semantic changes are made
=C2=A0 =C2=A0 =C2=A0 to any of the fixe= d fields.
I hope that is acceptable.=C2=A0

* Please add a normative reference for IEEE 1588-2008 (1588v2).
GIM>> Agreed and done.=C2=A0

* I am unsure about - "Any other value MAY be considered as sanity che= ck
failure"; IMHO document should clearly state which timestamp format it=
supports, it should have guidance on what the prefered one; it should allow= for
another format to be added in future, thus differentiating between a junk v= alue
v/s unsupported. Should this not have an IANA registry?
GIM>> At this time, it is unlikely that there will be a new format.= Would you agree that explicitly identifying two valid values in the specif= ication is a reasonable alternative to creating an IANA sub-registry?
=

* The timestamp fields in the figure and text are unclear. Is it to be
considered as two 32 bits fields for seconds and microseconds or a single 64-bit field?
GIM>> Thank you for pointing out t= his ambiguity in Figure 2. The timestamp is a 64-bit field that is interpre= ted according to the format identified in QTF/RTF. I've updated the fig= ure and clarified it in the text. I hope it is clearer now.

* Can QTF and RTF be of different formats?
GIM>>= Yes, timestamp formats used by the Sender and Responder could be different= .=C2=A0

### Section 3.2

* What is the reason for missing value 7? If it is intentional, you can mar= k it
as unassigned if that is the case.
GIM>> Thank y= ou for the question. Couldn't find the reason. AFAIK, the existing impl= ementation uses code points as listed in the draft. I hope that it would be= acceptable to leave the allocation as-is. Section 5.4.3.=C2=A0 BIER Echo R= eturn Codes=C2=A0lists it as Reserved. I agree that Unassigned is the right= thing. Done.

* There is no description for DDMAP mismatch.
GIM>&= gt; Thank you for pointing that out to me. I've added the following:
NEW TEXT:
=C2=A0 =C2=A0If the BitString in Header-H does = not match the BitString in Egress
=C2=A0 =C2=A0BitString Sub-TLV = of DDMAP TLV, a responding BFR will use "DDMAP
=C2=A0 =C2=A0= Mismatch" to report the problem.
WDYT?

### Section 3.3

* Consider adding some text on the handling of padding (if it can exist) an= d
its handling in the length field in the TLV.
GIM>&g= t; That is an interesting idea. The main objective of BIER Ping (we can use= this common term as a shorter version of Echo Request/Reply) is to monitor= the continuity of the path between the BFIR=C2=A0and selected BFER(s). Als= o, BIER Ping helps to validate the consistency of information between the d= ata plane and control plane in several aspects. These goals can be achieved= without using packets of variable length. You are right, BIER Ping can be = used to detect packet loss and measure packet delays between BFIR=C2=A0and = BFER. But it is not expected that the measured delay will be highly accurat= e. It seems like another OAM protocol, specifically designed for performanc= e monitoring, could be a better option. That could be an active measurement= protocol like STAMP or a hybrid protocol like IOAM or the Alternate Markin= g.

* Consider having a table for OAM TLV types defined in this document upfron= t.
GIM>> Each TLV is described in a separate sub= -section and, as a result, is explicitly listed in the Table of Contents. W= ould you suggest listing these TLVs in an additional format, e.g., a table,= elsewhere in the draft?=C2=A0

* Are multiple instances of TLV allowed? I see text for some but not all.
GIM>> I've added text throughout the document= . I hope the new text makes the handling of such cases clearer.=C2=A0
=

* How to handle unknown TLV?
GIM>> The Responder= is expected to transmit Echo Reply with the Return Code set to=C2=A0One or= more of the TLVs is not supported (2).=C2=A0

* Without context it is difficult to appreciate the need for various TLVs (Original, Target, Incoming, downstream....); Consider adding some backgrou= nd
text.
GIM>> Thank you for the suggestion. Extend= ed descriptions of the TLVs.=C2=A0

### Section 3.3.4

* Add description for MTU. Just MTU is not specific enough. What is MTU in = BIER
context?
GIM>> Added text explaining that this i= s the MTU of the ingress interface.=C2=A0

* Add text for Rsvd - MUST be set to zero when sending and ignored on recei= pt.
(please check this at all places)
GIM>> Thank yo= u for pointing this out. Should be done. Please let me know if I missed any= occasion.=C2=A0

* Why is the Sub-TLV type not being encoded? If we call it a TLV we should<= br> encode the T part of the TLV! I suggest both TLV and sub-TLV follow the sam= e
format as defined in Section 3.3. Or am I not understanding the meaning of<= br> Sub-tlv Length (which anyway is not described in the text)?
GIM>> Good catch, thank you! I've updated the figures that = display the formats of two sub-TLVs defined in the draft. Hope that it is c= learer now.=C2=A0

* Can both sub-TLVs be carried at the same time?
GIM&g= t;> Yes, that is a valid scenario.=C2=A0

### Section 3.3.5

* Please mention the length field value.
GIM>> I= n some cases, the Length field value is variable. Updated where its value i= s known a priori.=C2=A0

### Section 3.4

* The text "This encoding is similar..." is unclear, is it the sa= me or is there
any difference because of entropy? If there are differences that need to be=
stated.
GIM>> Good question. The encoding is ide= ntical to Type 9 defined in RFC 8029. Propose the following update:
OLD TEXT:
=C2=A0 =C2=A0This encoding is similar to the
=C2=A0 =C2=A0Multipath Type 9 encoding is defined in Section 3.4.1.1.1 o= f
=C2=A0 =C2=A0[RFC8029].=C2=A0
NEW TEXT:
=C2=A0 =C2=A0This encoding is the same as the
=C2=A0 =C2=A0Mult= ipath Type 9 encoding, defined in Section 3.4.1.1.1 of [RFC8029].

### Section 4.1

* Consider adding references here for Router Alert, and TTL for BIER-MPLS.<= br>
GIM>> The MPLS WG adopted the proposal to deprec= ate the Router Alert. Added the reference accordingly.=C2=A0

### Section 4.2

* In this text -

````
=C2=A0 =C2=A0BIER
=C2=A0 =C2=A0OAM MUST support ECMP path discovery between a BFIR and a give= n BFER
=C2=A0 =C2=A0and MUST support path validation and failure detection of any<= br> =C2=A0 =C2=A0particular ECMP path between BFIR and BFER.
````

Is this stating a requirement for BIER OAM solution to fulfill or are you stating that an implementation MUST support it? If it is a requirement it d= oes
not make much sense here!
GIM>> A good point, th= ank you. No, that is not a requirement, and I propose the following update:=
OLD TEXT:
=C2=A0 =C2=A0BIER
=C2=A0 =C2=A0OAM= MUST support ECMP path discovery between a BFIR and a given BFER
=C2=A0 =C2=A0and MUST support path validation and failure detection of any=
=C2=A0 =C2=A0particular ECMP path between BFIR and BFER.
NEW TEXT:
=C2=A0 =C2=A0BIER
=C2=A0 =C2=A0OAM is= expected to support ECMP path discovery between a BFIR and a
=C2= =A0 =C2=A0given BFER and MUST support path validation and failure detection= of
=C2=A0 =C2=A0any particular ECMP path between BFIR and BFER.<= /div>
I hope the update helps make the message clear and consiste= nt.

### Section 4.3

* Can the Sequence Number wrap? Why SHOULD for increment?

### Section 4.5

* Note that the 'MUST not' is not accepted usage as per RFC 2119. P= lease change
that to MUST NOT in this paragraph:

````
=C2=A0 =C2=A0 =C2=A0If Reply-Flag=3D0, BFR MUST release the variables and M= UST not send
=C2=A0 =C2=A0 =C2=A0any response to the Initiator.=C2=A0 If Reply-Flag=3D1,= proceed as below:=C2=A0
````
GIM>> Thank you for catching this. Fixed.= =C2=A0

### Section 5

* My suggestion would be to identify the registry. For the UDP Port, it mig= ht
be useful and explicit to provide the other fields like service-name and description as per the registry -
https://= www.iana.org/assignments/service-names-port-numbers/service-names-port-numb= ers.xhtml

* The allocation policy for the registry is missing. It is left for IANA to=
figure out what are the fields to be maintained in each of the registries.<= br>
* For section 5.3, please state that this is a new sub-registry and clearly=
state its name. You state that usage is not limited to OAM, then does it be= long
in "BIER OAM Parameters"? I am also wondering about this mixed re= gistry for
type and sub-type with a common value field - not sure if this is better as=
compared to separate sub-registries for TLV and sub-TLV space.

### Section 6

* I would guess that the surface area for attack is higher because of BIER<= br> which should be highlighted. Is that not the case?

## Nits:

* Expand BIER in the document title, that is the usual practice in publishe= d
BIER RFCs

* Expand OAM in the Abstract as well. You expand it in the Introduction.
* Your Abstract and Introduction are almost the same. IMHO they serve a
different purpose and thus consider having another look. If the intention i= s to
keep them the same then consider making them exactly the same (currently yo= u
have this extra text in the Introduction - "without any dependency on = other
layers like the IP layer.").

* Consider adding figure numbers and titles

* Section 3.1; Better to be explicit here and say UDP header.

```
=C2=A0 =C2=A0When the
=C2=A0 =C2=A0Reply mode is set to 2, the Responder BFR encapsulates the Ech= o reply
=C2=A0 =C2=A0payload with the IP header.
```

* Section 3.3.1; for BS Len, RFC 8296 uses the term BSL. Consider being
consistent. I also see BitStringLen.

=C2=A0 =C2=A0 * Add a "~" at the start of the field "BitStri= ng=C2=A0 (last 32 bits)" -- this
=C2=A0 =C2=A0 issue exists at multiple places.

* Multipath encoding is used only in 3.3.4.1.1, why not just move section 3= .4
there?

* Some of the TLV has "TLV Type=3D" in the figure, please just us= e Type=3D for
consistency.

* Section 4.4; In "... and the BIER header to BIER-Header."; I th= ink you mean
Header-H to BIER Header?

* I don't think you intend to thank "DIA Length - Downstream Inter= face Address
field Length" in the Acknowledgement :)

* Please check the HTML version and you will see various formatting issues = with
figures, consider fixing them now itself.

Thanks!
Dhruv

---

*In case of bad formatting, refer to this message at -
https://notes.ietf.org/draft-ietf-bier-ping?view*


_______________________________________________
BIER mailing list
BIER@ietf.org
https://www.ietf.org/mailman/listinfo/bier
--000000000000d7404105fc162140--