Subject: [netmod] mbj review of draft-ietf-netmod-routing-cfg-04

From: Martin Bjorklund <[email-masked]>
Date: Mon, 30 Jul 2012 15:26:29 -0700 (PDT)

Hi,

I have reviewed this document and here are my comments.

o  4.  bullet 1 (after Figure 2)

   o  Along with the main routing table, which must always be present,
      an additional routing table is configured.

  What does "must always be present mean"?  This needs to be
  clarified.  In my world, it should not be necessary to configure
  this, so I assume it means that it always exists operationally, even
  if it is not explicitly configured.

o  4.1.

  It would be useful with a paragraph that explains that an
  implementation doesn't have to support more than a single router
  (i.e., no logical routers).

  It would also be useful to include the text you wrote in 
  http://www.ietf.org/mail-archive/web/netmod/current/msg06845.html.

o  4.3.

  Why is the main routing table's name only a SHOULD?  If it not a
  MUST, how will a client know which table is main?

o  4.3.

   The naming scheme for additional routing tables, as well as
   restrictions on the number and configurability of routing tables are
   implementation-specific.

  What does "restrictions on configurability" mean?

o  4.4

   Each routing protocol instance is connected to exactly one routing
   table for each address family that the routing protocol instance
   supports.

  How do I know which address families a routing protocol instance
  supports?

o  4.4.1

  OLD:

   Every router instance MUST contain exactly one instance of the
   "direct" pseudo-protocol type.   The name of this instance MUST
   also be "direct". 

  NEW:

   Every router instance MUST contain exactly one "routing-protocol"
   instance of the "direct" pseudo-protocol type.   The name of this
   instance MUST also be "direct". 

  What does this MUST mean?  Do I have to configure this
  routing-protocol instance?  What happens if I try to delete it?

  Why is the name important?

  Isn't it better to state that if no routing-protocol instance of
  type "direct" is configured, then all routes are sent to the main
  routing table.

o  4.4.1

   A pseudo-protocol of the type "static" allows for specifying routes
   manually.  It MAY be configured in zero or multiple instances,
   although a typical implementation will have exactly one instance per
   logical router.

  Do you mean "typical configuration"?

o  4.5

   The default value for the route
   filter type is the identity "deny-all-route-filter".

  I still think this is confusing and not necessary.  If I define a
  filter and leave out the type, the filter will block everything.  I
  think the result will be much more clear if the type is mandatory.
  I.e., when a filter is defined, the type must also be specified.

o  4.6.

  I think we should stop using the semi-formal notation of specifying
  new rpcs.  The YANG definition is supposed to capture all this
  information in a more strict way.  This only leads to duplicate text
  and the opportunity for errors.  In fact, the text, but not the YANG
  module, was updated with the "error-tag" fix.

  I suggest you remove 4.6.1 and 4.6.2, and just provide a short
  descrioption of these operations in 4.6. (and fix the text in the
  YANG module).

  You also use the term "FIB" here, but "forwarding table" earlier.
  Also, the rpc active-route says that is reads the route from the
  FIB, but earlier you wrote that the FIB was optional.  So this
  should probably be rephrased.

o  5.2  identity direct

  Move text from 4.4.1 to the description.  Same for "static".

o  5.2  identity deny-all-route-filter

  Should it be called "deny-all-routes"?

o  5.2.  grouping afn-safi

        leaf address-family {
         type ianaaf:address-family;
         default "ipv4";
   
  Is this default really appropriate?  Isn't it better w/ no default?

o  5.2.

  There are some single-qouted strings in descriptions that should be
  changed to double-quoted strings (in all three modules).

o  5.2. 

         leaf name {
           type string;
           description
             "The unique router name.";
         }
  
   NEW:

     "An arbitrary name of the router instance.";

o  5.2

             leaf name {
               type string;
               description
                 "The name of the routing protocol instance.";
             }

  NEW:

    "An arbitrary name of the routing protocol instance.";

  OR: state that an implementation MAY restrict the allowed values of
  this name (if this is what you meant).

o  5.2.

  Currently you have:

       +--rw connected-routing-tables
       |  +--rw routing-table [name]
      
  Should the latter list be called "connected-routing-table" for
  consistency with the rest of the model?

  The impressive "must" expression in this list uses a "rt:" prefix on
  some nodes, but not all.  I suggest you remove it from all nodes for
  consistency.

o  5.2.

             container static-routes {
               must "../type='rt:static'" {

  This should be "when", not "must".  See also 4.4.2 bullet 4!

o  5.2.

           list routing-table {
             key "name";
             description
               "Each entry represents a routing table identified by the
                'name' key. All routes in a routing table must have the
                same AFN and SAFI.";

  The last sentence has a "must" that doesn't make much sense, since
  a route doesn't "uses afn-safi".   

    NEW:

                'name' key. All routes in a routing table have the
                same AFN and SAFI.";

o  5.2

               list route {
                 description
                   "A routing table entry. This data node must augmented

  s/must augmented/must be augmented/

o  5.2.

                 leaf source-protocol {
                   type leafref {
                     path "/routing/router/routing-protocols/"
                        + "routing-protocol/name";
                   }

  I think the leafref should be relative - otherwise you can point to
  protocols in other logical routers.

  The text says:

                      This routing protocol must
                      be configured (automatically or manually) in the
                      device."

  so what happens if the routing protocol instance is removed?  Will
  all its routes be removed?

o  5.2

               list recipient-routing-table {
                 ...
                 leaf name {
                   type leafref {
                     path "/routing/router/routing-tables/"
                        + "routing-table/name";
                   }

  This should also be a relative path.

o  7

        Every implementation must preconfigure a routing table with the
        name 'main-ipv4-unicast', which is the main routing table for
        IPv4 unicast.

  This contraditcs the SHOULD on page 12.  

o  7

         list route {
           ...
           leaf id {
             type uint32 {
               range "1..max";
             }

  Why do you have a numeric id if it is arbitrary?  Why not a string?

o  8

            NOTE: Parameter 'is-router' is not included, it is expected
            that it will be implemented by the 'ietf-ip' module.

  Remove this sentence, or rephrase.

o  8

         leaf min-rtr-adv-interval {
           ...
           description
             "The minimum time allowed between sending unsolicited
              multicast Router Advertisements from the interface.

              Must be no greater than 0.75 * max-rtr-adv-interval.

  Make a "must" statement out if this constraint.

o  8

         leaf cur-hop-limit {
           default "64";
           description
              ...
              The default should be set to the value specified in IANA
              Assigned Numbers that was in effect at the time of
              implementation.

  Either remove the default statement, or change the text.

o  8

         leaf default-lifetime {
           ...
              The default value is dynamic and should be set to 3 *
              max-rtr-adv-interval.

  I prefer to not use the term "dynamic default".  I suggest this:

        If this leaf is not configured, the server uses the value
        3 * max-rtr-adv-interval.

/martin