Re: [Gen-art] Genart last call review of draft-ietf-grow-bmp-local-rib-10

["Tim Evens (tievens)" <[e-mail]>]

Hi Thomas,

Thank you for your review and catching these nits.  Please see my comments marked [tievens] inline.
You can see my pending PR (https://github.com/TimEvens/draft-ietf-grow-bmp-loc-rib/pull/22) for all these changes.  Once approved, I'll submit revision 11.


On 3/20/21, 3:33 AM, "Thomas Fossati via Datatracker" <[e-mail]> wrote:


Reviewer: Thomas Fossati
Review result: Ready with Issues

Nits:

* Figure 1:
  * a couple of '+' are missing in the top-right and bottom-right
    corners of the Adj-RIB-In (Post) box;

[tievens] Updated.

* "e.g." => "e.g.," in multiple places;
[tievens] Updated.


* A few acronyms are not expanded:
    * IGP, BGP-LS, SPF, CSPF. VRF, ASN, BGP-ID, RD (route
      distinguisher?)


[tievens] I've updated these abbreviations following https://tools.ietf.org/html/rfc7322#section-3.6.

I suppose the statement  "The exception is an abbreviation that is so common that the readership of RFCs can be
   expected to recognize it immediately" is a bit subjective.


* Section 1.1
    * "directly effects" => "directly affects"

[tievens] Updated.

* Section 3
  * "an instance of an instance of BGP-4" => "an instance of BGP-4"
[tievens] Updated.

* Section 5
  * "post-policy" => "Post-Policy"
  * "ie." => "i.e.,"

[tievens] Updated.

* Section 5.1
  * "in The Loc-RIB, expressed" => "in the Loc-RIB, expressed"

[tievens] Updated.

* Section 5.2.1
  * Consider using "Peer Up" instead of "Peer UP" for consistency with
    the capitalisation use in RFC7854 (also in Sections 5.3, 5.4.1, 6.1,
    6.1.1, 6.1.3, and 8.3)

[tievens] Updated to Peer Up.


* Section 5.3
  * "peer Down" => "Peer Down"

[tievens] Updated.

* Section 6.1
  * "local router emulated peer." maybe "locally emulated peer."

[tievens] Updated.

* Section 6.1.1
  * "since it represents the same Loc-RIB instance" => "since they
    represents the same Loc-RIB instance"

[tievens] Updated.

Editorial improvements:

* Section 1.1
    * I had some troubles parsing: "Complexities introduced by the lack
      of access to Loc-RIB in order to derive (e.g. correlate) peer to
      router Loc-RIB:", in particular the bit "in order to derive (e.g.
      correlate)".  Is it "in order to derive (i.e., correlate)" or "in
      order to derive (or correlate)" or "in order to correlate"?

[tievens] How about changing it to: "Complexities introduced when correlating a received Adj-RIB-In as a router Loc-RIB:"

    * What does "suppresses more specifics" mean?  Is there a term
      missing?
[tievens] I've updated it to: "more specific prefixes"

    * What does "derive a Loc-RIB to a router" mean? Is it "derive the
      Loc-RIB of a router" instead?
[tievens] Yes. Updated.

    * I find this "The BGP-IDs and session addresses to router
      correlation requires additional data" a bit hard to parse. Maybe
      re-flow it as: "Correlating BGP-IDs and session addresses to a
      router requires additional data"

[tievens]  Updated to use your re-flow…

* Section 4.1
  * I find "to distinguish that it represents Loc-RIB with or without RD
    and local instances" a bit hard to parse.  I suggest rephrasing it
    to make it clearer.

[tievens]  Changed to: "A new peer type is defined for Loc-RIB to distinguish that it represents the router Loc-RIB, which may have a route distinguisher (RD)."

* Section 5
  * Re: setting the F flag.  It'd help if you put a forward ref to
    Section 6.1.2 here.  (Before getting to 6.1.2 I got baffled by F; in
    particular, it was not clear to me from the surrounding text what is
    the monitoring station supposed to do with partial information
    without knowing exactly how much and what kind of info has been
    left out.)

[tievens] Updated. Slight rewording: "As described in Section 6.1.2, a subset of Loc-RIB routes MAY be sent to a BMP collector by setting the F flag."

* Section 5.2
  * Should add-paths be ADD-PATH instead?  If so, maybe you could also
    add an informative reference to RFC7911

[tievens] Updated to ADD-PATH with reference.

  * In "The duplication allows the BMP receiver to use existing parsing"
    could you clarify what "existing parsing" mean?

[tievens] Updated to: "Repeat of the same Sent Open Message.  The duplication allows the BMP receiver to parse the expected received OPEN message as defined in section 4.10 of [RFC7854]."

* Section 5.5
  * Why would the receiver decide not to ignore a Route Mirror message?
    And what would happen if it decided so?  I'm asking because I don't
    understand the reasons for a SHOULD rather than a MUST here.

[tievens] Updated to clarify.  "Section 4.7 of [RFC7854] defines Route Mirroring for verbatim duplication of messages received.  This is not applicable to Loc-RIB considering PDUs are originated by the router.  Any received Route Mirroring messages SHOULD be ignored."
* Section 6.1.1
  * In "There MUST be multiple emulated peers for each Loc-RIB instance"
    I am unsure whether what you want to say is that "there MUST be at
    least one emulated peer for each Loc-RIB instance" (which is what I
    thought) or that "each Loc-RIB instance *always* has multiple
    emulated peers" (which the current text seems to say)?

[tievens] Changed to "There MUST be at least one emulated peer for each Loc-RIB instance"

* Section 8.2
  * It is not clear to me if saying "and proposes that peer flags are
    specific to the peer type" you are asking IANA to modify the
    contents and/or structure of the BMP Peer Flags registry?  If so,
    the request to IANA should be made more explicit.

[tievens] I have updated it to: "This document defines that peer flags are specific to the Loc-RIB instance peer type.  As defined in (Section 4.2)".  We do have a problem as IANA did add this flag under the general table as flag/bit 4, which is incorrect.  The flag is F in position 0, which would be documented as flag 0.   The draft requests that the peer flags be unique by peer type.   IANA would need to create a new table based on the peer type of 3.

* Section 8.3
  * Should "informational message TLV types" be "Initiation Message TLV
    type" instead?

[tievens]  No.  Considering the original draft RFC7854 defines some of the information TLV messages to be specific by type, it required IANA to create more than one table for informational TLVs as defined in section 4.4 of RFC7854.  RFC7854 defines that Peer UP does have information TLV String, but fails to identify the type number and does not request from IANA to define message TLVs for Peer Up.  Instead, RFC7854 does state in Section 4.4 that the defined info TLVs are used by both INIT and Peer Up messages, but it doesn't make that clear in the IANA request.  I'm thinking that IANA can update the title to indicate "Initiation and Peer Up" as they use the same registry/table.  RFC8671 also added a new type, which is documented under that table.