BGP Optimal Route Reflection (BGP ORR)
draft-ietf-idr-bgp-optimal-route-reflection-28

Thank you for the work on this document. I have a couple of very minor comments below, and a non blocking question: it seems to me that this document could have contained the "updates" header wrt RFC 4456. Was this considered by the working group?

Francesca

1. -----
 
FP: Please expand AS, POP, SPF, IBGP on first use.

2. -----

   The core of this solution is the ability for an operator to specify

FP: I would suggest adding the name and abbreviation BGP ORR here, otherwise it comes as a surprise in section 4.

I'm glad to see this draft moving forward!

I have some comments below, I hope they're helpful.

1. Introduction 

   [RFC4456] asserts that, because the IGP cost to a given point in the
   network will vary across routers, "the route reflection approach may
   not yield the same route selection result as that of the full IBGP
   mesh approach."  One practical implication of this assertion is that

Strictly speaking, it’s not an implication of the assertion, it’s an implication of the fact (that is being asserted). So, "... practical implication of this fact is that..." (or just "... implication of this is...").

2. Section 3.1

   One or more backup IGP locations SHOULD be allowed to be specified
   for redundancy.

Doesn’t this depend entirely on what option is chosen, of the three you’ve offered? They are, “per route reflector basis, per set of clients, or per client basis”. In the per client case, what would you use for a backup IGP location? When would you invoke the backup? (I’m imagining that the per-client case would generally use the client’s position in the IGP topology, in fact I imagine most implementations wouldn't force you to configure the IGP location when doing per-client.)

This sentence would also benefit from a forward reference to the additional discussion in Section 4, IMO. 

3. Section 3.1.1

I don’t think “BGP prefix” is a term of art, especially not as you’re using it. I think “BGP route” would be better. 

4. Section 3.2

The third paragraph talks about applying different policies. While it’s accurate, it’s a bit sparse. It’s very similar to what’s discussed in RFC 7947 Section 2.3, and especially Section 2.3.2.1. Might be worth informatively referencing that?

5. Section 4

When you write “hop-by-hop switching“, I think you mean forwarding, not switching, right?

   Modifying the IGP location of BGP ORR does not interfere with
   policies enforced before IGP tie-breaking (step e) in the BGP
   Decision Process Route.

That last word “Route” doesn’t need to be there. 

   (both should be equal to the [RFC4456] ones)

Both *what* should be equal to the RFC4456 one *what*? Confused.

I only have one nit to add to what others have pointed out.  In Section 3.1:

* "When specifing logical location ..." -- s/specifing/specifying/

Thank you to Linda Dunbar for the SECDIR review.

** Section 5.  Minor reframing of text
 
OLD
  Similarly to [RFC4456], this extension to BGP does not change the
   underlying security issues inherent in the existing IBGP.

NEW

This extension provides a new metric using additional information for computing routes for BGP router reflectors.  While any improperly used metric could impact the resiliency of the network, this extension does not change the underlying security issues inherent in the existing IBGP per [RFC4456].

** Typos

-- Section 3. Typo. s/consists in using/consists of using/

-- Section 3.1.1.  Typo. s/can not/cannot/

Just a quick note to say thanks to Dan for the OpsDir review, and to the authors for working to address them.

Thanks for the work on this document.

* Please expand BGP, IBGP, IGP , POP  in their first occurrence in the document.

Thank you for this document; it gives a clear explanation of what is to
be done and why, and I basically only have nit-level comments, with the
exception of the first remark.

Section 4

   This solution can be deployed in traditional hop-by-hop forwarding
   networks as well as in end-to-end tunneled environments.  In networks
   where there are multiple route reflectors and hop-by-hop forwarding
   without encapsulation, such optimizations SHOULD be consistently
   enabled on all route reflectors.  Otherwise, clients may receive an
   inconsistent view of the network, in turn leading to intra-domain
   forwarding loops.

The scope of "forwarding loops" as a potential problem makes me wonder
if the "SHOULD"-level requirement for this avoidance mechanism is the
right choice.

Section 5

Thank you for accepting the wording suggestion from Roman; it's a good
improvement.

I might consider reiterating that there are risks if multiple route
reflectors are present but using different policy/procedures, though the
earlier discussion may suffice.

Section 9.2

Whether RFC 7911 should be normative or informative seems potentially
subject to debate, if deployment of add-path is required in order for
correct operation of optimal route reflectors.

NITS

                                                    This ability enables
   the route reflector to send to a given set of clients routes with
   shortest distance to the next hops from the position of the selected
   IGP location.  [...]

This feels a bit like a garden-path sentence that makes the reader
backtrack to fix their parse tree; commas around "to a given set of
clients" might help but a more substantive restructuring might be
preferred.

                  This provides for freedom of route reflector physical
   location, and allows transient or permanent migration of this network
   control plane function to an arbitrary location.

Pedantically, such a migration seems like it would always have been
possible, just with a potentially significant impact on performance and
route quality; I'd suggest appending a phrase such as "with negligible
impact on performance".

   The choice of specific granularity (route reflector, set of clients,
   or client) is configured by the network operator.  An implementation
   is considered compliant with this document if it supports at least
   one listed grouping of IGP location.

It's not entirely clear to me how a "listed grouping of IGP location"
maps to the previous text.

Section 3.2

   If the required routing optimization is limited to the IGP cost to
   the BGP Next-Hop, only step e) and below as defined [RFC4271] section
   9.1.2.2, needs to be run multiple times.

