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

[ questions ]

* What's the origin for using rt-types:timer-value-seconds16 for the group
  and source expire value?  I spent some time with 2710 and 3810 looking
  for where this might have come from, but perhaps I missed something.

I'm almost hitting the "DISCUSS" button here over the fact that the shepherd writeup warned in several places that the YANG in this document had issues.  Were these resolved?

> There was an early YANG doctor review that found some issues. I believe the should be resolved. There are some errors compiling the model though.

> Early YANG doctor review was done. It would be good to get another check to see if everything is fine now.

> There are some minor nits as found by the tool, and YANG validation has a few issues.

> The early YANG doctor review only found minor issues, which should be addressed in version 08.  [We're at -16 now; was this never updated?]

Section 3:

* "The YANG data model defined in this document conforms to the Network Management Datastore Architecture (NMDA) [RFC8342]." -- This text also appears in Section 2.

Section 5:

* I concur with Roman's suggestion to refer here to RFC 4541.

Section 6:

* I suggest this be split into two subsections, which in my experience is more conventional (but not required).

Other:

There's a bug in whatever rendering engine was used here.  The footer for page 1 says the document expires January 2021, but all others say 2020.

** It caught me by surprise for a standards track document not to use RFC2119 words in a normative way

** Section 2.2.  Per “On the other hand, operational state parameters are not so widely designated as features …”, I didn’t follow the intent of this paragraph.  I’m not sure what it means to designate a operational state parameter.

** Section 5.  It would be worth noting that devices that use this YANG model should heed the Security Considerations in Section 6 of RFC4541.

** Section 5.  Per “If unauthorized action is invoked, the IGMP and MLD Snooping group tables will be cleared unexpectedly”, could the impact of this table flush please be explicitly stated.

** Editorial

-- Section 2.  Editorial. s/to avoid bandwidth waste/to avoid wasting bandwidth/
-- Section 5.  Editorial.  The sentence “Some of the actions in this YANG module may be considered sensitive or vulnerable in some network environments. “ appears to be included twice.  It’s only needed once.

Thank you for the work put into this document. On an editorial note, I found sometimes the text difficult to read (I am not an English-native speaker) due to very long sentences: e.g., the first 2 paragraphs of section 2.2.

Please find below a couple of non-blocking COMMENTs (and I would appreciate a reply to each of my COMMENTs).

I hope that this helps to improve the document,

Regards,

-éric

== COMMENTS ==
I am not a big expert in MLD or WiFi, but, it seems to me that this YANG module covers only the important use case of switches. But, if not mistaken, some WiFi access points also do a similar job and translates L2 mcast addresses into a series of L2 unicast addresses. Was it considered in this model ?

-- Section 1.1 --
English is not my native language, but, I have hard time to parse "mrouter: multicast router, which means nodes attached to a switch have multicast routing enabled" is a switch or a router ?

-- Section 2.3 --
No need to reply on this one, but, I really do not like separating the IPv4 and IPv6 branches of the trees. It looks to me like doubling the operational cost but not sharing common properties; hence, diminishing the usefulness of this document. I made the same comment for RFC 8652 and there is really no need to repeat what I consider as mistakes.

-- Section 4 --
The description parts of the YANG module are quite short so I may have overlooked some parts of it... so bear with my two COMMENTs below:

1) for a MLD group, why having the mac-address leaf ? AFAIK, the mac-address is derived from the group IPv6 mcast address. Storing related information twice in a model looks bad to me (I am a big fan of the SQL normal forms...)

2) some switches does not snoop MLD for link-local groups (sollicited node mcast for example). Is this feature listed in the list of features ?

Thank you for addressing my Discuss (and Comment!) points!

Discussed cleared.  Previous discuss comment:

Hi,

I appreciate that this YANG model has already passed a YANG doctor review, but this discuss is to understand the reasoning as to why both IGMP snooping and MLD snooping are in the same YANG module, yet have top level features to separate their functionality.

    4. IGMP and MLD Snooping YANG Module

      feature igmp-snooping {
        description
          "Support IGMP snooping.";
        reference
          "RFC 4541";
      }

      feature mld-snooping {
        description
          "Support MLD snooping.";
        reference
          "RFC 4541";
      }

It seems strange to me to have the entire YANG Model split under two separate feature statements.
I believe that it would have been better to split this into two separate YANG models, both following the same structure.  Possibly, a common YANG module could have been used to share groupings and definitions, but even then duplicating the contents of the model so that the description statements could be correct/accurate would be more helpful.

