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

Hongji Zhao <hongji.zhao@ericsson.com> Wed, 24 March 2021 02:20 UTC

Return-Path: <hongji.zhao@ericsson.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 C50CC3A1D87; Tue, 23 Mar 2021 19:20:22 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.352
X-Spam-Level:
X-Spam-Status: No, score=-2.352 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.251, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_MSPIKE_H2=-0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=ericsson.com
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 CRfn_Q3dxVDa; Tue, 23 Mar 2021 19:20:17 -0700 (PDT)
Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-eopbgr20043.outbound.protection.outlook.com [40.107.2.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id BE5DB3A1D84; Tue, 23 Mar 2021 19:20:13 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=cBh6d34zfc6hlxhDrFhEYQEDJ3ScU+Xtq7Uo/6CjaS6emm4mDFjfZg3wHbdwK5xm2yf7R3Xu4eY4oEy29ckfXI3rm65GqJVjwKDfWf16kzXfzk32iezV++IIGMb1JV4/a16gJwl5TFA9mcjd5zzNnT6lNJWLOiC+4063w2b7uC3sWU1ToFQJ+dpm98FRASdEOpvMqNY6kMx47h9njTOm0QnhaolQE0fd7uRHNctAq90ENNS/bWIkVG2pG0qXOoyHVr1w/LhhMn+UC0UM1SSEfXi3kFsnZ+Gmxq3ams27E6RsIXPdKbDwE985sK+etx+diKWrDaJrbbQhUErr9UmjUQ==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=hHaV0qz8GMiz0jIpZ0EdD+DfA8X1CJ+n6QF4bGt8o94=; b=NOfkxSReN+ji/wlx8u/4B9RfkaJ5xobsU0/oDPURS0Y10fkn0pgfylwvAhdvtaGFA6AH0K0jkinPaWj45dwdpwfGlWGeYheqpdzodEmpnBw1EBVuj3N8GlYoZrfocn6/UV/xqwY8IltaWGHhkmhkZxCI7C7iXrqr1pRV7vhDHG8eLX4/qEQFY9Zax2h4DGcjKBllFN15zFmv4xWPZBkc3KoU8cK/9m78FKEap4D3kPY7oouW5mGDox9XWN+xMnHSfpiqFgCg3IhPdB1bJl+Wd4a4f0ZMdkKqrMiQe6qKFAYOQnSjwaNNzMY37u9Q0RAOy7QQQMtTxDX5+P4Esb/Raw==
ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=ericsson.com; dmarc=pass action=none header.from=ericsson.com; dkim=pass header.d=ericsson.com; arc=none
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ericsson.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=hHaV0qz8GMiz0jIpZ0EdD+DfA8X1CJ+n6QF4bGt8o94=; b=FzxW86Zg+EoJGv23fUb3EQxQrTx0HyDnxHuGx0RsDtcqytIS65+v8KcrUs/7o7gBkAsW4eMmR1lFa0R/JYL4hnBHIeSt39F1KLrgyubhoVnkj156QV4dp0sZhxdq2j004vIZsClAeJeSOhWmbRdFhzf98Hj6xUwx/oWUWM8cvGo=
Received: from DB6PR0701MB2167.eurprd07.prod.outlook.com (2603:10a6:4:50::22) by DB7PR07MB3931.eurprd07.prod.outlook.com (2603:10a6:5:11::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3977.10; Wed, 24 Mar 2021 02:20:08 +0000
Received: from DB6PR0701MB2167.eurprd07.prod.outlook.com ([fe80::60ce:1206:3b99:540f]) by DB6PR0701MB2167.eurprd07.prod.outlook.com ([fe80::60ce:1206:3b99:540f%12]) with mapi id 15.20.3977.019; Wed, 24 Mar 2021 02:20:08 +0000
From: Hongji Zhao <hongji.zhao@ericsson.com>
To: Jan Lindblad <janl@tail-f.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "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>
Thread-Topic: Yangdoctors early review of draft-ietf-pim-igmp-mld-proxy-yang-03
Thread-Index: AQHXH93mnMmRyA6T+0KEP7kFPokK16qSaG8Q
Date: Wed, 24 Mar 2021 02:20:08 +0000
Message-ID: <DB6PR0701MB216787D9C5457348D5D5D5FE96639@DB6PR0701MB2167.eurprd07.prod.outlook.com>
References: <161650157841.14703.6401789949073298466@ietfa.amsl.com>
In-Reply-To: <161650157841.14703.6401789949073298466@ietfa.amsl.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
authentication-results: tail-f.com; dkim=none (message not signed) header.d=none;tail-f.com; dmarc=none action=none header.from=ericsson.com;
x-originating-ip: [119.28.15.239]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 02b2d93c-ed06-40e7-a47e-08d8ee6b58a5
x-ms-traffictypediagnostic: DB7PR07MB3931:
x-microsoft-antispam-prvs: <DB7PR07MB39319BF0521691CB5EDB86DE96639@DB7PR07MB3931.eurprd07.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:10000;
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: 5+2WJunRy2SgkoYEroj3URILsQiHoSI4wMXJWUbmABvbsw0jg7jfRnUpzsCkUbPAICsVH5tUPfjYeDUL5onTYLVxGWiXAFhrS4Im9AQQWQTIwS+VlufyvgzGbI4FXYHsYYmcWKTe2fz3x+oNKgyfueobZtKCQY9TUB/0tbNpvBwROB37MWn7zLmkcc6BrXl8L7eFeLJMAAsF+nwEPBust3P0ZLdZom56/9A9liJxu5rkIkzb5tlzDIGpJ+T+OQe20/j2JNGe+vDMldqkGVJuUMQ+YQgv0Dd8lgBbMEIZEBuNsDu/X058j5e4hCbM88dEvUKeU0N7jEpaC2v58MjZppzmzWv/xq+Dmat4hNq8JT6Vt0rcH8GvI/SI1JHqzpPR7+d6aziwZTE38FXnRFD0tTE1VCiTxSZZA3mJmZk/dWEq6KZ4TqzRD2GRFqplhtWUmeBElMELPkdoBIr+TNX9oSTWAnYFUptxMDNg5vrWnlCRQ4cruc/pnqSXcDNfLb+sYyUgkgHbmhh5e8XHRD9Uzs2kQgUIo+qfhRXc4IxitbOlIhrkjzJScL0ogMM4CIeuB8I/04ikj9gNIXJdDaiK+0XEKb2bye9NT4j3vPnvGXBo7EZtqrHVSQSNFF/zGeiQ63q+LBddlT4ubbh6ZtBAiDd16qG+pVB5rLImRO38018=
x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DB6PR0701MB2167.eurprd07.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(396003)(136003)(39860400002)(376002)(346002)(9686003)(5660300002)(44832011)(26005)(86362001)(8676002)(8936002)(55016002)(2906002)(83380400001)(38100700001)(76116006)(4326008)(52536014)(54906003)(110136005)(478600001)(33656002)(316002)(71200400001)(6506007)(186003)(53546011)(7696005)(66946007)(66556008)(64756008)(66446008)(66476007); DIR:OUT; SFP:1101;
x-ms-exchange-antispam-messagedata: =?utf-8?B?NlRrVEVPRGczbStnSG1BTHN0am9DckI4MWozbHl1T2RIczhHeTVNclgra1Ns?= =?utf-8?B?MUpJRWpkT2xvc09Qbk92SkxhdGJHa2tYWnQxWC9yM1h5OTJBSTNXcE9lazYz?= =?utf-8?B?RkxDamhZRDF3WlUwNVVHVm1CS25IRXM5YmptbjZIQm9wVEtCM09ESDRUR0ll?= =?utf-8?B?SW4rNjZqUkFQR2twUk5QWHl5emZ1bGVuNUtydWgxNmNuNEtxUmM0bGdYNXhx?= =?utf-8?B?eTFaT2tWMXNDQmozVjZ0VXJkLzd3NHJRTTdRMGpua3JqOXFVUVBBSkdzS29B?= =?utf-8?B?b0NaOVBxYU5vV3l2VW1pODNOOWlyUlFHbGNCT3FpRHZIREF1NXFIRk96Vm9s?= =?utf-8?B?a0MyWjJoY3ZPNW90YkNsbTh0emlJYnVVZmxzczVyNVgzZjlQY2hQcGUwY0NE?= =?utf-8?B?T0EzKzhEbFpMVVV5M2F5cU1NTWovUGowT1kxN1dSbkl0MW5tQjVHN3JGQStN?= =?utf-8?B?QUNrODJwTTNINUdiOHAremR0S0lMT2ZFdElUNTFVMVFOZUhrWUkvakJlT1ZU?= =?utf-8?B?a0NreWZzZ0JmdC9RZ1lNa0Yvd25QejVFQm1jQkY0R3d6YysydFRaRmZWSDBn?= =?utf-8?B?K3g1R2xINVJ5MVBVbzFrZG5rY3FLZ1BFVWdxVFpqQXBsTmdpNnFhU0lKeENU?= =?utf-8?B?cjhlb3RzS1Zha1F1YmF2SzUzQTVPKzdyQnpNOFQ1MzJrRkNDeWdFMGw2cTM2?= =?utf-8?B?ZTNrM2VMSVN3b3ZuK2xXb05SSjJGVG15SDRiUWJvNmx4MXQ4WVYvM3lUTlRn?= =?utf-8?B?cWxqS1dMRWJaZno3NmIwcEZ0c0FGbCt2SGJjYUZoYU83WlZ6MnFxZWxuWWhx?= =?utf-8?B?cWt0SG9CdmxRZXA3Q3gvOU1tR0svVGZUM0Y5cnprSndXajBORHdCNy9tTTds?= =?utf-8?B?Ri9TTmNtWWZpZTljSGw1QzVGMFJpZlVhNDg0MGV5ZktGR1pFV3NvcjBnQUxS?= =?utf-8?B?OThndnk4VnMyazc1YlNqV2prdTNyeUhpa0YyK1dITzh0NS91YytRcTdJeW85?= =?utf-8?B?U1VRN0pLZlpUOGhYenRuRyt2d1hTYVhhODc4RW9PdXpDZHlFcmlCQlhvRGhq?= =?utf-8?B?UFVKb1AwNDB5VHFkVHNzNnRSWTlpcHNxYUU4RU1tTzNFeWdqZm9yT0JjOUlD?= =?utf-8?B?dDJIWDIxSGRkdjg3TDhmVXNtOHBYNFQ5R1NUUWRtMm1vNU1BMTF5NWRzbVB0?= =?utf-8?B?R1RORUNDYTI0ZGtXODFES3VYbXV3d3VwS0pmejIreVFoKzFPamQzYkhIdFd6?= =?utf-8?B?NkJUTlp5L3AvNm5DTzZZZlFyUHAyOFJUY09tZEQyMklVYUl3Z3FUQXdpcGpm?= =?utf-8?B?YXRJUzlTeitCNGh5MTJ3aUNnbXp6QUh1M3VsSTc2TWViRlBRNExaVjlOWGEx?= =?utf-8?B?M09jN0hEaEQya0xUTGhrWENOVmdrUHBBNFd5YUpuZnNSR1l6YklEalJRV2Uw?= =?utf-8?B?TUkyRFJEWmp6eXZkR1M1NXdsbzBZS3lsUWJtcmZHNU1VVXdHU05rcDdRYmdu?= =?utf-8?B?ZUR5NWYyZWUyWlp2MlpCODR6MUpsVzFWMVZmQ2UybklVeEtNOXMwcTZqRkdh?= =?utf-8?B?OE5NelRRbDVyTnlaUU5Yeno5UkxRbnI3YzhzQy9sS3pTM2FGSXc3QitoSHVl?= =?utf-8?B?YktKcjlMUEhRelZQcEhXWk51UlF6bDB3NFdVeUNEODNabUNrNHB2YVdOS0JE?= =?utf-8?B?NUlrRHBVQ3BTUjh0enZQU1VPakVXT21Tak1ac2dabFJZVWcxVlFZTkFGS3ZH?= =?utf-8?Q?ygf2NbebK4dQrRLiX0dZYZOGEfUhOOnMjerTFCE?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-OriginatorOrg: ericsson.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DB6PR0701MB2167.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 02b2d93c-ed06-40e7-a47e-08d8ee6b58a5
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Mar 2021 02:20:08.8077 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 92e84ceb-fbfd-47ab-be52-080c6b87953f
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: BtvkQuOHaa6N9I10xBJ4v/ho2l61noTDCb0Tug9Z4YHv7qwR6gts43BAgOiyKvRLjTKM5/4KXiMvMOLVE9pGZqNKb+cDVEyJ8sYTfpzrbPs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR07MB3931
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/-Dnv7eJHvLK-LaUlOvzpGeaApR4>
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: Wed, 24 Mar 2021 02:20:23 -0000

Hi Jan,

Thanks a lot for your comments. We will address them as soon as possible.

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.

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