Re: [mpls] Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]

"Rob Wilton (rwilton)" <rwilton@cisco.com> Thu, 19 March 2020 16:04 UTC

Return-Path: <rwilton@cisco.com>
X-Original-To: mpls@ietfa.amsl.com
Delivered-To: mpls@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 3D2303A0837; Thu, 19 Mar 2020 09:04:33 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -9.6
X-Spam-Level:
X-Spam-Status: No, score=-9.6 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_PASS=-0.001, URIBL_BLOCKED=0.001, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.com header.b=f5CE2gYT; dkim=pass (1024-bit key) header.d=cisco.onmicrosoft.com header.b=CgIlrCWN
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 bMDjjdbKzmc2; Thu, 19 Mar 2020 09:04:28 -0700 (PDT)
Received: from alln-iport-1.cisco.com (alln-iport-1.cisco.com [173.37.142.88]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id EA1573A085C; Thu, 19 Mar 2020 09:04:27 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=40258; q=dns/txt; s=iport; t=1584633868; x=1585843468; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=4HuZbMONDgmIC++QRhkrkcX8Yqz2dz7JdxEbCXh2T1Y=; b=f5CE2gYTOjgCkCItwxGCuj+8Kzb1ojIAEs5ix9xmuxovXmnzxsxoZNGW Ca0e1rigywKgCbjSlzx3fI7mZvfubswMwr8DKM08ZBe1OaDvdESgQCeCj xKV4PP5Y7VIdJtXJXf8i6lgt0w04ET7mMjBgWLUNu48ebtv52rEtErJ4q E=;
IronPort-PHdr: =?us-ascii?q?9a23=3AZVNPXBwigKSLPcDXCy+N+z0EezQntrPoPwUc9p?= =?us-ascii?q?sgjfdUf7+++4j5YhSN/u1j2VnOW4iTq+lJjebbqejBYSQB+t7A1RJKa5lQT1?= =?us-ascii?q?kAgMQSkRYnBZufFkz/MPnsRyc7B89FElRi+iLzPA=3D=3D?=
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0CyAABll3Ne/5hdJa1dCQ4MAQEBAQE?= =?us-ascii?q?BAQEBAwEBAQERAQEBAgIBAQEBgXuBVCknBWxQCCAECyqEFoNFA4pvgl+JZo4?= =?us-ascii?q?ygUKBEANUCQEBAQwBASMKAgQBAYRDAheCBCQ4EwIDAQELAQEFAQEBAgEFBG2?= =?us-ascii?q?FVgyFYwEBAQECARIICQQNDAEBKQkFAQsEAgEIEQQBAQECAhQLBwICAh8RFQg?= =?us-ascii?q?IAgQBDQUIDA6COUyCSgMOIAEOohQCgTmIYnV/M4J/AQEFhRENC4IMAwaBDiq?= =?us-ascii?q?FIIVLJYEfGoFBP4ERR4FPfj6CG0kBAQOBJRESBBhNgkIygiyNWCQtBYIKO4Y?= =?us-ascii?q?bmHVECoI8h1eFBkqFGIRYgkqIKZBYjiheiQKCNowsg30CBAIEBQIOAQEFgWk?= =?us-ascii?q?igVhwFTuCbFAYDY4dDAEEEhWDO4RZO4UEPXQCAYEmizslgh0BAQ?=
X-IronPort-AV: E=Sophos;i="5.70,572,1574121600"; d="scan'208";a="449877524"
Received: from rcdn-core-1.cisco.com ([173.37.93.152]) by alln-iport-1.cisco.com with ESMTP/TLS/DHE-RSA-SEED-SHA; 19 Mar 2020 16:04:26 +0000
Received: from XCH-ALN-003.cisco.com (xch-aln-003.cisco.com [173.36.7.13]) by rcdn-core-1.cisco.com (8.15.2/8.15.2) with ESMTPS id 02JG4Qu9025715 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=FAIL); Thu, 19 Mar 2020 16:04:26 GMT
Received: from xhs-rcd-001.cisco.com (173.37.227.246) by XCH-ALN-003.cisco.com (173.36.7.13) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Thu, 19 Mar 2020 11:04:25 -0500
Received: from xhs-aln-003.cisco.com (173.37.135.120) by xhs-rcd-001.cisco.com (173.37.227.246) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Thu, 19 Mar 2020 11:04:25 -0500
Received: from NAM11-BN8-obe.outbound.protection.outlook.com (173.37.151.57) by xhs-aln-003.cisco.com (173.37.135.120) with Microsoft SMTP Server (TLS) id 15.0.1473.3 via Frontend Transport; Thu, 19 Mar 2020 11:04:25 -0500
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; =?utf-8?q?b=3DK42S6YjSCRiuVKqJtB6JM6amAj/pZskM3tx8moruE6PMTSJxP0+bxA8W/JQyE?= =?utf-8?q?XQUagInjk1R4wjgNFpkyBBX4XiTjnil9lMf8285H68tHa4GREPrOv8PE4ec6RjTEh?= =?utf-8?q?wCYdjwZ2vc8EemyCEfY45RXgfmAn36hodlR6GslmXK2x2Whf1qohjjPxq0gZrO6DH?= =?utf-8?q?kSFmiUIjJWGfAGDXFELIE1rdlOvYC8F5u4PM5glpPPjfGpJ6K5VD8HlXX9WOI9pos?= =?utf-8?q?0Jk6LmPBtu7bFvZtiHXI+OWzhnyNVXJ9Fnr20vKdOYPFBUgxctBm9j9KPHeGDcike?= =?utf-8?q?6HybCDSphtNmf01tiE26w=3D=3D?=
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; =?utf-8?q?h=3DFrom=3ADate=3ASubject=3AMessage-ID=3ACont?= =?utf-8?q?ent-Type=3AMIME-Version=3AX-MS-Exchange-SenderADCheck=3B?= =?utf-8?q?bh=3D4HuZbMONDgmIC++QRhkrkcX8Yqz2dz7JdxEbCXh2T1Y=3D=3B_b=3DEEtEa7?= =?utf-8?q?K4utj6aif7BxalrWO0APme+fPJgtxPibk7qJYJx+uXNlWWzU3j/0b2sXU9j0g9gIW?= =?utf-8?q?cQBqwrhPNtsht2IXQZX8qXWs6iTZnY1+d8DFZxueakZDcOQhkfKnc7cHGU27v9Cwt?= =?utf-8?q?SELuCWTMS47VdFW6KFTpHr7qwpbGPhdsW0HvZFCH2F6tsj/mmFUhMZwr1f6ltB8g0?= =?utf-8?q?JeEtb6E/lK31Sv7hknEHEmUq5uV0hJ+KQJ6OJIqLEjpeOu8CEqvuGBXXnPpfNwMd3?= =?utf-8?q?J3x9goYSMRRFJtTMg971TAmiS7Xhl3hPwJRAWH9sDV69Sp4D218sRGvlDSg2qVHWY?= =?utf-8?q?G5jPCo1kdCg=3D=3D?=
ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=cisco.com; dmarc=pass action=none header.from=cisco.com; dkim=pass header.d=cisco.com; arc=none
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cisco.onmicrosoft.com; s=selector2-cisco-onmicrosoft-com; =?utf-8?q?h=3DFrom=3ADate=3ASubject=3AM?= =?utf-8?q?essage-ID=3AContent-Type=3AMIME-Version=3AX-MS-Exchange-SenderADC?= =?utf-8?q?heck=3B_bh=3D4HuZbMONDgmIC++QRhkrkcX8Yqz2dz7JdxEbCXh2T1Y=3D=3B_b?= =?utf-8?q?=3DCgIlrCWN4KqWyDntetsaeqIZfrEHxaEReQUHVSZlhIObLcr1hydxuauWKZ9NOw?= =?utf-8?q?1MrALfXCaW7QOkRfP5MxwL1b6xGevtA2LEzxket1kLoOE9ITEutdlXbpdmrEcLRL/?= =?utf-8?q?T5cOddHkVD1p/yv1Bg4//pNkgohcl2OMFiAqs5u9pKJY=3D?=
Received: from MN2PR11MB4366.namprd11.prod.outlook.com (2603:10b6:208:190::17) by MN2PR11MB4567.namprd11.prod.outlook.com (2603:10b6:208:26d::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2814.22; Thu, 19 Mar 2020 16:04:24 +0000
Received: from MN2PR11MB4366.namprd11.prod.outlook.com ([fe80::3:2164:a8e2:33b3]) by MN2PR11MB4366.namprd11.prod.outlook.com ([fe80::3:2164:a8e2:33b3%5]) with mapi id 15.20.2835.017; Thu, 19 Mar 2020 16:04:24 +0000
From: "Rob Wilton (rwilton)" <rwilton@cisco.com>
To: Benjamin Kaduk <kaduk@mit.edu>, "Kamran Raza (skraza)" <skraza@cisco.com>
CC: "mpls@ietf.org" <mpls@ietf.org>, "mpls-chairs@ietf.org" <mpls-chairs@ietf.org>, Nicolai Leymann <n.leymann@telekom.de>, The IESG <iesg@ietf.org>, "draft-ietf-mpls-ldp-yang@ietf.org" <draft-ietf-mpls-ldp-yang@ietf.org>, Tarek Saad <tsaad.net@gmail.com>
Thread-Topic: Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]
Thread-Index: AQHV7fEHh8C8R6fr+Eu6LYwdKXXZQqhNYQKAgALDA0A=
Date: Thu, 19 Mar 2020 16:04:24 +0000
Message-ID: =?utf-8?q?=3CMN2PR11MB43666D942EB3FEF317EFE8C6B5F40=40MN2PR11MB4?= =?utf-8?q?366=2Enamprd11=2Eprod=2Eoutlook=2Ecom=3E?=
References: <A3E24404-81B0-4B35-A04F-478FFF17F083@cisco.com> <20200317205553.GO50174@kduck.mit.edu>
In-Reply-To: <20200317205553.GO50174@kduck.mit.edu>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
authentication-results: spf=none (sender IP is ) smtp.mailfrom=rwilton@cisco.com;
x-originating-ip: [2001:420:c0c0:1006::3c0]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 7f9bee18-5e72-4088-efc9-08d7cc1f31b0
x-ms-traffictypediagnostic: MN2PR11MB4567:
x-ms-exchange-transport-forked: True
x-microsoft-antispam-prvs: =?utf-8?q?=3CMN2PR11MB4567BD4B2D647287CB8994E8B5F?= =?utf-8?q?40=40MN2PR11MB4567=2Enamprd11=2Eprod=2Eoutlook=2Ecom=3E?=
x-ms-oob-tlc-oobclassifiers: OLM:5797;
x-forefront-prvs: 0347410860
x-forefront-antispam-report: SFV:NSPM; =?utf-8?q?SFS=3A=2810009020=29=284636?= =?utf-8?b?MDA5KSgzNjYwMDQpKDM3NjAwMikoMzQ2MDAyKSgzOTg2MDQwMDAwMikoMzk2?= =?utf-8?b?MDAzKSgxMzYwMDMpKDE5OTAwNCkoNjY5NDYwMDcpKDY2NTc0MDEyKSg4NjM2?= =?utf-8?b?MjAwMSkoMzE2MDAyKSg4OTM2MDAyKSg4Njc2MDAyKSgzMzY1NjAwMikoNjUw?= =?utf-8?q?6007=29=287696005=29=2853546011=29=2881156014=29=28110136005=29?= =?utf-8?b?KDgxMTY2MDA2KSg2NjM2MDAyKSgxODYwMDMpKDk2NjAwNSkoNDc4NjAwMDAxKSg3?= =?utf-8?q?1200400001=29=2876116006=29=2830864003=29=2854906003=29=285253601?= =?utf-8?q?4=29=285660300002=29=2815650500001=29=2855016002=29=2866556008=29?= =?utf-8?b?KDI5MDYwMDIpKDQzMjYwMDgpKDk2ODYwMDMpKDY2NDQ2MDA4KSg2NDc1NjAwOCko?= =?utf-8?q?66476007=29=28559001=29=28579004=29=3B?= DIR:OUT; SFP:1101; SCL:1; SRVR:MN2PR11MB4567; H:MN2PR11MB4366.namprd11.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1;
received-spf: None (protection.outlook.com: cisco.com does not designate permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: =?utf-8?q?ddte1gZT6aawclILuRGPgjP11kHcE5B?= =?utf-8?q?mlihYWtSAaNzT/LzquLOHjDFXu3dBers2ZCmOGAzhZ7nVmdZPi0W3Ypi8jfsFzKqS?= =?utf-8?q?J++HxkMYSItF2HDz2gQ9waZf//Xy6BYmvPotDVBY62wTAU+UcnZbyLRO8wItHhhga?= =?utf-8?q?gpCq7da47av15QULRZfXHSosPvBMgbw8XgTxE8yL3JSgYcPgRwHmqVGz+qYNMG/ch?= =?utf-8?q?DJFh+JFSmU2gJR1ZGOFUlJD8IlM2Z5p8JNPI9RJ+sSgY9bCZG6SATq+lCJDTclzas?= =?utf-8?q?nvRQ1FR3FkruQj44o53VPteyOjxnTj6Iiljo1yw/9QH0ZVOC84HhIWzvHnYgG88lC?= =?utf-8?q?H1U5VxaVYCzriFxmhj0KvOfak8E2yNyWwvhrYaesYZDl7VOjRRnGsnE7vwJtZgTKx?= =?utf-8?q?1KkTndjnt3ClvkjjgoVHR9GVPLrM1B5/6GlKN0XasRkuyFedg0/DArofX6wQ3gAsi?= =?utf-8?q?PhJ3Yq5npi910YmjQbYZ4z78HI+0xfhnNTQdx2cJjK6Wbo1A=3D=3D?=
x-ms-exchange-antispam-messagedata: =?utf-8?q?u8mdFCTPPKqJvvF2keh93D2rXkEgkW?= =?utf-8?q?jGwB817pWxPRl1bn6zaand2WFvaIIbXLXQCkWTNUPTgMy1E3p0AtrSU90oYMi+lFH?= =?utf-8?q?kLypBKTTd3d2bIT4sIUZuXaps+EMg3KgOCW0rOK0wvd94op0mJoyuGlHSj3Xce0uY?= =?utf-8?q?igyeliVkPlw=3D?=
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 7f9bee18-5e72-4088-efc9-08d7cc1f31b0
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Mar 2020 16:04:24.4417 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 5ae1af62-9505-4097-a69a-c1553ef7840e
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: =?utf-8?q?eAPaG5zvij9BDHYfFCSOW?= =?utf-8?q?pGBWZ7d33XV0yxYQpYhbwKODH287GBbNQZO7bAh3+VdPV6gk58PK6NHwDgked61Xw?= =?utf-8?q?=3D=3D?=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4567
X-OriginatorOrg: cisco.com
X-Outbound-SMTP-Client: 173.36.7.13, xch-aln-003.cisco.com
X-Outbound-Node: rcdn-core-1.cisco.com
Archived-At: <https://mailarchive.ietf.org/arch/msg/mpls/rc8wcaMvhBNQdRQmR7bI5vvVfN0>
Subject: Re: [mpls] Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]
X-BeenThere: mpls@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Multi-Protocol Label Switching WG <mpls.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/mpls>, <mailto:mpls-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/mpls/>
List-Post: <mailto:mpls@ietf.org>
List-Help: <mailto:mpls-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/mpls>, <mailto:mpls-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 19 Mar 2020 16:04:34 -0000

