A YANG Data Model for the Internet Group Management Protocol (IGMP) and Multicast Listener Discovery (MLD)
draft-ietf-pim-igmp-mld-yang-15

(1) I support Ben Kaduk’s DISCUSS about privacy considerations.

(2) Thanks for Section 1.3.  This upfront cross-reference was very helpful!

(3) Section 2.2.
   On the other hand, operational state parameters are not so widely
   designated as features, as there are many cases where the defaulting
   of an operational state parameter would not cause any harm to the
   system, and it is much more likely that an implementation without
   native support for a piece of operational state would be able to
   derive a suitable value for a state variable that is not natively
   supported.

I struggled to understand this very dense single sentence paragraph.
-- What does it mean for “operational state parameters are not so widely designated as features”?
-- What is a “defaulting of an operational state parameter”?

(4) Section 3.  Does the sentence “This model augments the core routing data model specification in [RFC8349]” suggest that this draft should update [RFC8349]?

(5) Reference Nits
** Section 1.  I would have expected a reference to NMDA (i.e., [RFC8342]).  This is done later in the draft but not the first time it is mentioned (here).

** idnits returned the following valid reference issues:

  == Missing Reference: 'I-D.ietf-netconf-yang-push' is mentioned on line
     178, but not defined

  == Missing Reference: 'RFC 8446' is mentioned on line 1910, but not defined

  == Missing Reference: 'RFC8341' is mentioned on line 1912, but not defined

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

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

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

  ** Downref: Normative reference to an Informational RFC: RFC 3569

  ** Obsolete normative reference: RFC 5246 (Obsoleted by RFC 8446)

  ** Obsolete normative reference: RFC 6536 (Obsoleted by RFC 8341)

(6) Editorial Nits

** Section 2.1.  The sentence “Even though there is no protocol specific notifications are defined in this model, the subscription and push mechanism defined in [I-D.ietf-netconf-subscribed-   notifications] and [I-D.ietf-netconf-yang-push]” doesn’t parse.  Likely remove the “are”.

** Section 2.1.  Per the sentence “Depending on the implementation choices, some systems may not allow some of the advanced parameters   configurable”, what is a “advanced parameters configurable”?  I think there is a typo there.

**  Section 2.3.  Per “The current document defines …”, shouldn’t this just be “The document defines …” since when published as an RFC there would be no notion of versioning as suggested by “current”?

** Section 2.3.  s/supports and only supports/only supports/

** Section 4.  Typo.  s/refered/referred/

** Section 4. Missing space.  /Querier.In RFC3376,/Querier. In RFC3376,/

Thanks for the work everyone has put into this document. 

I only have a couple of comments (but one important one about the 2 branches) and a couple of nits.

== COMMENTS ==

-- Section 2.1.1 and section 2.1.2 --

Those sections are about configuration parameters not covered at global or interface level. But, what about operational states, can the reader assume that they are all covered by this document ? It is really unclear.

-- Section 2.3 --

As I am not a multicast expert, I did not put a DISCUSS on this one. But, are MLD and IGMP so different? Why having TWO different branches for each address family... For SNMP, RFC 4292/4293 was made protocol version independent which is a big plus IMHO for operations. In any case, there should be more explanations why there are two branches than the one paragraph/two sentences in section 2.3. 
Moreover, it seems that the two schema branches are quite similar so having one protocol version independent branch appears to be doable.


== NITS ==

-- Section 1 --

Add a reference to NMDA (expanding the acronym is not really sufficient, state RFC 8342) ?

Expand CLI even if well-known.

I support Ben's DISCUSS and will be watching that discussion.

Other than my Discuss point (which seems to be resolvable with no
change), basically all I have is editorial nits -- thanks for
the well-written document!

Section 2.1

                                   Even though there is no protocol
   specific notifications are defined in this model, the subscription

