This is my review of draft-ietf-ippm-twamp-yang and implicitly of ietf-twamp.yang

Overall I think this document was reasonably easy to get through, but there are rather large amounts of repetition in the document, since much of it is said for both server and client sides, as well as in RFC text and again in the YANG module, and in UML as well as a pyang tree. A little less may be more.

If anything, the YANG description statements have less details than the RFC text. I would have wished it was the other way around. In my world the YANG module is the most important to get right. With all of this repetition, what if there are some discrepancies (now or introduced later) between the text and the YANG module? What is the authoritative source?

Section 2 leaves the TWAMP Operational Commands out of scope. While I can see why that is, it doesn't sound very productive to leave out essential parts of the TWAMP use case. Standardizing the measurement setup is all good, but without being able to collect the results in a standard way, the value diminishes quickly.

Section 4.1 says
   pm-reg-list
           A list of one or more Performance Metric Registry Index
           values (see [I-D.ietf-ippm-metric-registry], which
           communicate packet stream characteristics and one or more
           metrics to be measured.  All members of the pm-reg-list MUST
           have the same stream characteristics, such that they combine
           to specify all metrics that shall be measured on a single
           stream.

I don't understand what characteristics we're talking about here.

In section 4.2 page 16-17 and section 4.4 page 23 there are many references to leafs in the YANG module, which uses colon (:) to concatenate path elements, e.g. twamp-server:count and twamp-client:twamp-client-ctrl-connection:twamp-session-request:sid . I'd suggest that we use the YANG path concatenation symbol slash (/) instead. Especially since colon qualified symbols generally refer to namespaces in YANG context.

Section 4.1 and section 4.2 describes the max-count leaf like this:
   max-count
           If an attacking system sets the maximum value in Count
           (2**32), then the system under attack would stall for a
           significant period of time while it attempts to generate
           keys.  Therefore, TWAMP-compliant systems SHOULD have a
           configuration control to limit the maximum Count value.  The
           default max-count value SHOULD be 32768.

Since the YANG module specifies a configurable max-count with a default of 32768, there is no longer any SHOULD about this, it's now MUST. Also the description seems to be cut out of it's context and pasted here. There was no reference to attacking systems before this, so maybe a line of introduction would be in order.

Section 4.4 has similar wording
           The default value of
           REFWAIT SHALL be 900 seconds, and this waiting time MAY be
           configurable.

The YANG module specifies a leaf refwait that makes this configurable. There is no MAY about it. If we would want to keep the MAY in this statement, the leaf would have to be flagged with an if-feature.

Going through the YANG module in detail below. A general observation is that there are lot's of unnecessary quotes. E.g.
config "false"
position "6"
unique "mode"
mandatory "true"
etc. I think it's a nicer style would be to have all of this unnecessary clutter removed.

Going through the YANG module from top to bottom.

Line 122:
  typedef server-ctrl-connection-state {
    type enumeration {
      enum "active" {
        description "Active";
      }
      enum "servwait" {
        description "Servwait";
      }
    }
    description "Server control connection state";

Servwait is not a commonly used term, a little more detailed description of what these states mean would be in order.

Line 165:
  typedef sender-session-state {
    type enumeration {
      enum setup {
        description "Test session is active.";
      }
      enum failure {
        description "Test session is idle.";
      }

Enum names "setup", "failure" seem inconsistent with description "active", "idle". Rename or explain.

Line 208:
  container twamp {
    description "Top level container";

We know it's a top level container. Explain instead what TWAMP is, and what the four pillars inside are.

Line 210:
    container twamp-client {
      presence "twamp-client";
      description "Twamp client container";

I'd suggest renaming "twamp-client" to "client". It's already inside container "twamp". Presence statements should explain what it means if they are created, e.g. "Enables TWAMP-client functionality", and what if it doesn't exist.

Line 224:
        leaf priority {
          type uint16;
          description "priority";
        }

Explain that lower values indicate higher priority.

Line 235:
      list key-chain {
        key "key-id";
        leaf key-id {
          type string {
            length "1..80";
          }
          description "Key ID";
        }

Where is that max length of 80 characters coming from? Is there a reason/value for this limit?

Line 243:
        leaf secret-key {
          type string;
          description "Secret key";
        }

Is string appropriate here? Wouldn't binary be a more appropriate representation, which has a safe base64 textual encoding?

Line 259:
        leaf client-ip {
          type inet:ip-address;
          description "Client IP address";
        }

Explain what happens if client-ip is not set.

Line 280:
        leaf key-id {
          type string {
            length "1..80";
          }
          description "Key ID";
        }

Would it not be better if this was a leafref into the key-chain list?

Line 286:
        leaf max-count {
          type uint32 {
            range 1024..4294967295;
          }
          default 32768;
          description "Max count value.";
        }

As mentioned above, this is inconsistent with the RFC text's SHOULD. Here it is modeled as MUST. This value needs to be a power of two, say so in the description. Also explain the security implications of using a lower or higher number. A better range statement would be
range "1024|2048|4096|8192|16384|32768|65536|131072|262144|524288|1048576|2097152|4194304|8388608|16777216|33554432|67108864|134217728|268435456|536870912|1073741824|2147483648";

Alternatively, it might be more convenient for operators if this was modeled as a small power of two value with the range 10..31.

Line 298:
        leaf server-start-time {
          type uint64;
          config "false";
          description "The Start-Time advertized by the Server in
          the Server-Start message";
        }

What is the base and units of this value? Specify in description and using "units" keyword.

Line 344:
          leaf sender-ip {
            type inet:ip-address;
            description "Sender IP address";
          }

Explain what happens if not set.

Line 348:
          leaf sender-udp-port {
            type dynamic-port-number;
            description "Sender UDP port";
          }

Explain what happens if not set.

Line 365:
          leaf timeout {
            type uint64;
            default "2";
            description "The time (in seconds)Session-Reflector MUST
            wait after receiving a Stop-Session message.";
          }

Specify units using "units" keyword.

Line 379:
          leaf dscp {
            type inet:dscp;
            description "The DSCP value to be placed in the UDP
            header of TWAMP-Test packets generated by the
            Session-Sender, and in the UDP header of the TWAMP-Test
            response packets generated by the Session-Reflector
            for this test session.";
          }

Specify what happens if not set. Possibly add a default statement.

Line 397:
          leaf repeat {
            type uint32;
            default "0";
            description "Determines if the test session is to be
            run repeatedly. The default value of repeat is 0,
            indicating that once the session has completed, it
            will not be renegotiated and restarted. 1 thru 4,294,967,294
            indicate the number of repetitions, and the max value of
            4,294,967,295 indicates repeat forever.";
          }

Using magical values is a thing of the past and does not belong in YANG moduled, IMO. It's not making the configuration intent easy to parse by humans nor machines. Could we rather model this as
type union {
    type uint32 {
        range 0..4294967294;
    }
    type enumeration {
        enum forever;
    }
}

Line 407:
          leaf repeat-interval  {
            when "../repeat!='0'" {
              description "When repeat is not 0, the test is to be
              repeated";
            }
            type uint32;
            description "Repeat interval (in minutes)";
          }

Why minutes? Is this really following the principle of least surprise? In any case, add "units" statement.

Line 422:
    container twamp-server{
      if-feature server;
      presence "twamp-server";
      description "Twamp sever container";

I'd suggest renaming to "server". It's already inside container "twamp". Same comment about presence containers as for line 210.

Line 466:
      leaf dscp {
        type inet:dscp;
        description "The DSCP value to be placed in the IP header of
        TCP TWAMP-Control packets generated by the Server";
      }

Specify what happens if not set. Possibly add a default statement.

Line 471:
      leaf count {
        type uint32 {
          range 1024..4294967295;
        }
        description "Parameter used in deriving a key from a
        shared secret ";
      }
      leaf max-count {
        type uint32 {
          range 1024..4294967295;
        }
        default 32768;
        description "Max count value.";
      }

Describe that higher values are more secure, but require more computational power. Describe that this must be a power of two, and change the range to reflect the actual permissible values. Or change this to a power of two value with range 10..31.

Line 491:
      list key-chain {

Appears to be the same as on line 235. Turn this into a grouping? Same comments.

Line 578:
        leaf salt{
          type binary {
            length "16";
          }
          description "Salt MUST be generated pseudo-randomly";
        }

This is a config false value, not much point discussing how it MUST be generated.

Line 601:
    container twamp-session-sender{
      if-feature session-sender;
      presence "twamp-session-sender";
      description "Twamp session sender container";

I'd suggest renaming to "session-sender". It's already inside container "twamp". Otherwise same comments as for line 210.

Line 633:
        leaf number-of-packets {
          type uint32;
          description "The overall number of UDP test packets to be
            transmitted by the sender for this test session.";
        }

Specify what happens if not set. Possibly add a default statement.

Line 645:
            leaf periodic-interval-units  {
              type units;
              description "Periodic interval units";
            }

Specify what happens if not set. Possibly add a default statement.

Line 656:
            leaf lambda-units{
              type uint32;
              description "Lambda units.";
            }

Specify what happens if not set. Possibly add a default statement. Add a units "reciprocal-seconds" statement, and give a better description. It's not easy to figure out what value to use here currently.

Line 660:
            leaf max-interval{
              type uint32;
              description "maximum time between packet
              transmissions.";
            }

Maybe a better description would be "Maximum time between packet transmissions given in units specified by lambda-units."

Line 665:
            leaf truncation-point-units{
              type units;
              description "Truncation point units";
            }

What is this? I see no explanation in the document, other than the allowed values. What happens if you set it, or if it is left unset? Maybe add a default statement?

Line 680:
    container twamp-session-reflector {
      if-feature session-reflector;
      presence "twamp-session-reflector";
      description "Twamp session reflector container";

I'd suggest renaming to "session-reflector". It's already inside container "twamp". Otherwise same comments as on line 210.

Line 690:
      leaf refwait {
        type uint32 {
          range 1..604800;
        }
        default 900;
        description "REFWAIT (TWAMP test session timeout),
          the default value is 900";
      }

Add a units statement.

Section 6.1, in the second example has a port value out of range.
                  <reflector-udp-port>500001</reflector-udp-port>

Change to port 50001?

All throughout chapter 6 and 7 there are numerous examples of sender-udp-port and reflector-udp-port that are outside the dynamic port range. That would be invalid according to the YANG model.

                  <sender-udp-port>4001</sender-udp-port>

Change to port 54001? Must be in the dynamic port range.

Section 6 starts off stating "This section presents a simple but complete example of configuring all four entities in Figure 1", but only the three first examples are actually configurations. All the rest are the results from <get> operations with some filter.

Section 7, security considerations, is empty. Needs to be fixed.

Appendix A claims thart "This appendix extends the example presented in Section 6 by configuring more fields such as authentication parameters, dscp values and so on." All examples are however results of <get> operations. Several sender-udp-port and reflector-udp-port values are out of range.

/jan