Hi Ben, Kamran, 

> -----Original Message-----
> From: iesg <iesg-bounces@ietf.org> On Behalf Of Benjamin Kaduk
> Sent: 17 March 2020 20:56
> To: Kamran Raza (skraza) <skraza@cisco.com>
> Cc: mpls@ietf.org; mpls-chairs@ietf.org; Nicolai Leymann
> <n.leymann@telekom.de>de>; The IESG <iesg@ietf.org>rg>; draft-ietf-mpls-ldp-
> yang@ietf.org; Tarek Saad <tsaad.net@gmail.com>
> Subject: Re: Updated rev -08 posted [Re: Benjamin Kaduk's Discuss on
> draft-ietf-mpls-ldp-yang-07: (with DISCUSS and COMMENT)]
> 
> On Fri, Feb 28, 2020 at 04:39:14AM +0000, Kamran Raza (skraza) wrote:
> > Thanks for your detailed review, Ben.
> > We have posted a new rev -08 that takes care almost all of your comments
> and comments from other IESG reviewers.
> > On behalf of authors, Please see inline [skraza] for each of your
> comment/Q:
> >
> > 
> >     --------------------------------------------------------------------
> --
> >     DISCUSS:
> >
> > ----------------------------------------------------------------------
> >
> >     The YANG doctor last call review of the -06 notes that these modules
> are
> >     in violation of the expectations of RFC 8349 (at least w.r.t.
> defining a
> >     new identity for the control-plane protocol and possibly more).
> This
> >     document should either be brought into compliance or explain why it
> >     diverges.
> >
> > [skraza]: Fixed.
> >
> >     The YANG doctor's comments about default values for "local"
> >     configuration (e.g., graceful-restart configuration) causing the
> global
> >     configuration to never take effect should also be addressed.
> >
> > [skraza]: Fixed.
> 
> These both look addressed, to my eye, but I am very much not a YANG
> expert.
> I'd recommend trying to get a dedicated YANG review, perhaps from the
> incoming Management AD Rob Wilton.
[RW] 
I've checked the graceful-restart configuration and it looks fine. 

