Re: [Gen-art] Genart last call review of draft-ietf-opsawg-l2nm-15

[[e-mail]]

Hi Dale, 

Thank you for the review.

The changes to take into account this review can be tracked at: https://tinyurl.com/l2nm-latest

Please see inline for more context. 

Cheers,
Med

> -----Message d'origine-----
> De : Dale Worley via Datatracker <[e-mail]>
> Envoyé : mercredi 11 mai 2022 03:45
> À : [e-mail]
> Cc : [e-mail]; [e-mail];
> [e-mail]
> Objet : Genart last call review of draft-ietf-opsawg-l2nm-15
> 
> Reviewer: Dale Worley
> Review result: Ready with Nits
> 
> I am the assigned Gen-ART reviewer for this draft. The General
> Area Review Team (Gen-ART) reviews all IETF documents being
> processed by the IESG for the IETF Chair.  Please treat these
> comments just like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document:  draft-ietf-opsawg-l2nm-15
> Reviewer:  Dale R. Worley
> Review Date:  2022-05-10
> IETF LC End Date:  2022-05-13
> IESG Telechat date:  [not known]
> 
> I have not checked the Yang module itself, as the Yang Doctors
> will do a better job than I can.  Similarly, I have assumed that
> the specific information required for configuring VPNs has been
> set correctly by the members of the working group.  I have
> reviewed it from the point of view that I am qualified to, a
> reader with beginning knowledge about VPNs and Yang learning more
> about the subject.
> 
> Summary:
> 
>     This draft is basically ready for publication, but has nits
> that
>     should be fixed before publication.
> 
> Nits/editorial comments:
> 
> 2.  Terminology
> 
> Clarifying the wording here so as to make clear the relationships
> between these concepts would ease the learning curve for the
> inexperienced.  For example,
> 
>    VPN node:  Is an abstraction that represents a set of policies
>       applied on a PE and belonging to a single VPN service.
> 
> This is likely known in the VPN community, but I'm having a
> problem following it:  What exactly is a PE, or rather, what is
> its conceptual scope?

[Med] Added a pointer to RFC4026 where PE is defined. 


  A "Customer Edge-to-Provider Edge
> attachment circuit" is clear to the naive, because it's the
> tangible thing that connects the customer to the service provider.
> That suggests that the CE is the logical entity on the customer
> end of the attachment, and similarly the PE.  But is the PE unique
> to the attachment circuit, and thus the VPN has a lot of PEs that
> it interconnects, or is there a single PE in the VPN?

[Med] I think the new pointer to RFC4026 answers these questions.

> 
> Also, is there only one VPN node that is applied to any one PE, or
> can there be many?

[Med] There are many. This is indicated in the core text. We don't want to overload the terminology section. 

  This determines whether VPN nodes are one-to-
> one with PEs or whether they may have a wider scope.  It seems to
> be known that a VPN can have multiple VPN nodes, but that isn't
> stated in the definition either.
> 
>    VPN network access:  [...]
> 
>       A reference to the bearer is maintained to allow keeping the
> link
>       between the L2SM and the L2NM when both data models are used
> in a
>       given deployment.
> 
> This sentence is correct, but it doesn't seem to belong in this
> location,

[Med] Moved that sentence to appear under Section 7.6.1. 

 as it seems to describe an aspect of the data concerning
> an attachment circuit, whereas "VPN network access" is an
> abstraction of just one end of an attachment circuit.  Or does the
> conceptual model not have an abstraction of the other end of
> attachment circuits, thus allowing the network interface and the
> attachment circuit to be conflated, and thus the reference to the
> bearer can be considered to be a property of the VPN network
> access?
> 

[Med] The bearer reference is there to correlate between what was used in the service request (L2SM) and the network realization (L2NM, this draft).

> 4.  Reference Architecture
> 
> In Figure 1, some of the module names are missing the initial
> "ietf-".
> 

[Med] Thanks for catching this. This is actually using mix of prefixes and module names. this is not consistent. Went for prefixes (i.e., remove ietf- for the ES).

