Subject: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-11.txt

From: "Acee Lindem (acee)" <[email-masked]>
Date: Mon, 15 Feb 2021 15:55:47 +0000

Hi Alvaro,

Thank you for your extensive review and comments. Now that the document
is back in the WG, we have couple things to discuss in the WG:

    1. NLRI packing - Well it is almost implied, I think we need to
       limit a BGP Update to a single NLRI and BGP-LS Attribute.

    2. Initial synchronization - we need to discuss this in the draft
       and  potentially add an option to require this, such as the IS-IS
       suppress adjacency option. I don't think we'd want to require
       this as it would limit deployment.

    3. NEXT_HOP requiremeent - We now say we are following RFC 4760.
       However, we need to be sure we are consistent throughout.

    4. Single session requirement for BGP-LS. Right now we don't
       prevent this.

Also, given the sheer volume there are some easy one that we missed in
-13 and will add in -14

Please see replies inline.

Thanks,
Acee, Keyur, Victor, and other Authors

        Dear authors:

                Thank you for the work you've put so far into this document!

                I believe that the protocol is underspecified, and there is a
                significant amount of work still needed, which have resulted in a
                longer than expected set of comments/questions [*].

                I approached this document from the point of view of it defining a
                *new* link-state protocol, one that borrows from existing work.  I am
                loosely defining "protocol" here as a set of rules for what happens
                inside a specific AFI/SAFI that doesn't carry "traditional" BGP
                routes, similar to BGP-LS, FlowSpec, etc.  Specifically, I see BGP SPF
                as the sum of the BGP base as transport + BGP-LS encodings + SPF-based
                route calculation.  When adding new components, something has to be
                taken out.  I then expected a discussion about which parts of the BGP
                base are used/apply to BGP SPF, and which don't + a specification of
                how the BGP-LS encodings are used, and the definition of new TLVs +
                SPF details.

                Instead, the document is written as an extension to BGP-LS.  BGP-LS
                also uses the BGP transport (as is!), but it has a set of assumptions
                that are not in line with a link-state protocol.  For example, rfc7752
                is clear about the consumer-based distribution model, including that
                "a BGP speaker MUST NOT accept updates from a consumer peer".  More
                importantly, rfc7752 considers BGP-LS as transporting opaque data
                relevant only to the application, which is why there is no error
                checking (beyond the length): it is not BGP-LS's job to verify that
                the information is correct (or that it even makes sense).  This
                document doesn't address either of these issues, but it presents a
                peering model (??2.3) where a controller (aka consumer) sends routes to
                other peers.  IOW, by extending BGP-LS we inherit the good and the
                not-so-good.

                I believe I have a good understanding of what you are doing with BGP
                SPF.  We need to make sure that the intent is reflected in the
                specification.  To start, the document needs a clear explanation of
                what it is, a section(s) that explains what is being used from the
                rfc4271/rfc7752, and what isn't.  There are many questions that need
                to be answered (see comments in-line and some more questions/comments
                below [**]).

