Last Call Review of draft-ietf-lsvr-bgp-spf-13
review-ietf-lsvr-bgp-spf-13-rtgdir-lc-qu-2021-06-29-00

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?