Subject: [OPSAWG] AD review of draft-ietf-opsawg-vpn-common-08

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Mon, 12 Jul 2021 15:14:51 +0000

Hi,

This is my AD review of draft-ietf-opsawg-vpn-common-08.

Thank you for this document.  Again, just minor comments/suggestions.

1.
In section 3.  Description of the VPN Common YANG Module
"Encapsulation features  such as" -> "Encapsulation features.  Such as"
"Routing features  such as" -> "Routing features.  Such as"

2.
As a very minor comment.  Where you have lists (i.e., encapsulation features, routing features, service type) and particularly because they have references, it may be slightly easier to read if the list was indented.  Also does it make sense for this list to just be examples, or are they actually normative lists of service types that are supported?

E.g., 
   'service-type':  Used to identify the VPN service type.  
      Examples of supported service types are:
	  
	  o L3VPN,
	  
	  o Virtual Private LAN Service (VPLS) using BGP [RFC4761],
	  
	  o VPLS using Label Distribution Protocol (LDP) [RFC4762],
	  
	  o Virtual Private Wire Service (VPWS) [RFC8214],
      
	  o BGP MPLS-Based Ethernet VPN [RFC7432],
	  
	  o Ethernet VPN (EVPN) [RFC8365], and
	  
	  o Provider Backbone Bridging Combined with Ethernet
        VPN (PBB-EVPN) [RFC7623].

In the Yang Module:

3. OPSA => OPSAWG
Please can you check if this needs to be fixed for the L3NM YANG model as well.

4. 
  feature qinany {
    description
      "Indicates the support of the QinAny encapsulation.";
  }
  
Is there a reference, or perhaps a more detailed description that you can use here?

5.
Very minor:  
feature ipv4, feature ipv6, is it worth adding references to the RFCs?  I appreciate that they are obvious, but since you have references for everything else, it seems like it might be worth using adding them?

