Re: [Idr] AD Review of draft-ietf-idr-bgp-ls-segment-routing-ext-11

[Alvaro Retana <[e-mail]>]

On April 18, 2019 at 7:22:50 AM, Ketan Talaulikar (ketant) ([e-mail])
wrote:

Ketan:

Hi!

...

Until we agree on the formatting I’m not considering the related comments
(yours or mine) below, with a couple of exceptions.

Now that we seem to have agreed to disagree (which is ok - I understand
your reasons), I looked again at the specifics of the TLVs…. I put some
comments at the bottom of this message.  My focus was just on the TLVs and
the intent of being consistent.


...

1009 6.  Security Considerations

...

[major] "SR operates within a trusted domain...[RFC8402]...and its security
considerations also apply to BGP-LS sessions when carrying SR information."
 The Security Considerations in rfc8404 really only talk about the data
plane -- I don't see how they apply to the BGP-LS sessions.

*[KT] The concept of trusted domain in SR is being extended to BGP-LS
sessions as well.*

Sorry, but I don’t know what this means.


[major] "...precaution is necessary to ensure that the SR information
collected via BGP-LS is limited to specific controllers or applications..."
 This sounds as if you're referring to information that (once collected)
can be shared between controllers -- I think that case is out of scope.  If
you are trying to talk about the BGP sessions, then I think the language
needs a little more work.  BTW, the paragraph below also talks about BGP
sessions; suggestion: keep the common topics together.

*[KT] This is about not propagating BGP-LS SR information to controllers
and applications that are not part of this SR trusted domain. It is not
applicable to general BGP sessions since they might be setup to other
networks – e.g. eBGP for Internet. The point was not to setup BGP-LS
sessions to other peers which are outside the SR trusted domain.*

[major] The end of the last sentence says "...in a secure manner within
this SR domain".  Assuming that we're talking about BGP sessions, what does
that mean?  Does it mean anything beyond what BGP is normally specified to
do?  If not, then I would recommend taking that piece of text out to not
invite more questions than needed.

*[KT] As mentioned above, the BGP sessions may be setup outside the SR
domain as well. So we are saying that the BGP-LS sessions which exchange
the SR information do not go outside the SR domain.*

Thanks for the explanation.  Please reflect some of this on the document
itself — so that it is clear there.


BTW, it looks like idnits doesn’t like the formatting on Tables 5-7…take a
look at the errors at the top of
https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-idr-bgp-ls-segment-routing-ext-13.txt
I’m not sure what to do to fix that...


Thanks!

Alvaro.


[The line numbers come from idnits for -13.]



...

221 2.1.1.  SID/Label Sub-TLV


223   The SID/Label TLV is used as a sub-TLV by the SR Capabilities

224   (Section 2.1.2) and Segment Routing Local Block (SRLB)

225   (Section 2.1.4) TLVs and has the following format:


[minor] If we want to be consistent, this description is missing the "This
information is derived from..." text; there are sub-TLVs defined in both
IS-IS and OSPF.



...

249 2.1.2.  SR Capabilities TLV

...

257   o  IS-IS, as defined by the SR Capabilities sub-TLV in

258      [I-D.ietf-isis-segment-routing-extensions].


260   o  OSPFv2/OSPFv3, as defined by the SID/Label Range TLV in

261      [I-D.ietf-ospf-segment-routing-extensions] and

262      [I-D.ietf-ospf-ospfv3-segment-routing-extensions].


[mayor] The SID/Label Range TLV is not defined in the OSPFv3 draft.



...

305 2.1.3.  SR Algorithm TLV


307   The SR Algorithm TLV is used in order to advertise the SR Algorithms

308   supported by the node.  This information is derived from the protocol

309   specific advertisements.


311   o  IS-IS, as defined by the SR Algorithm sub-TLV in

312      [I-D.ietf-isis-segment-routing-extensions].


[nit] The name is "SR-Algorithm Sub-TLV".


314   o  OSPFv2/OSPFv3, as defined by the SR Algorithm TLV in

315      [I-D.ietf-ospf-segment-routing-extensions] and

316      [I-D.ietf-ospf-ospfv3-segment-routing-extensions].


[nit] The name is "SR-Algorithm TLV".


