Hi,
Thanks to the authors for the work on this draft. I've done a review which is the first time I have looked at the draft for several revisions. My thoughts are included below. I think that considerable editorial work is needed before we can claim that this document is ready for publication. Thanks, Adrian === It would be really good if authors ran idnits before presenting drafts as ready for WG last call. It is pretty easy to do, and fixing the issues is not at all hard. https://www6.ietf.org/tools/idnits?url=https://www.ietf.org/archive/id/draft -ietf-spring-srv6-network-programming-05.txt I think the most obvious thing that the tool points out is the absence of a Security Considerations section. While section 7 is obviously relevant, there would seem to be a significant piece of work that is needed before the draft can progress. --- Another 'normal' thing to do is check that the draft follows the editorial guidelines from the RFC Editor. Most notable is that the Requirements Language text should be moved into section 2.1. --- It would be good if you could put all section headers into title case. --- There are a lot of abbreviations in the document that are not deemed well-known by the RFC Editor and should be expanded on first use. 'SRv6' is not (yet) a well-known abbreviation according to the RFC Editor, so should be spelled out in the title, abstract, and on first use in the main body of text. --- The Abstract is a little terse. In particular, it should give an overview of what 'network programming' is. The Introduction says: This document defines the SRv6 Network Programming concept and aims at standardizing the main segment routing behaviors to enable the creation of interoperable overlays with underlay optimization and service programming. Some of that text would be good in the Abstract. -- Section 1 The first mention of 'Segment Routing' needs a reference. --- Section 1 moving forward in the segment list to any complex user-defined behavior. Is there a comma missing? --- Section 1 s/The network programming consists/Network programming consists/ --- Section 1 This document defines the SRv6 Network Programming concept and aims at standardizing the main segment routing behaviors to enable the creation of interoperable overlays with underlay optimization and service programming. The document 'aims', but does it succeed? :-) I think you should use... s/aims at standardizing/specifies/ --- Section 2 We assume that the SRH may be present multiple times inside each packet. It is good that you state this assumption for this draft. But clearly this is not normative for defining whether the SRH is allowed to be present more than once. I went to draft-ietf-6man-segment-routing-header to try to find a definitive statement and could not find one (perhaps I missed it?). Failing that, 8200 is normative and it says: Each extension header should occur at most once, except for the Destination Options header, which should occur at most twice (once before a Routing header and once before the upper-layer header). That means that your assumption: - is necessary for this document to work - is not a valid assumption. Clearly that is not good and not what you want to end up with. I think the solution is a fix to draft-ietf-6man-segment-routing-header either by pulling it back from the RFC Editor Queue to make and agree the change, or by writing a small bis to that draft to define and allow multiple presence. (That bis would be a normative reference from this draft.) I suppose it could be possible to have this draft update draft-ietf- 6man-segment-routing-header, but that is probably not ideal and would cause some inter-WG tensions. One way of the other, it's an easy fix, but the bottom line of this issue as things stand is that the assumption is false and blocks this draft. --- Section 2 When a packet is intercepted on a wire, it is possible that SRH[SL] is different from the DA. It is fairly obvious what this means, but you have introduced notation without definition. Perhaps insert some to text so that you end up with: SRH[n] indicates the nth one-based entry in the SID list as represented in the SRH. Thus, SRH[1] is SR3 in the example above. When a packet is intercepted on a wire, it is possible that SRH[SL] is different from the DA. --- Section 3 As introduced in RFC8402 an SRv6 Segment Identifier is a 128-bit value. Of course, it is true that an SRv6 SID is 128-bits, and this follows from what is said in 8402, but it is not what 8402 says. I suggest you replace this text with: RFC8402 defines an SRv6 Segment Identifier as an IPv6 address explicitly associated with the segment. Thus, an SRv6 Segment Identifier is a 128-bit value. --- 3.1 The FUNCT value zero is invalid. That's reasonable, but I couldn't find out what happens if I set a zero FUNCT. Where is that described? (Pedantically, but I don't expect a fix, if L is 128, I think FUNCT zero may be OK.) --- 3.1 A behavior may require additional arguments that would be placed immediately after the FUNCT. In such case, the SRv6 SID will have the form LOC:FUNCT:ARGS::. For this reason, the SRv6 SIDs are matched on a per longest-prefix-match basis. This is clear, but it conflicts with the definition of FUNCT given earlier: FUNCT (function) is the 128-L least significant bits of the SID. I would suggest that you reverse some of the text so that you start with LOC:FUNCT:ARGS:: then describe how the split between LOC and FUNCT is found (i.e., L), and then explain that ARGS might not always be needed in which case FUNCT uses all of the 128-L bits. --- I think you could usefully spend some time describing the classes of behaviors. Notably "End behaviors" and "Transit behaviors". This would go as a new section before section 4. You can describe how/where in the network the behaviors are executed, and you can describe whether the behaviors are bound to specific SIDs (SID types) or not. The, probably, rename section 4 to "End behaviors" which keeps it aligned with the name of section 5. --- 4. This is the only place you use "Adj SID" in the document. Either spell it out, or mention it in Section 2. Please spell out "decaps" on each use. --- 4.1 As a consequence, an End SID cannot be the last SID of a SID list and cannot be the DA of a packet without an SRH (unless combined with the PSP, USP or USD flavors Section 4.16). I think you mean s/cannot/MUST NOT/ The point is that I *can* place anything in a SID list, I just can't expect it to work. In fact, you should catch this in the pseudo code by testing Segments Left > 1. --- 4.1 "TBD-SRH" is mentioned many times in this document with the first time being in this section. I don't find this described in Section 9. --- 4.1 S03. Send an ICMP Parameter Problem message to the Source Address Code TBD-SRH (SR Upper-layer Header Error), Pointer set to the offset of the upper-layer header, interrupt packet processing and discard the packet Ambiguous punctuation. Probably... S03. Send an ICMP Parameter Problem message to the Source Address Code TBD-SRH (SR Upper-layer Header Error), Pointer set to the offset of the upper-layer header. Interrupt packet processing and discard the packet Similarly: S06. Send an ICMP Time Exceeded message to the Source Address, Code 0 (Hop limit exceeded in transit). Interrupt packet processing and discard the packet And: S10. Send an ICMP Parameter Problem to the Source Address, Code 0 (Erroneous header field encountered), Pointer set to the Segments Left field. Interrupt packet processing and discard the packet There are examples of this all through Section 4. --- 4.1 I was surprised by... S01. When an SRH is processed { S02. If (Segments Left == 0) { S03. Send an ICMP Parameter Problem message to the Source Address Code TBD-SRH (SR Upper-layer Header Error), Pointer set to the offset of the upper-layer header, interrupt packet processing and discard the packet S04. } S05. If (IPv6 Hop Limit <= 1) { S06. Send an ICMP Time Exceeded message to the Source Address, Code 0 (Hop limit exceeded in transit), interrupt packet processing and discard the packet S07. } Are you sure that Hop Limit processing is left so late? Isn't it one of the first things done with a packet before even the SRH is discovered to be present? --- 4.1 (and other sections) I spent rather too much time trying to work out S08. max_LE = (Hdr Ext Len / 2) - 1 S09. If ((Last Entry > max_LE) or (Segments Left > Last Entry+1)) { As far as I can see, for an SRH, Hdr Ext Len = 7 + (16 * (Last Entry + 1) ) + sum(all TLV lengths) So, while I see that you want to check that Last Entry doesn't point out beyond the end of the SRH, I don't see how your computation of max_LE helps. What have I missed? --- 4.2 s/with a set of J of/with a set, J, of/ Maybe S15. Set the packet's egress adjacency to J would read better as S15. Set the packet's egress adjacency to a member of J --- 4.2 Note that the End.X SID cannot be the last SID of a SID list and cannot be the DA of a packet without an SRH (unless combined with the PSP, USP or USD flavors Section 4.16). Hence the Upper-layer header should never be processed. Similar cannot/MUST NOT issue as in 4.1. The pseudocode fix in 4.1 would cover this point. --- 4.3 The End.T behavior is used for multi-table operation in the core. For this reason, an instance of the End.T behavior must be associated with an IPv6 FIB table T. Is this "MUST BE" or "is"? Error code "SR Upper-layer Header Error", Pointer set to the offset of the upper-layer header. You should call out "TBD-SRH" as the error code value. (Happens in several sections.) And (again)... Note that the End.T SID cannot be the last SID of a SID list and cannot be the DA of a packet without an SRH (unless combined with the PSP, USP or USD flavors Section 4.16). Hence the Upper-layer header should never be processed. Similar cannot/MUST NOT issue as in 4.1. The pseudocode fix in 4.1 would cover this point. --- 4.4 The End.DX6 SID must be the last segment in a SR Policy, and it must be associated with one or more L3 IPv6 adjacencies J. Again: is that "MUST" or "is"? Fix to S03. Send an ICMP Parameter Problem to the Source Address, Code 0 (Erroneous header field encountered), Pointer set to the Segments Left field. Interrupt packet processing and discard the packet and S02. Send an ICMP Parameter Problem message to the Source Address Code TBD-SRH (SR Upper-layer Header Error), Pointer set to the offset of the upper-layer header. Interrupt packet processing and discard the packet I wondered why you had two code fragments rather than a longer one with an "If" statement and two branches. Doesn't really matter, however, as a result of the current structure, the Notes that follow are a little ambiguous because you have two instances of S01 and S05 in the section. --- 4.5 Identical comments to 4.4 --- 4.6 This would be equivalent to the per-VRF VPN label in MPLS [RFC4364]. s/would be/is/ The End.DT6 SID must be the last segment in a SR Policy, and a SID instance must be associated with an IPv6 FIB table T. Again: is that "MUST" or "is"? --- 4.7 and 4.8 Identical comments to 4.6 --- 4.9 The End.DX2 SID must be the last segment in a SR Policy, and it must be associated with one outgoing interface I. Again: is that "MUST" or "is"? This section brings back the ambiguity in the Notes. --- 4.10 Any SID instance of the End.DX2V behavior must be associated with an L2 Table T. Again: is that "MUST" or "is"? --- 4.11 s/unicast . Any/unicast. Any/ Any SID instance of the End.DT2U behavior must be associated with an L2 Table T. Again: is that "MUST" or "is"? Usual issue with S02. Send an ICMP Parameter Problem message to the Source Address Code TBD-SRH (SR Upper-layer Header Error), Pointer set to the offset of the upper-layer header, interrupt packet processing and discard the packet L2OIF is a new (unknown) term (in 4.12 you use "L2 OIF") s/control plane/the control plane/ --- 4.12 Any SID instance of this behavior must be associated with a L2 table T. Additionally the behavior may take an argument: "Arg.FE2". It is an argument specific to EVPN ESI filtering and EVPN-ETREE used to exclude specific OIF (or set of OIFs) from L2 table T flooding. Again: is that "MUST" or "is"? Possibly s/may/MAY/ Where arguments are optional, I don't think you have made it clear what you do if no argument is supplied. I would presume you set the ARG bits to zero, but I am not clear whether zero could be interpreted as a valid argument. S06. Forward via all L2 OIFs excluding the one specified in Arg.F2 Earlier you called it Arg.FE2 --- 4.13 An End.B6.Encaps SID is never the last segment in a SID list. Any SID instantiation must be associated with an SR Policy B[I-D.ietf-spring-segment-routing-policy] and a source address A. Again: is that "MUST" or "is"? s/B[I-D/B [I-D/ This makes [I-D.ietf-spring-segment-routing-policy] a normative reference. --- 4.13 Step 13 is S13. Decrement Segments Left by 1 and the associated note is S13. The SRH MAY be omitted when the SRv6 Policy B only contains one Should the note be for S14? Similarly, should the S16 note be for S17? --- 4.14 In this way, the first segment is only introduced in the DA and the packet is forwarded according to it. I believe this sentence could be written more clearly. I tried to offer a suggestion, but discovered that I couldn't reliably guess what the intention is. --- 4.15 An End.BM SID is never the last SID, and any SID instantiation must be associated with an SR-MPLS Policy B[I-D.ietf-spring-segment-routing-policy]. Again: is that "MUST" or "is"? s/B[I-D/B [I-D/ --- 4.15 S14. Push a the MPLS label stack for B s/a the/the/ --- 4.16.1 and 4.16.2 The term "pop the SRH" is, I think, a new concept introduced at this point. We can deduce what it means, but not be certain that we will do the right thing. You need to add text to clearly define the meaning. Furthermore (and I'm willing to bet someone ese has mentioned this!) the PSP concept appears to move the ability to remove an SRH from the domain border to inside the domain. I wonder whether that is actually allowed. I also wonder how the rest of the network knows whether the PSP function is available at a specific node. You might want to recast PSP if its purpose is to handle "end" nodes that are not SRH capable. To do this you would recast the node that you have called the PSPS node as the edge of the domain (the end node) and set the action as "Pop and go". --- 5.1 As per [RFC8200], if a node N receives a packet (A, S2)(S3, S2, S1; SL=1) and S2 is neither a local address nor a local SID of N then N forwards the packet without inspecting the SRH. 8200 can't possibly say this because 8200 doesn't know about SIDs. Possibly you just want to change the reference to 8402. --- 5.1 This means that N treats the following two packets P1 and P2 with the same performance: P1 = (A, S2) P2 = (A, S2)(S3, S2, S1; SL=1) I don't think this is necessarily true since packet length and other things (e.g., hashing) may vary. So you either need to sharpen your meaning of "performance" or just content yourself with "MUST NOT examine the SRH". --- 5.1 A transit node MUST include the outer flow label in its ECMP load- balancing hash [RFC6437]. I find this odd in this document. You are describing the behavior at a node that is potentially a legacy node that is not SRH aware, yet you are using normative language to define its behavior. You could change this to: A transit node will use the outer flow label in its ECMP load- balancing hash as described in [RFC6437]. Or, unless you can give a good reason to include it, you can simply strike this paragraph. Or, possibly, the problem comes with the text in Section 5 that says: We define hereafter the set of basic transit behaviors. These behaviors are not bound to a SID and they correspond to source SR nodes or transit nodes [I-D.ietf-6man-segment-routing-header]. Perhaps it is not right that you could define a transit behavior for a "transit node" that is not capable of processing an SRH. If you were to fix the definition of a transit behavior to apply to "source SR nodes, or nodes that are capable of processing SRHs and that forward an IPv6 packet where the destination address of that packet is not locally configured as a segment nor a local interface." then that would fix my concern. There is a similar issue in 6.2 --- 5.2 By definition this behavior only applies at source nodes per 8402. In other words, you are describing domain recursion according to 8402. --- 5.2 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. For complete clarity... 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. --- 5.3 Ditto both comments for 5.2 --- I don't think 5.4 or 5.5 can happen at transit nodes. The thing to be encapsulated is an L2 frame and so is only seen at a source node. At this point, I think only 5.1 describes a transit behavior. 5.2 to 5.5 describe source behaviors. --- 6.1 I agree with this section, but I am concerned that you are updating draft-ietf-6man-segment-routing-headerby adding normative language to describe nodes that process the SRH. Does that mean you should use an "Updates" clause in the document header? After describing CNT-1 and CNt-2 you say... Furthermore, an SRv6 capable node maintains an aggregate counter CNT-3 tracking the IPv6 traffic that was received with a destination address matching a local interface address that is not a locally instantiated SID and the next-header is SRH with SL>0. That looks like s/maintains/MUST maintain/ unless you have a reference for the assertion that it does maintain CNT-3. --- 6.3 I think you have made [I-D.ietf-6man-spring-srv6-oam] a normative reference. --- 7. The SRH Section 5.1 defines how a domain of trust can operate SRv6-based services for internal traffic while preventing any external traffic from accessing the internal SRv6-based services. Probably "The SRH Section 5.1" is meant to be a reference to another document. --- Section 8 has... The concept of "SRv6 network programming" refers to the capability for an application to encode any complex program as a set of individual functions distributed through the network. Some functions relate to underlay SLA, others to overlay/tenant, others to complex applications residing in VM and containers. This looks like a useful paragraph to have at the top of the document. --- 8.1 The End, End.T and End.X SIDs express topological behaviors and hence are expected to be signaled in the IGP together with the flavors PSP, USP and USD[I-D.ietf-lsr-isis-srv6-extensions]. I think you are conflating SIDs with behaviors. Probably... The End, End.T, and End.X behaviors express topological behaviors and hence the SIDs that have these behaviors bound to them are expected to be signaled in the IGP together with the supported flavors (PSP, USP, or USD) [I-D.ietf-lsr-isis-srv6-extensions]. Likewise... OLD These three SIDs provide important topological behaviors for the IGP to build FRR/TI-LFA solution and for TE processes relying on IGP LSDB to build SR policies. NEW These SIDs provide important topological behaviors for the IGP to build FRR/TI-LFA solution and for TE processes relying on IGP LSDB to build SR policies. --- 8.2 BGP-LS is expected to be the key service discovery protocol. Every node is expected to advertise via BGP-LS its SRv6 capabilities (e.g. how many SIDs it can insert as part of a T.Encaps behavior) and any locally instantiated SID [I-D.ietf-idr-bgpls-srv6-ext]. We shouldn't write text that expresses expectations about the future. It doesn't age well in an RFC, and it is just unsubstantiated speculation. What about... [I-D.ietf-idr-bgpls-srv6-ext] describes a mechanism to enable each node to use BGP-LS to advertise its SRv6 capabilities (e.g., how many SIDs it can insert as part of a T.Encaps behavior) and any locally instantiated SIDs. This enables BGP-LS to be the key service discovery protocol. --- 8.3 OLD The End.DX4, End.DX6, End.DT4, End.DT6, End.DT46, End.DX2, End.DX2V, End.DT2U and End.DT2M SIDs are expected to be signaled in BGP [I-D.ietf-bess-srv6-services]. NEW The End.DX4, End.DX6, End.DT4, End.DT6, End.DT46, End.DX2, End.DX2V, End.DT2U and End.DT2M SIDs can be signaled in BGP [I-D.ietf-bess-srv6-services]. --- 8.4 OLD The following table summarizes which SIDs are signaled in which signaling protocol. END The following table summarizes which behaviors can signaled for SIDs in which signaling protocols. --- 8.4 The reader should also remember that any specific instantiated SR policy is always assigned a Binding SID. They should remember that BSIDs are advertised in BGP-LS as shown in Table 1. This needs a citation. --- 9. | 0 | 0x0000 | Reserved | Invalid | | 1-32767 | 0x0001-0x7FFF | Specification Required | | | 32768-49151 | 0x8000-0xBFFF | Reserved for experimental | | | | | use | | | 49152-65534 | 0xC000-0xFFFE | Reserved for private use | | | 65535 | 0xFFFF | Reserved | Opaque | Should use the 5226 names for the assignment policies. Hence: | 0 | 0x0000 | Reserved | Invalid | | 1-32767 | 0x0001-0x7FFF | Specification Required | | | 32768-49151 | 0x8000-0xBFFF | Experimental Use | | | 49152-65534 | 0xC000-0xFFFE | Private use | | | 65535 | 0xFFFF | Reserved | Opaque | --- 9. I am strongly opposed to such a large range being assigned for experimental use. RFC 3692 gives some guidance. The normal idea is that there should be enough codepoints to allow two experiments to be run on the same nodes without conflict, but few enough that they cannot be used as a proxy for allocating and registering values in the registry. I should think that 32 code points is enough. --- 9. According to 5226 "Specification Required" implies the use of a Designated Expert. That means that your document needs to give advice to the DE on how to make decisions about which code points are acceptable. --- 9. Would be nice to reformat Table 4 to avoid the line wrapping. --- 9. In table 4 you have... | 13 | 0x00 | End.B6.Insert | [I-D.filsfils-spring-srv6-net-pgm- | | | 0D | | insertion] | I think you should leave 13 as unassigned, and leave it to the other document to request assignment. Alternatively, you make draft-filsfils-spring-srv6-net-pgm-insertion a normative reference and block the progress of this draft until it is complete (which I don't think it is a very good idea). --- 12.2 As shown by idnits, you have a couple of unused references you should delete. --- 12.2 2473, 8200, and 8402 are all clearly used as normative references. Please move them to 12.1 From: ipv6 <ipv6-boun...@ietf.org> On Behalf Of bruno.decra...@orange.com Sent: 05 December 2019 17:15 To: 'SPRING WG List' <spring@ietf.org> Cc: 6...@ietf.org; draft-ietf-spring-srv6-network-programming <draft-ietf-spring-srv6-network-programm...@ietf.org> Subject: WGLC - draft-ietf-spring-srv6-network-programming Hello SPRING, This email starts a two weeks Working Group Last Call on draft-ietf-spring-srv6-network-programming [1]. Please read this document if you haven't read the most recent version, and send your comments to the SPRING WG list, no later than December 20. You may copy the 6MAN WG for IPv6 related comment, but consider not duplicating emails on the 6MAN mailing list for the comments which are only spring specifics. If you are raising a point which you expect will be specifically debated on the mailing list, consider using a specific email/thread for this point. This may help avoiding that the thread become specific to this point and that other points get forgotten (or that the thread get converted into parallel independent discussions) Thank you, Bruno [1] https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming-05 ____________________________________________________________________________ _____________________________________________ 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.
_______________________________________________ spring mailing list spring@ietf.org https://www.ietf.org/mailman/listinfo/spring
