Re: [Idr] John Scudder's Discuss on draft-ietf-idr-bgpls-srv6-ext-12: (with DISCUSS and COMMENT)

[John Scudder <[e-mail]>]

Cleared. Sorry for the egregious delay.

—John

> On Feb 1, 2023, at 5:42 PM, John Scudder <[e-mail]> wrote:
> 
> It’s near the top of my list. Sorry for the delay. 
> 
> —John
> 
>> On Feb 1, 2023, at 5:35 PM, Alvaro Retana <[e-mail]> wrote:
>> 
>> 
>> 
>> John:
>> 
>> Hi!
>> 
>> Can you please take a look at this?
>> 
>> [I think we may have talked about this earlier in the week, but I’m not sure. :-(  Sorry for insisting.]
>> 
>> Thanks!
>> 
>> Alvaro. 
>> 
>> On January 14, 2023 at 8:20:18 AM, Ketan Talaulikar ([e-mail]) wrote:
>> 
>>> Hi John,
>>> 
>>> Thanks for your detailed review and your comments/suggestions. Please check inline below for responses.
>>> 
>>> We've also posted an updated version with these changes included: https://datatracker.ietf.org/doc/html/draft-ietf-idr-bgpls-srv6-ext-13
>>> 
>>> 
>>> On Thu, Dec 15, 2022 at 2:49 AM John Scudder via Datatracker <[e-mail]> wrote:
>>> John Scudder has entered the following ballot position for
>>> draft-ietf-idr-bgpls-srv6-ext-12: 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-bgpls-srv6-ext/
>>> 
>>> 
>>> 
>>> ----------------------------------------------------------------------
>>> DISCUSS:
>>> ----------------------------------------------------------------------
>>> 
>>> # John Scudder, RTG AD, comments for draft-ietf-idr-bgpls-srv6-ext-12
>>> CC @jgscudder
>>> 
>>> ### Global, field definitions are unclear throughout
>>> 
>>> I have a problem with how several different fields are defined. For example, in
>>> Section 3.1, the description of the flags field says,
>>> 
>>> ```
>>>    *  Flags: 2 octet field.  The flags are derived from the SRv6
>>>       Capabilities sub-TLV/TLV of IS-IS (section 2 of
>>>       [I-D.ietf-lsr-isis-srv6-extensions]) and OSPFv3 (section 2 of
>>>       [I-D.ietf-lsr-ospfv3-srv6-extensions]).
>>> ```
>>> There are two problems here: "derived", and "and". On consideration, I think
>>> "and" is the more serious, although it wasn't what initially caught my
>>> attention. 
>>> 
>>> Regarding "derived": Normally when I think of something being "derived from"
>>> something else, I think of some derivation function, e.g. pressure can be
>>> derived from the volume of a confined gas according to Boyle's Law. Of course,
>>> the function can technically be the identity function, which appears to be what
>>> you mean here. There's a suggestion related to that in my second discuss point,
>>> below.
>>> 
>>> Regarding "and": As written (here and elsewhere), you're saying that the field
>>> comes from both places at once. That doesn't make sense on the face of it, and
>>> it led me down a rabbit hole I'll spare you the description of.
>>> 
>>> In the end, I guess how you arrived at your current text is probably by trying
>>> to capture that BGP-LS is a vessel to convey fields that originate in one of
>>> the IGPs, hence the fields aren't defined here and you don't want to write
>>> rules or semantics for them here. I think there are two basic changes you can
>>> make to your pattern that would address my concern.
>>> 
>>> First and less importantly, choose some verb other than "derive". "Copied"
>>> seems like the most precise, but something else could be proposed. (But again,
>>> see my second discuss point.)
>>> 
>>> KT> Ack. How about using "copied" in the context of a specific field and "derived" when we are talking about something like a TLV/sub-TLV?
>>>  
>>> 
>>> Second, use "or", not "and", or a similar approach that accomplishes the same
>>> end.
>>> 
>>> KT> Ack - the way the text is structured, "or" is more appropriate and this has been fixed.
>>>  
>>> 
>>> Rewriting the example text according to this suggestion, we'd have something
>>> like,
>>> 
>>> ```
>>>    *  Flags: 2 octet field.  The flags are copied from the SRv6
>>>       Capabilities sub-TLV/TLV of either IS-IS (section 2 of
>>>       [I-D.ietf-lsr-isis-srv6-extensions]) or OSPFv3 (section 2 of
>>>       [I-D.ietf-lsr-ospfv3-srv6-extensions]), depending on the
>>>       source protocol.
>>> ```
>>> I don't insist that you use this exact pattern, it's just an example of a
>>> minimal patch that addresses the issue, I don't claim it's the best fix. And
>>> TBH, if you made the other changes but left "copied" as "derived" I think it
>>> would have been clear enough in context. Indeed, I think just changing "and" to
>>> "or" could pass the smell test, especially if there's an introductory paragraph
>>> somewhere, setting up the pattern. (Again, see my second point.)
>>> 
>>> For some examples of how we've solved this for past BGP-LS specs, we can
>>> consider RFCs 8814 and 9085. In Section 2 of RFC 8814, it says, "When a BGP-LS
>>> speaker is originating the topology learnt via link-state routing protocols
>>> such as OSPF or IS-IS, the MSD information for the nodes and their links is
>>> sourced from the underlying extensions as defined in [RFC8476] and [RFC8491],
>>> respectively." Or, if we look at RFC 9085, you use "derived" but mostly in ways
>>> I find clear enough, for example, §2.3.5:
>>> 
>>> ```
>>>                                                           The
>>>    information advertised in the Range TLV is derived from the SID/Label
>>>    Binding TLV in the case of IS-IS and the OSPFv2/OSPFv3 Extended
>>>    Prefix Range TLV in the case of OSPFv2/OSPFv3.
>>> ```
>>> The use of "in the case of" makes it clear that there's only a single source
>>> for the field in any given instance. Or §2.1.1:
>>> 
>>> KT> I am leaning towards the use of "... in the case of ..." in addition to the "or" to further disambiguate. Does that work?
>>>  
>>> 
>>> ```
>>>    The SID/Label TLV is used as a sub-TLV by the SR Capabilities
>>>    (Section 2.1.2) and Segment Routing Local Block (SRLB)
>>>    (Section 2.1.4) TLVs.  This information is derived from the protocol-
>>>    specific advertisements.
>>> 
>>>    *  IS-IS, as defined by the SID/Label Sub-TLV in Section 2.3 of
>>>       [RFC8667].
>>> 
>>>    *  OSPFv2/OSPFv3, as defined by the SID/Label Sub-TLV in Section 2.1
>>>       of [RFC8665] and Section 3.1 of [RFC8666].
>>> ```
>>> The use of "from the protocol-specific advertisements" makes it clear in
>>> context.
>>> 
>>> I'm not going to flag each instance of this pattern in my review, but ask that
>>> you do a global search for "derived". I've noted some other issues that won't
>>> be revealed by a search for "derived" in my COMMENT section.
>>> 
>>> (I notice that in Section 7.1 you have "The flags map to the IS-IS or
>>> OSPFv3..." -- which works just fine as far as I'm concerned.)
>>> 
>>> ### Section 2
>>> 
>>> In the same vein, it also seems worth tightening up this paragraph from Section
>>> 2:
>>> 
>>> ```
>>>    When a BGP-LS router advertises topology information that it sources
>>>    from the underlying link-state routing protocol (as described in
>>>    [RFC7752]), then it maps the corresponding SRv6 information from the
>>>    SRv6 extensions for IS-IS [I-D.ietf-lsr-isis-srv6-extensions] and
>>>    OSPFv3 [I-D.ietf-lsr-ospfv3-srv6-extensions] protocols to their BGP-
>>>    LS TLVs/sub-TLVs for all SRv6 capable nodes in that routing protocol
>>>    domain.  When a BGP-LS router advertises topology information from
>>>    the BGP routing protocol (e.g., for EPE), then it advertises the SRv6
>>>    information from the local node alone.
>>> ```
>>> Perhaps this could be made more specific, for example:
>>> 
>>> ```
>>>    In most cases, a BGP-LS router will advertise topology information it has
>>>    sourced from an underlying link-state routing protocol, as described in
>>>    [RFC7752]. In such cases, it will derive the corresponding SRv6 information
>>>    from the SRv6 extensions for IS-IS [I-D.ietf-lsr-isis-srv6-extensions] or
>>>    OSPFv3 [I-D.ietf-lsr-ospfv3-srv6-extensions], as applicable. In practice,
>>>    this derivation comprises a simple copy of the relevant field into a field
>>>    of the corresponding BGP-LS TLV/sub-TLV. This document defines an
>>>    exception case where a BGP-LS router can originate topology information
>>>    itself, for EPE. Such information is advertised only on behalf of the
>>>    local router, in contrast to the usual [RFC7752] behavior of advertising
>>>    information for an entire underlying IGP domain.
>>> ```
>>> This rolls together a few of the windmills I've been tilting at, including
>>> supplying a small definition of "derives" to perhaps minimize rewriting in the
>>> rest of the document. (See also my second COMMENT.)
>>> 
>>> KT> Thanks for that suggestion. I've incorporated a lot from this. One thing to clarify is that RFC7752 covered both sourcing from IGPs and Direct/Static. BGP EPE was in a different RFC added later and there is work to extend it for BGP-only topologies (please see further responses).
>>>  
>>> 
>>> 
>>> ----------------------------------------------------------------------
>>> COMMENT:
>>> ----------------------------------------------------------------------
>>> 
>>> ## COMMENT
>>> 
>>> ### Global, is RFC 7752 the right reference?
>>> 
>>> Since we have rfc7752bis in the final stages of preparation (thanks for your
>>> work on it!), is RFC 7752 the best reference for this document to cite?
>>> 
>>> KT> We didn't want to put a dependency between this and RFC7752bis given that there are already many BGP-LS documents that point to RFC7752. Readers/implementers/operators would need to "move" to RFC7752bis in their minds when they think of BGP-LS. One more in that list should not be much of an issue?
>>>  
>>> 
>>> ### Section 1 isn't EPE the only case?
>>> 
>>> Related to my DISCUSS,
>>> 
>>> ```
>>>    This document describes extensions to BGP-LS to advertise the SRv6
>>>    SIDs and other SRv6 information from all the SRv6 capable nodes in
>>>    the IGP domain when sourced from link-state routing protocols and
>>>    directly from individual SRv6 capable nodes (e.g. when sourced from
>>>    BGP for EPE).
>>> ```
>>> Are there any other concrete cases other than EPE, that the document defines,
>>> where the router sources data directly into BGP-LS? If not, consider rewriting
>>> to make the text exhaustive instead of exemplary, by removing the "e.g." or
>>> turning it into an "i.e.".
>>> 
>>> KT> We have a WG document https://datatracker.ietf.org/doc/draft-ietf-idr-bgp-ls-bgp-only-fabric/ that extends this beyond BGP EPE. We don't want to preclude that.
>>>  
>>> 
>>> ### Section 4.1 IS-IS and OSPFv3
>>> 
>>> Related to my DISCUSS point, here we have
>>> 
>>> ```
>>>    The SRv6 End.X SID TLV is used to advertise the SRv6 SIDs associated
>>>    with an IGP Adjacency SID behavior that correspond to a point-to-
>>>    point or point-to-multipoint link or adjacency of the node running
>>>    the IS-IS and OSPFv3 protocols.
>>> ```
>>> That should be "the IS-IS or OSPFv3 protocol", I think, since generally, a node
>>> won't be running both at the same time. (Noted here since searching for
>>> "derived" won't find it.)
>>> 
>>> KT> Ack
>>>  
>>> 
>>> ### Section 4.2
>>> 
>>> Again related to DISCUSS point,
>>> 
>>> ```
>>>    *  Neighbor ID : 6 octets of Neighbor System-ID in IS-IS SRv6 LAN
>>>       End.X SID TLV and 4 octets of Neighbor Router-id in the OSPFv3
>>>       SRv6 LAN End.X SID TLV.
>>> ```
>>> Should be "... or 4 octets," I think. Figure 3 gets this right, so regardless
>>> of the bigger picture it's worth changing just for consistency with the figure.
>>> 
>>> KT> Fixed.
>>>  
>>> 
>>> ### Section 4.3
>>> 
>>> Again related.
>>> 
>>> ```
>>>    Each MSD-type is encoded in the BGP-LS Link MSD TLV as a one-octet
>>>    type followed by a one-octet value as derived from the IS-IS and
>>>    OSPFv3 Link MSD advertisements as specified in [RFC8814].
>>> ```
>>> Debatable since you refer back to a single underlying spec, but I think "IS-IS
>>> or OSPFv3".
>>> 
>>> KT> I know what you mean, and we have aligned for consistency.
>>>  
>>> 
>>> ### Section 6.1
>>> 
>>> This looks like "the exception that proves the rule":
>>> 
>>> ```
>>>    When advertising the SRv6 SIDs from the IGPs, the SID information is
>>>    derived from the SRv6 End SID sub-TLV of IS-IS (section 7.2 of
>>>    [I-D.ietf-lsr-isis-srv6-extensions]) and OSPFv3 (section 8 of
>>>    [I-D.ietf-lsr-ospfv3-srv6-extensions]).
>>> ```
>>> That is to say, the "when advertising" part implies you might advertise
>>> information from other sources too, I guess the Protocol-ID field has the same
>>> implication since that also can take values OSPFv2, Direct, and Static. So
>>> then, if the source is NOT IS-IS or OSPFv3, where is the SID information
>>> derived from?
>>> 
>>> KT> It is sourced by BGP-LS "direct"ly from the local node as covered by RFC7752(bis). Any locally instantiated SRv6 SID can be advertised by that node via BGP-LS - refer https://datatracker.ietf.org/doc/html/rfc8986#section-8.4. I realize that we are missing some details here and I've clarified as part of the cleanup for the "derived" usage.
>>>  
>>> 
>>> ## NITS
>>> 
>>> ### Section 4.2
>>> 
>>> ```
>>>    For a LAN interface, an IGP node announces normally only its
>>>    adjacency to the IS-IS pseudo-node (or the equivalent OSPF DR).
>>> ```
>>> I think that should be "normally announces". As written it expresses the
>>> implication that it announces other adjacencies abnormally. Maybe "ordinarily"
>>> would be better than "normally" since it doesn't carry the same risk of
>>> ambiguity.
>>> 
>>> KT> Ack.
>>>  
>>> 
>>> ```
>>>                                                           Without this
>>>    TLV, multiple BGP-LS Link NLRIs would need to be originated for each
>>>    additional adjacency to advertise the SRv6 End.X SID TLVs for these
>>>    neighbor adjacencies.
>>> ```
>>> Do you mean "... would need to be originated, one for each additional
>>> adjacency, to advertise..."?
>>> 
>>> KT> Ack. Rephrased for clarity.
>>>  
>>> 
>>> ### Section 5.1
>>> 
>>> ```
>>>    on that node.  These Locators are advertised as BGP-LS Prefix NLRI
>>>    objects along with the SRv6 Locator TLV in its BGP-LS Attribute.
>>> ```
>>> "Each Locator is advertised as a BGP-LS Prefix NLRI object along with the SRv6
>>> Locator TLV in its BGP-LS Attribute." (Agreement in number.)
>>> 
>>> KT> Ack
>>>  
>>> 
>>> ### Section 7.2
>>> 
>>> ```
>>>       -  B-Flag: Backup Flag.  If set, the SID is eligible for
>>>          protection using fast reroute (FRR).
>>> ```
>>> I suggest "eligible to be protected" instead.
>>> 
>>> KT> Ack
>>>  
>>> 
>>> ```
>>>       -  Other bits are reserved for future use and MUST be set to 0 and
>>>          ignored on receipt.
>>> ```
>>> ```
>>>    *  Reserved: 2 octet field.  The value MUST be set to 0 and ignored
>>>       on receipt.
>>> ```
>>> For both these cases, I suggest "MUST be set to 0 when originated, and..."
>>> 
>>> KT> Ack
>>>  
>>> 
>>> ### Section 8
>>> 
>>> ```
>>>    The sum of the LB Length, LN Length, Func.  Length, and Arg. Length
>>>    MUST be less than or equal to 128.
>>> ```
>>> I was a little surprised these don't have to total exactly 128 but I guess this
>>> is a deliberate choice and consistent with other specs? (Presumably, any
>>> leftover bits are don't-care.)
>>> 
>>> KT> Yes. Please refer to https://datatracker.ietf.org/doc/html/rfc8986#section-3.1
>>> 
>>> Thanks,
>>> Ketan
>>>  
>>> 
>>> ## 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
>>> 
>>> 
>>>