I think maybe s/and below/and subsequent steps/, and s/as defined/as
defined in/.

Section 4

   route reflectors.  More specifically, the choice of BGP path factors
   in either the IGP cost between the client and the NEXT_HOP (rather

I suggest s/factors in/takes into account/, since "factors in" is
something of a colloquialism.

   Modifying the IGP location of BGP ORR does not interfere with
   policies enforced before IGP tie-breaking (step e) in the BGP
   Decision Process Route.

I think "step e" has to be scoped to §9.1.2.2 of RFC 4271.

                                                          Similarly, if
   an IGP location is selected for the whole routing instance, the
   lowest precision is achieved, but the performance impact is minimal
   (both should be equal to the [RFC4456] ones).  [...]

The phrasing in the parenthetical seems surpising to me -- what does
"both" refer to?  I can only see the one "performance of the
single-IGP-location-for-the-whole-routing-instance" situation, which of
course should have equivalent performance to the stock RFC 4456 case.

Section 5

   This extension provides a new metric value using additional
   information for computing routes for BGP router reflectors.  While

s/router/route/

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.

Section 1. , paragraph 5, nit:
-    Initially this model was only employed for new services, e.g.  IP
-                                                                 ^
+    Initially this model was only employed for new services, e.g., IP
+                                                                 ^

Section 1. , paragraph 5, nit:
-    services including IPv4 and IPv6 Internet.  In such environments, hot
+    services, including the IPv4 and IPv6 Internet.  In such environments, hot
+            +          ++++

Section 3. , paragraph 2, nit:
-    IGP topology, it is identified by an IP address of this node (e.g. a
+    IGP topology, it is identified by an IP address of this node (e.g., a
+                                                                      +

Section 3. , paragraph 5, nit:
-    o  it has a different position in the IGP topology, and
-                                                        ^^^
+    o  it has a different position in the IGP topology, or
+                                                        ^^

Section 3.1.1. , paragraph 2, nit:
-    cost to reach such next hop.  Implementations which can not inform
-                                                           -
+    cost to reach such next hop.  Implementations which cannot inform

Section 1. , paragraph 3, nit:
>  deployments of route reflectors outside of the forwarding path. Initially t
>                                  ^^^^^^^^^^
This phrase is redundant. Consider using "outside".

Section 1. , paragraph 4, nit:
> ains desirable. Route reflectors outside of the forwarding path can be placed
>                                  ^^^^^^^^^^
This phrase is redundant. Consider using "outside".

Section 3.1. , paragraph 7, nit:
> uring next hop metric comparison. However such paths MUST still be considere
>                                   ^^^^^^^
A comma may be missing after the conjunctive/linking adverb "However".

Section 3.2. , paragraph 5, nit:
> imizations SHOULD be enabled in a consistent way on all route reflectors. Ot
>                              ^^^^^^^^^^^^^^^^^^^
Consider replacing this phrase with the adverb "consistently" to avoid
wordiness.

Section 4. , paragraph 6, nit:
> f clients or per client. A more fine grained granularity may translate into m
>                                 ^^^^^^^^^^^^
This word is normally spelled with a hyphen.

Section 4. , paragraph 7, nit:
> ce, the lowest precision is achieved but the performance impact is minimal (
>                                     ^^^^
Use a comma before "but" if it connects two independent clauses (unless they
are closely connected and short).

Section 4. , paragraph 7, nit:
> 4456] ones). The ability to run fine grained computations depends on the plat
>                                 ^^^^^^^^^^^^
This word is normally spelled with a hyphen.

Thanks for this document - sounds useful!  And thanks to Dan for the Ops-dir review.

May I please check, is there a YANG data model that covers this functionality?

Regards,
Rob