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

[Martin Bjorklund <[e-mail]>]

Hi,

I would like to direct the YANG doctors attention to one design choice
in this document:

The document defines a normal "config true" data model, but this data
model is not intended to be implemented by a server, but rather it
defines the file format of a "MUD file".

This idea is not new, it is used e.g. in the anima voucher document.
Normally, we would require that they use "rc:yang-data" to define such
a structure.

However, the MUD file is supposed to contain some top-level nodes
defined in the MUD YANG module, and also some /acl:access-lists
nodes.  So even if the MUD document could define a rc:yang-data
structure for the top-level nodes defined in the MUD document, it
cannot get the access lists into this rc:yang-data structure.

So the question to the YANG doctors is if this is ok, or if there is a
better way to define this file format.


/martin



Martin Bjorklund <[e-mail]> 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.
> 
> 
> _______________________________________________
> yang-doctors mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/yang-doctors
>