Re: [pim] AD Review of draft-ietf-pim-igmp-mld-snooping-yang-09

[Hongji Zhao <[e-mail]>]

Hi Alvaro,

Thanks a lot for your comments! We will address them as soon as possible.

BR/Hongji

-----Original Message-----
From: Alvaro Retana <[e-mail]> 
Sent: Saturday, January 18, 2020 6:43 AM
To: [e-mail]
Cc: Stig Venaas <[e-mail]>; [e-mail]; [e-mail]
Subject: AD Review of draft-ietf-pim-igmp-mld-snooping-yang-09

Dear authors:

Here is my review of the latest version (-09) of this document.

In general, there are many basic issues in the contents of the document.  For example: References are not present for fundamental work such as "BRIDGE and L2VPN", some of the sections don't follow the existing templates (Security Considerations, for example), etc.  These are issues that shouldn't be present in documents past WGLC! :-(

Also, there is a long list of idnits that must be fixed as well [1].


I have included comments in-line about those basic issues, and others.
Please take a close look below.


The datatracker is still showing several YANG Validation errors.
These were brought up during the WGLC, and it was suggested that the errors are a tool issue [2].  But that comment was for pyang 1.7.8 -- the current tool uses pyang 2.1.1; do we think the error is still in the tool?  Can someone please talk to the authors of the tool and point out the problem [3]?

This document augments an IEEE YANG model.  Has anyone (authors,
Chairs) made the IEEE aware of it?  To be honest, I don't know if there is a specific process we need to follow (I doubt it), but letting the IEEE know would be a minimum.


I will wait for this review to be addressed before starting the IETF LC.

Thanks!

Alvaro.


[1] https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-pim-igmp-mld-snooping-yang-09.txt
[2] https://mailarchive.ietf.org/arch/msg/pim/xD8PAlTqGIiyb2H3TeYT17w8zMU
[3] https://github.com/mbj4668/pyang


[Line numbers from idnits.]

...
17	Abstract

19	   This document defines a YANG data model that can be used to
20	configure and manage Internet Group Management Protocol (IGMP) and
21	Multicast Listener Discovery (MLD) Snooping devices. The YANG module in
22	this document conforms to Network Management Datastore Architecture
23	(NMDA).

[nit] No indent needed.


...
82	1. Introduction

84	This document defines a YANG [RFC6020] data model for the management of
85	Internet Group Management Protocol (IGMP) and Multicast Listener
86	Discovery (MLD) Snooping devices.

[major] Please add a reference to rfc4541.

[minor] rfc4541 is "based on best current practices for IGMPv2, with further considerations for IGMPv3- and MLDv2-snooping."  This document mostly uses IGMP and MLD (with no version indication).  Which versions of the protocols are supported in this document?  Just IGMPv2, IGMPv3 and MLDv2?  Please explicitly indicate that in this section.


...
94	1.1. Terminology

96	The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
97	"SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
98	"OPTIONAL" in this document are to be interpreted as described in BCP 14
99	[RFC2119].

[minor] There don't seem to be any rfc2119-keywords used, so this template is not needed.


101	The terminology for describing YANG data models is found in [RFC6020].

[] Please add text similar to §1.1/rfc8652.


103	1.2. Tree Diagrams

[major] This section should be replaced with the following:

   Tree diagrams used in this document follow the notation defined in
   [RFC8340].

The reference to rfc8340 should be Normative.

[major] A section similar to §1.4/rfc8652 (Prefixes in Data Node
Names) is needed to indicate the prefixes used.


...
131	2.1. Overview

133	The IGMP and MLD Snooping YANG module defined in this document has all
134	the common building blocks for the IGMP and MLD Snooping protocol.

[minor] "Snooping protocol"  It is not a protocol -- rfc4541 doesn't refer to it that way.  Instead, it uses simply "snooping switches".
Please use the same terminology.


136	The YANG module includes IGMP and MLD Snooping instance definition,
137	instance reference in the scenario of BRIDGE and L2VPN. The module also
138	includes the RPC methods for clearing IGMP and MLD Snooping group
139	tables.

[major] We need Normative references for where "the scenario of BRIDGE and L2VPN" are specified.


141	This YANG module conforms to Network Management Datastore Architecture
142	(NMDA)[RFC8342]. This NMDA architecture provides an architectural
143	framework for datastores as they are used by network management
144	protocols such as NETCONF [RFC6241], RESTCONF [RFC8040] and the YANG
145	[RFC7950] data modeling language.

[major] There is no reference for rfc6241 -- it must be Normative.

[major] There is no reference for rfc8040 -- it must be Normative.

[major] There is no reference for rfc7950 -- it must be Normative.


[major] Before going deeper into the structure, please include information about optional capabilities and how IPv4/IPv6 are handled.
See §2.2/§2.3 of rfc8652 for examples.


147	2.2. IGMP Snooping Instances

149	The YANG module defines igmp-snooping-instance which augments
150	/rt:routing/rt:control-plane-protocols/rt:control-plane-protocol.

[major] Please use descriptive text outside of the model.  For example:

   This model augments the core routing data model specified in [RFC8349].

         +--rw routing
            +--rw router-id?
            +--rw control-plane-protocols
            |  +--rw control-plane-protocol* [type name]
            |     +--rw type
            |     +--rw name
            |     +--rw igmp-snooping-instance     <= Augmented by this Model
                        ...
            |     +--rw mld-snooping-instance      <= Augmented by this Model
                        ...

   The "igmp-snooping-instance" container instantiates an IGMP Snooping Instance...

Please take a look at rfc8652 for examples of what is needed in this document.


[major] It may be confusing to other readers the reason for not augmenting, or even requiring, the IGMP/MLD model (rfc8652).  Please include some text in §2.1 to explain the relationship between snooping and the IGMP/MLD protocols themselves.  Specifically, the fact that the switches don't really nned to run the protocols.


152	All the IGMP Snooping related attributes have been defined in the igmp-
153	snooping-instance. The read-write attribute means configurable data. The
154	read-only attribute means state data.

[nit] s/attribute means/attribute represents


156	One igmp-snooping-instance could be referenced in one BRIDGE instance or
157	L2VPN instance. One igmp-snooping-instance corresponds to one BRIDGE
158	instance or L2VPN instance.

[major] These mentions of references from/correspondence to a "BRIDGE instance or L2VPN instance" makes me think that there are YANG modules somewhere that define those instances and that are not used as a reference in this document.


160	The value of scenario in igmp-snooping-instance is bridge or l2vpn. When
161	it is bridge, the igmp-snooping-instance will be referenced in the
162	BRIDGE scenario. When it is l2vpn, the igmp-snooping-instance will be
163	referenced in the L2VPN scenario.

[major] The scenarios need a description and/or reference...


165	The value of bridge-mrouter-interface, l2vpn-mrouter-interface-ac,
166	l2vpn-mrouter-interface-pw are filled by snooping device dynamically.
167	They are different from static-bridge-mrouter-interface, static-l2vpn-
168	mrouter-interface-ac, and static-l2vpn-mrouter-interface-pw which are
169	configured statically.

[major] The term "mrouter" is used in many places (alone and in variable names), but it is not defined anywhere.  Please define it.

[nit] s/by snooping device/by the snooping device


171	The attributes under the interfaces show the statistics of IGMP Snooping
172	related packets.

174	module: ietf-igmp-mld-snooping

[minor] I believe that the "module" line is not needed here, which may help with the long lines below.

175	  augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:
176	    +--rw igmp-snooping-instance {feature-igmp-snooping}?
177	    |  +--rw scenario?                            snooping-scenario-type
178	    |  +--rw enable?                              boolean
179	    |  +--rw forwarding-mode?                     enumeration
180	    |  +--rw explicit-tracking?                   boolean
{explicit-tracking}?
181	    |  +--rw exclude-lite?                        boolean {exclude-lite}?
182	    |  +--rw send-query?                          boolean
183	    |  +--rw immediate-leave?                     empty {immediate-leave}?
184	    |  +--rw last-member-query-interval?          uint16
185	    |  +--rw query-interval?                      uint16
186	    |  +--rw query-max-response-time?             uint16

[minor] Why are timer-value-* (rfc8294) not used?  Please consider using them for the variables above and others...

...
247	2.3. MLD Snooping Instances

249	The YANG module defines mld-snooping-instance which could be referenced
250	in the BRIDGE or L2VPN scenario to enable MLD Snooping.

[major] These mentions of references from/correspondence to a "BRIDGE instance or L2VPN instance" makes me think that there are YANG modules somewhere that define those instances and that are not used as a reference in this document.


252	The mld-snooping-instance is the same as IGMP snooping except changing
253	IPv4 addresses to IPv6 addresses.

255	module: ietf-igmp-mld-snooping

[minor] I believe that the "module" line is not needed here, which may help with the long lines below.


256	  augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol:

[major] There is no mention in this section of the augmentation, as was mentioned in §2.2.  Please be consistent and avoid shortcuts.


...
326	2.4. IGMP and MLD Snooping Instances Reference

[] Please make sure that some of this information is available earlier in the document, or that a forward reference to this Section exists to avoid questions about referring to the snooping instances.

328	The igmp-snooping-instance could be referenced in the scenario of BRIDGE
329	or L2VPN to configure the IGMP Snooping.

331	For the BRIDGE scenario this model augments /dot1q:bridges/dot1q:bridge
332	to reference igmp-snooping-instance. It means IGMP Snooping is enabled
333	in the whole bridge.

[major] There is no reference yet to what "dot1q" is.  Where is that module specified?  The reference should be Normative.



...
347	For the L2VPN scenario this model augments /ni:network-instances/
348	ni:network-instance/ni:ni-type/l2vpn:l2vpn to reference igmp-snooping-
349	instance. It means IGMP Snooping is enabled in the specified l2vpn
350	instance.

[major] There is no reference yet to what "ni" is.  Where is that module specified?  The reference should be Normative.


...
378	3. IGMP and MLD Snooping YANG Module

[major] Please add a statement before the module that includes any documents referenced in it: "This module references..."


...
387	  import ietf-inet-types {
388	    prefix "inet";
389	  }

[major] All the import statements need a reference, for example:

   import ietf-inet-types {
     prefix inet;
     reference
       "RFC 6991: Common YANG Data Types";
   }

Note that "RFC 6991", and not "RFC6991" is used inside the module.
Please review *all* the references inside the module and change them as needed.


...
446	  description
447	    "The module defines a collection of YANG definitions common for
448	     all Internet Group Management Protocol (IGMP) and Multicast
449	     Listener Discovery (MLD) Snooping devices.

[minor] Please add a reference here to RFC 4541.


...
475	  feature feature-igmp-snooping {
476	    description
477	      "Support IGMP snooping protocol.";

[minor] It is not a protocol...


478	    reference
479	      "RFC 4541, Section 1";
480	  }

[major] §1 is the Introduction.  Unless there is a very specific need to point to a section, just referencing the RFC is ok.


482	  feature feature-mld-snooping {
483	    description
484	      "Support MLD snooping protocol.";

[minor] It is not a protocol...


485	    reference
486	      "RFC 4541, Section 1";
487	  }

[major] §1 is the Introduction.

[] Some of the "features" introduced here are part of the snooping specification (rfc4541), or needed to operate the mechanism (for example static-mrouter-interface).  In those cases, there's no need to include a reference for each one.


489	  feature immediate-leave {
490	    description
491	      "Support configuration of immediate-leave.";
492	    reference
493	      "RFC 2236, Section 10";
494	  }

[major] I couldn't find any mention of immediate leave (or anything
similar) in rfc2236.  And in any case, §10 is the Security Considerations.


