[yang-doctors] Yangdoctors early review of draft-ietf-opsawg-mud-08

[Martin Bjorklund <[e-mail]>]

Reviewer: Martin Bjorklund
Review result: Ready with Issues

Hi,

I am the assigned YANG doctors reviewer for this document.  Here are
my comments:


o  Section 2 says:

   The MUD file is limited to the serialization of a
   small number of YANG schema, including the models specified in the
   following documents:

   o  [I-D.ietf-netmod-acl-model]

   o  [RFC6991]

   Is the intention that *only* these models are included, or *at
   least* these models are included?

   RFC6991 doesn't define any data nodes, so I don't think it needs to
   be listed.  I suggest you are a bit more specific, and list:

     o  ietf-access-control-list [I-D.ietf-netmod-acl-model]

     o  ietf-mud [...]


o  Section 3 uses the term "element" (it is used in other places as
   well).  YANG uses the term "data node" or "node".  Or "YANG data
   node".  I suggest you use one of these terms, and import the term
   in your Terminology section.

   Also, the YANG module uses the term "element" to refer to "device":

    leaf is-supported {
      type boolean;
      description
        "The element is currently supported
         by the manufacturer.";
    }


o  In your Terminology section you introduce the term "Thing".  But
   the text often use "device".  Maybe use "device" consistently?


o  In order to get consistent indentation of the YANG modules, I
   suggest you run:

     pyang -f yang ietf-mud.yang

   (and same for ietf-acldns.yang)


o  Ensure that description statements contain proper sentences.  Also
   ensure that the descriptions are descriptive.  As an example of the
   latter, this is not a good description:

    description
      "Which way are we talking about?";

   In general, I found that the main document had better descriptions
   than the YANG module.  Consider moving the text from the main
   document to the YANG module (this also reduces the risk of
   inconsistencies).  If don't want to move text, I think you need to
   spend some effort on almost all descriptions in the YANG module.


o  In both modules, make sure you have a single revision
   statement.  Note that in IETF-terms, a revision statement is added
   when a new version of the module is publsihed as an RFC (so the
   initial RFC would have one revision statement).


o  The "ietf-mud" module is a bit unorthodox; it defines configuration
   data nodes, but it is not supposed to be implemented by a normal
   NETCONF/RESTCONF server.  Rather, it will be instantiated in a JSON
   file.  I think this should be stated in the description of the
   module.


o  I don't think the feature "mud-acl" is necessary.  It is only used
   to make the acl augment conditional on the feature.  I think that
   if this module is supported, the feature is also supported.  Or do
   you envision implementations of this module that would not support
   this feature?  If so, maybe you can explain that use case in the
   document.


o  leaf cache-validity could use a "units" statement:

     units "hours";


o  I suggest you rename the grouping "access_lists" to "access-lists"
   for consitency.


o  Should any of the leafs in "/metainfo" be mandatory?


o  The "extensions" leaf-list mentions an IANA registry for
   extensions.  It would be usefule to mention this registry by name.

   Also, shouldn't this registry be defined in the IANA Considerations
   section?


o  Section 3.7 mentions a leaf "packet-direction".  There is no such
   leaf in the YANG module.  There is one called "direction-initiated"
   though.

   But since the "/device" container contains two different ACL sets,
   one for "to" and one for "from", is this augmentation really
   necessary?


o  The model has:

      leaf local-networks {
        type empty;
        description
          "this string is used to indicate networks
           considered local in a given environment.";

   This leaf is of type "empty", but the description says it is a
   string.

   Also, what is the format of this string?  (Hmm, I think the
   description is wrong, this should indeed be type empty).


o  Would it be useful with an indication of the revision of "ietf-mud"
   that is used as the schema for a MUD file?  I.e., something like a
   leaf "mud-module-revision" in the "metainfo" container.


o  The example in section 8 has some errors, e.g., it has some
   camelCase node names.