[Idr] AD Review of draft-ietf-idr-rfc7752bis-10

Alvaro Retana <aretana.ietf@gmail.com> Tue, 20 September 2022 19:30 UTC

From: Alvaro Retana <aretana.ietf@gmail.com>
MIME-Version: 1.0
Date: Tue, 20 Sep 2022 12:30:03 -0700
Message-ID: <CAMMESszHeVAuQ2qW+M7rrwMoWFBR5kqdGqE8841rKhNX_ruOPA@mail.gmail.com>
To: Ketan Talaulikar <ketant.ietf@gmail.com>
Cc: draft-ietf-idr-rfc7752bis@ietf.org, Jeff Haas <jhaas@juniper.net>, idr-chairs@ietf.org, idr@ietf.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/idr/ogqqSawVcx3TFueiKNxmnuG00h8>
Subject: [Idr] AD Review of draft-ietf-idr-rfc7752bis-10
Precedence: list

Ketan:

Hi! Thanks for the work and time you have put into this document!

I focused my review on the diffs and the effect they may have on the
original text. Please see detailed comments below.

Thanks!

Alvaro.

[Line numbers from idnits.]

...
65 Table of Contents

[nit] This is a long TOC -- consider reducing the detail from a depth
of 3 (4.2.*) to a depth of 2 (4.*). That may reduce the size by about
30%.

...
133 1. Introduction

135 The contents of a Link-State Database (LSDB) or of an IGP's Traffic
136 Engineering Database (TED) describe only the links and nodes within
137 an IGP area. Some applications, such as end-to-end Traffic
138 Engineering (TE), would benefit from visibility outside one area or
139 Autonomous System (AS) in order to make better decisions.

[] The general description and motivation (including §2) are centered
around IGP-derived information. But that is not the only use case as
information from other sources can also be transported. It would be
very nice if some text was included to that effect, maybe at the end
of this section, or the start of section 2.

This document focuses its descriptions around the propagation
of IGP-derived information...in general, the procedures apply
to other sources...

...
159 A router maintains one or more databases for storing link-state
160 information about nodes and links in any given area. Link attributes
161 stored in these databases include: local/remote IP addresses, local/
162 remote interface identifiers, link metric, and TE metric, link
163 bandwidth, reservable bandwidth, per Class-of-Service (CoS) class
164 reservation state, preemption, and Shared Risk Link Groups (SRLGs).
165 The router's BGP process can retrieve topology from these LSDBs and
166 distribute it to a consumer, either directly or via a peer BGP
167 speaker (typically a dedicated Route Reflector), using the encoding
168 specified in this document.

[minor] s/BGP process/BGP-LS process

This may be a good place to introduce the BGP-LS acronym, which is not
defined anywhere in the document.

...
173 +-----------+
174 | Consumer |
175 +-----------+
176 ^
177 |
178 +-----------+ +-----------+
179 | BGP | | BGP |
180 | Speaker |<----------->| Speaker | +-----------+
181 | RR1 | | RRm | | Consumer |
182 +-----------+ +-----------+ +-----------+
183 ^ ^ ^ ^
184 | | | |
185 +-----+ +---------+ +---------+ |
186 | | | |
187 +-----------+ +-----------+ +-----------+
188 | BGP | | BGP | | BGP |
189 | Speaker | | Speaker | . . . | Speaker |
190 | R1 | | R2 | | Rn |
191 +-----------+ +-----------+ +-----------+
192 ^ ^ ^
193 | | |
194 IGP IGP IGP

196 Figure 1: Collection of Link-State and TE Information

[minor] This figure now includes router names, which is good for later
identification and explanations. I understand the general BGP
distribution through route reflectors and figured that to be the
reason for naming the nodes in the middle RRx. However, there is no
specific tie-in made between the speakers in the middle row and a
route reflector function. OTOH, §4.9 mentions a route reflector
(abbreviated as RR), but then figure 31 names that BGP-LS speaker "R0"
(not RR0 to follow the naming here).

All this is to say that the first inclination anyone familiar with BGP
has is to associate "RR" with a route reflector. But that doesn't
seem to be the use in Figure 1 -- IOW, I don't see the need to label
BGP Speakers differently.

In the end, I'm just looking for consistency.

...
339 3. BGP Speaker Roles for BGP-LS

341 In the illustration shown in Figure 1, the BGP Speakers can be seen
342 playing different roles in the distribution of information using BGP-
343 LS. This section introduces terms that explain the different roles
344 of the BGP Speakers which are then used through the rest of this
345 document.

347 o BGP-LS Producer: The BGP Speakers R1, R2, ... Rn, originate link-
348 state information from their underlying link-state IGP protocols
349 into BGP-LS. If R1 and R2 are in the same IGP area, then likely
350 they are originating the same link-state information into BGP-LS.
351 R1 may also source information from sources other than IGP, e.g.
352 its local node information. The term BGP-LS Producer refers to
353 the BGP Speaker that is originating link-state information into
354 BGP.

[major] Please first define the term and then illustrate the function.
For example:

BGP-LS Producer: The term BGP-LS Producer refers to... [Then include
examples..]

[minor] "If R1 and R2 are in the same IGP area, then likely they are
originating the same link-state information into BGP-LS. R1 may also
source information from sources other than IGP, e.g. its local node
information."

These two sentences, while true, are not needed as they include
concepts that don't seem relevant -- both at this point and in
general.

- "IGP area" is only mentioned *before* this point, so the duplication
of information hinted here doesn't seem important

- this is the only place (except for when the Protocol-ID is defined)
that other sources are mentioned

A way to make these points relevant would be to provide more details
about them (elsewhere).

[nit] s/the BGP Speaker/a BGP Speaker

[major] "originating link-state information"

It sounds obvious and implicit that the BGP-LS information comes from
the IGP LSDB. However, this fact is mentioned only in passing (and in
a non-normative way) in the the Introduction:

The router's BGP process can retrieve topology from these LSDBs and
distribute it to a consumer, either directly or via a peer BGP
speaker (typically a dedicated Route Reflector), using the encoding
specified in this document.