...
503	  feature static-l2-multicast-group {
504	    description
505	      "Support configuration of L2 multicast static-group.";
506	    reference
507	      "RFC 4541, Section 2.1";
508	  }

[major] rfc4541 doesn't even contain the word "static".  If the feature is defined there with a different name, please use the same terminology, at least in the description.


510	  feature static-mrouter-interface {
511	    description
512	      "Support configuration of mrouter interface.";
513	    reference
514	      "RFC 4541, Section 2.1";
515	  }

[major] Same as above...


516	  feature rpc-clear-groups {
517	    description
518	      "Support clearing statistics by RPC for IGMP & MLD snooping.";
519	    reference
520	      "RFC 4541, Section 2.1";
521	  }

[major] I doubt that this specifically mentioned in rfc4541.  In this case, no reference is needed.


523	  feature explicit-tracking {
524	    description
525	      "Support configuration of per instance explicit-tracking.";
526	    reference
527	      "RFC 3376, Appendix B";
528	  }

[major] Appendix B (Summary of Changes from IGMPv2) doesn't specify what explicit tracking is.  Following the lead from rfc8653, use Section 3 of rfc6636 as the reference.  It should be Normative.


...
573	  identity igmp-snooping {
574	    base rt:control-plane-protocol;
575	    description
576	      "IGMP snooping protocol";
577	  }