Is there further hierarchical configuration that needs to be checked?

> 
> >     Please include some justification for why LDP IPv6 is considered an
> >     "extended feature" (which is particularly surprising given that
> Section
> >     2 classifies "IP" to refer to both IPv4 and IPv6 together).
> >
> > [skraza]:  This was an early decision taken and presented to WG and WG
> had agreed.
> > Moreover, Sec 1.1 does points out why LDP IPv6 is considered as an
> extended feature. The "base" features are the one most commonly
> implemented and deployed features.
> > The idea was that base must not contain any if-feature - given that
> LDPv6 is not widely implemented and was branded as if-feature, it is
> classified in the extended category then.
> > BTW, note that LDP has separate RFC for LDP IPv6.
> 
> It's not really clear to me that just because the protocol is described in
> separate RFCs for IPv4 and IPv6 we cannot present a unified management
> interface on top of it.  I note that several other ADs expressed concern
> about this topic, so I've moved this document onto the agenda for this
> week's IESG telechat to get a better sense on what the right thing to do
> is here.
[RW] 
I note that IPv6 isn't under if-feature in the extension module.  This would imply that implementations that wish to implement optional features in the extension module must also support LDP IPv6 (or deviate).  Is that intentional?


> 
> >
> >     We need to define the format/semantics of the md5-key string (e.g.,
> is
> >     it hex? base64{url,}?) either directly or by reference (as the YANG
> >     doctor notes).  Using a crypto-type would probably be appropriate,
> as
> >     would adding a note that tcp-md5 is obsoleted by TCP-AO.
> >
> > [skraza]: It is beyond our control as we are using it as-is from the
> RFC. Made the change anyways to add crypto-algo to widen the scope/type of
> key beyond MD5.
> 
> I'm still concerned that there isn't enough text here for me to be able to
> implement something and have confidence it will interoperate.  For
> example, the recommendations in RFC 3562 include the ability to use a
> direct binary key for TCP-md5.  Does this YANG stanza allow for doing so?
> If yes, what mechanism is used for converting input into "raw" binary, if
> needed?
> 
> That is, even if the actual protocol semantics for how TCP-MD5 uses a
> given key/password are out of your control, there is still ambiguity as to
> what mapping (if any) occurs between the YANG-configured value and what is
> passed to the TCP-MD5 implementation.
> 
> Furthermore, the authentication-type still only has a "string" leaf for
> the key, with (if I am reading the YANG correctly) the recommended key-
> chain functionality for protected key storage being left as an extension.
> It's not clear to me why we should be leaving the best-practice
> formulation as an extension.
> 
> >     In the peer-af-policy-container grouping's
> >     label-policy/advertise/prefix-list, we need to say if the filter is
> for
> >     inclusion or exclusion of outgoing label advertisements.
> >     Similarly for the incoming label advertisements in the 'accept'
> >     container (and most of the <foo>-list-ref usages?).
> >
> >     In the policy-container grouping's
> >     label-policy/assign/independent-mode/prefix-list, the description
> >     suggests that the contents will provide not just a list of prefixes
> that
> >     act as a filter, but also a map from prefix to label.  This is a
> >     qualitatively different usage than the previous occurrences of
> >     prefix-list-ref, and it seems like a different type may be needed
> for
> >     it.  Similarly for ordered-mode.
> >
> > [skraza]: Fixed as per latest changes that use rtgwg policy model
> constructs.
> 
> Ah, this took me a little more work to find, since the changes were in the
> prefix-list-ref typedef.  Thanks.
> 
> >     (pro-forma) This document has 6 authors, and per RFC 7322, as this
> is
> >     more than five, we are supposed to consider whether that's
> appropriate.
> >     Seeing nothing in the shepherd writeup or similar, I mention it
> here.
> >
> > [skraza]: We had taken an advise from Loa (MPLS WG chair) and would like
> to keep it to 6.
> 
> Sure; this was presumed to just be "pro forma" but I put it in since the
> shepherd writeup didn't mention it specifically.
> 
> >     --------------------------------------------------------------------
> --
> >     COMMENT:
> >
> > ----------------------------------------------------------------------
> >
> >     Please also respond to the other YANG doctor, rtg-dir, and other
> >     directorate reviewers' comments.
> >
> >     I support Roman's Discuss.
> >
> >     I suggest making a reference to RFC 5920 at some point, perhaps from
> the
> >     security considerations.
> > [skraza]: Done.
> >
> >     We have several instances of a "container ipv4" that's a "container
> with
> >     presence" in the jargon of RFC 7950.  As SEC AD one thing I look for
> is
> >     edge cases, such as when a subset of these containers are present
> but
> >     not all (and not none).  While for this specific case, it seems
> fairly
> >     natural to assume that IPv4 is enabled if any are present, it does
> make
> >     me wonder if the modeling is at an optimal state if there's even the
> >     possibility for such skewed signals.  (I didn't specifically audit
> for
> >     any other cases of "redundant" containers-with-presence.)
> >
> > [skraza]: This is the same structure as RFC 8344. The edge cases are OK.
> The ipv4/ipv6 container may contain none, one, or more nodes in the
> container. ipv4/ipv6 address family is enabled as long as the container is
> “present”. Your assumption is correct, and the modeling (and the protocol
> as well) is in a good state, even with such possibilities. The signals
> should be clear enough.
[RW] 

