Subject: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-mud-08

From: Martin Bjorklund <[email-masked]>
Date: Tue, 22 Aug 2017 06:38:14 -0700

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.

---

From: Eliot Lear <[email-masked]>
Date: Tue, 22 Aug 2017 16:03:13 +0200

Thanks, Martin, for your timely review.  We'll sort through the issues
and come back to the group and to you.

Eliot

---

From: Eliot Lear <[email-masked]>
Date: Mon, 28 Aug 2017 10:31:54 +0200

Hi Martin,

Thanks for performing the review.  Please see responses below.

>    least* these models are included?

ONLY.

>    be listed.  

Will remove.

> I suggest you are a bit more specific, and list:
>

Will do.

>    in your Terminology section.

Will replace.

>         "The element is currently supported
Will correct.

>    the text often use "device".  Maybe use "device" consistently?
Will correct.

>    (and same for ietf-acldns.yang)
We are making some changes, and are making heavy use of pyang --ietf
--strict...

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

>    initial RFC would have one revision statement).

Ok.

>    file.  I think this should be stated in the description of the
Ok.

>    this feature?  If so, maybe you can explain that use case in the

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

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

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.

>    Also, shouldn't this registry be defined in the IANA Considerations

Yes.

>    leaf in the YANG module.  There is one called "direction-initiated"

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

>    one for "to" and one for "from", is this augmentation really

Precisely so.  I've updated the working copy.

>    description is wrong, this should indeed be type empty).
That shouldn't say string.  It's empty.

>    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

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

---

From: Martin Bjorklund <[email-masked]>
Date: Mon, 28 Aug 2017 10:52:42 +0200 (CEST)

> >    least* these models are included?

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

> We are making some changes, and are making heavy use of pyang --ietf

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

Ok.

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

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.

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

/martin

---

From: Eliot Lear <[email-masked]>
Date: Mon, 28 Aug 2017 11:53:53 +0200

Hi Martin,

Trimming:

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

So clarified.

Ok.

> "access-lists" within the "mud" container.  I don't know how to solve

That would be quite bad.  Then in the alternative, if we go without
rc:yang-data, could we then simply leave the mud container as a presence
container?  Given the presence of the MUD-URL this has roughly the
concept intended with presence as I understand it.
> it turns out to be necessary, you can add it as a mandatory leaf in a

ok.  Let's have a think about that one.  If the file is aiming towards
self-describing, then this is a good add.

Eliot