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

Jan Lindblad <janl@tail-f.com> Thu, 08 April 2021 12:39 UTC

Return-Path: <janl@tail-f.com>
X-Original-To: pim@ietfa.amsl.com
Delivered-To: pim@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id F082A3A15B3; Thu, 8 Apr 2021 05:39:00 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.898
X-Spam-Level:
X-Spam-Status: No, score=-1.898 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id I6V-eKVb9UbF; Thu, 8 Apr 2021 05:38:56 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id 7A0143A15AA; Thu, 8 Apr 2021 05:38:56 -0700 (PDT)
Received: from [192.168.1.155] (213-67-237-150-no99.tbcn.telia.com [213.67.237.150]) by mail.tail-f.com (Postfix) with ESMTPSA id 470111AE034E; Thu, 8 Apr 2021 14:38:49 +0200 (CEST)
Content-Type: text/plain; charset="us-ascii"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
From: Jan Lindblad <janl@tail-f.com>
In-Reply-To: <DB6PR0701MB2167BF2C737C55ACFE752F6496749@DB6PR0701MB2167.eurprd07.prod.outlook.com>
Date: Thu, 08 Apr 2021 14:38:47 +0200
Cc: "yang-doctors@ietf.org" <yang-doctors@ietf.org>, "draft-ietf-pim-igmp-mld-proxy-yang.all@ietf.org" <draft-ietf-pim-igmp-mld-proxy-yang.all@ietf.org>, "pim@ietf.org" <pim@ietf.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <1C6422C0-EB2F-4E48-8B82-29B2B7D72E88@tail-f.com>
References: <161650157841.14703.6401789949073298466@ietfa.amsl.com> <DB6PR0701MB2167BF2C737C55ACFE752F6496749@DB6PR0701MB2167.eurprd07.prod.outlook.com>
To: Hongji Zhao <hongji.zhao@ericsson.com>
X-Mailer: Apple Mail (2.3608.120.23.2.4)
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/LMzloerxB9Oz69f0d_3lFb1rm6o>
Subject: Re: [pim] Yangdoctors early review of draft-ietf-pim-igmp-mld-proxy-yang-03
X-BeenThere: pim@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Protocol Independent Multicast <pim.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/pim>, <mailto:pim-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/pim/>
List-Post: <mailto:pim@ietf.org>
List-Help: <mailto:pim-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/pim>, <mailto:pim-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 08 Apr 2021 12:39:01 -0000

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

> Hi Jan,
> 
> Please check in line.  Thanks a lot! 
> 
> BR/Hongji
> 
> -----Original Message-----
> From: Jan Lindblad via Datatracker <noreply@ietf.org> 
> Sent: Tuesday, March 23, 2021 8:13 PM
> To: yang-doctors@ietf.org
> Cc: draft-ietf-pim-igmp-mld-proxy-yang.all@ietf.org; pim@ietf.org
> 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 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 {
> [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
> 
>