The authors don't really agree that this is a completely new link-state
protocol. We've added to clarify the relationship to both the BGP base
protocol and BGP-LS.

                I haven't read draft-ietf-lsvr-applicability yet -- if some of the
                answers are there, please point me to it.  When I finish that document
                I'll consider whether the documents need to be returned to the WG.
                For now, I'll rely on the Shepherd to help in moving this document
                forward.

                The mail archive contains early requests to both idr/lsr for review,
                but no answers. :-(   I would like both of those WGs to (again) be
                given the opportunity to comment -- we can do that as this document
                addresses some of my concerns.  [I'm putting this note here so we
                don't forget.]

It is better that LSR and IDR review the updated version as the changes are
significant.

                Thank you!!

                Alvaro.

                [*] This is a long review, and some mail clients (Outlook?) don't seem
                to handle long messages well.  Please take a look at the archive for
                the full review; I will put a link in the datatracker:
                https://datatracker.ietf.org/doc/draft-ietf-lsvr-bgp-spf/history/

                [**]

                It would be ideal for the document to include a "Summary of Operation"
                section, similar to ??3/rfc4271.  This would also serve as a place to
                point back at what applies and what doesn't.

We've added a "Document Overview" in section 1.3. This should suffice in giving
the reader a preview of what is coming.

                (1) Use of the BGP Base (rfc4271)

                - The well-known RIB structure from BGP (Adj-RIB-In, Loc-RIB, Adj-RIB-Out)
                didn't quite make it into BGP SPF.  It looks like the LSNDB may correspond
                to the Adj-RIB-In, and there is a Local RIB.  The non-existence of an
                Adj-RIB-Out implies changes.  [Note that it is obviously ok to not follow
                the same structure -- what needs to be explained are the differences.]

Since we have completely replaced the base BGP decision process, we don't use these
terms.

                - Related to the Adj-RIB-Out not existing is the very unnatural and forced
                way in which the text tries to retrofit the new SPF mechanism into the BGP
                Decision Process.  Why are you trying to do that?  Isn't it easier to simply
                point out that the SPF replaces the Decision Process (??9.1.*).  The
                Update-Send Process (??9.2) is described in terms of the Adj-RIB-Out, so it
                doesn't apply...the text already talks about the setting of the route
                advertisement timers (from ??9.2.1.1).  The rest of ??9.2.2, ??9.3 and ??9.4
                don't seem to apply -- but do we need to replace them with anything?  Please
                be explicit.

We believe this is clear in the updated text.

                - About the attributes...  In ??5.1 you mention that attributes that "would
                influence the Decision process...are ignored".

                What about other attributes, the ones that don't influence the Decision
                Process?  If all the information is encoded in the NLRI or carried in the
                BGP-LS Attribute, does BGP SPF need any other attribute?  Maybe requiring
                that other attributes not to be included (especially the mandatory ones) is
                too much, but can it be recommended?  Given that BGP (and rfc7606) require
                specific actions if the attributes are malformed, it would be bad if we
                apply treat-as-withdraw because of an error in an attribute that is not
                relevant in BGP SPF.  [Note: we need to add this risk to the Security
                Considerations.]

                Note that if a recommendation to not include mandatory attributes is
                present, then a note should be present related to this text in rfc7606:

                d.  If any of the well-known mandatory attributes are not present in
                an UPDATE message, then "treat-as-withdraw" MUST be used.  (Note
                that [RFC4760] reclassifies NEXT_HOP as what is effectively
                discretionary.)

We clarified the usage of the attributes in section 2. Error handling is covered in
section 7.

                - Are there any cases that would result in a BGP SPF-specific session reset?
                Does the NOTIFICATION message need new error codes/sub-codes?

Error handling is addressed in section 7.

                - It seems that the FSM applies, are there any exceptions?

                - rfc8212 Updates rfc4271, so it is part of the BGP Base.  Even though that
                RFC is intended at the use of BGP in the Internet, it doesn't specify any
                exceptions.  I would think that you don't want BGP SPF users to have to
                configure policies (for eBGP connections).  It is also important to clearly
                define the concept of domain in the text.

This is defined in the terminology section.

                - It seems that there is no concept of multiple flooding domains in BGP SPF.
                Given the scenario of single-hop eBGP sessions, I'm assuming that if
                multiple domains exist in the future they won't be determined by ASNs.

Right - single BGP SPF domain in the base version of the protocol.

                - Route-reflectors are mentioned several times and in one of the deployment
                models, but there is no such thing as a RR in BGP SPF: all nodes already
                "reflect" all the routes -- it is part of the synchronization requirement of
                a link-state protocol.  You might need to redefine the terminology.

I guess we could come up with different terminology and are open to suggestions. We
still use the term route-reflector or controller in version 12.

                - Optimal Route Reflection is also mentioned, but it cannot be realized
                without loosing synchronization.

This is removed.

                - Using a Confederation ID is mentioned, which seems ok.  I don't see an
                advantage/reason for having Confederations in a BGP SPF network.  Is there
                one?

Confederations are no longer applicable.

                - ??5 talks about outgoing policies.  Is ORF a valid tool to set those policies?

                (2) Use of the BGP-LS encodings.

                - This document is effectively defining a new AFI/SAFI combination -- yes,
                related to BGP-LS, but not the same.  IOW, you can define which parts of
                rfc7752 apply (if any), beyond the encodings.

We have added section 3 to address the relationship to BGP-LS.

                - Note that rfc7752 treats the data as opaque, not offering any validation.
                This document has to specify validation procedures for the TLVs used, along
                with error correction actions, etc..

We've done this for the applicable BGP-LS TLVs.

                - There are many BGP-LS TLVs defined, but only some will apply to BGP SPF.  We
                need the document to be very clear about that.  Note that rfc7752 says that
                "unrecognized types MUST be preserved and propagated".  Please make it clear
                that any type not required in this document is considered unrecognized and
                SHOULD NOT be included.

We don't want to disallow TLVs which will be utilized in future extensions.

                - I'm assuming that this document uses the same format as in Figure 5/rfc7752
                for the Link-State NLRI (but with the new SAFI), right?  Please explicitly
                point to it.

                - Which NLRI Types are supported?  I guess only 1-4.  What should the receiver
                to if a different type is received?

We don't want to disallow NLRIs which will be utilized in future extensions.

                - Which Protocol-ID should be used for the NLRI?   ??4 suggests 7 (BGP), but I
                think that is not correct (see more there).  What should the receiver do
                with a different value?

We have clarified this in section 5.2.

                - What should the Identifier (in the NLRI header) be set to?

We follow RFC 7752.

                - In a "traditional" link-state protocol, there are a number of LSAs that
                define a node: router LSA, network LSA, etc.  Please describe how a node is
                fully described in BGP SPF -- and how the different NLRIs are associated to
                describe that one node.  This description could serve as an introduction to
                ??4 (with forward references to where each part is discussed), or as a summary
                to that section.

We have defined Self-Originated NLRI in section 6.1.1. It seems this is sufficient and
there is no single NLRI for a node.

                Examples would be very nice.

                - What must be included as a Node Descriptor?  ??4 talks about a possible
                combination of different things (see comments/questions there).  It looks
                like a TLV with an ID is mandatory (see my note about TLV 515), what else?

This is described in section 5.2.

                - What must be included in a Link Descriptor?  Which TLVs are mandatory and
                which ones are optional?  There are some rules in ??3.2.2/rfc7752, do they
                apply?  How should errors be handled in these TLVs?

                - What must be included in a Prefix Descriptor?  Is any TLV mandatory?

                - When using the BGP-LS Attribute, which TLVs are mandatory and which are
                optional for each NLRI?

This is included in throughout section 5.

                - I haven't looked at the differences between rfc7752 and rfc7752bis and
                whether they would have an impact of anything in this document.  Please do
                so.

We've incorporated the error handling from the RFC 7752 BIS document in section 7.

                (3) Link-State

                - How does BGP SPF guarantee that the nodes are synchronized?  Other protocols
                have DBDs/CSNP/etc.  It seems easier in BGP SPF to not exchange all the
                information with a peer than it is in traditional link-state protocols.  Being
                able to easily break synchronization is a vulnerability that should be
                mentioned in the Security Considerations section.

Hmmm... Of your high-level meta-comments, this one is a good point. We will discuss how
to document this difference and whether an optional extension is needed.

                [Line numbers from idnits.]

                ...
                12                     Shortest Path Routing Extensions for BGP Protocol

                [major] So...the reason we have a separate WG is because this document
                is defining a new link state protocol -- yes, it reuses many parts of
                BGP, but it is not just an extension to BGP.  Please use a title that
                reflects this.

Changed draft name.

                ...
                15           Abstract

                17             Many Massively Scaled Data Centers (MSDCs) have converged on
                18             simplified layer 3 routing.  Furthermore, requirements for
                19             operational simplicity have led many of these MSDCs to converge on
                20             BGP as their single routing protocol for both their fabric routing
                21             and their Data Center Interconnect (DCI) routing.  This document
                22             describes a solution which leverages BGP Link-State distribution and
                23             the Shortest Path First (SPF) algorithm similar to Internal Gateway
                24             Protocols (IGPs) such as OSPF.

                [major] "This document describes a solution..."  A solution to what?
                As with the title, let's please position this specification for what
                it is.

We've reworked the abstract.

                ...
                101         1.  Introduction

                [] Too many claims without references.  Are there a min set of BGP
                RFCs that are applicable, as is?

                Include a roadmap of the document.  There are assumptions mentioned
                without proper justification.  If I didn't know what you're trying to
                do I wouldn't know what you're talking about.

This is included in section 1.3.

                103           Many Massively Scaled Data Centers (MSDCs) have converged on
                104           simplified layer 3 routing.  Furthermore, requirements for
                105           operational simplicity have led many of these MSDCs to converge on
                106           BGP [RFC4271] as their single routing protocol for both their fabric
                107           routing and their Data Center Interconnect (DCI) routing.
                108           Requirements and procedures for using BGP are described in [RFC7938].
                109           This document describes an alternative solution which leverages BGP-
                110           LS [RFC7752] and the Shortest Path First algorithm similar to
                111           Internal Gateway Protocols (IGPs) such as OSPF [RFC2328].

                [minor] The first part of the paragraph talks about using BGP and
                requirements that are applicable to the solution described in rfc7938.
                IOW, not applicable to this draft!  Note that depending on rfc7938
                would make it Normative -- I'm not sure what in that document, which
                is generic, would apply here.  Am I missing something?

RFC 7938 shouldn't be normative. We've clarified.

                113           [RFC4271] defines the Decision Process that is used to select routes
                114           for subsequent advertisement by applying the policies in the local
                115           Policy Information Base (PIB) to the routes stored in its Adj-RIBs-
                116           In.  The output of the Decision Process is the set of routes that are
                117           announced by a BGP speaker to its peers.  These selected routes are
                118           stored by a BGP speaker in the speaker's Adj-RIBs-Out according to
                119           policy.

                [] Direct copy from rfc4271...

And there is a reference.

                121           [RFC7752] describes a mechanism by which link-state and TE
                122           information can be collected from networks and shared with external
                123           components using BGP.  This is achieved by defining NLRI advertised
                124           within the BGP-LS/BGP-LS-SPF AFI/SAFI.  The BGP-LS extensions defined
                125           in [RFC7752] makes use of the Decision Process defined in [RFC4271].

                [major] s/ within the BGP-LS/BGP-LS-SPF AFI/SAFI./ using the BGP-LS AFI.

Changed to BGP-LS-SPF SAFI.

                127           This document augments [RFC7752] by replacing its use of the existing
                128           Decision Process.  Rather than reusing the BGP-LS SAFI, the BGP-LS-
                129           SPF SAFI is introduced to insure backward compatibility.  The Phase 1
                130           and 2 decision functions of the Decision Process are replaced with
                131           the Shortest Path First (SPF) algorithm also known as the Dijkstra
                132           algorithm.  The Phase 3 decision function is also simplified since it
                133           is no longer dependent on the previous phases.  This solution avails
                134           the benefits of both BGP and SPF-based IGPs.  These include TCP based
                135           flow-control, no periodic link-state refresh, and completely
                136           incremental NLRI advertisement.  These advantages can reduce the
                137           overhead in MSDCs where there is a high degree of Equal Cost Multi-
                138           Path (ECMPs) and the topology is very stable.  Additionally, using an
                139           SPF-based computation can support fast convergence and the
                140           computation of Loop-Free Alternatives (LFAs) [RFC5286] in the event
                141           of link failures.  Furthermore, a BGP based solution lends itself to
                142           multiple peering models including those incorporating route-
                143           reflectors [RFC4456] or controllers.

                [minor] "augments [RFC7752] by replacing"    This sounds as if you're
                extending the use of BGP-LS, when you're really defining a different
                use for what is specified there.   Maybe something along the lines of
                "uses the TLV/NLRI defined in rfc7752" would make it clearer.

                [minor] "BGP-LS-SPF SAFI is introduced to insure backward
                compatibility"  What does the new BGP-LS-SPF SAFI insure backwards
                compatibility with?  Again, you're taking what BGP-LS defined and
                using a new SAFI to transport it.  IOW, there are no changes to
                rfc7752 and no reason to worry about backwards compatibility.

                [minor] "The Phase 1 and 2 decision functions..."  The first sentence
                says that the Decision Process is replaced, so I fail to understand
                why you want to decompose it into phases vs. simply replacing the
                whole thing.   I'll read further...

We've reorganized the document to describe the relationship to the base BGP
protocol and BGP-LS in separate sections.

                [nit] "This solution avails..."  Maybe start a new paragraph there.
Fixed.

                [major] "support fast convergence and the computation of Loop-Free
                Alternatives (LFAs) [RFC5286] in the event of link failures"  Nice
                claim, but there is no mention of LFAs anywhere in the specification
                part of this document.  rfc5286 may apply as-is to the SPF defined
                here -- please include that information as part of the specification.

This is out of scope.

                [nit] "route-reflectors [RFC4456] or controllers."  From the client
                point-of-view, is there a difference?

No.

                145           Support for Multiple Topology Routing (MTR) as described in [RFC4915]
                146           is an area for further study dependent on deployment requirements.

                [minor] rfc4915 is MTR for OSPF.  How does it apply to a BGP
                transport?  IOW, we don't seem to need this paragraph as is.  It is
                however important to mention which "base" link-state functionality is
                not supported.

We have made it clear that MTR is out of scope.

                148         1.1.  BGP Shortest Path First (SPF) Motivation

                150           Given that [RFC7938] already describes how BGP could be used as the
                151           sole routing protocol in an MSDC, one might question the motivation
                152           for defining an alternate BGP deployment model when a mature solution
                153           exists.  For both alternatives, BGP offers the operational benefits
                154           of a single routing protocol.  However, BGP SPF offers some unique
                155           advantages above and beyond standard BGP distance-vector routing.

                [major] This is another place where the solution here is contrasted
                against rfc7938.  Please keep in mind that we're defining a new
                routing protocol -- one that could be used in place of "normal" BGP,
                but also in place of other things.  If anything, comparison with other
                DC-oriented protocols/implementations might fit better in the
                applicability draft.  However, comparison is almost never a good
                thing.

This is a Data Center alternative to RFC 7938. We can remove it if you insist but
we don't feel it is needed.

                [minor] "For both alternatives, BGP offers the operational benefits of
                a single routing protocol."  Do you explain somewhere how to run both
                modes at the same time?  Mentioning a "single routing protocol" sounds
                nice, but we really have two protocols potentially using the same
                peering.  I have questions/concerns later about that.

The draft makes it clear that this is by SAFI and the BGP-LS-SPF SAFI is
given priority.

                157           A primary advantage is that all BGP speakers in the BGP SPF routing
                158           domain will have a complete view of the topology.  This will allow
                159           support for ECMP, IP fast-reroute (e.g., Loop-Free Alternatives),
                160           Shared Risk Link Groups (SRLGs), and other routing enhancements
                161           without advertisement of addition BGP paths or other extensions.  In
                162           short, the advantages of an IGP such as OSPF [RFC2328] are availed in
                163           BGP.

                [] I think it would be a good idea to differentiate between a "BGP
                speaker" (rfc4271) and a "BGP SPF speaker" (this document).  Please
                take a pass at normalizing the terminology.  In this case s/BGP
                speakers in the BGP SPF routing domain/BGP SPF speakers in the routing
                domain

Good point. We've done this.

                [nit] s/addition BGP paths/additional BGP paths

Fixed.

                [minor] "addition BGP paths or other extensions"  Did you mean to put
                a reference to rfc7911?  Assuming that's what you meant.

Added.

                165           With the simplified BGP decision process as defined in Section 5.1,
                166           NLRI changes can be disseminated throughout the BGP routing domain
                167           much more rapidly (equivalent to IGPs with the proper
                168           implementation).

                [minor] "simplified BGP decision process as defined in Section 5.1"
                ??5.1 seems to talk about Phase 1, but 3 phases are mentioned a few
                paragraphs above.  As I mentioned then, you should consider talking
                about replacing the whole Decision Process, not just parts of it, or
                "simplifying" it.

We didn't change this in -12. It is still a BGP decision process.

                [major] What is a "BGP routing domain"?  It is used in several places,
                but it is not defined anywhere.

It is now defined.

                [minor] "much more rapidly (equivalent to IGPs with the proper
                implementation)."   Not only does this sound like a sales pitch
                ("Faster and Improved!"), but it caveats the claim to "IGPs with the
                proper implementation".  I would ask which are those, etc...but I
                would prefer if you stick to the facts.

Given that the best-path calculation doesn't have to be done before advertisement,
we are sticking with this advantage.

                170           Another primary advantage is a potential reduction in NLRI
                171           advertisement.  With standard BGP distance-vector routing, a single
                172           link failure may impact 100s or 1000s prefixes and result in the
                173           withdrawal or re-advertisement of the attendant NLRI.  With BGP SPF,
                174           only the BGP speakers corresponding to the link NLRI need withdraw
                175           the corresponding BGP-LS Link NLRI.  This advantage will contribute
                176           to both faster convergence and better scaling.

                [minor] "BGP-LS Link NLRI"   To avoid confusion, should we be calling
                this "BGP SPF Link NLRI"?

We fixed this throughout.

                [] "This advantage will contribute to both faster convergence and
                better scaling."  Nice for a marketing brochure.

                178           With controller and route-reflector peering models, BGP SPF
                179           advertisement and distributed computation require a minimal number of
                180           sessions and copies of the NLRI since only the latest version of the
                181           NLRI from the originator is required.  Given that verification of the
                182           adjacencies is done outside of BGP (see Section 2), each BGP speaker
                183           will only need as many sessions and copies of the NLRI as required
                184           for redundancy (e.g., one for the SPF computation and another for
                185           backup).  Functions such as Optimized Route Reflection (ORR) are
                186           supported without extension by virtue of the primary advantages.
                187           Additionally, a controller could inject topology that is learned
                188           outside the BGP routing domain.

                [major] "Given that verification of the adjacencies is done outside of
                BGP (see Section 2)..."  ??2 doesn't talk about "verification", but it
                talks about "connection discovery and liveliness detection".  Are you
                referring to both, or just one part?  It would be clearer if you were
                explicit.

                [minor] "Given that verification of the adjacencies is done outside of
                BGP (see Section 2), each BGP speaker will only need as many sessions
                and copies of the NLRI as required for redundancy (e.g., one for the
                SPF computation and another for backup)."

                Hmmm -- are you saying that the number of sessions and updates depends
                on how the session is verified?  I can't wait to get to ??2!

Please suggest text for section 4.

                [major] "Functions such as Optimized Route Reflection (ORR) are
                supported without extension by virtue of the primary advantages."
                draft-ietf-idr-bgp-optimal-route-reflection talks about modification
                of the Decision Process, but you're replacing it in this document.  I
                mean to say that ORR (as defined in the draft) cannot be supported
                with BGP SPF (because it doesn't support the Decision Process as
                described in rfc4271).  If you want to talk about the functionality,
                please explain what it is and explicitly how BGP SPF could achieve a
                similar result.  Note that ORR, from a BGP SPF point of view, relies
                on not advertising the full database to all the neighbors, resulting
                in not being synchronized on purpose...which would go against the
                "primary advantage...that all BGP speakers...will have a complete view
                of the topology".

We've removed ORR.

                190           Given that controllers are already consuming BGP-LS NLRI [RFC7752],
                191           reusing for the BGP-LS SPF leverages the existing controller
                192           implementations.

                [major] At some point you need to be explicit about the relationship
                between rfc7752 and this document.  Can all the BGP-LS information be
                reused?

                [minor] "reusing for the BGP-LS SPF"  Reusing what?  The "BGP-LS SPF"
                what?  Are we calling the new protocol "BGP SPF" or "BGP-LS SPF"..?

We have added a section for this.

                194           Another potential advantage of BGP SPF is that both IPv6 and IPv4 can
                195           be supported in the same address family using the same topology.
                196           Although not described in this version of the document, multi-
                197           topology extensions can be used to support separate IPv4, IPv6,
                198           unicast, and multicast topologies while sharing the same NLRI.

                [minor] "IPv6 and IPv4 can be supported in the same address family"
                This sounds as if you're calling IPv4/IPv6 the same AF...when you're
                really referring to the transport of the information.  Please put a
                forward reference to where this claim is specified.

We will add this reference in the next revision.

                [minor] "Although not described in this version of the document..."
                If not described, please don't mention them without a reference.

These would be "future" documents.

                200           Finally, the BGP SPF topology can be used as an underlay for other
                201           BGP address families (using the existing model) and realize all the
                202           above advantages.  A simplified peering model using IPv6 link-local
                203           addresses as next-hops can be deployed similar to [RFC5549].

                [major] "similar to [RFC5549]"  Similar?  Please put a forward
                reference to where this is specified/explained.

We've removed.

                ...
                213         2.  BGP Peering Models

                215           Depending on the requirements, scaling, and capabilities of the BGP
                216           speakers, various peering models are supported.  The only requirement
                217           is that all BGP speakers in the BGP SPF routing domain receive link-
                218           state NLRI on a timely basis, run an SPF calculation, and update
                219           their data plane appropriately.  The content of the Link NLRI is
                220           described in Section 4.2.

                [major] "Depending on the requirements, scaling, and capabilities of
                the BGP speakers, various peering models are supported."  I hope that
                the applicability draft talks about operational/deployment
                considerations when selecting the peering model.

It is out of scope for this document.

                [nit] "only requirement"  More than one is listed.

Fixed.

                [major] "receive link-state NLRI on a timely basis"  Is "timely"
                specified somewhere else?

                222         2.1.  BGP Single-Hop Peering on Network Node Connections

                224           The simplest peering model is the one described in section 5.2.1 of
                225           [RFC7938].  In this model, EBGP single-hop sessions are established
                226           over direct point-to-point links interconnecting the SPF domain
                227           nodes.  For the purposes of BGP SPF, Link NLRI is only advertised if
                228           a single-hop BGP session has been established and the Link-State/SPF
                229           address family capability has been exchanged [RFC4790] on the
                230           corresponding session.  If the session goes down, the corresponding
                231           Link NLRI will be withdrawn.  Topologically, this would be equivalent
                232           to the peering model in [RFC7938] where there is a BGP session on
                233           every link in the data center switch fabric.

                [major] This reference to rfc7938 makes it Normative.  Is that what
                you really want, to depend on the description of this peering model on
                rfc7938?  I would prefer to see the paragraph reworded to make rfc7938
                an example.

                For example>
                In this model, EBGP single-hop sessions are established over direct
                point-to-point links interconnecting the SPF domain nodes.  Link NLRI is
                only advertised if...   Topologically, this would be similar to the
                peering model in section 5.2.1 of [RFC7938] where there is a BGP session
                on every link.

Fixed as suggested.

                [minor] "address family capability has been exchanged [RFC4790]"   A
                forward reference to Section 3 would be better.  BTW, what is rfc4790?
                ;-)
