Re: [Lsr] AD Review of draft-ietf-lsr-isis-srv6-extensions-11

[Peter Psenak <[e-mail]>]

Hi Alvaro,

thanks for the review, please see inline (##PP):

On 26/02/2021 19:19, Alvaro Retana wrote:
> Dear authors:
> 
> Please find below my review of this document.  I appreciate you and
> the WG discussing the details, but there is more work needed to be
> done before starting the IETF LC (details inline).
> 
> Just one high-level comment: It is not clear to me why all the
> behaviors from rfc8986 are not covered in this document.  If some are
> not applicable, or are covered elsewhere, please explain in the text.

##PP
not all behaviors from rfc8986 are applicable to IGPs. Section 10 
("Advertising Endpoint Behaviors") lists the ones that are applicable to 
ISIS.

> 
> 
> Thanks!
> 
> Alvaro.
> 
> [Line numbers from idnits.]
> 
> 
> ...
> 16	Abstract
> 
> 18	   Segment Routing (SR) allows for a flexible definition of end-to-end
> 19	   paths by encoding paths as sequences of topological sub-paths, called
> 20	   "segments".  Segment routing architecture can be implemented over an
> 21	   MPLS data plane as well as an IPv6 data plane.  This draft describes
> 22	   the IS-IS extensions required to support Segment Routing over an IPv6
> 23	   data plane.
> 
> [nit] s/Segment routing architecture/The Segment routing architecture

##PP
fixed
> 
> 
> [nit] s/This draft/This document

##PP
fixed

> 
> 
> ...
> 108	1.  Introduction
> ...
> 119	   The network programming paradigm
> 120	   [I-D.ietf-spring-srv6-network-programming] is central to SRv6.  It
> 121	   describes how any behavior can be bound to a SID and how any network
> 122	   program can be expressed as a combination of SIDs.
> 
> [major] s/[I-D.ietf-spring-srv6-network-programming]/[RFC8986]/g

##PP
replaced all references to RFC8986
> 
> 
> ...
> 131	   This document defines one new top level IS-IS TLV and several new IS-
> 132	   IS sub-TLVs.
> 
> 134	   The SRv6 Capabilities sub-TLV announces the ability to support SRv6.
> 
> 136	   Several new sub-TLVs are defined to advertise various SRv6 Maximum
> 137	   SID Depths.
> 
> 139	   The new SRv6 Locator top level TLV announces SRv6 locators - a form
> 140	   of summary address for the set of topology/algorithm specific SIDs
> 141	   instantiated at the node.
> 
> 143	   The SRv6 End SID sub-TLV, the SRv6 End.X SID sub-TLV, and the SRv6
> 144	   LAN End.X SID sub-TLV are used to advertise which SIDs are
> 145	   instantiated at a node and what Endpoint behavior is bound to each
> 146	   instantiated SID.
> 
> [nit] There is some repetition in the last few paragraphs.  You talk
> about the new work, the the TLV, sub-TLVs, TLV again, and then the
> sub-TLVs.  A little consolidation would make this part read better.

##PP
Reordered and got rid of one of the paragraphs.


> 
> 
> 148	2.  SRv6 Capabilities sub-TLV
> 
> 150	   A node indicates that it supports the SR Segment Endpoint Node
> 151	   functionality as specified in [RFC8754] by advertising a new SRv6
> 152	   Capabilities sub-TLV of the router capabilities TLV [RFC7981].
> 
> [minor] What about the functionality in rfc8986?  I'm assuming that
> you want the nodes advertising this new sub-TLV to support both.  It
> may be implicit, but please make it explicit.

##PP
there are many behaviors specified in rfc8986, and we are not making 
that part of the capability. So we can not say the ISIS SRv6 capability 
means the support of entire  rfc8986.

> 
> 
> [minor] "router capabilities TLV [RFC7981]"  What should be the
> flooding scope of the TLV that includes this new sub-TLV?

##PP
It's up to the originator to set the S bit or not. We are not limiting 
it here.

> 
> 
> ...
> 159	      0                   1                   2                   3
> 160	       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> 
> [nit] Please align the numbering.  There's one other occurrence in this section.

##PP
done

> 
> 
> ...
> 180	           O-flag: If set, the router supports use of the O-bit
> 181	           in the Segment Routing Header(SRH) as defined in
> 182	           [I-D.ietf-6man-spring-srv6-oam].
> 
> [nit] s/Segment Routing Header(SRH)/Segment Routing Header (SRH)

##PP
fixed

> 
> 
> [major] Please ask IANA to set up a registry for the Flags.

##PP
Les has started a separate thread with you on this point. I will wait 
till that discussion is closed.

> 
> 
> 184	3.  Advertising Supported Algorithms
> 
> 186	   SRv6 capable router indicates supported algorithm(s) by advertising
> 187	   the SR Algorithm TLV as defined in [RFC8667].
> 
> [nit] s/SRv6 capable router/An SRv6 capable router

##PP
fixed
> 
> 
> [minor] s/SR Algorithm TLV/SR Algorithm sub-TLV

##PP
fixed

> 
> 
> ...
> 200	4.1.  Maximum Segments Left MSD Type
> 
> 202	   The Maximum Segments Left MSD Type specifies the maximum value of the
> 203	   "SL" field [RFC8754] in the SRH of a received packet before applying
> 204	   the Endpoint behavior associated with a SID.
> 
> [minor] s/specifies/signals/g

#PP
fixed
> 
> 
> [minor] Please expand SL.

##PP
done

> 
> 
> 206	      SRH Max SL Type: 41
> 
> 208	      If no value is advertised the supported value is assumed to be 0.
> 
> [major] What exactly does this MSD type signal?  Is this the
> expectation that the SL will be <= the value when received at the
> advertiser?  Is there an example of its use?  I'm having a hard time
> picturing when (for non PSP behaviors) the SL would be more than 0.
> 

##PP

Yes, you are correct. As described in the paragraph right above it, this 
MSD type specifies what is the maximum value of the "Segments Left" 
field in the received SRH that this node is capable of processing.
A simple example: an ingress PE encapsulates a packet with a new IPv6 
and SRH. The SRH contains three segments. The Segments Left value of 
that SRH is set as per RFC8754 4.1, which in this case is equal to 2.
The router processing the first segment in the segment list, must 
support as a minimum an SRH Max SL MSD value of 2 in order to be able to 
process such packet.


> 
> 210	4.2.  Maximum End Pop MSD Type
> 
> 212	   The Maximum End Pop MSD Type specifies the maximum number of SIDs in
> 213	   the SRH to which the router can apply "PSP" or USP" behavior, as
> 214	   defined in [I-D.ietf-spring-srv6-network-programming] flavors.
> 
> [minor] Please expand PSP and USP.

##PP
done

> 
> 
> ...
> 221	4.3.  Maximum H.Encaps MSD Type
> 
> 223	   The Maximum H.Encaps MSD Type specifies the maximum number of SIDs
> 224	   that can be included as part of the "H.Encaps" behavior as defined in
> 225	   [I-D.ietf-spring-srv6-network-programming].
> 
> [nit] s/included/pushed   That is the terminology used in rfc8986.

##PP
fixed.

> 
> 
> ...
> 229	      If the advertised value is zero or no value is advertised
> 230	      then the router can apply H.Encaps only by encapsulating
> 231	      the incoming packet in another IPv6 header without SRH
> 232	      the same way IPinIP encapsulation is performed.
> 
> 234	      If the advertised value is non-zero then the router supports both
> 235	      IPinIP and SRH encapsulation subject to the SID limitation
> 236	      specified by the advertised value.
> 
> [major] rfc8986 doesn't talk about IPinIP encapsulation, but is does say this:
> 
>     The push of the SRH MAY be omitted when the SRv6 Policy only contains
>     one segment and there is no need to use any flag, tag or TLV.
> 
> Suggestion (to replace the last two paragraphs)>
>      If the advertised value is zero or no value is advertised then the
>      headend can apply an SR Policy that only contains one segment, by
>      omitting the SRH push.
> 
>      A non-zero SRH Max H.encaps MSD indicates that the headend can push
>      an SRH up to the advertised value.

##PP
done, but used "insert" instead of "push".

> 
> 
> 238	4.4.  Maximum End D MSD Type
> 
> 240	   The Maximum End D MSD Type specifies the maximum number of SIDs in an
> 241	   SRH when performing decapsulation associated with "End.Dx" behaviors
> 242	   (e.g., "End.DX6" and "End.DT6") as defined in
> 243	   [I-D.ietf-spring-srv6-network-programming].
> 
> [minor] "(e.g., "End.DX6" and "End.DT6")"  This MSD-Type only applies
> to *.DX6, DT6 and DT46, right?  If so, please be specific about it.

##PP

This MSD applies to any SID Behaviors that result in the outer IPv6 
header being removed and inner packet forwarded (e.g., End.DX4, End.DX6, 
End.DT4, End.DT6, End.DT46, End with USD, …).

I have updated the text to:

"The Maximum End D MSD Type specifies the maximum number of SIDs present 
   in an SRH when performing decapsulation. These includes, but not 
limited to, End.DX6, End.DT4, End.DT46, End with USD, End.X with USD as 
defined in [RFC8986].



> 
> 
> 245	      SRH Max End D Type: 45
> 
> 247	      If the advertised value is zero or no value is advertised
> 248	      then it is assumed that the router cannot apply
> 249	      "End.DX6" or "End.DT6" behaviors if the outer IPv6 header
> 250	      contains an SRH.
> 
> [minor] What about End.DT46?

##PP
This also applies to End.DT46 as clarified in the previous answer.

I have updated the text to:
   “If the advertised value is zero or no value is advertised
    then it is assumed that the router cannot apply
    any behavior that results in decapsulation and forwarding
    of the inner packet if the other IPv6 header contains an SRH.”


> 
> [major] What about other behaviors?  Are there no limitations to speak
> of related to them?  Why?  rfc8986 says this:
> 
>     The IGP should also advertise the Maximum SID Depth (MSD) capability of
>     the node for each type of SRv6 operation -- in particular, the SR source
>     (e.g., H.Encaps), intermediate endpoint (e.g., End and End.X), and final
>     endpoint (e.g., End.DX4 and End.DT6) behaviors.

##PP
the MSD types we defined do cover all SRv6 operations

> 
> 
> 252	5.  SRv6 SIDs and Reachability
> ...
> 269	   Locators are routable and MAY also be advertised in Prefix
> 270	   Reachability TLVs (236 or 237).
> 
> 272	   Locators associated with Flexible Algorithms [I-D.ietf-lsr-flex-algo]
> 273	   SHOULD NOT be advertised in Prefix Reachability TLVs (236 or 237).
> 
> 275	   Locators associated with algorithm 0 and 1 (for all supported
> 276	   topologies) SHOULD be advertised in a Prefix Reachability TLV (236 or
> 277	   237) so that legacy routers (i.e., routers which do NOT support SRv6)
> 278	   will install a forwarding entry for algorithm 0 and 1 SRv6 traffic.
> 
> 280	   In cases where a locator advertisement is received in both a Prefix
> 281	   Reachability TLV and an SRv6 Locator TLV, the Prefix Reachability
> 282	   advertisement MUST be preferred when installing entries in the
> 283	   forwarding plane.  This is to prevent inconsistent forwarding entries
> 284	   between SRv6 capable and SRv6 incapable routers.
> 
> [] There is a confusing combination of normative behavior for the
> locators in the last few paragraphs: "Locators...MAY also be
> advertised...SHOULD NOT be advertised...SHOULD be advertised in a
> Prefix Reachability TLV".  I realize that there are conditions
> applied...perhaps reorder some of them.
> 
> Suggestion (to replace the last 4 paragraphs)>
> 
>     Locators associated with algorithm 0 and 1 (for all supported
>     topologies) SHOULD be advertised in a Prefix Reachability TLV (236 or
>     237) so that legacy routers (i.e., routers which do not support SRv6)
>     will install a forwarding entry for algorithm 0 and 1 SRv6 traffic.
> 
>     In cases where a locator advertisement is received in both a Prefix
>     Reachability TLV and an SRv6 Locator TLV, the Prefix Reachability
>     advertisement MUST be preferred when installing entries in the
>     forwarding plane.  This is to prevent inconsistent forwarding entries
>     between SRv6 capable and SRv6 incapable routers.
> 
>     Locators associated with Flexible Algorithms (see Section 4,
>     [I-D.ietf-lsr-flex-algo]) SHOULD NOT be advertised in Prefix Reachability
>     TLVs (236 or 237).


##PP
done

> 
> 
> [major] "locator advertisement is received in both a Prefix
> Reachability TLV and an SRv6 Locator TLV"  What information results in
> these being an advertisement for the same locator?  Is only the
> locator (prefix length, etc.) considered, or should the algorithm,
> metric, etc. also match?

##PP
it's just prefix/mask. I added some text to clarify that.


> 
> 
> [major] "the Prefix Reachability advertisement MUST be preferred when
> installing entries in the forwarding plane"   Ok, but what about the
> rest of the information in the SRv6 Locator TLV?  How should that be
> considered?  For example, I'm assuming that the information in the
> sub-TLVs is still needed.

##PP
yes, there is no impact to the rest of the data in the Locator TLV.
Added sentence to clarify.

> 
> 
> [major] When is it ok to advertise locators in Prefix Reachability
> TLVs when associated with Flexible Algorithms?  IOW, why it it only
> recommended to do so and not required?  I assume the answer has to do
> with the possibility of routers implementing flex-algo and not SRv6 in
> the network -- please mention that.

##PP
Advertising the FA locator in regular Prefix Reachability would make the 
forwarding for it to follow algo 0 path. I don't know what this could be 
good for, but we did not want to preclude that. I added the text about it.

> 
> 
> [major] The SRv6 Locator TLV is a new TLV.  If no Prefix Reachability
> TLVs are present, how should the new TLV be used for route
> calculation/installation?  The text above suggests its use, but there
> is no specification.

#PP
The locator TLV advertises the prefix, same way as the Prefix 
Reachability. The calculation and installation of the prefix 
reachability is equal to what is done for regular Prefix Reachability.
I added the following text to one of the above paragraphs:

"The processing of the prefix advertised in the SRv6 Locator TLV,
the calculation of its reachability and  the installation in the 
forwarding plane is similar to one used for Prefix Reachability TLV (236 
or 237)."


> 
> 
> ...
> 291	   SRv6 SIDs are not directly routable and MUST NOT be installed in the
> 292	   forwarding plane.  Reachability to SRv6 SIDs depends upon the
> 293	   existence of a covering locator.
> 
> [major] "MUST NOT be installed"  This action depends on how the SIDs
> are advertised.  For example, if they are included in a Prefix
> Reachability TLV then the receiver will install them.  IOW, this
> action should be specified from the point of view of the sender; 

##PP
This section talks about received  SRv6 SIDs, not locators. SIDs are not 
advertised in Prefix Reachability TLV. Remote SIDs are never installed 
in the forwarding.

I updated the text to say "SRv6 SIDs received from other nodes..."

for
> example, "SIDs MUST be advertised in the sub-TLVs...[or maybe]...MUST
> NOT be advertised in another TLV...".

##PP
the previous section talks about how SIDs are advertised, which is the 
only way.


> 
> 
> [major] If some SIDs are advertised as "sub-TLVs in TLVs 22, 23, 222,
> 223, and 141", how can the "MUST NOT be installed" be satisfied?

##PP
above is the only way how SIDs are advertised.

> 
> 
> ...
> 302	   In order for forwarding to work correctly, the locator associated
> 303	   with SRv6 SID advertisements MUST be the longest match prefix
> 304	   installed in the forwarding plane for those SIDs.  There are a number
> 305	   of ways in which this requirement could be compromised.  In order to
> 306	   ensure correct forwarding, network operators should take steps to
> 307	   make sure that this requirement is not compromised.
> 
> 309	   o  Another locator associated with a different topology/algorithm is
> 310	      the longest match
> 
> 312	   o  A prefix advertisement (i.e., from TLV 236 or 237) is the longest
> 313	      match
> 
> [major] "MUST be the longest match prefix installed"  s/MUST/must
> This is not a normative statement.

##PP
ok, changed to must.

> 
> 
> [minor] The last two sentences in the paragraph sound redundant to me.
> 
> Suggestion>
>     In order to ensure correct forwarding, network operators should take
>     steps to make sure that this requirement is not compromised.  For
>     example, the following situations should be avoided:

##PP
done

> 
> 
> 315	6.  Advertising Anycast Property
> ...
> 322	   A new flag in "Bit Values for Prefix Attribute Flags Sub-TLV"
> 323	   registry [RFC7794] is defined to advertise the anycast property:
> 
> [major nit] The flag is defined in the sub-TLV, not the registry.
> 
> NEW>
>     A new flag in Prefix Attribute Flags Sub-TLV [RFC7794] is defined to
>     advertise the anycast property:

##PP
done


thanks,
Peter
> 
> 
> ...
> 328	       When the prefix/SRv6 locator is configured as anycast, the A-flag
> 329	       SHOULD be set. Otherwise, this flag MUST be clear
>