[RTG-DIR] Rtgdir last call review of draft-ietf-rtgwg-bgp-pic-12

[Bruno Decraene via Datatracker <[e-mail]>]

Reviewer: Bruno Decraene
Review result: Has Issues

Hello,

I have been selected as the Routing Directorate reviewer for this draft. The
Routing Directorate seeks to review all routing or routing-related drafts as
they pass through IETF last call and IESG review, and sometimes on special
request. The purpose of the review is to provide assistance to the Routing ADs.
For more information about the Routing Directorate, please see
​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Although these comments are primarily for the use of the Routing ADs, it would
be helpful if you could consider them along with any other IETF Last Call
comments that you receive, and strive to resolve them through discussion or by
updating the draft.

Document: draft-ietf-rtgwg-bgp-pic-12.txt
Reviewer: Bruno Decraene
Review Date: 2021-01-29
IETF LC End Date: not started. (in WG LC)
Intended Status: Informational

Summary:
    I have some minor concerns about this document that I think should be
    resolved before publication.

Comments:

Draft is clear and very readable although the subject is a bit complex and
usually not well known from IETF readers/documents as the described behavior is
local to a node and largely implementation specific.

The terminology and some text are very implementation dependent. As such, the
document may not age well and may not map directly to other implementations.
May be the abstract / introduction could clarify that the document is partly an
architecture and partly a description of one specific implementation.

=================
Minor:
=================
References:
LDP and SR-MPLS are used interchangeably in the text. There doesn't seem to be
a reason for the former to be a normative reference while the latter an
informative reference.
----
IdNits has the following complaint:
    "The document has examples using IPv4 documentation addresses according
     to RFC6890, but does not use any IPv6 documentation addresses.  Maybe
     there should be IPv6 examples, too?"

It looks a priori easy to fix. e.g. replacing the second IPv4 route (e.g.
65000: 203.0.113.0/24) by an IPv6 one.
-----
Introduction
I feel that the introduction could introduce both PIC core and PIC edge, rather
than just "PIC"

-----
§1.1
The terminology section would benefit from adding the terms "PIC Core" and "PIC
Edge" (e.g. using the first sentences of sections 6.1 & 6.2)

------
§1.1

"IGP prefix: A prefix P/m (of any AFI/SAFI) that is learnt via
      an Interior Gateway Protocol, such as OSPF and ISIS, has a path
      for. "

A priori incorrect grammar. You could remove either ", has a path for" or "is
learnt via"

-----
   "o  Egress PE, "ePE": A BGP speaker that learns about a prefix
      through an eBGP peer and chooses that eBGP as the next-hop for
      that prefix."

May be
OLD: that eBGP
NEW: that eBGP peer

------
§1.1
"Adjacency" definition could be moved before the definition of "Primary path"
since the definition of the latter use the former.

-------
§1.1
"Forwarding chain" is used in some definition but not defined beforehand.

-------
§2
"It is not uncommon to see multiple destinations are reachable via the same
list of next-hops."

A priori incorrect grammar.
OLD: are reachable
could be "that are reachable" or "reachable"

-------
2.1.2
Another option to learn multiple BGP NH/path is simply to receive IBGP paths
from multiple BGP RR selection a different path as best.(and hence reflecting a
different path).
-------
§2.2

"2 ECMP paths with IGP-NH1 and IGP-NH2 reachable via the interfaces I1 and I2,
respectively." I don't precisely understand the sentence. "Respectively" means
that I1 (resp. I2) is used to reach IGP-N1 (resp. IGP-N2). Soe where are the 2
ECMP paths? Note that given the data in Figure 2, my understanding is that
there are only 2 interfaces (I1 and I2) used by both IGP prefixes. Hence ",
respectively" to be removed.

It seems that either "respectively" is to be removed, or that 2 interfaces are
missing (2ECMP*2 destinations)

For better illustration, Figure 1 could be extended by adding the connected
interfaces and neighbors of iPE.

Also the routing table could mention those interfaces
OLD:
         192.0.2.1/32
            via Core, Label:  IGP-L11
            via Core, Label:  IGP-L12

         192.0.2.2/32
            via Core, Label:  IGP-L21
            via Core, Label:  IGP-L22

NEW:
         192.0.2.1/32
            via I1, Label:  IGP-L11
            via I2, Label:  IGP-L12

         192.0.2.2/32
            via I1, Label:  IGP-L21
            via I2, Label:  IGP-L22

