Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-19

[Acee Lindem <[e-mail]>]

Hi Alvaro, 

> On Mar 7, 2023, at 12:32 PM, Alvaro Retana <[e-mail]> wrote:
> 
> Dear authors:
> 
> I have now finished looking at the diffs between my last review (-11)
> and -19.  I focused mainly on the changes and additions.  I know -20
> is out -- I was already half way through -19 and decided to not
> transcribe the comments as the diffs (-19 to -20) are small.
> 
> The document looks a lot better.  Thank you for the hard work.
> 
> 
> My main concern is still the lack of semantic checking (see my
> comments in §7.*).  BGP-LS (rfc7752bis) only does syntactic validation
> because any semantic checking is left to the BGP-LS Consumer (and the
> specifics left out of scope).  The way BGP SPF uses the information is
> different (all the nodes run SPF), so the validity of the contents
> need to be verified

Keyur and I discussed and we’re not sure you think is missing. 

> .
> 
> Thanks!
> 
> Alvaro.
> 
> 
> 
> [Line numbers from idnits.]
> 
> 
> ...
> 119 1.  Introduction
> 
> 121   Many Massively Scaled Data Centers (MSDCs) have converged on
> 122   simplified layer 3 routing.  Furthermore, requirements for
> 123   operational simplicity have led many of these MSDCs to converge on
> 124   BGP [RFC4271] as their single routing protocol for both their fabric
> 125   routing and their Data Center Interconnect (DCI) routing [RFC7938].
> 126   This document describes an alternative solution which leverages BGP-
> 127   LS [I-D.ietf-idr-rfc7752bis] and the Shortest Path First algorithm
> 128   used by Internal Gateway Protocols (IGPs) such as OSPF [RFC2328].
> 
> [major] Because BGP SPF is it's own protocol, please don't compare it
> to other protocols.  I'll try to point at cases where I find them, but
> please take a look too.
> 
> In this case, the SPF is not the same as what OSPF uses because the
> details are different, which is why it is specified later on.

I’ve removed the references but I believe referring to similarities to IGPs in general is appropriate. 

> 
> 
> 
> ...
> 151 1.1.  Terminology
> ...
> 167   Dijkstra Algorithm:  An algorithm for computing the shortest path
> 168      from a given node in a graph to every other node in the graph.  At
> 169      each iteration of the algorithm, there is a list of candidate
> 170      vertices.  Paths from the root to these vertices have been found,
> 171      but not necessarily the shortest ones.  However, the paths to the
> 172      candidate vertex that is closest to the root are guaranteed to be
> 173      shortest; this vertex is added to the shortest-path tree, removed
> 174      from the candidate list, and its adjacent vertices are examined
> 175      for possible addition to/modification of the candidate list.  The
> 176      algorithm then iterates again.  It terminates when the candidate
> 177      list becomes empty.  [RFC2328]
> 
> [minor] This seems to be a very long definition of a term -- IMO you
> only need the first sentence.  Because there is a full definition of
> how it works later, a reference to that would be better.

Ok - though I’m not sure this longer description isn’t useful. 

> 
> BTW, the other references to this term can be simplified:
> 
> s/Dijkstra algorithm Section 1.1/Dijkstra algorithm/g
> 
> 
> [major] The reference to rfc2328 is not needed.

This is still needed though it is not referenced as often. 

> 
> 
> 
> 179 1.2.  BGP Shortest Path First (SPF) Motivation
> ...
> 191   A primary advantage is that all BGP SPF speakers in the BGP SPF
> 192   routing domain will have a complete view of the topology.  This will
> 193   allow support for ECMP, IP fast-reroute (e.g., Loop-Free
> 194   Alternatives), Shared Risk Link Groups (SRLGs), and other routing
> 195   enhancements without advertisement of additional BGP paths [RFC7911]
> 196   or other extensions.  In short, the advantages of an IGP such as OSPF
> 197   [RFC2328] are availed in BGP.
> 
> [] "In short, the advantages of an IGP such as OSPF [RFC2328] are
> availed in BGP."
> 
> Even if this is a positive comparison, no need to mention OSPF.  Also,
> there are (good) aspects of OSPF that are not in BGP SPF: areas, for
> example.

