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

[Ketan Talaulikar <[e-mail]>]

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