Subject: [RTG-DIR] Rtgdir Last Call review of draft-ietf-lsvr-bgp-spf-13

From: Yingzhen Qu <[email-masked]>
Date: Tue, 22 Jun 2021 12:45:31 -0700

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 <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-lsvr-bgp-spf
Reviewer: Yingzhen Qu
Review Date: Jun 15th, 2021
Intended Status: Standards Track

Summary:

This document has some issues that should be at least considered prior to publication.

Comments:

Major issues:

Considering	the SPF Status attribute TLV has the same type for Node, Link and Prefix NLRI, it is implied that a BGP update message can only contain a single kind of NLRI (for example, Node or Link). I’d suggest make the draft explicitly state it.

About SPF Status TLV, use the Node NLRI as an example (section 5.2.1.2), it says in the draft: “If the SPF Status TLV is not included with the Node NLRI, the node is considered to be up and is available for transit traffic.”, then later, “If a BGP speaker received the Node NLRI but the SPF Status TLV is not received, then any previously received information is considered as implicitly withdrawn and the update is propagated to other BGP speakers. “. These two statements seem to conflict.

Comments inline:

[Line numbers from idnits]

78	       5.2.1.  Node NLRI Usage . . . . . . . . . . . . . . . . . . .  11
79	         5.2.1.1.  Node NLRI Attribute SPF Capability TLV  . . . . .  11
80	         5.2.1.2.  BGP-LS-SPF Node NLRI Attribute SPF Status TLV . .  12

[Minor]: section 5.2.1.1 should be changed to "BGP-LS-SPF Node NLRI Attribute SPF Capability TLV" to be consistent with other titles.

226	   Another potential advantage of BGP SPF is that both IPv6 and IPv4 can
227	   both be supported using the BGP-LS-SPF SAFI with the same BGP-LS-SPF
228	   NLRIs.  

[Minor]: potential advantage? I'd suggest remove "potential".
[nits]: both IPv6 and IPv4 can both be supported

315	   The BGP SPF extensions reuse the Node, Link, and Prefix NLRI defined
316	   in [RFC7752].  The usage of the BGP-LS NLRI, metric attributes, and
317	   attribute extensions is described in Section 5.2.1.

[minor]: metric attributes? Might be better just remove it.
s/Section 5.2.1/Section 5.2

357	   hop sessions) and the direct connection discovery and liveliness
358	   detection for the interconnecting links are independent of the BGP
359	   protocol.  the scope of this document.  For example, liveliness

[nits]: unfinished sentence: the scope of this document.

470	5.2.  Extensions to BGP-LS

472	   [RFC7752] describes a mechanism by which link-state and TE
473	   information can be collected from IGPs and shared with external
474	   components using the BGP protocol.  It describes both the definition
475	   of the BGP-LS-SPF NLRI that advertise links, nodes, and prefixes
476	   comprising IGP link-state information and the definition of a BGP
477	   path attribute (BGP-LS attribute) that carries link, node, and prefix

[Major]: line 475: should be "BGP-LS NLRI"

556	   If the SPF Status TLV is received and the corresponding Node NLRI has
557	   not been received, then the SPF Status TLV is ignored and not used in
558	   SPF computation but is still announced to other BGP speakers.  An
559	   implementation MAY log an error for further analysis.  If a BGP
560	   speaker received the Node NLRI but the SPF Status TLV is not
561	   received, then any previously received information is considered as
562	   implicitly withdrawn and the update is propagated to other BGP
563	   speakers.  A BGP speaker receiving a BGP Update containing a SPF
564	   Status TLV in the BGP-LS attribute [RFC7752] with a value that is
565	   outside the range of defined values SHOULD be processed and announced
566	   to other BGP speakers.  However, a BGP speaker MUST not use the
567	   Status TLV in its SPF computation.  An implementation MAY log this
568	   condition for further analysis.

[nits]: s/MUST not/MUST NOT
[minor]: s/BGP speaker/BGP SPF speaker
There are multi places in the draft using "BGP speaker" instead of "BGP SPF speaker", please go over and fix when applicable.
For example: 
1408	   excessive SPF calculations.  When a BGP speaker detects that its
section 7.1

