Re: [Idr] John Scudder's Discuss on draft-ietf-idr-rfc7752bis-14: (with DISCUSS and COMMENT)

[Ketan Talaulikar <[e-mail]>]

Hi John,

My apologies for the very long delay in getting back on this one. Thanks
for your very detailed review and please check inline below for responses.

I've posted an update that includes the changes discussed below:

https://datatracker.ietf.org/doc/html/draft-ietf-idr-rfc7752bis-15


On Tue, Dec 13, 2022 at 6:59 AM John Scudder via Datatracker <
[e-mail]> wrote:

> John Scudder has entered the following ballot position for
> draft-ietf-idr-rfc7752bis-14: Discuss
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to
> https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/
> for more information about how to handle DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-idr-rfc7752bis/
>
>
>
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
>
> # John Scudder, RTG AD, comments for draft-ietf-idr-rfc7752bis-14
> CC @jgscudder
>
> Thanks for the work you've put into this needed document. I reviewed the
> diff
> vs. RFC 7752, but didn't focus on any unchanged sections.
>
> Thanks also to the shepherd, Jeff Haas, for the meticulous and helpful
> shepherd
> write-up.
>
> As an aside to the author, my review would have been easier if a native
> HTML
> rendering had been available. I assume you must have uploaded txt instead
> of
> XML -- please consider uploading XML in the future? Thanks.
>
> ## DISCUSS
>
> ### Section 5.2.1, using ASN to disambiguate Router ID
>
> ```
>    BGP-LS uses the Autonomous System (AS) Number to
>    disambiguate the Router-IDs, as described in Section 5.2.1.1.
> ```
> My concern with this is straightforward: Section 5.2.1.1 doesn't describe
> the
> use of ASN to disambiguate Router ID. In fact, it's a little hard to
> understand
> WHAT Section 5.2.1.1 describes :-( but it's not that.
>
> I see that this claim is a carry forward from RFC 7752, but since you're
> touching it, I think you need to make it right, sorry. If I were confident
> I
> knew what the intent was, I'd propose a rewrite, but since I'm not, I'm
> just
> flagging the problem.
>

KT> I understand your concern. Thanks Jeff for your clarification and
providing the broader context. We do have the AS Number in there for this
purpose and this is explained in section 5.2.1.4. What is missing is that
AS Number is not mentioned in Section 5.2.1.1. My experience with
deployments is that operators can simplify things by using the BGP-LS
Instance-ID (that identifies an IGP domain) as unique across their entire
network without having to worry about their existing AS, IP or IGP
numbering.

We've added the following new text in the update posted to clarify:

< NEW >
Furthermore, in deployments where BGP-LS is used to advertise topology from
multiple-ASes, the AS Number is used to distinguish topology information
reported from different ASes.

Since the BGP-LS Instance-ID is operator assigned, its allocation scheme
can ensure that each IGP domain is uniquely identified even across a
multi-AS network.
< /NEW>


>
> ### Section 8.2.2, treat-as-withdraw
>
> I have a concern about this:
>
> ```
>    When the error that is determined allows for the router to skip the
>    malformed NLRI(s) and continue the processing of the rest of the BGP
>    UPDATE message (e.g. when the TLV ordering rule is violated), then it
>    MUST handle such malformed NLRIs as 'Treat-as-withdraw'.
> ```
> It's novel, in the context of RFC 7606, to allow malformed NLRI to result
> in
> something other than a session reset (or equivalent). I see why you are OK
> with
> it in the context of TLV misordering, however I think it's better to limit
> the
> exception to the exact case, instead of making it a general guideline open
> to
> implementor creativity.
>

KT> TLV misordering is one case. But there is also the case where we have a
container TLV (Local/Remote Node Descriptor TLVs) and then TLVs within it.
These containers allow us to skip over possible errors/issues within the
TLVs inside. I am not entirely sure how these things will evolve - though
we've tried not to introduced nested TLVs into the NLRIs. Then there is
also the case where the TLV lengths are fixed/known but we get something
different and it is still possible reject it and to parse over to the next.
In the end, we are trying to optimize/limit the impact of the fault and
hence the guideline instead of specific rules. This has been in the spirit
of RFC7606 - to try not to reset the session as far as possible.


> A second comment (not DISCUSS level but I'll include it here) is that you
> don't
> have to characterize this as treat-as-withdraw. Since an implementation
> that
> follows this rule will never accept such an NLRI, there will never be
> anything
> to withdraw. So, following the example of RFC 7606 Section 5.4, we'd have
> something like this as a replacement for the quoted sentence: "An NLRI that
> violates the TLV ordering rule MUST be discarded." There's probably a
> little
> knock-on rewriting that would suggest, too, as in:
>
> ```
>    An NLRI that violates the TLV ordering rule MUST be discarded. Other
> NLRI
>    error cases SHOULD be handled using 'AFI/SAFI disable', when...
> ```
> If you think there are other NLRI error cases that should get the more
> lenient
> treatment, my preference would be to enumerate them specifically, but if
> I'm
> missing some reason this should be left as written, let's discuss.
>
>
KT> I think I understand the issue with "treat-as-withdraw". If an NLRI was
problematic, it could not be handled as "treat-as-withdraw" since for that
action a proper parsing is a prerequisite. I see that error. "NLRI discard"
seems more appropriate in this case. I've updated the text accordingly.


>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> ## COMMENT
>
> ### Throughout, use of "new"
>
> The first four occurrences of "new" (up to but not including Section 5.2)
> might
> have been appropriate in RFC 7752 but these aren't new any more, you could
> remove them without harm.
>

KT> Ack


>
> ### Section 3, bullet 1
>
> ```
>            If R1 and
>       R2 are in the same IGP flooding domain, then they should originate
>       the same link-state information into BGP-LS.
> ```
> I assume the "should" above indicates expectation. It's a perfectly good
> English sentence, but I think there would be less scope for ambiguity if it
> were re-worded like "... then they would ordinarily originate the
> same..."  The
> "ordinarily" is suggested because as the document mentions elsewhere,
> BGP-LS is
> subject to policy so it would be possible, though not wise, to configure
> R1 and
> R2 with different policies leading to origination of different information
> into
> LS.
>

KT> Ack


>
> It's true that formally speaking, the RFC 2119 boilerplate moots this
> point,
> but the difference between theory and practice is greater in practice than
> it
> is in theory. I haven't picked on every lower-case "may" and "should" in
> your
> changeset, only the one that struck me as having genuine potential to lead
> to
> confusion (and a few more noted later in their own points).
>
> And since I'm commenting on this paragraph anyway, "source... sources" is a
> little funny: ```
>               R1 may also source
>               information from sources other than IGP
> ```
> especially since one is a verb and one is a noun. Consider "R1 can also
> originate information into BGP-LS from sources other than IGP"?
>

KT> Ack


>
> ### Section 3, bullet 3
>
> ```
>       The BGP implementation on RRm is doing the propagation of BGP-LS
>       UPDATE messages and performing BGP Decision Process.
> ```
> It's not, strictly speaking, propagating the messages, it's propagating the
> information from the messages, right? So perhaps, "The BGP implementation
> on
> RRm is propagating BGP-LS information. It performs the BGP Decision
> Process as
> part of deciding what information is to be propagated."
>
> I realize this is a fairly picky point but I've spent enough time, over the
> years, explaining to people that no we do not blindly forward UPDATE
> messages
> around, that I'd like to avoid propagating [*] the meme further.


> [*] Oops, sorry.
>

KT> Ack. I didn't realize someone could read it that way :-) ... but you
are right!


>
> ### Section 4
>
> You set up, but do not resolve, a conflict, in your first two paragraphs:
>
> ```
>    The origination and propagation of IGP link-state information via BGP
>    needs to provide a consistent and true view of the topology of the
>    IGP domain.
> ```
> and,
>
> ```
>    The link-state information advertised in BGP-LS from the IGPs is
>    derived from the IGP LSDB built using the OSPF Link State
>    Advertisements (LSAs) or the IS-IS Link State Packets (LSPs).
>    However, it does not serve as a true reflection of the originating
>    router's LSDB.
> ```
> What's a reader to think when they encounter this contradiction, between
> "needs
> to provide... a true view" and "does not serve as a true reflection"? I
> guess
> the former talks about the *topology* and the latter talks about the
> *LSDB*,
> but if you intend to make something of this subtle difference, it's
> escaped me.
>

KT> Yes, that is indeed the difference and it was not meant to be subtle.
Because this is important for the reader to grasp. Any suggestions to
improve would be most helpful.


> I presume that having set up this dichotomy, you have some resolution in
> mind
> for it -- please don't leave it as a conundrum to be solved by the reader.
>

KT> There is no dichotomy here to be resolved. It is just a factual
statement.


>
> ### Section 5.1, malformed
>
> ```
>           NLRIs having TLVs
>    which do not follow the above ordering rules MUST be considered as
>    malformed by a BGP-LS Propagator.  This ensures that multiple copies
>    of the same NLRI from multiple BGP-LS Producers and the ambiguity
>    arising therefrom is prevented.
> ```
> This is OK as written but the last sentence might be even clearer with a
> bit of
> additional explanatory text, as in "This insistence on canonical ordering
> ensures that multiple variant copes of..."?
>

KT> Ack


>
> ### Section 5.2, alleged deviation from RFC 7606
>
> ```
>    does not support a particular Link-State NLRI type.  To allow the
>    introduction of new Link-State NLRI types seamlessly in the future,
>    without the need for upgrading all BGP speakers in the propagation
>    path (e.g., a route reflector), this document deviates from the
>    default handling behavior specified by [RFC7606] for Link-State
>    address-family.  An implementation MUST handle unknown Link-State
>    NLRI types as opaque objects and MUST preserve and propagate them.
> ```
> I think you must be referring to the generic advice from Section 5.4 of RFC
> 7606:
>
>    A BGP speaker advertising support for such a typed address family
>    MUST handle routes with unrecognized NLRI types within that address
>    family by discarding them, unless the relevant specification for that
>    address family specifies otherwise.
>
> Right? If so I guess it would be helpful to cite that specifically, "...
> specified by Section 5.4, paragraph 2, of [RFC7606]" (paragraph-level
> citation
> optional but why not).
>

KT> Indeed. Updated.


>
> (And thanks for taking care with this case!)
>
> ### Section 5.2.1.4, deprecate harder!
>

> You note in Table 3 that the BGP-LS Identifier is deprecated, but in the
> description that follows you just call it optional. Regrettably, experience
> shows that although everyone thinks they know what "deprecated" means,
> different people have different assumptions. The paragraph at the end of
> the
> section helps, somewhat, but I think more could be done.
>
> ```
>    BGP-LS Identifier:  Opaque value (32-bit ID).  This is an optional
>       TLV.  Its original purpose was that, in conjunction with
>       Autonomous System Number (ASN), it would uniquely identify the
>       BGP-LS domain and that the combination of ASN and BGP-LS ID would
>       be globally unique.  However, the BGP-LS Instance-ID carried in
>       the Identifier field in the fixed part of the NLRI also provides a
>       similar functionality.  Hence the inclusion of the BGP-LS
>       Identifier TLV is not necessary.  If advertised, all BGP-LS
>       speakers within an IGP flooding-set (set of IGP nodes within which
>       an LSP/LSA is flooded) MUST use the same (ASN, BGP-LS ID) tuple
>       and if an IGP domain consists of multiple flooding-sets, then all
>       BGP-LS speakers within the IGP domain SHOULD use the same (ASN,
>       BGP-LS ID) tuple.
> ```
> It seems to me this could be cut down considerably, since a bunch of it is
> historical trivia and justification, and especially since the final
> paragraph
> repeats some of the needed information, and more clearly too.
>
> ```
>    BGP-LS Identifier:  Opaque value (32-bit ID).  This is an optional
>       TLV.  By default, it SHOULD NOT be advertised, but MAY be advertised
>       when enabled by configuration, for compatibility with [RFC7752]
>       implementations. See the final paragraph of this section for further
>       considerations and recommended default value.
> ```
> The curious reader can find the former definition and rules (that you're
> deprecating) by referring back to RFC 7752, I don't think it's needed to
> inline
> them, and indeed I think it's unhelpful for document usability. If you
> think
> it's desirable to retain the justification for deprecating it, that could
> go in
> Appendix A.
>

KT> I've trimmed the text here and moved these details in the appendix with
a pointer provided. I've taken most of your recommended text except for the
SHOULD NOT part. This has been hard to deprecate as it is part of the NLRI
and we can get into operational issues in deployments on software upgrades
if things are not done with care - I am just wary!


>
> ### Section 5.2.2, more lower-case "may"
>
> In this paragraph, I have a similar concern to the one I raised about
> Section 3
> --
>
> ```
>    An implementation may end up suppressing the advertisement of a Link
>    NLRI, corresponding to a half-link, from a link-state IGP unless the
>    IGP has verified that the link is being reported in the IS-IS LSP or
>    OSPF Router LSA by both the nodes connected by that link.  This 'two-
>    way connectivity check' is performed by link-state IGPs during their
>    computation and may be leveraged before passing information for any
>    half-link that is reported from these IGPs into BGP-LS.  This ensures
>    that only those Link State IGP adjacencies which are established get
>    reported via Link NLRIs.  Such a 'two-way connectivity check' may be
>    also required in certain cases (e.g., with OSPF) to obtain the proper
>    link identifiers of the remote node.
> ```
> There are three "may" in this paragraph. I think the first ("may end up")
> expresses expectation, but is that your desired meaning? If so, perhaps
> reword
> as "could end up"? If you're actually expressing permission, that's
> something
> more like an RFC 2119 MAY, perhaps either use the MAY, or at least get rid
> of
> "end up". The second doesn't concern me, though it could be "can". The
> third
> also is not very concerning but could be "could".
>

KT> Ack.


>
> ### Section 5.3, limiting the size of the BGP-LS Attribute
>
> Thanks for this discussion. One of the points you raise is:
>
> ```
>          BGP-LS Producers MUST
>    ensure that they limit the TLVs included in the BGP-LS Attribute to
>    ensure that a BGP UPDATE message for a single Link-State NLRI does
>    not cross the maximum limit for a BGP message.  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.
> ```
> In other words, it's a policy decision. But that means that in this
> scenario
> such policy filtering of TLVs would be expected on a per-producer basis --
> does
> this motivate any concern that inconsistent policy being applied at
> different
> BGP-LS Producers could lead to redundant but non-comparable information
> being
> propagated into BGP-LS? If so, is this worth noting?
>

KT> Ack. Added text to that effect.


>
> ### Section 5.3.1.1, reserved bits
>
> I notice the reserved bits are no longer flagged as such, which is OK, I
> see
> they're still called out as unallocated in the IANA registry. Is there a
> reason
> you haven't provided the customary SHOULD be zero on Tx, MUST be
> disregarded on
> Rx guidance, though? I guess you might have refrained simply because the
> advice
> isn't in RFC 7752, but it seems like good advice nonetheless and wouldn't
> introduce any backward compatibility issues.
>

KT> Ack


>
> ### Section 5.3.1.5, here's "may" again
>
> ```
>    In the case of OSPF, this TLV may be used only to advertise the TLVs
>    in the OSPF Router Information (RI) LSA [RFC7770].
> ```
> It sounds like you're trying to forbid use of the TLV for anything else, in
> which case this really does seem like a candidate for RFC 2119 treatment,
> as in
> something like,
>
> ```
>    In the case of OSPF, this TLV MUST NOT be used to advertise TLVs
>    other than those in the OSPF Router Information (RI) LSA [RFC7770].
> ```
> If you're not trying to do that, then this is yet again a case of the
> confusing
> "may".
>

KT> Ack


>
> ### Section 5.3.2.4, small metrics wording
>
> This wording is a little funny, as written it's contradictory:
>
> ```
>         IS-IS small metrics have a length of 1 octet.  Since the
>    ISIS small metrics are of 6-bit size, the two most significant bits
>    MUST be set to 0 and MUST be ignored by the receiver.
> ```
> I guess you mean something like "IS-IS small metrics are of 6-bit size,
> but are
> encoded in a 1 octet field; therefore the two most significant bits of the
> field MUST be set to 0 by the originator and MUST be ignored by the
> receiver."
> (You could also use a formulation around the legal range of small metrics
> being
> 0-63, but that makes it a little harder to talk coherently about the top
> two
> bits of the field.)
>

KT> Ack - incorporated your suggestion (not the range part though).


>
> ### Section 5.3.2.6, "may" again
>
> Same comment as for 5.3.1.5, mutatis mutandis.
>

KT> Ack


>
> ### Section 5.3.3.1, reserved bits
>
> same comment as for 5.3.1.1.
>

KT> Ack


>
> ### Section 5.3.3.6, "may" again
>
> Same comment as for 5.3.1.5, mutatis mutandis.
>

KT> Ack


>
> ### Section 5.4, needs a reference
>
> I'm glad you've started to introduce this practice into BGP! But I think
> this
> needs to be a reference, not a naked URL embedded in the body:
>
> ```
>    The Enterprise Codes are listed at <http://www.iana.org/assignments/
>    enterprise-numbers>
> ```
> Apart from other reasons to put this in the (Normative) references, the way
> it's embedded in the body right now ends up making it wrap as shown,
> leading
> the unwary reader to the wrong landing page.
>

KT> Ack


>
> ### Section 5.7, expand acronyms
>
> VRF should be expanded on first use. (Not starred on
> https://www.rfc-editor.org/materials/abbrev.expansion.txt)
>

KT> That's rather odd! I had to cross check what VRF actually stood for ;-)

Thanks,
Ketan


>
> ## NITS
>
> ## Notes
>
> This review is in the ["IETF Comments" Markdown format][ICMF], You can use
> the
> [`ietf-comments` tool][ICT] to automatically convert this review into
> individual GitHub issues.
>
> [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
> [ICT]: https://github.com/mnot/ietf-comments
>
>
>
>