This has been removed.  

> 
> 
> 
> 199   With the simplified BGP decision process as defined in Section 6,
> 200   NLRI changes can be disseminated throughout the BGP routing domain
> 201   much more rapidly (equivalent to IGPs with the proper
> 202   implementation).  The added advantage of BGP using TCP for reliable
> 203   transport leverages TCP's inherent flow-control and guaranteed in-
> 204   order delivery.
> 
> [minor] s/simplified BGP decision process/BGP SPF decision process
> 
> 
> [] s/ (equivalent to IGPs with the proper implementation)./.
> 
> You seem to claim that IGPs are not implemented properly. :-(

I’ve removed this since it is out of scope. The implementations of IGPs as a DC underlay is a longer discussion. 


> 
> 
> 
> ...
> 217   With controller and route-reflector peering models, BGP SPF
> 218   advertisement and distributed computation require a minimal number of
> 219   sessions and copies of the NLRI since only the latest version of the
> 220   NLRI from the originator is required.  Given that verification of the
> 221   adjacencies is done outside of BGP (see Section 4), each BGP SPF
> 222   speaker will only need as many sessions and copies of the NLRI as
> 223   required for redundancy (see Section 4).  Additionally, a controller
> 224   could inject topology that is learned outside the BGP SPF routing
> 225   domain.
> 
> [nit] "Given that verification of the adjacencies is done outside of
> BGP (see Section 4), each BGP SPF speaker will only need as many
> sessions and copies of the NLRI as required for redundancy (see
> Section 4)."
> 
> No need to refer to §4 twice in the same sentence.


Agreed. 


> 
> 
> [minor] Also, I didn't see anything in §4 (or anywhere else) that
> talked about adjacency verification "outside of BGP".

Note the section 4 about using BFD for liveliness detection. 

> 
> 
> 
> 227   Given that controllers are already consuming BGP-LS NLRI
> 228   [I-D.ietf-idr-rfc7752bis], this functionality can be reused for BGP-
> 229   LS-SPF NLRI.
> 
> [minor] rfc7752bis clarified the role of a Consumer (§3) and it avoids
> calling it a controller...in fact it leaves the interface to the
> Consumer as out of scope.  Also, the consuming functionality is
> different from running SPF, etc.  All this is to say that I think you
> should drop this sentence.

Ok


> 
> 
> 
> ...
> 264 2.  Base BGP Protocol Relationship
> 
> 266   With the exception of the decision process, the BGP SPF extensions
> 267   leverage the BGP protocol [RFC4271] without change.  This includes
> 268   the BGP protocol Finite State Machine, BGP messages and their
> 269   encodings, processing of BGP messages, BGP attributes and path
> 270   attributes, BGP NLRI encodings, and any error handling defined in the
> 271   [RFC4271] and [RFC7606].
> 
> [nit] s/defined in the/defined in
> 
> 
> 
> 273   Due to the changes to the decision process, there are mechanisms and
> 274   encodings that are no longer applicable.  While not necessarily
> 275   required for computation, the ORIGIN, AS_PATH, MULTI_EXIT_DISC,
> 276   LOCAL_PREF, and NEXT_HOP path attributes are mandatory [RFC4271] and
> 277   will be validated.  Unless explicitly specified in the context of BGP
> 278   SPF, all other attributes SHOULD NOT be advertised.  However, if they
> 279   are advertised, they will be accepted, validated, and propagated
> 280   consistent with the BGP protocol.
> 
> [major] "NEXT_HOP path attributes are mandatory [RFC4271]"
> 
> I know that I suggested this text, but rfc4760 converts the NEXT_HOP
> to optional.
> 
> 
> 
> ...
> 290   The BGP SPF extension fundamentally changes the decision process, as
> 291   described herein, to be more like a link-state protocol (e.g., OSPF
> 292   [RFC2328]).  Specifically:
> 
> [] Take out references to OSPF.  In this case, it is not only an
> example, but the description is not specific.
> 
> s/
>    The BGP SPF extension fundamentally changes the decision process, as
>    described herein, to be more like a link-state protocol (e.g., OSPF
>    [RFC2328]). Specifically:
> /
>    The BGP SPF extension fundamentally changes the decision process, as
>    described herein:

Ok
> 
> 
> 
> 294   1.  BGP advertisements are readvertised to neighbors immediately
> 295       without waiting or dependence on the route computation as
> 296       specified in phase 3 of the base BGP decision process.  Multiple
> 297       peering models are supported as specified in Section 4.
> 
> 299   2.  Determining the degree of preference for BGP routes for the SPF
> 300       calculation as described in phase 1 of the base BGP decision
> 301       process is replaced with the mechanisms in Section 6.1.
> 
> 303   3.  Phase 2 of the base BGP protocol decision process is replaced
> 304       with the Shortest Path First (SPF) algorithm, also known as the
> 305       Dijkstra algorithm Section 1.1.
> 
> [nit] Reorder these so the Phases are in sequence.

They are in the correct order for BGP SPF. 

> 
> 
> 
> 307 3.  BGP Link-State (BGP-LS) Relationship
> 
> 309   [I-D.ietf-idr-rfc7752bis] describes a mechanism by which link-state
> 310   and TE information can be collected from networks and shared with
> 311   external entities using BGP.  This is achieved by defining NLRI
> 312   advertised using the BGP-LS AFI.  The BGP-LS extensions defined in
> 313   [I-D.ietf-idr-rfc7752bis] make use of the decision process defined in
> 314   [RFC4271].  This document reuses NLRI and TLVs defined in
> 315   [I-D.ietf-idr-rfc7752bis].  Rather than reusing the BGP-LS SAFI, the
> 316   BGP-LS-SPF SAFI Section 5.1 is introduced to insure backward
> 317   compatibility for the BGP-LS SAFI usage.
> 
> [nit] s/SAFI Section 5.1/SAFI (Section 5.1)

Ok. 



> 
> 
> 
> 319   The BGP SPF extensions reuse the Node, Link, and Prefix NLRI defined
> 320   in [I-D.ietf-idr-rfc7752bis].  The usage of the BGP-LS NLRI,
> 321   attributes, and attribute extensions is described in Section 5.2.
> 322   The usage of others BGP-LS attribute TLVs is not precluded and is, in
> 323   fact, expected.  However, the details are beyond the scope of this
> 324   document and will be specified in future documents.
> 
> [nit] s/will be specified/may be specified

Sure

> 
> 
> [] "others BGP-LS attributes"
> 
> There is only one BGP-LS Attribute.  This got me thinking about
> clarifying, simplifying and avoiding some redundancy.
> 
> Suggestion>
> 
>    [Eliminate this sentence from the previous paragraph: "This document
>    reuses NLRI and TLVs defined in [RFC7752]."]
> 
>    The BGP SPF extensions reuse the format of the Link-State NLRI, the
>    BGP-LS Attribute, and the TLVs defined in [RFC7752bis]. The usage of
>    is described in Section 5.2. The usage of other BGP-LS TLVs or
>    extensions is not precluded and is, in fact, expected. However, the
>    details are beyond the scope of this document and may be specified
>    in future documents.
> 
>    [Eliminate the next paragraph.]
> 
> 
> 
> 326   Support for Multiple Topology Routing (MTR) similar to the OSPF MTR
> 327   computation described in [RFC4915] is beyond the scope of this
> 328   document.  Consequently, the usage of the Multi-Topology TLV as
> 329   described in section 3.2.1.5 of [I-D.ietf-idr-rfc7752bis] is not
> 330   specified.
> 
> [] See the suggestion above.
> 
> 
> [major] Also, unless used as part of the specification, please avoid
> references to other protocols -- specially comparing them.

I think I’ve removed any comparisons. 

> 
> 
> 
> 332   The rules for setting the NLRI next-hop path attribute for the BGP-
> 333   LS-SPF SAFI will follow the BGP-LS SAFI as specified in section 3.4
> 334   of [I-D.ietf-idr-rfc7752bis].
> 
> [] "section 3.4 of [I-D.ietf-idr-rfc7752bis]"
> 
> That section doesn't exist.
> 
> This pointer contradicts what §5.3 says.  Please see comments there.

Changed to 5.5 in [I-D.ietf-idr-rfc7752bis]. 

> 
> 
> 
> 336 4.  BGP Peering Models
> 
> 338   Depending on the topology, scaling, capabilities of the BGP SPF
> 339   speakers, and redundancy requirements, various peering models are
> 340   supported.  The only requirements are that all BGP SPF speakers in
> 341   the BGP SPF routing domain exchange BGP-LS-SPF NLRI, run an SPF
> 342   calculation, and update their routing table appropriately.
> 
> [minor] These are not the only possible deployments or the only ones
> supported, right?

But other routers in the deployments are not part of the BGP SPF routing domain. I’ve reworded. 


>  For example, a mixture is also possible.  It would
> be good to add something about these being examples of common
> deployments.

I don’t think this belongs here. This could be the topic the applicability or future draft. 


> 
> 
> 
> 344 4.1.  BGP Single-Hop Peering on Network Node Connections
> 
> 346   The simplest peering model is the one where EBGP single-hop sessions
> 347   are established over direct point-to-point links interconnecting the
> 348   nodes in the BGP SPF routing domain.  Once the single-hop BGP session
> 349   has been established and the BGP-LS-SPF AFI/SAFI capability has been
> 350   exchanged [RFC4760] for the corresponding session, then the link is
> 351   considered up from a BGP SPF perspective and the corresponding BGP-
> 352   LS-SPF Link NLRI is advertised.  Depending on the topology and
> 353   convergence requirements, it MAY be required that BGP End-of-RIB
> 354   (EoR) marker [RFC4724] is received for the BGP-LS-SPF SAFI prior to
> 355   advertisement.  This is analogous to the IGP requirement that the
> 356   Link State Databases be synchronized prior to advertisement of the
> 357   router link.  Enforcement of requiring the EoR constraint SHOULD be
> 358   configurable.  If the session goes down, the corresponding Link NLRI
> 359   will be withdrawn.  Topologically, this would be equivalent to the
> 360   peering model in [RFC7938] where there is a BGP session on every link
> 361   in the data center switch fabric.  The content of the Link NLRI is
> 362   described in Section 5.2.2.
> 
> [major] "BGP-LS-SPF AFI/SAFI capability"
> 
> This capability doesn't exist.  The Multiprotocol Extensions
> capability including the BGP-LS-SPF AFI/SAFI pair does...


Good catch. 


> 
> In the context of this document and in this section, it is not
> necessary to talk about the capability at all because it is obvious
> that the text is talking about a session used for BGP-LS-SPF...  Also,
> the specification of the requirement to exchange the capability is
> already in §5.1.  The text just adds complexity to what should be
> simple text.
> 
> 
> [**] I noticed the changes in -20 related to the EoR text.  Two comments:
> 
> - You only changed the text in this section, but the same applies to
> the next two models.
> 
> - Because the text applies to all peering models, it would be better
> to put it in §10.1.2 and keep it in a single place.
> 
> 
> 
> 364 4.2.  BGP Peering Between Directly-Connected Nodes
> 
> 366   In this model, BGP SPF speakers peer with all directly-connected
> 367   nodes but the sessions may be between loopback addresses (i.e., two-
> 368   hop sessions) and the direct connection discovery and liveliness
> 369   detection for the interconnecting links are independent of the BGP
> 370   protocol.  For example, liveliness detection could be done using the
> 371   BFD protocol [RFC5880].  Precisely how discovery and liveliness
> 372   detection is accomplished is outside the scope of this document

I think this discussion belongs with the peering models (all 3) but I’ve removed the configuration requirements. 

Thanks,
Acee