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

[Martin Bjorklund <[e-mail]>]

Eliot Lear <[e-mail]> wrote:
> 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.

Ok.  The text says "a small number ... including ...".  I suggest this
is clarified.

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

Ok, but --ietf does not check indentation etc.

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

Ok.

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

Ok, but note that this makes your usage of access-lists problematic -
if you define "rc:yang-data mud", you cannot have a file with
"access-lists" within the "mud" container.  I don't know how to solve
this...


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

So that the MUD controller knows how to parse the mud file?  OTOH, if
it turns out to be necessary, you can add it as a mandatory leaf in a
future model.

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



/martin