A YANG Data Model for Layer 2 Network Topologies
draft-ietf-i2rs-yang-l2-network-topology-18

Note: This ballot was opened for revision 14 and is now closed.

Martin Vigoureux Yes

Alvaro Retana No Objection

Benjamin Kaduk No Objection

Comment (2020-07-06 for -14)
Why is the "management-address" for a l2-node an IP address?  Does that
exclude any potential use of this data model for non-IP networks?

Section 3

   o  Links in the "ietf-network-topology" module are augmented as well
      with a set of Layer 2 parameters, allowing to associate a link
      with a name, a set of Layer 2 link attributes, and flags.

Interesting that names for links were not part of the core
network-topology module.  Are there any potential issues if other
ntework types also specify a link name and there is disagreement between
them?

Section 4

It reads a little oddly to use the flag-identity as the base type of a
typedef before the identity itself is declared, but I am way out of my
league here and defer to the YANG experts.

       description
         "VXLAN Network Identifier or VXLAN Segment ID.
          It allows up to 16 M VXLAN segments to coexist
          within the same administrative domain.

          The use of value '0' is implementation-specific.";

Is this intended as a nod to the use of 0 for the management VLAN?/
(I seem to recall this topic raising to some level of controversy in the
discussions around draft-ietf-bfd-vxlan.)

     /*

      * Features
      */

