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

Greg Mirsky <gregimirsky@gmail.com> Fri, 19 May 2023 19:37 UTC

Return-Path: <gregimirsky@gmail.com>
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 BF661C151069; Fri, 19 May 2023 12:37:18 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 0.408
X-Spam-Level:
X-Spam-Status: No, score=0.408 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_COMMENT_SAVED_URL=1.391, HTML_MESSAGE=0.001, HTML_TAG_BALANCE_BODY=0.1, MANY_SPAN_IN_TEXT=1, RCVD_IN_DNSWL_BLOCKED=0.001, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_HTML_ATTACH=0.01, URIBL_BLOCKED=0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=0.001] autolearn=no 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 yjdfjSaIrKjJ; Fri, 19 May 2023 12:37:14 -0700 (PDT)
Received: from mail-yw1-x1130.google.com (mail-yw1-x1130.google.com [IPv6:2607:f8b0:4864:20::1130]) (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 1EDFDC14F738; Fri, 19 May 2023 12:37:14 -0700 (PDT)
Received: by mail-yw1-x1130.google.com with SMTP id 00721157ae682-562108900acso25403297b3.2; Fri, 19 May 2023 12:37:14 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684525033; x=1687117033; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ikMWmmuTSxONm0ijJnNl5mUm4RhzoY1X55zIqnVgrT8=; b=quCiBsPO6/quUF9f5Th41u7S9p7xpJpvbXWBOnjGer3BL2aMpBUDG46dhUvlyOeMxI 5TNMkZumzPEdfxMlRNFUR6bgEJsM2kSID4kqDcWM5lBICoXzpNQHKl8hEyDViF25wh63 I3erwYsdXOU5gF/hdGBtz0gVFvaZpZDlwYXVkL4SNoHy8NcA5ewKxrE3gekx1Nxk5ynv ohi0bPrSmZCLGzLxrdiY9xpeaqRtnK/b1Ja9P5hQbQRRsdVxQ2FStmdL/VRQgCSgsrG8 XZb1whFLZrk7pCuN/APj1voM7mroAvgT9fJpRIT21ybPi4E+MW5aFuB62CR9qhvXEZnL 6NNg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684525033; x=1687117033; 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=ikMWmmuTSxONm0ijJnNl5mUm4RhzoY1X55zIqnVgrT8=; b=cAuM6N069UPqfUzJJr6bIPuqxzzvqnjzcHcEzPgSehnc8xWampaXvFlMNnILpqRd+D TmAsB1W2NQOg6pfCQ8dqW4c7mpp9H17Un7GWHL8spdohT5TTrWYLNMg5/QE/aBRi3UNj LGnroiNM0LmndrhrQjp7NZXhWfEb0W3bQYHN/OiqQmBJYV+ahoEiYtzSmeEMECH2eh0F +L2vNaOd7AWSCMGJy+E3ZqDtPu/FERjEYp8EVw5h/AIwLBN9S3UYXE160IaTRdLp5Z0j roB52XB5L7sVmj7YRpZbdl/KUdFaAw8B+AP5pVQ7RcICb+0+c6EnfJAiiuR+bzR5wnEQ nV2Q==
X-Gm-Message-State: AC+VfDxAtQ4BWjKjhjoYB/m5LqEJqavsWGXhEU+nBkLZXTbIgGl+Qwxp K+sxsPCCJNJgb7EEr1y6i7EbouYevjR07pkB+xHefrpK
X-Google-Smtp-Source: ACHHUZ7dSuSXjH2IwPdxiNhQJ+fL99dTkhv1OpdN2irsVQXzyMjPUDqkG1x29mw7hspL+gJdi47ojlifiEGpIb22X3k=
X-Received: by 2002:a0d:ea0b:0:b0:55d:c674:c7e2 with SMTP id t11-20020a0dea0b000000b0055dc674c7e2mr3566983ywe.41.1684525032535; Fri, 19 May 2023 12:37:12 -0700 (PDT)
MIME-Version: 1.0
References: <168267823708.49129.2910500144998874882@ietfa.amsl.com> <CA+RyBmWdGd3-2Jg6TeyigF9DzSaH8hbh59Wd-mHEM37kuOdZWA@mail.gmail.com>
In-Reply-To: <CA+RyBmWdGd3-2Jg6TeyigF9DzSaH8hbh59Wd-mHEM37kuOdZWA@mail.gmail.com>
From: Greg Mirsky <gregimirsky@gmail.com>
Date: Fri, 19 May 2023 12:37:01 -0700
Message-ID: <CA+RyBmW=LZ=TEzqxZjh4gvJis7g3SAqK=oV=SB_i8wqMUaXZWA@mail.gmail.com>
To: Dhruv Dhody <dd@dhruvdhody.com>
Cc: Routing Directorate <rtg-dir@ietf.org>, BIER WG <bier@ietf.org>, draft-ietf-bier-ping.all@ietf.org
Content-Type: multipart/mixed; boundary="0000000000002e55a805fc110ca1"
Archived-At: <https://mailarchive.ietf.org/arch/msg/rtg-dir/2e06UnG4pLREzQ51Xgputt9e8Kg>
Subject: Re: [RTG-DIR] 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 <rtg-dir.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/rtg-dir>, <mailto:rtg-dir-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/rtg-dir/>
List-Post: <mailto:rtg-dir@ietf.org>
List-Help: <mailto:rtg-dir-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/rtg-dir>, <mailto:rtg-dir-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 19 May 2023 19:37:18 -0000

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 <gregimirsky@gmail.com> 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 <
> 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 “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*
>>
>>
>>