Hello,

I have been selected as the Routing Directorate QA reviewer for this draft. My review is below.

Regards,

--John


Document: draft-ietf-i2rs-rib-data-model-05 
Reviewer: John Scudder 
Review Date: June 3, 2016 


Summary: 

The document is reasonably understandable, although I did find myself with a fair number of questions, which I've written below. As noted below, many of these questions may only have arisen because I am not skilled in the subject area. I did find one major issue, with the Security Considerations section, which will certainly need to be resolved somehow before the document is progressed.


Comments:

In preparing my review, I reviewed the draft itself, but did not carefully read all the references. As a result, it's possible some of my observations might be in the rough, when taken in context of the full body of work. It's also the case that I am not an experienced Yang practitioner, so some practices and idioms that may be well known could easily have escaped me, and a similar disclaimer applies.

Finally, I did not review Section 3, YANG Modules, in detail, although I made a few suggestions.


Major Issues:

1. There are two problems with the Security Considerations section. The first is that the section isn't good (sorry). More on this below. The second is that it references the i2rs-architecture document normatively, but that document is listed under Informative References. The second problem is easily fixed, of course, by simply moving the architecture document to Normative References.

 The security section, in its entirety, is:

   This document introduces no extra new security threat and SHOULD
   follow the security requirements as stated in
   [I-D.ietf-i2rs-architecture].

However, if I go look at the architecture document to see what "the security requirements" are that I am supposed to follow, I find a fairly long Security Considerations section which states nothing that formally appears to be a requirement. It also says that instantiations of i2rs will provide more detailed analysis of security properties. Since this document doesn't do that, there must be some other i2rs document that does, right? And this document should reference that one?

It seems perfectly reasonable to state that this document doesn't introduce any security considerations of its own, and to reference some foundational document. However, the current security section doesn't do that effectively, doesn't reference the correct document, and I wouldn't expect it to survive a Security Directorate review. Suggesting a rewrite is beyond the scope of this QA review, sorry, but one is needed. I don't think it necessarily needs to be very long, but it does need to correct these issues.


Minor Issues:

1. The author list of six people exceeds the current RFC Editor guidance of five or fewer named authors. See RFC 7322:

  The total number of authors or editors on the first page is generally
  limited to five individuals and their affiliations.  If there is a
  request for more than five authors, the stream-approving body needs
  to consider if one or two editors should have primary responsibility
  for this document, with the other individuals listed in the
  Contributors or Acknowledgements section.  

2. A number of symbols are not defined in Section 1.2, Tree Diagrams. I noticed the following as needing to be defined: { } x w n.

3. Possibly this is covered in a companion document or is well-known to those better versed in Yang than me, but I found myself wondering what the default values for optional items were. In particular, every writable boolean is optional. Many have no default value, for example sharing-flag has none. But return-failure-detail does have a default of false. Is it okay that some of these optional items have no default values? Is the expected behavior implementation-specific in that case?

4. "route-statistic" should probably be "route-statistics" (plural). Then again, looking at the content of "route-statistic", it's not what I would refer to as statistics at all. The content is state (active or inactive), installation state (installed, or not) and reason (I guess this indicates the reason the route entered its current state, see also comment 11 below). None of this seems like statistical data to me. Maybe "status"?

5.  The model doesn't seem to capture any restrictions on how nexthops can be chained. Presumably there are some restrictions, for example it wouldn't seem to make a whole lot of sense to chain egress-interface and egress-interface-ipv4-nexthop together, the more so since the respective outgoing-interfaces might conflict. Possibly capturing this is outside the scope of this document.

6. In various parts of the model, it appears to be possible to reference an object either by an opaque identifier or by value. Maybe this is a common idiom, but it wasn't obvious to me why this should be, it seems redundant. For example, every route has a route-index, but when I want to operate on a route with route-update or route-delete, I have to identify it by rib name and prefix. The utility of the index, then, is not clear to me. Another example is in Section 2.5:

   o  nh-delete: Delete a nexthop from a rib.  A name
      of a rib and a nexthop or nexthop identifier are passed as the
      input parameters.

Again, it's unclear to me why it's desirable to be able to delete the NH by either reference or value.

7. It was unclear to me why a reason is required for a route change notification, but not for a next hop change notification.

8. This construct appears three times:

       leaf rib-family {
         type rib-family-def;
         mandatory true;
         description
           "A reference to address family of a rib.";
       }

However, in two locations the description is "The address family of a rib."  and in the third, the "reference to" language is used. Probably this is just a cut and paste error, but because a reference is different from the thing itself (presumably a value), maybe the choice of language really does indicate some subtle difference? Because I haven't exhaustively reviewed Section 3, I think it is likely more such inconsistencies exist, and I think it would be good to check for them.

