Re: [RTG-DIR] Rtgdir last call review of draft-ietf-idr-bgp-prefix-sid-21

["Acee Lindem (acee)" <[e-mail]>]

Hi Bruno,

See inline.


On Jun 15, 2018, at 12:14 PM, [e-mail]<mailto:[e-mail]> wrote:

Hi Acee,

Thanks for your quick reply, accommodating my comments,  and the updated draft.

Please see comments inline [Bruno]

From: Acee Lindem (acee) [mailto:[e-mail]]
Sent: Thursday, June 14, 2018 3:02 AM

Hi Bruno,

Thanks for your review - you’ve raised some heretofore undetected ambiguities that need to be
rectified. I've taken most of your comments. I plan to publish an update including all the comments
that I've incorporated (tonight or tomorrow morning).

See replies inline.

On 6/13/18, 1:53 PM, "Bruno Decraene" <[e-mail]<mailto:[e-mail]>> wrote:

   Reviewer: Bruno Decraene
   Review result: Has Nits

    Hello,

   I have been selected as the Routing Directorate reviewer for this draft. The
   Routing Directorate seeks to review all routing or routing-related drafts as
   they pass through IETF last call and IESG review, and sometimes on special
   request. The purpose of the review is to provide assistance to the Routing ADs.
   For more information about the Routing Directorate, please see
   ​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

   Although these comments are primarily for the use of the Routing ADs, it would
   be helpful if you could consider them along with any other IETF Last Call
   comments that you receive, and strive to resolve them through discussion or by
   updating the draft.

   Document: draft-ietf-idr-bgp-prefix-sid-21
   Reviewer: Bruno Decraene
   Review Date: 2018-06-13
   IETF LC End Date:  2018-06-12
   Intended Status: Standard Track

   ==========================
   Summary:

       I have some minor concerns about this document that I think should be
       resolved before publication.

   ==========================
   Comments:

   Document is clear. Altough sometimes it's visible that the document has been
   significantly edited over its history and could probably be made clearer with a
   full rewrite from a single editor.

Yes - the original editors will be forever in my debt ;^)


   ==========================
   Major Issues:

   § IANA consideration

      "This document also requests creation of the "BGP Prefix-SID Label-
      Index TLV Flags" registry under the "Border Gateway Protocol (BGP)
      Parameters" registry, Reference: draft-ietf-idr-bgp-prefix-sid.
      Initially, this 16 bit flags registry will be empty.  Flag bits will
      be allocated First Come First Served (FCFS) consistent with the BGP-
      SID TLV Types registry."

   IMHO a registry of only 16 possible entries seems very small for a FCFS policy.
   Anyone would be able to deplete it in minutes. (cf RFC 8126 "It is also
   important to understand that First Come First Served really has no
   filtering."). Is this really the intention of the WG? (Actually I'm wondering
   what would be the monetary value of such a flag on the black market... If zero,
   this means that the flag are useless. If non-zero, the benefit may be worth the
   trouble)

   Same comment for the "BGP Prefix-SID Originator SRGB TLV Flags" registry.

I don't believe we need to consider attacks on the FCFS registries. You've got to believe that IANA
will only consider legitimate requests. If not, it seems the whole concept of FCFS is flawed.