This was meant to be RFC 4760.

                [nit] "For the purposes of BGP SPF..."  Is redundant: this whole
                document is talking about it.

Removed.

                235         2.2.  BGP Peering Between Directly Connected Network Nodes

                237           In this model, BGP speakers peer with all directly connected network
                238           nodes but the sessions may be multi-hop and the direct connection
                239           discovery and liveliness detection for those connections are
                240           independent of the BGP protocol.  How this is accomplished is outside
                241           the scope of this document.  Consequently, there will be a single
                242           session even if there are multiple direct connections between BGP
                243           speakers.  For the purposes of BGP SPF, Link NLRI is advertised as
                244           long as a BGP session has been established, the Link-State/SPF
                245           address family capability has been exchanged [RFC4790] and the
                246           corresponding link is considered is up and considered operational.
                247           This is much like the previous peering model only peering is on a
                248           single loopback address and the switch fabric links can be
                249           unnumbered.  However, there will be the same number of sessions as
                250           with the previous peering model unless there are parallel links
                251           between switches in the fabric.

                [nit] "How this is accomplished is outside the scope of this
                document."  This sentence is redundant...

Removed.

                [minor] "Link NLRI is advertised as long as a BGP session has been
                established...and the corresponding link is considered is up and
                considered operational."   Is there criteria somewhere about
                considering the link operational?  How is a BGP session paired with a
                "corresponding link" -- are there cases when the session would be up
                but the link won't?

Link discovery and liveliness are outside of BGP. The sessions used to advertise the
BGP-LS-SPF link-state is independent.

                [major] The reference to rfc4790 is out of place.

Fixed to RFC 4760.

                [minor] "unless there are parallel links"  Related to the previous
                comment: it seems to me that the statement about a "corresponding
                link" being up/operational and the potential existence of multiple
                links is somewhat at odds.

How so? There is separate BGP-LS-SPF state for each link.

                [nit] s/link is considered is up and considered operational/link is
                considered is up and operational

Fixed.

                253         2.3.  BGP Peering in Route-Reflector or Controller Topology

                255           In this model, BGP speakers peer solely with one or more Route
                256           Reflectors [RFC4456] or controllers.  As in the previous model,
                257           direct connection discovery and liveliness detection for those
                258           connections are done outside the BGP protocol.  More specifically,
                259           the Liveliness detection is done using BFD protocol described in
                260           [RFC5880].  For the purposes of BGP SPF, Link NLRI is advertised as
                261           long as the corresponding link is up and considered operational.

                [minor] "direct connection discovery"  I would assume that most of the
                connections to a RR are not direct.

Again, the BGP-LS-SPF link-state and the sessions with the controller or RR are
indenpendent.

                [major] "Liveliness detection is done using BFD"  Is this a
                requirement?  Just like any other BGP session, BFD can be used, but it
                doesn't have to, right?  If not required, please word this sentence
                with BFD as an example.

Fixed.

                [nit] s/using BFD protocol/using the BFD protocol

Fixed.

                [nit] "...Link NLRI is advertised as long as the corresponding link is
                up and considered operational."  The other sub-sections also mentioned
                that the BGP session had to be established and the capability
                exchanged, but not here.  Why?  To be honest, I think that mentioning
                all of it (session up, capability exchanged and link up) is obvious
                and not needed.

We are not establishing BGP sessions on all the links.

                263           This peering model, known as sparse peering, allows for many fewer
                264           BGP sessions and, consequently, instances of the same NLRI received
                265           from multiple peers.  It is discussed in greater detail in
                266           [I-D.ietf-lsvr-applicability].

                [nit] s/many fewer/fewer

Fixed.

                [major] Isn't there greater risk in delaying information?  Maybe I
                need to go read the other document...

This is topology independent and, in any case, it will be delayed less than with
base BGP given that it not required to run the best-path prior to readvertising the NLRI.

                268         3.  BGP-LS Shortest Path Routing (SPF) SAFI

                270           In order to replace the Phase 1 and 2 decision functions of the
                271           existing Decision Process with an SPF-based Decision Process and
                272           streamline the Phase 3 decision functions in a backward compatible
                273           manner, this draft introduces the BGP-LS-SFP SAFI for BGP-LS SPF
                274           operation.  The BGP-LS-SPF (AFI 16388 / SAFI TBD1) [RFC4790] is
                275           allocated by IANA as specified in the Section 6.  A BGP speaker using
                276           the BGP-LS SPF extensions described herein MUST exchange the AFI/SAFI
                277           using Multiprotocol Extensions Capability Code [RFC4760] with other
                278           BGP speakers in the SPF routing domain.

                [minor] "replace the Phase 1 and 2...and streamline the Phase 3"
                Again, aren't we replacing the whole Decision Process?

                [minor] "Phase 3 decision functions in a backward compatible manner"
                Again, with what?

This is clarified.

                [major] "SAFI TBD1"  Aren't there implementations already?  What SAFI
                value are they using, a private one?

Fixed all references.

                [major] The reference to rfc4790 is completely out of place.

This is removed.

                []
                OLD>
                A BGP speaker using the BGP-LS SPF extensions described herein MUST
                exchange the AFI/SAFI using Multiprotocol Extensions Capability Code
                [RFC4760] with other BGP speakers in the SPF routing domain.

                NEW>
                In order for two BGP speakers to exchange BGP SPF NLRI, they MUST exchange
                the Multiprotocol Extensions Capability [rfc5492] [rfc4760] to ensure that
                they are both capable of properly processing such NLRI.  This is done with
                AFI 16388 / SAFI TBDx for BGP-LS-SPF.

Suggested text incorporated.

                [major] Can the BGP SPF session also support other AFI/SAFI pairs?  If
                so, what are the implications/limitations because of the peering
                models.  Should this document (similar to rfc7752bis)
                recommend/require the isolation of BGP SPF in it's own BGP session?
                Why/why not?  IMHO, it should be required.

We didn't cover this in the -12 version.

                280         4.  Extensions to BGP-LS

                [] Are we extending BGP-LS or just using parts of it?

Section 5 in the new version.

                282           [RFC7752] describes a mechanism by which link-state and TE
                283           information can be collected from networks and shared with external
                284           components using BGP protocol.  It describes both the definition of
                285           BGP-LS NLRI that describes links, nodes, and prefixes comprising IGP
                286           link-state information and the definition of a BGP path attribute
                287           (BGP-LS attribute) that carries link, node, and prefix properties and
                288           attributes, such as the link and prefix metric or auxiliary Router-
                289           IDs of nodes, etc.

                [nit] s/using BGP protocol/using the BGP protocol

Fixed.

                [major] This document reuses the BGP-LS Attribute.  However, rfc7752
                says that it "MUST be ignored for all other address families".  My
                interpretation of that text is that it was not intended to be specific
                to the BGP-LS AFI, but the AFI/SAFI pairs defined there.  This
                interpretation may be too strict -- please include a note related to
                why it is ok to reuse the BGP-LS Attribute here.

The premise of the protocol and the WG is that it is reused. We are using the BGP-LS
AF but have a new SAFI. Hence, we don't violate RFC 7752.

                291           The BGP protocol will be used in the Protocol-ID field specified in
                292           table 1 of [I-D.ietf-idr-bgpls-segment-routing-epe].  The local and
                293           remote node descriptors for all NLRI will be the BGP Router-ID (TLV
                294           516) and either the AS Number (TLV 512) [RFC7752] or the BGP
                295           Confederation Member (TLV 517) [RFC8402].  However, if the BGP
                296           Router-ID is known to be unique within the BGP Routing domain, it can
                297           be used as the sole descriptor.

                [major] "The BGP protocol will be used in the Protocol-ID field
                specified in table 1 of [I-D.ietf-idr-bgpls-segment-routing-epe]."
                Assuming that you mean that the source Protocol-ID will be 7...  Why?
                BGP SPF is propagating information about the local node, why is the
                source protocol not set to Direct?  Note that the information
                advertised doesn't come from a different protocol.

We fixed and removed the reference.

                >From rfc7752: "direct access to interface information and wants to
                advertise a local link, then the Protocol-ID 'Direct' SHOULD be used."

We fixed and removed the reference.

                [minor] "The local and remote node descriptors for all NLRI..."   I'm
                assuming you're talking about the Local Node Descriptors and Remote
                Node Descriptors fields, right?  Please be specific.

This is specified with each type of NLRI.

                [major] Incorrect references.  s/BGP Router-ID (TLV
                516)...[RFC8402]/BGP Router-ID (TLV 516)
                [I-D.ietf-idr-bgpls-segment-routing-epe], and either the AS Number
                (TLV 512) [RFC7752] or the BGP Confederation Member (TLV 517)
                [I-D.ietf-idr-bgpls-segment-routing-epe]

Fixed or removed references.

                [major] Even if it may be obvious to you, please specify when to use
                TLV 512 and when to use TLV 517.
                draft-ietf-idr-bgpls-segment-routing-epe requires the use of TLV 512
                all the time (and include in it the Confederation ASN, if applicable)
                -- is there a reason not to follow the same recommendation?

We're only using TLV 512 now.

                [minor] s/BGP Router ID/BGP Identifier
                That's what rfc6286 uses. Note that the TLV name is ok.

We now use BGP Identifier throughout.

                [major] "if the BGP Router-ID is known to be unique within the BGP
                Routing domain"  How?  Is this a configuration/runtime decision?
                rfc6286 already required that the BGP Identifier be unique within an
                AS.

                [major] "...will be used...will be...can be used..."  Should any of
                this be Normative language?  I prefer to be explicit if these are
                requirements.

                [major] Related... Besides the language not being tight enough, there
                are too many options: 516 + ((512 OR 517) OR nothing).  How does the
                receiver know if what it got (say only TLV 516, for example) is what
                is should have received?

We've clarified this.

                [major] rfc7752 classifies the IGP Router-ID TLV (515) as mandatory.
                Given that BGP SPF is a new IGP protocol, why isn't that used instead?
                You can specify the content as the BGP ID.