[major] It is only defined in the OSPFv2 draft.



...

340 2.1.4.  SR Local Block TLV

...

357   o  OSPFv2/OSPFv3, as defined by the SR Local Block TLV in

358      [I-D.ietf-ospf-segment-routing-extensions] and

359      [I-D.ietf-ospf-ospfv3-segment-routing-extensions].


[major] This one is also not defined in the OSPFv3 document.


...

385      Flags: 1 octet of flags.  None are defined at this stage.


[major] I need to point out again that I really don't like the way this
field is treated.  Note that by decoupling the Flags from whatever is
defined on the IGP drafts introduces complexity and may result in
divergence.  Also, it is not consistent with other fields defined in this
document (e.g. Flags in §2.1.2, 2.2.1, 2.2.2, 2.3.1, 2.3.2, 2.3.4..), and
it doesn't comply with the description above ("This information is derived
from...").



...

400 2.1.5.  SRMS Preference TLV

...

414   o  OSPFv2/OSPFv3, as defined by the SRMS Preference TLV in

415      [I-D.ietf-ospf-segment-routing-extensions] and

416      [I-D.ietf-ospf-ospfv3-segment-routing-extensions].


[major] Again, only defined in the OSPFv2 draft...



...

438   The use of the SRMS Preference TLV is defined in

439   [I-D.ietf-isis-segment-routing-extensions],

440   [I-D.ietf-ospf-segment-routing-extensions] and

441   [I-D.ietf-ospf-ospfv3-segment-routing-extensions].


[minor] This last paragraph is redundant: the text above already says where
the information comes from.  It is obvious that the use of the TLV is
defined there too.



...

461 2.2.1.  Adjacency SID TLV


463   The Adjacency SID TLV is used in order to advertise information

464   related to an Adjacency SID.  This information is originated as in

465   Adj-SID sub-TLV of IS-IS [I-D.ietf-isis-segment-routing-extensions],

466   OSPFv2 [I-D.ietf-ospf-segment-routing-extensions] and OSPFv3

