Re: [spring] WGLC - draft-ietf-spring-srv6-network-programming

["Pablo Camarillo (pcamaril)" <[e-mail]>]

Hi Adrian,

I have reviewed your suggested changes and incorporated them in the latest revision of the document. 
Please see below inline for more detail.

Many thanks,
Pablo.

-----Original Message-----
From: Adrian Farrel <[e-mail]>
Organisation: Old Dog Consulting
Reply to: "[e-mail]" <[e-mail]>
Date: Wednesday, 11 December 2019 at 23:10
To: "Pablo Camarillo (pcamaril)" <[e-mail]>, "[e-mail]" <[e-mail]>, 'SPRING WG List' <[e-mail]>
Cc: 'draft-ietf-spring-srv6-network-programming' <[e-mail]>
Subject: RE: WGLC - draft-ietf-spring-srv6-network-programming

    Hi Pablo,
    
    Very good progress. Thanks for the work.
    
    I have snipped down to the conversation points below.
    
    The document has gone through a lot of changes in -06 and clearly there is a -07 in the pipe. I'll wait for that before doing a re-read.
    
    Best,
    Adrian
    
    [snip]
    
    >> 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.
    >
    > PC: Moved into 2.1.
    > Just for my knowledge, why should it be moved into 2.1 and not into 1.1?
     
    1.1 would be acceptable, but since 2. Is all about terminology and stuff, it sits there quite well.
    
    [snip]
    
    >> 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.
    >> 
    > PC: The SRH draft is defining the different types of nodes accurately. 
    > (Transit node, Source SR node, SR Endpoint).
    
    I have no issue with the definition of nodes (other than the way the concept is used in section 5).
    
    > This draft has
    >
    > Section1:
    >   Familiarity with the Segment Routing Header
    >   [I-D.ietf-6man-segment-routing-header] is assumed.
    
    Sure.
    
    > Section 5:
    >   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].
    >
    > Don’t you think that is enough?
    
    Well, obviously I found it unclear. In fact, section 4 had (at the time of my review) "Behaviors Associated with a SID" and I think I struggled with how "associated with" was different to "bound to".
    
    But now you have renamed section 4, this helps.
    
    Nevertheless, in Section 2 you have:
       SRv6 segment behavior: A packet processing behavior executed at an
       SRv6 segment endpoint.  Section 4 of this document defines behaviors
       related to traffic-engineering and overlay use-cases.  Other
       behaviors (e.g. service programming) are outside the scope of this
       document.
    
    This must have confused me, too. Looking at it now, it seems to constrain "behaviour" to the SR Endpoint Behaviors of section 4, meaning that section 5 comes as a "surprise". Indeed, section 2 seems to say that section 5 is out of scope of this document :-Z
    
    All I am asking for is some text early in the document that sets the scene. What is a behaviour? What classes of behaviour are we considering (end, transit, ...)? Who decides what behaviour to apply to what SID at which node? Not looking for pages of text. Probably you could do it in 5 lines.

PC: After reviewing all your comments on this topic: I have removed the previously known as "Transit" behavior.  Section 5 now is only about SR Policy Headend behaviors. All sub-sections have been renamed as well. I believe this helps the document.
    
    [snip]
     
    > 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?
    >
    > PC: Is there any difference on the hop limit check if I do it two lines earlier? 
    > I believe that the end result is the same.
     
    It makes a difference which ICMP error is sent. I would argue that you shouldn't be looking at the SRH of a packet when the Hop Limit has expired.

PC: I've checked with the SRH draft and the ICMP check is the last check happening in the pseudocode. I would tend to agree that this should be the last thing done once you have determined that the packet needs to be forwarded. 
    
    [snip]
     
    >> 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?
    > 
    > PC: How do you propose to check whether Last Entry value exceeds
    > the maximum possible theoretical Last Entry value?
    > The only information you have is the Header Extension Length. Based
    > on this I compute the maximum theoretical Last Entry possible value,
    > and then just compare the values.
    >
    > Maybe I am the one who have missed something. If you have a simpler
    > proposal please let me know.
    
    OK, we agree that the right thing to do is work out the largest possible Last Entry value (max_LE) and compare it to the actual Last Entry value. That is good.
    
    I also agree that all you have in hand is the Header Extension Length (Hdr Ext Len)
    
    I just don't think that you are computing the right value for max_LE using max_LE = (Hdr Ext Len / 2) - 1.
    
    I would agree that max_LE < (Hdr Ext Len / 2) - 1  so that comparing Last Entry with that value provides a check. I just don't think it is a very precise check.
    
    Perhaps my understanding of the actual value of Hdr Ext Len is incorrect, but if I am right, then you could probably get closer to a good number.
   
PC: Ok. I rechecked it and I believe that we cannot get any better than that.
[RFC8200]
Hdr Ext Len         8-bit unsigned integer.  Length of the Routing
                              header in 8-octet units, not including the
                              first 8 octets.
[draft-ietf-6man-segment-routing-header-26]
o  Last Entry: contains the index (zero based), in the Segment List,
   of the last element of the Segment List.

Let us consider an SRH containing k SIDs and 0 or more TLVs.
Hdr Ext Len >= k x (16 / 8) = k x 2
Last Entry = k - 1

Isolating k on one side of each equation:
k <= Hdr Ext Len / 2
k = Last Entry + 1

Combining the two equations together:
Last Entry + 1 <= Hdr Ext Len / 2

Isolating Last Entry on one side of the equation:
Last Entry <= Hdr Ext Len / 2 - 1

Further note that, if no TLV is present, we have:
Last Entry = Hdr Ext Len / 2 - 1