This is BGP SPF and we want to use the same BGP identifier as the base protocol.

                [major] For everything specified here, what should a receiver do if
                the information received is not what is expected.

Malformed NLRI is handled in the Error Handling section. NLRI that

                299         4.1.  Node NLRI Usage

                301           The BGP Node NLRI will be advertised unconditionally by all routers
                302           in the BGP SPF routing domain.

                [minor] "BGP Node NLRI"   rfc7752 uses "Node NLRI" -- are you using
                BGP to try and differentiate?
                [major] "will be"  Is this a requirement?  Should Normative language be used?

We've changed to Node NLRI and normative language.

                304         4.1.1.  Node NLRI Attribute SPF Capability TLV

                306           The SPF capability is a new Node Attribute TLV that will be added to
                307           those defined in table 7 of [RFC7752].  The new attribute TLV will
                308           only be applicable when BGP is specified in the Node NLRI Protocol ID
                309           field.  The TBD TLV type will be defined by IANA.  The new Node
                310           Attribute TLV will contain a single-octet SPF algorithm as defined in
                311           [RFC8402].

                [minor] s/new Node Attribute TLV that will be added to those defined
                in table 7 of [RFC7752]./new Node Attribute TLV.
                Note that we're not adding anything to the table in rfc7752 -- in the
                best case we're getting a new TLV code point from a registry.

Fixed.

                [minor] s/The new attribute TLV will only be applicable/The new
                attribute TLV is only applicable

Fixed.

                [nit] s/Protocol ID/Protocol-ID

                [] "BGP is specified in the Node NLRI Protocol ID"  See my comments
                elsewhere about using BGP as the Protocol-ID.

Fixed throughout.

                [major] "The TBD TLV type will be defined by IANA."  I know IANA
                hasn't assigned one yet, use TBDx instead.  Same question about
                implementation -- there is no private use here...

Fixed and early allocation of Arrcus implementation code points requested.

                [major] s/[RFC8402]/[RFC8665]   This should be a new Normative reference.

Fixed.

                313               0                   1                   2                   3
                314               0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
                315               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                316               |              Type             |             Length            |
                317               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                318               | SPF Algorithm |
                319               +-+-+-+-+-+-+-+-+

                [minor] Please add Figure numbers everywhere.

Not done in this version.

                [minor] Please indicate the Type; use TBDx to identify the different
                values to be assigned.

Based on these many comments, we've used the types that are requested.

                [major] Please indicate somewhere that the Length should be.  In this case 1.

Done.

                [] The length indication is not present in any of the other TLVs
                defined.  Please indicate the value there too.

Done.

                321           The SPF Algorithm may take the following values:
                ...
                335           Note that usage of Strict Shortest Path First (SPF) algorithm is
                336           defined in the IGP algorithm registry but usage is restricted to
                337           [I-D.ietf-idr-bgpls-segment-routing-epe].  Hence, its usage for BGP-
                338           LS SPF is out of scope.

                [major] There's no need to repeat the definitions from rfc8665,
                especially given that only one value is supported.  Just say that!

Discussion of strict SPF is removed.

                [major] I don't know where any restriction on algorithm 1 is
                documented (certainly not in
                I-D.ietf-idr-bgpls-segment-routing-epe)...  What about other values?
                If algorithm 1 is the only one supported, and maybe the only one for a
                while (?), why does it need to be signaled?

Discussion of strict SPF is removed.

                340           When computing the SPF for a given BGP routing domain, only BGP nodes
                341           advertising the SPF capability attribute will be included the
                342           Shortest Path Tree (SPT).

                [major] ??5 talks about SPF running only for nodes that advertised the
                same SFP Algorithm as the local node...so just advertising this TLV is
                not enough.  At least add a forward reference to ??5.

Reference will be added in the next revision.

                [major] Related...  If this is a mandatory TLV, please say so.
                However, it seems that a default value (0) be assumed if not present?

Clarified that it is mandatory.

                [minor] s/SPF capability attribute/SPF Capability TLV

Fixed.

                [major] Where is the SPF algorithm specification?  How is the routing
                table calculated from the BGP SPF information?

I guess you are asking for the reference again?

                344         4.1.2.  BGP-LS Node NLRI Attribute SPF Status TLV

                346           A BGP-LS Attribute TLV to BGP-LS Node NLRI is defined to indicate the
                347           status of the node with respect to the BGP SPF calculation.  This
                348           will be used to rapidly take a node out of service or to indicate the
                349           node is not to be used for transit (i.e., non-local) traffic.  If the
                350           SPF Status TLV is not included with the Node NLRI, the node is
                351           considered to be up and is available for transit traffic.

                [?] "A BGP-LS Attribute TLV to BGP-LS Node NLRI..."  What does this
                mean?  My guess is that this TLV is to only be used in the BGP-LS
                Attribute with a Node NLRI.  Is that it?

Well, RFC 7752 specifies attributes for each NLRI type. This is continuing in this
tradition.

                [major] "TLV to BGP-LS Node NLRI is defined"  By mentioning BGP-LS,
                are you implying that this new TLV can be used by an rfc7752
                implementation?  Or is the expectation that it will only be used by a
                BGP SPF SAFI is in use?  Either way, please be explicit.  This type of
                text is present in other sections.

                [] Using BGP-LS sometimes is getting confusing.

We use BGP-LS-SPF rather than BGP-LS throughout.

                353               0                   1                   2                   3
                354               0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
                355               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                356               |   TBD Type    |                       Length                  |
                357               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                358               | SPF Status    |
                359               +-+-+-+-+-+-+-+-+

                [major] The TLV format defined in rfc7752 is:

                0                   1                   2                   3
                0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
                +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                |              Type             |             Length            |
                +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                //                        Value (variable)                     //
                +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

                [] There are other places where the TLV format is not followed either.

This is now fixed throughout. The engineer implementing BGP SPF ignored these problems but
neglected to make the comment.

                361               BGP Status Values: 0 - Reserved
                362                                   1 - Node Unreachable with respect to BGP SPF
                363                                   2 - Node does not support transit with respect
                364                                       to BGP SPF
                365                               3-254 - Undefined
                366                                 255 - Reserved

                [major] What should the receiving node do with this information?  I
                understand how the overload bit/max-metric work -- I'm asking for a
                specification in this new protocol document.  A forward reference
                would be good.

Forward reference added.

                [minor] When does the status take effect?  Immediately?  Is there a
                "graceful shutdown" concept?

                [major] What should the receiver do if an unknown value is received?

Error handling is not covered.

                368         4.2.  Link NLRI Usage
                ...
                373           Link NLRI is advertised with local and remote node descriptors as
                374           described above and unique link identifiers dependent on the
                375           addressing.  For IPv4 links, the links local IPv4 (TLV 259) and
                376           remote IPv4 (TLV 260) addresses will be used.  For IPv6 links, the
                377           local IPv6 (TLV 261) and remote IPv6 (TLV 262) addresses will be
                378           used.  For unnumbered links, the link local/remote identifiers (TLV
                379           258) will be used.  For links supporting having both IPv4 and IPv6
                380           addresses, both sets of descriptors may be included in the same Link
                381           NLRI.  The link identifiers are described in table 5 of [RFC7752].

                [major] "...links supporting having both IPv4 and IPv6 addresses, both
                sets of descriptors may be included in the same Link NLRI."  Is it ok
                for one side of the link to advertise just one Link NLRI for a link
                (including both IPv4 and IPv6) while the other side advertises 2 Link
                NLRIs (one for IPv4 and the other for IPv6)?   If so, is there an
                expected default behavior?  Is one Link NLRI an optional behavior, or
                the default?  Why/when would a node choose to deviate from the
                default?

This is described as well as the applicability to the BGP SPF calcuation.

                383           The link IGP metric attribute TLV (TLV 1095) as well as any others
                384           required for non-SPF purposes SHOULD be advertised.  The metric value
                385           in this TLV is variable length dependent on specific protocol usage
                386           (refer to section 3.3.2.4 in [RFC7752]).  For simplicity, the BGP-LS
                387           SPF metric length will be 4 octets.  Algorithms such as setting the
                388           metric inversely to the link speed as done in the OSPF MIB [RFC4750]
                389           MAY be supported.  However, this is beyond the scope of this
                390           document.

                [minor] s/link IGP metric attribute TLV/IGP Metric TLV
                That's what rfc7752 calls it.

Fixed.

                [major] "TLV 1095...SHOULD be advertised"  When is it ok to not
                advertise the metric?  IOW, why isn't it required?

Fixed.

                [nit] s/any others/any other

Fixed.

                [major] "others required for non-SPF purposes SHOULD be advertised"
                What other TLVs?  Given that you're making this behavior Normative,
                you should provide more information.  BTW, what other non-SPF
                purposes?  This phrase is also used in ??4.3, but there's no
                explanation as to why this new protocol would carry non-SPF stuff, or
                what what may be.  Recommendation: leave the non-SPF part out.

This is removed.

                [minor]
                OLD>
                The metric value in this TLV is variable length dependent on specific
                protocol usage (refer to section 3.3.2.4 in [RFC7752]).  For simplicity,
                the BGP-LS SPF metric length will be 4 octets.

                NEW>
                The BGP SPF metric length is 4 octets.

Suggested text incorporated..

                [major] "Algorithms such as setting the metric inversely to the link
                speed as done in the OSPF MIB [RFC4750] MAY be supported.  However,
                this is beyond the scope of this document."

                Wait, what?!?  Setting, or even defining, a metric is out of scope?

                Ok, let's go with that for a second.  Is there any guidance or default
                settings that can be mentioned?  For example, I found this in rfc2328:

                A cost is associated with the output side of each router
                interface.  This cost is configurable by the system
                administrator.  The lower the cost, the more likely the
                interface is to be used to forward data traffic.  Costs are
                also associated with the externally derived routing data
                (e.g., the BGP-learned routes).

                The text should at least say that the cost is configurable before
                offering optional (MAY) implementations.

                What about consistency?  Does it matter for the SPF calculation if the
                nodes don't consistently calculate the local metrics the same way?  Is
                it ok for some nodes to use rfc4750, while others are manually
                configured with the inverse to that, just as an example?  Clearly
                there will be an impact and the manually configured links will be less
                preferred.  Can you provide operational guidance on what to consider
                when selecting and deploying an algorithm?

                I would really, really like to see a default mechanism defined.  While
                I'm assuming that most deployments will be form the same vendor, it
                would be nice for this new protocol to be interoperable out of the
                box.

We've clarified this as requested but do not specify a default metric.

                392         4.2.1.  BGP-LS Link NLRI Attribute Prefix-Length TLVs

                394           Two BGP-LS Attribute TLVs to BGP-LS Link NLRI are defined to
                395           advertise the prefix length associated with the IPv4 and IPv6 link
                396           prefixes.  The prefix length is used for the optional installation of
                397           prefixes corresponding to Link NLRI as defined in Section 5.3.

                [?] Where do the prefixes come from?

Clarified - "derived from the link descriptor addresses."

                [major] rfc7752 defines the IP Reachability Information TLV, which is
                mandatory and also includes the prefix length (and the prefix itself).
                Why isn't that one reused?

This is for Prefix NLRI.

                399               0                   1                   2                   3
                400               0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
                401               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                402               |      TBD IPv4 or IPv6 Type    |             Length            |
                403               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
                404               | Prefix-Length |
                405               +-+-+-+-+-+-+-+-+

                [minor] Indicate the different Types separately: TBD4 and TBD6, for example.

Fixed.

                407                 Prefix-length - A one-octet length restricted to 1-32 for IPv4
                408                                 Link NLRI endpoint prefixes and 1-128 for IPv6
                409                                 Link NLRI endpoint prefixes.

                [major] What should the receiver do if the Prefix-length is invalid?
                For example, if 44 is received for IPv4.

Error handling is added.

                411         4.2.2.  BGP-LS Link NLRI Attribute SPF Status TLV

                413           A BGP-LS Attribute TLV to BGP-LS Link NLRI is defined to indicate the
                414           status of the link with respect to the BGP SPF calculation.  This
                415           will be used to expedite convergence for link failures as discussed
                416           in Section 5.6.1.  If the SPF Status TLV is not included with the
                417           Link NLRI, the link is considered up and available.

                [major] What is the relationship between this TLV and the (other) SPF
                Status TLV from ??4.1.2?  For example, if this TLV is not included
                ("the link is considered up and available"), and the (other) SPF
                Status TLV indicates something else, which one has priority?