9. "To download N
          nexthops to the FIB, the N nexthops with the lowest
          value are selected."

What if more than N nexthops are tied for having the lowest value? As written, this is underspecified.

10.          "Nhop-lb-weight is a number between 1 and 99.";

First, this doesn't use the correct name of the value it is describing. Second, it's not an adequate description. (It tells the reader nothing helpful.)

11.              "Indicate the route reason."

Again, this doesn't tell the reader anything helpful. Reason for what?



Nits:

I found a number of editorial nits. Rather than enumerating them here, I edited my suggested changes into a copy of https://www.ietf.org/id/draft-ietf-i2rs-rib-data-model-05.txt.  I have provided both that copy, and a diff against the original, in attachments. Please review my changes and don't just accept them blindly, while I intended that all my changes were strictly editorial there's always the chance I altered the meaning unintentionally.

In addition, I noticed some other issues where I'm not sure how they should be resolved:

1. The term "RIB" is used inconsistently throughout the document. In some places it's capitalized, as "RIB". In others, it's lowercase, as "rib", or even mixed-case, "Rib". I didn't change this in my marked up copy, for two reasons. First, global search and replace would not be straightforward, because the string also occurs where lowercase is clearly appropriate, for example "ietf-i2rs-rib" (there are 331 occurrences of RIB in any combination of upper and lowercase by my count). Second, I didn't completely discount the possibility of the authors are deliberately using the lowercase term sometimes. I recommend either the term should be uppercase ("RIB") throughout, or if a distinction between "RIB" and "rib" is intended, that should be explained in the Definitions and Acronyms section.

2. "there should be a limitation on how many levels of lookup can be practically performed." I suspect what the authors mean here is "there might be a limitation", meaning a practical limitation might exist (indeed, probably does exist) in the hardware implementing forwarding. I've suggested that change in my marked up copy. However, may be the authors really do mean that there needs to be a configurable limitation to allow restriction to something less than what the hardware implements. The fact that lookup-limit is a rw value  seems to support this – I don't see why that would be a configurable value if it represents an expression of what the hardware is capable of. On the other hand, the document is silent about what an implementation is supposed to do with that value once configured. (Maybe one of the companion documents explains?)

3. The rib-list under routing-instance is indexed by a field called "name". Under the notification hierarchy the corresponding field is called "rib-name".  it works as written, of course, but it caused a little dissonance for me. (See #4 below as well.)

4. The name "rib-family" wasn't self-explanatory for me. "rib-address-family" would have been. For that matter, why prefix "rib-" onto it since you haven't prefixed "rib-" onto the other variables (see #3 above as well).

5. Shouldn't there be an ellipsis right below rw-route-vendor-attributes in Figure 1?

6. In Section 2.1, capability negotiation is referred to twice, once in the first paragraph and once in the last. I think you probably mean capability advertisement. The distinction is that in advertisement, an entity (the router) is telling another entity what it's capabilities are. There is nothing to negotiate about per se, if the other entity doesn't like the router's capabilities, it can't very well convince the router to change them. I have changed "negotiation" to "advertisement" in my marked up copy accordingly, if that's not right you should revert it but also clarify.

7. "nexthop-lbs" is a strange choice of name for load-balancing. I would suggest either dropping the S and just making it "nexthop-lb", or spelling it out in full, "nexthop-load-balance". But something the casual reader is likely to see as "next hop pounds" ("lbs" is the common abbreviation for the English system unit of weight pounds, of course) seems problematic.

8. I couldn't understand this sentence at all: 

      *  failure-detail: shows the specific failed routes that failure
         reason.

    Since I didn't understand it, I can't suggest a rewrite, sorry. (The text recurs three times in section 2.5.)

9. nh-add in Section 2.5 talks about "the network node" doing something:

   o  nh-add: Add a nexthop to a rib.  A name of the
      rib and a nexthop are passed as the input parameters.  The network
      node is required to allocate a nexthop identifier to the nexthop.
      The outputs include the result of the nexthop add operation.

   As far as I can tell, the entire rest of the document talks about "the i2rs agent" doing something. Should "network node" the rewritten accordingly?

10. Similar to #9, "A RIB data-model MUST support sending 2 kind of asynchronous notifications"  Doesn't seem right. Surely the data model per se doesn't support sending anything at all, it's the i2rs agent that supports it? I've tentatively replaced the text with "An implementation of this RIB data model" (cribbed from lower down in the doc).