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

[Eliot Lear <[e-mail]>]

Hi Andy,

It probably *does* make sense (it didn't and then we reorged a bit).

Eliot


On 8/23/17 6:59 PM, Andy Bierman wrote:
>
>
> On Wed, Aug 23, 2017 at 1:06 AM, Martin Bjorklund <[e-mail]
> <mailto:[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] <mailto:[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] <mailto:[e-mail]>
>     > https://www.ietf.org/mailman/listinfo/yang-doctors
>     <https://www.ietf.org/mailman/listinfo/yang-doctors>
>     >
>
>     _______________________________________________
>     yang-doctors mailing list
>     [e-mail] <mailto:[e-mail]>
>     https://www.ietf.org/mailman/listinfo/yang-doctors
>     <https://www.ietf.org/mailman/listinfo/yang-doctors>
>
>