There interpretation of the status TLV is dependent on the NLRI type. We will discuss this
further.

                ...
                427               BGP Status Values: 0 - Reserved
                428                                   1 - Link Unreachable with respect to BGP SPF
                429                               2-254 - Undefined
                430                                 255 - Reserved

                [major] What should the receiver do if an unknown value is received?

Error handling is added.

                432         4.3.  Prefix NLRI Usage

                [minor] rfc7752 defines 2 Prefix NLRI types, the IPv4 Topology Prefix
                NLRI and the IPv6 Topology Prefix NLRI.  I'm assuming that by "Prefix
                NLRI" you mean both.  Please write that somewhere.

Changed name of section.

                434           Prefix NLRI is advertised with a local node descriptor as described
                435           above and the prefix and length used as the descriptors (TLV 265) as
                436           described in [RFC7752].  The prefix metric attribute TLV (TLV 1155)
                437           as well as any others required for non-SPF purposes SHOULD be
                438           advertised.  For loopback prefixes, the metric should be 0.  For non-
                439           loopback prefixes, the setting of the metric is a local matter and
                440           beyond the scope of this document.

                [minor] s/local node descriptor/Local Node Descriptor
                That's the name of the field as defined in rfc7752.  Please be
                consistent with all names.

Fixed.

                OLD>
                Prefix NLRI is advertised with a local node descriptor as described
                above and the prefix and length used as the descriptors (TLV 265) as
                described in [RFC7752].

                NEW>
                The Prefix NLRI is advertised with the Local Node Descriptor as described
                above (Section 4).  The Prefix Descriptors field includes the IP
                Reachability Information TLV (TLV 265) as described in [RFC7752].

Suggested text incorporated.

                [minor] s/The prefix metric attribute TLV/The Prefix Metric TLV

Will be included in next revision.

                [major] "TLV 1155...SHOULD be advertised"  When is it ok to not
                advertise the metric?  IOW, why isn't it required?

Changed to MUST.

                [nit] s/any others/any other

Removed.

                442         4.3.1.  BGP-LS Prefix NLRI Attribute SPF Status TLV

                [] Wow!  This is the third SPF Status TLV!

                444           A BGP-LS Attribute TLV to BGP-LS Prefix NLRI is defined to indicate
                445           the status of the prefix with respect to the BGP SPF calculation.
                446           This will be used to expedite convergence for prefix unreachability
                447           as discussed in Section 5.6.1.  If the SPF Status TLV is not included
                448           with the Prefix NLRI, the prefix is considered reachable.

                [major] What is the relationship of this TLV with the other SPF Status
                TLVs?  Which one has priority?

The TLV is NLRI specific has the same code point. We will discuss.

                ...
                458               BGP Status Values: 0 - Reserved
                459                                   1 - Prefix down with respect to SPF
                460                               2-254 - Undefined
                461                                 255 - Reserved

                [minor] The other SPF Status TLVs use "Unreachable" instead of "down"
                to describe value 1.  Is there a difference in the meaning?  It would
                be nice to be consistent.

Updated.

                463         4.4.  BGP-LS Attribute Sequence-Number TLV

                465           A new BGP-LS Attribute TLV to BGP-LS NLRI types is defined to assure
                466           the most recent version of a given NLRI is used in the SPF
                467           computation.  The TBD TLV type will be defined by IANA.  The new BGP-
                468           LS Attribute TLV will contain an 8-octet sequence number.  The usage
                469           of the Sequence Number TLV is described in Section 5.1.

                [major] I assume that this TLV is mandatory for BGP SPF.  Please say so.

Yes.

                ...
                483           The 64-bit strictly increasing sequence number is incremented for
                484           every version of BGP-LS NLRI originated.  BGP speakers implementing
                485           this specification MUST use available mechanisms to preserve the
                486           sequence number's strictly increasing property for the deployed life
                487           of the BGP speaker (including cold restarts).  One mechanism for
                488           accomplishing this would be to use the high-order 32 bits of the
                489           sequence number as a wrap/boot count that is incremented anytime the
                490           BGP router loses its sequence number state or the low-order 32 bits
                491           wrap.

                [major] "sequence number is incremented"  Should this behavior be
                Normative?  s/is/MUST ?

Fixed.

                [major] "for every version of BGP-LS NLRI originated"   There are 3
                NLRI Types. If only one NLRI type is originated, should the Sequence
                Number change for the others?  Does this imply that the other NLRIs
                need to be updated too?