All of the above to be corrected based on the response to the first comments.
(are there 2 or 4 interfaces?)

-------
§ Figure 2
The draft uses NH1 for both BGP-NH1 and IGP-NH1 and they represent different
things. Some parts of Figure sometimes uses "NH1" alone (in OutLabel-List)
which is ambiguous.

Also
:s/BGP NH1/BGP-NH1
:s/IGP NH1/IGP-NH1

-------
§2.2
"For example, if the interface "I2" goes down, only the shared IGP pathlist
needs to be updated,"

It seems to me that the IGP OutLabel-List would also need to be updated.

------
§2.2
Figure 2 (and may be section 2.2) does not refer to local protection (IGP FRR,
eBGP FRR). I'd guess that some of the Paths are marked as backup. If so, I
would suggest to add this.

-------
§6.2.1
"IGP running on the iBGP peers instructs FIB to remove the IP"

Instead of "iBGP peers" I think that you have introduced the term "iPE" just
for the purpose. (alternative "iBGP routers" as "iBGP peers" raise the question
of "peers of who?" which introduce complexity if one wants to accommodate for
Route Reflectors)

-------
§3.2
In figure 3,
"
  VPN-L11               +---------+
   (Label-leaf)---+---->|Unlabeled|
                  |     +---------+
                  |     | VPN-L21 |
                  |     | (swap)  |
                  |     +---------+
                  |
                  |                    BGP Pathlist
                  |                   +------------+    Connected route
                  |                   |   CE-NH    |------>(to the CE)
                  |                   |path-index=0|
                  |                   +------------+
                  |                   |  VPN-NH2   |
     VPN-IP1 -----+------------------>|  (backup)  |------>IGP Leaf
"

It's not obvious whether the long vertical line should be read from Up to down
of from Down to up, as there is no arrow on it. My understanding is that it
should be read up to down. In which case, adding an arrow would help. e.g.
 VPN-L11
   (Label-leaf)---+---->
                  |
                  V
                  |


-------
§5.1
"a sample scenario"
Did you mean :s/sample/simple    ?

-------
§5.2
"the VPN prefix VPN-IP1 and VPN2-IP2."

The figure uses "VPN-IP2"  (VPN- vs VPN2-).
May be :s/prefix/prefixes

-------
§5.2
"   o  Both the routers ASBR21 and ASBR22 of Domain 2 advertise the same
      label LASBR21 and LASBR22 to the egress PEs ePE1 and ePE2,
      respectively, to the routers ASBR11 and ASBR22 of Domain 1."

may be "advertise the same label LASBR21 and LASBR22 for the egress PEs ePE1
and ePE2"  (:s/to/for)

-------
§5.2
OLD:
         65000: 203.0.113.0/24
            via ePE1 (192.0.2.2), VPN Label: VPN-L22
            via ePE2 (192.0.2.3), VPN Label: VPN-L32

NEW:

         65000: 203.0.113.0/24
            via ePE2 (192.0.2.2), VPN Label: VPN-L22
            via ePE3 (192.0.2.3), VPN Label: VPN-L32

(:s/PE1/PE2   :s/PE2/PE3 )

-------
§6.2.1
"hence the label VPN-L12 will be pushed"
In figure 2, I believed you used the name "VPN-L21" (L21 vs L12)

-------
§6.2.2
"Hence traffic will be forwarded used the backup path towards ePE2."
:s/used/using

-------
§6.2.2

" FIB manager on the edge router detecting the link failure applies the
following steps" May be you could add a reference to Figure 3 which precisely
describes this case. (otherwise the reader keeps using Figure 2 which is less
appropriate for this case.

-------
§6.2.2
Possibly the two cases (NH self and not NHS could be described in two sub-
sections. (When reading "In the second case", the reader may have lost the
context). In which case, the last paragraph "Let's try to apply the case of
next-hop self" could be grouped with the "First cast"/sub section.

But up to you. This is an editorial choice.

=================
Nits:
=================
Reference [3]. Somehow, the title and list of authors does not match the ones
in the referenced RFC. https://tools.ietf.org/html/rfc8277

------
§1.1
In Pathlist

OLD: . ").
NEW: .

------
§7.1
"This section provides
   details for each failure and now the hierarchical and shared FIB
   structure proposed in this document allows recovery that does not
   depend on number of BGP prefixes."

I would assume :s/now/how

------
:s/ABSRs /ASBRs