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

[Bruno Decraene <[e-mail]>]

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.

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

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

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

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

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

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

(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

(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

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

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

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

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

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

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

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

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

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

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

-----

§ 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?