No - each NLRI is separate. I think we need to clarify the packing.

                493           When incrementing the sequence number for each self-originated NLRI,
                494           the sequence number should be treated as an unsigned 64-bit value.
                495           If the lower-order 32-bit value wraps, the higher-order 32-bit value
                496           should be incremented and saved in non-volatile storage.  If by some
                497           chance the BGP Speaker is deployed long enough that there is a
                498           possibility that the 64-bit sequence number may wrap or a BGP Speaker
                499           completely loses its sequence number state (e.g., the BGP speaker
                500           hardware is replaced or experiences a cold-start), the phase 1
                501           decision function (see Section 5.1) rules will insure convergence,
                502           albeit, not immediately.

                [] "If by some chance the BGP Speaker is deployed long enough..."  I
                love positive thinking. :-(

This is the law of large numbers - it will never happen. We will remove this
statement in the next revision. The range is 0 - 18446744073709551615. At one
self-origination per milli-second, it will still takes 10's of millions of years.

                504         5.  Decision Process with SPF Algorithm
                ...
                516           The SPF based Decision process replaces the BGP best-path Decision
                517           process described in [RFC4271].  This process starts with selecting
                518           only those Node NLRI whose SPF capability TLV matches with the local
                519           BGP speaker's SPF capability TLV value.  Since Link-State NLRI always
                520           contains the local descriptor [RFC7752], it will only be originated
                521           by a single BGP speaker in the BGP routing domain.  These selected
                522           Node NLRI and their Link/Prefix NLRI are used to build a directed
                523           graph during the SPF computation.  The best paths for BGP prefixes
                524           are installed as a result of the SPF process.

                [minor] s/best-path Decision process/Decision Process
                rfc4271 doesn't use the term best-path.

                [nit] "best-path" is used later to describe the BGP SPF process.  That
                is ok, but you may want to consider "best route" too.

These are fixed. I believe "best-path" is an artifact of a Cisco implementation.

                [minor] "contains the local descriptor [RFC7752]"  I think you're
                pointing at rfc7752 because that's where the formats are defined.  You
                should (also) point at ??4 where the contents of the descriptor are
                explained.

We've updated.

                [minor] "...it will only be originated by a single BGP speaker in the
                BGP routing domain"   I had to read this several times.  It sounds as
                if you're trying to say that each set of NLRI is only originated by
                one node, is that it?  If so, that is not true: multiple nodes (maybe
                rogue ones) may originate information that doesn't belong to them,
                even an exact copy of a different node (with or without the same Local
                Node Descriptor).  I think a better/true statement could be something
                like:  Because the Link-State NLRIs always contain a Local node
                Descriptor [], it could be used to identify the originating BGP
                speaker in the domain.

We believe we've explained this better.

                [minor] "selected Node NLRI and their Link/Prefix NLRI"  I assume
                you'll explain later how they're selected...a forward reference would
                be nice.

Hopefully, the description will suffice.

                526           When BGP-LS-SPF NLRI is received, all that is required is to
                527           determine whether it is the best-path by examining the Node-ID and
                528           sequence number as described in Section 5.1.  If the received best-
                529           path NLRI had changed, it will be advertised to other BGP-LS-SPF
                530           peers.  If the attributes have changed (other than the sequence
                531           number), a BGP SPF calculation will be scheduled.  However, a changed
                532           NLRI MAY be advertised to other peers almost immediately and
                533           propagation of changes can approach IGP convergence times.  To
                534           accomplish this, the MinRouteAdvertisementIntervalTimer and
                535           MinASOriginationIntervalTimer [RFC4271] are not applicable to the
                536           BGP-LS-SPF SAFI.  Rather, SPF calculations SHOULD be triggered and
                537           dampened consistent with the SPF back-off algorithm specified in
                538           [RFC8405].

                [minor] s/When BGP-LS-SPF NLRI is received/When a BGP SPF UPDATE is received

We dont' really believe this is an improvement. We use the base BGP update message. It is
BGP-LS-SPF SAFI that is unique.

                [] BGP SPF and BGP-LS-SPF are used throughout for apparently the same
                thing.  Please be consistent.

BGP-LS-SPF is used consistently.

                [major] This paragraph sounds out of place to me.  It seems to
                describe what to do if an UPDATE containing the current best-path is
                received...but you haven't explained the details yet.  Perhaps move
                this to some place later.

This is an overview of the process. It belongs where it is.

                [] "...determine whether it is the best-path by examining the Node-ID
                and sequence number..."  This phrase sounds incomplete/wrong, I'm sure
                ??5.1 will provide more details.

Yes - this is an overview.

                [minor] s/If the received best-path NLRI had changed/If the best-path changes

No. That would be incorrect.

                [major] "BGP SPF calculation will be scheduled"  When?  s/will/MUST ??
                Should there be SPF timers (basic ones) defined somewhere?

                [major] "MAY be advertised to other peers almost immediately"  It
                sounds as if the optional part is not the advertisement, but doing it
                "almost immediately"; how fast is that?  Also, is the advertisement
                optional?  The action is what should carry the Normative language.

                [minor] "propagation of changes can approach IGP convergence times"
                This phrase sounds out of place: this document is a specification, not
                the place to compare this protocol with others.  BTW, I'm assmuming
                you consider BGP SPF an IGP, right?

Not exactly. It is BGP SPF.

                [major] Given that these are timers in rfc4271:
                s/MinRouteAdvertisementIntervalTimer and MinASOriginationIntervalTimer
                [RFC4271] are not applicable/MinRouteAdvertisementIntervalTimer and
                MinASOriginationIntervalTimer [RFC4271] MUST be set to 0

No - if there aren't applicable, they don't need to be set to anything.

                [minor] s/SHOULD be triggered and dampened consistent with the SPF
                back-off algorithm/SHOULD be triggered and dampened according to the
                SPF back-off algorithm

                [major] "SHOULD...SPF back-off algorithm"  Is rfc8405 recommended (and
                not required) because it is optional in a BGP SPF implementation, or
                because there are cases when it should not be used?  If the former,
                then you should use MAY instead; if the latter, then please mention
                when it wouldn't be used.

                [] In the description of the SPF algorithm, it would be nice to point
                out where rfc8405 would be applicable.

This has been re-written.

                540           The Phase 3 decision function of the Decision Process [RFC4271] is
                541           also simplified since under normal SPF operation, a BGP speaker would
                542           advertise the NLRI selected for the SPF to all BGP peers with the
                543           BGP-LS/BGP-LS-SPF AFI/SAFI.  Application of policy would not be
                544           prevented however its usage to best-path process would be limited as
                545           the SPF relies solely on link metrics.

                [major] This seems to be where you (try to) specify Phase 3.  Please
                make it clear that is what you're doing.  Note that ??9.1.3/rfc4271
                talks about more than which routes are advertised.

                [major] "BGP speaker would advertise"  MUST??

Fixed.

                [major] "advertise the NLRI selected for the SPF"  Only that?  ??5.1
                seems to potentially eliminate some NLRI (I have questions about that
                there) -- if all the valid received information is not propagated then
                there is no synchronization, which is needed for a link-state
                protocol.  What am I missing?

                [minor] s/BGP peers with the BGP-LS/BGP-LS-SPF AFI/SAFI /BGP SPF peers

This has been rewritten.

                [minor] I'm not sure what you're trying to say in the last sentence.
                When is policy ok?  What is the relationship between policy (which I
                interpret as filtering routes) and the metrics (beyond obviously not
                propagating anything)?

This is removed.

                547         5.1.  Phase-1 BGP NLRI Selection

                549           The rules for NLRI selection are greatly simplified from [RFC4271].

                [] Are you simplifying or replacing. I think making it clear that
                ??9.1.1/rfc4271 doesn't apply anymore is important.

                [major] Does this set of rules apply per NLRI?  IOW, do they apply to all NLRIs?

It applies to all NLRIs in the BGP-LS-SPF SAFI.

                551           1.  If the NLRI is received from the BGP speaker originating the NLRI
                552               (as determined by the comparing BGP Router ID in the NLRI Node
                553               identifiers with the BGP speaker Router ID), then it is preferred
                554               over the same NLRI from non-originators.  This rule will assure
                555               that stale NLRI is updated even if a BGP-LS router loses its
                556               sequence number state due to a cold-start.

                [minor] s/BGP Router ID/BGP Identifier/g
                That's the name used in rfc6286

Fixed throughout.

                []
                OLD>
                If the NLRI is received from the BGP speaker originating the NLRI
                (as determined by the comparing BGP Router ID in the NLRI Node
                identifiers with the BGP speaker Router ID), then it is preferred
                over the same NLRI from non-originators.

                NEW>
                Routes originated by directly connected BGP SPF peers are preferred.
                This condition can be determined by comparing the BGP Identifiers in
                the received Local Node Descriptor and OPEN message.

Fixed.

                [major] In the case where a prefix is multihomed to more than one BGP
                SPF router...  Let's say that one of those routers is directly
                connected, but the other one isn't, does this first rule mean that the
                receiver wouldn't take the advertisement (of a Link/Prefix NLRI) from
                the non-connected speaker into account?  Even if the topology is such
                that the metric is <=?

No - these are different Prefix NLRI since they are originated by different
BGP SPF routers.

                558           2.  If the Sequence-Number TLV is present in the BGP-LS Attribute,
                559               then the NLRI with the most recent, i.e., highest sequence number
                560               is selected.  BGP-LS NLRI with a Sequence-Number TLV will be
                561               considered more recent than NLRI without a BGP-LS Attribute or a
                562               BGP-LS Attribute that doesn't include the Sequence-Number TLV.

                [major] How can a route without the Sequence-Number TLV even be valid?

Sequence number is now mandatory.

                564           3.  The final tie-breaker is the NLRI from the BGP Speaker with the
                565               numerically largest BGP Router ID.

                []
                NEW>
                The route received from the BGP SPF speaker with the numerically larger BGP
                Identifier is preferred.

Fixed.

                [major] Is this the BGP ID of the directly connected peer, or the originator?

We use the term "BGP Speaker" to make it clear that is the peer - for the NLRI selection
of the same NLRI, the originator is, by definition, the same.

                567           When a BGP speaker completely loses its sequence number state, i.e.,
                568           due to a cold start, or in the unlikely possibility that that
                569           sequence number wraps, the BGP routing domain will still converge.
                570           This is due to the fact that BGP speakers adjacent to the router will
                571           always accept self-originated NLRI from the associated speaker as
                572           more recent (rule # 1).  When BGP speaker reestablishes a connection
                573           with its peers, any existing session will be taken down and stale
                574           NLRI will be replaced by the new NLRI and stale NLRI will be
                575           discarded independent of whether or not BGP graceful restart is
                576           deployed, [RFC4724].  The adjacent BGP speaker will update their NLRI
                577           advertisements in turn until the BGP routing domain has converged.

                [nit] s/When BGP speaker/When a BGP SPF speaker

This was missed and will be fixed in the next revision.

                [minor] "stale NLRI will be replaced by the new NLRI and stale NLRI
                will be discarded"   You mentioned 2 different actions for stale
                routes.

Fixed.

                [major] "whether or not BGP graceful restart is deployed, [RFC4724]"
                Does this mean that GR is not supported in BGP SPF?  It may be clearer
                to simply say that.

Removed.

                579           The modified SPF Decision Process performs an SPF calculation rooted
                580           at the BGP speaker using the metrics from Link and Prefix NLRI
                581           Attribute TLVs [RFC7752].  As a result, any attributes that would
                582           influence the Decision process defined in [RFC4271] like ORIGIN,
                583           MULTI_EXIT_DISC, and LOCAL_PREF attributes are ignored by the SPF
                584           algorithm.  Furthermore, the NEXT_HOP attribute value is preserved
                585           but otherwise ignored during the SPF or best-path.

                [minor] "metrics from Link and Prefix NLRI Attribute TLVs [RFC7752]"
                Maybe be more specific and point to the IGP metric attribute TLV (TLV
                1095) and the prefix metric attribute TLV (TLV 1155).

Added references.

                [major] "ORIGIN, MULTI_EXIT_DISC, and LOCAL_PREF attributes are ignored"

                Given that these attributes are not necessary for BGP SPF, can it be
                specified that they can optionally be omitted from a BGP SPF UPDATE?
                Optionally omitting would allow implementations to reuse more of their
                BGP code (and send them); but it would also allow some optimizations.
                And it doesn't eliminate them completely in case there's some use for
                them in the future.

We say they are ignored by the SPF algorithm. Any future usage would require
a new document anyway.

                [major] What about the AS_PATH?

It is still used for loop detection. Keyur to respond.

                [major] What about other attributes?  Not all influence the BGP
                decision process -- do we need them?  The more unnecessary stuff that
                is carried the more risk.

We address all the RFC 4271 attributes but not any future attibutes.

                [] Please remove the sentence about the NEXT_HOP and focus all
                discussion about it in ??5.4.

We missed this and it will be in the next revision.

                [major] rfc4760 says that if only multi-protocol extensions are used
                then an "UPDATE...SHOULD NOT carry the NEXT_HOP attribute".  If this
                is one of the exceptions, please be explicit about it.

We will add in next revision after discussion. We've added text about following
RFC 4760 but need to address the NEXT_HOP.

                [minor] "the SPF or best-path"  I guess you meant simply the SPF calculation.

We will change this in the next revision.

                587         5.2.  Dual Stack Support

                589           The SPF-based decision process operates on Node, Link, and Prefix
                590           NLRIs that support both IPv4 and IPv6 addresses.  Whether to run a
                591           single SPF instance or multiple SPF instances for separate AFs is a
                592           matter of a local implementation.  Normally, IPv4 next-hops are
                593           calculated for IPv4 prefixes and IPv6 next-hops are calculated for
                594           IPv6 prefixes.  However, an interesting use-case is deployment of
                595           [RFC5549] where IPv6 next-hops are calculated for both IPv4 and IPv6
                596           prefixes.  As stated in Section 1, support for Multiple Topology
                597           Routing (MTR) is an area for future study.

                [nit] s/is deployment/is the deployment

                [minor] "interesting use-case is deployment of [RFC5549]"  Is this
                case covered in the specification of the SPF?  If not, then it may be
                better to not include this mention here...unless it's obvious and I
                missed it.

                [minor] As mentioned in ??1, we don't need to mention MTR again if it's
                not supported.

Reworked this section.

                599         5.3.  SPF Calculation based on BGP-LS NLRI

                601           This section details the BGP-LS SPF local routing information base
                602           (RIB) calculation.  The router will use BGP-LS Node, Link, and Prefix
                603           NLRI to populate the local RIB using the following algorithm.  This
                604           calculation yields the set of intra-area routes associated with the
                605           BGP-LS domain.  A router calculates the shortest-path tree using
                606           itself as the root.  Variations and optimizations of the algorithm
                607           are valid as long as it yields the same set of routes.  The algorithm
                608           below supports Equal Cost Multi-Path (ECMP) routes.  Weighted Unequal
                609           Cost Multi-Path are out of scope.  The organization of this section
                610           owes heavily to section 16 of [RFC2328].

                [minor] s/BGP-LS domain/BGP SPF domain

This will be changed in the next revision.

                [major] "Variations and optimizations of the algorithm are valid as
                long as it yields the same set of routes."  This sounds as if the
                algorithm described here is not normative, just an example.  I expect
                to see a default algorithm -- or at least one who's output describes
                what MUST happen.

Reworded in -12.

                ...
                615           o  Local Route Information Base (RIB) - This is abstract contains
                616               reachability information (i.e., next hops) for all prefixes (both
                617               IPv4 and IPv6) as well as the Node NLRI reachability.
                618               Implementations may choose to implement this as separate RIBs for
                619               each address family and/or Node NLRI.

                [nit] s/This is abstract contains/This abstract structure contains  (?)

                [major] Is this the same as the Loc-RIB from rfc4271?  There is at
                least one place later where the term is used.  If possible, reuse
                terminology concepts.  It would be ideal if a comparison of the terms
                also existed; for example it looks like the LSNDB is equivalent to the
                Adj-RIB-In.

Reworded.

                621           o  Link State NLRI Database (LSNDB) - Database of BGP-LS NLRI that
                622               facilitates access to all Node, Link, and Prefix NLRI as well as
                623               all the Link and Prefix NLRI corresponding to a given Node NLRI.
                624               Other optimization, such as, resolving bi-directional connectivity
                625               associations between Link NLRI are possible but of scope of this
                626               document.

                [minor] This sounds redundant: "access to all Node, Link, and Prefix
                NLRI as well as all the Link and Prefix NLRI corresponding to a given
                Node NLRI"

Simplified.

                [minor] "bi-directional connectivity...possible but of scope of this
                document."  this is specified later in this section.

Removed - an implementation can avoid the check during the calculation if it is
done upon installation. However, that is an optimization.

                628           o  Candidate List - This is a list of candidate Node NLRI with the
                629               lowest cost Node NLRI at the front of the list.  It is typically
                630               implemented as a heap but other concrete data structures have also
                631               been used.

                [major] "list of candidate Node NLRI"  Where does this list come from?
                Is it the outcome of the rules in ??5.1?

The NLRI selected during 5. are eligible to be added. Will add a reference in the next revision.

                [major] "lowest cost"  How is this cost calculated?

This is explained.

                ...
                635           1.  The current local RIB is invalidated.  The local RIB is rebuilt
                636               during the course of the SPF computation.  The existing routing
                637               entries are preserved for comparison to determine changes that
                638               need to be installed in the global RIB.

                [major] Please define the "global RIB".

Added to data structures.

                [minor] "determine changes that need to be installed in"  ...or removed from

Reworded.

                ...
                643           3.  The Node NLRI with the lowest cost is removed from the candidate
                644               list for processing.  If the BGP-LS Node attribute includes an
                645               SPF Status TLV (Section 4.1.2) indicating the node is
                646               unreachable, the Node NLRI is ignored and the next lowest cost
                647               Node NLRI is selected from candidate list.  The Node
                648               corresponding to this NLRI will be referred to as the Current
                649               Node.  If the candidate list is empty, the SPF calculation has
                650               completed and the algorithm proceeds to step 6.

                [minor] "removed from the candidate list"  When was that list initialized?

Now initialized in step 1.

                ...
                662               *  If the prefix is in the local RIB and the cost is greater than
                663                   the Current route's metric, the Prefix NLRI does not
                664                   contribute to the route and is ignored.

                [major] I'm not sure if I'm reading this text correctly.  It sounds
                like this: if the cost of the prefix in the local RIB is > the route
                being considered, then ignore the route being considered.   But it
                should be the other way around, right?

                As I read this step, and the next one...  What is the "current route"?
                Is that the route in the global RIB?   Also, does "consider for
                installation" mean that the prefix is installed in the local RIB (but
                not the global RIB, yet)?  If so, then I guess the text makes sense.

                But it also makes sense if you're covering the case where there's a
                prefix from another node in the local RIB, and the route being
                considered has a higher metric than the one in the global RIB, so it
                is ignored.  Of course in this case there is no comparison with the
                router in the local RIB, but I have to then assume that it is there
                because the metric is <= to the global RIB...

                I'm just guessing...  Please clarify.  We don't want other people guessing too.

                666               *  If the prefix is in the local RIB and the cost is less than
                667                   the current route's metric, the Prefix is installed with the
                668                   Current Node's next-hops replacing the local RIB route's next-
                669                   hops and the metric being updated.

                [major] see comment above...

                [major] Where do the "Current Node's next-hops" come from?  The text
                is missing how that part is calculated/advertised...

                671               *  If the prefix is in the local RIB and the cost is same as the
                672                   current route's metric, the Prefix is installed with the
                673                   Current Node's next-hops being merged with local RIB route's
                674                   next-hops.

                [] see comment above...

This entire section has been rewritten.

                676           5.  All the Link NLRI with the same Node Identifiers as the Current
                677               Node will be considered for installation.  Each link will be
                678               examined and will be referred to in the following text as the
                679               Current Link.  The cost of the Current Link is the advertised
                680               metric in the Link NLRI added to the cost to reach the Current
                681               Node.

                [] This text clarifies the meaning of "Current Link" upfront. :-)

                [nit] Some of the text capitalizes "Current Link" (Node, etc.), but it
                is not written the same way in other places.  Please be consistent.

                683               *  Optionally, the prefix(es) associated with the Current Link
                684                   are installed into the local RIB using the same rules as were
                685                   used for Prefix NLRI in the previous steps.

                [major] "Optionally"  When would they be installed?  When wouldn't
                they be installed?

                ...
                692               *  The Current Link's endpoint Node NLRI is accessed (i.e., the
                693                   Node NLRI with the same Node identifiers as the Link
                694                   endpoint).  If it exists, it will be referred to as the
                695                   Endpoint Node NLRI and the algorithm will proceed as follows:

                [minor] "If it exists..."  It *has* to exist!  The start of step 5
                says that we're working with "the Link NLRI with the same Node
                Identifiers as the Current Node".  I guess there could be a race
                condition where the Node NLRI is not available anymore...but I hope
                that SPF is run with the available information, and completed before
                processing changes.  Right?  If so, where is that mentioned?

                [major] After I wrote the last comment I kept reading and realized
                you're talking about the Node NLRI of the other end of the link.  Be
                clear about the fact that the Node NLRI we're looking for is the one
                with a Local Node Descriptor equal to the Remote Node Descriptor in
                the Current Link.

                697                   +  If the BGP-LS Link NLRI attribute includes an SPF Status
                698                     TLV indicating the link is down, the BGP-LS Link NLRI is
                699                     considered down and the next BGP-LS Link NLRI is examined.

                [minor] For the Current Link, this can be checked after the Node status.

                [major] This same check needs to be done for the Endpoint Node, at the
                link and node level.

