Re: [RTG-DIR] [Bier] Rtgdir early review of draft-ietf-bier-ping-08

[Dhruv Dhody <[e-mail]>]

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 PM Greg Mirsky <[e-mail]> 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 attached for your convenience). Please let me
> know if you find your comments reasonably addressed.
>
> Best regards,
> Greg
>
> On Mon, May 8, 2023 at 9:19 AM Greg Mirsky <[e-mail]> 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
>> <https://datatracker.ietf.org/doc/draft-ietf-bier-ping/>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 AM Dhruv Dhody via Datatracker <
>> [e-mail]> 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 “early” 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 “early” 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’s
>>> 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 to
>>> 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 and
>>> 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 using
>>> 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 OAM
>> 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 check
>>> 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. Would
>> you agree that explicitly identifying two valid values in the specification
>> 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 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 Ping
>> 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 designed
>> 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 draft?
>>
>>>
>>> * 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.
>>
>>>
>>> * 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 TLVs
>>> (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 know
>> 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 now.
>>
>>>
>>> * 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 to
>>> 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 you
>>> 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. Please
>>> change
>>> that to MUST NOT in this paragraph:
>>
>>
>>> ````
>>>      If Reply-Flag=0, BFR MUST release the variables and MUST not send
>>>      any response to the Initiator.  If Reply-Flag=1, proceed as below:
>>
>> ````
>>>
>> 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 and
>>> description as per the registry -
>>>
>>> https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.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.
>>>
>>> * 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
>>> 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 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
>>> 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 (currently
>>> 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 reply
>>>    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=" in the figure, please just use Type=
>>> for
>>> consistency.
>>>
>>> * Section 4.4; In "... and the BIER header to BIER-Header."; I think you
>>> 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
> [e-mail]
> https://www.ietf.org/mailman/listinfo/bier
>