Early Review of draft-ietf-lisp-yang-11
review-ietf-lisp-yang-11-yangdoctors-early-hopps-2019-07-25-00

Request Review of draft-ietf-lisp-yang
Requested rev. no specific revision (document currently at 12)
Type Early Review
Team YANG Doctors (yangdoctors)
Deadline 2019-07-21
Requested 2019-05-21
Requested by Luigi Iannone
Authors Vina Ermagan, Alberto Rodriguez-Natal, Florin Coras, Carl Moberg, Reshad Rahman, Albert Cabellos-Aparicio, Fabio Maino
Draft last updated 2019-07-25
Completed reviews Yangdoctors Early review of -11 by Christian Hopps (diff)
Comments
Hello Yang Doctors,

In the LISP WG we have a document providing a Yang model for the whole thing (https://datatracker.ietf.org/doc/draft-ietf-lisp-yang/).
From our perspective, the document is stable and done.

However, we would like to avoid having the document sent back from the IESG because of issue with the model.

Hence, before going with a WG Last Call we would really appreciate if we can have a review from you.

Anyone that can help? 

Thanks 

Luigi Iannone
LISP WG Co-Chair
Assignment Reviewer Christian Hopps
State Completed
Review review-ietf-lisp-yang-11-yangdoctors-early-hopps-2019-07-25
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/5jSA8lycy8zj-LqiO1QDZKZIOrA
Reviewed rev. 11 (document currently at 12)
Review result On the Right Track
Review completed: 2019-07-25

Review
review-ietf-lisp-yang-11-yangdoctors-early-hopps-2019-07-25

* draft-ietf-lisp-yang-11 early yang-doctor review.

** Minor

   - Enumerations are not extensible and so should only be used when all the
     values are known and will not need to be added to. So, auth-algorithm-type
     should use identities and not an enumeration, as it almost for sure will
     need to be added to in the future. An example of this is present in RFC8177
     (keychains) which has identities derived from a base identity
     "crypto-algorithm".

   - "type string" is a very inclusive UTF-8 string, along with all legal UTF-8
     characters it includes tabs, spaces, newlines and carriage returns. This
     may not be what you actually want for things like "eid-id"s or
     "auth-key-id". You probably want to use a more restricted typedef variation
     of string (using a pattern to restrict its values).

   - Node name consistency, you probably should be consistent with the name for
     nodes of the same type. For example type "eid-id", sometimes the name is
     "id" other times "eid-id" is used.

   - TTL is limited to minute units. This may be overly restrictive. Couldn't
     there be some use (perhaps not common) e.g., perhaps when debugging, or in
     future versions of the protocol where seconds granularity might be useful?
     Changing these nodes later is non-backwards compatible and thus very
     painful to do.

** geo coordinates

   - It might be worth considering using the grouping in the geo-location module
     for specifying coordinates. The only drawback here would be if geo-location
     causes the publication to be delayed b/c lisp-yang finishes first. In any
     case the description for the coordinate nodes should echo more of the info from
     the LCAF RFC, in particular that the coordinate system used is WGS-84).

   - I found a lisp geo draft and it seems to specify a bit more detail than is
     covered in this module (e.g., the kilometer bit, radius, uncertainty). Not
     sure if that would be appropriate to add or not.

** Nits

  - Invalid example XML for LISP Map-Server.. The config namespace should not be
    "http://tail-f.com/ns/config/1.0".

  - Correct module vs model language.
    - OLD: <t>This module is the base LISP module that is augmented in multiple
    models to represent various LISP device roles.</t>
    - NEW: <t>This is the base LISP module.  It is further augmented by the
    LISP device role specific modules defined elsewhere in this document.</t>

  - Yang comments need some grammar fixes.
    - e.g., 'This augments *the* LISP decices list ...'

  - I got some warning from validation tools, but I'm not sure if they are
   valid, please double check though.

    - Pyang nits:
      #+begin_src bash
        for f in *.yang; do echo $f; \
        pyang --ietf --max-line-length 69 $f ; \
        done
        ietf-lisp-address-types@2019-02-23.yang
        ietf-lisp-etr@2019-02-23.yang
        ietf-lisp-etr@2019-02-23.yang:86: warning: line length 70 exceeds 69 characters
        ietf-lisp-etr@2019-02-23.yang:106: warning: line length 70 exceeds 69 characters
        ietf-lisp-etr@2019-02-23.yang:157: warning: line length 71 exceeds 69 characters
        ietf-lisp-etr@2019-02-23.yang:164: warning: line length 70 exceeds 69 characters
        ietf-lisp-etr@2019-02-23.yang:171: warning: line length 72 exceeds 69 characters
        ietf-lisp-etr@2019-02-23.yang:179: warning: line length 72 exceeds 69 characters
        ietf-lisp-itr@2019-02-23.yang
        ietf-lisp-itr@2019-02-23.yang:125: warning: line length 70 exceeds 69 characters
        ietf-lisp-mapresolver@2019-02-23.yang
        ietf-lisp-mapresolver@2019-02-23.yang:91: warning: line length 72 exceeds 69 characters
        ietf-lisp-mapserver@2019-03-05.yang
        ietf-lisp-mapserver@2019-03-05.yang:204: warning: line length 70 exceeds 69 characters
        ietf-lisp@2019-03-05.yang
        ietf-lisp@2019-03-05.yang:431: warning: line length 70 exceeds 69 characters
        ietf-lisp@2019-03-05.yang:485: warning: line length 71 exceeds 69 characters
        ietf-lisp@2019-03-05.yang:490: warning: line length 71 exceeds 69 characters
      #+end_src