> The bottom section of Figure 1 is:
> 
>     +------+  Bearer    +------+      +------+         +------+
>     | CE A + ---------- + PE A +      + PE B + ------- + CE B |
>     +------+ Connection +------+      +------+         +------+
> 
>                Site A                           Site B
> 
> Shouldn't there be some indication of connection between PE A and
> PE B?

[Med] that would overload the figure. That is supposed to be the "network" in the line right before the part you extracted. Updated to:

               +--------------------------------+
               \              Network         /  
+----+  Bearer  \ +----+              +----+ /       +----+
|CE A+ ---------- +PE A+              +PE B+ ------- +CE B|
+----+ Connection +----+              +----+         +----+

        Site A                                 Site B

> 
> Also, it's not clear why this set of boxes is integrated with the
> rest of Figure 1, as the lines in the figure don't seem to show
> any particular connection between these four boxes and the boxes
> above them.  This segment seems to be a generic VPN between two
> sites, but "Site A" and "Site B" don't seem to be referenced
> elsewhere.
> 

[Med] That was added on purpose to explicit some notions that are mentioned in the module (bearer, connection). 

> If the intention is that "Network" embraces all 4 of these boxes,
> then the ends of the dashed line above "Network" need to be
> aligned with the left and right edges of the sites, and possibly
> some "|" added to show the various interconnections, perhaps in
> this style:
> 
>                +----+----+   |                   |
>                | Config  |   |                   |
>                | Manager |   |                   |
>                +----+----+   |                   |
>                     |        |                   |
>                     | NETCONF/CLI..................
>                     |        |                   |
>     +---------------------------------------------------------+
>     |                            Network                      |
> 
>     +------+  Bearer    +------+      +------+         +------+
>     | CE A + ---------- + PE A +======+ PE B + ------- + CE B |
>     +------+ Connection +------+      +------+         +------+
> 
>                Site A                           Site B
> 
> But if you want to emphasize that L2NM only describes the part of
> the network between the PE's, you could do something like this:
> 
>                +----+----+   |                   |
>                | Config  |   |                   |
>                | Manager |   |                   |
>                +----+----+   |                   |
>                     |        |                   |
>                     | NETCONF/CLI..................
>                     |        |                   |
>                   +-------------------------------+
>                   |              Network          |
> 
>     +------+ Bearer     +------+      +------+ Bearer  +------+
>     | CE A + ---------- + PE A +======+ PE B + ------- + CE B |
>     +------+ Conn.      +------+      +------+ Conn.   +------+
> 
>                Site A                           Site B
> 
> 7.2.  VPN Profiles
> 
>    The 'vpn-profiles' container (Figure 5) is used by a VPN
> service
>    provider to define and maintain a set of VPN profiles [RFC9181]
> that
>    apply to one or several VPN services.
> 
>    This document does not make any assumption about the exact
> definition
>    of these profiles.
> 
> I am having a hard time understanding what is intended.  The first
> sentence says that the container provides a way to define
> profiles, but the rest of the document doesn't provide any way to
> define the profiles.  Is the intended meaning that the container
> lists the names of profiles that are defined somewhere else, so
> that once the names are listed in the container, the profiles can
> be referenced in the definitions of services?  Or is there an
> implication that additional Yang nodes can be added in vpn-
> profiles to provide the definitions?

[Med] What is intended is to allow operators to define locally a set of profiles (and thus leave the structure of that profile local). Only the identifiers of these profiles will be listed under the VPN module. The glue is local to an implementation. Only ids that are listed under this container, can be called for a service (e.g., forwarding profile). 

> 
> In addition, despite the phrasing, there seems to be no constraint
> that a particular profile is in fact applied to one or several
> services.  Is the intended meaning that the listed profiles *can
> be
> applied* to services?

[Med] Changed to "that apply to VPN services".

> 
> 8.1.  IANA BGP Layer 2 Encapsulation Types
> 
>        description
>          "ATM transparent cell transport";
> 
> For consistency with the other items, there should be a "." at the
> end of the description.

[Med] Agree.

> 
> 10.1.  YANG Modules
> 
> Perhaps this section would better be named "Registrations".

[Med] Updated to "Registering YANG Modules"