864	   When a BGP SPF speaker completely loses its sequence number state,
865	   i.e., due to a cold start, or in the unlikely possibility that that

[nits]: extra "that".

943	   o  Local Route Information Base (LOC-RIB) - This routing table
944	      contains reachability information (i.e., next hops) for all
945	      prefixes (both IPv4 and IPv6) as well as BGP-LS-SPF node
946	      reachability.  Implementations may choose to implement this with
947	      separate RIBs for each address family and/or Prefix versus Node
948	      reachability.  It is synonymous with the Loc-RIB specified in
949	      [RFC4271].

[nits]: s/Loc-RIB/LOC-RIB

1017	       *  If the Current-Prefix's corresponding prefix is in the LOC-RIB
1018	          and the cost is less than the current route's metric, the

[major]: I think this meant to be "more than the current route's metric"

1134	   operate today (i.e., "Ships-in-the-Night" mode).  There is no
1135	   implicit route redistribution between the BGP address families.

[major]: what about redistribution from other protocols, say OSPF?  

1167	   prior to withdrawal.  If the link becomes available in that period,
1168	   the originator of the BGP-LS-SPF LINK NLRI SHOULD advertise a more
1169	   recent version of the BGP-LS-SPF Link NLRI without the SPF Status TLV
1170	   in the BGP-LS Link Attributes. 

[major]: "without SPF Status TLV", this is related with the second “Major issues” above.

1263	   A BGP-LS-SPF Speaker MUST perform the following syntactic validation
1264	   of the BGP-LS-SPF NLRI to determine if it is malformed.

[minor]: there are a few places in the draft using "BGP-LS-SPF Speaker|speaker", may change to "BGP SPF speaker"?  

1425	   Within a BGP SPF Routing Domain, the IGP metrics for all advertised
1426	   links SHOULD be configured or defaulted consistently. 

[major]: in section 5.2.2, it says "One possible default for metric
   would be to give each interface a cost of 1 making it effectively a
   hop count.". Why not define a default value so all implementations will be consistent?

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Wed, 30 Jun 2021 15:09:20 +0000

Hi Yingzhen,

Thanks for the review. We’ve incorporated most of your comments into the -14 version.

From: Yingzhen Qu <[email-masked]>
Date: Tuesday, June 22, 2021 at 3:46 PM
To: Routing Directorate <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>
Cc: "[email-masked]" <[email-masked]>, Luc André Burdet <[email-masked]>, "Yemin (Amy)" <[email-masked]>
Subject: Rtgdir Last Call review of draft-ietf-lsvr-bgp-spf-13
Resent-From: <[email-masked]>
Resent-To: Acee Lindem <[email-masked]>, Shawn Zandi <[email-masked]>, Wim Henderickx <[email-masked]>, Keyur Patel <[email-masked]>
Resent-Date: Tuesday, June 22, 2021 at 3:45 PM

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

Summary:

Comments:

Major issues:

·         Considering the SPF Status attribute TLV has the same type for Node, Link and Prefix NLRI, it is implied that a BGP update message can only contain a single kind of NLRI (for example, Node or Link). I’d suggest make the draft explicitly state it.

I’ve added a paragraph on this in 5.1.1. If we were to make this a restriction, we’d need to discuss in WG.

·         About SPF Status TLV, use the Node NLRI as an example (section 5.2.1.2), it says in the draft: “If the SPF Status TLV is not included with the Node NLRI, the node is considered to be up and is available for transit traffic.”, then later, “If a BGP speaker received the Node NLRI but the SPF Status TLV is not received, then any previously received information is considered as implicitly withdrawn and the update is propagated to other BGP speakers. “. These two statements seem to conflict.

I’ve fixed this for all the status TLVs.

Comments inline:

Fixed.

Fixed both.

Changed to just “attributes” and fixed reference.

Removed.

Good catch – fixed.

Used “BGP SPF Speaker” consistently and fixed several “MUST not” instances.

