Early Review of draft-ietf-pim-igmp-mld-proxy-yang-03
review-ietf-pim-igmp-mld-proxy-yang-03-yangdoctors-early-lindblad-2021-03-23-00

This is the YD early review of draft-ietf-pim-igmp-mld-proxy-yang-03 as
requested by the PIM WG.

I find the proxy module to be short and clean. A few minor adjustments are
required and a couple more I would recommend, so I will declare this module as
"ready with nits".

1) The first sentence in section "1. Introduction" states that "This document
defines a YANG [RFC6020] data model ..." This reference should be updated to
RFC 7950 since the module is declared to be a YANG 1.1 module, and YANG 1.1 is
defined by the latter RFC.

2) The proxy module imports pim-base. The pim-base YANG module is extending
ietf-routing in a way that in my judgement is not exactly following the
directives for extensions stated in ietf-routing (this has been mentioned in
previous reviews). The pim-base module is out of scope for the current review,
but I mention this here since the proxy module under review will cement the
current way pim-base extends ietf-routing, as it contains must expressions with
the path to the pim configurations.

3) The must expressions on line 192 and 294 are referring to the pim-base
module use the wrong leaf name for the interface name. At least in the version
of ietf-pim-base@2017-03-09.yang that I have, which seems to be the latest
version posted on the IETF YANG github repo. With this version, "pim-base:name"
needs to be changed to "pim-base:interface" to match the pim-base module (or
pim-base could be changed).

ietf-igmp-mld-proxy.yang:
...
          leaf interface-name {
            type if:interface-ref;
            must "not( current() = /rt:routing"+
           "/rt:control-plane-protocols/pim-base:pim"+
           "/pim-base:interfaces/pim-base:interface"+
           "/pim-base:name )" {

ietf-pim-base.yang:
...
  revision 2017-03-09 {
...
        list interface {
          key "interface";
          description
            "List of pim interfaces.";
          leaf interface {

4) The module contains a list of multicast source addresses. The leaf
filter-mode decides whether the addresses listed should be read as allowed
(include) or disallowed (exclude). Since the interpretation changes quite
dramatically with the value of filter-mode, I would argue that filter-mode
should either be mandatory or have a default. Currently a conforming
implementation could leave filter-mode out, making it hard to correctly
interpret the response (with possible security implications?).

An even clearer approach might be to have two separate source address lists,
one for included addresses, one for excluded. If for some reason both must not
be used simultaneously, they could be placed in a YANG choice.

5) While many IETF YANG modules have very short prefixes (such as if, rt, inet,
yang) I think readability increases and opportunity for confusion by prefix
collision decreases if a longer prefixes were chosen. I would suggest using
prefix igmp-mld for example, rather than the current mnemonic prefix imp.

6) The indentation is mostly good, but is off in a few places. In particular, I
noticed lines 193-197, 295-299, 316.

/Jan