467   [I-D.ietf-ospf-ospfv3-segment-routing-extensions


[minor] It may be a simple change in the text, but I want to make sure...
The text above "This information is originated as in..." represents a
change (going forward) from the text used before: "This information is
derived from..."   The latter seems a better fit for this document.  Is
there a deeper reason for the change?



...

490      Flags. 1 octet value which sould be parsed as:


[nit] s/sould/should


[minor] s/be parsed/be set


[] Same comment for 2.2.2 and 2.3.1.


492      *  IS-IS Adj-SID flags are defined in

493         [I-D.ietf-isis-segment-routing-extensions] section 2.2.1.


495      *  OSPFv2 Adj-SID flags are defined in

496         [I-D.ietf-ospf-segment-routing-extensions] section 6.1.


498      *  OSPFv3 Adj-SID flags are defined in

499         [I-D.ietf-ospf-segment-routing-extensions] section 7.1.


[minor] This information about the flags, and the information below about
the SID/Index/Label, is redundant: the text above already says where the
information comes from.


[] Same comment for 2.2.2 and 2.3.1.


501      Weight: Weight used for load-balancing purposes.


503      Reserved: 2 octets that SHOULD be set to 0 and MUST be ignored on

504      receipt.


506      SID/Index/Label:


508      *  IS-IS: Label or index value as defined in

509         [I-D.ietf-isis-segment-routing-extensions],


511      *  OSPFv2: Label or index value as defined in

512         [I-D.ietf-ospf-segment-routing-extensions],


514      *  OSPFv3: Label or index value as defined in

515         [I-D.ietf-ospf-ospfv3-segment-routing-extensions],


517   The Flags and, as an extension, the SID/Index/Label fields of this

518   TLV need to be interpreted accordingly to the respective underlying

519   IS-IS, OSPFv2 or OSPFv3 protocol.  The Protocol-ID of the BGP-LS Link

520   NLRI should be used to determine the underlying protocol

521   specification for parsing these fields.


[minor] This last paragraph is true for all the (sub-)TLVs, and not just
for the ones where the text exists.  That is true, right?


[major] I know that I made a comment before about the text originally in
§2, and that that comment resulted in this new text.  My main concern about
the original text was its Normative nature.  I think that the current text
is ok, but the placement is inconsistent and putting it in a neutral place
may be better.  Yes, I'm asking you to take it back to §2: a generic,
non-normative statement.


523 2.2.2.  LAN Adjacency SID TLV

...

534   This information is originated as in LAN Adj-SID sub-TLV of IS-IS

535   [I-D.ietf-isis-segment-routing-extensions], OSPFv2

536   [I-D.ietf-ospf-segment-routing-extensions] and OSPFv3

537   [I-D.ietf-ospf-ospfv3-segment-routing-extensions].


[nit] For IS-IS, the name is "LAN Adj-SID".



...

605 2.2.3.  L2 Bundle Member Attribute TLV

...

617   This information is originated as in L2 Bundle Member Attributes TLV

618   of IS-IS [I-D.ietf-isis-l2bundles].  The equivalent functionality has

619   not been specified as yet for OSPF.


[nit] s/as in L2/as in the L2



...

696 2.3.1.  Prefix SID TLV


698   The Prefix SID TLV is used in order to advertise information related

699   to a Prefix SID.  This information is originated as in Prefix-SID

700   sub-TLV of IS-IS [I-D.ietf-isis-segment-routing-extensions], OSPFv2

701   [I-D.ietf-ospf-segment-routing-extensions] and OSPFv3

702   [I-D.ietf-ospf-ospfv3-segment-routing-extensions].


[nit] For OSPF, the name is "Prefix SID".



...

800 2.3.3.  Source Router Identifier (Source Router-ID) TLV


802   The Source Router-ID TLV contains the IPv4 or IPv6 Router-ID of the

803   originator of the Prefix.  For IS-IS protocol this is as defined in

804   [RFC7794] IPv4 or IPv6 Router-ID of the originating router.  For OSPF

805   protocol, this is as defined in [I-D.ietf-lsr-ospf-prefix-originator]

806   and is a 32 bit OSPF Router-ID of the originating router..


[nit] s/For xxx protocol/For the xxx protocol


[minor] To be consistent with the other sections...  rfc7794 talks about
the IPv4/IPv6 Source Router ID sub-TLV...and
I-D.ietf-lsr-ospf-prefix-originator about the Prefix Source Router-ID
sub-TLV.



...

829 2.3.4.  Range TLV


831   The range TLV is used in order to advertise a range of prefix-to-SID

832   mappings as part of the Segment Routing Mapping Server (SRMS)

833   functionality [I-D.ietf-spring-segment-routing-ldp-interop], as

834   defined in the respective underlying IGP SR extensions

835   [I-D.ietf-ospf-segment-routing-extensions],


837   [I-D.ietf-ospf-ospfv3-segment-routing-extensions] and

838   [I-D.ietf-isis-segment-routing-extensions].


[nit] There's an extra line...


[nit] s/range TLV/Range TLV


[major] Unlike other sections, this one doesn't explicitly mention which
(sub-)TLV the information is derived from.  This is specially important
because the Flags field below doesn't have a specific reference.


These explicit mentions show up in the "Advertisement Procedure"
sub-sections, which basically just say where the information is derived
from.  I don't understand why those sub-sections are needed for this case
and not all the other TLVs in this document.  ???    Suggestion: delete the
sub-sections.



...

884   Range TLV

885      Prefix-SID TLV (used as a sub-TLV in this context)


887   Where:


889   o  The Range TLV is defined in Section 2.3.4.


891   o  The Prefix-SID TLV (used as sub-TLV in this context) is defined in

892      Section 2.3.1.


[minor] These last few sentences are a little confusing, specially since
there's a reference to §2.3.4, which is this section!  Perhaps take the
text starting with "Where" out and simply put a reference to §2.3.1 where
the Prefix-SID TLV is mentioned.



...

922 2.4.  Equivalent IS-IS Segment Routing TLVs/Sub-TLVs


[major] The Tables below mention the reference draft names (good!), but
also specific sections.  Mentioning the sections is not great becasue they
may change -- for example §3.2 doesn't exist in the OSPFv3 draft.  Perhaps
a better idea would be to mention the name of the (sub-)TLVs along with the
draft reference (and no sections).