6.
  feature rtg-ospf-sham-link {
    description
      "Indicates support of OSPF sham links.";
	  
Does this mean that feature rtg-ospf excldues this support?  This feature seems very specific relative to the other features in this YANG module.

7.
As mentioned in the other document, would "feature carrierscarrier" be better as "feature carriers-carrier"?

8.
"Indicates the support of" => "Indicates support for"
"Indicates support of" -> "Indicates support for"

9. 
This model defines a lot of features, and I wasn't sure how helpful that will really be in practice.  Is the intention here for an SP to use features to customize the model to their needs?  I wonder if the heavy use of features won't work so well if both L2VPN and L3VPN's are being modelled and support different protocols/etc.  Will having the common features act as a limitation?  E.g., an alternative might be to express the features in the L2NM and L3NM models directly allowing them to enabled/disable different features.

10. Status leaves:
Would UP, DOWN, UNKNWON be better as Up, Down, Unknown?
Would "admin-enabled" be better than "admin-up" and "admin-disabled" be better than "admin-down".

For all the non-base identities, I would suggest removing the "Identity for" prefix in the descriptions.  It is self-evident that the descriptions are for identities, and the extra words probably would not help a GUI rendering of the description strings.

11. For all the "service type" identities, I would suggest ending all the descriptions with "service".
E.g., 
  "L3VPN service.";
  "Provider Backbone Bridging (PBB) EVPNs service.";
  "Virtual Private LAN Service (VPLS).";
  "Point-to-point Virtual Private Wire Service (VPWS).";
  "Provider Backbone Bridging (PBB) EVPN service.";
  "MPLS based EVPN service.";
  "VXLAN based EVPN service.";
  
12.
For the signalling identity descriptions:
      "Layer 2 VPNs using BGP signalling";
	  "Targeted LDP signalling.";
      "L2TP signalling.";

13.	  
For the bgp-signalling identity, does it make sense for the description to be specific to L2VPNs?  Couldn't bgp-signalling also be used for L3VPNs?

14.   
For the routing-protocol-type generic identities, would it make sense to add a "-routing" suffic to them, e.g., "direct-routing", "any-routing"?  Otherwise some of the identity names look fairly generic, but perhaps that is okay.

15.
vpn-topology:
 Is No P2P topology identity required?

16.
For identity qos-profile-direction:
  Rather than "site-to-wan" and "wan-to-site" identity names, would it be better to use "vpn" or "service" instead of wan.  E.g., "site-to-vpn", or "site-to-service"?

17.
  identity enhanced-vpn
  identity ietf-network-slice

Would it be helpful to add RFC or draft references for these identities?  E.g., [I-D.ietf-teas-enhanced-vpn], or an IETF network slice service [I-D.ietf-teas-ietf-network-slices]

18.
  identity protocol-type {
    description
      "Base identity for Protocol Type.";
  }

  identity unknown {
    base protocol-type;
    description
      "Not known protocol type.";
  }
  
Is this identity required/useful?  Wouldn't it be better to just leave the leaf not populated, but in the general case, shouldn't this always be specified?

19.
  identity encapsulation-type {
  vs  identity tag-type
   - These seem to both effectively convey similar information.
   - c-s-vlan, should probably be s-c-vlan, since it is normal to specify encapsulation from outermost inner.

20.
  identity tf-type
There is no type for unicast.  Is it not required?
  
21.  
   grouping vpn-description {

Which, if any, of these fields are expected to uniquely identify the VPN?
E.g., what, if any, uniqueness requirements are there for vpn-name?

    leaf vpn-id {
      type vpn-id;
      description
        "VPN identifier.
         This identifier has a local meaning.";
    }
	
What is meant by "local meaning", could this be clarified?

22. The vpn-id type seems to be used very generically for quite a few different things, and I was wondering whether having more specific subtypes of vpn-id might be helpful?

23.
What's the difference between service-timestamp and service-status?
Both include timestamps and status.  Perhaps slightly different names for these groupings might make them more consistent (e.g., oper-service-status, admin-oper-service-status).

24.
last-updated is okay, but note that ietf-interfaces.yang uses last-changed instead.  Given the description mentions change, last-change might be better?

25.
I'm not sure that the tree diagram examples in Appendix A is actually that useful, given that they do not represent what the model is now, just how it used to be.  I would suggest keeping the text that justifies the approach taken but remove the trees.

I also annotated part of the YANG model (just the grouping descriptions) with comments inline.  Please see suggestions on (#RW:) inline in the attached file.  It is up to you whether and how you want to incorporate these and I don't need to see your response.

Grammar Warnings (by automated tool):
Section: 3, draft text:
For example, diversity or redundancy constraints can be applied on a per group basis.

Warning:  In this context, per-group forms an adjective and is spelled with a hyphen.
Suggested change:  "per-group"

Regards,
Rob

---

From: <[email-masked]>
Date: Mon, 12 Jul 2021 17:34:36 +0000

Hi Rob, 

Many thanks for the review. A candidate updated version can be seen at: https://tinyurl.com/vpn-common-latest 

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Rob Wilton (rwilton) [mailto:[email-masked]]
> Envoyé : lundi 12 juillet 2021 17:15
> À : [email-masked]
> Cc : [email-masked]
> Objet : AD review of draft-ietf-opsawg-vpn-common-08
> 
> Hi,
> 

[Med] Fixed. 

> 2.
> As a very minor comment.  Where you have lists (i.e., encapsulation
> features, routing features, service type) and particularly because
> they have references, it may be slightly easier to read if the list
> was indented.  Also does it make sense for this list to just be
> examples, or are they actually normative lists of service types that
> are supported?

[Med] Sure. These are only examples. We cite them as we need to list include in the main text any reference quoted in the module. 

> Please can you check if this needs to be fixed for the L3NM YANG

[Med] Fixed. 

> Is there a reference, or perhaps a more detailed description that

[Med] This was also a comment raised in the WGLC, but we don't have any acceptable authoritative reference to cite for it.

> 5.
> Very minor:
> feature ipv4, feature ipv6, is it worth adding references to the
> RFCs?  I appreciate that they are obvious, but since you have
> references for everything else, it seems like it might be worth
> using adding them?

[Med] OK

> Does this mean that feature rtg-ospf excldues this support?  This
> feature seems very specific relative to the other features in this
> YANG module.

[Med] Yes. We are barrowing this one for RFC8299, fyi.

> be better as "feature carriers-carrier"?

[Med] No problem. Fixed.  

> "Indicates support of" -> "Indicates support for"

[Med] Fixed. 

> 
> 9.
> This model defines a lot of features, and I wasn't sure how helpful
> that will really be in practice.  Is the intention here for an SP to
> use features to customize the model to their needs?  I wonder if the
> heavy use of features won't work so well if both L2VPN and L3VPN's
> are being modelled and support different protocols/etc.  Will having
> the common features act as a limitation?  E.g., an alternative might
> be to express the features in the L2NM and L3NM models directly
> allowing them to enabled/disable different features.

[Med] The "common" set of features was initially included to rationalize the LxSM and LxNM. I don't know if/and to what extent this would have limitations when both L2xM and L3xM.  

> Would UP, DOWN, UNKNWON be better as Up, Down, Unknown?

[Med] Fixed. 

> be better than "admin-down".

[Med] I prefer the OLD as it is short + not problematic when including examples and make no folding is used (please trust me, that's a nightmare). 

> 
> For all the non-base identities, I would suggest removing the
> "Identity for" prefix in the descriptions.  It is self-evident that
> the descriptions are for identities, and the extra words probably
> would not help a GUI rendering of the description strings.

[Med] Good suggestion.

>   "VXLAN based EVPN service.";

[Med] OK.

> 	  "Targeted LDP signalling.";

[Med] Fixed. 

> 
> 13.
> For the bgp-signalling identity, does it make sense for the
> description to be specific to L2VPNs?  Couldn't bgp-signalling also
> be used for L3VPNs?

[Med] RFC4364 says:

" 
     - DOES NOT require that there be any explicit setup of the tunnels,
       either via signaling or via manual configuration;

     - DOES NOT require that there be any tunnel-specific signaling; "

> 
> 14.
> For the routing-protocol-type generic identities, would it make
> sense to add a "-routing" suffic to them, e.g., "direct-routing",
> "any-routing"?  Otherwise some of the identity names look fairly
> generic, but perhaps that is okay.
> 

[Med] OK to add the "-routing"  suffix. 

>  Is No P2P topology identity required?

[Med] The VPN topology is more related to how the site can communicate with each other. We are using the traditional roles: hub, spoke, etc. 

>   Rather than "site-to-wan" and "wan-to-site" identity names, would
> it be better to use "vpn" or "service" instead of wan.  E.g., "site-
> to-vpn", or "site-to-service"?

[Med] I prefer the old name, as sites can be considered by some as "part" of the VPN. 

> Would it be helpful to add RFC or draft references for these
> identities?  E.g., [I-D.ietf-teas-enhanced-vpn], or an IETF network
> slice service [I-D.ietf-teas-ietf-network-slices]

[Med] We don't cite those as they are not "stable" pointers yet. 

> Is this identity required/useful?  Wouldn't it be better to just
> leave the leaf not populated, but in the general case, shouldn't
> this always be specified?

[Med] that's OK for the write operations. What we had in mind is more read operations to report that the underlying transport is not known to the controller. 

>   identity encapsulation-type {

[Med] this one is about the encapsulation type. 

>   vs  identity tag-type

[Med] this is more about the tag type (c, s, etc.)

>    - c-s-vlan, should probably be s-c-vlan, since it is normal to
> specify encapsulation from outermost inner.
> 

[Med] We used c-s to follow the order in which c-* and s-* identities are listed. I don't have a preference. Changed to s-c-vlan

> There is no type for unicast.  Is it not required?

[Med] We are not defining it here as the main case we have for this is the so called "bum" (Broadcast, Unknown Unicast, or Multicast). 

> E.g., what, if any, uniqueness requirements are there for vpn-name?

[Med] It is the vpn-id. Update to make this clear in the module: 

"A VPN identifier that uniquely identifies a VPN"

> What is meant by "local meaning", could this be clarified?

[Med] Updated to: 

         This identifier has a local meaning, e.g., within 
         a service provider network.

> 
> 
> 22. The vpn-id type seems to be used very generically for quite a

[Med] Yes. 

> few different things, and I was wondering whether having more
> specific subtypes of vpn-id might be helpful?

[Med] Not sure this is useful as we don't associate any specific "structure". 

> 
> 
> 23.
> What's the difference between service-timestamp

[Med] This is about operational status. Updated to oper-service-timestamp

 and service-status?

[Med] This covers both admin and oper status. 

> Both include timestamps and status.  Perhaps slightly different
> names for these groupings might make them more consistent (e.g.,
> oper-service-status, admin-oper-service-status).
> 
> 24.
> last-updated is okay, but note that ietf-interfaces.yang uses last-
> changed instead.  Given the description mentions change, last-change
> might be better?

[Med] Went for "last-change". 

> 
> 25.
> I'm not sure that the tree diagram examples in Appendix A is
> actually that useful, given that they do not represent what the
> model is now, just how it used to be.  I would suggest keeping the
> text that justifies the approach taken but remove the trees.

[Med] That's a good input. 

> 
> I also annotated part of the YANG model (just the grouping
> descriptions) with comments inline.  Please see suggestions on
> (#RW:) inline in the attached file.  It is up to you whether and how
> you want to incorporate these and I don't need to see your response.
> 

[Med] Thanks. I incorporated almost all your suggestions. 

> Suggested change:  "per-group"

[Med] Fixed.

> Rob

_________________________________________________________________________________________________________________________

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.

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Tue, 13 Jul 2021 14:24:44 +0000

Hi Med,

Please see inline, just a couple of points to resolve ...

> -----Original Message-----
> From: [email-masked]
> <[email-masked]>
> Sent: 12 July 2021 18:35
> To: Rob Wilton (rwilton) <[email-masked]>; draft-ietf-opsawg-vpn-
> [email-masked]
> Cc: [email-masked]
> Subject: RE: AD review of draft-ietf-opsawg-vpn-common-08
> 
> Hi Rob,
> 

Okay.

> [Med] Sure. These are only examples. We cite them as we need to list include
> in the main text any reference quoted in the module.

Okay.

> > Please can you check if this needs to be fixed for the L3NM YANG

Okay.

> [Med] This was also a comment raised in the WGLC, but we don't have any
> acceptable authoritative reference to cite for it.

Okay, then perhaps give a brief description of what QinAny means, i.e., in this context I think that frames have a pair of VLAN Ids, where the outer VLAN Id is fixed, but the inner VLAN Id can take any valid VLAN Id value.

> > references for everything else, it seems like it might be worth

Okay.

> [Med] Yes. We are barrowing this one for RFC8299, fyi.

Okay, makes sense.

> > be better as "feature carriers-carrier"?

Okay.

> > "Indicates support of" -> "Indicates support for"

Okay.

> [Med] The "common" set of features was initially included to rationalize the
> LxSM and LxNM. I don't know if/and to what extent this would have
> limitations when both L2xM and L3xM.

Okay.

> > Would UP, DOWN, UNKNWON be better as Up, Down, Unknown?

Okay.

> [Med] I prefer the OLD as it is short + not problematic when including
> examples and make no folding is used (please trust me, that's a nightmare).

I'm not sure that I find that as a compelling reason. :-)

I presume that you aware of Kent's RFC and script for folding/unfolding instance data examples?

> > would not help a GUI rendering of the description strings.

Okay.

> >   "VXLAN based EVPN service.";

Okay.

> > 	  "Targeted LDP signalling.";

Okay.

>      - DOES NOT require that there be any tunnel-specific signaling; "

Okay.

> [Med] The VPN topology is more related to how the site can communicate
> with each other. We are using the traditional roles: hub, spoke, etc.
> 

Okay, but at least for L2VPN services, a P2P connection (VPWS) is also quite common, no? 

> [Med] I prefer the old name, as sites can be considered by some as "part" of

Okay.

> [Med] We don't cite those as they are not "stable" pointers yet.

I think that it is useful to include those as informative references, or perhaps otherwise leave out these two identities.  It feels strange to include them if they have no defined meaning.

> [Med] that's OK for the write operations. What we had in mind is more read
> operations to report that the underlying transport is not known to the
> controller.

Okay.

> [Med] this one is about the encapsulation type.

Okay.

> [Med] We used c-s to follow the order in which c-* and s-* identities are
> listed. I don't have a preference. Changed to s-c-vlan

Okay.

> [Med] We are not defining it here as the main case we have for this is the so
> called "bum" (Broadcast, Unknown Unicast, or Multicast).

Okay.

>          a service provider network.

Okay.

> [Med] Not sure this is useful as we don't associate any specific "structure".

E.g., my comment was in the context of when vpn-id is used at the data type for port-id in the L3VPN model.

> [Med] This is about operational status. Updated to oper-service-timestamp

Okay.

> [Med] This covers both admin and oper status.

Okay.

> [Med] Went for "last-change".

Okay.

> [Med] That's a good input.

Okay.

> [Med] Thanks. I incorporated almost all your suggestions.

Okay.

> > Suggested change:  "per-group"

Thanks,
Rob

> 
> >
> > Regards,
> > Rob
> 
> ________________________________________________________________
> _________________________________________________________
> 
> Ce message et ses pieces jointes peuvent contenir des informations
> confidentielles ou privilegiees et ne doivent donc
> a l'expediteur et le detruire ainsi que les pieces jointes. Les messages
> electroniques etant susceptibles d'alteration,
> This message and its attachments may contain confidential or privileged
> information that may be protected by law;

---

From: <[email-masked]>
Date: Tue, 13 Jul 2021 15:16:34 +0000

Re-,

I updated the file to take into account your feedback:

* Module: https://github.com/IETF-OPSAWG-WG/lxnm/commit/fcc9f6c1e79d5074469fe454d95770fe9f1becdb (in addition to the changes already reported as part of the L3NM review).
* I-D: https://tinyurl.com/vpn-common-latest 

Unless you have further comments, I will contact the secretariat to publish this version. Thanks.  

Please see inline.

Cheers,
Med

> Envoyé : mardi 13 juillet 2021 16:25
> À : BOUCADAIR Mohamed INNOV/NET <[email-masked]>;
> [email-masked]
> Cc : [email-masked]
> Objet : RE: AD review of draft-ietf-opsawg-vpn-common-08
> 
> Hi Med,
> 
> > Many thanks for the review. A candidate updated version can be
> seen at:
> > https://tinyurl.com/vpn-common-latest
> >
> > Please see inline.
> >
> > Cheers,
> > Med
> >
> > > De : Rob Wilton (rwilton) [mailto:[email-masked]] Envoyé :
> lundi
> > > 12 juillet 2021 17:15 À : draft-ietf-opsawg-vpn-
> [email-masked]
> > > Cc : [email-masked]
> > > "Encapsulation features  such as" -> "Encapsulation features.
> Such
> > > as"
> > [Med] Sure. These are only examples. We cite them as we need to
> list
> > include in the main text any reference quoted in the module.
> 
> Okay.
> 
> 
> >
> > >
> > > E.g.,
> > > Is there a reference, or perhaps a more detailed description
> that
> > > you can use here?
> > >
> >
> > [Med] This was also a comment raised in the WGLC, but we don't
> have
> > any acceptable authoritative reference to cite for it.
> 
> Okay, then perhaps give a brief description of what QinAny means,
> i.e., in this context I think that frames have a pair of VLAN Ids,
> where the outer VLAN Id is fixed, but the inner VLAN Id can take any
> valid VLAN Id value.

[Med] Added a note to the module.

> > > As mentioned in the other document, would "feature
> carrierscarrier"
> > > be better as "feature carriers-carrier"?
> >
> > [Med] No problem. Fixed.
> 
> Okay.
> 
> >
> > >
> > > 8.
> > [Med] The "common" set of features was initially included to
> > rationalize the LxSM and LxNM. I don't know if/and to what extent
> this
> > would have limitations when both L2xM and L3xM.
> 
> Okay.
> 
> 
> >
> > >
> > > 10. Status leaves:
> > > Would "admin-enabled" be better than "admin-up" and "admin-
> disabled"
> > > be better than "admin-down".
> >

[Med] That's may laziness then. 

> 
> I presume that you aware of Kent's RFC and script for
> folding/unfolding instance data examples?

[Med] I can't say no as we use it in the L2NM :-) 

The readability is still superior when we don't use the folding. 

> > > 11. For all the "service type" identities, I would suggest
> ending
> > > all the descriptions with "service".
> > > E.g.,
> > >   "L3VPN service.";

[Med] Yeah, but that is an intrinsic characteristic of VPWS. Setting "vpn-type" to "vpws" is sufficient in such case.   

> I think that it is useful to include those as informative
> references, or perhaps otherwise leave out these two identities.  It
> feels strange to include them if they have no defined meaning.

[Med] They are already listed as informative references. As I may not advance the laziness argument again, I added the I-D references to the module :-)

> > [Med] that's OK for the write operations. What we had in mind is
> more
> > read operations to report that the underlying transport is not
> known
> > to the controller.
> 
> Okay.
> 
> 
> >
> > >
> > > 19.
> > [Med] We used c-s to follow the order in which c-* and s-*
> identities
> > are listed. I don't have a preference. Changed to s-c-vlan
> 
> Okay.
> 
> >
> > > 20.
> > >   identity tf-type
> > [Med] We are not defining it here as the main case we have for
> this is
> > the so called "bum" (Broadcast, Unknown Unicast, or Multicast).
> 
> Okay.
> 
> >
> > >
> > > 21.
> > > Which, if any, of these fields are expected to uniquely identify
> the
> > > VPN?
> E.g., my comment was in the context of when vpn-id is used at the
> data type for port-id in the L3VPN model.

[Med] OK, thanks. I think this is now fixed with the changes reported in the L3NM.

> > > For example, diversity or redundancy constraints can be applied
> on a
> > > per group basis.
> > >

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Tue, 13 Jul 2021 16:35:45 +0000

Hi Med,

[Just to also close this thread/review.]

I think that you are good to go.

Thanks for accommodating my comments.

Regards,
Rob

> Sent: 13 July 2021 16:17
> * Module: https://github.com/IETF-OPSAWG-
> WG/lxnm/commit/fcc9f6c1e79d5074469fe454d95770fe9f1becdb (in addition
> to the changes already reported as part of the L3NM review).
> [Med] They are already listed as informative references. As I may not
> advance the laziness argument again, I added the I-D references to the
> module :-)
> 
> >
> > >
> > > >
> > > > 18.
> > > >   identity protocol-type {
> > > >     description