[minor] It is not a protocol...


579	  identity mld-snooping {
580	    base rt:control-plane-protocol;
581	    description
582	      "MLD snooping protocol";
583	  }

[minor] It is not a protocol...


...
623	    leaf version {
624	      type uint8 {
625	        range "1..3";
626	      }
627	      default 2;
628	      description "IGMP snooping version.";
629	    }

[minor] This is the version of what?  Because of the range, I guess it is the IGMP protocol version, and not the "IGMP snooping version."  Is that correct?

[major] If the previous guess is correct...  What does this version mean?  Given that the default is 2, does it mean that only IGMPv2 packets should be snooped?  I would assume that deployments already supporting IGMPv3 are common...


...
663	      leaf-list l2vpn-outgoing-ac {
664	        when 'derived-from-or-self(../../scenario,"ims:l2vpn")';
665	        type if:interface-ref;
666	        description "Outgoing AC in L2VPN forwarding";
667	      }

[minor] Expand AC on first use.


669	      leaf-list l2vpn-outgoing-pw {
670	        when 'derived-from-or-self(../../scenario,"ims:l2vpn")';
671	        type pw:pseudowire-ref;
672	        description "Outgoing PW in L2VPN forwarding";
673	      }

[minor] Expand PW on first use.


...
688	    leaf forwarding-mode {
689	      type enumeration {
690	        enum "mac" {
691	          description
692	          "MAC-based lookup mode";
693	        }
694	        enum "ip"  {
695	          description
696	          "IP-based lookup mode";
697	        }
698	      }
699	      default "ip";
700	      description "The default forwarding mode is ip";
701	    }