Intuitively, I don't like the combination of a presence container + enable leaf below it.  I've asked Martin (author of RFC 8344) why the construct is used in this way, since he must have had a good reason.  It isn't immediately obvious to me why what is written isn't a convoluted way of making it a non-presence container.

Regards,
Rob


> >
> >     It's not entirely clear to me why we need (all of) the
> specialization of
> >     structures into using inet:ipv4-address and inet:ipv6-address as
> opposed
> >     to just inet:ip-address.  I do see that the latter is used in
> several
> >     places already, and so assume that this was already considered;
> perhaps
> >     some of the discussion/motivation could be included in the document,
> >     though.
> >
> > [skraza]: When the type of an attribute is for a specific address family
> (ipv4 or ipv6), we use the address family specific type (inet:ipv4-address
> or inet:ipv6-address), so that user cannot provide a value of the wrong
> format.  inet:ip-address is simply a union of net:ipv4-address and
> inet:ipv6-address, and is used when the address family is not restricted.
> This is particular convenient when the user makes a query on a particular
> peer when the user does not have a particular address family in mind. The
> system will provide the address in a proper format.
> >
> >     Section 1
> >
> >        The data model is defined for following constructs that are used
> for
> >        managing the protocol:
> >
> >     nit: "the following".
> > [skraza]: Fixed.
> >
> >     I share the YANG doctor's concern that separating the configuration
> and
> >     operational state constructs is not consistent with the NMDA model.
> > [skraza]: Fixed.
> >
> >     Section 1.1
> >
> >        The "base" category contains the basic and fundamental features
> that
> >        are covered in LDP base specification [RFC5036] and constitute
> the
> >        minumum requirements for a typical base LDP deployment.  Whereas,
> the
> >        "extended" category contains all other non-base features.  All
> > the
> >
> >     This statement ("all other") is not future-proof.
> > [skraza]:  Removed the "all".
> >
> >     Section 3
> >
> >        o  This module aims to address only the core LDP parameters as
> per
> >           RFC specification, as well as some widely deployed non-RFC
> >           features (such as label policies, session authentication etc).
> >           Any vendor specific feature should be defined in a vendor-
> specific
> >           augmentation of this model.
> >
> >     [Even for widely-deployed non-RFC features we still need to provide
> a
> >     clear description of what semantics are being covered, whether
> >     in-document or via an external reference.]
> >
> > [skraza]: The only non-RFC config was related to label policies -
> Reworked/enhanced the text.
> >
> >        o  This model is a VPN Forwarding and Routing (VRF)-centric
> model.
> >           It is important to note that [RFC4364] defines VRF tables
> > and
> >
> >     side note: I would like the IETF to someday get to a point where
> "VPN"
> >     (the 'P' is for "private") is used only for protocols/deployments
> that
> >     provide confidentiality protection ("privacy") for the data being
> >     conveyed.  We are, of course, not there now, and I cannot insist on
> any
> >     change here.  But please consider whether an alternate phrasing
> would
> >     suffice, that includes just the "virtual" part but not "VPN".  Since
> we
> >     are mostly using "VRF" in the rest of the document, this is not as
> >     daunting as it sometimes is...
> >
> >     Section 4
> >
> >     It might be nice to have the tree show the ipv4 and ipv6 children in
> >     as similar an order as is possible (with the understanding that the
> >     semantics are not affected by the order of appearance, of course).
> >
> >            |  +--rw address-families
> >            |  |  +--rw ipv4!
> >            |  |  |  +--rw enable?                           boolean
> >            |  |  |  +--ro label-distribution-controlmode?   enumeration
> >            |  |  |  +--ro bindings
> >            |  |  |  |  +--ro address* [address]
> >            |  |  |  |  |  +--ro address               inet:ipv4-address
> >            |  |  |  |  |  +--ro advertisement-type?   advertised-
> received
> >            |  |  |  |  |  +--ro peer
> >            |  |  |  |  |     +--ro lsr-id?           leafref
> >            |  |  |  |  |     +--ro label-space-id?   leafref
> >
> >     Does this imply that there is only one peer per address?  Is that
> going
> >     to be a limitation for any deployment cases?
> >
> > [skraza]: Not an issue. Note that a peer may have one or more bound
> addresses.
> >
> >               +--rw ldp-ext:session-downstream-on-demand
> >               |       {session-downstream-on-demand-config}?
> >               |  +--rw ldp-ext:enable?      boolean
> >
> >     nit(?): missing '|' to connect down to ldp-ext:enable?
> >     Similarly for ldp-ext:max-wait a few lines later, and some other
> >     occurrences later in the document.
> >
> >
> > [skraza]: Some of these instances/typos were due to hand picked sub-
> tree.
> > Not an issue anymore as we are printing the entire tree as generated by
> the tool.
> >
> >
> >     Section 5.1.2
> >
> >     Am I reading this right that (e.g.) per-interface hello-holdtime is
> >     configurable as an extension and only the global hello-holdtime is
> >     present in the "base" module?
> >
> > [skraza]: Yes.
> >
> >     Section 7
> >
> >     "NHLFE" is used only once in this document (and is not listed as
> >     "well-known" at
> >     https://www.rfc-editor.org/materials/abbrev.expansion.txt); please
> >     expand it out.
> >
> > [skraza]: It is well known and listed in the above url. Please x-chk.
> 
> To be clear, the linked content uses the convention of an "*" marking to
> indicate that the expansion is "well-known" and does not need to be
> provided in a given document.  The entry for "Next Hop Label Forwarding
> Entry" does not include such a marking; perhaps the responsible AD should
> request it to be added, if it is indeed well-known?
> 
> >     Section 9
> >
> >     Is 2018 still the right copyright year for the modules themselves?
> >
> >
> > [skraza]: 2020-02-25 rev now.
> >
> >
> >     Section 9.1
> >
> >          typedef ldp-address-family {
> >            type identityref {
> >              base rt:address-family;
> >            }
> >
> >     Why can't we use rt:address-family directly in the one container
> that
> >     uses this?
> >
> > [skraza]: Fixed now.
> >
> >            description
> >              "Duration represented as 32 bit seconds with infinite.";
> >
> >     nits: "32-bit" (hyphenated), "with infinity".
> > [skraza]:  Fixed.
> >
> >          typedef downstream-upstream {
> >            type enumeration {
> >              enum downstream {
> >                description "Downstream information.";
> >              }
> >              enum upstream {
> >                description "Upstream information.";
> >              }
> >            }
> >            description
> >              "Received or advertised.";
> >          }
> >
> >     nit: copy/paste on the description?
> > [skraza]:  Fixed.
> >
> >              leaf used-in-forwarding {
> >                type boolean;
> >                description
> >                  "'true' if the lable is used in forwarding.";
> >
> >     nit: s/lable/label/
> > [skraza]:  Fixed.
> >
> >          grouping graceful-restart-attributes-per-peer {
> >            description
> >              "Per peer graceful restart attributes.
> >               On the local side, these attributes are configuration and
> >               operational state data. One the peer side, these
> attributes
> >               are operational state data reveiced from the peer.";
> >
> >     nit: s/reveiced/received/
> > [skraza]:  Fixed.
> >
> >          grouping peer-state-derived {
> >            description
> >              "The peer state information derived from the LDP protocol
> >               operatoins.";
> >
> >     nit: s/operatoins/operations/
> > [skraza]:  Fixed.
> >
> >          grouping peer-state-derived {
> >          [...]
> >            leaf next-keep-alive {
> >              type uint16;
> >              units seconds;
> >              config false;
> >              description "Time to send the next KeepAlive message.";
> >            }
> >
> >     nit: this description text sounds like it's giving an absolute time,
> but
> >     given that it's a uint16 I presume it's "time [from now] until
> sending".
> > [skraza]:  Clarified.
> >
> >            container received-peer-state {
> >              config false;
> >              description
> >                "Operational state information learned from the peer.";
> >
> >              uses graceful-restart-attributes-per-peer;
> >
> >              container capability {
> >                description "Configure capability.";
> >
> >     This is config-false; "Configure" in the description cannot be
> correct,
> >     though I suppose "Configured" could be.
> >     (Also applies to several of the child containers' descriptions.)
> >
> > [skraza]:  Fixed all these.
> >
> >            leaf up-time {
> >              type string;
> >              config false;
> >              description "Up time. The interval format in ISO 8601.";
> >
> >     We probably should have some sort of reference for ISO 8601 (though
> of
> >     course it cannot actually *be* a <xref> in the YANG module itself)
> >
> > [skraza]:  Changed to use timeticks64 now.
> >
> >            leaf notification {
> >              type yang:counter64;
> >              description
> >                "The number of messages sent or received.";
> >
> >     Is this the same as or different than "leaf total-messages"?
> > [skraza]:  Fixed.
> >
> >                    leaf label-distribution-controlmode {
> >
> >     nit: any reason to not hyphenate control-mode as well?
> > [skraza]:  Fixed.
> >
> >                container interfaces {
> >                  description
> >                    "A list of interfaces for LDP Basic Descovery.";
> >
> >     nit: s/Descovery/Discovery/
> > [skraza]:  Fixed.
> >
> >              container discovery {
> >                description
> >                  "Neibgbor discovery configuration and operational
> > state.";
> >
> >     nit: s/Neibgbor/Neighbor/
> > [skraza]:  Fixed.
> >
> >                        // ipv4
> >                        container hello-adjacencies {
> >                          config false;
> >                          description
> >                            "Containing a list of hello adjacencies.";
> >
> >                          list hello-adjacency {
> >                            key "adjacent-address";
> >                            config false;
> >                            description "List of hello adjacencies.";
> >
> >                            leaf adjacent-address {
> >                              type inet:ipv4-address;
> >                              description
> >                                "Neighbor address of the hello
> adjacency.";
> >                            }
> >
> >     I'm not an expert here, but I'm not really seeing why we need to
> >     specialize this container for v4 vs v6 as opposed to just using
> >     inet:ip-address and having something that's applicable to both.
> >
> > [skraza]: We could. There are pros and cons either way. The reasons to
> use separate containers in this model are:
> > 1) Operators usually plan and configure IPv4 and IPv6 separately, rather
> than mixing the address family together. The separate containers provide
> more focus and clear separations.
> > 2) Often times, there are attributes with different types for different
> address families. Mixing two together brings up some modeling difficulties
> to specifies differently for different address families.
> > 3) Sometimes, there are attributes that are applicable to one address
> family but not the other. Mixing the two containers together would require
> some difficult conditions, which would complicate the model and make the
> model less usable.
> > 4) Mixing the two containers can make the YANG file more compact, but
> not the configuration data and operational state data.
> > 5) The existing RFCs use the style of separate containers, such as RFC
> 8344 and RFC 8349.
> 
> Thanks for writing this out; it helps me understand the situation better.
> 
> >                        leaf adjacent-address {
> >                          type inet:ipv4-address;
> >                          description
> >                            "Configures a remote LDP neighbor and enables
> >                             extended LDP discovery of the specified
> >                             neighbor.";
> >
> >     Given that there's a sibling leaf "enable", do we still want "and
> >     enables" here?
> > [skraza]: We don’t need “enables” here. Removed it.
> >
> >
> >     Should peer/capability have a comment in its description that the
> >     container exists to be augmented by other (extension) modules?
> >
> > [skraza]: Yes. It is better. Added as suggested.
> >
> >          notification mpls-ldp-fec-event {
> >
> >     Am I reading this right that we only generate notifications on 0->1
> and
> >     1->0 transitions of "number of prefixes active for a given FEC"?
> Would
> >     we ever want to know about addition/removal of prefixes that do
> leave
> >     the FEC up?
> >
> > [skraza]: Here, the prefix is the FEC. Addition/removal of a prefix is
> equivalent to the addition/removal of the FEC. Changed the leaf name from
> prefix to fec to avoid the confusion.
> >
> >     Section 9.2
> >
> >     I can't find a pattern into which if-feature statements go into
> >     containers used to augment things vs. the augment directives
> themselves.
> >     Can we be more consistent (or am I missing the pattern)?
> >
> > [skraza]: It is not intended to set up a strict pattern between feature
> name and containers, while the purpose is to provide more descriptive
> feature names. That said, the parent container names are often used to
> provide the contexts for the feature name. e.g. In the case of “dual-
> stack-transport-pereference”, it seems that the parent container name
> “peers” is also beneficial, so we have the feature name “dual-stack-
> transport-preference” to “peers-dual-stack-transport-preference” to make
> the feature name fit better with the pattern and more descriptive.
> 
> Thanks for the explanation
> 
> >          feature dual-stack-transport-pereference {
> >            description
> >              "This feature indicates that the system allows to configure
> >               the transport connection pereference in a dual-stack
> > setup.";
> >
> >     nit: s/pereference/preference/ (twice)
> > [skraza]: Fixed.
> >
> >          feature policy-targeted-discovery-config {
> >            description
> >              "This feature indicates that the system allows to configure
> >               policies to control the acceptance of targeted neighbor
> >               discovery hello messages.";
> >
> >     nit: s/hello/Hello/
> >     [skraza]: Fixed.
> >
> >          typedef neighbor-list-ref {
> >            type string;
> >            description
> >              "A type for a reference to a neighbor address list.
> >               The string value is the name identifier for uniquely
> >               identifying the referenced address list, which contains a
> list
> >               of addresses that a routing policy can applied. The
> definition
> >               of such an address list is outside the scope of this
> >               document.";
> >
> >     If I'm reading correctly between here and where this is used, the
> >     semantics of this are better described as "implementation-specific"
> --
> >     that is, this is a label that refers to some local configured state,
> >     specifically, a list of neighbor addresses used as an ACL.
> (Emphasis on
> >     *local* -- it seems like it does not need to interact with anything
> >     else.)
> >     I do sympathize with the YANG doctor, though -- as-is, this feels
> >     underspecified.
> >     (BTW, IP ACLs are not considered to be a security measure by the
> broader
> >     security community.)
> >     Similarly for the subsequent <foo>-list-refs.
> >
> > [skraza]: We do understand the concerns, and have fixed these two types
> by using the https://tools.ietf.org/html/draft-ietf-rtgwg-policy-model,
> which was not available when we initially started to work on this model.
> 
> Great!
> 
> >              container interfaces {
> >                description
> >                  "A list of interfaces on which forwarding is
> > disabled.";
> >
> >     nit: given the separate 'ldp-disable' leaf, perhaps s/is/can be/ is
> in
> >     order.
> > [skraza]: Understand the confusion. Changed as suggested.
> >
> >     Also, the description of the list itself implies additional
> >     functionality, of listing the interfaces used for basic discovery.
> >
> > [skraza]: The description was wrong. We don’t intend to use this list
> for discovery. Fixed.
> >
> >          grouping graceful-restart-augment {
> >            description "Augmentation to graceful restart.";
> >
> >            leaf helper-enable {
> >              if-feature graceful-restart-helper-mode;
> >              type boolean;
> >              default false;
> >              description
> >                "Enable or disable graceful restart helper mode.";
> >
> >     Where is this "helper mode" documented?
> > [skraza]: This is not an RFC defined term but commonly known and used
> when GR timer is set to 0.
> >   Fixed the description in yang to highlight this.
> 
> Is this just staged for the -09?  I am not seeing it in the -08.
> 
> >          augment "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
> >            + "ldp:global" {
> >            description "Graceful forwarding nexthop augmentation.";
> >            uses global-forwarding-nexthop-augment;
> >          }
> >
> >     nit: I don't think "Graceful" is correct, in the description.
> >     [skraza]: Removed it.
> >
> >     Why is (the augmented)
> >     /rt:routing/rt:control-plane-protocols/ldp:mpls-
> ldp/ldp:global/ldp:address-families/ipv6/label-distribution-controlmode
> >     'config false'?  Doesn't RFC 5036 say that an LSR might support
> either
> >     as a configuration option?
> >
> > [skraza]: Yes, but no implementation supports such a cfg as change of
> such a mode is a massive disruptive operation - all implementation pick
> one mode as the default and only supported mode and stick to it.
> >   The LDP YANG design team had discussed this early enough and agreed to
> not provide this a cfg option.
> 
> Ah, understood.
> 
> >                leaf adjacent-address {
> >                  type inet:ipv6-address;
> >                  description
> >                    "Configures a remote LDP neighbor and enables
> >                     extended LDP discovery of the specified
> >                     neighbor.";
> >
> >     As for the ipv4 case, do we still want "and enables" here?
> > [skraza]: Fixed by removing it.
> >
> >          augment "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
> >            + "ldp:discovery/ldp:interfaces/ldp:interface/"
> >            + "ldp:address-families/ldp-ext:ipv6" {
> >            description "Interface address family IPv6 augmentation.";
> >            uses interface-address-family-ipv6-augment;
> >
> >     Didn't we just create .../ldp:address-families/ldp-ext:ipv6 with an
> >     earlier augmentation in this module?  (Similarly for
> >     /rt:routing/rt:control-plane-protocols/ldp:mpls-
> ldp/ldp:peers/ldp:peer/ldp:address-families/ldp-ext:ipv6
> >     which we augment with "uses peer-af-policy-container".)
> >
> > [skraza]: Yes. It is ok to augment a node which is part of another
> augmentation. The result is to add them all together. Doing so is to avoid
> too much irregular patterns, so that we have more similar construction
> between IPv4 and IPv6.
> >
> >     Appendix A
> >
> >                      "is-router": [null],
> >
> >     This looks weird to me ("array containing one element with value
> literal
> >     null") -- why does the array come into play?
> >
> >     [Xufeng]: Here is a little explanation to the seemingly confusing
> notation: the pair of square brackets does not indicate a normal array.
> Instead, it is a special notation to indicate that the value of a “empty”
> type leaf is “present” (Section 6.9. of RFC 7951). Semantically the line
> above says that “(the neighbor) is a router.”
> 
> Ah, thanks for the explanations.
> 
> -Ben