Subject: Re: [pim] Yangdoctors early review of draft-ietf-pim-igmp-mld-proxy-yang-03

From: Hongji Zhao <[email-masked]>
Date: Thu, 8 Apr 2021 08:38:52 +0000

Hi Jan,

Please check in line.  Thanks a lot! 

BR/Hongji

-----Original Message-----
From: Jan Lindblad via Datatracker <[email-masked]> 
Sent: Tuesday, March 23, 2021 8:13 PM
To: [email-masked]
Cc: [email-masked]; [email-masked]
Subject: Yangdoctors early review of draft-ietf-pim-igmp-mld-proxy-yang-03

Reviewer: Jan Lindblad
Review result: Ready with Nits

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.
[Authors] accepted and updated in draft-ietf-pim-igmp-mld-proxy-yang-04

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.
[Authors] Currently the pim-base module has been approved. If it is modified later, igmp proxy model could be revised accordingly.

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 [email-masked] 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 {
[Authors] I use revision 2018-04-16 for ietf-pim-base.yang, it is the latest version now.

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.

[Authors] I have set filter-mode as mandatory. 

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.
[Authors] accepted and updated in draft-ietf-pim-igmp-mld-proxy-yang-04

6) The indentation is mostly good, but is off in a few places. In particular, I noticed lines 193-197, 295-299, 316.
[Authors] accepted and updated in draft-ietf-pim-igmp-mld-proxy-yang-04

/Jan

---

From: Jan Lindblad <[email-masked]>
Date: Thu, 8 Apr 2021 14:38:47 +0200

Dear Hongji,

I looked at your comments. They look good. I can add that on issue #3, I agree that what you do is fine when you use the 2018-04-16 version of ietf-pim-base.yang.

Best Regards,
/jan