[There's also some related text in the new "Handling of Unreachable
IGP Nodes" section.]

Please be specific here as to where the information comes from -- and
how it has to be present in the LSDB for the BGP-LS Producer to
originate it.

356 o BGP-LS Consumer: The BGP Speakers RR1 and Rn are handing off the
357 BGP-LS information that they have collected to a consumer
358 application. The BGP protocol implementation and the consumer
359 application may be on the same or different nodes. The term BGP-
360 LS Consumer refers to the consumer application/process and not the
361 BGP Speaker. This document only covers the BGP implementation.
362 The consumer application and the design of the interface between
363 BGP and consumer application may be implementation specific and
364 outside the scope of this document.

[] As above, please define and then illustrate...

[minor] Figure 2 and 3 show the interaction with the PCE and ALTO
servers (aka consumers) to be done using "BGP with Link-State NLRI" --
but that is not what is specified here.

366 o BGP-LS Propagator: The BGP Speaker RRm propagates the BGP-LS
367 information between the BGP Speaker Rn and the BGP Speaker RR1.
368 The BGP implementation on RRm is doing the propagation of BGP-LS
369 updates and performing BGP best path calculations. Similarly, the
370 BGP Speaker RR1 is receiving BGP-LS information from R1, R2 and
371 RRm and propagating the information to the BGP-LS Consumer after
372 performing BGP best path calculations. The term BGP-LS Propagator
373 refers to the BGP Speaker that is performing BGP protocol
374 processing on the link-state information.

[] As above, please define and then illustrate...

[minor] Following on my comment about naming nodes in Figure 1 as RRx,
the definition of a BGP-LS Propagator sounds to me as what a route
reflector does. There's one exception: even through you don't mention
it, I'm assuming that Rn can also be a BGP-LS Propagator. IOW, not
only does the BGP-LS Propagator have a route reflector-like function
between BGP speakers, but it also propagates to the BGP-LS Consumer.

I wonder if explaining the functionality in terms of a route reflector
might make it easier for the reader to understand the processing done,
at least between BGP speakers. [Just a suggestion, no action
required.]

[major] s/performing BGP best path calculations/running the BGP
Decision Process/g

That is the rfc4271 terminology. There are a couple of "best-path"
later on too.

[major] What exactly do you mean by "BGP protocol processing on the
link-state information"? I assume you mean (as mentioned) finding the
best route and propagating. Is there anything else? The part about
"processing on the link-state information" gives the impression that
there might be something else (see the comment below about "performing
some of the validation and processing on behalf of the BGP-LS
Consumer").

376 The above roles are not mutually exclusive. The same BGP Speaker may
377 be the producer for some link-state information and propagator for
378 some other link-state information while also providing this
379 information to a consumer application. Nothing precludes a BGP
380 implementation performing some of the validation and processing on
381 behalf of the BGP-LS Consumer as long as it does not impact the
382 semantics of its role as BGP-LS Propagator as described in this
383 document.

[minor] s/be the producer for/be the BGP-LS Producer of

[minor] s/propagator/BGP-LS Propagator

[minor] s/consumer application/BGP-LS Consumer

[major] "Nothing precludes a BGP implementation performing some of the
validation and processing on behalf of the BGP-LS Consumer as long as
it does not impact the semantics of its role as BGP-LS Propagator as
described in this document."

s/BGP implementation/BGP speaker

The definition of BGP-LS Consumer clearly says that the BGP
implementation and the consumer application are independent of each
other. How can a BGP speaker perform "validation and processing on
behalf of the BGP-LS Consumer" if the operation of, and the interface
to, the BGP-LS Consumer are out of scope?

What type of "validation and processing" beyond basic BGP processing
are you referring to? By "basic" BGP processing I mean: UPDATE error
handling (§6.3/rfc4271) and UPDATE message handling (§9/rfc4271) --
IOW, nothing explicitly related to the content of what is specified
in this document.

I don't understand what you mean by "the semantics of its role as
BGP-LS Propagator" and what the impact of that may be.

Also, not all BGP-LS Propagators have a related BGP-LS Consumer
associated to them. Are you expecting all BGP-LS Propagators to
perform checks "on behalf of the BGP-LS Consumer" even if no BGP-LS
Consumer is attached?

...
389 4. Carrying Link-State Information in BGP
...
404 This section mainly describes the procedures at a BGP-LS Producer
405 that originate link-state information into BGP-LS.

[nit] s/at a BGP-LS Producer that originate/for a BGP-LS Producer to originate

407 4.1. TLV Format
...
424 The Length field defines the length of the value portion in octets
425 (thus, a TLV with no value portion would have a length of zero). The
426 TLV is not padded to 4-octet alignment. Unknown and unsupported
427 types MUST be preserved and propagated within both the NLRI and the
428 BGP-LS Attribute. The presence of unrecognized or unexpected TLVs
429 MUST NOT result in the NLRI or the BGP-LS Attribute being considered
430 as malformed.

[major] What is the difference between "unknown", "unsupported",
"unrecognized", and "unexpected"? All these words are used above (and
in other places) to (apparently) mean similar things. If these terms
have different meanings, please be clear about the distinction. If
they mean the same thing, please use one set or clarify that they are
all the same.

In the text above... If "unknown and unsupported" is the same as
"unrecognized or unexpected", then there's some redundancy.

...
443 All TLVs within the NLRI that are not specified as mandatory are
444 considered optional. All TLVs within the BGP-LS Attribute are
445 considered optional unless specified otherwise.

[minor] "TLVs...not specified as mandatory are considered optional"

Ok.

>From a BGP-LS point of view, and keeping in mind that this document
specifies the BGP behavior, what is the significance of classifying
the TLVs this way? I ask because this is one of the changes with
respect to rfc7752, but the BGP-LS Propagators don't check for
mandatory TLVs to be present (§7.2.2).

The classification seems inconsequential to me given that it is not
used. I assume the unwritten expectation is that the BGP-LS Consumer
will check. Is there anything else?

...
451 When there are multiple BGP-LS Producers originating the same link-
452 state information, implementation variations of BGP-LS Producers may
453 result in the generation of different and inconsistent BGP-LS updates
454 for the same link-state object based on the inclusion or exclusion of
455 optional TLVs. An inconsistency between BGP-LS Producers with
456 regards to the inclusion of optional TLVs in the NLRI results in
457 multiple NLRIs being generated for the same link-state object. A
458 BGP-LS Consumer would need the ability to merge such duplicate
459 updates to handle such situations. An inconsistency between BGP-LS
460 Producers with regards to the inclusion of optional TLVs in the BGP-
461 LS Attribute results in one of them being delivered to a BGP-LS
462 Consumer as part the BGP propagation and best-path selection
463 procedures in most typical deployments. This can result in a BGP-LS
464 Consumer missing out on some of the information in a potentially
465 unpredictable manner. The use of BGP-LS Producers that have a
466 consistent support for the origination of optional TLVs between them
467 can help mitigate such situations for the BGP-LS Consumers.

[major] "implementation variations"

Isn't the job of the specifications (including this one!) to be clear
enough guarantee interoperability? If the specifications are clear
then we don't need to worry about implementation variations. If the
specifications are not clear then we should fix them!

I know this text resulted from the discussion about BGP-LS Producers
inconsistently advertising information, specifically optional TLVs.
However, to support a specific feature the origination of the
corresponding TLVs becomes necessary. It seems to me that beyond
"implementation variations" we're talking about inconsistent
configuration (and maybe both).

469 4.2. The Link-State NLRI
...
486 New Link-State NLRI Types may be introduced in the future. Since
487 supported NLRI type values within the address family are not
488 expressed in the Multiprotocol BGP (MP-BGP) capability [RFC4760], it
489 is possible that a BGP speaker has advertised support for Link-State
490 but does not support a particular Link-State NLRI type. To allow the
491 introduction of new Link-State NLRI types seamlessly in the future,
492 without the need for upgrading all BGP speakers in the propagation
493 path (e.g. a route reflector), this document deviates from the
494 default handling behavior specified by [RFC7606] for Link-State
495 address-family. An implementation MUST handle unrecognized Link-
496 State NLRI types as opaque objects and MUST preserve and propagate
497 them.

[minor] s/advertised support for Link-State/advertised support for BGP-LS

...
625 A router MAY run multiple protocol instances of OSPF or ISIS whereby
626 it becomes a border router between multiple IGP domains. Both OSPF
627 and IS-IS MAY also run multiple routing protocol instances over the
628 same link. See [RFC8202] and [RFC6549]. These instances define
629 independent IGP routing domains. The Identifier field carries a
630 64-bit BGP-LS Instance Identifier (Instance-ID) number that is used
631 to identify the IGP routing domain where the NLRI belongs. The NLRIs
632 representing link-state objects (nodes, links, or prefixes) from the
633 same IGP routing instance MUST have the same Identifier field value.

[major] "A router MAY run multiple protocol instances...MAY also run
multiple routing protocol instances over the same link"

These are statements of fact, not Normative options resulting from BGP-LS.

s/MAY/may

[nit] "See [RFC8202] and [RFC6549]."

I don't think these references are needed.

[minor] The figures were updated to octets: s/64-bit/8-octet

[major] "BGP-LS Instance Identifier (Instance-ID)"

The figures above don't use either of these names, they all use
"Instance". Please be consistent. Looks like most of the text uses
"Instance-ID", but there are also some mentions of "Identifier field".

635 NLRIs with different Identifier field values MUST be considered to be
636 from different IGP routing instances. The Identifier field value 0
637 is RECOMMENDED to be used when there is only a single protocol
638 instance in the network where BGP-LS is operational.

[major] "NLRIs with different Identifier field values MUST be
considered to be from different IGP routing instances."

This sentence seems to be a restatement of the "Unique BGP-LS
Instance-ID values MUST be assigned" phrase below -- or maybe the
phrase below is a restatement of this one. Please consolidate the
guidance.

640 An implementation that supports multiple IGP instances MUST support
641 the configuration of unique BGP-LS Instance-IDs at the routing
642 protocol instance level. The network operator MUST assign consistent
643 BGP-LS Instance-ID values on all BGP-LS Producers within a given IGP
644 domain. Unique BGP-LS Instance-ID values MUST be assigned to routing
645 protocol instances operating in different IGP domains. This allows
646 the BGP-LS Consumer to build an accurate segregated multi-domain
647 topology based on the Identifier field even when the topology is
648 advertised via BGP-LS by multiple BGP-LS Producers in the network.

[major] "MUST assign consistent BGP-LS Instance-ID values"

If you mean "the same", please say so. Otherwise, what does
"consistent" mean in this case?

[minor] "The network operator MUST assign consistent BGP-LS
Instance-ID values on all BGP-LS Producers within a given IGP domain."

It seems to me that this requirement can have a significant impact on
the operation of the network -- both when configured correctly
(configuration maintenance, etc.) as well as if not. Please include
some text about this (operational cost) in §7.1.

[nit] s/allows the BGP-LS Consumer/can allow the BGP-LS Consumer

Given that the operation of the BGP-LS Consumer is out of scope.

[nit] It seems to me that the last sentence is a run-on. Perhaps cut
the last part ("...even when the topology...").

650 When the above-described semantics and recommendations are not
651 followed, a BGP-LS Consumer may see duplicate link-state objects for
652 the same node, link, or prefix when there are multiple BGP-LS
653 Producers deployed. This may also result in the BGP-LS Consumers
654 getting an inaccurate network-wide topology.

[major] "duplicate link-state objects" are not an issue, unless they
have different Instance-IDs. IOW, the description above focusses on
the effect of having "multiple BGP-LS Producers deployed" and not on
the root cause: misconfiguration.

656 When adding, removing, or modifying a TLV/sub-TLV from a Link-State
657 NLRI, the BGP-LS Producer MUST withdraw the old NLRI by including it
658 in the MP_UNREACH_NLRI. Not doing so can result in duplicate and in-
659 consistent link-state objects hanging around in the BGP-LS table.

[nit] Move this paragraph to the end of this section, after the
descriptors have been introduced as TLVs.

...
668 4.2.1. Node Descriptors

670 Each link is anchored by a pair of Router-IDs that are used by the
671 underlying IGP, namely, a 48-bit ISO System-ID for IS-IS and a 32-bit
672 Router-ID for OSPFv2 and OSPFv3. An IGP may use one or more
673 additional auxiliary Router-IDs, mainly for Traffic Engineering
674 purposes. For example, IS-IS may have one or more IPv4 and IPv6 TE
675 Router-IDs [RFC5305] [RFC6119]. These auxiliary Router-IDs MUST be
676 included in the node attribute described in Section 4.3.1 and MAY be
677 included in the link attribute described in Section 4.3.2. The
678 advertisement of the TE Router-IDs helps a BGP-LS Consumer to
679 correlate multiple link-state objects (e.g. in different IGP
680 instances or areas/levels) to the same node in the network.

[major] "Router-IDs MUST be included in the node attribute described
in Section 4.3.1 and MAY be included in the link attribute described
in Section 4.3.2."

To avoid any potential confusion, please be specific about which TLVs
are to be used. It seems that in both cases the same TLVs (1028/1029)
are to be used, right?

[nit] s/helps a BGP-LS Consumer to correlate/can help a BGP-LS
Consumer to correlate

Given that the operation of the BGP-LS Consumer is out of scope.

...
757 4.2.1.4. Node Descriptor Sub-TLVs
...
775 Autonomous System: Opaque value (32-bit AS Number). This is an
776 optional TLV. The value SHOULD be set to the AS Number associated
777 with the BGP process originating the link-state information. An
778 implementation MAY provide a configuration option on the BGP-LS
779 Producer to use a different value; e.g., to avoid collisions when
780 using private AS numbers.

782 BGP-LS Identifier: Opaque value (32-bit ID). This is an optional
783 TLV. In conjunction with Autonomous System Number (ASN), uniquely
784 identifies the BGP-LS domain. The combination of ASN and BGP-LS
785 ID MUST be globally unique. All BGP-LS speakers within an IGP
786 flooding-set (set of IGP nodes within which an LSP/LSA is flooded)
787 MUST use the same ASN, BGP-LS ID tuple. If an IGP domain consists
788 of multiple flooding-sets, then all BGP-LS speakers within the IGP
789 domain SHOULD use the same ASN, BGP-LS ID tuple.

[major] This description and the text at the end of this section
conflicts. For example, the requirement that the "combination of ASN
and BGP-LS ID MUST be globally unique" conflicts with the
recommendation (below) to use 0 as the default value.

Please include the current description here. More comments below.

...
799 IGP Router-ID: Opaque value. This is a mandatory TLV when
800 originating information from IS-IS, OSPF, direct or static. For
801 an IS-IS non-pseudonode, this contains a 6-octet ISO Node-ID (ISO
802 system-ID). For an IS-IS pseudonode corresponding to a LAN, this
803 contains the 6-octet ISO Node-ID of the Designated Intermediate
804 System (DIS) followed by a 1-octet, nonzero PSN identifier (7
805 octets in total). For an OSPFv2 or OSPFv3 non-pseudonode, this
806 contains the 4-octet Router-ID. For an OSPFv2 pseudonode
807 representing a LAN, this contains the 4-octet Router-ID of the
808 Designated Router (DR) followed by the 4-octet IPv4 address of the
809 DR's interface to the LAN (8 octets in total). Similarly, for an
810 OSPFv3 pseudonode, this contains the 4-octet Router-ID of the DR
811 followed by the 4-octet interface identifier of the DR's interface
812 to the LAN (8 octets in total). The TLV size in combination with
813 the protocol identifier enables the decoder to determine the type
814 of the node. For Direct or Static configuration, the value SHOULD
815 be taken from an IPv4 or IPv6 address (e.g. loopback interface)
816 configured on the node.

[major] "the value SHOULD be taken from an IPv4 or IPv6 address"

When is it ok not to? Why is this recommended and not required? It
seems to me that using a configured address is convenient, but just
that.

818 There can be at most one instance of each sub-TLV type present in any
819 Node Descriptor. The sub-TLVs within a Node Descriptor MUST be
820 arranged in ascending order by sub-TLV type. This needs to be done
821 to compare NLRIs, even when an implementation encounters an unknown
822 sub-TLV. Using stable sorting, an implementation can do a binary
823 comparison of NLRIs and hence allow incremental deployment of new key
824 sub-TLVs.

[major] "There can be at most one instance of each sub-TLV type
present in any Node Descriptor."

What should the receiver do if the there is more than one instance of a sub-TLV?

826 The BGP-LS Identifier was introduced by [RFC7752] and its use is
827 being deprecated by this document. Implementations MUST continue to
828 support this sub-TLV for backward compatibility. The default value
829 of 0 is RECOMMENDED to be used when a BGP-LS Producer includes this
830 sub-TLV when originating information into BGP-LS. Implementations
831 MAY provide an option to configure this value for backward
832 compatibility reasons. The use of the Instance-ID in the Identifier
833 field is the RECOMMENDED way of segregation of different IGP domains
834 in BGP-LS.

[major] "MUST continue to support this sub-TLV for backward compatibility"

I'm assuming that the expectation is that eventually no one will use
this TLV, right? What does "MUST continue to support" mean? Do you
mean that support shouldn't stop immediately so that there's a
graceful transition, or that it has to be supported forever? The text
implies forever.

What about new implementations? The text includes "continue to
support", which presumably means that an implementation did support
the TLV at some point.

All this is to say that because "unknown and unsupported types MUST be
preserved and propagated" (from $4.1) then it would be ok if some
implementations stopped supporting it -- or if there was a mix of
support in the network. Am I missing something?

[major] "is the RECOMMENDED way"

This phrase sounds like a statement of fact, not a Normative recommendation.

Even if s/RECOMMENDED/recommended, the use of the Instance-ID is not
just recommended, it is *the* way to do it, right?

836 4.2.2. Link Descriptors

838 The Link Descriptor field is a set of Type/Length/Value (TLV)
839 triplets. The format of each TLV is shown in Section 4.1. The Link
840 Descriptor TLVs uniquely identify a link among multiple parallel
841 links between a pair of anchor routers. A link described by the Link
842 Descriptor TLVs actually is a "half-link", a unidirectional
843 representation of a logical link. To fully describe a single logical
844 link, two originating routers advertise a half-link each, i.e., two
845 Link NLRIs are advertised for a given point-to-point link.

847 A BGP-LS Consumer should not consider a link between two nodes as
848 being available unless it has received the two Link NLRIs
849 corresponding to the half-link representation of that link from both
850 the nodes. This check is similar to the 'two-way connectivity check'
851 that is performed by link-state IGPs and is also required to be done
852 by BGP-LS Consumers of link-state topology.

[major] The operation of the consumer application is out of scope.
This document can't require or recommend anything.

OTOH, it can describe what the intent is with respect to the
"half-link" discussion in the previous paragraph, for example:

The Link Descriptor field is a set of Type/Length/Value (TLV)
triplets. The format of each TLV is shown in Section 4.1. The Link
Descriptor TLVs uniquely identify a link among multiple parallel
links between a pair of anchor routers. A link described by the Link
Descriptor TLVs actually is a "half-link", a unidirectional
representation of a logical link.

A link between two nodes is not considered complete (or available)
unless it is described by the two Link NLRIs corresponding each to
the half-link representation from the pair of anchor routers. This
check is similar to the 'two-way connectivity check' that is
performed by link-state IGPs.

854 A BGP-LS Producer MAY suppress the advertisement of a Link NLRI,
855 corresponding to a half link, from a link-state IGP unless it has
856 verified that the link is being reported in the IS-IS LSP or OSPF
857 Router LSA by both the nodes connected by that link. This 'two-way
858 connectivity check' is performed by link-state IGPs during their
859 computation and may be leveraged before passing information for any
860 half-link that is reported from these IGPs into BGP-LS. This ensures
861 that only those Link State IGP adjacencies which are established get
862 reported via Link NLRIs. Such a 'two-way connectivity check' may be
863 also required in certain cases (e.g. with OSPF) to obtain the proper
864 link identifiers of the remote node.

[nit] s/half link/half-link
All the other mentions use the "-".

[major] "A BGP-LS Producer MAY suppress the advertisement of a...half
link...unless it has verified that the link is being reported [in the
IGP] by both the nodes connected by that link."

Another way of looking at this option is to require the converse
behavior: "the BGP-LS Producer MUST NOT suppress...if the two-way
connectivity has been advertised on the IGP".

I both cases we would be requiring the BGP-LS Producer to perform the
two-way connectivity check using the IGP information. That seems to
me to be way outside the scope of BGP (or BGP-LS).

As far as I can tell, the two-way connectivity check is done by the
IGPs when running SPF, so that result wouldn't be available in the
LSBD.

...
875 +-----------+---------------------+--------------+------------------+
876 | TLV Code | Description | IS-IS | Reference |
877 | Point | | TLV/Sub-TLV | (RFC/Section) |
878 +-----------+---------------------+--------------+------------------+
879 | 258 | Link Local/Remote | 22/4 | [RFC5307] / 1.1 |
880 | | Identifiers | | |
881 | 259 | IPv4 interface | 22/6 | [RFC5305] / 3.2 |
882 | | address | | |
883 | 260 | IPv4 neighbor | 22/8 | [RFC5305] / 3.3 |
884 | | address | | |
885 | 261 | IPv6 interface | 22/12 | [RFC6119] / 4.2 |
886 | | address | | |
887 | 262 | IPv6 neighbor | 22/13 | [RFC6119] / 4.3 |
888 | | address | | |
889 | 263 | Multi-Topology | --- | Section 4.2.2.1 |
890 | | Identifier | | |
891 +-----------+---------------------+--------------+------------------+

893 Table 4: Link Descriptor TLVs

895 The information about a link present in the LSA/LSP originated by the
896 local node of the link determines the set of TLVs in the Link
897 Descriptor of the link.

899 If interface and neighbor addresses, either IPv4 or IPv6, are
900 present, then the IP address TLVs MUST be included, and the Link
901 Local/Remote Identifiers TLV MUST NOT be included in the Link
902 Descriptor. The Link Local/Remote Identifiers TLV MAY be included
903 in the link attribute when available. IPv6 link-local addresses
904 MUST NOT be carried in the IPv6 address TLVs as descriptors of a
905 link as they are not considered unique.

[major] "If interface and neighbor addresses, either IPv4 or IPv6, are
present, then the IP address TLVs MUST be included..."

Given the TLV names in Table 4, this sentence is confusing because the
name of the TLVs ("interface address" and "neighbor address") are not
used, but the description is used as the condition.

[major] "MUST NOT be carried in the IPv6 address TLVs"

Please be explicit by using the names of the TLVs.

...
924 4.2.2.1. Multi-Topology ID
...
953 The MT-ID TLV MAY be present in a Link Descriptor, a Prefix
954 Descriptor, or the BGP-LS attribute of a Node NLRI. In a Link or
955 Prefix Descriptor, only a single MT-ID TLV containing the MT-ID of
956 the topology where the link or the prefix is reachable is allowed.
957 In case one wants to advertise multiple topologies for a given Link
958 Descriptor or Prefix Descriptor, multiple NLRIs MUST be generated
959 where each NLRI contains a single unique MT-ID. When used in the
960 Link or Prefix Descriptor TLV for IS-IS, the Bits R are reserved and
961 MUST be set to 0 (as per Section 7.2 of RFC 5120 [RFC5120]) when
962 originated and ignored on receipt.

[nit] s/BGP-LS attribute/BGP-LS Attribute/g

That seems to be the most used form in this document.

...
969 4.2.3. Prefix Descriptors
...
989 The Multi-Topology Identifier TLV MUST be included in Prefix
990 Descriptor if the underlying IGP prefix object is associated with a
991 non-default topology.

[nit] s/included in Prefix Descriptor/included in the Prefix Descriptor

993 4.2.3.1. OSPF Route Type

995 The OSPF Route Type TLV is an optional TLV corresponding to Prefix
996 NLRIs originated from OSPF. It is used to identify the OSPF route
997 type of the prefix. An OSPF prefix MAY be advertised in the OSPF
998 domain with multiple route types. The Route Type TLV allows the
999 discrimination of these advertisements. The OSPF Route Type TLV MUST
1000 be included advertisement when the type is either being signaled
1001 explicitly or can be determined via another advertisement for the
1002 same prefix (refer section 2.1 of [RFC7684]). The format of the OSPF
1003 Route Type TLV is shown in the following figure.

[major] "The OSPF Route Type TLV MUST be included advertisement when
the type is either being signaled explicitly or can be determined via
another advertisement for the same prefix (refer section 2.1 of
[RFC7684])."

nit: s/included advertisement/included in the advertisement

What do you mean by "signaled explicitly"? Does it mean that the
route type is signaled in the OSPFv2 Extended Prefix TLV, and is not
0? If so, please be explicit.

What does it mean that the type "can be determined via another
advertisement for the same prefix"? I didn't find anything related to
that in §2.1/rfc7684. Did I miss it?

...
1061 4.3. The BGP-LS Attribute
...
1074 The size of the BGP-LS Attribute may potentially grow large depending
1075 on the amount of link-state information associated with a single
1076 Link-State NLRI. The BGP specification [RFC4271] mandates a maximum
1077 BGP message size of 4096 octets. It is RECOMMENDED that an
1078 implementation support [RFC8654] to accommodate a larger size of
1079 information within the BGP-LS Attribute. BGP-LS Producers MUST
1080 ensure that they limit the TLVs included in the BGP-LS Attribute to
1081 ensure that a BGP update message for a single Link-State NLRI does
1082 not cross the maximum limit for a BGP message. The determination of
1083 the types of TLVs to be included MAY be made by the BGP-LS Producer
1084 based on the BGP-LS Consumer applications requirement and is outside
1085 the scope of this document. When a BGP-LS Propagator finds that it
1086 is exceeding the maximum BGP message size due to addition or update
1087 of some other BGP Attribute (e.g. AS_PATH), it MUST consider the
1088 BGP-LS Attribute to be malformed and handle the propagation as
1089 described in Section 7.2.2. This brings the deployment consideration
1090 where the consistent propagation of BGP-LS information with an update
1091 size larger than 4096 octets can only happen along a set of BGP
1092 Speakers that all support [RFC8654].

[minor] RFC8654 formally Updated rfc4271, so there's no need to
mention the base spec here.

OLD>
The BGP specification [RFC4271] mandates a maximum BGP message size
of 4096 octets. It is RECOMMENDED that an implementation support
[RFC8654] to accommodate a larger size of information within the
BGP-LS Attribute.

NEW>
It is RECOMMENDED that an implementation support [RFC8654] to
accommodate a larger size of information within the BGP-LS Attribute.

[major] "The determination of the types of TLVs to be included MAY be
made by the BGP-LS Producer based on the BGP-LS Consumer applications
requirement and is outside the scope of this document."

If the determination and the consumer applications are out of scope,
we can't use Normative language. In this case, the "MAY" is
indicating a fact anyway. s/MAY/may

[major] "When a BGP-LS Propagator finds that it is exceeding the
maximum BGP message size due to addition or update of some other BGP
Attribute (e.g. AS_PATH), it MUST consider the BGP-LS Attribute to be
malformed and handle the propagation as described in Section 7.2.2."

This sentence refers to actions to be taken when forwarding the BGP
UPDATE, but the text in §7.2.2 refers to checks done when the UPDATE
is received... In any case, if I guess correctly, this is the text
that applies:

When the error determined allows for the router to skip the malformed
BGP-LS Attribute and continue the processing of the rest of the
update message (e.g. when the BGP-LS Attribute length and the total
Path Attribute Length are correct but some TLV/sub-TLV length within
the BGP-LS Attribute is invalid), then it MUST handle such malformed
BGP-LS Attribute as 'Attribute Discard'.

Is that it? Can we be explicit here about the action?

Suggestion>
When a BGP-LS Propagator finds that it is exceeding the maximum
BGP message size due to addition or update of some other BGP
Attribute (e.g. AS_PATH), it MUST consider the BGP-LS Attribute
to be malformed, apply the "Attribute discard" error-handling
approach [rfc7606], and handle the propagation as described in
Section 7.2.2.

[minor] s/with an update/with a BGP UPDATE

...
1168 4.3.1.3. Node Name TLV
...
1185 [RFC5301] describes an IS-IS-specific extension and [RFC5642]
1186 describes an OSPF extension for the advertisement of Node Name which
1187 MAY encoded in the Node Name TLV.

[major] The "MAY" is not normative, it just states a fact: s/MAY/may

[nit] s/may encoded/may be encoded

...
1207 4.3.1.5. Opaque Node Attribute TLV
...
1224 In the case of OSPF, this TLV MAY be used to advertise information
1225 carried using the TLVs in the "OSPF Router Information (RI) TLVs"
1226 registry [RFC7770] under the IANA OSPF Parameters registry.

[major] "this TLV MAY be used"

If not used this way, what's the other option? I'm trying to figure
out the normative value of this "MAY" -- it seems to me that it is
just providing an example of things that can be included, or do you
intend it to be Normative?

Other RFCs define BGP-LS extensions for some RI TLVs (rfc8814, for
example). Even if (normatively) optional, is it ok to also send the
MSD information in this Opaque Node Attribute TLV when rfc8814 is also
supported?

s/MAY/may

** §4.3.2.6 and §4.3.3.6 have similar statements. The same comment
applies there.

[minor] s/in the "OSPF Router Information (RI) TLVs" registry
[RFC7770] under the IANA OSPF Parameters registry./in the OSPF Router
Information (RI) LSA [RFC7770].

...
1398 4.3.2.6. Opaque Link Attribute TLV
...
1415 In the case of OSPFv2, this TLV MAY be used to advertise information
1416 carried using the TLVs in the "OSPFv2 Extended Link Opaque LSA TLVs"
1417 registry [RFC7684] under the IANA OSPFv2 Parameters registry. In the
1418 case of OSPFv3, this TLV MAY be used to advertise information carried
1419 using the TLVs in the "OSPFv3 Extended-LSA Sub-TLVs" registry
1420 [RFC8362] under the IANA OSPFv3 Parameters registry.

[minor] s/in the "OSPFv2 Extended Link Opaque LSA TLVs" registry
[RFC7684] under the IANA OSPFv2 Parameters registry./in the OSPFv2
Extended Link Opaque LSA [RFC7684].

[minor] s/in the "OSPFv3 Extended-LSA Sub-TLVs" registry [RFC8362]
under the IANA OSPFv3 Parameters registry./in an OSPFv3 Extended LSA
[RFC8362].

...
1598 4.3.3.6. Opaque Prefix Attribute TLV
...
1615 In the case of OSPFv2, this TLV MAY be used to advertise information
1616 carried using the TLVs in the "OSPFv2 Extended Prefix Opaque LSA
1617 TLVs" registry [RFC7684] under the IANA OSPFv2 Parameters registry.
1618 In the case of OSPFv3, this TLV MAY be used to advertise information
1619 carried using the TLVs in the "OSPFv3 Extended-LSA Sub-TLVs" registry
1620 [RFC8362] under the IANA OSPFv3 Parameters registry.

[minor] s/in the "OSPFv2 Extended Prefix Opaque LSA TLVs" registry
[RFC7684] under the IANA OSPFv2 Parameters registry./in the OSPFv2
Extended Prefix LSA [RFC7684].

[minor] s/in the "OSPFv3 Extended-LSA Sub-TLVs" registry [RFC8362]
under the IANA OSPFv3 Parameters registry./in an OSPFv3 Extended LSA
[RFC8362].

...
1636 4.4. Private Use

1638 TLVs for Vendor Private use are supported using the code point range
1639 reserved as indicated in Section 6. For such TLV use in the NLRI or
1640 BGP-LS Attribute, the format as described in Section 4.1 is to be
1641 used and a 4 octet field MUST be included as the first field in the
1642 value to carry the Enterprise Code. For a private use NLRI Type, a 4
1643 octet field MUST be included as the first field in the NLRI
1644 immediately following the Total NLRI Length field of the Link-State
1645 NLRI format as described in Section 4.2 to carry the Enterprise Code.
1646 The Enterprise Codes are listed at <http://www.iana.org/assignments/
1647 enterprise-numbers>. This enables the use of vendor-specific
1648 extensions without conflicts.

1650 Multiple instances of private-use TLVs MAY appear in the BGP-LS
1651 Attribute.

[major] This whole section is not needed. The IANA Considerations
section should indicate the ranges that are for Private Use.

Also, because the use is *private*, this document cannot specify what
the contents are. It is a given that a TLV has to comply with the
format specified in this document, but there's no need to point at
that again.

Note the definition of "Private Use" from rfc8126:

Private Use is for private or local use only, with the type and
purpose defined by the local site. No attempt is made to prevent
multiple sites from using the same value in different (and
incompatible) ways. IANA does not record assignments from registries
or ranges with this policy (and therefore there is no need for IANA
to review them) and assignments are not generally useful for broad
interoperability. It is the responsibility of the sites making use
of the Private Use range to ensure that no conflicts occur (within
the intended scope of use).

...
1688 4.7. OSPF Virtual Links and Sham Links

[] There are other mentions of "virtual links" (from the original
text) that don't refer to OSPF virtual links (for example, §4,2 says
this: "For modeling virtual links, such as described in Section 5, the
Protocol-ID 'Static configuration' SHOULD be used."). Please make it
is clear what you're referring to (by qualifying the other instances).

...
1720 4.9. Handling of Unreachable IGP Nodes

1722 The origination and propagation of IGP link-state information via BGP
1723 needs to provide a consistent and true view of the topology of the
1724 IGP domain. BGP-LS provides an abstraction of the IGP specifics and
1725 BGP-LS Consumers may be varied types of applications. While the
1726 information propagated via BGP-LS from a link-state routing protocol
1727 is sourced from that protocol's LSDB, it does not serve as a true
1728 reflection of the originating router's LSDB since it does not include
1729 the LSA/LSP sequence number information. The sequence numbers are
1730 not included since a single NLRI update may be put together with
1731 information that is coming from multiple LSAs/LSPs.

[] This is a nice introductory paragraph to BGP-LS in general. Too
bad it is hidden here and not in the Introduction (for example).

[minor] "...it does not serve as a true reflection of the originating
router's LSDB since it does not include the LSA/LSP sequence number
information."

There's also other information (virtual links, Type 4 LSAs, etc.) that
is not included.

...
1738 A BGP-LS Consumer talks to a BGP route-reflector (RR) R0 which is
1739 aggregating the BGP-LS feed from the BGP-LS Producers R2 and R3.
1740 Here R2 and R3 provide a redundant topology feed via BGP-LS to R0.
1741 Normally, R0 would receive two identical copies of all the Link-State
1742 NLRIs from both R2 and R3 and it would pick one of them (say R2)
1743 based on the standard BGP best-path decision process.

[minor] s/route-reflector/route reflector/g

1745 Consumer
1746 ^
1747 |
1748 R0
1749 (BGP Route Reflector)
1750 / \
1751 / \
1752 a1 / a0 \ a1
1753 R1 ------ R2 -------- R3 ------ R4
1754 a1 | | a1
1755 | |
1756 R5 ---------------------------- R6
1757 a1

1759 Figure 31: Incorrect Reporting due to BGP Path Selection

[nit] s/Consumer/BGP-LS Consumer

[minor] Please explain what a0 and a1 mean.

[minor] Please indicate that R0 is a BGP-LS Propagator.

...
1765 Now, R5 will remove the link 5-6 from its Router LSA, and this
1766 updated LSA is available at R2. R2 also has a stale copy of R6's
1767 Router LSA which still has the link 6-5 in it. Based on this view in
1768 its LSDB, R2 will advertise only the half-link 6-5 that it derives
1769 from R6's stale Router LSA.

[nit] s/5-6/R5-R6/g

[nit] s/6-5/R6-R5/g

...
1777 Now, the BGP-LS Consumer receives both the Link NLRIs corresponding
1778 to the half-links from R2 and R3 via R0. When viewed together, it
1779 would not detect or realize that the area 1 is partitioned. Also if
1780 R2 continues to report Link-State NLRIs corresponding to the stale
1781 copy of Router LSA of R4 and R6 nodes then R0 would prefer them over
1782 the valid Link-State NLRIs for R4 and R6 that it is receiving from R3
1783 based on its BGP decision process. This would result in the BGP-LS
1784 Consumer getting stale and inaccurate topology information. This
1785 problem scenario is avoided if R2 were to not advertise the link-
1786 state information corresponding to R4 and R6 and if R3 were to not
1787 advertise similarly for R1 and R5.

[nit] s/Router LSA of R4 and R6 nodes/R4 and R6's Router LSAs

[?] "...if R2 continues to report Link-State NLRIs corresponding to
the stale copy of Router LSA of R4 and R6 nodes then R0 would prefer
them over the valid Link-State NLRIs for R4 and R6 that it is
receiving from R3 based on its BGP decision process."

Can you please explain why this is the case? Wouldn't the NLRIs be
different and both be propagated? I'm probably missing something.

1789 A BGP-LS Producer SHOULD withdraw all link-state objects advertised
1790 by it in BGP when the node that originated its corresponding LSP/LSAs
1791 is determined to have become unreachable in the IGP. An
1792 implementation MAY continue to advertise link-state objects
1793 corresponding to unreachable nodes in a deployment use-case where the
1794 BGP-LS Consumer is interested in receiving a topology feed
1795 corresponding to a complete IGP LSDB view. In such deployments, it
1796 is expected that the problem described above is mitigated by the BGP-
1797 LS Consumer via appropriate handling of such a topology feed in
1798 addition to the use of either a direct BGP peering with the producer
1799 nodes or mechanisms such as [RFC7911] when using RR. Details of
1800 these mechanisms are outside the scope of this draft.

[minor] s/with the producer/with the BGP-LS Producer

[nit] s/using RR/using RRs

[nit] s/this draft/this document

...
1841 4.11. Router-ID Anchoring Example: OSPF Pseudonode

[] Please use addresses reserved for documentation in the examples (RFC6890).

...
2002 6. IANA Considerations

[major] Please include this text:

Because this document obsoletes [RFC7752] and [RFC9029], IANA is
asked to change all registration information that references those
documents to instead reference [[this document]].

Then we can get rid of redundant text below.

...
2032 6.1.1. BGP-LS NLRI Types Registry
...
2038 Type NLRI Type Reference
2039 -----------------------------------------------------------
2040 0 Reserved [This document]
2041 1 Node NLRI [This document]
2042 2 Link NLRI [This document]
2043 3 IPv4 Topology Prefix NLRI [This document]
2044 4 IPv6 Topology Prefix NLRI [This document]
2045 65000-65535 Private Use [This document]

2047 Allocations within the registry under the "Expert Review" policy
2048 require documentation of the proposed use of the allocated value and
2049 approval by the Designated Expert assigned by the IESG (see
2050 [RFC8126]).

[minor] Please also point at the definition of the "Private Use" policy.

Suggestion>
A range is reserved for Private Use [rfc8126]. All other allocations
are to be made using the "Expert Review" policy...

[nit] s/(see [RFC8126])/[RFC8126]/g

2052 6.1.2. BGP-LS Protocol-IDs Registry
...
2058 Protocol-ID NLRI information source protocol Reference
2059 ---------------------------------------------------------------------
2060 0 Reserved [This document]
2061 1 IS-IS Level 1 [This document]
2062 2 IS-IS Level 2 [This document]
2063 3 OSPFv2 [This document]
2064 4 Direct [This document]
2065 5 Static configuration [This document]
2066 6 OSPFv3 [This document]
2067 200-255 Private Use [This document]

2069 Allocations within the registry under the "Expert Review" policy
2070 require documentation of the proposed use of the allocated value and
2071 approval by the Designated Expert assigned by the IESG (see
2072 [RFC8126]).

[minor] Please also point at the definition of the "Private Use" policy.

Suggestion>
A range is reserved for Private Use [rfc8126]. All other allocations
are to be made using the "Expert Review" policy...

2074 6.1.3. BGP-LS Well-Known Instance-IDs Registry

2076 The "BGP-LS Well-Known Instance-IDs" registry that was set up via
2077 [RFC7752] is no longer required. It may be retained as deprecated
2078 and no further assignments be made from it.

[major] If not needed, why retain it? That can only cause confusion.

2080 6.1.4. BGP-LS Node Flags Registry
...
2086 Bit Description Reference
2087 -----------------------------------------------
2088 0 Overload Bit (O-bit) [This document]
2089 1 Attached Bit (A-bit) [This document]
2090 2 External Bit (E-bit) [This document]
2091 3 ABR Bit (B-bit) [This document]
2092 4 Router Bit (R-bit) [This document]
2093 5 V6 Bit (V-bit) [This document]
2094 6-7 Unassigned

2096 Allocations within the registry under the "RFC Required" policy (see
2097 [RFC8126]).

[major] The RFC Required policy allows any RFC to allocate a value;
from rfc8126:

With the RFC Required policy, the registration request, along with
associated documentation, must be published in an RFC. The RFC need
not be in the IETF stream, but may be in any RFC stream (currently an
RFC may be in the IETF, IRTF, IAB, or Independent Submission streams
[RFC5742]).

Unless otherwise specified, any type of RFC is sufficient (currently
Standards Track, BCP, Informational, Experimental, or Historic).

IOW, no one is required to review the request. Given the size of this
range, it seems like a risky strategy. Why are you deviating from the
existing "Expert Review" criteria?

2099 6.1.5. BGP-LS MPLS Protocol Mask Registry
...
2111 Allocations within the registry under the "RFC Required" policy (see
2112 [RFC8126]).

[major] Same comment as above about the RFC Required registration policy.

2114 6.1.6. BGP-LS IGP Prefix Flags Registry
...
2128 Allocations within the registry under the "RFC Required" policy (see
2129 [RFC8126]).

[major] Same comment as above about the RFC Required registration policy.

2131 6.1.7. BGP-LS TLVs Registry

2133 The "BGP-LS Node Descriptor, Link Descriptor, Prefix Descriptor, and
2134 Attribute TLVs" registry was setup via [RFC7752]. This document
2135 requests IANA to rename that registry to "BGP-LS NLRI and Attribute
2136 TLVs" and to remove the column for "IS-IS TLV/Sub-TLV" from the
2137 registry since that column is not relevant for the allocation and
2138 maintenance of BGP-LS code points. The values 0-255 are reserved.
2139 The values 256-65535 will be used for code points allocation. The
2140 range 65000-65535 is for Private Use. The registry has been populated
2141 with the values shown in Table 12 and the reference for all those
2142 allocations should be updated to this document instead of [RFC7752].
2143 Allocations within the registry under the "Expert Review" policy
2144 require documentation of the proposed use of the allocated value and
2145 approval by the Designated Expert assigned by the IESG (see
2146 [RFC8126]).

[major] "...Table 12 and the reference for all those allocations
should be updated to this document instead of [RFC7752]."

I'm assuming that this statement applies to all entries in Table 12
and not just the ones referencing rfc7752. Is that correct?

[minor] There's some redundancy above.

Suggestion>

The "BGP-LS Node Descriptor, Link Descriptor, Prefix Descriptor,
and Attribute TLVs" registry was setup via [RFC7752]. This document
requests IANA to rename that registry to "BGP-LS NLRI and Attribute
TLVs" and to remove the "IS-IS TLV/Sub-TLV" column. Table xx
summarizes shows the registration procedures. The registry was pre-
populated with the values shown in Table 12 and the reference for
all those allocations should be updated to [this document].

[minor] Please include a table that shows the allocation policies --
in this case two lines: one showing the "Expert Review" range and the
other for "Private Use".

...
2213 7.1.1. Operations

2215 Existing BGP operational procedures apply. No new operation
2216 procedures are defined in this document. It is noted that the NLRI
2217 information present in this document carries purely application-level
2218 data that has no immediate impact on the corresponding forwarding
2219 state computed by BGP. As such, any churn in reachability
2220 information has a different impact than regular BGP updates, which
2221 need to change the forwarding state for an entire router. It is
2222 expected that the distribution of this NLRI SHOULD be handled by
2223 dedicated route reflectors in most deployments providing a level of
2224 isolation and fault containment between different NLRI types. In the
2225 event of dedicated route reflectors not being available, other
2226 alternate mechanisms like separation of BGP instances or separate BGP
2227 sessions (e.g. using different addresses for peering) for Link-State
2228 information distribution SHOULD be used.

[minor] s/It is expected that the distribution of this NLRI SHOULD be
handled by dedicated route reflectors in most deployments
providing/Distribution of the BGP-LS NLRI SHOULD be handled by
dedicated route reflectors to provide

...
2252 7.1.5. Impact on Network Operation

2254 The frequency of Link-State NLRI updates could interfere with regular
2255 BGP prefix distribution. A network operator MAY use a dedicated
2256 Route-Reflector infrastructure to distribute Link-State NLRIs.

[major] "MAY use a dedicated Route-Reflector infrastructure"

This statement is in contradiction with the recommendation in §7.1.1.
To solve that, you can either restate ("As mentioned in §7.1.1....")
or s/MAY/may

...
2281 7.2.2. Fault Management
...
2292 A BGP-LS Speaker MUST perform the following syntactic validation of
2293 the Link-State NLRI to determine if it is malformed.

2295 o Does the sum of all TLVs found in the BGP MP_REACH_NLRI attribute
2296 correspond to the BGP MP_REACH_NLRI length?

[major] "sum of all TLVs...correspond to the BGP MP_REACH_NLRI length"

The sum of all TLVs results in the number of TLVs, which is not the
same as the length. s/sum of all TLVs/sum of all TLV lengths

Note that the next two bullets have the same issue.

2298 o Does the sum of all TLVs found in the BGP MP_UNREACH_NLRI
2299 attribute correspond to the BGP MP_UNREACH_NLRI length?

2301 o Does the sum of all TLVs found in a Link-State NLRI correspond to
2302 the Total NLRI Length field of all its Descriptors?

2304 o Is the length of the TLVs and, when the TLV is recognized then,
2305 its sub-TLVs in the NLRI valid?

[minor] s/.../Is the length of the TLVs and sub-TLVs in the NLRI valid?

...
2313 When the error determined allows for the router to skip the malformed
2314 NLRI(s) and continue the processing of the rest of the update message
2315 (e.g. when the TLV ordering rule is violated), then it MUST handle
2316 such malformed NLRIs as 'Treat-as-withdraw'. In other cases, where
2317 the error in the NLRI encoding results in the inability to process
2318 the BGP update message (e.g. length related encoding errors), then
2319 the router SHOULD handle such malformed NLRIs as 'AFI/SAFI disable'
2320 when other AFI/SAFI besides BGP-LS are being advertised over the same
2321 session. Alternately, the router MUST perform 'session reset' when
2322 the session is only being used for BGP-LS or when it 'AFI/SAFI
2323 disable' action is not possible.

[minor] s/when it/if (?)

When would that not be possible? Are you thinking of a bug (or maybe
a configuration), or something else?

...
2330 A BGP-LS Speaker MUST perform the following syntactic validation of
2331 the BGP-LS Attribute to determine if it is malformed.

2333 o Does the sum of all TLVs found in the BGP-LS Attribute correspond
2334 to the BGP-LS Attribute length?

[major] s/sum of all TLVs/sum of all TLV lengths

...
2339 o Is the length of each TLV and, when the TLV is recognized then,
2340 its sub-TLVs in the BGP-LS Attribute valid?

[minor] s/.../Is the length of each TLV and sub-TLV in the BGP-LS
Attribute valid?

...
2352 Note that the 'Attribute Discard' action results in the loss of all
2353 TLVs in the BGP-LS Attribute and not the removal of a specific
2354 malformed TLV. The removal of specific malformed TLVs may give a
2355 wrong indication to a BGP-LS Consumer of that specific information
2356 being deleted or not available.

[] The same is true for the 'Treat-as-withdraw' case with the NLRI --
by, for example, not ordering the TLVs, the information is lost.

[major] This point is here for the record...

rfc7606 says that 'attribute discard' "MUST NOT be used except in the
case of an attribute that has no effect on route selection or
installation."

Even if the operation of the BGP-LS Consumer is out of scope, it is
expected to (in many cases) calculate routes and program the network
with them. As explained above (and considering also the NLRI
'Treat-as-withdraw' case), the BGP-LS Consumer will not have a
complete picture which can affect routing in the network.

A simple attack vector is to not order the TLVs in an otherwise valid
route, or change the length of a TLV... The impact goes beyond BGP-LS
and what this document covers. :-(

2358 When a BGP Speaker receives an update message with Link-State NLRI(s)
2359 in the MP_REACH_NLRI but without the BGP-LS Attribute, it is most
2360 likely an indication that a BGP Speaker preceding it has performed
2361 the 'Attribute Discard' fault handling. An implementation SHOULD
2362 preserve and propagate the Link-State NLRIs in such an update message
2363 so that the BGP-LS Consumers can detect the loss of link-state
2364 information for that object and not assume its deletion/withdraw.
2365 This also makes it possible for a network operator to trace back to
2366 the BGP-LS Propagator that detected the fault with the BGP-LS
2367 Attribute.

[major] "SHOULD preserve and propagate"

This is a very useful signal that something went wrong. Why is it
recommended and not required? When is it ok to not preserve and
propagate.

2369 An implementation SHOULD log an error for any errors found during
2370 syntax validation for further analysis.

[minor] "log an error for any errors"

Perhaps "log a message"?

2372 A BGP-LS Propagator, even when also operating as a BGP-LS Consumer,
2373 SHOULD NOT perform semantic validation of the Link-State NLRI or the
2374 BGP-LS Attribute to determine if it is malformed or invalid. Some
2375 types of semantic validation that are not to be performed by a BGP-LS
2376 Propagator are as follows (and this is not to be considered as an
2377 exhaustive list):

[major] "BGP-LS Propagator, even when also operating as a BGP-LS Consumer"

Going back to the definitions back in §3, from the BGP point of view,
the BGP-LS Propagator cannot operate as a BGP-LS Consumer because the
consumer is not a BGP Speaker. Maybe you meant something along the
lines of: "even when the BGP-LS Consumer application coexists in the
same node". ??

[major] "BGP-LS Propagator...SHOULD NOT perform semantic validation"

When it is ok for the BGP-LS Propagator to perform the validation?
Why is this action recommended and not required?

Semantic validation is not defined in this document (or any other
BGP-LS document), so my question is really, why would the BGP-LS
Propagator even consider the validation? IOW, the "SHOULD NOT" seems
to be stating a fact, and not be a Normative statement: s/SHOULD
NOT/should not

...
2389 Each TLV MAY indicate the valid and permissible values and their
2390 semantics that can to be used only by a BGP-LS Consumer for its
2391 semantic validation. However, the handling of any errors may be
2392 specific to the particular application and outside the scope of this
2393 document. A BGP-LS Consumer should ignore unrecognized and
2394 unexpected TLV types in both the NLRI and BGP-LS Attribute portions
2395 and not consider their presence as an error.

[major] "TLV MAY indicate the valid and permissible values"

This sounds like a statement of fact: s/MAY/may

[major] The last sentence is a specific instruction to the BGP-LS
Consumer -- it should be removed.

2397 7.2.3. Configuration Management
...
2421 An implementation SHOULD allow the operator to configure the maximum
2422 size of the BGP-LS Attribute that may be used on a BGP-LS Producer.

[?] How does limiting the size of the BGP-LS Attribute help anything?
Is this related to the maximum message size or something else?

...
2444 7.2.6. Security Management

2446 An operator SHOULD define an import policy to limit inbound updates
2447 as follows:

2449 o Drop all updates from peers that are only serving BGP-LS
2450 Consumers.

[major] The "SHOULD" above conflicts with "MUST NOT accept updates" in
§9. I realize that the action here is about the operator defining
policy vs a BGP speaker not accepting updates...but the result of the
specification would be inconsistent if the requirement in §9 depends
on the recommendation here.

See also related comments in §9 related to "peers that are only
serving BGP-LS Consumers".

...
2515 9. Security Considerations
...
2522 In the context of the BGP peerings associated with this document, a
2523 BGP speaker MUST NOT accept updates from a peer that is only
2524 providing information to a BGP-LS Consumer. That is, a participating
2525 BGP speaker should be aware of the nature of its relationships for
2526 link-state relationships and should protect itself from peers sending
2527 updates that either represent erroneous information feedback loops or
2528 are false input. Such protection can be achieved by manual
2529 configuration of consumer peers at the BGP speaker.

[major] "MUST NOT accept updates from a peer that is only providing
information to a BGP-LS Consumer"

This piece of text changed from "MUST NOT accept updates from a
consumer peer". In that context, the requirement is a local decision
(the node knows the nature of the connection to the consumer peer) and
can (if the connection was in fact a BGP session) enforce the
requirement.

The new text implies that other BGP speakers are aware of where the
BGP-LS Consumers are and what the role of each BGP speaker connected
to them is. How is that communicated so that the requirement can be
enforced?

I could see the requirement being that "updates from BGP-LS Consumers
MUST be ignored", but the nature of "the interface between BGP and
consumer application ...[is] outside the scope of this document" (from
§3), so I don't see how the behavior can be mandated.

Tying in the text from §7.2.6 -- is the expectation that this
requirement be fulfilled by the operator by configuration? If so,
then the actions in §7.2.6 have to be required, and I would like to
see a discussion (a couple of sentences would be ok) about what the
risk is. The text above talks about "updates that either represent
erroneous information feedback loops or are false input" -- erroneous
or false information is a risk anyway (it may have even been erroneous
at the IGP level). I don't see how a feedback loop could cause issues
(beyond more traffic in the control plane).

2531 An operator SHOULD employ a mechanism to protect a BGP speaker
2532 against DDoS attacks from BGP-LS Consumers. The principal attack a
2533 consumer may apply is to attempt to start multiple sessions either
2534 sequentially or simultaneously. Protection can be applied by
2535 imposing rate limits.

[major] "The consumer application and the design of the interface
between BGP and consumer application may be implementation specific
and outside the scope of this document."

...it is not possible to even recommend what that interface should do.

Note that even if the details are out of scope, you may be able to
provide (in §3) some expectations. For example, from the point of
view of BGP-LS, the communication of information to the BGP-LS
Consumer is expected to be one-way: only from the BGP speaker to the
consumer application. Something like this would address the points in
the last two paragraphs. The Security Considerations would then be
around that expectation not being met (but any mitigation would also
be outside the scope of this document).

2537 Additionally, it may be considered that the export of link-state and
2538 TE information as described in this document constitutes a risk to
2539 confidentiality of mission-critical or commercially sensitive
2540 information about the network. BGP peerings are not automatic and
2541 require configuration; thus, it is the responsibility of the network
2542 operator to ensure that only trusted consumers are configured to
2543 receive such information.

[major] "BGP peerings are not automatic...it is the responsibility of
the network operator to ensure that only trusted consumers..."

Again, the interface is outside the scope of this document and the
text above assumes a BGP session. Rephrase in terms of a risk with a
mitigation out of scope.

[major] There are other risks that result from the use of BGP-LS, for example:

- originating information that is not in the LSDB (i.e. false
information), or leaving information out

- removing the BGP-LS Attribute (for not reason)

- reordering the TLVs in an otherwise good NLRI

Yes, all these make assumptions about what the BGP-LS Consumer will do
with the information (which is stated at the front of the document).
And these are not new things in BGP. However, they are new
BGP-LS-specific risks. Mentioning them and acknowledging that these
are existing risks in BGP too would go a long way to show that all
angles have been considered.

...
2604 12.1. Normative References
...
2709 [RFC6549] Lindem, A., Roy, A., and S. Mirtorabi, "OSPFv2 Multi-
2710 Instance Extensions", RFC 6549, DOI 10.17487/RFC6549,
2711 March 2012, <https://www.rfc-editor.org/info/rfc6549>.

[minor] This reference can be Informative.

...
2728 [RFC7752] Gredler, H., Ed., Medved, J., Previdi, S., Farrel, A., and
2729 S. Ray, "North-Bound Distribution of Link-State and
2730 Traffic Engineering (TE) Information Using BGP", RFC 7752,
2731 DOI 10.17487/RFC7752, March 2016,
2732 <https://www.rfc-editor.org/info/rfc7752>.

[major] This document obsoletes rfc7752, so this reference should be
Informative.

...
2743 [RFC8202] Ginsberg, L., Previdi, S., and W. Henderickx, "IS-IS
2744 Multi-Instance", RFC 8202, DOI 10.17487/RFC8202, June
2745 2017, <https://www.rfc-editor.org/info/rfc8202>.

[minor] This reference can be Informative.

...
2756 [RFC9029] Farrel, A., "Updates to the Allocation Policy for the
2757 Border Gateway Protocol - Link State (BGP-LS) Parameters
2758 Registries", RFC 9029, DOI 10.17487/RFC9029, June 2021,
2759 <https://www.rfc-editor.org/info/rfc9029>.

[major] This document obsoletes rfc9029, so this reference should be
Informative.

2761 12.2. Informative References
...
2819 [RFC7770] Lindem, A., Ed., Shen, N., Vasseur, JP., Aggarwal, R., and
2820 S. Shaffer, "Extensions to OSPF for Advertising Optional
2821 Router Capabilities", RFC 7770, DOI 10.17487/RFC7770,
2822 February 2016, <https://www.rfc-editor.org/info/rfc7770>.

[major] This reference should be Normative.

[EoR -10]

[Idr] AD Review of draft-ietf-idr-rfc7752bis-10 Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-rfc7752bis-… Ketan Talaulikar
Re: [Idr] AD Review of draft-ietf-idr-rfc7752bis-… Ketan Talaulikar
Re: [Idr] AD Review of draft-ietf-idr-rfc7752bis-… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-rfc7752bis-… Ketan Talaulikar
Re: [Idr] AD Review of draft-ietf-idr-rfc7752bis-… Alvaro Retana
Re: [Idr] AD Review of draft-ietf-idr-rfc7752bis-… Ketan Talaulikar
Re: [Idr] AD Review of draft-ietf-idr-rfc7752bis-… Alvaro Retana