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