Thank you for your work on this document and YANG model.  I also have a few minor comments/suggestions that may improve the document and YANG module:

    2. Design of Data Model

    An IGMP/MLD snooping switch [RFC4541] analyzes IGMP/MLD packets and sets
    up forwarding tables for multicast traffic. If a switch does not run
    IGMP/MLD snooping, multicast traffic will be flooded in the broadcast
    domain. If a switch runs IGMP/MLD snooping, multicast traffic will be
    forwarded based on the forwarding tables to avoid wasting bandwidth. The
    IGMP/MLD snooping switch does not need to run any of the IGMP/MLD
    protocols. Because the IGMP/MLD snooping is independent of the IGMP/MLD
    protocols, the data model defined in this document does not augment, or
    even require, the IGMP/MLD data model defined in [RFC8652].
    The model covers considerations for Internet Group Management Protocol
    (IGMP) and Multicast Listener Discovery (MLD) Snooping Switches
    [RFC4541].

It wasn't clear to me what the final sentence was trying to say.  Perhaps it should be merged with the penultimate sentence in the paragraph?


    The YANG module includes IGMP and MLD Snooping instance definition,
    using instance in the scenario of BRIDGE [dot1Qcp] and L2VPN [draft-
    ietf-bess-l2vpn-yang]. The module also includes actions for clearing
    IGMP and MLD Snooping group tables.

I find the use of the terminology of "scenario of " to be somewhat strange.  I would probably have
referred to these a "L2 forwarding pradigms" or "L2 forwarding instances".  If this terminology is changed then it would need to be fixed elsewhere in this document and the YANG model.


    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.

With NMDA, the server also has the option of not returning a value for a given item of operational data (RFC 8342, section 5,3, paragraph 4).  Although this doesn't conform to the data model, the semantics are well defined - i.e. the client cannot infer anything about the value that has not been returned.

    2.3. Position of Address Family in Hierarchy

    IGMP Snooping only supports IPv4, while MLD Snooping only supports IPv6.
    The data model defined in this document can be used for both IPv4 and
    IPv6 address families.

    This document defines IGMP Snooping and MLD Snooping as separate schema
    branches in the structure. The benefits are:

    *  The model can support IGMP Snooping (IPv4), MLD Snooping (IPv6), or
    both optionally and independently. Such flexibility cannot be achieved
    cleanly with a combined branch.
    
    *  The separate branches for IGMP Snooping and MLD Snooping can
    accommodate their differences better and cleaner. The two branches can
    better support different features and node types.
    
I would suggest rewording this first sentence to something like:

"Having separate branches for IGMP Snooping and MLD Snooping allows minor differences in their
 behavior to be modelled more simply and cleanly".

    3. Module Structure

    A configuration data node is marked as mandatory only when its value
    must be provided by the user. Where nodes are not essential to protocol
    operation, they are marked as optional.  Some other nodes are essential
    but have a default specified, so that they are also optional and need
    not be configured explicitly.

This paragraph seems to just describe standard YANG modelling and can be removed.

    3.1. IGMP Snooping Instances

    The value of scenario in igmp-snooping-instance is bridge or l2vpn. When it is bridge, igmp-snooping-instance will be used in the BRIDGE

As per previous comments, this first sentence does not read well for me.


    The values of bridge-mrouter-interface, l2vpn-mrouter-interface-ac, l2vpn-mrouter-interface-pw are filled by the snooping device dynamically. They are different from static-bridge-mrouter-interface, static-l2vpn-mrouter-interface-ac, and static-l2vpn-mrouter-interface-pw which are configured

Ideally, these static nodes would not have been necessary, instead relying on the NMDA split between configuration and state, but that would probably require the default model to always allow them to be statically configured.  In NMDA, features can be implemented per-datastore but it is not clear how well that would work here.

      units one-tenth-second;

Perhaps "units deciseconds" would be better?

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

      leaf num-query {
        type yang:counter64;
        description
          "The number of Membership Query messages.";
        reference
          "RFC 2236";
      }

For these counters, rather than "num-XXX", I think that they would be better as "XXX-count", or if these relate to the number of packets "XXX-pkts" (as per RFC 8343).

           container statistics {
             description
               "The interface statistics for IGMP snooping";

             container received {
               description
                 "Statistics of received IGMP snooping packets.";

               uses igmp-snooping-statistics;
             }
             container sent {
               description
                 "Statistics of sent IGMP snooping packets.";

               uses igmp-snooping-statistics;
             }
           }

Should the descriptions for received and sent be for "snooped IGMP packets"?  The equivalent MLD structure probably also needs a similar fix.