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

["Ketan Talaulikar (ketant)" <[e-mail]>]

Hi Alvaro,

Thanks again for your review.

I’ve posted the v14 of the draft with the changes to address your comments below.


  https://tools.ietf.org/html/draft-ietf-idr-bgp-ls-segment-routing-ext-14

Please check inline below for responses to your comments.


From: Alvaro Retana <[e-mail]>
Sent: 09 May 2019 01:28
To: Ketan Talaulikar (ketant) <[e-mail]>
Cc: Susan Hares <[e-mail]>; [e-mail]; idr@ietf. org <[e-mail]>; [e-mail]
Subject: RE: AD Review of draft-ietf-idr-bgp-ls-segment-routing-ext-11

On April 18, 2019 at 7:22:50 AM, Ketan Talaulikar (ketant) ([e-mail]<mailto:[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.

[KT] I think I understand your point. I will update as below. Further statements describe the application of SR trusted domain to BGP-LS sessions.

   SR operates within a trusted domain [RFC8402<https://tools.ietf.org/html/rfc8402>]. 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 collected via BGP-LS is
   limited to specific consumers in a secure manner within this SR
   domain.

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

[KT] Ack – I’ve updated the text.



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

[KT] That’s strange, since I don’t see any errors at the link above. The tables don’t look very neat at this time though due to long draft names. I hope it would fix itself once the IGP draft names are replaced by their RFC numbers.



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.

[KT] ack – updated.

...

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.

[KT] You are right. The OSPFv3 spec re-uses the same TLV defined in the OSPFv2 spec (https://tools.ietf.org/html/draft-ietf-ospf-ospfv3-segment-routing-extensions-23#section-4). This change was done during IESG review in v19 of the OSPFv3 spec and we missed updating this document for the same. I will remove the reference to 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".

[KT] Ack - fixed



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

[KT] ack – fixed.



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

[KT] ack fixed as mentioned in previous response.

...

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.

[KT] Ack – fixed. Same as previous – the OSPFv3 spec points to the OSPFv2 for these TLVs in RI LSA.



...

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

[KT] Fixed. I refer to the ISIS spec for the ISIS flags and none are specified for OSPFv2/v3. So this aligns with 2.1.2 now.

...

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

[KT] Ack – fixed as described previously.

...

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.

[KT] Agree – removed this para.

...

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?

[KT] No deeper reason really that I know of. I will use “derived from” everywhere to be consistent.

...

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.

[KT] Fixed all of the above



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.

[KT] The “where information comes from” is common to all TLVs. The specific fields whose values are IGP protocol specific are only in select TLVs and hence this information is clarified for those fields.



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?

[KT] It is true only for the specific TLVs and their specific fields that are mentioned.



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

[KT] As explained above, this text placement is being made specifically for those TLVs/fields where this IGP protocol specific encoding is being used.



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

[KT] Fixed to LAN-Adj-SID for IS-IS



...

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

[KT] Fixed (changed to “derived from”).

...

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

[KT] Fixed

...

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

[KT] fixed



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

[KT] Fixed.

...

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

[KT] I don’t see it in the XML and there is a page break in the txt file

[nit] s/range TLV/Range TLV

[KT] Fixed

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

[KT] I have re-organized to remove these sub-sections. The text is now incorporated in the main TLV description itself so that it is consistent with other TLVs in this document.

...

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.

[KT] Updated based on previous comment.

...

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

[KT] I’ve updated the tables to list the BGP-LS TLV names, their IGP TLV name+codepoint and the draft/RFC reference.

I hope we didn’t get into mail truncation issue this time 😊

Thanks,

Ketan