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

[Ketan Talaulikar <[e-mail]>]

Hi Alvaro,

Thanks for your detailed review and feedback/suggestions. Please check
inline below for responses.

I have also posted an updated version with the changes discussed below:
https://datatracker.ietf.org/doc/html/draft-ietf-idr-rfc7752bis-11


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%.
>

KT> This depth is coming from RFC7752 and I've found it very useful.
Reducing it further will make it more difficult to jump to the specifics.
IMHO the current level strikes a good balance.


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

KT> Ack


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

KT> Ack


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

KT> Fixed the figure and description in Section 4.9 to use the RR
terminology as in Figure 1.


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

KT> Ack


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

KT> I believe this is an important concept to describe. Do you see an issue
with doing so? Further in Sec 4.9, we get into some of the challenges this
poses and so it seemed good to first introduce that two BGP producers
sourcing info from the same IGP area are going to be originating the same
information.


>
> - 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).
>

KT> Added a sentence about "other sources" in the introduction section.


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

KT> Ack


>
>
> [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.
>

KT> Ack. However, I've added this text in Sec 4 instead as that seemed more
appropriate.


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

KT> Ack


>
>
> [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.
>

KT> Fixed the description in Figure 2 & 3 - removed the "BGP LS NRLI" from
those interfaces.


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

KT> Ack


>
>
> [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.


KT> Correct.


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

KT> Correct.


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

KT> Yes, that was the idea. Further in the section, we cover how these
roles may be mixed up for a given BGP Speaker.


>
>
> [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.
>

KT> Ack


>
>
> [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?


KT> There is also the parsing/validation of the link-state information.


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

KT> Ack to all 3 above.


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

KT> Ack


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

KT> Agree that the text is unclear and somewhat conflicting. Can you please
check if the updated text addresses your points above?


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

KT> Ack


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

KT> "Unknown" and "unrecognized" are the same - text is now updated with
"unknown" everywhere. "Unexpected" would be when a TLV that is associated
with say a Node NLRI gets included with a Link NLRI - this is clarified.
The difference between "unknown" and "unsupported" is that while the TLV
type code point is known in the later case, the support (e.g., for
origination) may not be complete.


>
>
> ...
> 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?
>

KT> The goal is to specify which TLVs are really required so as to
adequately describe and advertise a given link-state object. So this is
guidance for the BGP-LS Producer implementations and as an extension is
helpful for consumers too.


>
>
> ...
> 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).
>

KT> The objective here is to describe the consequences of implementation
variations that we have seen in the production and interop tests. Some of
them may be from implementations that followed RFC7752 that was not as
clear about which TLVs are optional and which are mandatory in the NLRI.
Even with this document (and other BGP-LS specs), we can still have
inconsistencies in the BGP-LS attribute simply because two implementations
don't support the same attributes for an object.


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

KT> Ack


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

KT> Ack


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

KT> ack


>
>
> [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".
>

KT> The name of the field in the BGP-LS NLRI is "Identifier" and what it
carries is called the "BGP-LS Instance-ID". The text is updated to refer to
the "Identifier" field only in the figures and then once more where we
indicate that this field carries the BGP-LS Instance-ID. The rest of the
text is updated to refer to the BGP-LS Instance-ID alone. I hope this will
clarify. Ideally, I would have liked to rename the Identifier field to
BGP-LS Instance-ID, but there is also another BGP-LS Identifier TLV (now
deprecated) which can get even more confusing.


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

KT> Indeed, it is a restatement. Have removed the use of normative language
from the earlier paragraph and retained the normative language meant as
operator guidance. Also, moved text to consolidate.


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

KT> Ack. Changed "consistent" to "same".


>
>
> [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.
>

KT> Ack. Added a reference in Secs 7.1 and 7.2.3 to point to this section.
Please suggest if something further needs to be added.


>
>
> [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.
>

KT> Ack


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

KT> Ack


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

KT> Fixed. Changed "duplicate" to "more than one" and clarified the
Instance-ID part.


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

KT> Ack


>
>
> ...
> 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?
>

KT> Ack. Clarified.


>
>
> [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.
>

KT> Ack


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

KT> Updated the text.


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

KT> The SHOULD was to cover for those implementations following RFC7752
that may be doing something different. E.g., if the node also has OSPF
configured and is originating local (non-OSPF) info as well, then it could
use the OSPF Router-ID that does not necessarily need to be an IP address
on the node. Added this clarification.


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

KT> Seems like I missed making this normative and also not covered it in
the error handling. The NLRI should be considered malformed in that case.
Fixed the text and the error handling section too.


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

KT> Changed the MUST to SHOULD and added clarification in the text. Note
that this is a TLV in the NLRI. So when there are multiple BGP-LS Producers
that are a mix of RFC7752 and this BIS, we can end up in multiple
link-state objects due to differences in NLRI encoding. Hopefully, at some
point, we can stop advertising this.


>
>
> [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?
>

KT> We have covered it normatively previously. This was just a
restatement/reminder. Updated the text.


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

KT> Ack. Updated text.


>
>
> 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 "-".
>

KT> Ack


>
>
> [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.
>

KT> Whether the result of 2-way connectivity check is stored/available in
the LSDB is implementation specific. It is not so that information from
LSAs/LSPs can be simply plucked from a LSDB built from raw IGP PDUs and
advertised into BGP-LS. Information from multiple LSAs/LSPs can contribute
to a single link-state object. This requires additional processing in the
form of collating information into some form of a topology database (nodes,
links, prefixes). Ultimately, it is up to the implementation how it
realizes the LSDB.


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

KT> Fixed


>
>
> [major] "MUST NOT be carried in the IPv6 address TLVs"
>
> Please be explicit by using the names of the TLVs.
>

KT> Fixed.


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

KT> Ack


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

KT> Ack


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

KT> Ack


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

KT> Agree. Things were not very clear. Can you please check the updated
text?


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

KT> You are correct. However, out there in production, the
deployment/support for RFC8654 is not really widespread. Hence, I believe,
the mention of the 4K limit and reference to RFC4271 is important for
implementations. Also for operators as mentioned later in the same section.


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

KT> Ack.


>
>
> [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.
>

KT> Ack and thanks for the suggestion. Also, added the handling for when a
neighbor does not support extended messages that I realized was missed out.


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

KT> Ack


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

KT> Ack


>
>
> [nit] s/may encoded/may be encoded
>

KT> Ack


>
>
> ...
> 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?
>

KT> The point was that only the OSPF RI LSA TLVs can be encoded in this
node opaque attribute. Given that the TLV/sub-TLV space is determined on a
per LSA basis, it is not possible to mix "native" OSPF encodings from
different LSAs. We needed to be specific on how a receiver may be able to
parse and interpret these opaque OSPF encodings. I've updated the text to
clarify this and please let me know if that works.


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

KT> That would be duplication and so not make sense - even if it is not a
problem. It can so happen that a Producer does not (yet) support RFC8814
and so continues to advertise the OSPF MSD TLV into the opaque TLV.


>
> s/MAY/may
>
> ** §4.3.2.6 and §4.3.3.6 have similar statements.  The same comment
> applies there.
>

KT> Ack


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

KT> Ack


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

KT> Ack


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

KT> Ack but the text is tweaked a bit.


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

KT> Ack


>
>
> [minor] s/in the "OSPFv3 Extended-LSA Sub-TLVs" registry [RFC8362]
> under the IANA OSPFv3 Parameters registry./in an OSPFv3 Extended LSA
> [RFC8362].
>
>
KT> Ack but the text is tweaked a bit.


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

KT> The section and encoding rule for private use is required - please see
response to next comment. IANA considerations does indicate ranges 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.
>

KT> There is the need to ensure some encoding rules for private so that the
TLVs of private use from one implementation are not misread by another
implementation. Without such rules, there is no way to even figure out
which vendor/implementation's private use is being referred to. E.g.,
https://www.rfc-editor.org/rfc/rfc5613.html#section-2.6


>
> 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).
>

KT> Nothing in the draft is contradicting or against the above. Note that a
"local site" can have multiple vendor implementations.


>
>
> ...
> 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).
>

KT> The reference is clearly made to OSPF virtual links in this section -
clarified in a couple of instances with text updates. In the rest of the
document (RFC7752 base), "virtual links" are used in various contexts -
e.g. physical vs virtual (as in a tunnel), or a virtual representation of a
link (e.g. "aggregate link" in Sec 5). In most places, the reference seems
to be evident to me but I am open to suggestions for changes/qualification.


>
>
> ...
> 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).
>

KT> Would it make sense to insert a new section 4 "Advertising IGP
information into BGP-LS" to hold this text and some other that has been
added in section 4 in this update v11? The introduction is focussed on
other aspects and this might get lost in there.


>
>
> [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.
>

KT> Ack.


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

KT> Ack


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

KT> Ack


>
>
> [minor] Please explain what a0 and a1 mean.
>

KT> Ack


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

KT> Ack


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

KT> Ack to both above.


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

KT> Ack


>
>
> [?] "...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.
>

KT> It should be "could" instead of "would" above - it depends on the BGP
decision process. The Node and Prefix NLRIs belonging to R5 remain the same
regardless of the state of the R5-R6 link. So it is possible that the stale
Router LSA carrying an older value of a stub prefix metric gets preferred
over a newer value for that stub prefix in the "current" Router LSA. Hope
some of the text updates clarify this.


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

KT> ack


>
>
> [nit] s/using RR/using RRs
>

KT> Ack


>
>
> [nit] s/this draft/this document
>

KT> Ack


>
>
> ...
> 1841    4.11.  Router-ID Anchoring Example: OSPF Pseudonode
>
> [] Please use addresses reserved for documentation in the examples
> (RFC6890).
>

KT> Fixed


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

KT> Ack


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

KT> Ack


>
>
> [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...
>

KT> Ack


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

KT> Will ask for IANA to remove that registry.


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

KT> It should have been the IETF Review policy. Given the limited size,
that would seem to be prudent IMHO.


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

KT> Same as above - IETF Review.


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


>

KT> Same as above - IETF Review.


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

KT> That is correct. Fixed.


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

KT> Ack. Updated with some minor changes.


>
>
> [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".
>

KT> Ack


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

KT> Ack


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

KT> Updated text and also reference to 7.1.1.


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

KT> Ack


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

KT> We cannot parse/check the sub-TLVs for unrecognized TLVs. Updated the
text.


>
>
>
> ...
> 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  (?)
>

KT> Ack


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

KT> That would be an implementation or local policy choice. Note that with
AFI/SAFI disable, subsequent updates would just be ignored and sometimes it
may be preferred to perform session reset.


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

KT> Ack


>
>
>
> ...
> 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?
>

KT> Updated


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

KT> This is correct - we cannot do anything better for malformed NLRI.


>
>
> [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."
>

KT> This is true in the case of BGP-LS.


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

KT> This is a debatable point and depends on the consumer application. If a
node or link is malformed, what would be better - to simply remove it from
the topology view for the consumer or keep it with an error flagged on it.
In either case, that node/link would be unusable, but in the latter case,
at least the consumer application can detect that this is not a link/node
down but some BGP-LS error and flag it appropriately. In conclusion, we
have gone with the latter approach.


>
> 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. :-(
>

KT> Sure. But does this not apply to BGP in general?


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

KT> Good point. There is a flip side here. Assume that there were two
BGP-LS Producers for the same link-state object. For some reason, one of
them generated a malformed TLV in the BGP-LS attribute  while the other did
not. If we do "treat as withdraw" then that malformed update is gone and
the other will go through. However, if we were to retain with "attribute
discard" then there is no guarantee which of the two updates would go
through. I was tempted to introduce a tweak/change in the BGP Decision
process for BGP-LS to consider updates w/o the BGP-LS Attribute as less
preferred. However, it would have been a significant change. Perhaps we can
still check/socialize with the WG?


>
>
>
> 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"?
>

KT> Ack


>
>
>
> 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".   ??
>

KT> Yes. Fixed the text.


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

KT> Ack. Semantic validation is not defined. Will change to non-normative.


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

KT> Ack


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

KT> Ack


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

KT> Yes, it is related to the maximum message size that an operator knows
can be used safely across their network.


>
>
>
> ...
> 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".
>

KT> Changing to SHOULD NOT in Sec 9 to be consistent.


>
>
>
> ...
> 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"
>

KT> Text is changed for consistency.


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

KT> The BGP Speakers are not aware and it is for the operator to enable
such policies.


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


KT> Yes


> 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).
>

KT> Updated the text. The feedback loop is when a consumer can originate
information that it receives such that it replaces the valid link-state
objects that are being originated by the legitimate BGP-LS Producers.


>
>
>
> 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).
>

KT> Removed that paragraph and added text instead in Sec 3 about the
one-way channel.


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

KT> Changed "consumers" to BGP Speakers. Added text about the interface to
BGP-LS consumers having similar implications but that that is out of scope
of this document.


>
>
> [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.
>

KT> Ack. Added text to cover this.


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

KT> Ack


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

KT> Ack


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

KT> ack


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

KT> Ack


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


KT> Ack

Thanks,
Ketan



>
> [EoR -10]
>