[nits]: extra "that".

Removed.

In this case, RFC 4271 uses “Loc-RIB”.

Clarified this so that there is no ambiguity as to which metric is referenced.

This has typically been out of scope in routing protocol documents – although I just saw a new draft dealing with BGP redistribution. In any case, I think this topic would warrant further discussion in a different draft.

Since all the SPF Status conditions indicate unreachability or a degraded state, this is correct.

Yup – fixed all these.

We discussed and don’t want to add a default as different deployments could have totally different requirements. Also, other IGPs don’t have default metrics (at least, the link-state ones).

Thanks,
Acee

---

From: Yingzhen Qu <[email-masked]>
Date: Wed, 30 Jun 2021 14:50:50 -0700

Hi Acee,

Thanks for addressing my comments. I had a quick look at the changes in version -14, and have a question about the following comment. The change done does make the second point clear, but my original comment was about the fourth point. Please also consider make the term consistent, “Current-Prefix’s” vs. “current route’s”.

> Clarified this so that there is no ambiguity as to which metric is referenced.

Thanks,
Yingzhen

> On Jun 30, 2021, at 8:09 AM, Acee Lindem (acee) <[email-masked]> wrote:
> 
> Hi Yingzhen, 
>  
> From: Yingzhen Qu <[email-masked] <mailto:[email-masked]>>
> To: Routing Directorate <[email-masked] <mailto:[email-masked]>>, "[email-masked] <mailto:[email-masked]>" <[email-masked] <mailto:[email-masked]>>, "[email-masked] <mailto:[email-masked]>" <[email-masked] <mailto:[email-masked]>>
> Cc: "[email-masked] <mailto:[email-masked]>" <[email-masked] <mailto:[email-masked]>>, Luc André Burdet <[email-masked] <mailto:[email-masked]>>, "Yemin (Amy)" <[email-masked] <mailto:[email-masked]>>
> Resent-From: <[email-masked] <mailto:[email-masked]>>
> Resent-To: Acee Lindem <[email-masked] <mailto:[email-masked]>>, Shawn Zandi <[email-masked] <mailto:[email-masked]>>, Wim Henderickx <[email-masked] <mailto:[email-masked]>>, Keyur Patel <[email-masked] <mailto:[email-masked]>>
> 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 seehttp://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir <http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir>
>

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 1 Jul 2021 00:15:42 +0000

Hi Yingzhen,

From: Yingzhen Qu <[email-masked]>
Date: Wednesday, June 30, 2021 at 5:51 PM
To: Acee Lindem <[email-masked]>
Cc: Routing Directorate <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, Luc André Burdet <[email-masked]>, "Yemin (Amy)" <[email-masked]>
Subject: Re: Rtgdir Last Call review of draft-ietf-lsvr-bgp-spf-13

Hi Acee,

This is inconsistent but in the case “current route’s” should be “LOC-RIB route’s” to be correct with respect to the rule. There were two instances of this inconsistency. I will fix in the next revision along with a couple comments from Haibo.

Thanks,
Acee

Thanks,
Yingzhen

On Jun 30, 2021, at 8:09 AM, Acee Lindem (acee) <[email-masked]<mailto:[email-masked]>> wrote:

Hi Yingzhen,

From: Yingzhen Qu <[email-masked]<mailto:[email-masked]>>
Date: Tuesday, June 22, 2021 at 3:46 PM
To: Routing Directorate <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>
Cc: "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, Luc André Burdet <[email-masked]<mailto:[email-masked]>>, "Yemin (Amy)" <[email-masked]<mailto:[email-masked]>>
Subject: Rtgdir Last Call review of draft-ietf-lsvr-bgp-spf-13
Resent-From: <[email-masked]<mailto:[email-masked]>>
Resent-To: Acee Lindem <[email-masked]<mailto:[email-masked]>>, Shawn Zandi <[email-masked]<mailto:[email-masked]>>, Wim Henderickx <[email-masked]<mailto:[email-masked]>>, Keyur Patel <[email-masked]<mailto:[email-masked]>>
Resent-Date: Tuesday, June 22, 2021 at 3:45 PM

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 seehttp://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Summary:

