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

[Alvaro Retana <[e-mail]>]

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).
1507	These data nodes may be considered sensitive or vulnerable in some
1508	network environments. Write operations (e.g., edit-config) to these data
1509	nodes without proper protection can have a negative effect on network
1510	operations. These are the subtrees and data nodes and their
1511	sensitivity/vulnerability:

[major] For all the nodes in this section, you must add individual
comments about their vulnerability.  Again, look at rfc8652 for
exmaples.


1513	/rt:routing/rt:control-plane-protocols/rt:control-plane-protocol/ims:igmp-snooping-
1514	instance
1515	/rt:routing/rt:control-plane-protocols/rt:control-plane-protocol/ims:mld-snooping-
1516	instance

[minor] Please try to summarize the locations.


...
1538	Unauthorized access to any data node of these subtrees can adversely
1539	affect the IGMP & MLD Snooping subsystem of both the local device and
1540	the network. This may lead to network malfunctions, delivery of packets
1541	to inappropriate destinations, and other problems.

[major] Please be specific on the how.  For example, changing the
configuration of the instance is a specific issue.


...
1565	5. IANA Considerations

1567	RFC Ed.: In this section, replace all occurrences of 'XXXX' with the
1568	actual RFC number (and remove this note).

[nit] Not only in this section: the module itself contains the same notation.


1570	This document registers the following namespace URIs in the IETF XML

1572	registry [RFC3688]:

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


1574	--------------------------------------------------------------------

[nit] These's no need to include the "separators" ("---...---").


...
1598	6. Normative References

1600	   [P802.1Qcp/D2.2] IEEE Approved Draft Standard for Local and
1601	   Metropolitan Area Networks, "Bridges and Bridged Networks Amendment:
1602	   YANG Data Model", Mar 2018

[major] This reference is not used anywhere, but I guess it is the
YANG model that corresponds to dot1q.

[major] Is there any reason not to reference the final specification:
IEEE 802.1Qcp-2018?


...
1622	   [RFC4604] Holbrook, H., Cain, B., and B. Haberman, "Using Internet
1623	             Group Management Protocol Version 3 (IGMPv3) and Multicast
1624	             Listener Discovery Protocol Version 2 (MLDv2) for Source-
1625	             Specific Multicast", RFC 4604, August 2006.

== Unused Reference: 'RFC4604' is defined on line 1622, but no explicit
   reference was found in the text


1627	   [RFC4607] Holbrook, H. and B. Cain, "Source-Specific Multicast for
1628	             IP", RFC 4607, August 2006.

== Unused Reference: 'RFC4607' is defined on line 1627, but no explicit
   reference was found in the text


...
1634	   [RFC6021] Schoenwaelder, J., Ed., "Common YANG Data Types", RFC 6021,
1635	             October 2010.

** Obsolete normative reference: RFC 6021 (Obsoleted by RFC 6991)


...
1646	   [draft-ietf-pim-igmp-mld-yang-06] X. Liu, F. Guo, M. Sivakumar, P.
1647	             McAllister, A. Peter, "A YANG data model for Internet Group
1648	             Management Protocol (IGMP) and Multicast Listener Discovery
1649	             (MLD)", draft-ietf-pim-igmp-mld-yang-06, Oct 20, 2017.

[major] This document has been obsoleted by rfc8652.  This reference
can be Informative.


1651	   [draft-bjorklund-netmod-rfc7223bis-00] M. Bjorklund, "A YANG Data
1652	             Model for Interface Management",  draft-bjorklund-netmod-
1653	             rfc7223bis-00, August 21, 2017

[minor] This reference is obsolete and has been replaced by rfc8343.


1655	   [draft-bjorklund-netmod-rfc7277bis-00] M. Bjorklund, "A YANG Data
1656	             Model for IP Management", draft-bjorklund-netmod-
1657	             rfc7277bis-00, August 21, 2017

[minor] This reference is obsolete and has been replaced by rfc8344.


1659	   [draft-ietf-netmod-revised-datastores-03] M. Bjorklund, J.
1660	             Schoenwaelder, P. Shafer, K. Watsen, R. Wilton, "Network
1661	             Management Datastore Architecture", draft-ietf-netmod-
1662	             revised-datastores-03, July 3, 2017

[minor] This reference is obsolete and has been replaced by rfc8342.


1664	   [draft-ietf-bess-evpn-yang-02] P.Brissette, A. Sajassi, H. Shah, Z.
1665	             Li, H. Chen, K. Tiruveedhula, I. Hussain, J. Rabadan, "Yang
1666	             Data Model for EVPN", draft-ietf-bess-evpn-yang-02, March
1667	             13, 2017

[minor] This reference is unused.


1669	   [draft-ietf-bess-l2vpn-yang-08] H. Shah, P. Brissette, I. Chen, I.
1670	             Hussain, B. Wen, K. Tiruveedhula, "YANG Data Model for
1671	             MPLS-based L2VPN", draft-ietf-bess-l2vpn-yang-06.txt,
1672	             February 17, 2018

[minor] It looks like this might be the reference for ietf-l2vpn and
ietf-pseudowires, but it is not used anywhere in the document.


1674	   [draft-ietf-rtgwg-ni-model-12] L. Berger, C. Hopps, A. Lindem, X.
1675	             Liu, "YANG Model for Network Instances", draft-ietf-rtgwg-
1676	             ni-model-12.txt, March 19, 2018

[minor] This reference is obsolete and has been replaced by rfc8529.


1678	Appendix A.  Data Tree Example

1680	A.1 Bridge scenario

1682	This section contains an example for bridge scenario in the JSON
1683	encoding [RFC7951], containing both configuration and state data.

[major] There's no reference to rfc7951; make it Informative.

[] I don't have any experience in the JSON encoding.  Was a tool used
to create the examples?  If so, it may be a good idea to mention it.
Also, has anyone (besides the authors) reviewed the examples?


...
1714	{
1715	"ietf-interfaces:interfaces":{

[major] The "interfaces" container in the module doesn't use the "if" prefix.