[Bruno]
As you changed the registry to "Expert review", I believe additional guidance need to be indicated to the expert. (sorry, and I'm not even paid by the number of comments ;-) )
According to https://tools.ietf.org/html/rfc8126#section-4.5

  "The registry's  definition needs to make clear to registrants what information is
  necessary.  [...]

  The required documentation and review criteria, giving clear guidance
  to the designated expert, should be provided when defining the
  registry.  It is particularly important to lay out what should be
  considered when performing an evaluation and reasons for rejecting a
  request.  It is also a good idea to include, when possible, a sense
  of whether many registrations are expected over time, or if the
  registry is expected to be updated infrequently or in exceptional
  circumstances only.”


You probably saw my response to Alvaro and know that I think the requirement for guidance is bull. However, I’ve never been weak in this domain and would suggest the following:


The designated experts must be good and faithful stewards of the above registries,
assuring that each request is legitimate and corresponds to a viable use case. Given
the limited number of bits in the flags registries and the applicability to a single TLV,
additional scrutiny should be afforded to flag bit allocation requests. In general, no
single use case should require more than one flag bit and, should the use case
require more, alternate encodings using new TLVs should be considered.





   ==========================
   Minor Issues:

   I think that the Abstract should mention that this specification is dedicated
   to SR-MPLS (as opposed to covering both SR-MPLS & SRv6, which was the case at
   the beginning of its history) ---------------------- §1

Since the BGP Prefix-SID attribute could be extended in the future to SRv6, I don't want to make
such distinction.

[Bruno] I agree for the first part.
Regarding the last part, such distinction is already explicit in the draft: "The applicability and support for Segment Routing over IPv6 is beyond the scope of this document."

I'm asking to make this clear from the abstract.
IOW, the content of the document has changed by removing all SRv6 content. It seems logical to me that the Abstract be also updated to reflect that change. e.g.

OLD:
  This document defines an optional, transitive BGP attribute for
  announcing BGP Prefix Segment Identifiers (BGP Prefix-SID)
  information.

NEW
  This document defines an optional, transitive BGP attribute for
  announcing BGP Prefix Segment Identifiers (BGP Prefix-SID)
  information and the specification for SR-MPLS SIDs.

Ok. I will add this to the abstract in the -25 version.





   "A BGP-Prefix Segment (and its BGP Prefix-SID) is a BGP segment
      attached to a BGP prefix."

   I'd propose the following change

      A BGP-Prefix Segment is a BGP labeled prefix with a Prefix-SID attached.


   Rationals:
   1) I'd rather say that this is the SID which is attached to the prefix, rather
   than the Segment. 2) Given that this document is dedicated to SR-MPLS, we can
   probably say that the BGP prefix is labeled.

I agree this is better but will leave out the "labeled" since the attribute could be extended in the
future.

[Bruno] Agreed. Thanks.
In the same spirit, you may want to revisit the following text from §6 Error Handling

"   When a BGP Speaker receives a BGP Update message containing a
  malformed or invalid BGP Prefix-SID attribute attached to a IPv4/IPv6
  Labeled Unicast prefix [RFC8277], it MUST ignore the received BGP
  Prefix-SID attributes and not advertise it to other BGP peers."

And possibly also the following text:
"  The BGP Prefix-SID attribute defined in this document can be attached
  to prefixes from Multiprotocol BGP IPv4/IPv6 Labeled Unicast
  ([RFC4760], [RFC8277]).  Usage of the BGP Prefix-SID attribute for
  other Address Family Identifier (AFI)/ Subsequent Address Family
  Identifier (SAFI) combinations is not defined herein but may be
  specified in future specifications."

I think that what is meant, is that the "Label-Index TLV"  and "Originator SRGB TLV" may be attached to BGP IPv4/IPv6 Labeled Unicast  ([RFC4760], [RFC8277]) routes.

Since this is the Prefix-SID attribute, I think “prefix” is as clear as “route”. For these AFI/SAFI combinations, I really don’t see that there is any ambiguity here as “routes” and “prefixes” are often used interchangeably. However, given your preference and the fact that they are used interchangeably, I will use route in these two paragraphs.




   But up to the authors.



   -----
   §1
   "within the SR/BGP domain"
   Can you point to the definition of "BGP domain"? If not, "SR domain" is
   probably enough. ---

Agreed.

      The BGP Prefix-SID attribute defined in this document can be attached
      to prefixes from Multiprotocol BGP labeled IPv4/IPv6 Unicast
      ([RFC4760], [RFC8277]).  Address Family Identifier (AFI)/ Subsequent
      Address Family Identifier (SAFI) combinations.

   1) The full point "." in the middle of the sentence is probably not intended.
   2) I don't find the following text crystal clear: "BGP labeled IPv4/IPv6 Unicast
      ([RFC4760], [RFC8277]).  Address Family Identifier (AFI)/ Subsequent
      Address Family Identifier (SAFI) combinations."

