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

[[e-mail]]

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

> -----Message d'origine-----
> De : Rob Wilton (rwilton) [mailto:[e-mail]]
> Envoyé : mardi 13 juillet 2021 16:25
> À : BOUCADAIR Mohamed INNOV/NET <[e-mail]>;
> [e-mail]
> Cc : [e-mail]
> Objet : RE: AD review of draft-ietf-opsawg-vpn-common-08
> 
> Hi Med,
> 
> Please see inline, just a couple of points to resolve ...
> 
> > -----Original Message-----
> > From: [e-mail]
> > <[e-mail]>
> > Sent: 12 July 2021 18:35
> > To: Rob Wilton (rwilton) <[e-mail]>; draft-ietf-opsawg-
> vpn-
> > [e-mail]
> > Cc: [e-mail]
> > Subject: RE: AD review of draft-ietf-opsawg-vpn-common-08
> >
> > 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:[e-mail]] Envoyé :
> lundi
> > > 12 juillet 2021 17:15 À : draft-ietf-opsawg-vpn-
> [e-mail]
> > > Cc : [e-mail]
> > > Objet : AD review of draft-ietf-opsawg-vpn-common-08
> > >
> > > 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"
> > >
> >
> > [Med] Fixed.
> 
> 
> Okay.
> 
> >
> > > 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.
> 
> Okay.
> 
> 
> >
> > >
> > > 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.
> >
> > [Med] Fixed.
> 
> Okay.
> 
> >
> > >
> > > 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?
> > >
> >
> > [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.

> 
> 
> >
> > > 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
> 
> Okay.
> 
> >
> > >
> > > 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.
> >
> > [Med] Yes. We are barrowing this one for RFC8299, fyi.
> 
> Okay, makes sense.
> 
> 
> >
> > >
> > > 7.
> > > As mentioned in the other document, would "feature
> carrierscarrier"
> > > be better as "feature carriers-carrier"?
> >
> > [Med] No problem. Fixed.
> 
> Okay.
> 
> >
> > >
> > > 8.
> > > "Indicates the support of" => "Indicates support for"
> > > "Indicates support of" -> "Indicates support for"
> >
> > [Med] Fixed.
> 
> Okay.
> 
> >
> > >
> > > 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.
> 
> Okay.
> 
> 
> >
> > >
> > > 10. Status leaves:
> > > Would UP, DOWN, UNKNWON be better as Up, Down, Unknown?
> >
> > [Med] Fixed.
> 
> Okay.
> 
> >
> > > Would "admin-enabled" be better than "admin-up" and "admin-
> disabled"
> > > 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).
> 
> I'm not sure that I find that as a compelling reason. :-)

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

> 
> 
> 
> >
> > >
> > > 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.
> 
> Okay.
> 
> 
> >
> > >
> > > 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.";
> > >
> >
> > [Med] OK.
> 
> Okay.
> 
> >
> > > 12.
> > > For the signalling identity descriptions:
> > >       "Layer 2 VPNs using BGP signalling";
> > > 	  "Targeted LDP signalling.";
> > >       "L2TP signalling.";
> >
> > [Med] Fixed.
> 
> Okay.
> 
> >
> > >
> > > 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; "
> 
> Okay.
> 
> 
> >
> > >
> > > 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.
> >
> > > 15.
> > > vpn-topology:
> > >  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.
> >
> 
> Okay, but at least for L2VPN services, a P2P connection (VPWS) is
> also quite common, no?

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

> 
> > >
> > > 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"?
> >
> > [Med] I prefer the old name, as sites can be considered by some as
> > "part" of the VPN.
> 
> Okay.
> 
> >
> > >
> > > 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]
> >
> > [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] 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
> > >       "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?
> >
> > [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.
> > >   identity encapsulation-type {
> >
> > [Med] this one is about the encapsulation type.
> >
> > >   vs  identity tag-type
> 
> Okay.
> 
> >
> > [Med] this is more about the tag type (c, s, etc.)
> >
> > >    - 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.
> > >
> >
> > [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
> > > 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).
> 
> Okay.
> 
> >
> > >
> > > 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?
> >
> > [Med] It is the vpn-id. Update to make this clear in the module:
> >
> > "A VPN identifier that uniquely identifies a VPN"
> >
> > >
> > >
> > >     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?
> >
> > [Med] Updated to:
> >
> >          This identifier has a local meaning, e.g., within
> >          a service provider network.
> 
> Okay.
> 
> >
> > >
> > >
> > > 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".
> 
> 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.

> 
> 
> 
> >
> > >
> > >
> > > 23.
> > > What's the difference between service-timestamp
> >
> > [Med] This is about operational status. Updated to
> > oper-service-timestamp
> >
> 
> Okay.
> 
> >  and service-status?
> >
> > [Med] This covers both admin and oper status.
> 
> Okay.
> 
> >
> > > 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".
> 
> Okay.
> 
> >
> > >
> > > 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.
> 
> Okay.
> 
> >
> > >
> > > 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.
> 
> Okay.
> 
> >
> > > 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"
> >
> > [Med] Fixed.
> 
> Thanks,
> Rob
> 
> 
> >
> > >
> > > Regards,
> > > 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.