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

From: Benoit Claise <[email-masked]>
Date: Wed, 7 Sep 2016 17:57:25 +0200

Hi Mahesh,

Can you please let us know what the status is, after the YANG doctors 
review.
See Jan's first paragraph below.

Regards, Benoit
> Mahesh, all,
>
>> The authors have updated the document based on the comments sent in 
>> this e-mail. The latest version of the draft can be found here 
>> <https://tools.ietf.org/html/draft-ietf-ippm-twamp-yang-01>. Please 
>> let us know if the document addresses your comments.
>
> Hmm. I have checked several times to see if the link above really 
> leads to the latest document, but I find it does. Many of the points I 
> suggested needed changes have been fixed. Most, however, have not been 
> changed or fixed in any way. While the opinions expressed in my review 
> comments are certainly not binding to the WG in any way, I find some 
> of this status quo strange. Even a number of plain errors remain 
> unfixed. It is also odd that in many cases fixes are suggested by [mj] 
> in this mail thread, but do not appear in the actual document or YANG 
> spec.
>
> I apologize if perhaps I'm missing something fundamental.
>
>>> [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.
>
> I think this is an excellent change, which increases the 
> readability+maintainability of both the RFC and the YANG.
>
>>> [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.
>
> The module currently defines the max-count as follows:
>
>         leaf max-count {
>           type uint32 {
>             range 1024..4294967295;
>           }
>           default 32768;
>           description
>             "This parameter limits the maximum Count value.
>
>             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.";
>         }
>
> With this definition, there is no way for a system that implements 
> this module to use anything else than 32768 for the default value. The 
> RFC text says SHOULD, the YANG text says MUST through the use of the 
> default statement. I find this problematic. One or the other should be 
> changed to not be mutually contradictory.
>
> For MUST behavior: change the RFC text to say MUST.
>
> For SHOULD behavior: remove the YANG "default" statement, so that each 
> implementation can pick its own implementation dependent default. If 
> there is no default statement, then each implementor can implement the 
> behavior they like for the case that this optional leaf is left unset. 
> There is no such freedom if the RFC keeps a default statement in the 
> YANG module.
>
>>>> 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.
>
> Sounds good. But it's not in the YANG.
>
>>>> 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.
>
> The name change is implemented, nice. But the presence statement 
> doesn't describe anything useful. As noted above it should be 
> something like
> presence "Enables TWAMP-client functionality";
>
>     container client {
>       if-feature control-client;
>       presence client;
>       description
>         "Configuration of the TWAMP Control-Client logical entity.";
>
>>>> 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.”;
>
> Fine, but this text isn't in the YANG.
>
>>>> 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 for 
>>> key-chain and its attributes.
>
> They are still here now, YANG unchnaged.
>
>>>> 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.
>
> Not fixed yet.
>
>>>> 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.
>
> There are two leaf count at different levels under /twamp/server. One 
> of them specifically says power of 2. Shouldn't they have the same 
> description and range?
>
> Leaf max-count is described as "This parameter limits the maximum 
> Count value.", wouldn't that mean that it needs to be a power of two 
> as well?
> Would it make sense to have a must statement on count, must "current() 
> <= ../max-count" ?
>
> Regardless of the comment above, the more specific range is not seen 
> anywhere in the YANG.
>
>       leaf count {
>         type uint32 {
>           range 1024..4294967295;
>         }
>         description
>             "Parameter used in deriving a key from a shared
>             secret as described in <a 
> href="./rfc4656#section-3.1">Section&nbsp;3.1 of  RFC 4656</a>,
>             and are communicated to the Control-Client as part
>             of the Server Greeting message.
>
>             count MUST be a power of 2.
>
>             count MUST be at least 1024.
>
>             count SHOULD be increased as more computing power
>             becomes common.";
>       }
>
>>>> 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”;
>
> Very good. But it's not in the latest YANG.
>
>>>> 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??
>
> No change in the YANG as yet.
>
>>>> 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;
>>>    ….
>>> }
>
> Good, but not in the YANG.
>
>>>> 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.
>
> It's not in the YANG.
>
>>>> 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.
>
> No change to the YANG as yet.
>
>>>> 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;
>
> Very good, but it's not in the YANG.
>
>>>> 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.
>
> Not yet.
>
>>>> 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.
>
> No change in YANG yet.
>
>>>> 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.
>
> No change.
>
>>>> Line 645:
>>>>            leaf periodic-interval-units  {
>>>>              type units;
>>>>              description "Periodic interval units";
>>>>            }
>>>>
>>>> Specify what happens if not set. Possibly add a default statement.
>
> Not adressed.
>
>>>> 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.
>
> No change.
>
>>>> 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.
>
> No change.
>
>>>> 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.
>
> None of the examples seem to have been corrected.
>
> /jan
>
>
>
> _______________________________________________
> ippm mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/ippm