Thanks, Martin, for your timely review. We'll sort through the issues and come back to the group and to you.
Eliot
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?
>
> 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.
>
>
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ OPSAWG mailing list [email protected] https://www.ietf.org/mailman/listinfo/opsawg
