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

From: Thomas Fossati via Datatracker <[email-masked]>
Date: Sat, 20 Mar 2021 03:33:21 -0700

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?

---

From: "Tim Evens (tievens)" <[email-masked]>
Date: Tue, 30 Mar 2021 03:22:43 +0000

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" <[email-masked]> wrote:

Reviewer: Thomas Fossati

Nits:

* Figure 1:

[tievens] Updated.

[tievens] Updated.

[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

[tievens] Updated.

* Section 3
[tievens] Updated.

* Section 5

[tievens] Updated.

* Section 5.1

[tievens] Updated.

* Section 5.2.1

[tievens] Updated to Peer Up.

* Section 5.3

[tievens] Updated.

* Section 6.1

[tievens] Updated.

* Section 6.1.1

[tievens] Updated.

Editorial improvements:

* Section 1.1

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

[tievens] Yes. Updated.

[tievens]  Updated to use your re-flow…

* Section 4.1

[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

[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

[tievens] Updated to ADD-PATH with reference.

[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

[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

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

* Section 8.2

[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

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

---

From: Thomas Fossati <[email-masked]>
Date: Tue, 30 Mar 2021 13:21:08 +0000

Hi Tim,

On 30/03/2021, 04:22, "Tim Evens (tievens)" <[email-masked]> wrote:
> 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.

Thanks!

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

I understand that the flags space is *very* small and that in hindsight
making the flags dependent on the peer types would have been a better
choice.  However, retroactively changing the wire semantics looks like
something that should be treated with care.

If you really want to go that way I think the document should contain an
analysis of the impact on existing deployments and any dependent tooling
around those.  The requests to IANA should be made more precise too: for
example, what happens to the V, L, A, and O flags?  To which peer types
would they be associated with in the new table?

Or, you could make it someone else's problem, after the 8th flag has been
allocated.

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

This IANA situation looks quite intricate.  I suggest you clarify exactly
what the desired state of the BMP parameters registry is and make it
crystal clear in the IANA considerations section because as it stands
I don't think it is easily actionable.

cheers!

PS: Have you considered adding an "Implementation Status" section to the
doc (BCP 205) and talk a bit about OpenBMP?

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.