[GROW] Genart last call review of draft-ietf-grow-bmp-local-rib-10

[Thomas Fossati via Datatracker <[e-mail]>]

Reviewer: Thomas Fossati
Review result: Ready with Issues

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-grow-bmp-local-rib-??
Reviewer: Thomas Fossati
Review Date: 2021-03-20
IETF LC End Date: 2021-03-29
IESG Telechat date: Not scheduled for a telechat

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
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

This draft is on the right track but has open issues, described in the
review.

Some high level notes:

* A nice set of use cases that provide compelling reasons for this BMP
  extension are presented clearly and concisely at the beginning of the
  document.

* It seems to me that this extension is in line with the original
  design goals of the protocol (i.e., simplicity, usefulness,
  ease of implementation, minimally service affecting).

* This document updates BMP by providing a complete replacement
  for the monitoring of routes originated into BGP by the local router.
  This is made clear by the "update 7854" label, as well as the finer
  grained reference found in Section 1, which points to Section 8.2 of
  RFC7854.

* Overall the document is very readable but there are a few places that
  need some editorial polishing to eliminate possible ambiguities.

Nits:

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

* "e.g." => "e.g.," in multiple places;

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

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

* Section 3
  * "an instance of an instance of BGP-4" => "an instance of BGP-4"

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

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

* 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)

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

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

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

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"?

    * What does "suppresses more specifics" mean?  Is there a term
      missing?

    * What does "derive a Loc-RIB to a router" mean? Is it "derive the
      Loc-RIB of a router" instead?

    * 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"

* 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.

* 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.)

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

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

* 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.

* 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)?

* 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.

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