PC: If I'm still missing the point let me know!

    [snip] 
     
    >> 4.12
    >> 
    >> 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.
    >
    > PC: On a per deployment basis the operator decides which value of ARG represent “No arguments”.
    
    Oh, well, I understand that the ARG bits apply to a function that is (presumably) known across the network, so I suppose that what you are saying is that when an operator deploys a function that may take arguments she must configure all participating nodes to have the same understanding of those arguments and how to set the bits when no arguments are supplied (in the case where arguments were optional for the function).
    
    Now, it would be good to make some of that clear (perhaps in the last paragraph of 3.1).
    
    But, we are in the business of making interoperable implementations, so this need to configure this feature needs to be spelled out.

PC: Added a paragraph about it at the end of 3.1.
    
    >> 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.
    > 
    > This makes [I-D.ietf-spring-segment-routing-policy] a normative
    > reference.
    >
    > PC: I disagree. Example RFC8660 (Segment Routing with the
    > MPLS Data Plane).
     
    It is a very different usage made in that document.
    Here you are saying that an implementation of this document must do something related to [I-D.ietf-spring-segment-routing-policy].
    To me, that means that you can't implement this document without reading [I-D.ietf-spring-segment-routing-policy]

PC: All the definitions required by this paragraph are covered in RFC8402 section 5. I have removed the reference to draft ietf-spring-segment-routing-policy from the draft.
    
    [snip]
    
    >> 4.16.1 and 4.16.2
    [snip] 
    >> 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".
    > 
    > PC: This is one use-case indeed.  I don’t follow you in the notion of
    > “Pop and Go”.
    
    An MPLS concept associated with determining the forwarding behaviour, stripping the label, and forwarding.
    
    [snip]
    
    >> 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".
    >
    > PC: If the SRH or anything below is not being processed, there is no
    > performance impact, no? 
     
    Oh, you meant "speed of throughput" not "treated identically". The use of "treats" confuses the sentence.
    
    OK. That comes under "sharpen your meaning of performance".
    
    Perhaps:
       This means that N forwards the following two packets P1 and P2 at the
       same rate:
    
    ?

PC: Text removed  (entire section) as per my comment above. 
    

    [snip]
     
    >> 5.2
    >> 
    >> By definition this behavior only applies at source nodes per 8402. In
    >> other words, you are describing domain recursion according to 8402.
    >
    > PC: Not sure what resolution you are looking for here.
     
    I'm not sure either 😊
    
    I think I am wondering how you can have "Transit with Encapsulation" since "Transit" means in the middle of the network (at a transit node) and "Encapsulation" means at a source node.
    If a transit node is doing encapsulation, then you are recursing (which is OK).
    
    When I waffled about behaviors before, it seemed to me that you have source, transit, and end behaviors. What you are describing in this section is a source behaviour. The fact that the source is encapsulating a packet that is already SRv6, is not so interesting as the fact that it is encapsulating a packet.
    
    But then see also 5.3 and this discussion below.
    
    >> 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.
    >
    > PC: 
    > There are two types of source nodes. 
    > You can have a source node that is doing plain IP transit. You have a
    > local policy to encapsulate all packets matching X to be steered into
    > a segment routing policy with behavior T.Encaps.
    > The other type of source node is the SR endpoints that instantiate a
    > BindingSID. This is also an Source node that steers all packets received
    > with IPv6 DA = BSID.
    > Section 5 is describing transit behaviors. 
    
    Confused me here. You say that these source nodes are transit nodes? That makes the definition of the two node types very unclear.
    
    Just because the node is not a host does not make it a transit node. We have a good and clear definition of a transit node as a node that receives and sends an SRv6 packet. Nodes that add an SRH are source nodes. Nodes that remove the SRH are end nodes.
     
    But in 5.4 the thing that is encapsulated is an L2 frame. So not plain IP transit, and not a packet received with a BSID in the DA.
    At least, the text in 5.4 appears to say that.
    So really, is this transit behaviour?

PC: See comment above about the entire section 5.
    
     
    >> 6.1
    >> 
    >> I agree with this section, but I am concerned that you are updating 
    >> draft-ietf-6man-segment-routing-header by 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.
    >
    > PC: I disagree that this is an update to the SRH. The SRH defines a
    > header and one type of segment processing.
    > Defining a counter is not related to the SRH at all.
     
    Well, perhaps you didn't just say what you meant to say because the text I quoted most clearly says that the counter is counting events related to the SRH.
    
    But to the point: the document says...
       Any SRv6 capable node SHOULD implement the following set of combined
       counters (packets and bytes):
    "Any SRv6 capable node" is clearly in the scope of draft-ietf-6man-segment-routing-header

PC: This document talks of the processing and behaviours of SRv6. And hence the counters are included in this draft.

    >> 6.3
    >> 
    >> I think you have made [I-D.ietf-6man-spring-srv6-oam] a normative
    >> reference.
    > 
    > PC: Why? Its defining new behaviors for the purpose of OAM, but
    > those are not required in NET-PGM.
    
    You're right. Section 6.3 doesn't actually say anything.
    
    [snip]
     
    >> 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.
    >
    > PC: This comment has also been raised by Bruno (as individual). I
    > agree and I have proposed to Bruno this:
    >   Experimental and Private use deleted
    >   Specification Required changed to FCFS as per Bruno’s suggestion
    >   New block in the previous space of experimental and private that
    >   is reserved for future use of IETF.
    
    That works.
    The correct language for the last block is "Reserved. Not to be allocated."
    (Note for you: a future standards track document can change the allocation policy for that block meaning it can then be used.)

PC: Thanks for the correction. Fixed. Thanks.