Hi Med,

On Wed, Sep 22, 2021 at 09:40:17AM +0000, mohamed.boucad...@orange.com wrote:
> Hi Ben, 
> 
> Thank you for the review. The changes to address your review can be tracked 
> at: 
> https://github.com/IETF-OPSAWG-WG/lxnm/commit/108707e8b2aea6590b0a9695756c7546887c9614
>  

Thanks.  Also inline...


> 
> Please see inline. 
> 
> Cheers,
> Med
> 
> > -----Message d'origine-----
> > De : Benjamin Kaduk via Datatracker [mailto:nore...@ietf.org]
> > Envoyé : mercredi 22 septembre 2021 06:19
> > À : The IESG <i...@ietf.org>
> > Cc : draft-ietf-opsawg-vpn-com...@ietf.org; opsawg-cha...@ietf.org;
> > opsawg@ietf.org; adr...@olddog.co.uk; adr...@olddog.co.uk
> > Objet : Benjamin Kaduk's No Objection on draft-ietf-opsawg-vpn-common-
> > 10: (with COMMENT)
> > 
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-opsawg-vpn-common-10: No Objection
> > 
> > When responding, please keep the subject line intact and reply to all
> > email addresses included in the To and CC lines. (Feel free to cut this
> > introductory paragraph, however.)
> > 
> > 
> > Please refer to https://www.ietf.org/blog/handling-iesg-ballot-
> > positions/
> > for more information about how to handle DISCUSS and COMMENT positions.
> > 
> > 
> > The document, along with other ballot positions, can be found here:
> > https://datatracker.ietf.org/doc/draft-ietf-opsawg-vpn-common/
> > 
> > 
> > 
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> > 
> > I have a bold proposal for consideration: since we are defining a new
> > common set of groupings for VPN and, in some cases, proposing to rename
> > existing terminology already, can we consider moving away from the term
> > "VPN" itself?  The word "private" is ambiguous as to what level of
> > privacy is being provided (e.g., network routing vs cryptographic) and
> > who the conveyed content is to be private from.  A term like "virtual
> > network" removes that ambiguity, and lets us use the explicit attributes
> > to convey whether encryption is in use when appropriate.
> > 
> > There is no particular triggering point (at least, not yet) at which it
> > becomes clearly correct to make a breaking change like this, so we may
> > end up just having to pick a time somewhat arbitrarily, if we are to
> > make such a change at all.  Perhaps now is that time; perhaps not.
> 
> [[Med]] I hear some of your observations but VPN is a well-established name 
> for a widely deployed technology. I don't think it is a good idea to touch 
> this. Please note that there is already generic model for VNs 
> (https://datatracker.ietf.org/doc/draft-ietf-teas-actn-vn-yang/) that is 
> distinct from what we are doing here.

Well, I don't think that just because VPN is well-established we should
ignore the ambiguity in what it describes.  So, I still want to touch it at
some point, but do not insist on doing so right now.  I am sure that we can
find some non-VPN term other than VN if there is desire to
distinguish...but we can defer that conversation, of course.

> > 
> > Section 3
> > 
> >           grouping vpn-profile-cfg
> >             +-- valid-provider-identifiers
> >                +-- external-connectivity-identifier* [id]
> >                |       {external-connectivity}?
> >                |  +-- id?   string
> > 
> > Presumably I am just misreading RFC 8340, but I thought that the list
> > key was implicitly mandatory, so it would appear as just "id" (as
> > opposed to "id?").  (Many instances.)
> 
> [[Med]] Good question. Actually we trusted what is in Section 3.2 of 8340. 
> That's the output we get from pyang. Note that ? is not displayed when the 
> grouping is used in another model (L2NM/L3NM). Will double check this with 
> Martin.  

Thanks.

> > 
> >    'vpn-route-targets':
> >       *  A YANG grouping that defines Route Target (RT) import/export
> >          rules used in a BGP-enabled VPN (e.g., [RFC4364][RFC4664]).
> > 
> > On very quick skim, it's not clear what motivates the RFC 4664 reference
> > -- "route target" does not appear, for example, nor does "import" or
> > "export".
> 
> [[Med]] This is just to insist that it can be used for both L3 and L2VPNs. 
> Unlike L3NM, there are plenty L2VPN technologies; hence the choice to point 
> to a generic L2 framework.

Okay, so the references are just generic "VPN" (or "BGP-enabled VPN")
references, and I was misreading to ascribe more meaning to them.

If we wanted to spend a few more words (far from clear), maybe "(suitable
VPN technologies include but are not limited to [RFC4364][RFC4664])".

