Re: Comments on l2tpv3-yang-model: Covering keyed-v6-tunnel?

Hi Bing, 

Thanks for response. Please see inline.

Cheers,
Qi

On Jan 21, 2015, at 4:12 AM, Liubing (Leo) <[email protected]> wrote:

> Hi Qi,
> 
> Thanks much for your review and comments. Replies are inline.
> 
>> -----Original Message-----
>> From: L2tpext [mailto:[email protected]] On Behalf Of Qi Sun
>> Sent: Monday, January 19, 2015 9:50 PM
>> To: [email protected]
>> Cc: [email protected]
>> Subject: [L2tpext] Comments on l2tpv3-yang-model: Covering
>> keyed-v6-tunnel?
>> 
>> Dear authors,
>> 
>> I have read draft-shen-l2tpext-l2tpv3-yang-model-00, which provides the
>> configuration and management of l2tpv3 tunnels. A few comments below.
>> 
>> The most significant one is that, this YANG model is about managing
>> RFC3931 in general, which does not cover the requirements from
>> draft-ietf-l2tpext-keyed-ipv6-tunnel. Since the essence of
>> draft-ietf-l2tpext-keyed-ipv6-tunnel (as I see) is to simplify L2TPv3 tunnel by
>> taking some parts from RFC3931, the YANG model for L2TPv3 should also be
>> able to cover the YANG model for keyed-v6-tunnel.
>> 
>> I've worked out a YANG diagram tree for keyed-v6-tunnel (and a YANG
>> model). The YANG model makes use of some parts from the
>> l2tpv3-yang-model (especially the "static" case), and also adds some
>> parameters for its own. Since the length of Cookie should always be 64 bits,
>> the following YANG model doesn't include it as a parameter to configure.
>> 
>> <YANG for keyed-v6-tunnel>
>> module: keyed-v6-l2tpv3
>>  +--rw keyed-v6-l2tpv3
>>     +--rw enabled?                          boolean
>>     +--rw l2tpv3TunnelInstances
>>        +--rw l2tpv3TunnelInstance* [tunnelName]
>>           +--rw tunnelName                  string
>>           +--rw srcIfName                     if:interface-ref //
>> *unique*
>>           +--rw srcIPv6                         inet:ipv6-address
>> /* unique *
>>           +--rw dstIPv6                         inet:ipv6-address
>> *        */
>>           +--rw localSessionId?             uint32
>>           +--rw remoteSessionId?         uint32
>>           +--rw localCookies
>>           |  +--rw localCookie* [cookieName]
>>           |     +--rw cookieName            enumeration //
>> "new"/"old"
>>           |     +--rw localHighCookie       hexBinary
>>           |     +--rw localLowCookie        hexBinary
>>           +--rw remoteCookie
>>           |  +--rw remoteHighCookie         hexBinary
>>           |  +--rw remoteLowCookie          hexBinary
>>           +--rw mtu                                 unit16
>>           +--rw (oam_method)
>>           |  +--:(ieee8021ag!)
>>           |  +--:(ituy1731!)
>>           +--rw vccv_enabled?               boolean
>> //There is a separate "operational state" subtree, which isn't shown
>> above.
>> </YANG for keyed-v6-tunnel>
>> 
>> I'm thinking if it's a good idea to merge the two models (with some
>> explanation text or a separate section in the l2tpv3-yang-model draft), since
>> there are some similarities. Otherwise, a separate I-D could be written to
>> document the keyed-v6-tunnel YANG.
>> 
>> Could you, authors and the WG, share your ideas on this? Thanks a lot!
> 
> [Bing] Actually, supporting l2tpext-keyed-ipv6-tunnel is an important goal when we wrote the YANG model. But it was not specifically called out in the draft. Maybe we need to make it clearer in the next version.
> I think the essential management objects of l2tpext-keyed-ipv6-tunnel are already in the current draft, just as you said, they are in the "static" case. And we thought it might be better to include it in just one l2tpv3 model.

[Qi] I also think it would be good that we just have one model. The question with the current version is that, there is no text mentioning l2tpext-keyed-ipv6-tunnel, neither a reference to that draft, which confusing me how to manage the l2tpext-keyed-ipv6-tunnel if someone only reads this YANG model draft. It would be helpful if some text/section is added for that draft, IMHO. 

Do you think adding some “default” values for some parameters to make it default to act as a keyed-v6-tunnel would helpful to explicitly express the support for keyed-v6-tunnel? Not sure if this is a good idea. 

> 
> Some new elements you proposed are:
> - rw MTU
> We thought it might be good to add this element to current model, since the NMS might need to configure this parameter. Thanks for the suggestion.
> - rw (oam_method)
> The OAM function is needed. But I'm not sure whether it should be defined in L2TPv3 model? Maybe leave it to the OAM relevant models is a better choice.

