Eliot Lear <[email protected]> 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
_______________________________________________
OPSAWG mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/opsawg