Comments:

Major issues:

•         Considering the SPF Status attribute TLV has the same type for Node, Link and Prefix NLRI, it is implied that a BGP update message can only contain a single kind of NLRI (for example, Node or Link). I’d suggest make the draft explicitly state it.

•         About SPF Status TLV, use the Node NLRI as an example (section 5.2.1.2), it says in the draft: “If the SPF Status TLV is not included with the Node NLRI, the node is considered to be up and is available for transit traffic.”, then later, “If a BGP speaker received the Node NLRI but the SPF Status TLV is not received, then any previously received information is considered as implicitly withdrawn and the update is propagated to other BGP speakers. “. These two statements seem to conflict.

Comments inline:

Fixed both.

Removed.

Good catch – fixed.

[nits]: extra "that".

Removed.

Yup – fixed all these.

Thanks,
Acee

---

From: Yingzhen Qu <[email-masked]>
Date: Wed, 30 Jun 2021 17:37:20 -0700

Hi Acee,

Thanks for the clarification. I thought “current route” meant “the Current-Prefix’s”, hence the confusion.

Thanks,
Yingzhen

> On Jun 30, 2021, at 5:15 PM, Acee Lindem (acee) <[email-masked]> wrote:
> 
> Hi Yingzhen, 
>  
> To: Acee Lindem <[email-masked] <mailto:[email-masked]>>
> Cc: Routing Directorate <[email-masked] <mailto:[email-masked]>>, "[email-masked] <mailto:[email-masked]>" <[email-masked] <mailto:[email-masked]>>, "[email-masked] <mailto:[email-masked]>" <[email-masked] <mailto:[email-masked]>>, "[email-masked] <mailto:[email-masked]>" <[email-masked] <mailto:[email-masked]>>, Luc André Burdet <[email-masked] <mailto:[email-masked]>>, "Yemin (Amy)" <[email-masked] <mailto:[email-masked]>>
>> On Jun 30, 2021, at 8:09 AM, Acee Lindem (acee) <[email-masked] <mailto:[email-masked]>> wrote:
>>  
>> Hi Yingzhen, 
>>

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Thu, 1 Jul 2021 00:42:47 +0000

Hi Yingzhen,

From: rtg-dir <[email-masked]> on behalf of Yingzhen Qu <[email-masked]>
Date: Wednesday, June 30, 2021 at 8:37 PM
To: Acee Lindem <[email-masked]>
Cc: Routing Directorate <[email-masked]>, "Yemin (Amy)" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, "[email-masked]" <[email-masked]>, Luc André Burdet <[email-masked]>
Subject: Re: [RTG-DIR] Rtgdir Last Call review of draft-ietf-lsvr-bgp-spf-13

Hi Acee,

Yes, best case it was ambiguous, worst it was wrong.

Thanks for Catching,
Acee

Thanks,
Yingzhen

On Jun 30, 2021, at 5:15 PM, Acee Lindem (acee) <[email-masked]<mailto:[email-masked]>> wrote:

Hi Yingzhen,

From: Yingzhen Qu <[email-masked]<mailto:[email-masked]>>
Date: Wednesday, June 30, 2021 at 5:51 PM
To: Acee Lindem <[email-masked]<mailto:[email-masked]>>
Cc: Routing Directorate <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, "[email-masked]<mailto:[email-masked]>" <[email-masked]<mailto:[email-masked]>>, Luc André Burdet <[email-masked]<mailto:[email-masked]>>, "Yemin (Amy)" <[email-masked]<mailto:[email-masked]>>
Subject: Re: Rtgdir Last Call review of draft-ietf-lsvr-bgp-spf-13

Hi Acee,

Thanks,
Acee

Thanks,
Yingzhen

Hi Yingzhen,

Summary:

Comments:

Major issues:

Comments inline:

Fixed both.

Removed.

Good catch – fixed.

[nits]: extra "that".

Removed.

Yup – fixed all these.

Thanks,
Acee