[minor] rfc4541 doesn't talk about forwarding modes.  It talks about how "switches may maintain forwarding tables based on either MAC addresses or IP addresses" and how "if a switch supports both types of forwarding tables then the default behavior should be to use IP addresses."  Perhaps forwarding-type of forwarding-table-type would be more descriptive.  In any case, please at least add a better description.


703	    leaf explicit-tracking {
704	      if-feature explicit-tracking;

[nit] Using the same name for the feature and the leaf may be confusing.  This is done in several places.


705	      type boolean;
706	      default false;
707	      description
708	        "Track the IGMP v3 & MLD v2 membership reports
709	         from individual hosts. It contributes to saving network
710	         resources and shortening leave latency.";
711	    }

[minor] s/IGMP v3 & MLD v2/IGMPv3 and MLDv2


...
738	    leaf last-member-query-interval {
739	      type uint16 {
740	        range "1..1023";
741	      }
742	      units seconds;
743	      default 1;
744	      description
745	        "Last Member Query Interval, which may be tuned to modify
746	         the leave latency of the network.";
747	      reference "RFC3376. Sec. 8.8.";
748	    }

[major] The Last Member Query Interval has a default of 10 (in units of 1/10 second).  While that does correspond to 1 second, it doesn't reflect the same values as in rfc3376.  It may be clearer to the user of the module to count in seconds...but not all the granularity expected from rfc3376 can be displayed.  If you want to maintain the units as seconds, please describe the use when the model is introduced (§2.*), and explain the potential loss of fidelity.

Note that this is not the only parameter that has the same issue.


...
760	    leaf query-max-response-time {
761	      type uint16;
762	      units seconds;
763	      default 10;
764	      description
765	        "Query maximum response time specifies the maximum time
766	         allowed before sending a responding report.";
767	      reference "RFC3376. Sec. 4.1.1, 8.3, 8.14.3.";
768	    }

[minor] rfc3376 only uses "maximum response time" once (in §9.1).
§4.1.1 uses "Max Resp Time" (in units of 1/10 second), §8.3 uses "Max Response Time" (also in units of 1/10 second), and §8.14.3 just talks about how to use the "Max Response Time".  Please be consistent and specific.


770	    leaf require-router-alert {
771	      if-feature require-router-alert;
772	      type boolean;
773	      default false;
774	      description
775	        "When the value is true, router alert should exist
776	         in the IP head of IGMP or MLD packet.";
777	    }

