[Idr] AD Review of draft-ietf-idr-bgpls-segment-routing-epe-17

[Alvaro Retana <[e-mail]>]

Dear authors:

I just finished reviewing this document.  Please take a look at in-line
comments below.

In general, I think that the document needs work in being consistent with
terminology and its use.  Also, more details are needed in the Management
and Security sections.

I have major concerns about the references to
draft-ietf-spring-segment-routing-central-epe.  In the current text,
draft-ietf-spring-segment-routing-central-epe is not only referenced as an
example of the use of the SIDs defined here, but in a Normative way to
indicate how the TLVs should be treated.  The obvious question is about the
applicability of the extensions (see the text in §5 about "other use
cases").  The importance given to
I-D.ietf-spring-segment-routing-central-epe in this document, which should
elevate it to a Normative reference, is probably more than it deserves
since it just "illustrates the application of Segment Routing to solve the
BGP Egress Peer Engineering (BGP-EPE) requirement" and it doesn’t contain a
list of protocol requirements nor it mandates how the new TLVs should be
used.  Note also that draft-ietf-spring-segment-routing-central-epe refers
normatively to this document to explain the use case, so pointing
Normatively back to it creates a loop....  Please treat
draft-ietf-spring-segment-routing-central-epe as what is should be: a good
Informative reference — I put specific comments in Sections 3 and 5 below.

I will wait for at least the major items to be addressed before starting
the IETF Last Call.

Thanks!

Alvaro.


[The line numbers come from idnits.]

...
32 Requirements Language

34   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
35   "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
36   document are to be interpreted as described in RFC 2119 [RFC2119].

[major] Please use the rfc8174 template.


...
103 1.  Introduction
...
113   The SR architecture [RFC8402] defines three types of BGP Peering
114   Segments that may be instantiated at a BGP node:

116   o  Peer Node Segment (Peer-Node-SID) : instruction to steer to a
117      specific peer node

119   o  Peer Adjacency Segment (Peer-Adj-SID) : instruction to steer over
120      a specific local interface towards a specific peer node

122   o  Peer Set Segment (Peer-Set-SID) : instruction to load-balance to a
123      set of specific peer nodes

[major] rfc8402 doesn't use the same names as above.  Please be consistent
with the language already defined there.  Here, and across the rest of the
document.


...
138   One use-case for these BGP Peering Segments is to enable computation
139   of SR paths that enable Central BGP-EPE as described in
140   [I-D.ietf-spring-segment-routing-central-epe].  This use-case
141   comprises of a centralized controller that learns the BGP Peering
142   SIDs via BGP-LS and then uses this information to program a SR policy
143   [I-D.ietf-spring-segment-routing-policy] at any node in the domain to
144   perform traffic steering via a specific BGP egress node to a specific
145   EBGP peer(s) optionally also over a specific interface.

[minor] I-D.ietf-spring-segment-routing-central-epe doesn't mention
I-D.ietf-spring-segment-routing-policy anywhere.


147   This document introduces a new BGP protocol type for BGP-LS NLRI and
148   defines new BGP-LS Node and Link description TLVs to facilitate
149   advertising BGP-LS Link NLRI that represent the BGP peering topology.
150   Further, it specifies the BGP-LS Attribute TLVs for advertisement of
151   the BGP Peering Segments (i.e.  Peer Node SID, Peer Adjacency SID,
152   and Peer Set SID) to be advertised in the same BGP-LS Link NLRI.

[major] s/introduces a new BGP protocol type for BGP-LS NLRI/introduces a
new BGP-LS Protocol-ID

[minor] s/Node and Link description TLVs/Node and Link Descriptor TLVs

[minor] s/Link NLRI/Link-State NLRI/g

154 2.  Segment Routing Documents

156   The main reference is the SR architecture defined in [RFC8402].

158   The SR BGP-EPE architecture and use-case is described in
159   [I-D.ietf-spring-segment-routing-central-epe].

[nit] This section doesn't add anything that hasn't been already
mentioned.  Please consider removing it.

161 3.  BGP Peering Segments

163   As described in [I-D.ietf-spring-segment-routing-central-epe], a BGP-
164   EPE enabled Egress PE node MAY advertise SIDs corresponding to its
165   attached peers.  These SIDs are called BGP peering segments or BGP
166   Peering SIDs.  In case of EBGP, they enable the expression of source-
167   routed inter-domain paths.

[major] "As described in [I-D.ietf-spring-segment-routing-central-epe], a
BGP-EPE enabled Egress PE node MAY advertise SIDs..."
I-D.ietf-spring-segment-routing-central-epe is just an illustrative
document; it uses Normative language just to describe the requirements for
the solution, and not to specify behavior (as suggested above).  In this
case the "MAY" seems to be used to describe the use case:  s/MAY/may


169   An ingress border router of an AS may compose a list of SIDs to steer
170   a flow along a selected path within the AS, towards a selected egress
171   border router C of the AS, and to a specific EBGP peer.  At minimum,
172   a BGP-EPE policy applied at an ingress PE involves two SIDs: the Node
173   SID of the chosen egress PE and then the BGP Peering SID for the
174   chosen egress PE peer or peering interface.

176   Each BGP session MUST be described by a Peer Node SID.  The
177   description of the BGP session MAY be augmented by additional Peer
178   Adjacency SIDs.  Finally, multiple Peer Node SIDs or Peer Adjacency
179   SIDs MAY be part of the same group/set in order to group EPE
180   resources under a common Peer-Set SID.

[minor] Please add forward references to the sections where more details
are provided.


182   When the extensions defined in this document are applied to the EPE
183   use-case defined in [I-D.ietf-spring-segment-routing-central-epe],
184   then the following BGP Peering SIDs need to be instantiated on a BGP
185   router for each of its BGP peer sessions that are enabled for EPE

[major] I-D.ietf-spring-segment-routing-central-epe is being used as
Normative (below) justification for the definition of peering segments.  If
possible, please describe the specification in general terms...  The text
above and in §5 (where it talks about different behaviors for "other use
cases") raises the question of whether this document applies in general to
any (known) use of the segments defined here, or only to BGP-EPE.  IOW,
what are those "other use cases"?  Where are they described to guarantee
that the specification applies to them?


187   o  One Peer-Node-SID MUST be instantiated to describe the BGP peer
188      session.

190   o  One or more Peer-Adj-SID MAY be instantiated corresponding to the
191      underlying link(s) to the directly connected BGP peer session.

193   o  A Peer-Set-SID MAY be instantiated and additionally associated and
194      shared between one or more Peer-Node-SIDs or Peer-Adj-SIDs.

196   While an egress point in a topology usually refers to EBGP sessions
197   between external peers, there's nothing in the extensions defined in
198   this document that would prevent the use of these extensions in the
199   context of IBGP sessions.  However, unlike EBGP sessions which are
200   generally between directly connected BGP routers which are also along
201   the traffic forwarding path, IBGP peer sessions may be setup to BGP
202   routers which are not in the forwarding path.  As such, when the IBGP
203   design includes sessions with route-reflectors, a BGP router SHOULD
204   NOT instantiate a BGP Peering SID for those sessions to peer nodes
205   which are not in the forwarding path since the purpose of BGP Peering
206   SID is to steer traffic to that specific peers.  Thus, the
207   applicability for IBGP peering may be limited to only those
208   deployments where the IBGP peer is also along with forwarding data
209   path.  Further details and the use-cases of BGP Peering SIDs and
210   their BGP-LS extensions to IBGP deployments are beyond the scope of
211   this document.

[nit] s/along with forwarding data path/along the forwarding data path

[major] "...details...of BGP Peering SIDs and their BGP-LS extensions to
IBGP deployments are beyond the scope of this document."  The text in the
same paragraph says: "nothing...would prevent the use of these extensions
in the context of IBGP sessions".  I think that is a contradiction because
there are already descriptions in this document.   Solution: take out the
last sentence.

213   The BGP Peering SIDs instantiated as described above are then
214   advertised via BGP-LS Link NLRI as described in the sections below.

[minor] I think that what you meant is something like this: "Any
instantiated BGP Peering SIDs are then advertised using BGP-LS, as
described in the following sections."

216 4.  BGP-LS NLRI for BGP

[minor] Perhaps "BGP Protocol-ID", there's no new NLRI for BGP

[minor] s/BGP-LS NLRI/BGP-LS Link-State NLRI/g   The NLRI is called
"Link-State", not "BGP-LS".

218   This section describes the BGP-LS NLRI encodings that describe the
219   BGP peering and link connectivity between BGP routers.

221   This document specifies the advertisement of BGP peering topology
222   information via BGP-LS NLRI which requires use of a new BGP protocol
223   identifier.

[minor] s/BGP protocol identifier/BGP-LS Protocol-ID    The new definition
is for BGP-LS, not BGP.

225      Protocol-ID : BGP (codepoint 7 Early Allocation by IANA Section 8
226      from the registry "BGP-LS Protocol-IDs")

[nit] Consider using a table.


...
271         Value: 4 octet unsigned non-zero integer representing the BGP
272         Identifier as defined in [RFC4271] and [RFC6286].

[minor] rfc6286 Updated rfc4271, so there's no need to include both. s/BGP
Identifier as defined in [RFC4271] and [RFC6286]./BGP Identifier [RFC6286].


...
282         Value: 4 octet unsigned non-zero integer representing the
283         Member ASN inside the Confederation [RFC5065].

[major] rfc5065 uses "Member-AS Number", please be consistent: s/Member ASN
inside the Confederation [RFC5065]./Member-AS Number [RFC5065].


285 4.2.  Mandatory BGP Node Descriptors
...
293   o  Autonomous System Number, which contains the ASN or confederation
294      identifier (ASN), if confederations are used, of the local BGP
295      node.

[minor] Is this TLV new, or are you referring to TLV 512 [rfc7752]?  Please
include the TLV number (and a reference, if appropriate) to avoid confusion.

297   Note that [RFC6286] (section 2.1) requires the BGP identifier
298   (router-id) to be unique within an Autonomous System and non-zero.
299   Therefore, the <ASN, BGP Router-ID> tuple is globally unique.

[nit] I'm not sure how this paragraph is relevant at this point...unless
you're implying that the receiver should check...if so, how should the
information be used?


...
311 4.3.  Optional BGP Node Descriptors

313   The following Node Descriptors TLVs MAY be included in BGP-LS NLRI as
314   Local Node Descriptors when distributing BGP information:

316   o  Member-ASN, which contains the ASN of the confederation member, if
317      BGP confederations are used, of the local BGP node.

[major] This description matches the definition of the TLV (§4.1), but it
also matches the description of the ASN descriptor in §4.2.  What is the
difference?  What if the contents are different?  [Same question applies
below.]


...
327   o  Node Descriptors as defined in defined in [RFC7752].

[nit] s/as defined in defined in/as defined in


329 5.  BGP-LS Attributes for BGP Peering Segments
...
370                 Figure 3: BGP-LS Peering SIDs TLV Format

[minor] s/BGP-LS Peering SIDs/BGP Peering SIDs


...
376   o  Length: variable.

[major] According to the description below, the length is variable, but
also known: it's either 7 or 8.  Please indicate the specific valid
values.  What should the receiver do if the length is not valid?


...
393      *  B-Flag: Backup Flag.  If set, the SID refers to a path that is
394         eligible for protection.

[major] There's no description of how this flag should be used.  When
should this flag be set?

BGP-LS, in general, carries information that is stored in routing protocol
databases, including BGP.  Where is this flag derived from?

396      *  P-Flag: Persistent Flag: If set, the SID is persistently
397         allocated, i.e., the SID value remains consistent across router
398         restart and session/interface flap.

[minor] There's text below that says that SIDs "SHOULD be persistent".  I
then imagine that the setting of this flag reflects the ability of the node
to persistently maintain the allocations...vs a SID-by-SID indication.
IOW, I guess that a node either can persistently maintain the allocations
or it can't (for all SIDs).  Is that correct?  If so, it seems to make more
sense that this indication would be at a Node level.  [No change needed,
just wanting to better understand...]

400      *  Rsvd bits: Reserved for future use and MUST be zero when
401         originated and ignored when received.

[minor] Do you intend for the Reserved bits to be assigned by IANA (in
later documents)?  Should they be kept in sync?

403   o  Weight: 1 octet.  The value represents the weight of the SID for
404      the purpose of load balancing.  An example use of the weight is
405      described in [RFC8402].

[major] Where does this value come from?

407   o  SID/Index/Label.  According to the TLV length and to the V and L
408      flags settings, it contains either:

[minor] draft-ietf-idr-bgp-ls-segment-routing-ext defines an SID/Label
Sub-TLV.  Why wasn't that reused here?  [Again, just trying to understand.]

410      *  A 3 octet local label where the 20 rightmost bits are used for
411         encoding the label value.  In this case, the V and L flags MUST
412         be SET.

[major] What should the receiver do if the V-flag is not set, but the
Length is 7?  Or when the V-Flag is set, but the Length is not 7?  It seems
to me that the V-Flag is not needed because the Length by itself can
determine the contents.

[major] What if the Length is set to 7, the V-Flag is set, but the L-Flag
isn't?   There are more possible error combinations...


414      *  A 4 octet index defining the offset in the SRGB (Segment
415         Routing Global Block as defined in [RFC8402] advertised by this
416         router.  In this case, the SRGB MUST be advertised using the
417         extensions defined in
418         [I-D.ietf-idr-bgp-ls-segment-routing-ext].

[major] Does this text mean that the "SR-Capabilities TLV" MUST be included
in the BGP-LS Attribute...OR...that I-D.ietf-idr-bgp-ls-segment-routing-ext
MUST also be implemented (and the information defined there received by the
consumer)?

[minor] s/SRGB (Segment Routing Global Block as defined in
[RFC8402]/Segment Routing Global Block (SRGB) [RFC8402]

[minor] If an index is included, the V-Flag is unset, what about the L-Flag?

420   The values of the Peer-Node-SID, Peer-Adj-SID, and Peer-Set-SID Sub-
421   TLVs SHOULD be persistent across router restart.

423   The Peer-Node-SID TLV MUST be included in the BGP-LS Attribute for
424   the BGP-LS Link NLRI when advertising BGP peering information for the
425   use case described in [I-D.ietf-spring-segment-routing-central-epe]
426   and MAY be omitted for other use cases.

428   The Peer-Adj-SID and Peer-Set-SID TLVs MAY be included in the BGP-LS
429   Attribute for the BGP-LS Link NLRI when advertising BGP peering
430   information for the use case described in
431   [I-D.ietf-spring-segment-routing-central-epe] and MAY be omitted for
432   other use cases.

[major] See my comments in §3 about the references to
I-D.ietf-spring-segment-routing-central-epe.



...
[minor] I think the explanation in the sub-sections below is useful.
However, please consider changing their titles to "Advertisement of the
Peer-Node-SID/Peer-Adj-SID/Peer-Set-SID".

438 5.1.  Peer-Node-SID
...
444   The Peer-Node-SID, at the BGP node advertising it, has the following
445   semantics:

447   o  SR header operation: NEXT (as defined in [RFC8402]).

449   o  Next-Hop: the connected peering node to which the segment is
450      associated.

[minor] Because the description above comes from rfc8402, maybe the
reference should be at the start, and not just specific to NEXT.  IOW, put
the reference after "semantics".

[nit] rfc8402 talks about "SR operation", not "SR header operation".

[Comments apply to the other subsections below.]


...
485 5.2.  Peer-Adj-SID
...
513   o  Link Descriptors MUST include the following TLV, as defined in
514      [RFC7752]:

516      *  Link Local/Remote Identifiers (TLV 258) contains the 4-octet
517         Link Local Identifier followed by the 4-octet Link Remote
518         Identifier [RFC5307].  The value 0 is used by default when the
519         link remote identifier is unknown.

[minor] The reference to rfc5307 is not needed because that relationship is
resolved in rfc7752.


...
561 6.  Illustration

[major] Isn't this the same example as in
draft-ietf-spring-segment-routing-central-epe.  Please point to that (maybe
in the Introduction) instead of copying the text here.  Otherwise, there
doesn't seem to be value in draft-ietf-spring-segment-routing-central-epe.


...
751 7.  Implementation Status

[major] Unfortunately, this section says nothing.  At least please put a
link to the implementation report:
https://trac.ietf.org/trac/idr/wiki/draft-ietf-idr-bgpls-segment-routing-epe%20


...
860 9.  Manageability Considerations
...
868   Specifically, the malformed NLRI attribute tests for syntactic checks
869   in the Fault Management section of [RFC7752] now apply to the TLVs
870   for the BGP-LS NLRI TLVs defined in this document.  The semantic or
871   content checking for the TLVs specified in this document and their
872   association with the BGP-LS NLRI types or their associated BGP-LS
873   Attributes is left to the consumer of the BGP-LS information (e.g. an
874   application or a controller) and not the BGP protocol.

[nit] s/the malformed NLRI attribute tests/the malformed attribute tests

[minor] s/apply to the TLVs for the BGP-LS NLRI TLVs defined in this
document/apply to the TLVs defined in this document

876   A consumer of the BGP-LS information is retrieving this information
877   from a BGP protocol component, that is doing the signaling over a
878   BGP-LS session, via some APIs or a data model (refer Section 1 and 2
879   of [RFC7752]).  The handling of semantic or content errors by the
880   consumer would be dictated by the nature of its application usage and
881   hence is beyond the scope of this document.  It may be expected that
882   an error detected in the NLRI descriptor TLVs would result in that
883   specific NLRI update being unusable and hence its update to be
884   discarded along with an error log.  While an error in Attribute TLVs
885   would result in only that specific attribute being discarded with an
886   error log.

[nit] s/is retrieving this/retrieves this

[minor] "...retrieving this information...over a BGP-LS session, via some
APIs or a data model (refer Section 1 and 2 of [RFC7752])."  I think that
even mentioning that an API or data model can be used instead of BGP-LS is
a stretch -- that is not how I interpret the initial sections in rfc7752
(which are just background sections), and there are no formal API/data
model definition.

[major] "...an error detected in the NLRI descriptor TLVs would result in
that specific NLRI update being unusable and hence its update to be
discarded along with an error log."  According to rfc7752, this statement
is true only for syntactic errors, not semantic ones.

Semantic errors ("left to the consumer", as mentioned above) means that the
BGP-LS session can be used to transport trash (trash-in-trash-out).  This
issue is bigger than this document -- but (because we don't have a current
solution) I would like to see it mentioned somewhere as a potential issue
(maybe in the Management or Security Considerations sections, your choice).

[major] "...an error detected in the NLRI descriptor TLVs would result in
that specific NLRI update being unusable and hence its update to be
discarded..."  This text says that an error in a TLV results in the whole
update (not just the TLV or the BGL-LS attribute) being discarded.  This
action goes beyond what is specified in rfc7752, which doesn't really
specify what do to in those cases.  If that is what is expected here, then
please make this behavior more prominent...use Normative language, etc..

[major] rfc7752 does mandate the use of "attribute discard" (rfc7606). I
would really like to see a discussion (or at least a mention) around the
fact that discarding an attribute may result in the receiver not having
complete information.  In the case of SR, this implies that the controller
may not have complete information to calculate the paths.

I think this is an issue bigger than this document, so I am not asking you
to solve it...I'm just asking for the document to acknowledge that it
exists and to mention what the potential impact may be.


...
895   BGP Peering Segments are associated with the normal BGP routing
896   peering sessions.  However, the BGP peering information along with
897   these Peering Segments themselves are advertised via a distinct BGP-
898   LS peering session.  It is expected that this isolation as described
899   in [RFC7752] is followed when advertising BGP peering topology
900   information via BGP-LS.

[major] rfc7752 doesn't talk about this type of isolation (it only talks
about isolation through dedicated RRs).  See also comment in the Security
Considerations section related to isolation.


...
915 10.  Security Considerations

917   [RFC7752] defines BGP-LS NLRI to which the extensions defined in this
918   document apply.  The Security Considerations section of [RFC7752]
919   also applies to these extensions.

[minor] Add something like this: "The procedures and new TLVs defined in
this document, by themselves, do not affect the BGP-LS security model
discussed in [RFC7752]."

921   BGP-EPE enables engineering of traffic when leaving the
922   administrative domain via an egress BGP router.  Therefore precaution
923   is necessary to ensure that the BGP peering information collected via
924   BGP-LS is limited to specific controllers or applications in a secure
925   manner.  By default, Segment Routing operates within a trusted domain
926   (refer Security Considerations section in [RFC8402] for more detail)
927   and its security considerations also apply to BGP Peering Segments.
928   The BGP-EPE policies are expected to be used entirely within this
929   trusted SR domain (e.g. between multiple AS/domains within a single
930   provider network).

[nit] s/trusted domain (refer Security Considerations section in [RFC8402]
for more detail)/trusted domain [RFC8402]

[major] "...precaution is necessary to ensure that the BGP peering
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.

[minor] You talk about "controllers or applications" (here and in other
places in the document).  rfc8402 talks about "consumers" to refer to the
same thing.  Please be consistent.  Suggestion: use "consumers" here as
well.


932   The isolation of BGP-LS peering sessions is also required to ensure
933   that BGP-LS topology information (including the newly added BGP
934   peering topology) is not advertised to an external BGP peering
935   session outside an administrative domain.

[major] What does "isolation of BGP-LS peering sessions" mean?  Why is it
not Normatively REQUIRED?

[major] From prior experience (see draft-ietf-idr-te-pm-bgp), the
SecDir/Sec ADs asked for text related to why it is ok to transport the new
information in BGP.  This is the text that resulted from that discussion:

   The TLVs introduced in this document are used to propagate IGP
   defined information ([I-D.ietf-lsr-isis-rfc7810bis] and [RFC7471].)
   These TLVs represent the state and resource availability of the IGP
   link.  The IGP instances originating these TLVs are assumed to
   support all the required security and authentication mechanisms (as
   described in [I-D.ietf-lsr-isis-rfc7810bis] and [RFC7471]) in order
   to prevent any security issue when propagating the TLVs into BGP-LS.
   The advertisement of the link attribute information defined in this
   document presents no additional risk beyond that associated with the
   existing set of link attribute information already supported in
   [RFC7752].

Consider adding something similar here.


...
991 13.2.  Informative References
...
1000   [I-D.ietf-idr-bgp-ls-segment-routing-ext]
1001              Previdi, S., Talaulikar, K., Filsfils, C., Gredler, H.,
1002              and M. Chen, "BGP Link-State extensions for Segment
1003              Routing", draft-ietf-idr-bgp-ls-segment-routing-ext-09
1004              (work in progress), October 2018.

[major] This reference must be Normative.

...
1018   [RFC7752]  Gredler, H., Ed., Medved, J., Previdi, S., Farrel, A., and
1019              S. Ray, "North-Bound Distribution of Link-State and
1020              Traffic Engineering (TE) Information Using BGP", RFC 7752,
1021              DOI 10.17487/RFC7752, March 2016,
1022              <https://www.rfc-editor.org/info/rfc7752>.

[major] This reference should be Normative.