This is fixed in the -22 version.

   IMO adding the numerical values would help (my undertanding is AFI/SAFI 1/4 and
   2/4). Alternatively using the names as per the IANA registries. Also using the
   AFI/SAFI order in "Address Family Identifier (AFI)/ Subsequent Address Family
   Identifier (SAFI) " while the SAFI/AFI order in "BGP labeled IPv4/IPv6 Unicast"
   is misleading.

   Proposed NEW:
      The BGP Prefix-SID attribute defined in this document may be attached
      to BGP IPv4/IPv6 labelled routes. (AFI/SAFI 1/4 or 2/4).

A previous review asked for expansion of AFI/SAFI. I will change the order to "BGP IPv4/IPv6
Labeled Unicast" consistent with AFI/SAFI throughout.

[Bruno] Thanks.


   -----
   "o  A BGP Prefix-SID MAY be global between domains when the
         interconnected domains agree on the SID allocation scheme."

   I think you mean :s/global/domain wide

Since I've defined SR domain more precisely, I will replace the unqualified "domain" with "AS" in
this sentence.

[Bruno] Looks better, thanks.

   (as opposed to the term "Global Segment" which has a different meaning.)

   BTW in "A BGP Prefix-SID is always a global SID
   ([I-D.ietf-spring-segment-routing])" I guess you mean :s/global SID/global
   Segment

No - I mean a global SID which is an index into the SRGB.

[Bruno] OK, you are correct.


   (for the same reason)

   BTW2
   "An SR domain
      is defined as a single administrative domain for global SID
      assignment."

   Probably :s/global SID/domain wide SID

No - it is the scope of the global SID assignment.

[Bruno] OK

   BTW3
   "An SR domain
      is defined as a single administrative domain for global SID
      assignment."

   may be :s/global SID assignment/domain wide SID assignement"
   or :s/global SID/SID

No

[Bruno] OK


   -----
   OLD
      o  A BGP Prefix-SID MAY be attached to a prefix.  In addition, each
         prefix will likely have a different AS_PATH attribute.  This
         implies that each prefix is advertised individually, reducing the
         ability to pack BGP advertisements (when sharing common
         attributes).

   Proposed NEW:
      o  A BGP Prefix-SID MAY be attached to a prefix.  This
         implies that each prefix is advertised in individually, reducing the
         ability to pack BGP advertisements (when sharing common
         attributes).

   (second sentence is not relevant to this specification and seems to be specific
   to the MSDC use case)

Agreed.

   ---
   "The BGP Prefix-SID advertised for BGP prefix P indicates that the
      segment routed path should be used"

    may be :s/segment routed path/SR segment

   ("segment routed path" is not well defined to me. Usually, the terms used are
   "segment routing" or "soure routed path")) ---- "2.1.  MPLS BGP Prefix SID"
   "4.1.  MPLS Dataplane: Labeled Unicast"

You don't think it is clear that the "segment routed path" uses SR segments for routing???

[Bruno] To be honest, I don't really get what this sentence is supposed to specify.
First; what does "should be used" means? Is this supposed to influence any decision? Which one? At best, I could understand "may be used"
Second, for SR-MPLS, which is the only scope of this document, we are talking about the regular MPLS LSP. Nothing really new except a best effort to keep the label unchanged...assuming that the SRGB is the same on all nodes. We may call it "segment", but it's not "routed" any differently.
I'll leave the terminology to you, but I'd welcome a clarification on what the is standardized behavior specified by this sentence.

