Re: [L3sm] RFC 8049 on YANG Data Model for L3VPN Service Delivery

[<[e-mail]>]

Hi,

Pls find inline comments.


-----Original Message-----
From: L3sm [mailto:[e-mail]] On Behalf Of Ogaki, Kenichi
Sent: Thursday, March 16, 2017 10:26
To: 'Jan Lindblad'; 'Benoit Claise'
Cc: [e-mail]; [e-mail]
Subject: Re: [L3sm] RFC 8049 on YANG Data Model for L3VPN Service Delivery

Hi Jan, All,

See comments inline [KO], please.
Stephane, Luis, could you correct me if I misunderstand? 

Thanks,
Kenichi

-----Original Message-----
From: L3sm [mailto:[e-mail]] On Behalf Of Jan Lindblad
Sent: Thursday, March 09, 2017 6:48 PM
To: Benoit Claise
Cc: [e-mail]; [e-mail]
Subject: Re: [L3sm] RFC 8049 on YANG Data Model for L3VPN Service Delivery

Benoît, all,

> On 9 Mar 2017, at 20:09, Benoit Claise <[e-mail]> wrote:
> 
> Dear all,
> 
> It seems like Jan discovered a bug.
> Can the authors confirm?

Since the issue I found was in a quick scan, I decided to go through the entire document in a little more ordered fashion. My findings are below. Issue #1 is what I reported earlier. I am truly sorry I did not go through the YANG before the release, but there isn't much to do about that now.


I was interested in the final result, so I took a quick look at the model. Within a couple of minutes I stumbled upon this, which is almost certainly not what the author had intended.

#1)

1995: grouping site-devices /l3vpn-svc/sites/site
  container devices {
   must "/l3vpn-svc/sites/site/management/type = "+
    "'provider-managed' or "+
    "/l3vpn-svc/sites/site/management/type ="+
    "'co-managed'" {

To me, it appears that the author intended to say that the "container device" configuration is only applicable when the current site management type is 'provider-managed' or 'co-managed'. That would make sense. 

The use of "must" and the use of the absolute paths above changes the meaning in YANG to "as soon as any l3vpn is configured, there must be at least one l3vpn (not necessarily the current one) that is 'provider-managed' or 'co-managed'." One implication would be that it would not be legal to have a collection of only 'customer-managed' vpns. What would be legal, though, is to have one 'provider-managed' l3vpn with no device configuration and another 'customer-managed' with devices configured. I expect this was not the intention.

I think an expression that reflects the intent would be

  container devices {
   when "../management/type = "+
    "'provider-managed' or "+
    "/l3vpn-svc/sites/site/management/type ="+
    "'co-managed'" {

This would make the container only exist when the management type of this site specifically is 'provider-managed' or 'co-managed'.

[KO] Martin's following modification seems correct.
[SLI] I also agree with Martin's proposal.

>   container devices {
>    when "../management/type = 'provider-managed' or "
>       + "../management/type = 'co-managed'" {


Now I went through the YANG and discovered a couple more similar things, as well as a slew of optional leafs where the meaning of a non-existent leaf is undefined. 

#2)

2011: grouping site-devices /l3vpn-svc/sites/site/devices/device/ 
    leaf location {
     type leafref {
      path "/l3vpn-svc/sites/site/locations/"+
       "location/location-id";

The location and name of the grouping in which 'leaf location' is defined makes me believe the intent is to only allow pointers to locations within the current site. Currently this leafref allows pointing to any location in any site (as well as no reference at all). If my assumption about the modeler's intent is right, the path should be

      path "../../../locations/location/location-id";

[KO] Basically agree. However, a service provider may allow his/her customer to use two or more l3vpn-svc instances and refer their parameters from each other.

[SLI] I would tend to limit to the objects created in the current site.


#3)

On line 2019 there is another must statement which wants to be a when, and the path is a little too liberal, I'd think.

2019: grouping site-devices /l3vpn-svc/sites/site/devices/device
    container management {
     must "/l3vpn-svc/sites/site/management/type"+
      "= 'co-managed'" {

I think the modeler's intent should be

     must "../../../management/type = 'co-managed'" {

[KO] Agree.
[SLI] Agree.


#4)

There's probably nothing wrong with this one, but since my confidence in guessing the modeler's intent is low here, and there are many previous leafref mistakes in this module, maybe someone should assert that this is what's intended

2139: grouping site-vpn-policy /l3vpn-svc/sites/site
      leaf vpn-id {
       type leafref {
        path "/l3vpn-svc/vpn-services/vpn-svc/vpn-id";
       }

[OK] The current path is correct. Not "vpn-svc", but "vpn-service".

#5)

Again, my guess is that the modeler would like to allow references to site-network-access objects from within the current site rather than in any site.

2395:/l3vpn-svc/sites/site/site-network-accesses/site-network-access grouping access-vpn-policy
     leaf vpn-policy-id {
      type leafref {
       path "/l3vpn-svc/sites/site/"+
       "vpn-policy-list/vpn-policy/"+
       "vpn-policy-id";
      }

If so, the leafref path should be

       path "../../../vpn-policy-list/vpn-policy/vpn-policy-id";

[KO] We need one more "../" and s/vpn-policy-list/vpn-policies/ .
       path "../../../../vpn-policies/vpn-policy/vpn-policy-id";


#6)

Assuming I'm right in my suspicion that the current module has errors as described above, I'd say they are grave enough to require fixing. On a far lower severity, there are lot's of leafs and choices which are optional, but declare nothing about any meaning if absent. Probably many of them should be made mandatory or have a default. Most of the rest should probably have a sentence or two added to the description. Below I have collected the line numbers and optional element names where I honestly don't know what the system would do if left without a value. It may be that l3vpn experts would know in some cases, but it wouldn't hurt to spell out.

Not fixing these undefined cases limits the interoperability, IMO.

629: leaf nat-enabled
653: choice group-format
757: leaf rp-address
966: choice target-flavor
1149:leaf svc-input-bandwidth
1156:leaf svc-output-bandwidth
1163:leaf svc-mtu
1177:leaf enabled
1194:leaf signalling-type
1237:choice match-type
1272:choice qos-profile
1362:leaf enabled
1367:leaf layer
1382:choice profile
1504:leaf area-address
1509:leaf metric
1526:leaf metric
1550:leaf autonomous-system
1555:leaf-list address-family
1639:leaf-list address-family
1662:leaf-list address-family
1742:leaf provider-address
1747:leaf customer-address
1752:leaf mask
1803:leaf-list server-ip-address
1821:leaf provider-address
1826:leaf customer-address
1831:leaf mask
1850:leaf bfd-enabled
1856:choice holdtime
1979:leaf type
2052:leaf site-vpn-flavor
2087:choice lan
2280:leaf site1
2285:leaf site2
2311:leaf src-site
2316:leaf dst-site
2349:leaf local-sites-role
2406:leaf vpn-id


Best Rergards,
/jan

_______________________________________________
L3sm mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/l3sm

_______________________________________________
L3sm mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/l3sm

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.