Subject: [i2rs] AD review of draft-ietf-i2rs-rib-data-model-10

From: Alia Atlas <[email-masked]>
Date: Mon, 19 Feb 2018 18:30:48 -0500

As is expected, I have done my AD review
of draft-ietf-i2rs-rib-data-model-10. First I would like to thank the
authors - Amit, Lixing, Nitin, Mach, Hari, and Sri - as well as the many
folks who have reviewed and contributed over the years.

I do have some minor comments on details of what is modeled. These are
technical concerns that need to be resolved.  Having communicated the
urgency to Amit already today, I know that he is expecting to be a highly
responsive author during this period.  Therefore, I am requesting IETF Last
Call and placing this on the March 8 IESG telechat.

This document does an excellent job of standardizing the interface to the
RIB, which will have significant impact in the industry over time.

Minor:

1) TTL related comments:  First, the actions for ttl and hop-count should
be aligned. I know of no reason that IPv4 and IPv6 should be different in
this regard to tunnel interactions.  I expect this was just an oversight,
where the ttl-actions increased.  That said, I am dubious about the need
for either decrease-and-copy-to-inner or decrease-and-copy-to-next.   For
the latter,  it isn't typical to modify the label TTL inside during a label
swap. It is during a label pop and then a label push, but that is 2
different actions.  I do not see a need for this action.

identity decrease-and-copy-to-next {
     base "ttl-action";
     description
       "Decrease TTL by one and copy the TTL
        to the next header.For example: when
        MPLS label swapping, decrease the TTL
        of the inner label and copy it to the
        outer label.";
   }

2) It would be good to have informative references to GRE, VXLAN, and NVGRE
given in the descriptions.

 identity gre-tunnel {
     base "tunnel-type";
     description
       "GRE tunnel type";
   }

   identity vxlan-tunnel {
     base "tunnel-type";
     description
       "VxLAN tunnel type";
   }

   identity nvgre-tunnel {
     base "tunnel-type";

3) I don't understand the purpose of having an in-label here.  The match
condition is what determines  the in-label.  The label-swap just swaps
whatever is there to the specified out-label. There is no ability to do a
match at this point in the next-hop part - unless you are expecting to
compare the details in the RIB to the route's match-condition when the
next-hop is resolved?  But next-hop resolution isn't per route.  I really
think it just has to be removed - here and earlier in the draft.

      case label-swap {
           container label-swap {
             description
               "Label swap operation.";
             leaf in-label {
               type uint32;
               mandatory true;
               description
                 "The label to be swapped.";
             }
             leaf out-label {
               type uint32;
               mandatory true;
               description
                 "The out MPLS label.";
             }

4) In the grouping ipv6-header, I see that the next-header field is
specified.  It seems to me that this would depend upon what the match
condition and type of packet was.  One could specify separate next-hops for
each different type of match, of course, but specifying the next-header
field instead of having the router figure it out seems very fragile.  Is
there a strong reason to leave it in?

   leaf next-header {
       type uint8;
       mandatory true;
       description
         "The next header of the IPv6 header.";
     }

5)  "leaf route-preference { type uint32;"  The description should limit
the range of values; as mentioned in the introduction.

6) lookup-limit :  can you explain more in the description what this is
for?  When I see "lookup", I think about the match condition.  Is this a
limit around the levels for resolving next-hops? Is this limiting the
recursion depth on the next-hops?  Is this something else?

Nits:

a) Sec 2.4:  typo "Figure 5 (as shown blow)"  - blow should be "below"

b) typo "Reolved nexthop state" should be "Resolved nexthop state"

Regards,
Alia