Ok - see below.




   There is no need anymore for this title/hierarchy (given that SR-MPLS BGP
   prefix SID are the only ones defined in this document (i.e. SRv6 removed) ----
   "The operator assigns a globally unique label index,"

I'm going to retain the hierarchy since separating out the MPLS specific part will make it more
straight forward to extend the document to SRv6 in the future.

[Bruno] OK, assuming that in the previous comment, the sentence has some real meaning/impact on the technical specification.
Otherwise the following change seems to cover both your comment and mine:

OLD:
2.  BGP-Prefix-SID

  The BGP Prefix-SID advertised for BGP prefix P indicates that the
  segment routed path should be used (as described below) if the BGP
  best path selects the corresponding Network Layer Reachability
  Information (NLRI).

2.1.  MPLS BGP Prefix SID

NEW:

2.  MPLS BGP Prefix SID

Joyeux anniversaire tôt - I will go with this and remove the troublesome paragraph above.




   The SR architecture and SR-MPLS documents use either a "label" or a "index".
   Here it's an index so I'd propose to use a consistent terminology, hence
   :s/label index/index (Multiple times in the document.)

I don't want to change this since I believe it was Alvaro who asked for "label index" in a previous
review.

[Bruno] OK, makes sense.


   -----
         "The operator assigns a globally unique label index, L_I, to a
         locally sourced prefix of a BGP speaker N which is advertised to
         all other BGP speakers in the SR domain."

   I thnk that BGP use the term "locally originated" (or simply "originated")
   rather than "locally sourced prefix"

Agreed.

   ------
         "If the BGP speakers are not all configured with the same SRGB, and
         if traffic-engineering within the SR domain is required, each node
         may be required to advertise its local SRGB in addition to the
         topological information."

   - What do you mean by "traffic engineering"? I think the issue comes from
   stacking the BGP SID below other SID(s)/labels rather than related to
   "traffic-engineering". - I think that "may be required to advertise its local
   SRGB" is an understatement. (otherwise, please explain how the label is
   computed) - "If the BGP speakers are not all configured with the same SRGB" how
   is this information supposed to be known by the source node? Guessing seems
   dangerous. - If we need the SRGB of the originator, we probably also need the
   real SID of the originator. In which case, in the following sentence I think we
   need :s/SHOULD/MUST

What are you suggesting? This is just setting the context for the two alternatives for SRGB
advertisement and indicating that BGP-LS is the preferred one for TE in the general sense. If this a
rat-hole that will result in much debate, I could remove both these statement relating to traffic
engineering as they are not required for the normative specifications in this document.

[Bruno]
From a technical standpoint, I believe that the point is that: the SRGB is needed if global segments are stacked below the BGP segment, e.g. for traffic engineering purpose.
This is a bit different as saying the SRGB is required for TE, as arguably BGP-EPE is TE, but as it uses local segment, we don't need the SRGB.

I'm suggesting:
OLD:
     If the BGP speakers are not all configured with the same SRGB, and
     if traffic-engineering within the SR domain is required, each node
     may be required to advertise its local SRGB in addition to the
     topological information.

NEW:
     If a global segment is stacked below the BGP segment, e.g. for traffic engineering purpose, the knowledge of the SRGB of the originator of the prefix Segment is required in order to compute the local label used by the originator.

Ok.




