[Idr] AD Review of draft-ietf-idr-bgp-prefix-sid-07

[Alvaro Retana <[e-mail]>]

Dear authors:

Hi!  I just finished reading this document.

I have a significant number of comments (please see below).  Two of them I
would like to highlight up here.

(1) References to draft-ietf-spring-segment-routing-msdc

In the current text, draft-ietf-spring-segment-routing-msdc is not only
referenced as an example of the use of the BGP Prefix-SID attribute, but in
a Normative way to indicate how the attribute should be treated.  While
there’s nothing wrong with draft-ietf-spring-segment-routing-msdc (it
already passed IETF LC and is in IESG Evaluation), I’m sure there are other
applications for the new attribute, right?  The importance given to it in
this document, which should elevate it to a Normative reference, is
probably more than it deserves since it just focuses on describing "the
design to deploy segment routing in [large-scale] data-centers” and it
doesn’t contain a list of requirements nor it mandates how the new
attribute should be used.  Note also
that draft-ietf-spring-segment-routing-msdc 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-msdc as
what is should be: a good Informative reference — I put specific comments
above about the text below (see M1).


(2) Error Handling

The error handling (in Section 6) specifies that “attribute discard”
(rfc7606) should be used if the attribute is malformed.  I think that
behavior has a direct effect on the path used to forward the traffic, which
puts it then in conflict with rfc7606 because it clearly says that
"Attribute discard...MUST NOT be used except in the case of an attribute that
has no effect on route selection or installation.”  Please see M9 below.


I will wait for the Major comments to be addressed before starting the IETF
LC.

Thanks!

Alvaro.



Major:

M1. References to draft-ietf-spring-segment-routing-msdc.  As I mentioned
above, I believe that this document gives a lot more importance
to draft-ietf-spring-segment-routing-msdc than it deserves — for one, I
think that draft-ietf-spring-segment-routing-msdc should not be considered
as a Normative source for this document, but as it stands right now its
reference would have to in fact be Normative.

M1.1 Section 1: "As described in [I-D.ietf-spring-segment-routing-msdc],
the BGP Prefix-SID attribute defined in this document can be attached to
prefixes from AFI/SAFI...labeled IPv4/IPv6 Unicast...unlabeled IPv6
Unicast”.  draft-ietf-spring-segment-routing-msdc does describe that, but
within the DC application — IOW, just because one use case uses the BGP
Prefix SID in that way doesn’t mean that it determines it’s use…just an
example.  s/As described in [I-D.ietf-spring-segment-routing-msdc]/

M1.2. s/[I-D.ietf-spring-segment-routing-msdc] describes use cases where
the Prefix-SID is used for the above
AFI/SAFI./[I-D.ietf-spring-segment-routing-msdc] describes use cases where
the Prefix-SID is used for the above AFI/SAFI in a MSDC.

M1.3. Section 1: "As described in [I-D.ietf-spring-segment-routing-msdc], a
BGP Prefix-SID MAY be attached to a prefix.”

M1.3.1. draft-ietf-spring-segment-routing-msdc doesn’t even use Normative
language

M.1.3.2. ...even if it did, what is the option?  Why is “MAY” used?  What
else can the BGP Prefix-SID be attached to?  Everywhere else in this
document, the text talks about attachment to a prefix.

M1.3.3. Section 5. (Announcing BGP-Prefix-SID Attribute) makes a similar
statement: "The BGP Prefix-SID attribute MAY be attached to labeled BGP
prefixes (IPv4/IPv6) [RFC3107] or to IPv6 prefixes [RFC4760].”  Same
questions: is there another option?  In this case, did you mean “…MUST only
be attached to…”?

M1.4. Section 2.1: "As described in [I-D.ietf-spring-segment-routing-msdc]
the operator assigns a globally unique “index”, L_I…”.  Again, that’s an
example…. s/As described in [I-D.ietf-spring-segment-routing-msdc]/

M1.4.1. [nit] BTW, is there a reason for “index” to be in “”??  It isn’t
anywhere else.

