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? 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? Also, is there only one VPN node that is applied to any one PE, or can there be many? 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, 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? 4. Reference Architecture In Figure 1, some of the module names are missing the initial "ietf-". 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? 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. 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? 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? 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. 10.1. YANG Modules Perhaps this section would better be named "Registrations". 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. 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: 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". 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". "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." 10.3. Pseudowire Types Similar nits as for section 10.2. 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 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 [END] -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
