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

[Andy Bierman <[e-mail]>]

On Wed, Aug 23, 2017 at 1:06 AM, Martin Bjorklund <[e-mail]> wrote:

> 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.
>
>
Why is it a problem to use a grouping in rc:yang-data?
This clearly needs to be rc:yang-data, not a real module with config=true
objects.
I think the artifact can contain data that matches the structure of config
data.
It is an implementation detail to convert the artifact to config data in a
datastore.



>
> /martin
>
>
>
Andy


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