> > 
> > Section 4
> > 
> >      identity pim-proto {
> >        if-feature "pim";
> >        base routing-protocol-type;
> > 
> > This is in the section with the group-management-protocols; is "routing-
> > protocol-type" really the intended base?
> 
> [[Med]] Yes. Both PIM and other group management protocols fall under:
> 
>   /*
>    * Identities related to multicast
>    */ 
> 
> > 
> >      identity embb {
> >      [...]
> >      identity urllc {
> >      [...]
> >      identity mmtc {
> > 
> > Similar to Roman's comment, a reference seems useful for these.
> > If we intend to implicitly rely on RFCs 8299 and/or 8466 for
> > terminology, we should say so in the terminology section.
> 
> [[Med]] Added this note under the terminology section:
> 
>    The document inherits many terms from [RFC8299] and [RFC8466]
>    (e.g., Enhanced Mobile Broadband (eMBB), Ultra-Reliable and Low
>    Latency Communications (URLLC), Massive Machine Type Communications
>    (mMTC)).

Thanks!

> > 
> >        leaf vpn-id {
> >          type vpn-common:vpn-id;
> >          description
> >            "A VPN identifier that uniquely identifies a VPN.
> >             This identifier has a local meaning, e.g., within
> >             a service provider network.";
> > 
> > Thank you for indicating the scope of validity of the identifier!
> > (Elsewhere as well.)
> > 
> >      grouping service-status {
> >        [...]
> >            leaf last-change {
> >              type yang:date-and-time;
> >              description
> >                "Indicates the actual date and time of the service status
> >                 change.";
> > 
> > What's the motiviation behind leaving this as "config true"?  When would
> > it be useful to set it manually?
> 
> [[Med]] This can be set, for example, when a new administrative status is 
> set. 

But would it be set to a time other than the time when the new
administrative status was set, i.e., a value that can be automatically
determined?

> > 
> >        list vpn-target {
> >          [...]
> >          leaf id {
> >            type int8;
> >            description
> >              "Identifies each VPN Target.";
> > 
> > I wasn't able to find the motivation for limiting to int8 in RFC 4364,
> > though I mostly assume I'm just looking in the wrong place.
> 
> [[Med]] I confirm that there is no such concept in 4364. The intuitive design 
> would be to use leaf-list, but the reason for having this is a list with an 
> id is recorded in this note:
> 
>          Note that this is modelled as a list to ease the reuse of this
>          grouping in modules where a pointer is needed (e.g., associate
>          an operator with RTs).
> 
> We don't see the need to go beyond uint8. As illustrated, for example, in 
> L3NM a typical usage of this would be simply:

Okay.  I am not sure if "pointer" or "index" is better for conveying the
desired meaning ("pointer" might suggest "leafref" to some readers).

> ==
>                          "vpn-targets": {
>                            "vpn-target": [
>                              {
>                                "id": "1",
>                                "route-targets": [
>                                  "0:65550:1"
>                                ],
>                                "route-target-type": "both"
>                              }
>                            ]
>                          }
> ==
> 
> > (But why *signed* int8?)
> 
> [[Med]] This should be unsigned. Fixed. 
> 
> > 
> > Section 5
> > 
> > While there may be no direct security considerations from what is
> > effectively just defining some new compound types for reuse, I think we
> > may want to consider some privacy considerations.  For example, we have
> > the "customer-name" field in the vpn-description, and it is sometimes
> > the case that customers want to remain confidential.  Thus, any
> > instantiations of the grouping would incur a potential need for
> > confidentiality and minimizing the scope of distribution.
> > 
> 
> [[Med]] We do already discuss this for example in the L3NM. No problem to add 
> a note here as well. I added this NEW text:
> 
>    Modules that use the groupings that are defined in this document
>    should identify the corresponding security considerations.  For
>    example, reusing some of these groupings will expose privacy-related
>    information (e.g., customer-name).  Disclosing such information may
>    be considered as a violation of the customer-provider trust
>    relationship. 

Thanks, that looks good.

> 
> > NITS
> > 
> > Section 4
> > 
> >      feature bearer-reference {
> >        description
> >          "Indicates support for the bearer reference access constraint.
> >           That is, the reuse of a network connection that was already
> >           ordered to the service provider apart from the IP VPN site.";
> > 
> > I echo Roman's comment that this feature would benefit from a reference
> > or further discussion; the current description leaves me unclear on what
> > is meant and with no recourse for learning more.  (RFCs 4026 and 4176
> > are presented as generic references for VPN-related terms, but do not
> > cover the concept of "bearer reference".)
> 
> [[Med]] The description is basically echoing RFC8299: 
> 
> ==
>    o  The "bearer-reference" parameter is used in cases where the
>       customer has already ordered a network connection to the SP apart
>       from the IP VPN site and wants to reuse this connection.
> == 
> 
> We can update the description to first introduce the concept of bearer:
> 
>       "A bearer refers to properties of the CE-PE attachment that
>        are below Layer 3.

Thanks; combined with the note in the intro about 8299 terminology that
should be enough for people to figure it out.

And thanks for the other updates as well,

Ben

> > 
> >        reference
> >          "I-D.ietf-teas-ietf-network-slice-framework:
> >             Framework for IETF Network Slices";
> > 
> > draft-ietf-teas-ietf-network-slice-framework is replaced by draft-ietf-
> > teas-ietf-network-slices.
> 
> [[Med]] Shame on me as I'm contributing to that work :-( Fixed. Thanks.
> 
> > 
> >      identity rsvp-te {
> >        base protocol-type;
> >        description
> >          "Transport is based on RSVP-TE.";
> >        reference
> >          "RFC 3209: RSVP-TE: Extensions to RSVP for LSP Tunnels";
> > 
> > Is the transport itself RSVP-TE, or is it that RSVP-TE is used to
> > provision the tunnels used as transport?  (Similar question for bgp-lu?)
> 
> [[Med]] This means "relies upon" to setup the underlay transport. 
> 
> OLD:
>   Transport is based on
> NEW:
>  Transport setup relies upon
> 
> > 
> >      identity dot1q {
> >        if-feature "dot1q";
> >        base encapsulation-type;
> >        description
> >          "Dot1q encapsulation.";
> > 
> > I suppose the feature declaration does so, but maybe some "802" is in
> > order here as well?
> 
> [[Med]] We didn't as we though this is redundant with the reference under the 
> dot1q feature:
> 
>     reference
>       "IEEE Std 802.1Q: Bridges and Bridged Networks"; 
> 
> > 
> >      identity ethernet-type {
> >        base encapsulation-type;
> >        description
> >          "Ethernet encapsulation type.";
> >      }
> > 
> >      identity vlan-type {
> >        base encapsulation-type;
> >        description
> >          "VLAN encapsulation.";
> >      }
> > 
> >      identity untagged-int {
> >        base encapsulation-type;
> >        description
> >          "Untagged interface type.";
> >      [...]
> > 
> > Should we be more consistent about whether the description ends in
> > "type"?
> 
> [[Med]] Indeed. Fixed. 
> 
> > 
> >      identity lag-int {
> >        if-feature "lag-interface";
> >        base encapsulation-type;
> >        description
> >          "LAG interface type.";
> >        reference
> >          "IEEE Std. 802.1AX: Link Aggregation";
> > 
> > lag-int is the only encapsulation-type identify element that has a
> > reference stanza.  We did mention LAG in the context of 802.1AX earlier,
> > so maybe it's not needed?
> 
> [[Med]] Right. We should be consistent. 
> 
> > 
> >      identity web {
> >        base customer-application;
> >        description
> >          "Web applications (e.g., HTTP, HTTPS).";
> >      [...]
> >      identity file-transfer {
> >        base customer-application;
> >        description
> >          "File transfer application (e.g., FTP, SFTP).";
> > 
> > Maybe we could just list HTTPS and SFTP as the (respective) examples, in
> > 2021?
> 
> [[Med]] These are only examples. I tend to keep the same description as in 
> RFC8299 as this is related to the customer traffic. 
> 
> > 
> >        leaf vpn-name {
> >          type string;
> >          description
> >            "A name used to refer to the VPN.";
> > 
> > Should we say a little more about how the name differs from the id,
> > given that both are "type string"?
> 
> [[Med]] The main difference is provided as part of the "id" description: the 
> id is what is used to uniquely identify the VPN. 
> 
> That's said, I updated the description for better clarity as follows: 
> 
> NEW:
>         "A name used to associate a name with the service
>          in order to facilitate the identification of 
>          the service";
> 
> > 
> >          leaf import-policy {
> >            type string;
> >            description
> >              "Defines the 'import' policy.";
> >          }
> >          leaf export-policy {
> >            type string;
> >            description
> >              "Defines the 'export' policy.";
> > 
> > Does it "define" or merely "identify" the policy?  Naively, a
> > "definition" would be a rather long string...
> 
> [[Med]] "identifies" is more appropriate. Fixed. 
> 
> > 
> >      grouping vpn-components-group {
> >        [...]
> >        container groups {
> >          description
> >            "Lists the groups to which a VPN node,a site, or a network
> > 
> > space after comma.
> > 
> > 
> 
> [[Med]] Fixed. Thank you. 
> 
> _________________________________________________________________________________________________________________________
> 
> 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.
> 

_______________________________________________
OPSAWG mailing list
OPSAWG@ietf.org
https://www.ietf.org/mailman/listinfo/opsawg

Reply via email to