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

[Eliot Lear <[e-mail]>]

Thanks, Martin, for your timely review.  We'll sort through the issues
and come back to the group and to you.

Eliot


On 8/22/17 3:38 PM, Martin Bjorklund wrote:
> 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.
>
>
>