|
Hi Bruno,
See inline.
Hi Acee,
Thanks for your quick reply, accommodating my comments, and the updated draft.
Please see comments inline [Bruno]
From: Acee Lindem (acee) [mailto:acee@xxxxxxxxx]
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" <bruno.decraene@xxxxxxxxxx> 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.
|
