Border Gateway Protocol - Link State (BGP-LS) Extensions for Segment Routing
draft-ietf-idr-bgp-ls-segment-routing-ext-18

(1) Section 2.0.  Does the sentence, “This document adds additional BGP-LS Attribute TLVs in order to encode SR information” imply that this document should explicitly update RFC7752?

(2) Add more detailed citations
-- Section 2.x.  Section 2.1.x.  When citing a reference to explain a given TLV/sub-TLV, consider adding the relevant section number to improve readability.  For example:

OLD: Section 2.1.1, IS-IS, as defined by the SID/Label sub-TLV in [I-D.ietf-isis-segment-routing-extensions].

NEW: Section 2.1.1, IS-IS, as defined by the SID/Label sub-TLV in Section 3.2 of [I-D.ietf-isis-segment-routing-extensions].

-- Section 2.1.2  Per the octet of flags, please specify the specific section number of [I-D.ietf-isis-segment-routing-extensions].

-- Section 2.2.*.  Consider adding the appropriate section number in the references when described the flags field.

(3) Section 2.1.4.  Per “SID/Label sub-TLV (as defined in Section 2.1.1) which encodes the first label in the range”, why is this just the first label?  Isn’t there the potential of a series of labels each in a distinct SID/Label sub-TLV?

(4) Section 2.2.2.  Is there a reference to explain the semantics of the weight field?

(5) Section 2.3.1.  Is there a reference to explain the semantics of the algorithm field?

(6) Editorial Nits:
-- Section 2.1.1.  Missing word:
s/The TLV and has the following format:/
/The TLV and sub-TLV has the following format:/

-- Section 2.1.5.  Typo.  s/a unsigned/an unsigned/

I'd like to strongly support Mirja's comment -- having this buried in the Security Considerations section makes it easy to miss, and that does set the tone for the rest of the document..

Also, thank you for this document -- it is nice to see this finally getting out the door. Special thanks for the Manageability Considerations section

Thanks for the work that everyone did on this document. I have only one
very minor comment.

§2.1.2:

>     Reserved: 1 octet that SHOULD be set to 0 and MUST be ignored on
>     receipt.

Making this a "SHOULD" rather than a "MUST" seems like it might interfere with
any future attempts to use this field, since compliant implementations might
have set the byte to arbitrary values.

This comment applies to other "reserved" fields in this document.

Just an absolute nit in Section 1:

   A segment can
   represent any instruction; topological or service-based.

The semicolon is the wrong punctuation here.  A colon would work, as would a comma or a dash.

-15

I agree with Mirja that mentioning the scope earlier in the document
would be helpful (but will follow the discussion in her ballot thread).
Basically, even though the SR Architecture is clear about the scope
being limited to a trusted administrative domain, it's still helpful to
remind the reader briefly at the start of the document that there is
some expectation of trust to all participants receiving this
information, and that it is not expected to leave into the broader
Internet or generic BGP peers.

Section 1

   When Segment Routing is enabled in an IGP domain, segments are
   advertised in the form of Segment Identifiers (SIDs).  The IGP link-
   state routing protocols have been extended to advertise SIDs and
   other SR-related information.  IGP extensions are described in: IS-IS
   [I-D.ietf-isis-segment-routing-extensions], OSPFv2
   [I-D.ietf-ospf-segment-routing-extensions] and OSPFv3
   [I-D.ietf-ospf-ospfv3-segment-routing-extensions].  [...]

nit: I think "described for" is more appropriate than "described in",
which might suggest to the reader that these are part of the core
protocols.

Section 2.1.1

      Length: Variable.  Either 3 or 4 depending whether the value is
      encoded as a label or as an index/SID.

nit: I think the secdir reviewer's concern might be alleviated by
spelling these constants as 0x0003 and 0x0004.

Section 2.1.2

Are the "SID/Label sub-TLV N" fields actually variable length?  The
description seems to be saying that we can only use the "label" variant
encoding, which makes the overall length of the sub-TLV 7 bytes, always.

      Flags: 1 octet of flags as defined in
      [I-D.ietf-isis-segment-routing-extensions] for IS-IS.  The flags
      are not currently defined for OSPFv2 and OSPFv3 and SHOULD be set
      to 0 and MUST be ignored on receipt.

I agree with the other reviewers that the SHOULD here is surprising.
Perhaps "Until the specification of flag value usage for OSPFv2 and/or
OSPFv3 is defined, the flags MUST be set to zero and ignored on
receipt"?

