Ballot for draft-ietf-i2rs-yang-l2-network-topology
Yes
No Objection
Note: This ballot was opened for revision 14 and is now closed.
Thank you for addressing the SECDIR feedback (and thank you Christian Huitema for providing it).
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'
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.]
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."; } } }