[mpls] mldp was Re: Yangdoctors last call review of draft-ietf-mpls-ldp-yang-06

tom petch <ietfc@btconnect.com> Tue, 15 October 2019 16:38 UTC

Return-Path: <ietfc@btconnect.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 D2E5212081B; Tue, 15 Oct 2019 09:38:41 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 0.247
X-Spam-Level:
X-Spam-Status: No, score=0.247 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RATWARE_MS_HASH=2.148, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001] autolearn=no autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=btconnect.onmicrosoft.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 uA1MKV3XNecX; Tue, 15 Oct 2019 09:38:39 -0700 (PDT)
Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-eopbgr20129.outbound.protection.outlook.com [40.107.2.129]) (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 918551201C6; Tue, 15 Oct 2019 09:38:38 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OJDlM22u2QDhUzI+V1sqMVWrMhrmXIWoDjqPJjFh7wKH5s0FUMvTUOS9sYI5jrvjGeqCY30VwnxstV5pWVjeAsu26g2hosoED5mVJfMrOg8cG7u4Q5+k/Cc8PR0XVVi9fkxaH2Uro45YQGmCur9rEv+01LgMwSKGbNAKU9fz8pNvdAjsd234WXwwumLTbZX51LPz1SKty7XtngAtRB5Lk4GBLwVl4whIcAh4IIS0q/ODBEUkCdg05ZgW3ln2RBY8cur5V12k21OXmoZCcjDESizQIvhEpURFbifXoirHkoBoQGEJsYETMMWevWmWniit6dqY/Gr2behD7HVFg4OMMw==
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=R+/Wb8xRvoPfrwQM03hlzeSD8XFSOz8Z9qq0+/6o5dY=; b=ck+8HfGZHkGE+pIAfxUx/QNy2AwLA1xQpKfylM/0LNiyfYooQ+AoXGeInTweGG9GKgyGL4yyYdkdDwrZK6Glb/fJ7AxP4sTi0c1l+LfAGDFib79WAtIbGrau3rLGHVOKtXi08QiwJpK3BDShw3ZV18p/QEol6d1OTSDYaIa2Z2ohS9M8PnzgWIQosQQ2S1hXk91ei+8WR8MWY+zFIdka/82n1C3bTzljhdYq0+2GYOsedXDhwqMqEd15WFCsROVrNOCqC0w6soy26N61m/sEPd1nKjPK/pJYgHomPgEP+HbePGRCdAWGk3ayKQhDHcgjem9H4WYkZiQk0c5J2uRyNQ==
ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=btconnect.com; dmarc=pass action=none header.from=btconnect.com; dkim=pass header.d=btconnect.com; arc=none
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=btconnect.onmicrosoft.com; s=selector2-btconnect-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=R+/Wb8xRvoPfrwQM03hlzeSD8XFSOz8Z9qq0+/6o5dY=; b=SgQmLDinWUiwUS+81mCvnYBjXim0ICNXGxxip5g7e5P9r0dRshikQTRlZjBiiQ1uhq2NuBUWFS/DrqoOObIRg3fxBsqVo8mLbSbS5ayQpgZ5VN/8qmobNWj6EzTseMtJMVmS9bzoOKurICFl/ChbfxsdPa8N672q2D0gu5sC4eA=
Received: from DB7PR07MB5147.eurprd07.prod.outlook.com (20.178.42.32) by DB7PR07MB4732.eurprd07.prod.outlook.com (52.135.136.20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2367.14; Tue, 15 Oct 2019 16:38:36 +0000
Received: from DB7PR07MB5147.eurprd07.prod.outlook.com ([fe80::d5a9:784f:d667:ef14]) by DB7PR07MB5147.eurprd07.prod.outlook.com ([fe80::d5a9:784f:d667:ef14%4]) with mapi id 15.20.2347.023; Tue, 15 Oct 2019 16:38:35 +0000
From: tom petch <ietfc@btconnect.com>
To: Jan Lindblad <janl@tail-f.com>
CC: "mpls@ietf.org" <mpls@ietf.org>, "draft-ietf-mpls-ldp-yang.all@ietf.org" <draft-ietf-mpls-ldp-yang.all@ietf.org>
Thread-Topic: mldp was Re: [mpls] Yangdoctors last call review of draft-ietf-mpls-ldp-yang-06
Thread-Index: AQHVg3b9Qta8xAnglUm0npj1f/Artg==
Date: Tue, 15 Oct 2019 16:38:35 +0000
Message-ID: <011501d58376$aa19e120$4001a8c0@gateway.2wire.net>
References: <157114122559.18000.6531210525259761076@ietfa.amsl.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-clientproxiedby: LO2P123CA0045.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600::33) To DB7PR07MB5147.eurprd07.prod.outlook.com (2603:10a6:10:68::32)
authentication-results: spf=none (sender IP is ) smtp.mailfrom=ietfc@btconnect.com;
x-ms-exchange-messagesentrepresentingtype: 1
x-mailer: Microsoft Outlook Express 6.00.2800.1106
x-originating-ip: [86.139.211.103]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: efcfe4a2-58e5-4b6e-6e51-08d7518e1fa9
x-ms-traffictypediagnostic: DB7PR07MB4732:
x-ms-exchange-purlcount: 1
x-microsoft-antispam-prvs: <DB7PR07MB47323688CBF3A1E7EFFA5F65A0930@DB7PR07MB4732.eurprd07.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:10000;
x-forefront-prvs: 01917B1794
x-forefront-antispam-report: SFV:NSPM; SFS:(10019020)(39860400002)(136003)(366004)(376002)(346002)(396003)(13464003)(189003)(199004)(446003)(52116002)(305945005)(7736002)(966005)(3846002)(6116002)(71200400001)(102836004)(81686011)(6506007)(76176011)(81816011)(386003)(61296003)(62236002)(44716002)(186003)(54906003)(71190400001)(14454004)(316002)(478600001)(26005)(476003)(14496001)(486006)(99286004)(2906002)(6486002)(44736005)(66066001)(6436002)(8936002)(1556002)(50226002)(81166006)(8676002)(6512007)(25786009)(4326008)(6306002)(9686003)(81156014)(5660300002)(14444005)(66446008)(64756008)(66946007)(66556008)(6916009)(66476007)(4720700003)(256004)(86362001)(98474003)(74416001)(7726001); DIR:OUT; SFP:1102; SCL:1; SRVR:DB7PR07MB4732; H:DB7PR07MB5147.eurprd07.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:0;
received-spf: None (protection.outlook.com: btconnect.com does not designate permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: PImUdKwMMggRad/7PA5ZxraHdEy83uxskoRW5HM+PsGRafTCymNmRqCuG3zYEnG7TrtCXexbQxhd+PengVM3ygYfcZmlRs1lH7M5QaYzPMBatOGO6mybidfahpvQJUGChA17ReiME/X88BczNa9reKH+jL3kNnV72A/X7HYNpVJbrSWRailHF9q2LrwI1+S0gJU/CgroQM4CyfLF53aRGCk8C0JLkSif79cr/oas929zogwd04166jwaucbm/QhcY09U2s1WvDAAkWkJradN2N2QjT2COpsE2RAvZjAE1gIXLmM0dGc6xpxLM0s33h89Kc7p7/sTRxxIZUXiWOvd+3DrnaEOTT6LCYkuHSWFZ5L2cxN1AWfdYq6FcWlupITVd47kO+SlOn+kbH12twGRYdPVnMMwIRjVUKwOlaJ8ulttUqgYRnwRtw4bX6pQOPH4bQFWLaFAEMXNwyStQQggbQ==
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <AC025C7C6100DA40B6DB0E3916C87A8C@eurprd07.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: btconnect.com
X-MS-Exchange-CrossTenant-Network-Message-Id: efcfe4a2-58e5-4b6e-6e51-08d7518e1fa9
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Oct 2019 16:38:35.7996 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: cf8853ed-96e5-465b-9185-806bfe185e30
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: KDcPl1vlxcQzxtG0sybhixnyfBmoluaXWyIQZHSk0L7FXb+LwdcpPfs7xUMSo/i6TqzRsUQ1Hrmrgm20bqU4mg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR07MB4732
Archived-At: <https://mailarchive.ietf.org/arch/msg/mpls/pHP20eOUP8Z9Gg5_d_OlkLlj69Q>
Subject: [mpls] mldp was Re: Yangdoctors last call review of draft-ietf-mpls-ldp-yang-06
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: Tue, 15 Oct 2019 16:38:47 -0000

Jan

It would likely help if you could also look at
draft-ietf-mpls-mldp-yang-06
which augments
draft-ietf-mpls-ldp-yang-06
with such as
[widescreen needed]
module: ietf-mpls-mldp
  augment
/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/ldp:global/ldp:capab
ility:
and
     augment "/rt:routing/rt:control-plane-protocols/"
       + "ldp:mpls-ldp/ldp:peers/ldp:peer/ldp:received-peer-state/"
       + "ldp:capability/mldp:mldp" {
i.e. there are another 20 or so augment which are affected by the
changes you propose.

Note that the prefixes are ldp: and mldp: so the identities for the
control plane protocols are probably better as those and not mpls-ldp or
mpls-mldp.  But is mldp a separate protocol in RFC8349 terms or just
part of ldp? I do not know.

The mldp draft has just completed WGLC. I looked at it but gave up on it
because the line lengths were too long to display on my screen.

Tom Petch


----- Original Message -----
From: "Jan Lindblad via Datatracker" <noreply@ietf.org>;
To: <yang-doctors@ietf.org>;
Cc: <mpls@ietf.org>;; <draft-ietf-mpls-ldp-yang.all@ietf.org>;;
<ietf@ietf.org>;
Sent: Tuesday, October 15, 2019 1:07 PM
Subject: [mpls] Yangdoctors last call review of
draft-ietf-mpls-ldp-yang-06


> Reviewer: Jan Lindblad
> Review result: Ready with Issues
>
> Modules: ietf-mpls-ldp.yang, ietf-mpls-ldp-extended.yang
> Review result: Ready with Issues
> Reviewer: Jan Lindblad
>
> This is my YANG doctor LC review of draft-ietf-mpls-ldp-yang-06. I
find the
> module ready with issues, even if there are a couple rather
fundamental things
> to work out. It is evident that the authors have spent considerable
time to
> make the module clean and tidy, which is why I can say it's more or
less ready
> even when there are fundamentals to work out. In my judgement it may
be
> possible to work out all remaining issues rather quickly.
>
> #1) Fitting mpls-ldp into the ietf-routing framework
>
> The modules at hand, ietf-mpls-ldp.yang and
ietf-mpls-ldp-extended.yang, are
> augmenting into the ietf-routing module (RFC 8022). I find this highly
> appropriate. RFC 8022 has a section on how control plane protocols are
meant to
> fit into the ietf-routing framework:
>
> 5.3.2.  Defining New Control-Plane Protocols
>
>    It is expected that future YANG modules will create data models for
>    additional control-plane protocol types.  Such a new module has to
>    define the protocol-specific configuration and state data, and it
has
>    to integrate it into the core routing framework in the following
way:
>
>    o  A new identity MUST be defined for the control-plane protocol,
and
>       its base identity MUST be set to "rt:control-plane-protocol" or
to
>       an identity derived from "rt:control-plane-protocol".
> ....
>    o  Configuration parameters and/or state data for the new protocol
>       can be defined by augmenting the "control-plane-protocol" data
>       node under both "/routing" and "/routing-state".
>
> The current draft is doing neither of the above two points. IMHO, it
should do
> both. Or if there is strong reason not to do that, incorporate a
statement from
> the ietf-routing authors and a clear explanation of why the mpls-ldp
related
> modules would be exempted. At present, this deviation from the RFC
8022
> architecture is never mentioned in the draft.
>
> Even if this issue falls under fundamental issues, the way I look at
it, it
> would be quite easy to fix. Adjusting the two draft modules to comply
with the
> points above requires changes to two dozen augment statement paths and
a
> handful of leafref paths. Adding the identity and corresponding when
derived
> from statement are a couple of lines.
>
> In principle, this change opens up for multiple instances of mpls-ldp.
This
> might be useful if a system is used with two entirely separate
networks (such
> as two separate customers), where each one might be running an
mpls-ldp
> instance. If this is not desired, a must statement could be introduced
to limit
> the number of instances to one.
>
> #2) Global level vs. local (e.g. interface or peer) level
>
> The module has several configurations that have a global and local
level, such
> as configurations per peer or per interface. Presumably, these local
level
> configurations are meant to override the global default when set and
let the
> global default prevail when nothing specific is specified at the local
level.
>
> The way this is being currently implemented in the modules, however,
makes it
> impossible not to set a value at the local level, so the local setting
will
> always prevail. The global configuration would have no effect at all
(except in
> those cases where a local level configuration is marked with an
if-feature and
> the server would not announce that feature).
>
> For example:
>
> On the global level, there is a graceful-restart/enable leaf:
>
>       container global {
> ....
>         container graceful-restart {
>           leaf enable {
>             type boolean;
>             default false;
>           }
>
> Further down, we have another graceful-restart/enable leaf, used per
peer:
>
>   grouping graceful-restart-attributes-per-peer {
> ....
>     container graceful-restart {
>       leaf enable {
>         type boolean;
>         default false;
>       }
>
> I would assume this latter enable leaf should take precedence over the
global
> graceful-restart when set on the peer. But if not set on the peer, the
global
> graceful-restart value should be used. Since the peer
graceful-restart/enable
> leaf has a default value, however, it will never happen that it is not
set, and
> will thus render the global value completely irrelevant for all peers.
>
> The fix is simple: Do not have a default statement on the local leafs.
Explain
> in the description statement what happens when the element has no
value (the
> globally configured value will be used). Make sure the global leaf has
a
> default (they all do already).
>
> The same applies to a rather long list of leafs in the module. I don't
dare
> listing them here, because I would surely miss a few. My point is that
the
> global/local relationships needs to be identified, local defaults
removed. It
> would also make sense to mention how this is supposed to work in the
> descriptions.
>
> #3) Unlinked multikey leafrefs
>
> The grouping ldp-peer-ref has two leafrefs, one for each key in the
list of ldp
> peers. Two leafrefs are exactly what you need to reference an entry in
a list
> with two keys, so that's good. It is not enough to simply have two
unlinked
> leafrefs, however. That would still allow them to have values that are
valid
> separately, but not point to any entry in the list combined. If you
have a copy
> of "Network Programmability with YANG", this is explained in detail on
pp.
> 451-454.
>
> To link the leafrefs, you'd need to update the path expression in the
> label-space-id leaf as follows:
>
>   grouping ldp-peer-ref {
>     description
>       "An absolute reference to an LDP peer, by the LDP ID, which
>        consists of the LSR ID and the Label Space ID.";
>
>     leaf lsr-id {
>       type leafref {
>         path "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
>           + "ldp:peers/ldp:peer/ldp:lsr-id";
>       }
>       description
>         "The LSR ID of the peer, as a portion of the peer LDP ID.";
>     }
>     leaf label-space-id {
>       type leafref {
>         path "/rt:routing/rt:control-plane-protocols/ldp:mpls-ldp/"
>           + "ldp:peers/ldp:peer[lsr-id =
>           current()/../lsr-id]/ldp:label-space-id";
>       }
>
> The paths above would also have to be adjusted as part of fixing issue
#1
> above. If this fixing results in allowing multiple mpls-ldp instances,
the
> grouping above needs to be extended with one additional, linked
leafref that
> points out which mpls-ldp instance is being targeted. Or if that is
always the
> same instance as the reference is coming from, a reformulation of the
paths
> above could ensure no mpls-ldp instance name would be required. In
that case,
> the paths should be relative and never venture outside the mpls-ldp
container.
> I would be happy to explain this further.
>
> #4) Weakly defined types for neighbor-list-ref, prefix-list-ref,
peer-list-ref
>
> These types are not leafrefs, but strings without any YANG
substatements to
> define the format. The only thing the description does is to claim
that the
> entities they refer to are outside the scope of this document. For an
> operator/programmer encountering this type, that isn't very helpful,
and is not
> going to be interoperable.
>
>   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.";
>   }
>
> I'm not sure if this is fixable by sharpening the YANG module, but
maybe more
> could be done to guide a confused reader. What would the user do to
find out
> the format of this type and valid values? Add to the description.
>
> #5) Interfaces or mpls, who's on top?
>
> In the mpls-ldp modules, there are two interface lists and two
leafrefs
> pointing to interfaces. This makes me wonder if some of the YANG
structure
> would belong more naturally as an augment to ietf-interfaces instead?
I mean,
> it doesn't sound fun to define the same interfaces in a lot of
different
> places, just because a protocol needs some additional information.
This is just
> a casual observation from an mpls-innocent guy, so you can disregard
if you
> feel it's irrelevant.
>
> #6) MD5 key
>
> There is a leaf md5-key of type string. Is this leaf sensitive from a
security
> point of view? A plaintext string would not be ideal if that is the
case.
> Choose a crypto type instead. Also, the format of this string is not
defined.
> Hex without spaces? Explain the format in the description.
>
> ><
>
> _______________________________________________
> mpls mailing list
> mpls@ietf.org
> https://www.ietf.org/mailman/listinfo/mpls