The case for Reserved seems even more clear to mandate setting to zero.

Section 2.1.3

Do the algorithm values need to appear in any specific order?
The note about the maximum length being 256 seems to imply that
duplicates are not expected, but I do not see any specific text
forbidding them.

Section 2.1.4

[Same comments as Section 2.1.2 about variable length sub-TLVs, and
SHOULD/MUST]

Section 2.2.1

   The Adjacency SID TLV is used in order to advertise information
   related to an Adjacency SID.  This information is derived from Adj-
   SID sub-TLV of IS-IS [I-D.ietf-isis-segment-routing-extensions],
   OSPFv2 [I-D.ietf-ospf-segment-routing-extensions] and OSPFv3
   [I-D.ietf-ospf-ospfv3-segment-routing-extensions].

nit: I think this should be "Adj-SID sub-TLVs" plural.

      Weight: 1 octet carrying the weight used for load-balancing
      purposes.

Maybe reference RFC 8402 for the weight semantics?  (Otherwise we have
to guess whether a larger value means to send more traffic vs. less
traffic.)

[Same comment about SHOULD/MUST for Reserved]

Section 2.2.2

[same comment about Weight and SHOULD/MUST for Reserved]

Do we want some IANA considerations and/or regisitry modifications to
provide for any future BGP-LS Attribute TLVs that might be defined and
be appropriate to include as sub-TLVs of the L2 Bundle Member Attribute
TLV?

Section 2.3.1

[same comment about SHOULD/MUST for Reserved]

Section 2.3.3

The figure says "4 or 6 octet Router-ID"; should that bt "4 or 16" to
match the body text?

Section 2.3.4

[same comment about SHOULD/MUST for Reserved]

It's a little weird to blandly cite the OSPF document for the Range Size
definition and expect that to plainly transfer over to the IS-IS case as
well.  (Also, the linked document has three different usages for "Range
Size"; I assume we mean the one in Section 4, "OSPF Extended  Prefix
Range TLV", since it's the right width, but it might be worth stating
explicitly.)

I didn't take the time to try to convince myself that the sub-TLV usage
for prefix-to-SID mappings matches up with the statement that the length
is "11 or 12 depending on Label or Index encoding of the SID", but I
think the TLV overhead would push the length larger than 12.

Section 5

The links for "required security and authentication mechanisms" are a
little surprising, as those documents are the segment-routing-extensions
documents for IS-IS and OSPF, respectively, and do not define the
mechamisms themselves, rather referring to other document for  the
details of the security mechanisms.  Furthermore, there seem to only be
SHOULD-level requirements for authentication mechanisms, and only in
certain cases (and only in the OSPFv2 document).  So to say "the required
mechanisms" here seems misleading, in that the set of *required*
mechanisms seems to be the empty set.

I always have a hard time accepting statements about "present no
additional risk"; we are going from presenting "generic"
link-state/prefix/node information across the BGP-LS domain to
additionally presenting information about the SR topology and segment
location/identifiers.  So, one might concoct a scenario in which an
attacker can learn about the internal SR configuration/topology based on
the information conveyed by the mechanisms in  this document, which is
in some sense an "additional risk".  That said, it is probably a minor
one, given the expected scope of distribution.

There is the following statement on the applicability of this approach in the security consideration section:

“The SR traffic engineering
   policies using the SIDs advertised via BGP-LS are expected to be used
   entirely within this trusted SR domain (e.g. between multiple AS/
   domains within a single provider network).  Therefore, precaution is
   necessary to ensure that the SR information advertised via BGP-LS
   sessions is limited to consumers in a secure manner within this
   trusted SR domain.”

As this is every essential to the scope of the document I would like to see this earlier in the document, e.g. in the intro, and own applicability section, or even in the abstract.


One additional comment on the shepherd write-up: 
I find the write-up a bit confusing but I assume that this document has wg consensus, even though it might be rough. There is a request to the IESG to make a judgment if this approach should be taken forward in general. However, if there are no technical or security concerns here and there is wg consensus, I don’t think I understand this request; expect this is not seen as covered by the charter, however, I don’t think this is indicated in the shepherd write-up.

* Section 2.3.3.

I think the figure should say "4 or 16 octet Router-ID" instead of "4 or 6 octet Router-ID" as the IPv6 router ID will be 16 octets long