This entire section has been rewritten.

                701                   +  All the Link NLRI corresponding the Endpoint Node NLRI will
                702                     be searched for a back-link NLRI pointing to the current
                703                     node.  Both the Node identifiers and the Link endpoint
                704                     identifiers in the Endpoint Node's Link NLRI must match for
                705                     a match.  If there is no corresponding Link NLRI
                706                     corresponding to the Endpoint Node NLRI, the Endpoint Node
                707                     NLRI fails the bi-directional connectivity test and is not
                708                     processed further.

                [major] "back-link NLRI"  Please specify this further: what fields
                from where need to match?

                Suggestion>
                Each Link NLRI corresponding the Endpoint Node NLRI is examined for a
                back-link pointing to the Current Node.  Specifically, the following
                values MUST be the same: the Remote Node Descriptor in the Endpoint Node's
                Link NLRI and the Local Node Descriptor in the Current Node's Link NLRI,
                as well as the corresponding remote and local IPv4/IPv6 addresses in the
                Link NLRIs.   If there is no corresponding...

                This text may still be confusing (specially the part about the IP addresses)...

This is rewritten with the checks specified explicitly. We since there are remote
descriptors, we only support P2P links. We do support parallel P2P links with the
subnet match. We may need to add text for parallel unnumbered links.

                710                   +  If the Endpoint Node NLRI is not on the candidate list, it
                711                     is inserted based on the link cost and BGP Identifier (the
                712                     latter being used as a tie-breaker).

                [minor] I'm not sure what the "based on the link cost and BGP
                Identifier (the latter being used as a tie-breaker)" tries to say.
                Judging from the next two steps, it sounds as if the node is put in
                the candidate list.  Why wouldn't this node be on the candidate list
                already?  (see related question where the Candidate List is defined)

This has been clarified.

                714                   +  If the Endpoint Node NLRI is already on the candidate list
                715                     with a lower cost, it need not be inserted again.

                [minor] "with a lower cost" than what?

                717                   +  If the Endpoint Node NLRI is already on the candidate list
                718                     with a higher cost, it must be removed and reinserted with
                719                     a lower cost.

                [minor] "with a higher cost" than what?

This has been clarified.

                [minor] "removed and reinserted"  I'm not sure why this would happen.
                See related questions when the Candidate List is defined.

This can easily happen if you have process one node with high cost link to the candidate
node followed by a node with a slighly higher direct cost but lower cost to the candidate
node.

                721               *  Return to step 3 to process the next lowest cost Node NLRI on
                722                   the candidate list.

                [nit] Should this be its own step?

Step 3 does this so it is not a separate step.

                724           6.  The local RIB is examined and changes (adds, deletes,
                725               modifications) are installed into the global RIB.

                [major] The local RIB started empty (Step 1), so all the changes are
                adds.  Please be more specific about what is being compared and how
                the adds, deletes, modifications are determined.

This has been reworked.

                727         5.4.  NEXT_HOP Manipulation

                [minor] This sub-section doesn't seem to belong in the Decision Process section.

Agreed it has been moved.

                [major] rfc4760 says this:

                An UPDATE message that carries no NLRI, other than the one encoded in
                the MP_REACH_NLRI attribute, SHOULD NOT carry the NEXT_HOP attribute.
                If such a message contains the NEXT_HOP attribute, the BGP speaker
                that receives the message SHOULD ignore this attribute.

                My reading of the rest of this section is that the text is trying to
                specify rules that are not consistent with rfc4760.  The use of the
                NEXT_HOP to export BGP SPF information should already be covered in
                rfc7752.  It seems to me that this section is not needed.

We have reworked this section.

                729           A BGP speaker that supports SPF extensions MAY interact with peers
                730           that don't support SPF extensions.  If the BGP-LS address family is
                731           advertised to a peer not supporting the SPF extensions described
                732           herein, then the BGP speaker MUST conform to the NEXT_HOP rules
                733           specified in [RFC4271] when announcing the Link-State address family
                734           routes to those peers.

                [major] "If the BGP-LS address family is advertised to a peer not
                supporting the SPF extensions..."  Do you mean to say that the BGP SPF
                information can be exported using BGP-LS?  I can see that.  However,
                there are other things that need to be specified.  For example, what
                Protocol-ID should be used?

                [major] "MUST conform to the NEXT_HOP rules specified in [RFC4271]"
                Which rules?  Are you talking about ??5.1.3?  In any case, note that
                we're now talking about using BGP-LS to export information; the
                Normative language doesn't belong here as this should already be
                covered in rfc7752.

This has been removed.

                736           All BGP peers that support SPF extensions would locally compute the
                737           Loc-RIB next-hops as a result of the SPF process.  Consequently, the
                738           NEXT_HOP attribute is always ignored on receipt.  However, BGP
                739           speakers SHOULD set the NEXT_HOP address according to the NEXT_HOP
                740           attribute rules specified in [RFC4271].

                [major] "locally compute the Loc-RIB next-hops as a result of the SPF
                process"  That's what I expected, but I don't see that part specified
                in the SPF description.

The next-hop calculation is there now.

                [major] "SHOULD set the NEXT_HOP address according to...[RFC4271]"
                Why isn't this action required?  When would rfc4271 not be followed?

                [major] Note that the text in ??5.1 says that the "NEXT_HOP attribute
                value is preserved but otherwise ignored", but the text here
                recommends/requires a potential change.  Should the NEXT_HOP attribute
                be ignored or used?  It seems to me that this section is unnecessary.

We've removed it. The mixing of a BGP SPF Routing Domain and a base BGP Routing domain
is more complex than just specifying the NEXT_HOP rules.

                742         5.5.  IPv4/IPv6 Unicast Address Family Interaction

                744           While the BGP-LS SPF address family and the IPv4/IPv6 unicast address
                745           families install routes into the same device routing tables, they
                746           will operate independently much the same as OSPF and IS-IS would
                747           operate today (i.e., "Ships-in-the-Night" mode).  There will be no
                748           implicit route redistribution between the BGP address families.
                749           However, implementation specific redistribution mechanisms SHOULD be
                750           made available with the restriction that redistribution of BGP-LS SPF
                751           routes into the IPv4 address family applies only to IPv4 routes and
                752           redistribution of BGP-LS SPF route into the IPv6 address family
                753           applies only to IPv6 routes.

                [nit] s/install routes/may install routes

Fixed.

                [minor] s/There will be no/There is no

Fixed.

                [major] "implementation specific redistribution mechanisms SHOULD be
                made available with the restriction..."  I'm not sure if the SHOULD
                applies to mechanisms "made available" or the restriction.  In either
                case, do we need this text?  Given that redistribution happens from
                the RIB, only IPvx routes will go into the IPvx receiver -- IOW, this
                seems to be more a RIB function than one dependent on where the
                information came from.

Reworked this.

                [major] You don't say anything about redistribution into BGP SPF.  Is
                that ok?  I don't think I saw the concept of an "external" route
                anywhere...

Redistribution has been removed and you are correct that there is no concept of an
external route. This is an area that requires more specification.

                755           Given the fact that SPF algorithms are based on the assumption that
                756           all routers in the routing domain calculate the precisely the same
                757           SPF tree and install the same set of routes, it is RECOMMENDED that
                758           BGP-LS SPF IPv4/IPv6 routes be given priority by default when
                759           installed into their respective RIBs.  In common implementations the
                760           prioritization is governed by route preference or administrative
                761           distance with lower being more preferred.

                [nit] s/calculate the precisely/calculate precisely

Fixed.

                [nit] "precisely the same SPF tree and install the same set of routes"
                Given that each node calculates SPF from its own perspective, the
                result is not precisely the same...it is consistent, but not the same.
                Just a nit.

Fixed.

                [minor] s/BGP-LS SPF IPv4/IPv6 routes/BGP SPF routes

This is the BGP-LS-SPF SAFI.

                [major] "RECOMMENDED...routes be given priority"  When is it ok to not
                give higher priority?  IOW, why is this not required?

Scheduling prioritization amongst address families is an implementation matter and
shouldn't be dictated by the protocol.

                [major] "routes be given priority"  Priority over what?  Over other
                BGP-related routes?  Over other IGPs?  Wouldn't all IGPs want the same
                thing?

As for installation, this has always been a implementation choice. We recommend
preference or admin distance lower for BGP-LS-SPF SAFI routes over other BGP
routes. The IGP relationship is deployment dependent.

                763         5.6.  NLRI Advertisement and Convergence

                [minor] This section only talks about convergence, not advertisement.

Advertisement actions are required for convergence.

                [major] Related to advertisement, when are the different NLRIs
                advertised?  For Link NLRI, for example, I assume that changes in
                cost/metric would trigger a new UPDATE, right?  What about the other
                NLRI?

NLRI are advertised when a changed or more recent version is received. See
last paragraph section 6.

                765         5.6.1.  Link/Prefix Failure Convergence

                767           A local failure will prevent a link from being used in the SPF
                768           calculation due to the IGP bi-directional connectivity requirement.
                769           Consequently, local link failures should always be given priority
                770           over updates (e.g., withdrawing all routes learned on a session) in
                771           order to ensure the highest priority propagation and optimal
                772           convergence.

                [] How should this type of failure be signaled?  Should the
                Link/Prefix NLRI be Withdrawn or should a metric of "infinity" be
                used?  Is there even a concept of "infinity"?

The Status TLV is used initially to avoid waiting for all copies of the NLRI to
be withdrawn. The NLRI will subsequently be withdrawn.

                [minor] "local link failures should always be given priority over
                updates"  Should there be Normative language here somewhere?  This is
                a change in behavior with respect to rfc4271 -- what if there are
                other AFI/SAFIs negotiated over the same link/session?

Fixed.

                774           An IGP such as OSPF [RFC2328] will stop using the link as soon as the
                775           Router-LSA for one side of the link is received.  With normal BGP
                776           advertisement, the link would continue to be used until the last copy
                777           of the BGP-LS Link NLRI is withdrawn.  In order to avoid this delay,
                778           the originator of the Link NLRI will advertise a more recent version
                779           of the BGP-LS Link NLRI including the SPF Status TLV Section 4.2.2
                780           indicating the link is down with respect to BGP SPF.  After some
                781           configurable period of time, e.g., 2-3 seconds, the BGP-LS Link NLRI
                782           can be withdrawn with no consequence.  If the link becomes available
                783           in that period, the originator of the BGP-LS LINK NLRI will simply
                784           advertise a more recent version of the BGP-LS Link NLRI without the
                785           SPF Status TLV in the BGP-LS Link Attributes.

                [minor] "normal BGP...BGP-LS Link NLRI is withdrawn"   BGP-LS is not
                "normal" BGP, and the transport is opaque; IOW, the information is not
                used for routing.

