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

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

Hi Adrian,

Happy New Year.
Please see inline.

Many thanks for your comments,
Pablo.

-----Original Message-----
From: Adrian Farrel <[e-mail]>
Organisation: Old Dog Consulting
Reply to: "[e-mail]" <[e-mail]>
Date: Friday, 27 December 2019 at 20:29
To: "Pablo Camarillo (pcamaril)" <[e-mail]>
Cc: "[e-mail]" <[e-mail]>
Subject: RE: WGLC - draft-ietf-spring-srv6-network-programming

    Thanks Pablo and Happy Imminent New Year.
    
    > I have reviewed your suggested changes and incorporated them
    > in the latest revision of the document. 
    
    Great. I will try to pull that and review it soon.
    
    > Please see below inline for more detail.
    
    [snipped]
         
        >> 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. 
    
    AF: So, just to check I understand what you are saying. If a packet arrives with Hop Limit already at zero you would still look inside the SRH?

PC: As per RFC8200, if a packet arrives with Hop Limit equal to zero, you process the packet normally.
Then, when you attempt to forward it, if Hop Limit is zero, then you discard the packet.
This makes a difference when you are the last segment (like End.DX4).
      
        >> 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!
    
    AF: Very (very) sorry. My bad.
    I entirely missed " Length of the Routing header *in*8-octet*units*, not including the first 8 octets"
    Now I feel stupid.
    
        >> 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.
    
    AF: Last try before I give up on this one (you may be relieved to know).
    Does every device that supports SRv6 have to support this document? I had formed the impression not: that is a simple SRv6 routing node functions fine without this document (even if this document also describes it's behaviour). But, in the quoted text you are adding description of function for all "SRv6 capable nodes" that is not described elsewhere. So either:
    - it is not possible to have an SRv6 capable node prior to this draft
    or
    - you are updating some other document that describes SRv6 capable nodes

PC: I think we can address this by changing "Any SRv6 capable node" by "A node supporting this document ...".
    
    Many thanks for the work,
    Adrian