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

"Acee Lindem (acee)" <acee@cisco.com> Mon, 15 February 2021 15:56 UTC

IronPort-PHdr: 9a23:+4KCEhMimzD0brl1Iu4l6mtXPHoupqn0MwgJ65Eul7NJdOG58o//OFDEvKwx3lDMVITfrflDjrmev6PhXDkG5pCM+DAHfYdXXhAIwcMRg0Q7AcGDBEG6SZyibyEzEMlYElMw+Xa9PBtaHc//YxvZpXjhpTIXEw/0YAxyIOm9E4XOjsOxgua1/ZCbYwhBiDenJ71oKxDjpgTKvc5Qioxneas=
From: "Acee Lindem (acee)" <acee@cisco.com>
To: Alvaro Retana <aretana.ietf@gmail.com>
CC: "lsvr@ietf.org" <lsvr@ietf.org>, "draft-ietf-lsvr-bgp-spf@ietf.org" <draft-ietf-lsvr-bgp-spf@ietf.org>
Thread-Topic: AD Review of draft-ietf-lsvr-bgp-spf-11.txt
Thread-Index: AQHXA7MGMknYL+ylSUWvRR4bYNq0YA==
Date: Mon, 15 Feb 2021 15:55:47 +0000
Message-ID: <F1DFEA4B-600C-4989-AA84-F81AAF3BD19B@cisco.com>
Accept-Language: en-US
Content-Language: en-US
user-agent: Microsoft-MacOutlook/16.45.21011103
Content-Type: multipart/alternative; boundary="_000_F1DFEA4B600C4989AA84F81AAF3BD19Bciscocom_"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e3d83b6b-be06-4fb1-c14a-08d8d1ca28ed
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Feb 2021 15:55:47.2264 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 5ae1af62-9505-4097-a69a-c1553ef7840e
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: YI4y8H8WT3JVLw4wBfQMUwS7kmDKo1vQ51R0dheGKCszKGmliJRBywGlbHuLHR9Y
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5086
X-OriginatorOrg: cisco.com
X-Outbound-SMTP-Client: 173.37.102.13, xch-rcd-003.cisco.com
X-Outbound-Node: rcdn-core-5.cisco.com
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsvr/K1wQXfvJGWj44B43liOLAc4j3k4>
Subject: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-11.txt
X-BeenThere: lsvr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Vector Routing <lsvr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsvr>, <mailto:lsvr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsvr/>
List-Post: <mailto:lsvr@ietf.org>
List-Help: <mailto:lsvr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsvr>, <mailto:lsvr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 15 Feb 2021 15:56:05 -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$

[Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-11.txt Acee Lindem (acee)
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Alvaro Retana
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Alvaro Retana
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Alvaro Retana
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Alvaro Retana
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Acee Lindem
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Acee Lindem
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Alvaro Retana
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Alvaro Retana
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Acee Lindem
Re: [Lsvr] AD Review of draft-ietf-lsvr-bgp-spf-1… Acee Lindem