Hi AMT yang model authors, I've done a Yang Doctor Early review for draft-ietf-mboned-amt-yang-04. Thank you for your work on the document so far, and for sending it for an early review. This is a good model, and although I have quite a few comments, most of them are just to bring the document in line with best practice. I also have comments on the names and types of some leaves, and a recommendation to enhance the document to provide some examples of using the model to configure AMT. I've split my review into four sections: A. Comments on the document (excluding the model itself). B. Comments on the structure of the Yang model. C. Comments on other aspects of the Yang model. D. Comments on the description strings. Thank you in advance for your time in going through my comments below. Rob Wills. A. Comments on the document =========================== 1. Introduction This should state that the document contains a YANG module that is compliant with NMDA (RFC8342). eg "The YANG module in this document conforms to the Network Management Datastore Architecture (NMDA) [RFC8342]." ---- 2. Model Overview Please move Section 1.4, "Prefixes in Data Node Names" to be a subsection of Section 2. Also, please could you change the layout of the table to match the corresponding Section 2.3 of RFC8349. ---- 4. Security Considerations Please update the first two paragraphs to match the latest security considerations template: The "ietf-amt" YANG module defines a data model that is designed to be accessed via YANG-based management protocols, such as NETCONF [RFC6241] and RESTCONF [RFC8040]. These protocols have to use a secure transport layer (e.g., SSH [RFC4252], TLS [RFC8446], and QUIC [RFC9000]) and have to use mutual authentication. The Network Configuration Access Control Model (NACM) [RFC8341] provides the means to restrict access for particular NETCONF or RESTCONF users to a preconfigured subset of all available NETCONF or RESTCONF protocol operations and content. ---- 4. Security Considerations There are various typos in the two paragraphs that say "Under". In both cases, it should say: Under /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol/: ---- 4. Security Considerations There are various typos in the subtrees, mostly using colons instead of slashes. For example, the first one, "amt:relay:relay-addresses/relay-address" should say "amt/relay/relay-addresses/relay-address". ---- 5. IANA Considerations The ietf-amt module must be reigstered in both the "IETF XML Registry" and the "YANG Module Names" registry. See Section 5 of RFC 8407 for example text. ---- Usage examples It would be nice if the document could include a couple of usage examples for the Yang module. See Section 3 of RFC 9127 for some examples of how to do this. B. Comments on the structure of the Yang model ============================================== Rename "amt-tunnels" -> "tunnels", and the same for the list underneath it. Rename "multicastflows" -> "multicast-flows", and the same for the list underneath it. Consider renaming "amtrelay" -> "amt-relay" and "amtgateway" -> "amt-gateway". Perhaps there is a reason why they are combined into one word though? There's some duplication in node names for the stats containers. It would be good to shorten child node names where it's clear what the meaning is from the parent: "amtrelay-received-message-statistics" -> "received" "amtrelay-sent-message-statistics" -> "sent" "amtrelay-error-message-statistics" -> "errors" "amtgateway-received-message-statistics" -> "received" "amtgateway-sent-message-statistics" -> "sent" Could the "ifIndex" leaf be renamed to "interface", to be consistent with other leaves such as "upstream-interface"? C. Comments on other aspects of the Yang model ============================================== For each of the imports, the reference string should be written using a colon not a comma. I would also prefer it if there was no full stop, but this is a nit. For example, "RFC 8294, Common YANG Data Types for the Routing Area." should be "RFC 8294: Common YANG Data Types for the Routing Area" The same applies to the reference string in the module's revision statement. ---- The module's contact details should be filled in, in this sort of form: "WG Web: WG List: Editor: xxx " ---- The d-flag leaf does not need quotes around the default value. ie change 'default "false"' -> 'default false'. Similarly, the enum items do not need quotes around the value. ie change 'value "0"' -> 'value 0'. ---- I'm curious about the use of an identity-ref for address-family. My understanding is that AMT is only ever supported on IPv4/v6, and the rest of the model is written that way. Would an enum with two entries, ipv4 and ipv6, be more appropriate here? ---- There are many counters in the model which currently have uint32/uint64 type. It would be better to use yang:zero-based-counter32 and yang:zero-based-counter64 defined in the ietf-yang-types module. ---- The amt/gateway/pseudo-interfaces/pseudo-interface/{ifIndex,upstream-interface} leaves have type interface-ref, which is an interface name. But the "ifIndex" leaf name and the description strings say it can also be an ifindex. Does the model even need to say anything about this? An alternative would be to use the following description strings: list pseudo-interface { key "interface"; leaf interface { type if:interface-ref; description "Pseudo interface." } ... leaf upstream-interface { type if:interface-ref; description "Upstream interface." } ... } D. Comments on the description strings ====================================== In the description strings, the module uses inconsistent capitalization for "AMT Relay" vs "AMT relay". Is there a reason for the distinction? ---- I don't understand these two descriptions: "The anycast IP address of AMT Relay Discovery Address." "The unicast IP address of AMT Relay Address." ---- I also have some comments on specific description strings which hopefully are easy to search for: Change "Each entry contains parameters for a AMT relay Address..." -> "Each entry contains parameters for an AMT Relay address...". Change "The Address family." -> "Address family." Change "The AMT tunnel is being establishing." -> "The AMT tunnel is being established." Change "The Ip address of AMT relay address." -> "IP address of the AMT Relay address." Change "The method of discover relay address." -> "The method used to discover the relay address."