Re: [Tsv-art] Tsvart last call review of draft-ietf-mpls-bfd-directed-27

[Greg Mirsky <[e-mail]>]

Hi Lars,
thank you for your review, detailed questions, and helpful suggestions.
Please find my notes below tagged GIM>>. Attached, please find the updated
working version of the draft.

Regards,
Greg

On Mon, Apr 15, 2024 at 12:38 PM Lars Eggert via Datatracker <
[e-mail]> wrote:

> Reviewer: Lars Eggert
> Review result: On the Right Track
>
> # tsvart review of draft-ietf-mpls-bfd-directed-27
>
> CC @larseggert
>
> This document has been reviewed as part of the transport area review
> team's ongoing effort to review key IETF documents. These comments were
> written primarily for the transport area directors, but are copied to the
> document's authors and WG to allow them to address any issues raised and
> also to the IETF discussion list for information.
>
> When done at the time of IETF Last Call, the authors should consider this
> review as part of the last-call comments they receive. Please always CC
> [e-mail] if you reply to or forward this review.
>
> ## Comments
>
> ### Section 3.1, paragraph 7
> ```
>      Reverse Path field contains none, one or more sub-TLVs.  Any non-
>      multicast Target FEC Stack sub-TLV (already defined, or to be defined
>      in the future) for TLV Types 1, 16, and 21 of MPLS LSP Ping
>      Parameters registry MAY be used in this field.  Multicast Target FEC
> ```
> I think you mean "no other sub-TLV than X, Y, Z MUST be used"?(The MAY
> makes anything allowed.)
>
GIM>> I think that your suggestion is close but could the new wording be
interpreted that some of sub-TLV MUST be present? Would the following
update make the use of the normative language clear:
OLD TEXT:
   Reverse Path field contains none, one or more sub-TLVs.  Any non-
   multicast Target FEC Stack sub-TLV (already defined, or to be defined
   in the future) for TLV Types 1, 16, and 21 of MPLS LSP Ping
   Parameters registry MAY be used in this field.
NEW TEXT:
   Reverse Path field MAY contain none, one, or more sub-TLVs.  Only
   non-multicast  Target FEC Stack- sub-TLVs (already defined, or to be
   defined in the future) for  TLV Types 1, 16, and 21 of MPLS LSP Ping
   Parameters registry MUST be used  in this field.

WDYT?


> ### Section 3.1, paragraph 6
> ```
>      MAY be included in the BFD Reverse Path TLV.  However, the number of
>      sub-TLVs in the Reverse Path field MUST be limited.  The default
>      limit is 128, but an implementation MAY be able to control that
> ```
> Why must it be limited? And what unit is the default of 128 expressed
> in, bytes (for the "length" field)? Or number of entries?
>
GIM>> Yes, the concern is for the number of entries. That is the result of
addressing the comments by Andrew Allston
<https://mailarchive.ietf.org/arch/browse/mpls/?gbt=1&index=Z9l-u8LtZS4NJF1KtawvcXBDFJQ>.
As Andrew explained, the concern is not for the size of the TLV but about
the possible impact on the control plane (sort of DoS attack).

>
> ### Section 4, paragraph 0
> ```
>   4.  Use Case Scenario
> ```
> It would be more helpful if this example came before Section 3.
>
GIM>> If WG Chairs, the Shepherd, and the Responsible AD agree that that
change would be considered editorial, not the technical update that
requires an additional agreement/confirmatiom by MPLS WG, I could move
Section 4 as you've suggested.

>
> ### Inclusive language
>
> Found terminology that should be reviewed for inclusivity; see
> https://www.rfc-editor.org/part2/#inclusive_language for background and
> more
> guidance:
>
>  * Term `traditional`; alternatives might be `classic`, `classical`,
> `common`,
>    `conventional`, `customary`, `fixed`, `habitual`, `historic`,
>    `long-established`, `popular`, `prescribed`, `regular`, `rooted`,
>    `time-honored`, `universal`, `widely used`, `widespread`
>
GIM>> Although the Implementation Status section to be removed by  the RFC
Editor, updated as s/traditional/commonly used/. Would that be acceptable?

>
> ## Nits
>
> All comments below are about very minor potential issues that you may
> choose to
> address in some way - or ignore - as you see fit. Some were flagged by
> automated tools (via https://github.com/larseggert/ietf-reviewtool), so
> there
> will likely be some false positives. There is no need to let me know what
> you
> did with these suggestions.
>
> ### Typos
>
> #### "Abstract", paragraph 1
> ```
> -    there may be a need to direct egress BFD peer to use a specific path
> +    there may be a need to direct the egress BFD peer to use a specific
> path
>
GIM>> Done

> +                                 ++++
> -    request that allows a BFD system requests that the remote BFD peer
> -                                            -
> -    transmits BFD control packets over the specified LSP.
> -            -
> +    request that allows a BFD system to request that the remote BFD peer
> +                                     +++
> ```
> The rest of the doc has a bunch more grammar nits like this which make
> following the content unnecessarily difficult. Please proofread.
>
> ### Section 2, paragraph 2
> ```
>      *  detection by an ingress node of a failure on the reverse path may
>         not be unambiguously interpreted as the failure of the path in the
>         forward direction.
> ```
> A bulleted list with a single bullet is a bit pointless?
>
> ### Section 3.1, paragraph 6
> ```
>      If the egress LSR cannot find the path specified in the Reverse Path
>      TLV it MUST send Echo Reply with the received BFD Discriminator TLV,
>      Reverse Path TLV and set the Return Code to "Failed to establish the
>      BFD session.  The specified reverse path was not found" Section 3.2.
>
GIM>> Thank you for pointing that out to me. That must be a forward
reference. Would enclosing Section 3.2 in parentheses help?

> ```
> What about Section 3.2?
>
> ### Section 3.1, paragraph 6
> ```
>      The BFD Reverse Path TLV MAY be used in the bootstrapping of a BFD
>      session process described in Section 6 [RFC5884].  A system that
> ```
> Section 6 *of* RFC5884?
>
GIM>> Thank you. Done. Also, I fixed several other similar occurences.

>
> ## Notes
>
> This review is in the ["IETF Comments" Markdown format][ICMF], You can use
> the
> [`ietf-comments` tool][ICT] to automatically convert this review into
> individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].
>
> [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
> [ICT]: https://github.com/mnot/ietf-comments
> [IRT]: https://github.com/larseggert/ietf-reviewtool
>
>
>