Re: [spring] AD Review of draft-ietf-spring-segment-routing-12

["Les Ginsberg (ginsberg)" <[e-mail]>]

Alvaro –

Apologies for the long delay in responding. The transference of the pen from Stefano resulted in a longer delay than it should have.

A new version has been published which addresses your comments. Specific replies inline.

From: spring [mailto:[e-mail]] On Behalf Of Alvaro Retana (aretana)
Sent: Thursday, August 10, 2017 12:00 PM
To: [e-mail]<mailto:[e-mail]>
Cc: [e-mail]<mailto:[e-mail]>; [e-mail]<mailto:[e-mail]>; Martin Vigoureux <[e-mail]<mailto:[e-mail]>>
Subject: [spring] AD Review of draft-ietf-spring-segment-routing-12

Dear authors:

I just finished reviewing this document – sorry for the delay in processing.  Thanks for all the work you’ve but into this document!

I have some significant concerns (see below for details).  In general, the document presents an incomplete view of the architecture: details about important pieces are barely mentioned or not fully described, as is the case of the role of a central controller (PCE), and the BGP and Binding Segments.

Also, some of the architectural details are predicated on specific bits defined in the extensions, where this document should describe the general operation and leave the details (like bit names) to the extensions.  Note that I’m not asking that you don’t mention the extensions – pointing to them is fine --, I’m asking you to define the functionality in general and not as a function of the extensions.  For an example, see point M5.1. below.

I will wait for at least the Major comments to be addressed before starting the IETF Last Call.

Thanks!

Alvaro.



Major:

M1. The Introduction mentions several types of segments, and it says that the LDP LSP, RSVP-TE LSP, and BGP LSP segments are “illustrated in [I-D.ietf-spring-segment-routing-mpls]”.  But that is only true for the first two, for which examples are shown.  Where are these segment types defined?  The definition, and not the examples, is the Major issue here.  This document being the main architecture document should include the complete description.  BTW, the list is only about the “MPLS instantiation”, are there similar types of segments for IP?

[Les:] The list has been modified – but some definitions will still be found in [I-D.ietf-spring-segment-routing-mpls] where it makes sense to do so. We prefer that so that we do not duplicate definitions.

M2. From Section 2. (Terminology): “Using the same SRGB on all nodes within the SR domain ease operations and troubleshooting and is expected to be a deployment guideline.”  It is “expected to be a deployment guideline” where/when??  Given that this document is the general architecture, I figured that maybe draft-ietf-spring-segment-routing-mpls contained that deployment recommendation, but all that document says is: “As described in [I-D.ietf-spring-segment-routing], using the same SRGB on all nodes within the SR domain eases operations and troubleshooting and is expected to be a deployment guideline.”  So…where are the Deployment/Operational considerations related to the SRGB?  I note that neither document (draft-ietf-spring-segment-routing or draft-ietf-spring-segment-routing-mpls) include them.  I would expect some information to be in this (general) document and other more specific information (like the considerations about using the same SRGB) to be in the more specific document (draft-ietf-spring-segment-routing-mpls, in this case).

[Les:] Definition of the SRGB has been enhanced and the definition of an SR Domain has been introduced and discussed. We hope this clarifies deployment considerations as regards SRGB/SRLB.

M2.1.  The example illustrated in Figure 2 would be a great place to demonstrate the value of having the same SRGB.  In fact, the text now shows things not working and it even warns by saying that “using anycast segment without configuring the same SRGB on nodes belonging to the same device group may lead to misrouting”, but no explanation of how it should be done the right way.

[Les:] The example illustrates that the same Segment needs to be associated with the anycast prefix on all nodes advertising the anycast prefix and  IS the “right way” to do things. Clarifications related to the use of the same SRGB throughout the SR Domain hopefully help.

M3. From Section 2. (Terminology): “…a global segment is represented by a globally-unique index.”