(please feel free to rephrase)


      "If multiple valid paths for the same prefix are received from
      multiple BGP speakers or, in the case of [RFC7911], from the same BGP
      speaker, and the BGP Prefix-SID attributes do not contain the same
      label index, then the label index from the best path BGP Prefix-SID
      attribute SHOULD be chosen ""

   ---

         "As defined in [I-D.ietf-spring-segment-routing], the label index
         L_I is an offset into the SRGB.  Each BGP speaker derives its
         local MPLS label, L, by adding L_I to the start value of its own
         SRGB, and programs L in its MPLS dataplane as its incoming/local
         label for the prefix."

   Actually the way to compute the MPLS labels from the index and SRGB is defined
   in https://tools.ietf.org/html/draft-ietf-spring-segment-routing-mpls Plus I'd
   rather use a pointer to this SR-MPLS specification rather than re-specify the
   computation. (especially since the text/algo is different, because the text
   from draft-ietf-idr-bgp-prefix-sid simply assumes that the SRGB is composed of
   a single block, which is not general enough for a specification. --- "

Ok - I now reference draft-ietf-spring-segment-routing-mpls.

[Bruno] ok, thanks.


         Section 3.1 of this document specifies the Label-Index TLV of the
         BGP Prefix-SID attribute; this TLV can be used to advertise the
         label index for a given prefix.

      In order to advertise the label index of a given prefix P and,
      optionally, the SRGB, an extension to BGP is needed: the BGP Prefix-
      SID attribute.  This extension is described in subsequent sections."

   Could probably be rephrased to avoid redundancy.

I agree. Given the whole section described the BGP Prefix-SID attribute, this paragraph can be
removed.

[Bruno] agreed, thanks

   ---
   "   The Label-Index and Originator SRGB TLVs are used only when SR is
      applied to the MPLS dataplane."

   Given that the scope of this document is now only MPLS-SR, this sentence may
   probably be removed. -----

I'd leave it in to simplify extension to the SRv6 data plane with a subsequent document.

[Bruno] ok.

"SRGB: 3 octets of base followed by 3 octets of
   range."

   The term "base" is not defined neither in SR-architecture, SR-ISIS, SR-MPLS.

   Actually SR-ISIS and SR-MPLS use different terms... and both documents are
   under WGLC... SR-MPLS: "the SRGB is specified by the list of
      MPLS Label ranges [Ll(1),Lh(1)], [Ll(2),Lh(2)],..., [Ll(k),Lh(k)]
      where  Ll(i) =< Lh(i)."

   SR-ISIS:
   One or more SRGB Descriptor entries, each of which have the
         following format:

            Range: 3 octets.
            SID/Label sub-TLV (as defined in Section 2.3).

I will use the following unambiguous definition:

          3 octets specifying the first label in the range followed
           by 3 octets specifying the number of labels in the range.

[Bruno]  Thanks for accommodating the nitpicking.


   -----

      "The Originator SRGB TLV contains the SRGB of the node originating the
      prefix to which the BGP Prefix-SID is attached.  The Originator SRGB
      TLV MUST NOT be changed during the propagation of the BGP update

      The originator SRGB describes the SRGB of the node where the BGP
      Prefix SID is attached. "

      Some redundancy (1rst and last sentence) could be removed.

I agree. The last sentence will be removed.

   -----
   "   The receiving routers concatenate the ranges and build the Segment
      Routing Global Block (SRGB) as follows:

            SRGB = [100, 199]
                   [1000, 1099]
                   [500, 599]

      The indexes span multiple ranges:

               index=0 means label 100
               ...
               index 99 means label 199
               index 100 means label 1000
               index 199 means label 1099
               ...
               index 200 means label 500
               ..."

   I'm not sure that there is a need to respecify the way labels are computed from
   SRGB and index.        A reference to SR-MPLS would probably be better.

Someone asked for an examples way back in the review process (way before I had taken over as
editor). However, I will change this to a  reference to the SR-MPLS document.



------ OLD:
      Consistent with [RFC7606], only the first occurrence of the BGP
      Prefix-SID attribute will be considered and subsequent occurrences
      will be discarded.  Similarly, only the first occurrence of a BGP
      Prefix-SID attribute TLV of a given TLV type will be considered
      unless the specification of that TLV type allows for multiple
      occurrences.

   Proposed NEW (even more consistent with RFC 7606)
      As per with [RFC7606], if the BGP
      Prefix-SID attribute appears more than once in an UPDATE
          message, then all the occurrences of the attribute other than the
          first one SHALL be discarded and the UPDATE message will continue
          to be processed.
      Similarly, if a recognized TLV appears more than once in an BGP
      Prefix-SID attribute while the specification only allows for a single
      occurence
          , then all the occurrences of the TLV other than the
          first one SHALL be discarded and the Prefix-SID attribute will continue
          to be processed.


   That being said, the first sentence does not define any new behavior, so is not
   really needed IMHO. -----

I think it is good to have application of RFC 7606 specified explicitly. I have updated as you
proposed.


   "4.1.  MPLS Dataplane: Labeled Unicast

      A BGP session supporting the Multiprotocol BGP labeled IPv4 or IPv6
      Unicast ([RFC8277]) AFI/SAFI is required.

      The BGP Prefix-SID attribute MUST contain the Label-Index TLV and MAY
      contain the Originator SRGB TLV.  A BGP Prefix-SID attribute received
      without a Label-Index TLV MUST be considered as "invalid" by the
      receiving speaker."

   Stricto census, the sentence prohibits the use of the BGP Prefix-SID  for other
   usage than carrying Label-Index, including for non labeled AFI/SAFI. I think
   you rather mean the following

   proposed NEW:
   When the  BGP Prefix-SID attribute is attached to a BGP labeled IPv4 or IPv6
      Unicast ([RFC8277]) AFI/SAFI, it MUST contain the Label-Index TLV and MAY
      contain the Originator SRGB TLV.  A BGP Prefix-SID attribute received
      without a Label-Index TLV MUST be considered as "invalid" by the
      receiving speaker.

I agree this is better and consistent with my desire not to preclude extension to SRv6 or other
address families.


   ----
   OLD:
      The label index provides the receiving BGP speaker with guidance as
      to the incoming label that SHOULD be assigned by that BGP speaker.

   The sentence could probably be simplified. e.g.

   NEW   The label index provides guidance, to the receiving BGP speaker, for the
   choice of its incoming label.

I'll go with a variation of that:

   The label index provides guidance to the receiving BGP speaker as to its
   choice of incoming label.

[Bruno] even better (obviously ;-) ). Thanks.


   ---
   "   If multiple valid paths for the same prefix are received from
      multiple BGP speakers or, in the case of [RFC7911], from the same BGP
      speaker, and the BGP Prefix-SID attributes do not contain the same
      label index, then the label index from the best path BGP Prefix-SID
      attribute SHOULD be chosen with a notable exception being when
      [RFC5004] is being used to dampen route changes."

   It's not clear to me why the situation is different with RFC5004, nor what
   should be done in this case. Could you please elaborate? ---

