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

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

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

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


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

          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. 

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

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


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


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


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

Thanks,
Acee