[nit] s/IP head/IP header


...
821	    leaf version {
822	      type uint8 {
823	        range "1..2";
824	      }
825	      default 2;
826	      description "MLD snooping version.";
827	    }

[minor] This is the version of what?  Because of the range, I guess it is the MLD protocol version, and not the "MLD snooping version."  Is that correct?

[major] If the previous guess is correct...  What does this version mean?  Given that the default is 2, does it mean that only MLDv2 packets should be snooped?


...
1122	  grouping igmp-snooping-statistics {
1123	    description
1124	      "The statistics attributes for IGMP snooping.";

1126	      leaf num-query {
1127	        type yang:counter64;
1128	        description
1129	          "The number of query messages.";
1130	        reference
1131	          "RFC 2236, Section 2.1";
1132	      }

[major] It is nice that you're putting specific references in.
However, the terminology (in most cases in this group) doesn't match.
For example, rfc2236 talks about a "Membership Query" (vs simply "query messages").  Given that this is a Standards Track document, it is important to be specific about the terminology.


1133	      leaf num-membership-report-v1 {
1134	        type yang:counter64;
1135	        description
1136	          "The number of membership report v1 messages.";
1137	        reference
1138	          "RFC 3376, Section 4";
1139	      }

[major] rfc3376/§4 points at rfc1112 as the reference for where the Version 1 Membership Report is specified.


1140	      leaf num-membership-report-v2 {
1141	        type yang:counter64;
1142	        description
1143	          "The number of membership report v2 messages.";
1144	        reference
1145	          "RFC 3376, Section 4";
1146	      }

[major] rfc3376/§4 points at rfc2236 as the reference for where the Version 2 Membership Report is specified.


...
1154	      leaf num-leave {
1155	        type yang:counter64;
1156	        description
1157	          "The number of leave messages.";
1158	        reference
1159	          "RFC 3376, Section 4";
1160	      }

[major] rfc3376/§4 points at rfc2236 as the reference for where the Version 2 Leave Group is specified.  Is that the message you're referring to?


1161	      leaf num-non-member-leave {
1162	        type yang:counter64;
1163	        description
1164	          "The number of non member leave messages.";
1165	        reference
1166	          "RFC 3376, Section 4";
1167	      }

[major]  I didn't find in rfc3376/§4 anything about non members.  I'm assuming that the counter above is simply the number of Leave Group messages received from nodes that were not members of the group.  Is that true?  If so, it may not be necessary to add a reference.


...
1189	      leaf num-report-v1 {
1190	        type yang:counter64;
1191	        description
1192	          "The number of Version 1 Multicast Listener Report.";
1193	        reference
1194	          "RFC 3810, Section 5";
1195	      }

[major] rfc3810/§5 points at rfc2710 as the correct reference.


...
1203	      leaf num-done {
1204	        type yang:counter64;
1205	        description
1206	          "The number of Version 1 Multicast Listener Done.";
1207	        reference
1208	          "RFC 3810, Section 5";
1209	      }

[major] rfc3810/§5 points at rfc2710 as the correct reference.


...
1318	    container igmp-snooping-instance {
1319	      when 'derived-from-or-self(../rt:type, "ims:igmp-snooping")' {
1320	        description
1321	          "This container is only valid for IGMP snooping protocol.";

[minor] It is not a protocol...


...
1346	    container mld-snooping-instance {
1347	      when 'derived-from-or-self(../rt:type, "ims:mld-snooping")' {
1348	        description
1349	          "This container is only valid for MLD snooping protocol.";

[minor] It is not a protocol...


...
1371	  augment "/dot1q:bridges/dot1q:bridge" {
1372	    description
1373	      "Reference IGMP & MLD snooping instance in BRIDGE scenario";
...
1389	  augment "/dot1q:bridges/dot1q:bridge"+
1390	    "/dot1q:component/dot1q:bridge-vlan/dot1q:vlan" {
1391	    description
1392	      "Reference IGMP & MLD snooping instance in BRIDGE scenario";

[nit] These two augmentations have the same description.


...
1491	4. Security Considerations

[major] Please update the text in this section using the latest template, and make sure to update the references:
https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines


...
1505	There are a number of data nodes defined in this YANG module that are
1506	writable/creatable/deletable (i.e., config true, which is the default)