nit: there seems to be a spurious blank line here.

     grouping l2-node-attributes {
         [...]
         leaf sys-mac-address {
           type yang:mac-address;
           description
             "System MAC address.";
         }

Is there only one "System MAC address" per node?

         leaf delay {
           type uint32;
           units "microseconds";
           description
             "Link delay in microseconds.";

I guess we don't expect to use this model for deep-space links :)

       container l2-termination-point-attributes {
         description
           "Containing L2 termination point attributes.";
         leaf description {
           type string;
           description
             "Port description.";

Is a termination point always a "port", or should the description be
modified?

             list qinq {
               [...]
               leaf svlan-id {
                 type dot1q-types:vlanid;
                 description
                   "SVLAN ID.";
               }
               leaf cvlan-id {
                 type dot1q-types:vlanid;
                 description
                   "CVLAN ID.";

Could we perhaps expand "service" and "customer"?

           }
           //case ethernet

RFC 6020 suggests that YANG comments are "C++-style", which would seem
to indicate that the single-line comment could start on the same line as
the closing brace.  This, in turn, would save some confusion since the
block comments apply to the content after the comment, but these
comments apply to the content before the comment.
(Also later on as well.)

         leaf tp-state {
           [...]
             enum others {
               value 4;
               description
                 "The termination point is in other state.";
             }

Is there a plan for how substructure of these "others" states might be
added in the future?

Section 6

Thank you for updating the privacy considerations in response to the
secdir review!

   In the case of a topology that is configured, i.e. whose origin is
   "intended", the undesired configuration could become effective and be

Perhaps toss the word "datastore" in here somewhere to remind the
less-clueful reader (i.e., me) that origin is an NMDA concept?  Though
if it's sufficiently obvious, no need to do it just for me.

   reflected in the operational state datastore, leading to disruption
   of services provided via this topology might be disrupted.  For those

nit: deduplicate "disruption"/"disrupted".

   reasons, it is important that the NETCONF access control model is
   vigorously applied to prevent topology misconfiguration by
   unauthorized clients.

Should we condense "NACM" here since we provided the acronym at the
start of the paragraph?

   o  l2-node-attributes: A malicious client could attempt to sabotage
      the configuration of important node attributes, such as the name
      or the management-address.

I don't feel a need for a text change here (since "such as" suffices),
but would a change to the node's MAC address be similarly impactful?

   Some of the readable data nodes in this YANG module may be considered
   sensitive or vulnerable in some network environments.  It is thus
   important to control read access (e.g., via get, get-config, or
   notification) to these data nodes.  In particular, the YANG model for
   layer 2 topology may expose sensitive information, for example the
   MAC addresses of devices.  Unrestricted use of such information can

I think I've been told that in some environments the topology itself
(especially VLAN/VXLAN identifiers) can be considered sensitive.  What's
written here is consistent with that, and I don't insist on a change to
the text, but wondered if that was seen as a common enough thing to be
worth mentioning.

Section 8.1

RFC 3688 could arguably be demoted to informative, as could RFC 7951.

Section 8.2

If we use types defined in [IEEE802.1Qcp], that seems like a normative
reference to me.

Noting the discussion at
https://www.ietf.org/about/groups/iesg/statements/normative-informative-references/
about even optional features still being normative references, I think
RFC 7348 would be better placed as a normative reference.  Note that
there is not a process/downref issue in doing so, since it is already
listed at https://datatracker.ietf.org/doc/downref/

I could go either way (informative or normative) for RFC 8342;
presumably there's a convention to stick to.

Appendix B

I was going to reference
https://www.iab.org/2016/11/07/iab-statement-on-ipv6/ and suggest IPv6
addresses as example management-addresses, but I have a lingering memory
that the IPv4 form is still used to identify nodes even in v6-only
environments.  Do the right thing, of course.

[Note that I did not do an extensive consistency/sanity check on the
example topology or try to reconstruct it from the JSON.]

Erik Kline No Objection

Murray Kucherawy No Objection

Robert Wilton (was Discuss) No Objection

Comment (2020-09-17)
Clearing discuss held for IEEE review:

The IEEE Yangsters (members of the IEEE YANG experts) who reviewed and contributed review comments to this draft are comfortable with version -18.  Note, that these represent informal reviews of individuals and do not represent a formal IEEE consensus position."

Regards,
Rob


Mostly minor/editorial comments  (###) on the YANG model, but I do think that it would be helpful for these to be discussed and addressed as appropriate:

4.  Layer 2 Topology YANG Module

     import ietf-inet-types {
       prefix inet;
       reference
         "Section 4 of RFC 6991";
     }
     import ietf-yang-types {
       prefix yang;
       reference
         "Section 3 of RFC 6991";
     }

###      
These references should probably both just be: "RFC 6991: Common YANG Data Types"

     revision 2020-06-29 {
       description
         "Initial revision";
       reference
         "RFC XXXX: A YANG Data Model for Layer 2 Network
                    Topologies";
     }

###
I would reorder these sections to be (which will solve the forward reference issue mentioned by Ben):
   feature-stmt
   identity-stmt
   typedef-stmt

I'm surprised that pyang didn't flag this.

     /*
      * Typedefs
      */

     typedef vni {
       type uint32 {
         range "0..16777215";
       }
       description
         "VXLAN Network Identifier or VXLAN Segment ID.
          It allows up to 16 M VXLAN segments to coexist
          within the same administrative domain.

          The use of value '0' is implementation-specific.";
       reference
         "RFC 7348: Virtual eXtensible Local Area  Network (VXLAN):
                    A Framework for Overlaying Virtualized Layer 2
                    Networks over Layer 3 Networks";
     }

     typedef l2-flag-type {
       type identityref {
         base flag-identity;
       }
       description
         "Base type for L2 flags. One example of L2 flag
          type is trill which represents trill topology
          type.";
     }

###
This isn't really a base type.  Given where this flag is used, would this be better as an "network-flag-type", with a description more similar to the ones below?


     typedef node-flag-type {
       type identityref {
         base flag-identity;
       }
       description
         "Node flag attributes. The physical node can be
          one example of node flag attribute.";
     }

     typedef link-flag-type {
       type identityref {
         base flag-identity;
       }
       description
         "Link flag attributes. One example of link flag
          attribute is the pseudowire.";
     }

###
Should there be identities defined for l2-flag, node-flag and link-flag that derive from flag-identity?  That would them make these three typedefs reference different things rather than
all referencing the same base flags.


     typedef l2-network-event-type {
       type enumeration {
         enum add {
           value 0;
           description
             "A Layer 2 node or link or termination-point
              has been added.";
         }
         enum remove {
           value 1;
           description
             "A Layer 2 node or link or termination-point
              has been removed.";
         }
         enum update {
           value 2;
           description
             "A Layer 2 node or link or termination-point
              has been updated.";
         }
       }
       description
         "Layer 2 network event type for notifications.";
     }

###
Since these are events, I would suggest renaming these
"add" -> "addition", "remove" -> "removal", "update"

     typedef neg-mode {
       type enumeration {
         enum full-duplex {
           description
             "Indicates full-duplex mode.";
         }
         enum auto-neg {
           description
             "Indicates auto-negotiation mode.";
         }
         enum half-duplex {
           description
             "Indicates half-duplex mode.";
         }
       }
       description
         "Indicates the type of the negotiation mode.";
     }
###
What negotiation do you mean?  Does this mean IEEE 802.3 auto-negotation?  If so, it would be more correct to have a separate leaf for duplex vs auto-negotation.

     /*
      * Features
      */

     feature VLAN {
       description
         "Indicates that the system supports the
          vlan functions (also known as an IEEE 802.1Q tag).";
     }

     feature QinQ {
       description
         "Indicates that the system supports the
          qinq functions (also known as IEEE 802.1ad double tag).";
     }
###
I think that the description should be more clear on what is supported here: 2 VLAN tags, as defined by IEEE 802.1ad.  The features should also include reference statements.

Possibly we need to find a better name for these features (IEEE will possibly comment on this).


     feature VXLAN {
       description
         "Indicates that the device supports VXLAN functions.";
       reference
         "RFC 7348: Virtual eXtensible Local Area  Network (VXLAN):
                    A Framework for Overlaying Virtualized Layer 2
                    Networks over Layer 3 Networks";
     }

     /*
      * Identities
      */

     identity flag-identity {
       description
         "Base type for flags.";
     }
###
Probably the name should be just "flag" rather than "flag-identity"

     identity eth-encapsulation-type {
       base ift:iana-interface-type;
       description
         "Base identity from which specific Ethernet
          encapsulation types are derived.";
       reference
         "RFC 7224: IANA Interface Type YANG Module";
     }

###
I would rename the base identity to probably "l2-encapsulation-type".  It also probably doesn't help to derive from ift:iana-interface-type.

     identity ethernet {
     ...
     identity vxlan {
       base eth-encapsulation-type;
       description
         "VXLAN MAC in UDP encapsulation.";
     }
     
###
Please add references for protocols defined in the identities.

     /*
      * Groupings
      */

     grouping l2-network-type {

Dong, et al.            Expires December 31, 2020              [Page 12]
Internet-Draft     Layer 2 Network Topology Data Model         June 2020

       description
         "Indicates the topology type to be L2.";
       container l2-network {
         presence "Indicates L2 Network";
         description
           "The presence of the container node indicates
            L2 Topology.";
       }
     }

     grouping l2-network-attributes {
       description
         "L2 Topology scope attributes.";
       container l2-network-attributes {
         description
           "Contains L2 network attributes.";
         leaf name {
           type string;
           description
             "Name of the L2 network.";
###
Perhaps just "Name of the network".  Are there any restrictions here on how long the name can be, can it contain spaces, etc?
             
         }
         leaf-list flag {
           type l2-flag-type;
           description
             "L2 network flags.";
         }
       }
     }
     
###
Perhaps expand the description to "Flags that can be associated with the network"

I would suggest changing the name of all "flag" to "flags" where they are leaf-lists.


     grouping l2-node-attributes {
       description
         "L2 node attributes";
       container l2-node-attributes {
         description
           "Contains L2 node attributes.";
         leaf name {
           type string;
           description
             "Node name.";
         }
         leaf description {
           type string;
           description
             "Node description.";
         }
         leaf-list management-address {
           type inet:ip-address;
           description
             "System management address.";
         }
         leaf sys-mac-address {
           type yang:mac-address;
           description
             "System MAC address.";
         }
         leaf management-vid {
           if-feature "VLAN";
           type dot1q-types:vlanid;
           description
             "System management VID.";
     
###     
Please change "management-vid" to "management-vlan-id", and fix the description.

Probably could expand on all the descriptions of the "System" properties to explain what these system properties are and how they are used (e.g. to manage the device).

         }
         leaf-list flag {
           type node-flag-type;
           description
             "Node operational flags.";
         }
       }
     }

     grouping l2-link-attributes {
       description
         "L2 link attributes";
       container l2-link-attributes {
         description
           "Contains L2 link attributes.";
         leaf name {
           type string;
           description
             "Link name.";
         }
         leaf-list flag {
           type link-flag-type;
           description
             "Link flags.";
         }
         leaf rate {
           type decimal64 {
             fraction-digits 2;
           }
           units "Mbps";
           description
             "Link rate.";
             
###
Please expand on this description.  Does this describe how the link is operating at the physical
layer, or its bandwidth?

         }
         leaf delay {
           type uint32;
           units "microseconds";

           description
             "Link delay in microseconds.";
             
###
Is this uni-directional delay, or bi-directional?  Please clarify.


         leaf maximum-frame-size {
           type uint32;
           description
             "Maximum L2 frame size. If L2 frame is an Ethernet
              frame, the Ethernet header should be included;
              if L2 frame is other type (e.g., PPP), the L2
              header should be included.";
              
###
This needs to clarify whether the 4 byte CRC is included in the frame size.


             leaf eth-encapsulation {
               type identityref {
                 base eth-encapsulation-type;
               }
###
As per comments on the identities, I think that this should be "encapsulation".
               
               description
                 "Encapsulation type of this
                  termination point.";
             }
             leaf lag {
               type boolean;
               default "false";
               description
                 "Defines whether lag is supported or not.";
###
Does this leaf indicate whether LAG is supported, or enabled on the link?

             }
             leaf-list member-link-tp {
               when "../lag = 'true'" {
                 description
                   "Relevant only when the lag interface is supported.";
               }
               type leafref {
                 path "/nw:networks/nw:network/nw:node/"
                    + "nt:termination-point/nt:tp-id";
               }
               description
                 "Member link termination points.";
             }
             leaf mode {
               type neg-mode;
               default "auto-neg";
               description
                 "Exposes the negotiation mode.";
             }
###
Suggest changing the name to "ethernet-auto-neg-mode", and refining the description.

I would also suggest splitting duplex and auto-negotiation into 2 separate leaves to more accurately represent

             
             leaf port-vlan-id {
               when "derived-from-or-self(../eth-encapsulation"
                  + ", 'l2t:vlan')" {
                 description
                   "Only applies when the type of the Ethernet
                    encapsulation is 'vlan'.";
               }
               if-feature "VLAN";
               type dot1q-types:vlanid;
               description
                 "Port VLAN ID is the VLAN identifier that
                  will be assigned to any untagged frames entering
                  the switch on the specific port.";
             }
###
Probably naming this as the "default-untagged-vlan" may be more helpful.
             
             list vlan-id-name {
               when "derived-from-or-self(../eth-encapsulation"
                  + ", 'l2t:vlan')" {
                 description
                   "Only applies when the type of the Ethernet
                    encapsulation is 'vlan'.";
               }
###
I think that I would name this list "vlans"
               

               leaf vlan-name {
###
I would suggest renaming this leaf to just "name"


                 "Interface configured SVLANs and CVLANs.";
               leaf svlan-id {
                 type dot1q-types:vlanid;
                 description
                   "SVLAN ID.";
               }
               leaf cvlan-id {
                 type dot1q-types:vlanid;
                 description
                   "CVLAN ID.";
               }
             }
             
###
Suggest "SVLAN" => "S-VLAN" and "CVLAN" => "C-VLAN"
             
             container vxlan {
               when "derived-from-or-self(../eth-encapsulation"
                  + ", 'l2t:vxlan')" {
                 description
                   "Only applies when the type of the Ethernet
                    encapsulation is 'vxlan'.";
               }
               if-feature "VXLAN";
               leaf vni-id {
                 type vni;
                 description
                   "VXLAN Network Identifier (VNI).";
               }
               description
                 "Vxlan encapsulation type.";
             }
           }

           //case ethernet
           case legacy {
             leaf layer-2-address {
               type yang:phys-address;
               description
                 "Interface Layer 2 address.";
             }
             leaf encapsulation {
               type identityref {
                 base ift:iana-interface-type;
               }
               description
                 "Other legacy encapsulation type of this
                  termination point.";
             }
           }
           //case legacy such as atm, ppp, hdlc, etc.
         }
         //choice termination-point-type
         leaf tp-state {
           type enumeration {
             enum in-use {
               value 1;
               description
                 "The termination point is in forwarding state.";
             }
             enum blocking {
               value 2;
               description
                 "The termination point is in blocking state.";
             }
             enum down {
               value 3;
               description
                 "The termination point is in down state.";
             }
             enum others {
            
###
Please rename others => other
               value 4;
               description
                 "The termination point is in other state.";
###
Please modify the description to something like "The termination point is in another state, not listed in the enumeration." 
                
             }
           }
           config false;
           description
             "State of the termination point.";
         }
       }
     }

Roman Danyliw No Objection

Comment (2020-07-06 for -14)
No email
send info
Thank you for addressing the SECDIR feedback (and thank you Christian Huitema for providing it).

Warren Kumari No Objection

Éric Vyncke No Objection

Comment (2020-07-03 for -14)
Thank you for the work put into this document. 

Please find below a couple on non-blocking COMMENTs (and I would appreciate a reply to each of my COMMENTs because for some of them I was close to ballot a DISCUSS).

I hope that this helps to improve the document,

Regards,

-éric

== DISCUSS ==


== COMMENTS ==

Generic: is there a reason why the YANG validation results in 19 errors and 4 warnings? Sue Harres (the shepherd) mentions in her write-up that 9 errors are linked to missing IEEE but what about the 10 remaining errors ?

Has there been a review by IEEE of this YANG model? While the shepherd is extensive and detailed, there is no mention of a coordination with IEEE.

-- Section 3 --

  "The Layer 2 (L2) network topology YANG module is designed to be
   generic and applicable to Layer 2 networks built with different L2
   technologies."
Is this statement correct? What about LoraWAN, Sigfox, and other LP-WAN technologies? Or technologies that may be using different MTU sizes on each direction? or having more parameters than this (such as being NBMA that should be captured).

Should "sys-mac-address? " rather be "management-mac-address? "

I must admit that I am not familiar with the ietf-topology YANG model, so, the following COMMENTs can be plain wrong :-( ...

It is unclear to me the difference between 'node' and 'termination-point'. If not defined in the ietf-topology, then please define before first use (I had to read the YANG module to understand).

Why is 'ethernet' used rather than 'ieee802', notably to cover IEEE 802.11 ?

While most termination points have a single MAC address, are we sure that no termination point will ever have more than one MAC address ?

'rate' leaf is in Mbps and with 2 decimals, i.e., the lowest rate is 10 kbps and this is already higher than some layer-2 links. Any reason to ignore lower rate links ?

-- Section 4 --
"leaf maximum-frame-size" please specify whether Ethernet pre-amble, inter-frame gap, and CRC should be included. The text for Ethernet and for PPP are identical, so, why repeating it ? 

== NITS ==

Sometimes 'L2' is used, sometimes 'Layer 2' is used. Not very consistent ;-) I am not an English speaker, but, I believe 'Layer 2 topology' should be written 'layer-2 topology'

(Alissa Cooper; former steering group member) No Objection

No Objection ( for -14)
No email
send info

(Barry Leiba; former steering group member) No Objection

No Objection ( for -14)
No email
send info

(Deborah Brungard; former steering group member) No Objection

No Objection ( for -14)
No email
send info

(Magnus Westerlund; former steering group member) (was Discuss) No Objection

No Objection ( for -14)
No email
send info