M3.1.  I couldn’t find anywhere a discussion about the use of the index.  When it is discussed in 3.1.2, it seems to be an understood concept: “A Prefix-SID is allocated in the form of an index in the SRGB…”  Even if straightforward, I think the concept of the index should be explained (maybe with an example) and not assumed.  I again went to look at draft-ietf-spring-segment-routing-mpls, but that document just points back to this one: “The notion of indexed global segment, defined in [I-D.ietf-spring-segment-routing]…”

[Les:] Definition of SID has been made more explicit and how an index is used to obtain the Segment in the SR MPLS case has been explicitly defined in Section 3.1.2.

M3.2. I assume that “globally-unique index” is really unique to the SR Domain, right?  I ask this question because “globally-unique IPv6 address” has a different connotation (as in unique world-wide, not just inside a domain).  Please clarify.

[Les:] SR Domain has been defined and the scope of the Segment more explicitly defined for both the SR MPLS and SRv6 case.

M3.3.  Note that the latest version (-10) of draft-ietf-spring-segment-routing-mpls introduced the SR Local Block (SRLB) which is not defined in this document.  I mention it here because it was introduced right after the pointer about the index…

[Les:] SRLB definition added.

M4. Section 3.1.1. (Prefix-SID Algorithm)

M4.1. This section starts by justifying the use of an algorithm based on the IGP protocol extensions: the extensions have an algorithm field, so we should use it.  The justification should be the other way around: this document (the architecture) defines the concept of an algorithm and the extensions implement it.  Please re-word the first 3 paragraphs – note that the instance/topology references seem superfluous to me.

[Les:] References to specifics of IGP advertisements have been removed and an expanded discussion of algorithm has been added.

M4.2. The algorithms are not really defined – there are mentions of a “well known ECMP-aware SPF algorithm”, but no specific reference to what that is.  The last sentence in the Section mentions that the “details of the two defined algorithms are defined in…” pointing to the extension drafts – but those drafts just offer an additional piece of information in (from the OSPF document): “the standard shortest path algorithm as computed by the OSPF protocol”.  Ideally the algorithms would be defined here (even if it is just to say: “the standard algorithm used in the corresponding IGP”), and the extensions would just reference it.

[Les:] See previous response.

M4.3.  When should the packets be dropped??  The following text points to at least 3 different places where it should be dropped, one marked with a MUST.  Please clarify.

[Les:] This has been made more explicit.

M4.3.1. “A router MUST drop any SR traffic associated with the SR algorithm to the adjacent router, if the adjacent router has not advertised support for such SR algorithm.”  Ok, this sounds as if the traffic is dropped one hop before the router not supporting the algorithm – and it has a MUST in it.

M4.3.2. “The ingress node of an SR domain validates that the path to a prefix…includes nodes all supporting the advertised algorithm.  As a consequence, if a node on the path does not support algorithm X, the IGP-Prefix segment will be interrupted and will drop packet on that node.”  This sounds like the traffic would be dropped on the node not supporting the algorithm.

M4.3.3. “It's the responsibility of the ingress node using a segment to check that all downstream nodes support the algorithm of the segment.”  This last sentence hints at the fact that the ingress node should maybe even stop the traffic there, or maybe not sending it unless all nodes in the path support the same algorithm…


M5. Section 3.1.2. (MPLS Dataplane).

M5.1. The first bullet (again!) explains the architecture in function of the extensions.  The architecture should explain what needs to be done and the extensions would then do it…  Suggestion:

[Les:] Text has been revised to be independent of the IGP specifics.

NEW>
   In order to achieve a behavior equivalent to Penultimate Hop Popping
   in MPLS [Reference], a Node N advertising a Prefix-SID SID-R for its
   attached prefix R MUST be able to instruct its connected neighbors to
   perform the NEXT operation while processing SID-R.  Upon receiving
   this instruction from Node N, its neighbors of N MUST perform the NEXT
   operation while processing SID-R.  Alternatively, the neighbors of N
   MUST perform the CONTINUE operation while processing SID-R.