M1.4.2. [minor] Also, please be consistent.  In some places you use “Label
Index” (as in the packet format), but also “label index”, label_index,
label-index, Label-Index, simply index or even the (redundant?) "index
L_I".  Some of those may be ok, but many seem to talk about the same thing
while calling it by slightly different names.

M1.5. Section 2.2: "As illustrated in
[I-D.ietf-spring-segment-routing-msdc]...the BGP Prefix-SID consists of an
IPv6 address…”. s/As illustrated in [I-D.ietf-spring-segment-routing-msdc]/

M1.6. Section 3.3: "It is used to build segment routing policies when
different SRGB's are used in the fabric
([I-D.ietf-spring-segment-routing-msdc]).”
 I’m assuming that the fabric is not the only use case of having different
SRGBs, right?  Make the statement general and not dependent
on I-D.ietf-spring-segment-routing-msdc.

M1.7. Section 8: "This document defines a new BGP attribute in order to
address the use case described in [I-D.ietf-spring-segment-routing-msdc].”
 I’m sure that is not the only use case…reword to show it is an example.

M1.8. Section 9: "The BGP Prefix-SID attribute addresses the requirements
introduced in [I-D.ietf-spring-segment-routing-msdc]…”.
draft-ietf-spring-segment-routing-msdc
doesn’t actually present requirements…but an application in the DC...


M2. TLV Definitions:

M2.1. [minor] what is the unit used in the Length?  Bytes, octets, bits

M2.2. Please define a registry and a registration policy for the Flag
fields.


M3. What should be done if a Label-Index TLV is received if an unlabeled
IPv6 prefix or if the IPv6 SID TLV is received with a labeled prefix?  I
assume they MUST be ignored…but please include that in the text.


M4. IPv6 SID TLV

M4.1. Section 3.2 defines this TLV as optional ("IPv6-SID TLV MAY be
present”), ok.  Section 4.2 then says that "If present, then the
receiver assumes
that the originator supports SR on the IPv6 dataplane.”  This seems to
indicate that the purpose of this TLV is not only to advertise an SID, but
to communicate support for SR, is that correct?  Later, 5.2 again says that
the sender "MAY include the IPv6 SID TLV”.  If this TLV is not present, can
the receiver assume that the originator doesn’t support SR?  I’m wondering
about the optional nature of the TLV, it seems to be mandatory if the
sender wants the receiver to know that it supports SR, and whenever an IPv6
SID needs to be advertised (which seems like always when using the IPv6
dataplane).  What am I missing?

M4.2. [nit] s/IPv6-SID/IPv6 SID


M5. Section 4: "A BGP speaker receiving a BGP Prefix-SID attribute from an
EBGP neighbor residing outside the boundaries of the SR domain, SHOULD discard
the attribute unless it is configured…”. If that is the only exception,
then s/SHOULD/MUST

M5.1. The same text is present in 4.2.  If the text in 4 applies to both
dataplanes, then the text in 4.2 is redundant.


M6. In 4.1: "A BGP speaker MAY be locally configured with an
SRGB=[SRGB_Start, SRGB_End].  The preferred method for deriving the SRGB is
a matter of local node configuration.”  Given that the "method for deriving
the SRGB is a matter of local node configuration”, that “MAY” is out of
place since it has no Normative value.  s/MAY/may


M7. Section 4.1 explains the conditions under which the BGP Prefix-SID
attribute should be considered unacceptable, and it says that the receiver
of “an unacceptable BGP Prefix-SID attribute...MUST treat the path as if it
came without a Prefix-SID attribute.”  However, Section 5.1 says that a
"speaker that advertises a path received from one of its neighbors SHOULD
advertise the Prefix-SID received with the path without modification
regardless of whether the Prefix-SID was acceptable”.  I think there is a
Normative contradiction here because the MUST seems to imply that the
attribute is to be dropped (“as if it came without”), but the SHOULD
indicates the opposite.  I can also see how the text could be interpreted
as “just ignore the attribute but forward it”.  Please clarify so there is
no confusion.

