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

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Med,

[Just to also close this thread/review.]

I think that you are good to go.

Thanks for accommodating my comments.

Regards,
Rob


> -----Original Message-----
> From: [e-mail]
> <[e-mail]>
> Sent: 13 July 2021 16:17
> 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
> 
> 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.