M5.1.1. The penultimate bullet refers again to the extensions (but it only mentions the P-bit this time).  Please explain what the architecture intends to do, but not from the point of view of the extensions.  This and the last bullet seem to be the result of the first one (above).  I think that the first bullet would have enough text for the FIB entries to be obvious, but if you want to include this corollary, please do it right after.

M5.2. “A Prefix-SID is allocated in the form of an index in the SRGB (or as a local MPLS label) according to a process similar to IP address allocation.”  The SRGB is defined (in the Terminology section) as “the set of local labels reserved for global segments”, what is the difference between “an index in the SRGB” and “a local MPLS label”?  I’m hoping that the explanation of the concept of index would clarify this.

M5.2.1. Three bullets down… “If a node learns a Prefix-SID having a value that falls outside the locally configured SRGB range, then the node MUST NOT use the Prefix-SID…”  Going back to my previous question: it looks like “a local MPLS label” would have to be included in the SRGB – again, clarifying upfront would help a lot.  Note that this piece of text falls into the recommendation of having the same SRGB configured in all nodes.

M5.2.2.  Related to the concept of index and its relationship to the SRGB…the next bullet says: “…the segment is global (the SID is allocated from the SRGB or as an index)”.  We now have “globally-unique index”, “an index in the SRGB”, and “the SRGB or as an index” (separately).

[Les:] Terminology has been clarified and used more consistently. “globally-unique-index” is no longer used.

M6. Section 3.3. (IGP-Anycast Segment, Anycast SID): “…the value of the SID following the anycast SID MUST be understood by all nodes advertising the same anycast segment.”  It seems to me that this is really a statement of fact: all nodes, not just ones advertising an anycast segment must understand what to do with the next SID…  IOW, s/MUST/must  I would even take this sentence out because it is redundant and obvious.

[Les:] Sentence has been removed.

M7. Section 3.4. (IGP-Adjacency Segment, Adj-SID) also tries to explain functionality starting from the extensions.

[Les:] Here as well we have removed references to IGP specifics and discussed the functionality on its own.

M7.1. “The encodings of the Adj-SID include the a set of flags among which there is the B-flag.  When set, the Adj-SID refers to an adjacency that is eligible for protection (e.g.: using IPFRR or MPLS-FRR).”  If the Adjacency Segment is one that is locally significant to the node advertising it, what is the purpose of signaling that it is eligible for protection?  Wouldn’t that be a local decision as well?  Maybe an example of how the architecture is expected to work would help.

M7.2. “The encodings of the Adj-SID also include the L-flag.  When set, the Adj-SID has local significance.  By default, the L-flag is set.”  The definition of IGP-Adjacency Segment already says that it is “local (unless explicitly advertised otherwise)”, which makes this statement unnecessary.  If you want to keep it, please figure out a way that doesn’t justify it based on a bit in the extensions.