RFC 5004 dampens the rate at which BGP changes the best-path. This was also one of Alvaro's
comments.

[Bruno] IMO, the label index being used should be the one from best path selected by BGP.
Changing the rate/way the best path is computed is orthogonal to be.
That being said, that specification is just _trying_ to allocate/use the same label, but does not rely on using the same label. So I picking any label may just work... well at least for SR-MPLS, while this part is more generic.
So up to you.

I’m going to leave it since this is based on a previous comment from a BGP implementor. In the words of Abraham Lincoln, “You can’ t please all the people all the time.” Expect the other changes in the -25 version.

Thanks,
Acee




"It should be
   noted that while SRGBs and
         SIDs are advertised using 32-bit values, the derived label is
         advertised in the 20 right-most bits."

   The derived labeled is not advertised, so I don't understand the goal/meaning
   of the sentence.

I think this came from the IGP drafts. I will remove it.

   -----

   § Security considerations

      "To prevent a Denial-of-Service (DoS) or Distributed-Denial-of-Service
      (DDoS) attack due to excessive BGP updates with an invalid or
      conflicting BGP Prefix-SID attribute, message rate-limiting as well
      as suppression of duplicate messages SHOULD be deployed."

   What do you mean exactly by "message rate-limiting"? Especially what do you
   mean by "message" and what is "rate-limited" exactly? The local use of the
   Prefix SID attribute? It's propagation? In which case, should be propagation
   delayed or never propagated?

Although you are the first person to misinterpret this, I will replace "message" with "error log
message" to avoid confusion with BGP protocol messages.

[Bruno] Thanks. Indeed, I had interpreted it as BGP message (following "excessive BGP updates")

Many thanks,
--Bruno

Thanks,
Acee





_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.