Subject: Re: [ippm] [yang-doctors] YD review of draft-ietf-ippm-twamp-yang

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 30 Jun 2016 17:11:29 -0700

Jan,

After a discussion with some of the authors of the drafts, here are some comments/questions related to your review of the YANG module. 

> On Jun 17, 2016, at 6:26 AM, Jan Lindblad <[email-masked]> wrote:
> 
> 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.

[mj] Our decision was to move the current set of documentation on each attribute from the draft into the description field of the YANG module, and reducing the documentation within the draft itself to a high level overview. You will find as a result, that it will address some of the questions you have raised about attributes.

> 
> 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.

[mj] What you say is true for YANG in general. However, you will find that current standard for TWAMP does not call for any operational parameters to be collected and supported. Different vendors might collect different parameters, if they do at all, with nothing in common that could be documented in a YANG model.

> 
> 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.

[mj] In general there seems to be some confusion around the usage of RFC 2119 language for terms such as MUST. SHOULD. MAY etc. as it relates to text in the draft and its implication/use in the YANG module. We could remove its usage in the YANG module,or change the SHOULD to a should,  and leave the text in the draft if that helps to draw the difference. A SHOULD in an RFC is a RECOMMEND, which for valid reasons can be ignored by the system. Therefore ….

> 
> 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.

[mj] The reason that max-count is a SHOULD and not a MUST is because it is still a recommendation of the count. Nowhere in the model do we enforce it with a must/if statement. Implementations can choose any value within that range if they want to change the default value.

We can add text for “attacking system”.

> 
> 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.

[mj] Will remove quotes around these statements.

> 
> 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.

[mj] When we move the description from the draft into the description statement, we will add reference to Servwait.

> 
> 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.

[mj] We are inconsistent between the text in the draft which says:

   sender-session-state
           This read-only item can be either Active or Idle.

and the values we have assigned to enums. Will either update the description or change the enum.

> 
> 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.

[mj] How about this?

Container twamp consists of the four models that together describe the TWAMP deployment. They are TWAMP Control-Client, Server, Session-Sender
   and Session-Reflector. 

> 
> 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.

[mj] Ok. Will rename twamp-client to client. You do bring up a good point on what if does not exist. Let me discuss with the authors.

> 
> Line 224:
>        leaf priority {
>          type uint16;
>          description "priority";
>        }
> 
> Explain that lower values indicate higher priority.

[mj] How about this?

description “Expressed as a 16-bit unsigned integer, where zero is the highest priority and
   subsequent values monotonically increasing) with their corresponding
   mode (expressed as a 32-bit Hexadecimal value).  Depending on the
   Modes available in the Server Greeting, the Control-Client MUST
   choose the highest priority Mode from the configured mode-preference-
   chain list.”;

> 
> 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?

[mj] References to key-chain, key-id, secret-key will be removed from this draft. We will refer to the YANG model in https://tools.ietf.org/html/draft-ietf-rtgwg-yang-key-chain-06 <https://tools.ietf.org/html/draft-ietf-rtgwg-yang-key-chain-06> for key-chain and its attributes.

> 
> Line 259:
>        leaf client-ip {
>          type inet:ip-address;
>          description "Client IP address";
>        }
> 
> Explain what happens if client-ip is not set.

[mj] When we move the description of client ip into the model, it will address this issue.

> 
> 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?

[mj] Same comment as key-chain above.

> 
> 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.

[mj] I think there some confusion brought about by the range and default value as it applies to 'max-count'. While the range and default value have numbers that are power of 2, I am not sure the implication is that the value that can be set has to be power of 2. At least not for this attribute. It might be true for the attribute ‘count'. For that attribute, I agree that we could use the range statement that you have defined above.

> 
> 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.

[mj] How about this?

leaf server-start-time {
   ….
   units seconds;
   description 
       “The Start-Time advertised in seconds by the Server in the
         Server-Start message”; 

> 
> Line 344:
>          leaf sender-ip {
>            type inet:ip-address;
>            description "Sender IP address";
>          }
> 
> Explain what happens if not set.

[mj] When we move the description of server ip into the model, it will address this issue.

> 
> Line 348:
>          leaf sender-udp-port {
>            type dynamic-port-number;
>            description "Sender UDP port";
>          }
> 
> Explain what happens if not set.

[mj] Good point. While we say what happens if the value is set to zero, we do not say what happens if the value is not set at all. A default value of 0??

> 
> 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.

[mj] How about this?

leaf timeout {

   ….
   units seconds;
   ….
}

> 
> 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.

[mj] Moving the description from the draft into the model should address this.

> 
> 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;
>    }
> }

[mj] Agree. Will update the model.

> 
> 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.

[mj] s/minutes/seconds/

and add

   units seconds;

> 
> 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.

[mj] Accept.

> 
> 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.

Moving the description from the draft into the model will address this.

> 
> 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.
> 

[mj] Same response as other key-chain comment.

> 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.

[mj] Agreed.

> 
> 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.”

[mj] Agree.

> 
> 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?

[mj] Ok. Will add reference to Section 4.6 of RFC 7312. Units will be seconds.

> 
> 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.

Agree.

> 
> 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.

Will correct.

> 
> 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.

Will correct text.

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

Will update.

> 
> 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.

Will change text and fix port values.

Thanks
> 
> /jan
> 
> _______________________________________________
> yang-doctors mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/yang-doctors

Mahesh Jethanandani
[email-masked]