> 
> 10.2.  BGP Layer 2 Encapsulation Types
> 
> There are a number of ways the wording of this section could be
> improved.
> 
>    This document defines the initial version of the IANA-
> maintained
>    "iana-bgp-l2-encaps" YANG module (Section 8.1).  IANA is
> requested to
>    add this note for both modules:
> 
> "both modules" isn't correct here, as only one module has been
> mentioned by this point in the section.

[Med] You are right. This should be for this specific registry. Fixed. 

> 
>       BGP Layer 2 encapsulation types must not be directly added
> to the
>       "iana-bgp-l2-encaps" YANG module.  They must instead be
>       respectively added to the "BGP Layer 2 Encapsulation Types"
>       registry [IANA-BGP-L2].
> 
> It's not clear what the word "respectively" means in this context.
> It would be clearer if the second sentence was expanded to:

[Med] removed "respectively". (we used to have in a working version one note for both modules but we didn't "clean" the text when dedicated sections were used in the published version).

> 
>       BGP Layer 2 encapsulation types must not be directly added
> to
>       the "iana-bgp-l2-encaps" YANG module.  They must instead be
>       added to the "BGP Layer 2 Encapsulation Types" registry
>       [IANA-BGP-L2], and then a derived identity added to
>       "iana-bgp-l2-encaps".
> 
> --
> 
>    The name of the
>    "identity" is the lower-case of the encapsulation name provided
> in
>    the description.
> 
> "the description" is ambiguous here; you mean "the description in
> the registry".

[Med] Yes.

> 
> But for most of the existing encapsulation types, the identity
> name is not, strictly speaking, the lower-case of the
> encapsulation name in the registry, but rather something similar.
> Suitable wording could be "a lower-case version of the
> encapsulation name".

[Med] Works for me. 

> 
>    "reference":   Replicates the reference from the registry and
> add the
>                   title of the document.
> 
> Possibly better phrased "Replicates the reference from the
> registry with the title of the document added."

[Med] ACK. 

> 
> 10.3.  Pseudowire Types
> 
> Similar nits as for section 10.2.
> 

[Med] ACK.

> A.4.  VPWS-EVPN Service Instance
> 
> Figure 29 would be clearer if a little more space was used to
> separate "ESI{1,2}" from the network elements.
> 
> 
>                       |<-------- EVPN Instance --------->|
>                       |                                  |
>                 |     V                                  V  |
>                 |     +-----+   +--------------+   +-----+  |
>          +----+ |     | PE1 |===|              |===| PE3 |  |
> +----+
>          |    +-------+     |   |              |   |     +-------+
> |
>          |    | |     +-----+   |              |   +-----+  |    |
> |
>          | CE1| |               |     Core     |            |
> |CE2 |
>          |    | |     +-----+   |              |   +-----+  |    |
> |
>          |    +-------+     |   |              |   |     +-------+
> |
>          +----+ |     | PE2 |===|              |===| PE4 |  |
> +----+
>               ^ |     +-----+   +--------------+   +-----+  |    ^
>               | |                                           |    |
>               |  ESI1                                       ESI2 |
>               |                                                  |
>               |<-------------- Emulated Service ---------------->|
> 
>                      Figure 29: An Example of VPWS-EVPN
> 

[Med] Made some changes. Hope this is looking better. 

> 
> A.5.  Automatic ESI Assignment
> 
> Figure 32 would be a little clearer if "LACP" was moved down a
> line (in parallel with how ES is raised a line).
> 
>               ES
>               |     +-----+      +--------------+   +-----+
>        +----+ |     | PE1 |======|              |===| PE3 |
> +----+
>        |    +-------+     |      |              |   |     +-------
> + CE3|
>        |    | |     +-----+      |              |   +-----+
> +----+
>        | CE1| |                  |     Core     |
>        |    | |     +-----+      |              |   +-----+
> +----+
>        |    +-------+     |      |              |   |     +-------
> + CE2|
>        +----+ |     | PE2 |======|              |===| PE4 |
> +----+
>               |     +-----+      +--------------+   +-----+
>             LACP
> 
>            Figure 32: An Example of Automatic ESI Assignment
> 

[Med] Done.  

> [END]
> 
> 


_________________________________________________________________________________________________________________________

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.