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

[[e-mail]]

Hi Rob, 

Many thanks for the careful AD review. 

Staring with the last part. You can track the changes at: https://tinyurl.com/l2nm-latest. Please see inline for more context.

There are also other edits that I made to fix nits, update references, etc. 

Cheers,
Med

> -----Message d'origine-----
> De : Rob Wilton (rwilton) <[e-mail]>
> Envoyé : jeudi 17 mars 2022 21:53
> À : [e-mail]
> Cc : [e-mail]
> Objet : AD review of draft-ietf-opsawg-l2nm-12
> 
> 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.
> 
> ---
...
> 
> 
> 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,".
> 

[Med] I adjusted the sentence for better clarity. 

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

[Med] Fixed. 

> 
> (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!

[Med] Please check https://github.com/IETF-OPSAWG-WG/lxnm/issues/241 (especially the response from Raul when I asked the same question about frequency vs window).

> 
> (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?

[Med] Indeed. removed "-like". I lost the context why this name ended in the draft. 

> 
> (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.

[Med] Good catch. Fixed. Thanks.

> 
> (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.

[Med] Because they share a set of common attributes. 

> 
> 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.
> 

[Med] The signaling type is set at the service level (signaling-type). Among the values that can be used for that attribute, we do have l2tp-signaling or ldp-signaling. 

> (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?

[Med] I agree that only a subset of these values are useful, but we need to avoid that this document conflicts with the authoritative RFCs. For example, rfc4667#section-4.2 says the following: 

   Before establishing the intended pseudowire, each pair of peering PEs
   exchanges control connection messages to establish a control
   connection.  Each advertises its supported pseudowire types, as
   defined in [PWE3IANA], in the Pseudowire Capabilities List AVP.

FWIW, Adrian get in touch with PALS WG chairs about some entries for this registry. A stale reference was found and the PALS WG chairs contacted IANA to fix that.

> 
> (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.

[Med] This is inherited from what is the service model (L2SM, RFC8466):

                default "2";
                description
                  "Maximum number of MAC addresses learned from
                   the subscriber for a single service instance.
                   The default allowed maximum number of MAC
                   addresses is 2.";

> 
> (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?

[Med] Updated the text to make the intent explicit: "or simply send a warning log message (without dropping the packet)"

  In the case of flood, does that
> mean that the MAC address is not learned?

[Med] It is. 

> 
> (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.

[Med] Fixed.

> 
> (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)?
> 

[Med] 

> (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)?
> 

[Med] An attacker that accesses a node can a lot of harm. This includes of course intercept, read, or redirect to traffic. However, in addition to access control, "such activity can be detected by adequately monitoring and tracking network configuration changes" as noted at the end of the bullet you quoted.  

Added "or intercept/redirect the traffic to a non-authorized node" to highlight the risk. 

> (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.
> 

[Med] Done. 

> (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
> 

[Med] Fixed. Thanks. 

> (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.
> 

[Med] OK. Made the change for both modules. 

> (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.
> 
> 

[Med] Fixed. 

> --------
> 
> Grammar nits from an automated tool.

[Med] Fixed those that I think are appropriate. 

> 
> 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


_________________________________________________________________________________________________________________________

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.