Re: [Lsr] AD Review for draft-ietf-ospf-yang-21

"Acee Lindem (acee)" <acee@cisco.com> Sat, 22 June 2019 23:04 UTC

Return-Path: <acee@cisco.com>
X-Original-To: lsr@ietfa.amsl.com
Delivered-To: lsr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id AECF0120130; Sat, 22 Jun 2019 16:04:01 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.501
X-Spam-Level:
X-Spam-Status: No, score=-14.501 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_HI=-5, SPF_PASS=-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=SRmlvt2I; dkim=pass (1024-bit key) header.d=cisco.onmicrosoft.com header.b=lMmDTV9U
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 cuPnbXqTIW7f; Sat, 22 Jun 2019 16:03:58 -0700 (PDT)
Received: from rcdn-iport-2.cisco.com (rcdn-iport-2.cisco.com [173.37.86.73]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 8C98D12018B; Sat, 22 Jun 2019 16:03:58 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=14362; q=dns/txt; s=iport; t=1561244638; x=1562454238; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-id:content-transfer-encoding: mime-version; bh=qKe3Sj27QqLPUmTqd37+lJzY4vR4FawIsgpl3HCEeC8=; b=SRmlvt2ICC6Hrb7sSIQPXSyv1GAYEc/GP9LNNKDrVLjX3RcyipUhjWWj AcVkP5nG83ANL2hTNUmv7spPincfYRgMihq9bDkw1JMzzb07NwcC/Kj85 3PRs7DeFcxpT3UR1Y5EPFQjZ4RQD4iEquIpdg+Hm4dbhBZ3weyDFicCHT Y=;
IronPort-PHdr: 9a23:ZOGNhh/LXflMgP9uRHGN82YQeigqvan1NQcJ650hzqhDabmn44+/YR7E/fs4iljPUM2b8P9Ch+fM+4HYEW0bqdfJq3UeaNpJXh4Bh98RmlkpC8OIIUb6N/XtKSc9GZcKWQ==
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: A0ChAADdsg5d/5BdJa1kHAEBAQQBAQcEAQGBVQUBAQsBgUNQA4E/IAQLKIQWg0cDjmCDWYhHjXOBLhSBEANUCQEBAQwBAS0CAQGEQAIXgkcjNgcOAQMBAQQBAQIBBW2KNwyFSwIBAxIRBA0MAQE3AQ8CAQgOBAgCIwMCAgIfERQBAg4CBAENBSKDAIFrAx0BmSkCgTiIX3F+M4J5AQEFhHkNC4IRCYEMKAGLXReBf4EQAScfghc1PoIagWYMIAEXgwoygiaLbkSCHIUciDKNMj8JAoIUj3SDbhuCKIcNhAqKCI0miRyKHoNJAgQCBAUCDgEBBYFXBSyBWHAVOyoBgkGBEYEwDBeDTYpTcoEpjnoBAQ
X-IronPort-AV: E=Sophos;i="5.63,406,1557187200"; d="scan'208";a="583070772"
Received: from rcdn-core-8.cisco.com ([173.37.93.144]) by rcdn-iport-2.cisco.com with ESMTP/TLS/DHE-RSA-SEED-SHA; 22 Jun 2019 23:03:57 +0000
Received: from XCH-RCD-007.cisco.com (xch-rcd-007.cisco.com [173.37.102.17]) by rcdn-core-8.cisco.com (8.15.2/8.15.2) with ESMTPS id x5MN3vMg006082 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=FAIL); Sat, 22 Jun 2019 23:03:57 GMT
Received: from xhs-rtp-002.cisco.com (64.101.210.229) by XCH-RCD-007.cisco.com (173.37.102.17) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Sat, 22 Jun 2019 18:03:56 -0500
Received: from xhs-rtp-002.cisco.com (64.101.210.229) by xhs-rtp-002.cisco.com (64.101.210.229) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Sat, 22 Jun 2019 19:03:55 -0400
Received: from NAM05-CO1-obe.outbound.protection.outlook.com (64.101.32.56) by xhs-rtp-002.cisco.com (64.101.210.229) with Microsoft SMTP Server (TLS) id 15.0.1473.3 via Frontend Transport; Sat, 22 Jun 2019 19:03:55 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cisco.onmicrosoft.com; s=selector2-cisco-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=qKe3Sj27QqLPUmTqd37+lJzY4vR4FawIsgpl3HCEeC8=; b=lMmDTV9Ur/+vXoKwiQmLAGtZk97QkcTyqe85OArMo75pgBsBZm42KFGE77fD+QtpPjb0fxJTdubk4ZQNWFiROG3BpaBLpdi8exiRk0FWdVXwHHH5E54UIXaNyvsPTgyWO3jdtgpAswHP4ZFJv691piuCqwaJ5qJIXZHZ2x/Xjyg=
Received: from MWHPR11MB1902.namprd11.prod.outlook.com (10.175.53.139) by MWHPR11MB1821.namprd11.prod.outlook.com (10.175.53.136) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2008.13; Sat, 22 Jun 2019 23:03:53 +0000
Received: from MWHPR11MB1902.namprd11.prod.outlook.com ([fe80::f1d4:41cf:84d6:ff73]) by MWHPR11MB1902.namprd11.prod.outlook.com ([fe80::f1d4:41cf:84d6:ff73%2]) with mapi id 15.20.2008.014; Sat, 22 Jun 2019 23:03:53 +0000
From: "Acee Lindem (acee)" <acee@cisco.com>
To: Alvaro Retana <aretana.ietf@gmail.com>, "draft-ietf-ospf-yang@ietf.org" <draft-ietf-ospf-yang@ietf.org>
CC: "stephane.litkowski@orange.com" <stephane.litkowski@orange.com>, "lsr-chairs@ietf.org" <lsr-chairs@ietf.org>, "lsr@ietf.org" <lsr@ietf.org>
Thread-Topic: AD Review for draft-ietf-ospf-yang-21
Thread-Index: AQHVG+281sKnENvLf0eo2GqtHHjXvKaoIpoA
Date: Sat, 22 Jun 2019 23:03:53 +0000
Message-ID: <77F1A67E-2EB8-453E-8E89-70C55A820E03@cisco.com>
References: <CAMMESsztO1a4fnT2Gx2GDKcYVLtWS52WZ=HmPdQ9VFqSEtvG7Q@mail.gmail.com>
In-Reply-To: <CAMMESsztO1a4fnT2Gx2GDKcYVLtWS52WZ=HmPdQ9VFqSEtvG7Q@mail.gmail.com>
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=acee@cisco.com;
x-originating-ip: [2001:420:c0c8:1008::842]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 572f978d-146b-4756-e047-08d6f765e5b7
x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600148)(711020)(4605104)(1401327)(2017052603328)(7193020); SRVR:MWHPR11MB1821;
x-ms-traffictypediagnostic: MWHPR11MB1821:
x-microsoft-antispam-prvs: <MWHPR11MB1821ED3210D6A264EAA42197C2E60@MWHPR11MB1821.namprd11.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:1265;
x-forefront-prvs: 0076F48C8A
x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(366004)(376002)(39860400002)(136003)(346002)(396003)(199004)(189003)(51914003)(186003)(316002)(2501003)(33656002)(5660300002)(99286004)(102836004)(478600001)(76176011)(8936002)(6506007)(66556008)(25786009)(81156014)(64756008)(66446008)(66476007)(81166006)(8676002)(36756003)(86362001)(71190400001)(71200400001)(54906003)(14444005)(5024004)(110136005)(4326008)(53936002)(14454004)(76116006)(91956017)(486006)(73956011)(11346002)(305945005)(66946007)(256004)(7736002)(46003)(6512007)(68736007)(6246003)(66574012)(2906002)(6486002)(476003)(446003)(6436002)(229853002)(2616005)(6116002); DIR:OUT; SFP:1101; SCL:1; SRVR:MWHPR11MB1821; H:MWHPR11MB1902.namprd11.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1;
received-spf: None (protection.outlook.com: cisco.com does not designate permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam-message-info: q6k/n3l1KWPxOpvCZ1qc30LMYrvDZWT1toiu3yk/ym3LZNU3nOyjimtRM5FGAp65gd30+3B+5m0cXOEFkmFZnSOjE5YJLSNyrA24jmRIwrBJ6dklfA860CchLNkoLc6U0J/oVVP+Dwiup3ifxf1XXQDJ+AJGvLqy/SZsAvHbPLKczSRjxO384/9TTboD+o/GsdwMuilsEE52pz3/w29BfrcoWngGcX+ZqDkWbouCSQ9rkv61xL2uEzgfaeL7661EiuCx2eOi23CjAyKzZg+1NOKa6Q3yqnDbiTALVr1+jUrCejmX80zBEi3MbrA82lFxJATI5rTjoDMjsZur+xTkJBWH/jVdMUNTKVEJw1tzg0SWPkvs3Ic0jP0sqAbCgE7tep36F9Hv9Kr+LAyvfNOcZkUCXsZjgwP+YaSKLYkvzKw=
Content-Type: text/plain; charset="utf-8"
Content-ID: <493A5DE311B66F44B8D49A532C5E5D77@namprd11.prod.outlook.com>
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 572f978d-146b-4756-e047-08d6f765e5b7
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Jun 2019 23:03:53.3170 (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: acee@cisco.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB1821
X-OriginatorOrg: cisco.com
X-Outbound-SMTP-Client: 173.37.102.17, xch-rcd-007.cisco.com
X-Outbound-Node: rcdn-core-8.cisco.com
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/bIUyF1yUzWOPpX_1S0KUWkY0eYI>
Subject: Re: [Lsr] AD Review for draft-ietf-ospf-yang-21
X-BeenThere: lsr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Routing Working Group <lsr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsr>, <mailto:lsr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsr/>
List-Post: <mailto:lsr@ietf.org>
List-Help: <mailto:lsr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsr>, <mailto:lsr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sat, 22 Jun 2019 23:04:02 -0000

Hey Alvaro, 

Thanks for the review - we will address your comments in the -22 version. See inline. 


On 6/5/19, 6:26 PM, "Alvaro Retana" <aretana.ietf@gmail.com> wrote:

    Dear authors:
    
    I just finished reading this document.  I have some comments in-line,
    including a couple that I consider major -- I'll wait for whose to be
    addressed before moving forward.
    
    The datatracker reports 4 errors in the model, mostly related to
    ietf-bfd-types.  Please take a look.
    
    Thanks!
    
    Alvaro.
    
    
    [Line numbers from id-nits.]
    
    ...
    81 1.  Overview
    ...
    92   This document defines a YANG data model that can be used to configure
    93   and manage OSPF and it is an augmentation to the core routing data
    94   model.  If fully conforms to the Network Management Datastore
    95   Architecture (NDMA) [RFC8342].  A core routing data model is defined
    96   in [RFC8349], and it provides the basis for the development of data
    97   models for routing protocols.  The interface data model is defined in
    98   [RFC8343] and is used for referencing interfaces from the routing
    99   protocol.  The key-chain data model used for OSPF authentication is
    100   defined in [RFC8177] and provides both a reference to configured key-
    101   chains and an enumeration of cryptographic algorithms.
    
    [nit] s/If fully conforms/It fully conforms

Fixed. 
    
    
    103   Both OSPFv2 [RFC2328] and OSPFv3 [RFC5340] are supported.  In
    104   addition to the core OSPF protocol, features described in other OSPF
    105   RFCs are also supported.  These includes demand circuit [RFC1793],
    106   traffic engineering [RFC3630], multiple address family [RFC5838],
    107   graceful restart [RFC3623] [RFC5187], NSSA [RFC3101], and OSPF(v3) as
    108   a PE-CE Protocol [RFC4577], [RFC6565].  These non-core features are
    109   optional in the OSPF data model.
    
    [nit] s/OSPF(v3) as a PE-CE Protocol/use as a PE-CE Protocol    Using
    "OSPF(v3)" is confusing because it is not clear whether OSPFv3-only is
    meant or both, but v2 is not mentioned...

Sure "OSPFv2 or OSPFv3" 
    
    
    ...
    138 2.1.  OSPF Operational State
    
    140   The OSPF operational state is included in the same tree as OSPF
    141   configuration consistent with Network Management Datastore
    142   Architecture [RFC8342].  Consequently, only the routing container in
    143   the ietf-routing model [RFC8349] is augmented.  The routing-state
    144   container is not augmented.
    
    [nit] s/consistent with Network/consistent with the Network

Fixed
    
    
    146 2.2.  Overview
    ...
    154   module: ietf-ospf
    155     augment /rt:routing/rt:control-plane-protocols/
    156              rt:control-plane-protocol:
    157       +--rw ospf
    158             .
    159             .
    160          +--rw operation-mode?          identityref
    161          +--rw af?                      identityref
    162             .
    163             .
    164          +--rw areas
    165          |  +--rw area* [area-id]
    166          |     +--rw area-id                   area-id-type
    167          |        .
    168          |        .
    169          |     +--rw virtual-links
    170          |     |  +--rw virtual-link* [transit-area-id router-id]
    171          |     |     .
    172          |     |     .
    173          |     +--rw sham-links {pe-ce-protocol}?
    174          |     |  +--rw sham-link* [local-id remote-id]
    175          |     |     .
    176          |     |     .
    177          |     +--rw interfaces
    178          |        +--rw interface* [name]
    179          |           .
    180          |           .
    181          +--rw topologies {multi-topology}?
    182             +--rw topology* [name]
    183                .
    184                .
    
    186   The ospf module is intended to match to the vendor specific OSPF
    187   configuration construct that is identified by the local identifier
    188   'name'.  The field 'version' allows support for OSPFv2 and OSPFv3.
    
    [minor] The 'version' field doesn't appear in the tree above.  §2.3 says
    the same thing with more details.  Perhaps take the sentence out of this
    paragraph.

Yeah - I removed the sentence. I was thinking of adding it to the tree but it is not there is the full tree. 
    
    
    190   The ospf container includes one OSPF protocol engine instance.  The
    191   instance includes OSPF router level configuration and operational
    192   state.
    
    [??] One of the things that is confusing me is the instance concept in the
    model.  I understand instances in OSPF, and I see how the module is
    organized around instances ("grouping instance-..."), including even
    per-instance Router-ID.  But I don't see anywhere the definition of an
    instance-id (except attached to an interface (§2.7) and specific to OSPFv3
    (§3))...if wanting to configure (or read from) a specific instance, how
    does the client differentiate?  [YANG is not my thing, so this may be
    obvious to everyone else.  If it is, a quick pointer would be very
    appreciated.]

I have removed all instances of "engine" from the text and model. Also, I've just added:

The ietf-ospf model defines a single instance of OSPF which may be instantiated as an OSPFv2 or OSPFv3 instance. Multiple instances are instantiated as multiple control-plane protocols instances.

At one time, the model did include multiple instances. However, this was the manifestation of a poor implementation choice for a particular implementation. 
    
    
    ...
    209 2.4.  Optional Features
    ...
    221   3.   explicit-router-id: Support explicit per-instance Router-ID
    222        specification.
    
    [minor] Is there a reference?  Should the document point to rfc5340/rfc6549
    here?

No - explicit protocol instance router-id configuration is not standardized. Apparently, there are implementations that don't allow protocol instance configuration but rely on global router-id explicit configuration. We got input from implementations beyond the affiliations of the authors. 
    
    
    ...
    234   8.   ttl-security: Support OSPF Time to Live (TTL) security check
    235        suppression [RFC5082].
    
    [minor] s/suppression/support    Right?

Right - fixed. 
    
    237   9.   nsr: Support OSPF Non-Stop Routing (NSR).
    
    [minor] Reference?

No IETF reference for OSPF NSR. However, I have a few patents in this area - should the model reference these? __

    
    ...
    242   11.  admin-control: Support Administrative control of the protocol
    243        state.
    
    [minor] This seems like a "basic" feature: enabling/disabling the
    protocol.  Why was it decided to make it optional?  Are there
    implementations that don't support enabling/disabling?

I'm happy to remove it. 
    
    
    ...
    261   17.  ospfv2-authentication-trailer: Support OSPFv2 Authentication
    262        trailer as specified in [RFC5709] or [RFC7166].
    
    [minor] s/RFC7166/RFC7474

Yup. Fixed. 
    
    ...
    267   19.  ospfv3-authentication-trailer: Support OSPFv3 Authentication
    268        trailer as specified in [RFC7474].
    
    [minor] s/RFC7474/RFC7166

How could I missed this... Fixed
    
    
    ...
    300 2.5.  OSPF Router Configuration/Operational State
    ...
    308   module: ietf-ospf
    ...
    336          +--rw enable?               boolean {admin-control}?
    
    [nit] Perhaps the admin-control should be moved closer to the start of this
    branch so it is clear what is being enabled/disabled.

Ok
    
    
    ...
    464 2.6.  OSPF Area Configuration/Operational State
    ...
    570          |     |     +--rw hello-interval?       uint16
    571          |     |     +--rw dead-interval?        uint32
    572          |     |     +--rw retransmit-interval?  uint16
    
    [minor] Why aren't rt-types:timer-value-* used for the *-timer/interval
    definitions?  I would have thought this was exactly the reason for the
    definitions in rfc8294.

I'd like to use these types universally. However, YANG seems to have a restriction that I can't further restrict the range. This really sucks. Will do what I can and make the best trade-off. 
    
    
    ...
    1054 2.9.  OSPF RPC Operations
    ...
    1061   o  clear-neighbor: restart a particular set of OSPF neighbor.
    
    [minor] Should the description be something like: "reset a set of OSPF
    neighbors on a particular interface"?

Yes. Fixed. 
    
    1063     rpcs:
    1064       +---x clear-neighbor
    1065       |  +---w input
    1066       |     +---w routing-protocol-name
    1067       |     +     -> /rt:routing/control-plane-protocols/
    1068       |     +         control-plane-protocol/name
    1069       |     +---w interface?               if:interface-ref
    
    
    ...
    1076 3.  OSPF YANG Module
    ...
    1143        Editor:   Derek Yeung
    1144                  <mailto:derek@arrcus.com>
    1145        Author:   Acee Lindem
    1146                  <mailto:acee@cisco.com>
    1147        Author:   Yingzhen Qu
    1148                  <mailto:yingzhen.qu@huawei.com>
    1149        Author:   Jeffrey Zhang
    1150                  <mailto:zzhang@juniper.net>
    1151        Author:   Ing-Wher Chen
    1152                  <mailto:ingwherchen@mitre.org>
    1153        Author:   Dean Bogdanovic
    1154                  <mailto:ivandean@gmail.com>
    1155        Author:   Kiran Agrahara Sreenivasa
    1156                  <mailto:kk@employees.org";
    
    [minor] This list is not the same as what is in the front page of this
    document.

Sure. Fixed.
    
    
    ...
    1685     typedef if-state-type {
    1686       type enumeration {
    ...
    1712         enum backup {
    1713           value "6";
    1714           description
    1715             "Interface Backup Designated Router (BDR) state.";
    1716         }
    
    [nit] s/backup/bdr   To match the other instances where a BDR is referenced

Good catch. Fixed. 

Thanks,
Acee