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

[John Scudder via Datatracker <[e-mail]>]

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

Second, use "or", not "and", or a similar approach that accomplishes the same
end.

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:

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


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

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

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

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

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

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

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

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

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

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

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

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

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