[Qi] I’m not trying to define management for OAM function here. It’s only a “switch” to turn the function on/off. If you read the code, the it’s only a presence container which conveys the information of whether this function is enabled. 

For more info, please refer to this: http://tools.ietf.org/html/rfc6020#section-7.5.1
There is an example which illustrates the presence container with the “ssh” function. That container doesn’t define management parameters for ssh, just enable ssh or not. 

> - rw vccv_enabled
> I don't know what it means. Would you mind explain it? Thanks.

[Qi] Sorry for not describing it clearly. It is mentioned in https://tools.ietf.org/html/draft-ietf-l2tpext-keyed-ipv6-tunnel-01#section-6 :
“
   In addition the Pseudowire Virtual Circuit Connectivity Verfiication
   ( VCCV ) RFC5085 [RFC5085] MAY be used.
"

> 
>> Here are a few more comments on the content of the draft, detailed as
>> follows.
>> 1) According to RFC3931, the L2TPv3 tunnel should support the mode of
>> "one tunnel, multiple sessions". However, the current YANG model doesn't
>> support it. I would suggest the parameter of Sessionid be a list, with
>> corresponding modifications.
> [Bing] For auto tunnels, we didn't define the session objects in current model, since we thought the sessions are automatically generated by the protocol module thus don't need to be managed. So I think there is no limitation to forbidden multi-sessions-per-tunnel in current model.

[Qi] It’s only correct for the case os “auto tunnels”. Not true for the “static” branch. I don’t think there is a limitation in _RFC3931_ that the static configuration of l2tpv3 only supports “one session per tunnel”?

> But your comment triggered me to think of that maybe we need to define read-only objects to allow the NMS to read the automatically generated sessions (and probably reflect the multi-sessions-per-tunnel feature). We'll consider it in the next version. What's your opinion on this?

[Qi] Agreed. That’s why I suggested to include a operational state subtree for the model. That subtree should be able to get all information that might be useful for operating the system.

> 
> For l2tpext-keyed-ipv6-tunnel, there is no problem since each keyed IPv6 tunnel has one and only one associated session, there is no problem of it.

[Qi] Right. 

> 
>> 2) The expression of choice-case structure in the diagram tree is not the
>> common style used by other YANG models. Please refer to RFC7277.
> [Bing] I don't quite understand. Would you mind elaborating more? Thanks.

[Qi] Sure. Please see the following example.

<draft-shen-l2tpext-l2tpv3-yang-model-00>
            +-- rw tunnelType                           enum
            |     +-- rw static:
            |     |   +-- rw localSessionId?            uint32
            |     |   +-- rw remoteSessionId?           uint32
            ...  ...  ...
            |     +-- rw auto:
            |           +-- rw ctrlName            string
            |           +-- rw encapType           enum
</draft-shen-l2tpext-l2tpv3-yang-model-00>

<RFC7277>
           |  +--rw address* [ip]
           |  |  +--rw ip               inet:ipv4-address-no-zone
           |  |  +--rw (subnet)
           |  |     +--:(prefix-length)
           |  |     |  +--rw ip:prefix-length?   uint8
           |  |     +--:(netmask)
           |  |        +--rw ip:netmask?         yang:dotted-quad
</RFC7277>

In your code, the parameter “tunnelType” is a “choice” and both “static” and “auto” are cases. The expression of choice-case in the diagram tree is not typical in other YANG models, e.g. RFC7277, RFC7407, etc. Here I extract the related part from RFC7277, where “subset” is a choice and “prefix-length” and “netmask” are cases.
There are some other “choice"s, which might need improving.

And typically, there is an explanation in those YANG RFCs, saying that:

   o  Brackets "[" and "]" enclose list keys.

   o  Abbreviations before data node names: "rw" means configuration
      (read-write), and "ro" means state data (read-only).

   o  Symbols after data node names: "?" means an optional node, "!"
      means a presence container, and "*" denotes a list and leaf-list.

   o  Parentheses enclose choice and case nodes, and case nodes are also
      marked with a colon (":").

   o  Ellipsis ("...") stands for contents of subtrees that are not
      shown.

Not sure if the practice has changed since I don’t quite follow the netmod WG. Please keep me up-to-date if the netmod WG has decided to use a different pattern. Thanks!

> 
>> 3) Typically, there is a separate operational state subtree for those "ro"
>> parameters. Some restructures might be helpful to make the model clearer, if
>> you like. Also refer to RFC 7277.
> [Bing] A good suggestion. We'll do it in the next version. Thanks.
> 
> 
> Best regards,
> Bing
> 
> 
>> Hope that helps.
> 
>> 
>> Cheers,
>> Qi
>> 
>> 
>> 
>> 
>> 
>> 
>> _______________________________________________
>> L2tpext mailing list
>> [email protected]
>> https://www.ietf.org/mailman/listinfo/l2tpext

_______________________________________________
L2tpext mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/l2tpext