Right - but I don't see how that impacts withdrawal. Anyway, we consistently refer to
BGP-LS-SPF now.

                [major] s/ originator...will advertise / originator...MUST/SHOULD (?) advertise

Changed to SHOULD.

                [major] "a more recent version"  MUST/SHOULD increase the Sequence Number...

There is SHOULD now.

                [nit] s/SPF Status TLV Section 4.2.2/SPF Status TLV (Section 4.2.2)

Fixed.

                [major] "configurable period of time, e.g., 2-3 seconds"   Please
                define a timer and set a default.

We indicate this is implementation dependent. We could revise in the next revision.

                787           Similarly, when a prefix becomes unreachable, a more recent version
                788           of the BGP-LS Prefix NLRI will be advertised with the SPF Status TLV
                789           Section 4.3.1 indicating the prefix is unreachable in the BGP-LS
                790           Prefix Attributes and the prefix will be considered unreachable with
                791           respect to BGP SPF.  After some configurable period of time, e.g.,
                792           2-3 seconds, the BGP-LS Prefix NLRI can be withdrawn with no
                793           consequence.  If the prefix becomes reachable in that period, the
                794           originator of the BGP-LS Prefix NLRI will simply advertise a more
                795           recent version of the BGP-LS Prefix NLRI without the SPF Status TLV
                796           in the BGP-LS Prefix Attributes.

                [] Same comments...

Need to add "SHOULD" here in the next revision.

                798         5.6.2.  Node Failure Convergence

                800           With BGP without graceful restart [RFC4724], all the NLRI advertised
                801           by node are implicitly withdrawn when a session failure is detected.
                802           If fast failure detection such as BFD is utilized, and the node is on
                803           the fastest converging path, the most recent versions of BGP-LS NLRI
                804           may be withdrawn while these versions are in-flight on longer paths.
                805           This will result the older version of the NLRI being used until the
                806           new versions arrive and, potentially, unnecessary route flaps.
                807           Therefore, BGP-LS SPF NLRI SHOULD always be retained before being
                808           implicitly withdrawn for a brief configurable interval, e.g., 2-3
                809           seconds.  This will not delay convergence since the adjacent nodes
                810           will detect the link failure and advertise a more recent NLRI
                811           indicating the link is down with respect to BGP SPF Section 5.6.1 and
                812           the BGP-SPF calculation will failure the bi-directional connectivity
                813           check.

                [nit] s/by node/by a node

Fixed.

                [minor] "the most recent versions of BGP-LS NLRI may be withdrawn
                while these versions are in-flight on longer paths"    I don't
                understand what this means -- the "while these versions" part is
                throwing me off.

I mean that you've received the most recent version from the node that went down
and you haven't received it from other nodes.

                [major] "SHOULD always be retained"  s/SHOULD always/SHOULD   When
                would it be ok to not retain?  IOW, why is this not a requirement?

We can change this "SHOULD".

                [minor] "BGP-LS SPF NLRI SHOULD always be retained"  I'm assuming you
                exclusively mean Node NLRIs, and not all, right?  Also, I guess you
                mean to say that when the NLRI changes it should be retained...

I mean NLRI received from the adjacent node that went down. This is where the
BGP implicit withdrawal is applicable. We can attempt to revise.

                [minor] "brief configurable interval, e.g., 2-3 seconds"  Can we use
                the same timer as before?

We indicate this is implementation dependent. I guess will add these timers in
the next revision (or a future revision).

                [nit] s/BGP SPF Section 5.6.1/BGP SPF (Section 5.6.1)

Fixed.

                [nit] s/will failure/will fail

Fixed.

                815         5.7.  Error Handling

                817           When a BGP speaker receives a BGP Update containing a malformed SPF
                818           Capability TLV in the Node NLRI BGP-LS Attribute [RFC7752], it MUST
                819           ignore the received TLV and the Node NLRI and not pass it to other
                820           BGP peers as specified in [RFC7606].  When discarding a Node NLRI
                821           with malformed TLV, a BGP speaker SHOULD log an error for further
                822           analysis.

                [major] "MUST...as specified in [RFC7606]"  Where?  Are you referring
                to one of the error handling strategies?

                [major] What about the other TLVs?  rfc7752 only covers malformed TLVs
                as ones with a bad length...but doesn't say anything about other
                values.  That is on purpose because BGP-LS just carries information
                around.  But that is not the case with BGP SPF.  IOW, you have to
                specify what to do if the different fields are not as expected, when
                is a TLV considered malformed, what to do about it, etc..

This entire section has been rewritten and expanded.

                824         6.  IANA Considerations

                826           This document defines an AFI/SAFI for BGP-LS SPF operation and
                827           requests IANA to assign the BGP-LS/BGP-LS-SPF (AFI 16388 / SAFI TBD1)
                828           as described in [RFC4760].

                [major] rfc4760 sets up the Registry, but it doesn't specify what IANA
                should do unless you're more explicit with the request.

                Suggestion>
                This document defines the use of a new SAFI (TBD1) for BGP SPF operation
                (Section 3), and requests IANA to assign a value from the xxx range in the
                Subsequent Address Family Identifiers (SAFI) Parameters registry.

We've incorporated your suggestion.

                830           This document also defines five attribute TLVs for BGP-LS NLRI.  We
                831           request IANA to assign types for the SPF capability TLV, Sequence
                832           Number TLV, IPv4 Link Prefix-Length TLV, IPv6 Link Prefix-Length TLV,
                833           and SPF Status TLV from the "BGP-LS Node Descriptor, Link Descriptor,
                834           Prefix Descriptor, and Attribute TLVs" Registry.

                [major] The draft defines 3 different SPF Status TLVs (all with the
                same format), but only one value is requested?  Is that the intent?  I
                can see how the one TLV can be came to work across all 3 NLRIs, even
                with a common set of SPF Status values, but that is not explained
                anywhere.

That is the intent. We need to discuss NLRI packing.

                [minor] It would be very nice if a table was included to show the TLV
                types and the name.

This has been added.

                836         7.  Security Considerations

                838           This extension to BGP does not change the underlying security issues
                839           inherent in the existing [RFC4271], [RFC4724], and [RFC7752].

                [] Wow!  No Security Considerations. :-(

                [major] Instead of claiming no new security issues, you should say
                that the considerations in those RFCs apply to BGP SPF.  The
                considerations of course include recommendations that you may not have
                to include here.

                [major] Can a BGP SPF session be used with other AFI/SAFI pairs?  It
                is not clear anywhere whether it can be.  If yes, then the interaction
                related to the deployment models may need to be explained/considered.
                If not, then the question about authentication will definitely come
                up.

We allow but will discuss.

                [major] Please add pointers to the BGP Security Analysis documents;
                rfc4272 and rfc6952.

We've added this.

                [major] It is not clear whether rfc4724 is a requirement or not...

                [major] I believe that the Security Considerations from rfc7752 don't
                apply here at all.  They discuss aspects specific to the inclusion of
                consumers in the deployment model -- consumers don't exist in BGP SPF.
                Also, link-state information is not exported (except for the brief
                hint in ??5.4) so the risks mentioned there are not applicable either.

                [major] Related to rfc7752, the more important fact is that it
                considers all the BGP-LS information to be opaque, which means that
                the security considerations don't include anything related to what a
                rogue node can do inside the system.  Some examples, a rogue node can
                (off the top of my head):

                - advertise someone else's Local Node Descriptor (prompting the real
                node to "fight back")

                - advertise a whole set of nodes "behind" it -- maybe more useful
                during a partition event

                - advertise phantom routers resulting in a trigger of SPF, potentially
                delaying convergence

                - indicate the wrong remote link address or Node Descriptor which can
                disrupt the topology

                - advertise links present on remote router as stub links (prefixes in
                this case), resulting in others thinking that the destinations are
                also (maybe only, if the metrics are properly crafted) reachable in a
                different place

                - send an incorrect SPF Status, forcing a link (or even a node) out of
                SPF consideration

                - the metric (and other information) can be changed in-transit, and
                other nodes would not be able to detect the difference...there's no
                checksum or other protection in the LSAs (the group of
                Node/Link/Prefix NLRI related to a node).

                - arbitrarily/selectively stop the propagation of information

                [Some of these are clearly related to the link-state nature of BGP SPF.]

We may need more here but describing the consequences of every TLV is not done
in IGPs either.

                Note that there may not be mitigation for some of these rogue router
                behaviors.  The use of the protocol in a limited domain may be the
                only possible countermeasure.  But claiming that there are no new
                vulnerabilities is just wrong.

Right - there are not mitigations in the IGPs if a router is compromised
        but still has valid authentication credentials.

                [minor] rfc4593 could also be a good Informative reference about
                generic threats, including some of what I mention above.

This has been added.

                [major] Route origin validation (and BGPSec) don't apply to BGP SPF.
                Whatever door those mechanisms were meant to close are still open in
                BGP SPF.  Specifically, there's no verification about the origination
                of prefixes: anyone can advertise a prefix --- with the proper metric
                it can result in erroneous traffic delivery.

We will say BGPSec is not applicable in the next revision.

                ...
                846         8.1.  Configuration

                848           In addition to configuration of the BGP-LS SPF address family,
                849           implementations SHOULD support the configuration of the
                850           INITIAL_SPF_DELAY, SHORT_SPF_DELAY, LONG_SPF_DELAY, TIME_TO_LEARN,
                851           and HOLDDOWN_INTERVAL as documented in [RFC8405].

                [major] "SHOULD support the configuration of...[RFC8405]"  rfc8405
                says that "All the parameters MUST be configurable", the "SHOULD" here
                is in conflict.  It is not necessary to specify support for that again
                here.

We've changed this to a MUST consistent with RFC 8405.

                853         8.2.  Operational Data

                855           In order to troubleshoot SPF issues, implementations SHOULD support
                856           an SPF log including entries for previous SPF computations, Each SPF
                857           log entry would include the BGP-LS NLRI SPF triggering the SPF, SPF
                858           scheduled time, SPF start time, SPF end time, and SPF type if
                859           different types of SPF are supported.  Since the size of the log will
                860           be finite, implementations SHOULD also maintain counters for the
                861           total number of SPF computations of each type and the total number of
                862           SPF triggering events.  Additionally, to troubleshoot SPF scheduling
                863           and back-off [RFC8405], the current SPF back-off state, remaining
                864           time-to-learn, remaining holddown, last trigger event time, last SPF
                865           time, and next SPF time should be available.

                [] "SPF computations of each type"  There are multiple types??

We have added (if supported).

                ...
                933         12.1.  Normative References
                ...
                966           [RFC8402]  Filsfils, C., Ed., Previdi, S., Ed., Ginsberg, L.,
                967                       Decraene, B., Litkowski, S., and R. Shakir, "Segment
                968                       Routing Architecture", RFC 8402, DOI 10.17487/RFC8402,
                969                       July 2018, <https://www.rfc-editor.org/info/rfc8402>.

                [major] This reference is used in error -- it is not needed.

Removed.

                ...
                977         12.2.  Information References
                ...
                1005        [RFC4750]  Joyal, D., Ed., Galecki, P., Ed., Giacalone, S., Ed.,
                1006                    Coltun, R., and F. Baker, "OSPF Version 2 Management
                1007                    Information Base", RFC 4750, DOI 10.17487/RFC4750,
                1008                    December 2006, <https://www.rfc-editor.org/info/rfc4750>.

                [major] There is Normative language referring to this RFC, so the
                reference should be Normative.

Now normative.

                1010        [RFC4760]  Bates, T., Chandra, R., Katz, D., and Y. Rekhter,
                1011                    "Multiprotocol Extensions for BGP-4", RFC 4760,
                1012                    DOI 10.17487/RFC4760, January 2007,
                1013                    <https://www.rfc-editor.org/info/rfc4760>.

                [major] This reference should be Normative.

Now normative.

                1015        [RFC4790]  Newman, C., Duerst, M., and A. Gulbrandsen, "Internet
                1016                    Application Protocol Collation Registry", RFC 4790,
                1017                    DOI 10.17487/RFC4790, March 2007,
                1018                    <https://www.rfc-editor.org/info/rfc4790>.

                [major] This reference is used in error; it is not needed.

Removed.

ACEE-M-3B86:ietf-lsvr-bgp-spf acee$