nit: s/there is// -- the "are defined in this model" takes care of
the grammatical necessities here.
   and push mechanism defined in [I-D.ietf-netconf-subscribed-
   notifications] and [I-D.ietf-netconf-yang-push] can be used by the
   user to subscribe notifications on the data nodes in this model.

nit: "subscribe to notifications"

   The model contains all basic configuration parameters to operate the
   protocols listed above. Depending on the implementation choices,

nit: "all the basic"

Section 4

     grouping interface-common-config-attributes {
       [...]
       leaf query-interval {
         [...]
         description
           "The Query Interval is the interval between General Queries
            sent by the Querier.In RFC3376, Querier's Query
            Interval(QQI) is represented from the Querier's Query
            Interval Code in query message as follows:

nit: one or two (not zero) spaces after the end of the sentence.
nit: "In RFC3376, the Querier's Query Interval (QQI)"

       leaf exclude-lite {
         if-feature intf-exclude-lite;

side-note: I misparsed this (I think) the first few times I read it,
since "exclude lite" can be taken as an imperative command to not use
the lite version.  But it seems this is really just an ordinary
feature-enablment tag about whether to use the EXCLUDE filtering that
is available in the lite version of the protocol.  I don't know whether
reordering to "lite-exclude" would be a net win or net loss due to
causing some other confusion, though.

     grouping interface-state-attributes-igmp {
       [...]
       list group {
         [...]
         list source {
           [...]
           list host {
	     [...]
             leaf host-address {
               type inet:ipv4-address;
               description
                 "The IPv6 address of the host.";

nit: "ipv6-address"

     grouping interface-state-group-attributes-igmp-mld {

nit: I thought most of the other shared groupings just didn't use a
suffix, as opposed to using the "-igmp-mld" combined suffix.
(Similarly for interface-state-source-attributes-igmp-mld and
interface-state-host-attributes-igmp-mld, which does cause me to wonder
if I'm not perceiving "most" correctly.)

Section 5

   igmp-mld:global

     This subtree specifies the configuration for the IGMP attributes
     at the global level on an IGMP instance.  Modifying the
     configuration can cause IGMP membership deleted or reconstructed
     on all the interfaces of an IGMP instance.

nit: "to be deleted or reconstructed"  (Similarly for the following
paragraphs.)

The description of the considerations for unauthorized read access are
fairly generic and do not specify specific potential harms, but I will
not insist on any changes here.

One very minor question about data tree representation:

Should this figure have vertical bars from rw ssm-map down to the plus for rw ssm-map-source-addr?
                +--rw ssm-map*
                |       [ssm-map-source-addr ssm-map-group-policy]
                |       {intf-ssm-map}?
                |  +--rw ssm-map-source-addr     ssm-map-ipv4-addr-type
                |  +--rw ssm-map-group-policy    string
                +--rw static-group* [group-addr source-addr]
                |       {intf-static-group}?
                |  +--rw group-addr
                |  |       rt-types:ipv4-multicast-group-address
                |  +--rw source-addr
                |          rt-types:ipv4-multicast-source-address

Thus making the unattached brances being attached?

Two quick questions: 

1) Not sure about the current practice about YANG models but shouldn’t this document eventually update RFC8349?

2) Also maybe it would make sense to discuss the sensitivity of explicit-tracking separately in the security consideration section?

I do have a general concern with the document in relation to its handling of multiple protocol versions. There are features in the yang models that should be conditional but they do not seem to be. Here are some examples.

* The source specific features are to be used with IGMPv3 and MLDv2 and will not work with the earlier versions
* The router alert check is not optional for MLD or IGMPv3, but is required to be disabled for compatibility with earlier versions of IGMP. I would also make this feature conditional on the IGMP version. If not you need to rethink the defaults for this.

I would like to understand the authors' views on how they plan to address the potential consistency issues due to these features being unbound in the model. I would be fine if it is either addressed with yang constructs or with some explanatory text to this point.