[OPSAWG] AD review of draft-ietf-opsawg-l2nm-12

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

Hi,

Apologies for the delay, but I have now managed my AD review of draft-ietf-opsawg-l2nm-12.  (Also attached in case my email is truncated ...)

I would like to thank the authors, WG for their work on this document, and the shepherd for his review.  I found the document to be well written and pretty straightforward to follow and understand.  I also believe that this document is a useful addition to the YANG and Network Management Ecosystem, to thank you for that.

Anyway, here my comments.  I think that they mostly pretty minor, but perhaps a few that my need a bit more thought.  Hopefully they will help improve the doc.
 
---

Moderate comments:

(1) 
	   The VPN network access is comprised of:

	   'id':  Includes an identifier of the VPN network access.

	   'description':  Includes a textual description of the VPN network
		  access.

	   'interface-id':  Indicates the interface on which the VPN network
		  access is bound.

	   'global-parameters-profile':  Provides a pointer to an active
		  'global-parameters-profile' at the VPN node level.  Referencing an
		  active 'global-parameters-profile' implies that all associated
		  data nodes will be inherited by the VPN network access.  However,
		  some of the inherited data nodes (e.g., ACL policies) can be
		  overridden at the VPN network access level.  In such case,
		  adjusted values take precedence over inherited ones.

It wasn't entirely clear to me how this works with the global parameters defined at the VPN network access level and the VPN node level work.  Is this meant to be a 3 tier hierarchy, or is it always only 2 tiers?  Are you allowed to reference different global profiles both at the VPN network access level and the VPN node level?  Possibly, some slightly expanded description here may be helpful (and/or in the YANG module).


(2)                     |  +--rw encapsulation
                          |  |  +--rw encap-type?            identityref
                          |  |  +--rw dot1q
                          |  |  |  +--rw tag-type?   identityref
                          |  |  |  +--rw cvlan-id?   uint16
						  
Did you consider adding support for ranges or sets of VLAN Ids (e.g., a list of non-overlapping ranges) (both for the single and double tagged cases)?


(3)                           |  +--rw lag-interface
						  
I'm slightly surprised that you don't have parameters for the physical interfaces, and I can understand your justification for this, but then you do have configuration for LAG, including configuration for the underlying member interfaces.  This feels slightly inconsistent to me at the level that the configuration is being provided.  Understanding the rationale a bit more here might be helpful, and I think that we should double check that this should definitely be in this model.


(4)                            +--rw svc-pe-to-ce-bandwidth
                                 |       {vpn-common:inbound-bw}?
                                 |  +--rw pe-to-ce-bandwidth* [bw-type]

Can you specify different bandwidths for multiple CoS fields?  It looks like the list key is the bw-type, and hence you would only be able to specify the bandwidth for a single CoS field?  Is that sufficient?


(5)  8.3.  Ethernet Segments

The names used here don't seem to be particularly friendly.  Is giving them more human understandable identity names an option, or are these names directly mirroring some other registry?  Or perhaps even something like esi-type-1-lacp, esi-type-3-mac, etc?


(6)        leaf svc-mtu {
         type uint32;
         description
           "Service MTU, it is also known as the maximum transmission
            unit or maximum frame size. When a frame is larger than
            the MTU, it is fragmented to accommodate the MTU of the
            network.";
       }
	  
MTU's turn up in various places.  Given that MTUs seem to mean different things for different people, please can you check the descriptions and given that this model is for L2VPN's be super explicit about whether these MTUs are representing the L3 payload, or the max frame size including the L2 header and presumably FCS sequence as well.


(7)      identity mac-action {
       description
         "Base identity for a MAC action.";
     }
	 
Would an VPN implementation ever want to drop and log rather than just doing one or the other?


(8) Hierarchical groupings and defaults

I want to check what the model/plan is regarding hierarchical config (e.g., grouping parameters-profile) and default values.  It looks like some of the leaves in the provide have default values, which I believe will be ambiguous when it comes to hierarchical config.  I.e., normally I would never expect that a leaf with a default value defined would fall back to the hierarchical policy because logically the leaf always has a value if it is in scope.  One solution to this problem (that is a bit more verbose), would be to take the defaults out of the leaves in the grouping, and then add them back in using "refine" them using refine statements when the global parameters-profile is used.  Another choice would be to not express these using the YANG "default" keyword at all, and instead describe the defaults in the description.

Generally, defining defaults where we can is probably useful, but we need to be careful with any hierarchical config/policies.


