Skip to main content

Last Call Review of draft-ietf-idr-bgp-ls-sbfd-extensions-07
review-ietf-idr-bgp-ls-sbfd-extensions-07-genart-lc-fossati-2022-04-16-00

Request Review of draft-ietf-idr-bgp-ls-sbfd-extensions
Requested revision No specific revision (document currently at 10)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2022-04-27
Requested 2022-04-12
Authors Zhenbin Li , Shunwan Zhuang , Ketan Talaulikar , Sam Aldrin , Jeff Tantsura , Greg Mirsky
Draft last updated 2022-04-16
Completed reviews Rtgdir Last Call review of -07 by Ron Bonica (diff)
Genart Last Call review of -07 by Thomas Fossati (diff)
Opsdir Last Call review of -07 by Dan Romascanu (diff)
Assignment Reviewer Thomas Fossati
State Completed
Review review-ietf-idr-bgp-ls-sbfd-extensions-07-genart-lc-fossati-2022-04-16
Posted at https://mailarchive.ietf.org/arch/msg/gen-art/z7foYt5MX582UENCXftzpZCyd6g
Reviewed revision 07 (document currently at 10)
Result Ready
Completed 2022-04-16
review-ietf-idr-bgp-ls-sbfd-extensions-07-genart-lc-fossati-2022-04-16-00
I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-idr-bgp-ls-sbfd-extensions-??
Reviewer: Thomas Fossati
Review Date: 2022-04-16
IETF LC End Date: 2022-04-27
IESG Telechat date: Not scheduled for a telechat

This document is compact and well written.  I think it is ready for
publication.  Thank you editors, and all the people involved in its
drafting.  I have left a few small editorial suggestions (caveat: I am
completely unfamiliar with the subject matter) in case you may find them
useful.

## Section 3, first para:

OLD
   The BGP-LS [RFC7752] specifies the Node NLRI for the advertisement of
   nodes and their attributes using the BGP-LS Attribute. 

NEW
   BGP-LS [RFC7752] specifies the Node NLRI for the advertisement of
   nodes and their attributes using the BGP-LS Attribute. 

## Section 3, second para:

I had to jump a couple of times between the textual definition and the
TLV layout to understand how the two actually match.  Specifically, I
think it's the "Length: variable." bit that triggered my confusion.

So, I have an editorial suggestion that would have saved me from a
(very) mild cognitive dissonance:

OLD
      0                   1                   2                   3
      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |              Type             |             Length            |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                         Discriminator 1                       |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                    Discriminator 2 (Optional)                 |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                               ...                             |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                    Discriminator n (Optional)                 |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

               Figure 1: S-BFD Discriminators TLV

     where:

   o  Type: 1032 (early allocation by IANA)

   o  Length: variable.  It MUST be a minimum of 4 octets and increments
      of 4 octets for each additional discriminator.

   o  Discriminator n: 4 octets each, carrying an S-BFD local
      discriminator value of the node.  At least one discriminator MUST
      be included in the TLV.

NEW
      0                   1                   2                   3
      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |              Type             |          Length=4*n           |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                         Discriminator 1                       |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                    Discriminator 2 (Optional)                 |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                               ...                             |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |                    Discriminator n (Optional)                 |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

   where Type is 1032, Length is 4*n, and n is the number of S-BFD local
   discriminator values of the node carried in the TLV.  n MUST be at least
   1.

## Section 3, ignorant hat on:

I am not sure what is the typical number of discriminators expected in a
TLV?  It looks to me that ~65K/4 is a decent magnitude.  What is the
expectation in case the number of discriminators is higher than the
maximum allowed by the TLV encoding?  Is this a valid scenario?  If so,
how is overflow handled?

## Section 4, Table 1:

The structure doesn't seem to match the layout of the "BGP-LS Node
Descriptor, etc." IANA registry.  In particular:
* The length field seems spurious?
* The IS-IS TLV/Sub-TLV is missing — but I guess this can be skipped, so
  not a problem;
* The first column heading should be "TLV Code Point";
* The Reference column is missing.

OLD
         +---------------+--------------------------+----------+
         |  Code Point   | Description              | Length   |
         +---------------+--------------------------+----------+
         |     1032      | S-BFD Discriminators TLV | variable |
         +---------------+--------------------------+----------+

NEW
         +----------------+--------------------------+-----------+
         | TLV Code Point | Description              | Reference |
         +----------------+--------------------------+-----------+
         |      1032      | S-BFD Discriminators TLV |  RFCthis  |
         +----------------+--------------------------+-----------+

## Section 5, first sentence:

OLD
   The new protocol extensions introduced in this document augment the
   existing IGP topology information that was distributed via [RFC7752].

NEW
   The new protocol extensions introduced in this document augment the
   existing IGP topology information that was distributed via BGP-LS
   [RFC7752].

## Section 6, first para:

OLD
   The new protocol extensions introduced in this document augment the
   existing IGP topology information that can be distributed via
   [RFC7752].

NEW
   The new protocol extensions introduced in this document augment the
   existing IGP topology information that can be distributed via BGP-LS
   [RFC7752].

# Section 5, third sentence:

Typo: "now encompasses" should be "now encompass"