M7.1. I assume that the acceptable condition doesn’t just apply to the MPLS
data plane.  It may be clearer if the text in 4.1 was moved to a common
section.

M7.2. Note that the same text (from 5.1) is repeated in 5.2.  It may be
clearer if the specification was in a common place instead.


M8. Section 6: "If the BGP Prefix-SID attribute appears more than once in
an BGP Update message, then, according to [RFC7606], all the occurrences of
the attribute other than the first one SHALL be discarded and the BGP Update
message SHALL continue to be processed.”  Note that rfc7606 doesn’t say
exactly that, instead it says "all the occurrences of the attribute other
than the first one SHALL be discarded and the UPDATE message will continue to
be processed”.  Yes, it’s a slight difference, but it’s not the same.  In
this case, the Normative language is already in rfc7606, so there’s no real
need to repeat it here (if you do, please use “”).  Suggestion:
 “…according to rfc7606, only the first occurrence will be considered.”


M9.  If the attribute is malformed then the index or SID in it won’t be
used by the receiver (attribute discard) as specified in Section 6.
Section 4.1 explains how a local label is assigned if the BGP Prefix-SID
attribute is unacceptable.

M9.1. I’m assuming that the local label assignment in 4.1 includes the case
when the attribute is malformed (i.e. not just the unacceptable case).  Is
that true?   Using a local label would result in successfully delivering
the traffic to the destination, but not using the intended SR path, right?
If that is true, then the malformed attribute case would have an effect on
route selection/installation and could result (among other things) in the
traffic taking an unwanted path due to policy, a path that is congested,
etc.…and that is explicitly the case where rfc7606 specifies that attribute
discard MUST NOT be used.  It seems that any action beyond attribute
discard may be too harsh given that a local label can be assigned…but the
current specification ("MUST treat the path as if it came without a
Prefix-SID attribute...MUST assign a local (also called dynamic) label”) is
in conflict with rfc7606.   I would be ok if the document explained the
potential issues and made the local label behavior optional (pending a
discussion in the WG and maybe an update to rfc7606).  BTW, was this
discussed in the WG (I couldn’t find a related discussion in the archive)?

M9.2. What about the IPv6 case?  If the attribute is malformed, then the
receiver won’t have an IPv6 SID to use — again, there seems to be a clear
effect on route selection and installation.


M10. Section 8. (Manageability Considerations).  There’s a Normative
conflict on these two sentences: "By default, a BGP Prefix-SID attribute
SHOULD NOT be originated and attached to a prefix.  The operator MUST be
capable of explicitly enabling the BGP Prefix-SID origination.”  The “MUST"
indicates that the operator has to be involved, but the "SHOULD NOT"
implies that there are cases where it is ok for the operator not to be
involved.


M11. Security Considerations

M11.1. You should mention the Security Considerations of the overall SR
architecture.

M11.2. While I agree with the rest of this section, I think there’s a new
risk that is introduced by the attribute: the modification of the TLVs in
transit.  For example, if one of the transit BGP speakers modifies the
Label Index to make it an unacceptable TLV (according to 4.1) then it would
have the effect of potentially causing the traffic to take a different
path.  I don’t think there’s a mitigation mechanism (specially because the
document describes upfront the unacceptable criteria — which is what opens
this door), but recognizing the modification risk and at least mentioning
that all the routers are part of the same domain (trusted) would be a good
idea.


M12. The Reference to rfc4760 should be Normative.



Minor:

P1. Please update the Requirements Language to use the new template in
rfc8174.

P2. rfc3107 hs been obsoleted by rfc8277

P3. s/as_path/AS_PATH

P4. s/is assumed to be done through/is done through

P5. "As defined in [I-D.ietf-spring-segment-routing-mpls], the index L_I is
an offset in the SRGB.”  The reference should
be I-D.ietf-spring-segment-routing (that is where the index and the SRGB
are explained).

P6. Please be consistent when referring to the new attribute: s/Prefix-SID
attribute/BGP Prefix-SID attribute



Nits:

N1. s/It i assumed/It is assumed