M8. Section 3.5. (Binding Segment).  A binding segment is not described anywhere in this document – please do so!  Section 3.5.1. (Mapping Server) mentions a “Remote-Binding SID S advertised by the mapping server”, and says that more “details are described in the SR/LDP interworking procedures ([I-D.ietf-spring-segment-routing-ldp-interop]”, but that draft doesn’t mention a Binding Segment.  Section 5. (IGP Mirroring Context Segment) says that “the binding segment [is] defined in SR IGP protocol extensions ( [I-D.ietf-isis-segment-routing-extensions], [I-D.ietf-ospf-segment-routing-extensions] and [I-D.ietf-ospf-ospfv3-segment-routing-extensions])”; however, those documents don’t mention a Binding Segment, they just (except for the OSPFv2 draft) define TLVs.  Note that I-D.ietf-ospf-segment-routing-extensions points back to this document when referring to the definition of a Binding SID, completing a circular reference.

[Les:] Section on Binding Segment has been rewritten and the  Mirror Context discussion is a sub-section of that section.

M8.1. BTW, Section 5. (IGP Mirroring Context Segment) says that the “Mirror SID is advertised using the binding segment”, which then looks like the Mirror Segment is an application of the Binding Segment (+ an explicit indication of it).  I think that Section 5 should then be moved to be a sub-section of 3.5.

M8.2. Part of the Security Considerations (in Section 8.1) are related to the Binding SID, which is another reason for clearly explaining that part of the architecture here.


M9. Section 3.5.1. (Mapping Server) mentions a Mapping Server, but it punts to I-D.ietf-spring-segment-routing-ldp-interop for further details.  While the SRMS can be used for interoperability cases, I think that it is an important part of the overall architecture and as such it should be described and discussed in this document instead…including any Manageability Considerations (as mentioned in Section 9).

[Les:] We have completely removed mention of SRMS from this document. While it is a useful functionality we did not consider it a key element of the architecture i.e., the architecture definition is not altered by the presence/absence of SRMS functionality.

M10. Section 3.6. (Inter-Area Considerations)

M10.1. This section shows an example of the behavior: maintain the SID across area boundaries.  But it doesn’t actually say how the architecture is expected to work.  IOW, in the example the SID is maintained, but should that always happen (MUST, SHOULD)? Or is it just an example (MAY)?

[Les:] With the introduction of the “SR Domain” definition we believe this point is now clarified.

M10.2. Another case of explaining the architecture based on the extension functionality: “When an ABR propagates a prefix from one area to another it MUST set the R-Flag.”  Maybe try something like this instead: “The re-advertisement of an SID that originated on a different IGP area MUST be indicated.”

[Les:] References to IGP specifics have been removed.

M10.3. [minor] As the advertisement moves to the left (from Area 2 to the Backbone…), the ABRs change the Node-SID for a Prefix-SID, right?  The description in the last 3 paragraphs uses Node-SID: “node S…pushes Node-SID(150)”.

[Les:] The “Segment” might change if different SRGB ranges were configured on different routers, but the Prefix-SID MUST be the same within a given SR Domain – which is what this example is illustrating. Text is now more explicit on this point.

M10.4. [minor] Going back to global vs local significance, it would be nice to remind the readers at this point that the SR domain is really the whole IGP network, and not just one flooding domain…which is why SID 150 can be used throughout.

[Les:] Again, SR Domain definition hopefully resolves this issue.

M11. Section 4. (BGP Peering Segments) is the only non-IGP focused section in this document.  The architecture of the EPE solution (as described in draft-ietf-spring-segment-routing-central-epe) goes beyond what has already been discussed here because it incorporates elements such as a mandatory centralized controller, which was optional before (a PCE server is mentioned only casually in the Terminology section).  This leaves us with an incomplete architecture for the BGP case.  I would prefer it if the whole architecture was defined in this single document (i.e. expand this section by potentially moving parts from draft-ietf-spring-segment-routing-central-epe – which might leave that document without enough content to stand alone).

[Les:] We feel that we have the right balance and would like to leave EPE document with its current scope.

M12. Section 8. (Security Considerations).  The main part of this section talks about the instructions on the packets – is adding that meta-data a security concern?  I think it could be because someone watching could tell which “forwarding path elements (e.g.: nodes, links, services, etc.)” are used for specific flows, etc.   But I also think that the risk is mitigated by the fact that the information MUST NOT be exposed outside the SR domain.  Mentioning that, and the fact that the SR domain will usually be under a single administration would be a good thing.

[Les:] SR Domain definition is explicit on this point.


M13. Section 9. (Manageability Considerations) says that “an implementation SHOULD protect the network in case of conflict detection by providing a deterministic resolution approach…addressed in [I-D.ietf-spring-conflict-resolution].”  That “SHOULD” is in conflict with I-D.ietf-spring-conflict-resolution (in WGLC) where it says that “All protocols which support SR MUST adhere to the policies defined in this document.”  It seems like the easy way forward is to s/SHOULD/MUST.  In either case, I think that I-D.ietf-spring-conflict-resolution should be a Normative reference.

[Les:] I think SHOULD is the correct verb. As conflicts occur only because of misconfiguration, it is not required that an implementation support conflict resolution. It will be the marketplace which determines the importance of conflict resolution.



Minor:

P1. Introduction

P1.1. The term “service chain” is used in the Abstract and in the Introduction.  Given that the concept is not vital to the architecture and that there might be unnecessary confusion with SFC, I would suggest taking it out.

[Les:] Term has been removed from the document.

P1.2.  Related…  Section 1.1 says that this document “defines…the service segments”, but there’s no specific mention of “service segments” anywhere, except for a very quick mention in Section 5. (IGP Mirroring Context Segment).  What am I missing?  Consider taking “service segments” off the list of things this document defines.

[Les:] Done

P1.3. The last paragraph says that “This document defines a set of instructions (called segments) that are required to fulfill the described use-cases.”  The use cases are not mentioned until 1.1.  Suggestion: merge Section 1 and 1.1 to provide proper flow to the text.

[Les:] Section 1.1 has been removed.

P1.4. Speaking of use cases, I-D.ietf-spring-oam-usecase doesn’t actually include use cases that will affect the architecture.  It includes use case of how to use monitoring system defined in it.  Same comment for the mention in Section 9.


P2. From Section 2. (Terminology)

P2.1. SID is not defined, just extended.  Besides the examples, please provide an actual definition.

[Les:] Done

P2.2. “The CONTINUE instruction is implemented as the SWAP instruction in the MPLS dataplane.”  Please include a reference to where the “SWAP instruction” is defined.  I assume you’re really referring to “label swapping” in rfc3031, are you?  If so, please be consistent as “MPLS dataplane” and “MPLS architecture” are used in different places.

[Les:] Done

P2.3. The “Local Segment” definition says that for IPv6 it “is not advertised in any routing protocol”.  But for MPLS it is advertised, right?  Please clarify.  I think that some of the vocabulary is a little confusing, for example, the definition of IGP-Adjacency Segment says that it “is local (unless explicitly advertised otherwise) to the node that advertises it” – this means that for IPv6 this piece of the architecture wouldn’t apply because a Local Segment is not advertised.  To add to the confusion, draft-filsfils-spring-srv6-network-programming talks about local SIDs…  IOW, in some places it sounds as if what should be the general architecture only really applies to MPLS – or maybe we need a more explicit local qualifier.

[Les:] Definitions have been revised to be more explicit.

P2.4. “The PCEP discovery capability is described in [I-D.ietf-pce-segment-routing].”  That document doesn’t even mention the word “discovery”.  Please use the proper name for easier cross-reference.

[Les:] Done

P2.5. “unless explicitly advertised otherwise” is used to qualify the global/local nature of a segment.  How do the characteristics of the segment change if “explicitly advertised otherwise”?  For example, if an IGP-Prefix Segment is advertised as local, how do its characteristics change, or do they?

[Les:] This phrase has been removed. The only use case for Prefix-SID is globally scoped.

P2.5.1. In Section 3.4. (IGP-Adjacency Segment, Adj-SID), the use of an Adj-SID is illustrated for both the local and global cases, but both explanations say the same thing: “forwarded along the shortest-path to N and then be switched by N, without any IP shortest-path consideration, towards link L” – I don’t see a difference in behavior between the local and global nature of this SID.  There is some additional text related to global: “The use of global Adj-SID allows to reduce the size of the segment list when expressing a path at the cost of additional state (i.e.: the global Adj-SID will be inserted by all routers within the area in their forwarding table).”  If node N is the one executing on the Adj-SID, how does including it in other FIBs help?

[Les:] The distinction is in the Segment stack required to achieve the desired behavior.
In the local ADJ-SID case two segments are required: (SN, SNL) where SN is a Node-SID which directs the packet to the desired node – and SNL is the local ADJ-SID which directs the packet over the desired link once the packet has reached Node N.
In the Global ADJ-SID case only one Segment is required (SNL).

P2.6. None of the segments after Section 3.4 (binding, BGP, mirroring) are defined in the Terminology.  Please do so for completeness.

[Les:] This has been cleaned up.


P3. Referring to Figure 1: “…each PE device is connected to two routers in each of the groups A and B.”  The routers connected to the PEs (Rx) are not in either group.

[Les:] Text modified appropriately

P4. s/Obviously//

[Les:] Removed

P5. Section 3.4. (IGP-Adjacency Segment, Adj-SID): “The remote node MAY be an adjacent IGP neighbor or a non-adjacent neighbor…”  The “MAY” is out of place as there isn’t another option, it is one or the other.  s/MAY/may

[Les:] Done

P6. Maybe it’s because the rest of 3.5 is incomplete, but Section 3.5.2. (Tunnel Head-end) has no context to speak of – without that, it feels like an orphan section.

[Les:] Use of the term “tunnel” has been removed and replaced by the more generic term SR Policy.

P7. Section 8.1. (MPLS Data Plane)

P7.1. You included a couple of references at the end of this section, which is good.  I would however not explicitly mention specific sections, as the whole RFCs (rfc4381 and rfc5920) are about MPLS-related security.

[Les:] Section references removed.

P7.2. You compare the use of a single segment to RSVP-TE – if the security characteristics are similar, please provide a reference.

[Les:] Done

P8. References

P8.1. RFC2460 was obsoleted by RFC8200 / RFC6822 was obsoleted by RFC8202

[Les:] Done

P8.2. I think that the reference to RFC4206 should be Informative.

[Les:] Done


Nits:

N1. The Abstract is very long – I would even say too long.  If it was up to me, I would leave just the first paragraph.

[Les:] We have received comments on both sides of this issue. We prefer to leave the abstract as is.

N2. The second to last paragraph in the Introduction (“Numerous use-cases…”) references the “marketing style” content of I-D.ietf-spring-oam-usecase.  I have asked the authors of that document to please focus their content.  Please consider taking this paragraph out.

[Les:] Done

N3. s/participating into the source based/participating in the source based

[Les:] Text revised

N4. References.  Please add Informative references to netconf (Section 2), PCEP (2), FRR (3.1.1), “parallel adjacency suppression” (3.4).

[Les:] For netconf/PCEP references have been added.
There is no reference for “parallel adjacency suppression”. Some implementations do this and some don’t.

N5. Section 3: “IGP segments require extensions in link-state IGP protocols.  IGP extensions are required in order to advertise the IGP segments.”  These 2 sentences say the exact same thing.

[Les:] Text corrected.

N6. Please avoid using “we” in the text – except for the Acknowledgements.

[Les:] Done

N7. s/Regardless Segment Routing/Independent of Segment Routing

[Les:] Done

N8. s/and not to each other individual nodes in the LAN/and not to individual nodes in the LAN

[Les:] Text Corrected

N9. s/([I-D.ietf-spring-segment-routing-ldp-interop]/[I-D.ietf-spring-segment-routing-ldp-interop]

[Les:] I do not see what you are correcting here. ???

N10. “IGP deployed using areas”  An area is an OSPF-specific construct – flooding domain is more generic.

[Les:] Area is NOT specific to OSPF – IS-IS supports this as well. Text has been revised however – hopefully it suffices.

N11. It would be nice if the examples used IPv6 instead of IPv4.
[Les:] Hmmm…the Inter-area example does use IPv6, but the anycast example uses IPv4. Is this now forbidden?

   Les