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

[Ketan Talaulikar <[e-mail]>]

Hi Alvaro,

Thanks for your detailed review and comments/feedback. I will work on them
and get back with updates next week.

Thanks,
Ketan


On Wed, Sep 21, 2022 at 1:00 AM Alvaro Retana <[e-mail]>
wrote:

> 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]
>