Early Review of draft-ietf-mboned-amt-yang-04
review-ietf-mboned-amt-yang-04-yangdoctors-early-wills-2025-09-15-00

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:   <https://datatracker.ietf.org/wg/mboned/>
        WG List:  <mailto:mboned@ietf.org>

        Editor:   xxx
                  <mailto: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."