(9) Various comment related to handling VLAN tag rewrites:

                       container dot1q {
                         container rewrite {
                           description
						   
                             leaf pop {
                               type enumeration {
                                 enum 1 {
                                   description
                                     "Allows one (1) tag removal.";
                                 }
                                 enum 2 {
                                   description
                                     "Allows two (2) tags removal.";
                                 }
                               }
                               description
                                 "Tag removal.";
                             }

                             leaf translate {
                               type enumeration {

                                 enum 1-to-1 {
                                   description
                                     "Allows one (1) to one (1) tag
                                      translation.";
                                 }
                                 enum 1-to-2 {
                                   description
                                     "Allows one (1) to two (2) tags
                                      translation.";
                                 }
                                 enum 2-to-1 {
                                   description
                                     "Allows two (2) to one (1) tag
                                      translation.";
                                 }
                                 enum 2-to-2 {
                                   description
                                     "Allows two (2) to two (2) tags
                                      translation.";
                                 }

(i) This rewrite command is currently under dot1q and not qinq, hence it only seems to apply to single tagged interfaces.
(ii) Logically, any pops should be limited to only popping matched tags (e.g., don't allow a 2 tag pop if a single tag is matched) or otherwise the operation cannot be done symmetrically.
(iii) leaf pop could just be defined as an int8 (range 1-2) rather than an enumeration.
(iv) Currently there is no way of pushing more than one tag (which currently must be a C-VLAN tag).
(v) There is no choice in the Ethertype of the tag that is being pushed (but perhaps this is okay, if a single tag is pushed it is always a C-VLAN, if two tags are pushed then it is S-VLAN, C-VLAN).
(vi) Do you want/need the translate enumeration leaf alongside the push and pop?
(vii) The "direction" enum only has one option, perhaps allowing a deviation to support asymmetric rewrites? But those cannot really be expressed with how the current model is defined anyway.  It might be worth making symmetric the default choice, but I'm not sure whether you want the leaf at all?


(10)
                         leaf speed {
                           type uint32;
                           units "mbps";
                           default "10";
                           description
                             "LACP speed.";
                         }
				
10 Mbps seems like a slow default LACP speed.  Does this need a default at all, Ethernet interfaces would generally negotiate to the fastest supported speed.  The same comment applies to the port speed.


(11)
I did wonder whether it is okay to include the BGP and PW types as part of this draft?  Personally, since they will be controlled by IANA anyway, and this is where they are being used I think that is okay, but I was wondering whether IDR/BESS had been consulted on this at all, it might be worth checking with them that they are okay.



Minor comments/nits:

(1)
   In particular, the model can be used in the communication
   interface between the entity that interacts directly with the
   customer, the service orchestrator (either fully automated or a human
   operator), and the entity in charge of network orchestration and
   control (a.k.a., network controller/orchestrator) by allowing for
   more network-centric information to be included.

Nit (since this is explained more clearly later in the document): It was unclear to me whether this this sentence is about 3 entities or 2.  I.e., specifically, whether the "entity that interacts directly with the customer" is the service orchestrator.  For clarity, would it be clearer as: ",i.e., the service orchestrator,".

(2)    'signaling-type':  Indicates the signaling that is used for setting
      pseudowires. 
	  
Nit: setting pseudowires => setting up pseudowires?

(3)
      'mac-loop-prevention':  Container for MAC loop prevention.

         'window':  The timer when a MAC mobility event is detected.

         'frequency':  The number of times to detect MAC duplication,
            where a 'duplicate MAC address' situation has occurred and
            the duplicate MAC address has been added to a list of
            duplicate MAC addresses.
			
The description of frequency wasn't really clear to me, perhaps this could be made clearer, or perhaps I just need educating!

(4)    'multicast-like':  Controls whether multicast is allowed in the
      service.

'multicast-like' seems like a strange name.  Wouldn't the service either support/emulate multicast or not?

(5) 	7.5.  VPN Nodes

               +--rw vpn-nodes
                  +--rw vpn-node* [vpn-node-id]
                     +--rw vpn-node-id            vpn-common:vpn-id
                     +--rw description?           string
                     +--rw ne-id?                 string
                     +--rw role?                  identityref

'role' doesn't seem to be described in the description that follows, or specifically, it is not in the description where I expected it to be.

(6) 
             |  +--rw (signaling-option)?
             |     ...
             |     +--:(bgp)
             |     |  ...
             |     +--:(ldp-or-l2tp)

It's not obvious to me why LDP and L2TP are bundled together in the same signaling option.

More generally, I was surprised that you don't have containers at the top-level of the signaling-options, e.g, to be explicit about what signaling option is being used (i.e., what branch of the choice is being selected).  Is the thinking that you already have  "signaling-type" earlier in the definition.  Personally, I think that having containers here would probably be clearer, but I'm happy to leave it to the authors discretion.

(7) Section 8.1/8.2

Quite a lot of these types look like they are probably dead or not useful.  I guess that publishing them all to keep them directly in sync with the associated IANA registries makes sense, but I wonder if it would be helpful to have any text indicating that only some of these types are likely to be useful, or perhaps highlight the ones that are more likely to be used?

(8)
           leaf mac-num-limit {
             type uint16;
             default "2";
             description
               "Maximum number of MAC addresses learned from
                the customer for a single service instance.";
           }

"2" feels like a slightly strange default here, rather than say "1" or having no default.  What is the basis of this value.

(9)
                          leaf action {
                           type identityref {
                             base mac-action;
                           }
                           default "warning";
                           description
                             "specify the action when the upper limit is
                              exceeded: drop the packet, flood the
                              packet, or simply send a warning log
                              message.";
                         }

In the case of sending a warning log message, does this mean that the packet is still forwarded normally?  In the case of flood, does that mean that the MAC address is not learned?

(10)
               leaf router-id {
                 type rt-types:router-id;
                 description
                   "A 32-bit number in the dotted-quad format that is
                    used to uniquely identify a node within an
                    autonomous system (AS). ";
               }

Nit: Trailing space in the description.

(11)                            container bum-management {
                             description
                               "Broadcast-unknown-unicast-multicast
                                management.";
                             leaf discard-broadcast {
                               type boolean;
                               description
                                 "Discards broadcast, when enabled.";
                             }
                             leaf discard-unknown-multicast {
                               type boolean;
                               description
                                 "Discards unknown multicast, when
                                  enabled.";
                             }
                             leaf discard-unknown-unicast {
                               type boolean;
                               description
                                 "Discards unknown unicast, when
                                  enabled.";
                             }
                           }

>From the descriptions, it looks like these could be default "false" (subject to the comment about hierarchical configuration)?

(12)
9.  Security Considerations

   *  'ethernet-segments' and 'vpn-services': An attacker who is able to
      access network nodes can undertake various attacks, such as
      deleting a running L2VPN service, interrupting all the traffic of
      a client. 
	  
Is there a risk that an attacker could add a VPN endpoint that they control, and hence either inject or snoop traffic in the L2VPN service (which is arguably worse than either interrupting or deleting the L2VPN service)?

(13)
	10.2.  BGP Layer 2 Encapsulation Types

    This document defines the initial version of the IANA-maintained
	"iana-bgp-l2-encaps" YANG module. 
	
Perhaps include the reference back to the section where the YANG module is defined?  And similarly for the PW types YANG module.

(14)
   When a Layer 2 encapsulation type is added to the "BGP Layer 2
   Encapsulation Types" registry, a new "identity" statement must be
   added to the "iana-bgp-l2-encaps" YANG module.  The name of the
   "identity" is the lower-case of encapsulation name provided in the
   description.  The "identity" statement should have the following sub-
   statements defined:
   
Nit:  of encapsulation name => of the encapsultion name

(15)
   When the "iana-bgp-l2-encaps" YANG module is updated, a new
   "revision" statement must be added in front of the existing revision
   statements.
   
Possibly refine this to (for both modules) - there was a case where IANA accidentally created two entries with the same revision date when adding 2 types:

   When the "iana-bgp-l2-encaps" YANG module is updated, a new
   "revision" statement with a unique revision date must be added in front
   of the existing revision statements.

(16)
	A.5

             "auto-ethernet-segment-identifier": "01:11:00:11:00:11:\
   11:9A:00:00"
   
lowercase A would be canonical in the hex string.


--------

Grammar nits from an automated tool.

Grammar Warnings:
Section: 2, draft text:
The VPN node will identify the service providers node on which the VPN is deployed. 
Warning:  Apostrophe might be missing.
Suggested change:  "providers'"

Section: 7.2, draft text:
An external connectivity may be an access to the Internet or a restricted connectivity such as access to a public/private cloud. 
Warning:  Uncountable nouns are usually not used with an indefinite article. Use simply access.
Suggested change:  "access"

Section: 7.4, draft text:
Some of the data nodes can be adjusted at the VPN node or VPN network access levels. 
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "Some"

Section: 7.6, draft text:
A 'vpn-network-access' ([vpn_network_access_tree]) represents an entry point to a VPN service . 
Warning:  Don't put a space before the full stop.
Suggested change:  "."

Section: 7.6, draft text:
A 'vpn-network-access' includes information such as the connection on which the access is defined , the specific Layer 2 service requirements, etc.
Warning:  Put a space after the comma, but not before the comma.
Suggested change:  ","

Section: 7.6, draft text:
The VPN network access is comprised of:
Warning:  Did you mean comprises or consists of or is composed of?
Suggested change:  "comprises"

Section: 7.6, draft text:
However, some of the inherited data nodes (e.g., ACL policies) can be overridden at the VPN network access level. 
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "some"

Section: 7.6.1, draft text:
The L2NM inherits the 'member-link-list' structure from the L2SM (including indication of OAM 802.3ah support [IEEE-802-3ah].
Warning:  Unpaired symbol: ')' seems to be missing

Section: 7.6.4, draft text:
An ACL can be based on source MAC address, source MAC address mask, destination MAC address , and destination MAC address mask. 
Warning:  Put a space after the comma, but not before the comma.
Suggested change:  ","

Section: 9, draft text:
Some of the readable data nodes in the "ietf-l2vpn-ntw" YANG module may be considered sensitive or vulnerable in some network environments. 
Warning:  If the text is a generality, 'of the' is not necessary.
Suggested change:  "Some"

Section: A.1, draft text:
This section provides an example to illustrate how the L2NM can be used to mange BGP-based VPLS. 
Warning:  Did you mean manage?
Suggested change:  "manage"

Regards,
Rob