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

[Eliot Lear <[e-mail]>]

Hi Martin,

Thanks for performing the review.  Please see responses below.

 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?

ONLY.

>    RFC6991 doesn't define any data nodes, so I don't think it needs to
>    be listed.  

Will remove.

> I suggest you are a bit more specific, and list:
>
>      o  ietf-access-control-list [I-D.ietf-netmod-acl-model]
>
>      o  ietf-mud [...]

Will do.

> 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.

Will replace.

>    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.";
>     }
>
Will correct.

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

> 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)
>
We are making some changes, and are making heavy use of pyang --ietf
--strict...

> 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.

As we move toward completion I may ask for your help.  I am nervous of
simply moving chunks of text into the descriptions because I think we
have a fairly readable document outside of the model.   I am perfectly
happy to copy text in, however.

> 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).

Ok.

> 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.
>
Ok.

> 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.

Fair point.  This was included in keeping with the acl model's
direction, but given that it is already an augment, you're right.

> o  leaf cache-validity could use a "units" statement:
>
>      units "hours";
>
Ok.

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

Ok.

> o  Should any of the leafs in "/metainfo" be mandatory?
>
Yes.  This has been the subject of considerable discussion.  I propose to modify the model to include an rc:yang-data statement, as well as to make the mud container a presence container.  I believe either would allow us to address this matter.  In addition, we'll add a mud-url element to make it possible for the entry to be self-identifying.  This would allow for a very simply "uses", should someone want to borrow the model to build out a configuration data store.

Indeed, based on these changes, we're attempting to consolidate the # of containers (or at least avoid an explosion of them).

Finally, this leaves open precisely which nodes should be mandatory.  To me, last-update should be mandatory, as well as the mud-url.  Joe would like "is-supported" covered, and I am not adverse.  I don't think "cache-validity" needs to be mandatory, but rather should have a default value of 48 hours.
 

> 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?

Yes.

> 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.

This is an artifact of an earlier version that needs to be removed.

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

Precisely so.  I've updated the working copy.

> 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).
>
That shouldn't say string.  It's 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.

For what purpose?

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

That's corrected.


Thanks VERY much again.

There are still finishing touches to do to the model, such that it is well documented and easily serialized, given the above changes.

Eliot